feat: add hook data support #1620
Conversation
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Summary of ChangesHello @alexandraoberaigner, 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 reintroduces and refines the support for 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
|
![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 re-introduces hook data support, which is a great addition for allowing hooks to maintain state across execution stages. The implementation is solid, with good test coverage for the new functionality. I've identified a few areas for improvement, mainly concerning code clarity, consistency, and a potential thread-safety issue in the HookData implementation. My detailed comments and suggestions are provided below.
| executeHooksUnchecked( | ||
| flagValueType, hookDataPairs, hookContext, (hook, ctx) -> hook.after(ctx, details, hints)); |
There was a problem hiding this comment.
The flagValueType parameter is passed to executeHooksUnchecked, but it's no longer used there because the hooks are pre-filtered in getHookDataPairs. You can remove this parameter from the call to simplify the code. This change should be made in conjunction with updating the executeHooksUnchecked method signature.
executeHooksUnchecked(hookDataPairs, hookContext, (hook, ctx) -> hook.after(ctx, details, hints));| Hook hook = hookDataPair.getLeft(); | ||
| HookData hookData = hookDataPair.getRight(); |
There was a problem hiding this comment.
To align with the suggested change in Pair.java for consistent naming, getLeft() and getRight() should be updated to getKey() and getValue().
Hook hook = hookDataPair.getKey();
HookData hookData = hookDataPair.getValue();| private <T> void executeHooksUnchecked( | ||
| FlagValueType flagValueType, | ||
| List<Pair<Hook, HookData>> hookDataPairs, | ||
| HookContext hookContext, | ||
| BiConsumer<Hook<T>, HookContext> hookCode) { |
There was a problem hiding this comment.
The flagValueType parameter is no longer used in this method, as the hookDataPairs list is already filtered. It can be removed to simplify the method signature.
private <T> void executeHooksUnchecked(
List<Pair<Hook, HookData>> hookDataPairs,
HookContext hookContext,
BiConsumer<Hook<T>, HookContext> hookCode) {| Hook hook = hookDataPair.getLeft(); | ||
| HookData hookData = hookDataPair.getRight(); |
There was a problem hiding this comment.
To align with the suggested change in Pair.java for consistent naming, getLeft() and getRight() should be updated to getKey() and getValue().
Hook hook = hookDataPair.getKey();
HookData hookData = hookDataPair.getValue();| Hook hook = hookDataPair.getLeft(); | ||
| HookData hookData = hookDataPair.getRight(); |
There was a problem hiding this comment.
To align with the suggested change in Pair.java for consistent naming, getLeft() and getRight() should be updated to getKey() and getValue().
Hook hook = hookDataPair.getKey();
HookData hookData = hookDataPair.getValue();| hookContext = | ||
| HookContext.from(key, type, this.getMetadata(), provider.getMetadata(), new ImmutableContext(), defaultValue); |
There was a problem hiding this comment.
The ImmutableContext class now provides a static EMPTY instance. It's better to use this constant instead of creating a new instance with new ImmutableContext(), especially in a performance-sensitive method like evaluateFlag. This improves consistency and avoids an unnecessary object allocation.
| hookContext = | |
| HookContext.from(key, type, this.getMetadata(), provider.getMetadata(), new ImmutableContext(), defaultValue); | |
| hookContext = | |
| HookContext.from(key, type, this.getMetadata(), provider.getMetadata(), ImmutableContext.EMPTY, defaultValue); |
| public K getLeft() { | ||
| return key; | ||
| } | ||
|
|
||
| public V getRight() { | ||
| return value; | ||
| } |
There was a problem hiding this comment.
The getter methods getLeft() and getRight() are inconsistent with the field names (key, value), constructor parameters, and toString() implementation, which all use "key" and "value". For better consistency throughout this new class, consider renaming these getters to getKey() and getValue(). The usages in HookSupport.java will also need to be updated.
| public K getLeft() { | |
| return key; | |
| } | |
| public V getRight() { | |
| return value; | |
| } | |
| public K getKey() { | |
| return key; | |
| } | |
| public V getValue() { | |
| return value; | |
| } |
There was a problem hiding this comment.
No strong opinion about it. @aepfli didn' we talk about this being good practice to use left/right?
alexandraoberaigner
changed the title
| @@ -0,0 +1,14 @@ | |||
| package dev.openfeature.sdk; | |||
|
|
|||
| public class HookContextWithData<T> extends HookContext<T> { | |||
There was a problem hiding this comment.
i am not sure we want to duplicate everything, i think the previous implementation with helper methods to access the object within is fine.
There was a problem hiding this comment.
What do you mean with duplicate everything exactly? If I make HookContext a class instead of an interface I need to call its ctor and pass the data...or did I miss something here? or are you suggesting something else than inheritance?
There was a problem hiding this comment.
Thinking of using a decorator...do you think this is overkill?
There was a problem hiding this comment.
i think a decorator pattern is a perfect fit for this. we can even leave the hookcontext as a @value class - and keep its immutability
There was a problem hiding this comment.
we cannot keep @value unfortunately since the decorator extends from HookContext...pls let me know if I missed something
There was a problem hiding this comment.
i thought when you said decorator about something like
public class HookContextWithData<T> extends HookContext<T> {
private final HookContext<T> hookContext;
public HookContextWithData(HookContext hookcontext, HookData hookData) {
this.hookdata = hookdata;
this.hookContext = hookContext;
}
public getFlagKey() {
return hookContext.getFlagKey()
}
// ... and for the others except hookdata
public getHookData() {
return hookdata;
}
}this way, we do not need to duplicate the other properties of HookContext, just the hookData is special to each of them.
There was a problem hiding this comment.
I meant the decorator pattern 🙈
There was a problem hiding this comment.
With your example I'd need to add a parameterless ctor to HookContext - which is not feasable because of the @nonnull annotations on the members - as soon as we extend we need to call the super() ctor
There was a problem hiding this comment.
but we could add a protected constructor to the hookContext, passing null for all the values, can't we? theoretically? (lombok makes everthing hard)
There was a problem hiding this comment.
Well, I guess we can ignore the hint... but feels wrong
| return Optional.ofNullable(new ImmutableContext()); | ||
| } | ||
| }); | ||
| client.addHooks(new Hook<String>() { |
There was a problem hiding this comment.
| client.addHooks(new Hook<String>() { | |
| client.addHooks(new StringHook () { |
I think we need to use the dedicated implementations for the filter for flag type to work
There was a problem hiding this comment.
As part of this PR or in general?
alexandraoberaigner
force-pushed
the
feat/hook-data-support
branch
from
328d9ec to
9d41b69
Compare
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
alexandraoberaigner
force-pushed
the
feat/hook-data-support
branch
from
9d41b69 to
0761dd1
Compare
alexandraoberaigner
changed the title
|
This PR
this re-adds hook data support avoiding breaking changes in the binary (we introduced with v1.18.0)
Related Issues
#1472
###Notes
With v1.18.1 we reverted the hook data feature: