Skip to content

AVRO-2883: Fix namespace mapping #1610

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
martin-g merged 7 commits into from
Apr 19, 2022
Merged

AVRO-2883: Fix namespace mapping #1610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6326ee3
Remove unused package references
Mar 20, 2022
1187cf8
Replace namespace in text schema
Mar 20, 2022
c434957
Remove namespace mapping
Mar 20, 2022
e3c2804
Add unit tests
Mar 20, 2022
ec47628
Match namespace mapping used in ticket
Mar 20, 2022
916c63f
Make ReplaceMappedNamespacesInSchema private
Mar 20, 2022
a131791
Mark NamespaceMapping obsolete
Mar 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions lang/csharp/src/apache/codegen/Avro.codegen.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,6 @@
<WarningsAsErrors />
</PropertyGroup>

<!-- See lang/csharp/README.md for tool and library dependency update strategy -->
<ItemGroup>
<PackageReference Include="Newtonsoft.Json" Version="$(NewtonsoftJsonVersion)" />
<PackageReference Include="System.CodeDom" Version="$(SystemCodeDomVersion)" />
<PackageReference Include="System.Reflection" Version="$(SystemReflectionVersion)" />
<PackageReference Include="System.Reflection.Emit.ILGeneration" Version="$(SystemReflectionEmitILGenerationVersion)" />
<PackageReference Include="System.Reflection.Emit.Lightweight" Version="$(SystemReflectionEmitLightweightVersion)" />
</ItemGroup>
Copy link
Member

@martin-g martin-g Mar 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not C# developer but this change does not look related to the issue with the namespaces by avrogen.
Please explain.

Copy link
Contributor Author

@zcsizmadia zcsizmadia Mar 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It is not related. I just got a watning that those packages are not used. All the reflection related code is in the main library. I guess there used to be code in avrogen which used relection, however it is not present. I can move it to a seperate ticket easily if needed. I considered this change just like removing unused using WhateverPackage;

Copy link
Contributor Author

@zcsizmadia zcsizmadia Mar 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw when I do sneaky stuff like this, usually create a specific commit for it and explain somewaht in the commit mnessage. 6326ee3


