3317: [ruby] Keep all union record errors and tag them accordingly#11
3317: [ruby] Keep all union record errors and tag them accordingly#11martin-augment wants to merge 4 commits intomainfrom
Conversation
..So not only the errors from the first failed union type are kept
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| error | ||
| end.join("; ") | ||
| schema_name_prefix = "#{failed_complex_type[:schema_name]}: " if failed_complex_type[:schema_name] | ||
| complex_type_errors << "#{schema_name_prefix}#{error_msg}" |
There was a problem hiding this comment.
schema_name_prefix is assigned only when schema_name is present; on iterations without a name, the variable can retain the previous iteration’s value and incorrectly prefix errors. Consider resetting the prefix each loop iteration to avoid leaking a prior schema name.
🤖 React with 👍 or 👎 to let us know if the comment was useful.
There was a problem hiding this comment.
value:annoying; category:bug; feedback:the model wrongly assumes that the local variable would leak to the next iteration. This could happen with "for loop" in Ruby but the code here uses an iterator.
Here is a small test showing that temp does not survive to the next loop:
#!/usr/bin/env ruby
i = 0
collection = ['a', 'b', 'c']
collection.each do |x|
temp = "#{x}" if i % 2 == 0
print "#{i} #{x}: #{temp}\n"
i = i + 1
endIt prints:
0 a: a
1 b:
2 c: c
Code Review for PR #11: Keep all union record errors and tag them accordinglySummaryThis PR improves error reporting for union type validation failures by collecting and displaying all complex type errors with schema names, rather than just showing the first one. This is a valuable enhancement for debugging validation issues. Code Quality & Best PracticesStrengths✅ Improved error messages: The new error format provides much better context by showing all failed union member validation errors Issues & Suggestions1. Code Style - Unnecessary mapping (line 205-207)The .map operation is a no-op and should be simplified to: Location: lang/ruby/lib/avro/schema_validator.rb:205-207 2. Code Style - Trailing whitespace (line 203, 208)Line 203 has double space after = and line 208 has trailing space Location: lang/ruby/lib/avro/schema_validator.rb:203,208 3. Code Style - String literal choice (line 215)Using %Q for a simple string with interpolation is unconventional. Standard double-quoted string is more idiomatic. Location: lang/ruby/lib/avro/schema_validator.rb:215 4. Code Style - Redundant conditional blocks (line 222-224)The original one-liner was more concise and Ruby-idiomatic. Consider reverting to: Location: lang/ruby/lib/avro/schema_validator.rb:222-224 Potential Bugs
|
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe schema validator now aggregates errors from all failed union member types instead of stopping at the first failure, providing consolidated error messages that include schema names and structured error blocks for improved debugging. Changes
Sequence DiagramsequenceDiagram
participant Validator as Validator
participant Union as Union Handler
participant Members as Schema Members
participant ErrorLog as Error Aggregator
rect rgb(200, 220, 240)
Note over Validator,ErrorLog: Old Flow: Stop at First Error
Validator->>Union: validate union with [null, record1, record2]
Union->>Members: check member 1 (null)
Members-->>Union: OK
Union->>Members: check member 2 (record1)
Members-->>Union: FAIL
Union-->>Validator: return first error only
end
rect rgb(220, 240, 200)
Note over Validator,ErrorLog: New Flow: Aggregate All Errors
Validator->>Union: validate union with [null, record1, record2]
Union->>ErrorLog: initialize aggregator
Union->>Members: check member 1 (null)
Members-->>Union: OK
Union->>Members: check member 2 (record1)
Members-->>Union: FAIL → with schema name
ErrorLog->>ErrorLog: record error for record1
Union->>Members: check member 3 (record2)
Members-->>Union: FAIL → with schema name
ErrorLog->>ErrorLog: record error for record2
Union->>ErrorLog: build consolidated message
ErrorLog-->>Validator: return aggregated error block
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes involve logic modifications to error aggregation and reporting within union validation, paired with multiple test expectation updates across different test cases. The core logic change is contained but requires careful verification of the error consolidation behavior and message formatting. Test updates are consistent in pattern but span multiple test methods with varying union compositions. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.81.1)lang/ruby/test/test_schema_validator.rbError: The lang/ruby/lib/avro/schema_validator.rbError: The Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lang/ruby/lib/avro/schema_validator.rb (3)
205-207: Remove redundant map operation.The
.map do |error| error endis an identity function that returns each error unchanged. You can simplify this by directly calling.join("; ")on the errors array.Apply this diff:
- error_msg = failed_complex_type[:result].errors.map do |error| - error - end.join("; ") + error_msg = failed_complex_type[:result].errors.join("; ")
208-208: Remove trailing whitespace.There's trailing whitespace after the string interpolation and before the
ifkeyword.Apply this diff:
- schema_name_prefix = "#{failed_complex_type[:schema_name]}: " if failed_complex_type[:schema_name] + schema_name_prefix = "#{failed_complex_type[:schema_name]}: " if failed_complex_type[:schema_name]
215-215: Consider using standard string interpolation.The
%Q{}syntax is unconventional here. Standard string interpolation with double quotes would be more idiomatic and readable.Apply this diff:
- type_mismatches = %Q{\nUnion type specific errors:\n#{complex_type_errors.join("\n")}} if complex_type_errors.any? + type_mismatches = "\nUnion type specific errors:\n#{complex_type_errors.join("\n")}" if complex_type_errors.any?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lang/ruby/lib/avro/schema_validator.rb(1 hunks)lang/ruby/test/test_schema_validator.rb(2 hunks)
🔇 Additional comments (6)
lang/ruby/lib/avro/schema_validator.rb (3)
222-224: Verify the use ofnextwithinfindblock.Using
nextinside afindblock to return a boolean value is unconventional. The standard pattern would be to simply return the boolean expression directly. However, this may be intentional to skip to the next iteration when the datum is nil and the schema is null (indicating a match).Please verify this logic is correct. The
next datum.nil?will returntruefrom thefindblock if datum is nil (causingfindto return the schema), orfalseif datum is not nil (causingfindto continue to the next schema). This appears correct for null compatibility checking.
201-217: Enhanced union error reporting looks good.The implementation successfully aggregates errors from all complex union member types and presents them in a structured, debuggable format. The conditional inclusion of schema names provides valuable context when available, and the separation of complex type errors into a dedicated block improves readability.
228-232: Proper failure context tracking.The enhancement to include
schema_namein the failure hash when available provides valuable debugging context. The conditional assignment ensures backward compatibility when schemas don't have names.lang/ruby/test/test_schema_validator.rb (3)
334-348: Test expectations correctly updated.The expected error message properly reflects the new union error format with the "Union type specific errors" block. The multi-line format using heredoc improves readability.
564-569: Enhanced error message validation.The updated expectation correctly validates the new format that includes both the union type list (with named types) and the specific per-member error details.
572-606: Excellent test coverage for complex unions.This new test provides comprehensive coverage for a complex union scenario with multiple record types and an enum. It validates that:
- All complex member types are evaluated
- Per-member errors are collected and tagged with schema names
- Extra field validation works across multiple union branches
- Error messages are properly formatted and aggregated
The test data with intentionally missing required fields (
name,species) and extra fields (namo,color) effectively exercises the error aggregation logic.
2 participants
3317: To review by AI
Summary by CodeRabbit
Bug Fixes
Tests