[GitHub] [avro] martin-g commented on a diff in pull request #2062: [ruby] Keep all union record errors and tag them accordingly

["martin-g (via GitHub)" <gi...@apache.org>]

martin-g commented on code in PR #2062:
URL: https://github.com/apache/avro/pull/2062#discussion_r1084117029


##########
lang/ruby/lib/avro/schema_validator.rb:
##########
@@ -208,11 +213,16 @@ def validate_union(expected_schema, datum, path, result, options)
       def first_compatible_type(datum, expected_schema, path, failures, options = {})
         expected_schema.schemas.find do |schema|
           # Avoid expensive validation if we're just validating a nil
-          next datum.nil? if schema.type_sym == :null
+          if schema.type_sym == :null
+            next datum.nil?
+          end
 
           result = Result.new
           validate_recursive(schema, datum, path, result, options)
-          failures << { type: schema.type_sym, result: result } if result.failure?
+
+          failure = { type: schema.type_sym, result: result }
+          failure[:schema] = schema.name if schema.is_a?(Avro::Schema::RecordSchema)

Review Comment:
   What about the other named schemas : Enum and Fixed ?



##########
lang/ruby/lib/avro/schema_validator.rb:
##########
@@ -208,11 +213,16 @@ def validate_union(expected_schema, datum, path, result, options)
       def first_compatible_type(datum, expected_schema, path, failures, options = {})
         expected_schema.schemas.find do |schema|
           # Avoid expensive validation if we're just validating a nil
-          next datum.nil? if schema.type_sym == :null
+          if schema.type_sym == :null
+            next datum.nil?
+          end
 
           result = Result.new
           validate_recursive(schema, datum, path, result, options)
-          failures << { type: schema.type_sym, result: result } if result.failure?
+
+          failure = { type: schema.type_sym, result: result }
+          failure[:schema] = schema.name if schema.is_a?(Avro::Schema::RecordSchema)
+          failures << failure if result.failure?

Review Comment:
   IMO it would be cleaner if the whole block is wrapped in `if`, as the change you made above (L216).
   There is no need to create `failure` object if `result.failure?` is `false`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org