<ItemGroup>
<ProjectReference Include="..\main\Avro.main.csproj" />
</ItemGroup>
Expand Down
13 changes: 2 additions & 11 deletions lang/csharp/src/apache/codegen/AvroGen.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,9 @@ public static int GenProtocol(string infile, string outdir,
try
{
string text = System.IO.File.ReadAllText(infile);
Protocol protocol = Protocol.Parse(text);

CodeGen codegen = new CodeGen();
codegen.AddProtocol(protocol);

foreach (var entry in namespaceMapping)
codegen.NamespaceMapping[entry.Key] = entry.Value;
codegen.AddProtocol(text, namespaceMapping);

codegen.GenerateCode();
codegen.WriteTypes(outdir);
Expand All @@ -174,13 +170,8 @@ public static int GenSchema(string infile, string outdir,
try
{
string text = System.IO.File.ReadAllText(infile);
Schema schema = Schema.Parse(text);

CodeGen codegen = new CodeGen();
codegen.AddSchema(schema);

foreach (var entry in namespaceMapping)
codegen.NamespaceMapping[entry.Key] = entry.Value;
codegen.AddSchema(text, namespaceMapping);

codegen.GenerateCode();
codegen.WriteTypes(outdir);
Expand Down
76 changes: 72 additions & 4 deletions lang/csharp/src/apache/main/CodeGen/CodeGen.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
using System.Linq;
using System.Reflection;
using System.Text;
using System.Text.RegularExpressions;
using Microsoft.CSharp;

namespace Avro
Expand Down Expand Up @@ -63,6 +64,7 @@ public class CodeGen
/// <value>
/// The namespace mapping.
/// </value>
[Obsolete("NamespaceMapping is not used, use AddProtocol(string ...) or AddSchema(string ...) instead!")]
public IDictionary<string, string> NamespaceMapping { get; private set; }
Copy link
Contributor

@KyleSchoonover KyleSchoonover Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this potentially API breaking to remove this?

Copy link
Contributor Author

@zcsizmadia zcsizmadia Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a good call out. I reshuffled the code and added back the NamespaceMapping property and marked it obsolete.
Thebiggest issue with this serious bug is that the best solution is to change the schema text before the actual parsing, otherwise the whole already created Schema or Protocol object must be recreated. The documentation might need mapping. So there might be some actual breaking for some people who generate code on the fly and not via avrogen, however using a dynamically compiled code is not very likely since using the generated SpecificRecord is not very convinient.


/// <summary>
Expand All @@ -80,7 +82,6 @@ public CodeGen()
{
Schemas = new List<Schema>();
Protocols = new List<Protocol>();
NamespaceMapping = new Dictionary<string, string>();
NamespaceLookup = new Dictionary<string, CodeNamespace>(StringComparer.Ordinal);
}

Expand All @@ -103,6 +104,19 @@ public virtual void AddProtocol(Protocol protocol)
Protocols.Add(protocol);
}

/// <summary>
/// Parses and adds a protocol object to generate code for.
/// </summary>
/// <param name="protocolText">The protocol.</param>
/// <param name="namespaceMapping">namespace mapping key value pairs.</param>
public virtual void AddProtocol(string protocolText, IEnumerable<KeyValuePair<string, string>> namespaceMapping = null)
{
// Map namespaces
protocolText = ReplaceMappedNamespacesInSchema(protocolText, namespaceMapping);
Protocol protocol = Protocol.Parse(protocolText);
Protocols.Add(protocol);
}

/// <summary>
/// Adds a schema object to generate code for.
/// </summary>
Expand All @@ -112,6 +126,19 @@ public virtual void AddSchema(Schema schema)
Schemas.Add(schema);
}

/// <summary>
/// Parses and adds a schema object to generate code for.
/// </summary>
/// <param name="schemaText">schema object.</param>
/// <param name="namespaceMapping">namespace mapping key value pairs.</param>
public virtual void AddSchema(string schemaText, IEnumerable<KeyValuePair<string, string>> namespaceMapping = null)
{
// Map namespaces
schemaText = ReplaceMappedNamespacesInSchema(schemaText, namespaceMapping);
Schema schema = Schema.Parse(schemaText);
Schemas.Add(schema);
}

/// <summary>
/// Adds a namespace object for the given name into the dictionary if it doesn't exist yet.
/// </summary>
Expand All @@ -129,9 +156,7 @@ protected virtual CodeNamespace AddNamespace(string name)

if (!NamespaceLookup.TryGetValue(name, out CodeNamespace ns))
{
ns = NamespaceMapping.TryGetValue(name, out string csharpNamespace)
? new CodeNamespace(csharpNamespace)
: new CodeNamespace(CodeGenUtil.Instance.Mangle(name));
ns = new CodeNamespace(CodeGenUtil.Instance.Mangle(name));

foreach (CodeNamespaceImport nci in CodeGenUtil.Instance.NamespaceImports)
{
Expand Down Expand Up @@ -1153,5 +1178,48 @@ public virtual void WriteTypes(string outputdir)
}
}
}

/// <summary>
/// Replace namespace(s) in schema or protocol definition.
/// </summary>
/// <param name="input">input schema or protocol definition.</param>
/// <param name="namespaceMapping">namespace mappings object.</param>
private static string ReplaceMappedNamespacesInSchema(string input, IEnumerable<KeyValuePair<string, string>> namespaceMapping)
{
if (namespaceMapping == null || input == null)
return input;

// Replace namespace in "namespace" definitions:
// "namespace": "originalnamespace" -> "namespace": "mappednamespace"
// "namespace": "originalnamespace.whatever" -> "namespace": "mappednamespace.whatever"
// Note: It keeps the original whitespaces
return Regex.Replace(input, @"""namespace""(\s*):(\s*)""([^""]*)""", m =>
{
// m.Groups[1]: whitespaces before ':'
// m.Groups[2]: whitespaces after ':'
// m.Groups[3]: the namespace

string ns = m.Groups[3].Value;

foreach (var mapping in namespaceMapping)
{
// Full match
if (mapping.Key == ns)
{
ns = mapping.Value;
break;
}
else
// Partial match
if (ns.StartsWith($"{mapping.Key}."))
{
ns = $"{mapping.Value}.{ns.Substring(mapping.Key.Length + 1)}";
break;
}
}

return $@"""namespace""{m.Groups[1].Value}:{m.Groups[2].Value}""{ns}""";
});
}
}
}
125 changes: 96 additions & 29 deletions lang/csharp/src/apache/test/AvroGen/AvroGenTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
using System.Reflection;
using System.Collections.Generic;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.Emit;
using NUnit.Framework;
using Avro.Specific;

Expand Down Expand Up @@ -263,6 +261,51 @@ class AvroGenTests
]
}";

// https://issues.apache.org/jira/browse/AVRO-2883
private const string _schema_avro_2883 = @"
{
""type"" : ""record"",
""name"" : ""TestModel"",
""namespace"" : ""my.avro.ns"",
""fields"" : [ {
""name"" : ""eventType"",
""type"" : {
""type"" : ""enum"",
""name"" : ""EventType"",
""symbols"" : [ ""CREATE"", ""UPDATE"", ""DELETE"" ]
}
} ]
}";

// https://issues.apache.org/jira/browse/AVRO-3046
private const string _schema_avro_3046 = @"
{
""type"": ""record"",
""name"": ""ExampleRecord"",
""namespace"": ""com.example"",
""fields"": [
{
""name"": ""Id"",
""type"": ""string"",
""logicalType"": ""UUID""
},
{
""name"": ""InnerRecord"",
""type"": {
""type"": ""record"",
""name"": ""InnerRecord"",
""fields"": [
{
""name"": ""Id"",
""type"": ""string"",
""logicalType"": ""UUID""
}
]
}
}
]
}";

