-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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-3956: [Java] Fix NPE in Protocol#equals #2791
Conversation
a3da6fb
to
af974cd
Compare
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return name.hashCode() + namespace.hashCode() + types.hashCode() + messages.hashCode() + propsHashCode(); | ||
return name.hashCode() + Objects.hash(namespace) + types.hashCode() + messages.hashCode() + propsHashCode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add the other parameters to Objects.hash
? That would read even better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New hash would be backward incompatible with the old one in that case. If that's ok, I do not see any problems switching to Objects.hash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@opwvhk So what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backwards compatibility is not an issue.
The Protocol#hashCode()
method has never specified differently, and thus uses the general contract of hashCode()
. That states that multiple invocations on the same object, if unchanged, yield the same result. However, it is explicitly stated that the result "need not remain consistent from one execution of an application to another execution of the same application".
See https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Object.html#hashCode()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've updated Protocol#hashCode
@mitasov-ra Please file a Jira issue and update the PR title/description. |
Thank you, @mitasov-ra ! |
* Fix NPE in Protocol#equals * Better Protocol#hashCode
AVRO-3956
What is the purpose of the change
Fix of NPE in Protocol#equals.
As of documentation,
namespace
is optional inProtocol
and thus can benull
.Protocol#equals
contains direct dereference ofnamespace
, which causesNullPointerException
when comparing Protocols withnull
innamespace
.This MR fixes that.
Verifying this change
(Please pick one of the following options)
This change is a trivial rework / code cleanup without any test coverage.
Documentation