AVRO-4094: [C#] Updating mapped namespaces referenced in types #3247
Conversation
Co-authored-by: Paul Podgorsek <[email protected]>
Co-authored-by: Paul Podgorsek <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| if (token is JObject obj) | ||
| { | ||
| if (obj.ContainsKey("type")) |
Check notice
Code scanning / CodeQL
Inefficient use of ContainsKey Note
| { | ||
| return mapping.Value; | ||
| } | ||
| else if (originalNamespace.StartsWith($"{mapping.Key}.")) |
There was a problem hiding this comment.
This calls String.StartsWith(String), which uses StringComparison.CurrentCulture; but should instead use StringComparison.Ordinal.
With StringComparison.CurrentCulture, it is possible that originalNamespace.StartsWith($"{mapping.Key}.") returns true but originalNamespace.Length < $"{mapping.Key}.".Length, which would then cause the subsequent Substring call to throw. This would happen if mapping.Key includes characters that are ignored in linguistic comparisons.
There was a problem hiding this comment.
Great point- I've updated this to use Ordinal comparison now
| // Recurse into all properties of the object | ||
| foreach (var property in obj.Properties()) | ||
| { | ||
| UpdateNamespacesInJToken(property.Value, namespaceMapping); | ||
| } |
There was a problem hiding this comment.
This will replace namespace-looking strings in "type" properties even within the "default" property of a field. Those should not be modified because they affect the meaning of the schema, rather than only the local code generation.
There was a problem hiding this comment.
I've added a list of properties which shouldn't be considered for the namespace mapping such as default/ doc/ symbols and a few others- However I'd disagree on type - this is a property which could have a namespace and I've an example of this in the test I included as part of this PR
Changing string comparison in ReplaceNamespace to use Ordinal string comparison Adding blacklist for unqualified props which we shouldn't be looking to do any namespace replaces on
What is the purpose of the change
Fixing AVRO-4094 - The use of namespace mappings currently updates namespaces, but not types that reference the namespace in the type name. This PR resolves that.
Due to the complexity of the type attribute, being either simple strings, arrays or complex types- potentially even nested combinations of the above, I use newtonsoft to build the object and recursively walk properties as opposed to the regex approach used in the namespace replacement. This feels a far more maintainable approach given this level of potential complexity.
Verifying this change
This change added tests and can be verified as follows:
Documentation