private Assembly TestSchema(
string schema,
IEnumerable<string> typeNamesToCheck = null,
Expand Down Expand Up @@ -412,6 +455,18 @@ private Assembly TestSchema(
{
"org/apache/avro/codegentest/testdata/NullableLogicalTypesArray.cs"
})]
[TestCase(
_schema_avro_2883,
new string[]
{
"my.avro.ns.TestModel",
"my.avro.ns.EventType",
},
new string[]
{
"my/avro/ns/TestModel.cs",
"my/avro/ns/EventType.cs"
})]
public void GenerateSchema(string schema, IEnumerable<string> typeNamesToCheck, IEnumerable<string> generatedFilesToCheck)
{
TestSchema(schema, typeNamesToCheck, generatedFilesToCheck: generatedFilesToCheck);
Expand All @@ -428,6 +483,45 @@ public void GenerateSchema(string schema, IEnumerable<string> typeNamesToCheck,
{
"org/apache/csharp/codegentest/testdata/NullableLogicalTypesArray.cs"
})]
[TestCase(
_nestedLogicalTypesUnion,
"org.apache.avro.codegentest.testdata", "org.apache.csharp.codegentest.testdata",
new string[]
{
"org.apache.csharp.codegentest.testdata.NestedLogicalTypesUnion",
"org.apache.csharp.codegentest.testdata.RecordInUnion"
},
new string[]
{
"org/apache/csharp/codegentest/testdata/NestedLogicalTypesUnion.cs",
"org/apache/csharp/codegentest/testdata/RecordInUnion.cs"
})]
[TestCase(
_schema_avro_2883,
"my.avro.ns", "my.csharp.ns",
new string[]
{
"my.csharp.ns.TestModel",
"my.csharp.ns.EventType",
},
new string[]
{
"my/csharp/ns/TestModel.cs",
"my/csharp/ns/EventType.cs"
})]
[TestCase(
_schema_avro_3046,
"com.example", "Example",
new string[]
{
"Example.ExampleRecord",
"Example.InnerRecord",
},
new string[]
{
"Example/ExampleRecord.cs",
"Example/InnerRecord.cs"
})]
[TestCase(
_nullableLogicalTypesArray,
"org.apache.avro.codegentest.testdata", "org.apache.@return.@int", // Reserved keywords in namespace
Expand Down Expand Up @@ -492,33 +586,6 @@ public void GenerateSchemaWithNamespaceMapping(
TestSchema(schema, typeNamesToCheck, new Dictionary<string, string> { { namespaceMappingFrom, namespaceMappingTo } }, generatedFilesToCheck);
}

[TestCase(
_nestedLogicalTypesUnion,
"org.apache.avro.codegentest.testdata", "org.apache.csharp.codegentest.testdata",
new string[]
{
"org.apache.avro.codegentest.testdata.NestedLogicalTypesUnion",
"org.apache.avro.codegentest.testdata.RecordInUnion"
},
new string[]
{
"org/apache/csharp/codegentest/testdata/NestedLogicalTypesUnion.cs",
"org/apache/csharp/codegentest/testdata/RecordInUnion.cs"
})]
public void GenerateSchemaWithNamespaceMapping_Bug_AVRO_2883(
string schema,
string namespaceMappingFrom,
string namespaceMappingTo,
IEnumerable<string> typeNamesToCheck,
IEnumerable<string> generatedFilesToCheck)
{
// !!! This is a bug which must be fixed
// !!! Once it is fixed, this test will fail and this test can be removed
// https://issues.apache.org/jira/browse/AVRO-2883
// https://issues.apache.org/jira/browse/AVRO-3046
Assert.Throws<AssertionException>(() => TestSchema(schema, typeNamesToCheck, new Dictionary<string, string> { { namespaceMappingFrom, namespaceMappingTo } }, generatedFilesToCheck));
}

[TestCase(_logicalTypesWithCustomConversion, typeof(AvroTypeException))]
[TestCase(_customConversionWithLogicalTypes, typeof(SchemaParseException))]
[TestCase(_nestedLogicalTypesUnionFixedDecimal, typeof(SchemaParseException))]
Expand Down
2 changes: 1 addition & 1 deletion lang/csharp/src/apache/test/CodGen/CodeGenTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public void TestReservedKeywords()
[TestCase("a.b.int", "a.b.@int")]
[TestCase("int.long.while", "@int.@long.@while")] // Reserved keywords
[TestCase("a.value.partial", "a.value.partial")] // Contextual keywords
[TestCase("a.value.b.int.c.while.longpartial", "[email protected][email protected]")] // Rseserved and contextual keywords
[TestCase("a.value.b.int.c.while.longpartial", "[email protected][email protected]")] // Reserved and contextual keywords
public void TestMangleUnMangle(string input, string mangled)
{
// Mangle
Expand Down