fix: update equals & hashCode of model classes to satisfy the expected contract #1446
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
03cd7c0 to
436abb6
Compare
…d contract According to the official Javadocs, there is a contract that must exist between #equals(Object)[1] and #hashCode()[2] for every class. Prior to this change the contract was not upheld for the model classes due to multiple reasons: 1. Some classes did not define equals or hashCode instead relying on the default implementations from java.lang.Object 2. Some classes were using different sets of fields between the two methods 3. Some classes were not performing deep evaluation 4. Some classes were serializing themselves to the apiary models before calling equals on the serialized instances All model classes have been updated to ensure: 1. They define an equals and hashCode 2. The contract is upheld for these methods Additionally, all internal usage of the deprecated DeleteRule has been removed from BlobInfo. Instead, LifecycleRule are used exclusively. Deprecated getDeleteRule and setDeleteRule have had their implementations changed to perform an immediate conversion to/from LifecycleRule as necessary. ApiaryConversions has been updated to ignore DeleteRule when encoding & decoding. A codec for converting between DeleteRule <-> LifecycleRule has been added to BackwardCompatibilityUtils. Corresponding tests for conversions between the various types of DeleteRules have been added in BackwardCompatibilityUtilsTest. Utils.ifNonNull has been updated to test the return value of the mapping function before consuming it. Data.<Boolean>nullOf(Boolean.class) returns `true` which can lead to unexpected behavior if a null value is provided to it indirectly. New Codec#andThen(Codec) has been added to allow easily connecting two Codecs. [1] https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#equals-java.lang.Object- [2] https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#hashCode--
436abb6 to
88016b9
Compare
frankyn
reviewed
Jun 14, 2022
Contributor
frankyn
left a comment
There was a problem hiding this comment.
Overall LGTM, thanks for simplifying the story for deleteRules and lifecycleRules to have a single source
| return Objects.equals( | ||
| Conversions.apiary().bucketInfo().encode(this), | ||
| Conversions.apiary().bucketInfo().encode(that)); | ||
| return Objects.equals(generatedId, that.generatedId) |
Contributor
There was a problem hiding this comment.
Is there a test that can be added to check if all metadata values are set? This can be easily missed again.
Collaborator
Author
There was a problem hiding this comment.
I feel like in general this is more of a "java best practice" to regenerate equals and hashcode when you add a property.
We may want to look into replacing the direct models with something generated in the future, which would automatically include all of the properties.
Contributor
There was a problem hiding this comment.
Would you be open to Autoclass?
Collaborator
Author
There was a problem hiding this comment.
Possibly, we'd have to investigate if it supported all the odd translations and hierarchies we need to support from a binary & source compatibility standpoint
frankyn
approved these changes
Jun 15, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
According to the official Javadocs, there is a contract that must exist between
#equals(Object)[1] and #hashCode()[2] for every class.
Prior to this change the contract was not upheld for the model classes due to
multiple reasons:
implementations from java.lang.Object
equals on the serialized instances
All model classes have been updated to ensure:
Additionally, all internal usage of the deprecated DeleteRule has been removed
from BlobInfo. Instead, LifecycleRule are used exclusively. Deprecated
getDeleteRule and setDeleteRule have had their implementations changed to
perform an immediate conversion to/from LifecycleRule as necessary.
ApiaryConversions has been updated to ignore DeleteRule when encoding & decoding.
A codec for converting between DeleteRule <-> LifecycleRule has been added to
BackwardCompatibilityUtils. Corresponding tests for conversions between the
various types of DeleteRules have been added in BackwardCompatibilityUtilsTest.
Utils.ifNonNull has been updated to test the return value of the mapping
function before consuming it. Data.nullOf(Boolean.class) returns
truewhich can lead to unexpected behavior if a null value is provided to it
indirectly.
New Codec#andThen(Codec) has been added to allow easily connecting two Codecs.
[1] https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#equals-java.lang.Object-
[2] https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#hashCode--