Skip to content

Conversation

@Scylin232
Copy link

Description

Quote from AVRO-4137:

At the moment, the C# library Avro matches Union types with Enum in the following way:

// SpecificDefaultWriter.cs - LN159 - protected override bool Matches(Schema sc, object obj)

case Schema.Type.Enumeration:
   return obj.GetType().IsEnum && (sc as EnumSchema).Symbols.Contains(obj.ToString());

This leads to the scenario where several Enums declared within one Union schema cannot have the same value names, because upon serialization, the Enum that is declared earlier will be chosen, not the one that was originally passed.

It is further suggested to conduct a check on the type name and the schema itself:

case Schema.Type.Enumeration:
  return obj.GetType().IsEnum && (sc as EnumSchema).Symbols.Contains(obj.ToString()) && sc.Name == obj.GetType().Name;

If you believe that this could lead to unforeseen problems (such as breaking backward compatibility), it is proposed to make the check optional, by default turned off. There may be other, more reliable methods to solve this issue, but the demonstrated one seemed the most optimal in my mind 😃

Thank you!

What is the purpose of the change

This pull request introduces SpecificWriter's Enum matching by name, fixing AVRO-4137

Verifying this change

This change is already covered by existing tests.

Documentation

  • Does this pull request introduce a new feature? no

@github-actions github-actions bot added the C# label May 26, 2025
((obj as ISpecificRecord).Schema as RecordSchema).SchemaName.Equals((sc as RecordSchema).SchemaName);
case Schema.Type.Enumeration:
return obj.GetType().IsEnum && (sc as EnumSchema).Symbols.Contains(obj.ToString());
return obj.GetType().IsEnum && (sc as EnumSchema).Symbols.Contains(obj.ToString()) && sc.Name.Equals(obj.GetType().Name);

Check warning

Code scanning / CodeQL

Dereferenced variable may be null Warning

Variable
obj
may be null at this access as suggested by
this
null check.
Variable
obj
may be null at this access as suggested by
this
null check.
((obj as ISpecificRecord).Schema as RecordSchema).SchemaName.Equals((sc as RecordSchema).SchemaName);
case Schema.Type.Enumeration:
return obj.GetType().IsEnum && (sc as EnumSchema).Symbols.Contains(obj.ToString());
return obj.GetType().IsEnum && (sc as EnumSchema).Symbols.Contains(obj.ToString()) && sc.Name.Equals(obj.GetType().Name);

Check warning

Code scanning / CodeQL

Dereferenced variable may be null Warning

Variable
obj
may be null at this access as suggested by
this
null check.
Variable
obj
may be null at this access as suggested by
this
null check.
@martin-g martin-g requested a review from Copilot October 23, 2025 10:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes enum matching in union types by adding name comparison to prevent incorrect schema selection when multiple enums share the same symbol values. The change ensures that when serializing union types containing enums, the correct enum schema is matched based on both symbol membership and type name, not just symbol presence.

Key changes:

  • Enhanced enum matching logic to include name comparison alongside symbol validation
  • Applied the fix consistently across both SpecificWriter and SpecificDatumWriter classes

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
lang/csharp/src/apache/main/Specific/SpecificWriter.cs Added name equality check to enum matching logic in Matches method
lang/csharp/src/apache/main/Specific/SpecificDatumWriter.cs Added name equality check to enum matching logic in UnionBranchMatches method

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

((obj as ISpecificRecord).Schema as RecordSchema).SchemaName.Equals((sc as RecordSchema).SchemaName);
case Schema.Type.Enumeration:
return obj.GetType().IsEnum && (sc as EnumSchema).Symbols.Contains(obj.ToString());
return obj.GetType().IsEnum && (sc as EnumSchema).Symbols.Contains(obj.ToString()) && sc.Name.Equals(obj.GetType().Name);
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

The name comparison may fail for enums in namespaced schemas. The schema's Name property typically contains only the simple name, while obj.GetType().Name returns the C# type name. Consider using SchemaName.Equals() or Fullname for more robust matching, similar to how the Record case on line 210 uses SchemaName.Equals().

Suggested change
return obj.GetType().IsEnum && (sc as EnumSchema).Symbols.Contains(obj.ToString()) && sc.Name.Equals(obj.GetType().Name);
return obj.GetType().IsEnum && (sc as EnumSchema).Symbols.Contains(obj.ToString()) &&
((sc as EnumSchema).SchemaName.Equals(obj.GetType().FullName));

Copilot uses AI. Check for mistakes.
((obj as ISpecificRecord).Schema as RecordSchema).SchemaName.Equals((sc as RecordSchema).SchemaName);
case Schema.Type.Enumeration:
return obj.GetType().IsEnum && (sc as EnumSchema).Symbols.Contains(obj.ToString());
return obj.GetType().IsEnum && (sc as EnumSchema).Symbols.Contains(obj.ToString()) && sc.Name.Equals(obj.GetType().Name);
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

The name comparison may fail for enums in namespaced schemas. The schema's Name property typically contains only the simple name, while obj.GetType().Name returns the C# type name. Consider using SchemaName.Equals() or Fullname for more robust matching, similar to how the Record case on line 140 uses SchemaName.Equals().

Suggested change
return obj.GetType().IsEnum && (sc as EnumSchema).Symbols.Contains(obj.ToString()) && sc.Name.Equals(obj.GetType().Name);
return obj.GetType().IsEnum
&& (sc as EnumSchema).Symbols.Contains(obj.ToString())
&& (sc as EnumSchema).SchemaName.Equals(obj.GetType());

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant