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 6 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>

@martin-g martin-g Mar 20, 2022

Copy link
Copy Markdown
Member

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.

@zcsizmadia zcsizmadia Mar 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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;

@zcsizmadia zcsizmadia Mar 20, 2022

Copy link
Copy Markdown
Contributor Author

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
57 changes: 51 additions & 6 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 @@ -18,6 +18,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;

namespace Avro
{
Expand Down Expand Up @@ -148,14 +149,14 @@ public static int GenProtocol(string infile, string outdir,
try
{
string text = System.IO.File.ReadAllText(infile);

text = ReplaceMappedNamespacesInSchema(text, namespaceMapping);

Protocol protocol = Protocol.Parse(text);

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

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

codegen.GenerateCode();
codegen.WriteTypes(outdir);
}
Expand All @@ -174,14 +175,14 @@ public static int GenSchema(string infile, string outdir,
try
{
string text = System.IO.File.ReadAllText(infile);

text = ReplaceMappedNamespacesInSchema(text, namespaceMapping);

Schema schema = Schema.Parse(text);

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

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

codegen.GenerateCode();
codegen.WriteTypes(outdir);
}
Expand All @@ -193,5 +194,49 @@ public static int GenSchema(string infile, string outdir,

return 0;
}

/// <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}""";
});
}

}
}
13 changes: 1 addition & 12 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 @@ -57,14 +57,6 @@ public class CodeGen
/// </value>
public IList<Protocol> Protocols { get; private set; }

/// <summary>
/// Gets mapping of Avro namespaces to C# namespaces.
/// </summary>
/// <value>
/// The namespace mapping.
/// </value>
public IDictionary<string, string> NamespaceMapping { get; private set; }

@KyleSchoonover KyleSchoonover Mar 21, 2022

Copy link
Copy Markdown
Contributor

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?

@zcsizmadia zcsizmadia Mar 21, 2022

Copy link
Copy Markdown
Contributor Author

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>
/// Gets list of generated namespaces.
/// </summary>
Expand All @@ -80,7 +72,6 @@ public CodeGen()
{
Schemas = new List<Schema>();
Protocols = new List<Protocol>();
NamespaceMapping = new Dictionary<string, string>();
NamespaceLookup = new Dictionary<string, CodeNamespace>(StringComparer.Ordinal);
}

Expand Down Expand Up @@ -129,9 +120,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
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