fix: Check for equality of value instead of equality of instance. #1100
Conversation
This file contains 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
mattcasters
reviewed
Dec 9, 2021
| @@ -59,7 +60,20 @@ public boolean encrypted() | |||
|
|
|||
| private boolean isCustomized() | |||
| { | |||
| return this != DEFAULT; | |||
| return !(DEFAULT.encrypted() == this.encrypted() && DEFAULT.hasEqualTrustStrategy( this )); | |||
There was a problem hiding this comment.
Indeed this is the source of contention across serialization exercises. This change would fix that.
michael-simons
requested review from
gjmwoods
and removed request for
injectives
gjmwoods
approved these changes
Dec 10, 2021
isCustomized returned the wrong value for all instances that have been serialized and read back as they obviously does not point to the default instance. The solution is to check for equality of the values, not for equality of the instances. This is a minor change in behaviour: A setting that is created with the builder matching the exact same defaults is now considered to be not customized. This solution has been preferred over a custom readResolve returning the DEFAULT when things match as it would otherwise be able to create an instance that identifies itself as customized but changed to not customized when read back, basically the oppositte of what is fixed here: An instances created as not customized must never be read back as customized.
These tests needed to be adapted due to the fact that the previous test did not actually change the default value.
michael-simons
force-pushed
the
issue/secsettings-serialized
branch
from
916e03b
to
0cd6f00
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
isCustomized returned the wrong value for all instances that have been serialized and read back as they obviously does not point to the default instance.
The solution is to check for equality of the values, not for equality of the instances. This is a minor change in behaviour: A setting that is created with the builder matching the exact same defaults is now considered to be not customized.
This solution has been preferred over a custom readResolve returning the DEFAULT when things match as it would otherwise be able to create an instance that identifies itself as customized but changed to not customized when read back, basically the oppositte of what is fixed here: An instances created as not customized must never be read back as customized.