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

AVRO-3938: [Java] Add null guards for Schema.Parser #2738

Merged
merged 3 commits into from Feb 21, 2024

Conversation

pingpingy1
Copy link
Contributor

What is the purpose of the change

This pull request provides a fail-fast guard against the private final property Schema.Parser.validate, implementing AVRO-3938.

Verifying this change

This change added the test TestSchema.testParserNullValidate.

Documentation

  • Does this pull request introduce a new feature? no

The `Schema.Parser` class has a private final property `validate`.
This means that the value provided to its constructor is saved and
cannot be changed at a later time.
However, if `null` is passed as `validate`, then it is bound to throw a
`NullPointerException` at an arbitrary time.
This commit adds a fail-fast guard against `null` to prevent this.
@github-actions github-actions bot added the Java Pull Requests for Java binding label Feb 15, 2024
@pingpingy1
Copy link
Contributor Author

@martin-g How do you think the test should be modified? I can't think of how to verify the change and honestly don't think one is strictly needed here.

Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
@martin-g
Copy link
Member

IMO the test should verify that the Parser behaves the same way as when NO_VALIDATOR is passed.
I am not familiar with the Java SDK but I guess there is/are test(s) for new Parser(NO_VALIDATOR) already. You can update them to test with null too.

@pingpingy1
Copy link
Contributor Author

@martin-g Just pushed a new test case involving an empty string as a name. PTAL.

@pingpingy1
Copy link
Contributor Author

@martin-g Seems the PR passed the checks. Could you checki if this PR could be merged by any chance? Thanks.

@martin-g martin-g merged commit 642e270 into apache:main Feb 21, 2024
8 checks passed
@martin-g
Copy link
Member

Thank you, @pingpingy1 !

RanbirK pushed a commit to RanbirK/avro that referenced this pull request May 13, 2024
* AVRO-3938: Add null guards for Schema.Parser

The `Schema.Parser` class has a private final property `validate`.
This means that the value provided to its constructor is saved and
cannot be changed at a later time.
However, if `null` is passed as `validate`, then it is bound to throw a
`NullPointerException` at an arbitrary time.
This commit adds a fail-fast guard against `null` to prevent this.

* Apply suggestions from code review

Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>

* Modify test case

---------

Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java Pull Requests for Java binding
Projects
None yet
2 participants