fix: context serialization missing props (with revert) #1768
Conversation
This reverts commit 6f3a30a. Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Summary of ChangesHello @toddbaert, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request serves as a revert of a previous commit, aiming to refine the handling of context serialization and equality within the SDK. It simplifies the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
toddbaert
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors ImmutableContext and LayeredEvaluationContext. In ImmutableContext, manual equals() and hashCode() implementations and a cachedHashCode field were removed in favor of Lombok's @EqualsAndHashCode annotation. Corresponding test cases were adjusted, and minor display name corrections were suggested by the reviewer for ImmutableContext equality tests. In LayeredEvaluationContext, the cachedMap field and its caching logic were removed, simplifying asMap() and asObjectMap() to construct maps on demand. The manual equals() and hashCode() methods were also removed from LayeredEvaluationContext, which the reviewer identified as a significant behavioral change, reverting to identity-based equality inconsistent with other EvaluationContext implementations. The reviewer recommended re-implementing value-based equals() and hashCode() for LayeredEvaluationContext to ensure consistent behavior and prevent issues in collections, such as by delegating to asMap().
| return map; | ||
| } | ||
|
|
||
| void putHookContext(EvaluationContext context) { |
There was a problem hiding this comment.
The equals() and hashCode() methods have been removed from this class. This reverts to Object's identity-based equality, which is inconsistent with other EvaluationContext implementations like ImmutableContext and MutableContext that provide value-based equality. This is a significant behavioral change that could break consumers relying on value comparison (e.g., in collections).
If this was an unintended side-effect of removing the caching logic, please consider re-implementing equals() and hashCode() to restore value semantics, for example by delegating to asMap(). Without this, LayeredEvaluationContext will not behave as expected in HashMaps or other collections that rely on equals.
| } | ||
|
|
||
| @DisplayName("Two different ImmutableContext objects with different contents are not considered equal") | ||
| @DisplayName("Two different MutableContext objects with the different contents are not considered equal") |
There was a problem hiding this comment.
The display name for this test seems to have a copy-paste error, as it refers to MutableContext in a test for ImmutableContext. This should be corrected for clarity in test reports.
| @DisplayName("Two different MutableContext objects with the different contents are not considered equal") | |
| @DisplayName("Two different ImmutableContext objects with different contents are not considered equal") |
| } | ||
|
|
||
| @DisplayName("ImmutableContext hashCode is stable across multiple invocations") | ||
| @DisplayName("Two different MutableContext objects with the same content are considered equal") |
There was a problem hiding this comment.
The display name for this test incorrectly refers to MutableContext instead of ImmutableContext. This should be corrected for clarity.
| @DisplayName("Two different MutableContext objects with the same content are considered equal") | |
| @DisplayName("Two ImmutableContext objects with the same content are considered equal") |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1768 +/- ##
============================================
+ Coverage 92.08% 93.66% +1.58%
- Complexity 601 602 +1
============================================
Files 55 55
Lines 1427 1406 -21
Branches 156 154 -2
============================================
+ Hits 1314 1317 +3
+ Misses 67 50 -17
+ Partials 46 39 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…feature#1768)" This reverts commit 4cb39a4.
This reverts commit 4cb39a4. Co-authored-by: vishalup29 <vishalupadhyay977@gmail.com>
This reverts commit 4cb39a4. Co-authored-by: Vishalup29 <67001661+Vishalup29@users.noreply.github.com>
This reverts commit 4cb39a4. Co-authored-by: Vishalup29 <67001661+Vishalup29@users.noreply.github.com> Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
…feature#1768)" This reverts commit 4cb39a4.
This reverts commit 4cb39a4. Co-Authored-By: Vishalup29 <67001661+Vishalup29@users.noreply.github.com> Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
This reverts commit 6f3a30a, which causes significant issues (serialization of context doesn't work, most/all attributes seem to be missing, see open-feature/java-sdk-contrib#1665 for a demo).
#1756