fix: revert hook data to resolve bytecode incompatibility #1613
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @chrfwow, 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 addresses a critical issue by reverting a recently added 'hook data' feature that inadvertently introduced a binary breaking change. The changes simplify the OpenFeature SDK's hook mechanism by removing the HookData concept and consolidating HookContext into a single, more direct class. This ensures compatibility and reduces complexity in how hooks manage context during flag evaluation.
Highlights
- Revert of "Hook Data" Feature: This pull request fully reverts the previously introduced 'feat: add hook data' change, which was identified as causing a binary breaking change in the SDK.
- Simplification of HookContext: The
HookContexthas been refactored from an interface into a concrete class, directly incorporating its properties and eliminating the need for separateHookContextWithoutDataandHookContextWithDataclasses. - Removal of HookData and Related Utilities: The
HookDatainterface, its default implementation, and thePairutility class have been entirely removed, streamlining the internal hook management mechanisms. - Streamlined Hook Execution Logic: The
HookSupportclass has been updated to reflect the removal ofHookData, simplifying the methods responsible for executingbefore,after,error, andfinallyAfterhooks during flag evaluation.
Using Gemini Code Assist
The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
chrfwow
changed the title
toddbaert
changed the title
There was a problem hiding this comment.
Code Review
This pull request reverts the 'hook data' feature to fix a binary breaking change. The revert is comprehensive, removing related classes and simplifying the hook execution logic. However, I've identified a critical issue in OpenFeatureClient where the refactoring could lead to a NullPointerException during hook execution if an error occurs early in the process. I've provided suggestions to fix this by ensuring a valid HookContext is always available for all hook stages.
| List<Hook> mergedHooks = null; | ||
| HookContext<T> afterHookContext = null; | ||
|
|
||
| try { | ||
| final var stateManager = openfeatureApi.getFeatureProviderStateManager(this.domain); | ||
| var stateManager = openfeatureApi.getFeatureProviderStateManager(this.domain); | ||
| // provider must be accessed once to maintain a consistent reference | ||
| final var provider = stateManager.getProvider(); | ||
| final var state = stateManager.getState(); | ||
| hookContext = | ||
| HookContextWithoutData.from(key, type, this.getMetadata(), provider.getMetadata(), defaultValue); | ||
|
|
||
| // we are setting the evaluation context one after the other, so that we have a hook context in each | ||
| // possible exception case. | ||
| hookContext.setCtx(mergeEvaluationContext(ctx)); | ||
| var provider = stateManager.getProvider(); | ||
| var state = stateManager.getState(); | ||
|
|
||
| mergedHooks = ObjectUtils.merge( | ||
| provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getMutableHooks()); | ||
| hookDataPairs = hookSupport.getHookDataPairs(mergedHooks, type); | ||
| var mergedCtx = hookSupport.beforeHooks(type, hookContext, hookDataPairs, hints); | ||
| hookContext.setCtx(mergedCtx); | ||
|
|
||
| var mergedCtx = hookSupport.beforeHooks( | ||
| type, | ||
| HookContext.from( | ||
| key, | ||
| type, | ||
| this.getMetadata(), | ||
| provider.getMetadata(), | ||
| mergeEvaluationContext(ctx), | ||
| defaultValue), | ||
| mergedHooks, | ||
| hints); | ||
|
|
||
| afterHookContext = | ||
| HookContext.from(key, type, this.getMetadata(), provider.getMetadata(), mergedCtx, defaultValue); |
There was a problem hiding this comment.
This change introduces a potential NullPointerException. The afterHookContext variable is initialized to null and only assigned a value after beforeHooks completes successfully. If an exception is thrown during beforeHooks execution (or earlier, e.g., during hook merging), afterHookContext will remain null. This null value is then passed to errorHooks and afterAllHooks in the catch and finally blocks, which violates the OpenFeature specification requiring a non-null HookContext.
To fix this, we should ensure a valid HookContext is always available. The suggested change establishes a hookContext early and updates it immutably, preventing the null context issue. Subsequent comments will adjust the rest of the method to use this new hookContext variable.
List<Hook> mergedHooks = null;
HookContext<T> hookContext = null;
try {
var stateManager = openfeatureApi.getFeatureProviderStateManager(this.domain);
// provider must be accessed once to maintain a consistent reference
var provider = stateManager.getProvider();
var state = stateManager.getState();
mergedHooks = ObjectUtils.merge(
provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getMutableHooks());
hookContext =
HookContext.from(
key,
type,
this.getMetadata(),
provider.getMetadata(),
mergeEvaluationContext(ctx),
defaultValue);
var mergedCtx = hookSupport.beforeHooks(
type,
hookContext,
mergedHooks,
hints);
hookContext = hookContext.withCtx(mergedCtx);| hookSupport.errorHooks(type, afterHookContext, error, mergedHooks, hints); | ||
| } else { | ||
| hookSupport.afterHooks(type, hookContext, details, hookDataPairs, hints); | ||
| hookSupport.afterHooks(type, afterHookContext, details, mergedHooks, hints); |
There was a problem hiding this comment.
As part of the fix for the potential NullPointerException identified in the previous comment, this should be updated to use the hookContext variable instead of afterHookContext.
| hookSupport.errorHooks(type, afterHookContext, error, mergedHooks, hints); | |
| } else { | |
| hookSupport.afterHooks(type, hookContext, details, hookDataPairs, hints); | |
| hookSupport.afterHooks(type, afterHookContext, details, mergedHooks, hints); | |
| hookSupport.errorHooks(type, hookContext, error, mergedHooks, hints); | |
| } else { | |
| hookSupport.afterHooks(type, hookContext, details, mergedHooks, hints); |
| details.setErrorMessage(e.getMessage()); | ||
| enrichDetailsWithErrorDefaults(defaultValue, details); | ||
| hookSupport.errorHooks(type, hookContext, e, hookDataPairs, hints); | ||
| hookSupport.errorHooks(type, afterHookContext, e, mergedHooks, hints); |
There was a problem hiding this comment.
As part of the fix for the potential NullPointerException, this should be updated to use the hookContext variable instead of afterHookContext.
| hookSupport.errorHooks(type, afterHookContext, e, mergedHooks, hints); | |
| hookSupport.errorHooks(type, hookContext, e, mergedHooks, hints); |
| hookSupport.errorHooks(type, afterHookContext, e, mergedHooks, hints); | ||
| } finally { | ||
| hookSupport.afterAllHooks(type, hookContext, details, hookDataPairs, hints); | ||
| hookSupport.afterAllHooks(type, afterHookContext, details, mergedHooks, hints); |
There was a problem hiding this comment.
As part of the fix for the potential NullPointerException, this should be updated to use the hookContext variable instead of afterHookContext.
| hookSupport.afterAllHooks(type, afterHookContext, details, mergedHooks, hints); | |
| hookSupport.afterAllHooks(type, hookContext, details, mergedHooks, hints); |
chrfwow
force-pushed
the
revert-hook-data
branch
from
bf6aa7d to
c804651
Compare
Signed-off-by: christian.lutnik <[email protected]>
Signed-off-by: christian.lutnik <[email protected]>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1613 +/- ##
============================================
+ Coverage 92.59% 93.41% +0.81%
+ Complexity 502 491 -11
============================================
Files 50 46 -4
Lines 1215 1184 -31
Branches 106 105 -1
============================================
- Hits 1125 1106 -19
+ Misses 60 48 -12
Partials 30 30
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:
|
3 participants
The addition of the hook data has resulted in a binary breaking change, which this PR attemts to fix