AVRO-4312: [csharp] Validate names against the Avro name grammar#3890
Open
iemejia wants to merge 3 commits into
Open
AVRO-4312: [csharp] Validate names against the Avro name grammar#3890iemejia wants to merge 3 commits into
iemejia wants to merge 3 commits into
Conversation
Record/fixed/enum names, record field names and protocol message names were accepted verbatim when parsing, unlike enum symbols which were already validated. Because the code generator splices some of these values directly into generated C# source (for example protocol message names into a case label and field names into Get/Put switch bodies), an out-of-spec name produced malformed or unexpected generated code. Add a shared, Unicode-aware name validator (first character a letter or '_', remaining characters letters, digits or '_') and apply it to schema names, field names and message names during parsing. The rule is Unicode-aware to preserve the existing support for non-ASCII names, and namespaces are intentionally not validated because the code generator's namespace-mapping feature rewrites them to C#-specific values (such as "@return") and re-parses the schema. Add negative tests for invalid field, record and message names.
There was a problem hiding this comment.
Pull request overview
Adds Avro name-grammar validation to the C# implementation to prevent invalid schema/field/message names from being accepted during parsing (protecting AvroGen/codegen from malformed identifiers and identifier-injection style inputs).
Changes:
- Introduces a shared
SchemaName.ValidateNamehelper and enforces it for schema names, record field names, and protocol message names. - Adds negative tests covering invalid record names, invalid field names, and invalid protocol message names.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lang/csharp/src/apache/test/Schema/SchemaTests.cs | Adds negative schema-parse cases for invalid record/field names. |
| lang/csharp/src/apache/test/Protocol/ProtocolTest.cs | Adds negative protocol-parse cases for invalid message names. |
| lang/csharp/src/apache/main/Schema/SchemaName.cs | Introduces centralized name validation and applies it to schema simple names. |
| lang/csharp/src/apache/main/Schema/Field.cs | Enforces name validation for record field names. |
| lang/csharp/src/apache/main/Protocol/Message.cs | Enforces name validation for protocol message names. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| throw new ArgumentNullException(nameof(name), "name cannot be null."); | ||
| } | ||
|
|
||
| SchemaName.ValidateName(name, "field"); |
Comment on lines
+41
to
+55
| if (string.IsNullOrEmpty(name) | ||
| || !(char.IsLetter(name[0]) || name[0] == '_')) | ||
| { | ||
| throw new SchemaParseException($"Invalid {what} name: {name}"); | ||
| } | ||
|
|
||
| for (int i = 1; i < name.Length; i++) | ||
| { | ||
| char c = name[i]; | ||
| if (!(char.IsLetterOrDigit(c) || c == '_')) | ||
| { | ||
| throw new SchemaParseException($"Invalid {what} name: {name}"); | ||
| } | ||
| } | ||
| } |
Address review feedback: - Validate record field aliases with the same rule as field names, since aliases participate in schema resolution and are names per the Avro grammar. Previously they were accepted verbatim. - Escape control characters (and quote the value) when embedding an invalid name in the SchemaParseException message, so a crafted name containing newlines or other control characters cannot produce multi-line or ambiguous error output. Add a negative test for an invalid field alias.
Comment on lines
+41
to
+57
| internal static void ValidateName(string name, string what) | ||
| { | ||
| if (string.IsNullOrEmpty(name) | ||
| || !(char.IsLetter(name[0]) || name[0] == '_')) | ||
| { | ||
| throw new SchemaParseException($"Invalid {what} name: {Quote(name)}"); | ||
| } | ||
|
|
||
| for (int i = 1; i < name.Length; i++) | ||
| { | ||
| char c = name[i]; | ||
| if (!(char.IsLetterOrDigit(c) || c == '_')) | ||
| { | ||
| throw new SchemaParseException($"Invalid {what} name: {Quote(name)}"); | ||
| } | ||
| } | ||
| } |
Address review feedback: ValidateName inspected individual UTF-16 code units, which rejected valid supplementary-plane letters and digits that are encoded as surrogate pairs. Iterate by Unicode scalar value using the char.IsLetter(string, int) / char.IsLetterOrDigit(string, int) overloads and advance past surrogate pairs. Add a test that a name containing a supplementary-plane letter (U+20000) is accepted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change
Record/fixed/enum names, record field names and protocol message names were
accepted verbatim when parsing, unlike enum symbols which were already
validated. Because the code generator splices some of these values directly
into generated C# source (for example protocol message names into a
caselabel and field names into
Get/Putswitch bodies), an out-of-spec nameproduced malformed or unexpected generated code.
This change adds a shared, Unicode-aware name validator (first character a
letter or
_, remaining characters letters, digits or_) and applies it toschema names, field names and message names during parsing.
Notes:
names (e.g. the
TestRecordFieldNamescase).namespace-mapping feature rewrites them to C#-specific values (such as
@returnfor reserved words) and re-parses the schema.Part of AVRO-4312.
Verifying this change
This change added tests and can be verified as follows:
Schema/SchemaTests.cs(invalid record name, invalidfield name, and a field name attempting identifier injection) and
Protocol/ProtocolTest.cs(TestInvalidMessageName).Avro.testsuite (1526 tests) passes, includingthe AvroGen code-generation tests.
Documentation