Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ruby] Keep all union record errors and tag them accordingly #2062

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

davidgm0
Copy link

I wasn't able to create an issue in JIRA, since I don't seem to be able to request a user. I am a first time contributor, apologies in case something is not precise enough

What is the purpose of the change

Improving validation errors for unions of records
When no valid union is found, this line selects the first error that it finds. If there would be 3 different possible schemas, there are 3 failures, however only the first one will be considered.

complex_type_failed = failures.detect { |r| COMPLEX_TYPES.include?(r[:type]) }

The returned error also does not give any context of the error, only about the invalid fields.

As an example, consider we have 3 types of adresses PersonalAddress, WorkAddress, SecondAddress and we made a typo: We used the field compan instead of company, which belongs to WorkAddress. No type will work for the union.
{ "compan" => "acme inc.", "some_other_field" => "something else" }

Since PersonalAddress is the first type that fails, the errors of PersonalAddress are the ones that will be returned. Since PersonalAddress has different structure, all fields that aren't the same as in WorkAddress will be shown as errors. The error will be something like

at .address extra field 'compan' - not in schema
at .address extra field 'some_other_field' - not in schema

This gets very confusing because 1) The issue is that no union matched 2) It is not clear because only one of the error is displayed.

I'd be happy if this is fix in some other fashion, this is just an example. With we replace detect with select and we add the schema name to the error. Then the error will be like:

PersonalAddress at .address extra field 'compan' - not in schema
PersonalAddress at .address extra field 'some_other_field' - not in schema
WorkAddress at .address extra field 'compan' - not in schema
SecondAddress at .address extra field 'compan' - not in schema
SecondAddress at .address extra field 'some_other_field' - not in schema

Verifying this change

This change added tests and can be verified as follows:

  • Modified existing test, existing tests pass

Documentation

  • Does this pull request introduce a new feature? no

@github-actions github-actions bot added the Ruby label Jan 20, 2023
..So not only the errors from the first failed union type are kept
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)
Copy link
Member

Choose a reason for hiding this comment

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

What about the other named schemas : Enum and Fixed ?


failure = { type: schema.type_sym, result: result }
failure[:schema] = schema.name if schema.is_a?(Avro::Schema::RecordSchema)
failures << failure if result.failure?
Copy link
Member

Choose a reason for hiding this comment

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

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

@martin-g
Copy link
Member

I wasn't able to create an issue in JIRA

JIRA issue is needed because this is what we use for the release changelog later.
You can request a JIRA account at private@avro.apache.org. Send us your preferred username, display name and email address.

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.

2 participants