Skip to content

Conversation

alexandraoberaigner
Copy link
Contributor

@alexandraoberaigner alexandraoberaigner commented Sep 19, 2025

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:

revert hook data to resolve bytecode incompatibility (#1613) (9845601)

@alexandraoberaigner alexandraoberaigner requested review from a team as code owners September 19, 2025 12:24
Copy link
Contributor

Summary of Changes

Hello @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 HookData within the OpenFeature SDK, allowing individual hooks to maintain state across their lifecycle stages during a flag evaluation. This feature was previously removed in v1.18.0 and is now brought back without introducing breaking changes to the binary. The implementation ensures that each hook instance receives its own isolated data store, enabling more complex and stateful hook logic.

Highlights

  • Reintroduction of Hook Data Support: The pull request re-enables the ability for hooks to store and retrieve data across their various execution stages during a flag evaluation, a feature previously removed in v1.18.0.
  • New HookData Interface and Implementation: A new HookData interface is introduced, along with a DefaultHookData implementation, providing a mechanism for hooks to maintain state isolated to a single flag evaluation.
  • Enhanced HookContext: The HookContext class is updated to include a HookData field, allowing hooks to access their specific data store throughout the evaluation process.
  • Updated Hook Lifecycle Management: The HookSupport class is refactored to manage HookData instances, ensuring they are created per hook and passed correctly through the before, after, error, and finallyAfter stages.
  • Comprehensive Testing: New unit tests for HookData and HookContextWithData are added, and existing HookSupport tests are extended to validate the correct isolation and persistence of hook data across stages.
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

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

  1. 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.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@alexandraoberaigner alexandraoberaigner changed the title feat: add hook data support [DRAFT] feat: add hook data support Sep 19, 2025
@alexandraoberaigner alexandraoberaigner force-pushed the feat/hook-data-support branch 2 times, most recently from 9d41b69 to 0761dd1 Compare September 22, 2025 11:08
@alexandraoberaigner alexandraoberaigner changed the title [DRAFT] feat: add hook data support feat: add hook data support Sep 22, 2025
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
alexandraoberaigner and others added 5 commits September 23, 2025 14:25
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Signed-off-by: Guido Breitenhuber <guido.breitenhuber@dynatrace.com>
Signed-off-by: Guido Breitenhuber <guido.breitenhuber@dynatrace.com>
Signed-off-by: Guido Breitenhuber <guido.breitenhuber@dynatrace.com>
Signed-off-by: Guido Breitenhuber <guido.breitenhuber@dynatrace.com>
Copy link
Member

@aepfli aepfli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like the object based approach, but i have a hard time supporting the changes how hooksupport/hookexecutor is created for each execution. We are bound now for testing, and have no easy way to replace this implementation with a different one.

…tic factory method to ctor

Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

❌ Patch coverage is 92.06349% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.20%. Comparing base (3ef41f5) to head (0e897f2).
⚠️ Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
src/main/java/dev/openfeature/sdk/HookContext.java 79.16% 1 Missing and 4 partials ⚠️
src/main/java/dev/openfeature/sdk/HookSupport.java 95.74% 0 Missing and 2 partials ⚠️
...in/java/dev/openfeature/sdk/OpenFeatureClient.java 91.30% 0 Missing and 2 partials ⚠️
src/main/java/dev/openfeature/sdk/ObjectHook.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1620      +/-   ##
============================================
+ Coverage     92.06%   93.20%   +1.14%     
- Complexity      488      521      +33     
============================================
  Files            46       51       +5     
  Lines          1184     1265      +81     
  Branches        105      112       +7     
============================================
+ Hits           1090     1179      +89     
+ Misses           62       50      -12     
- Partials         32       36       +4     
Flag Coverage Δ
unittests 93.20% <92.06%> (+1.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Copy link
Contributor

@chrfwow chrfwow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have plans for e2e tests?

Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
…ludeFromGeneratedCoverageReport, replace delomboked functions with lombok annotations

Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
@alexandraoberaigner
Copy link
Contributor Author

Do we have plans for e2e tests?

as discussed: will be a follow up task to add a gherkin test

Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
@aepfli
Copy link
Member

aepfli commented Sep 30, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request re-introduces hook data support, which was previously reverted. The changes are quite extensive, refactoring the hook execution logic to be more structured and to support per-hook data stores for the duration of a flag evaluation. A key aspect of this PR is maintaining binary compatibility by deprecating old constructors and methods in HookContext rather than removing them.

My review focuses on improving code clarity, robustness, and removing redundancy. I've suggested simplifying the lazy initialization in DefaultHookData to make it thread-safe and easier to read. I've also pointed out a complex Optional handling in HookSupport that could be made more straightforward, and some redundant null checks in OpenFeatureClient. Overall, the changes are well-structured and achieve the goal of adding hook data support while being mindful of backward compatibility.

Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Copy link

Copy link
Contributor

@chrfwow chrfwow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, however, I would love to see a test that does a full flag evaluation using HookData

for (int i = 0; i < hookSupportData.hooks.size(); i++) {
Pair<Hook, HookContext> hookContextPair = hookSupportData.hooks.get(i);
HookContext curHookContext = sharedContext.hookContextFor(null, new DefaultHookData());
Pair<Hook, HookContext> updatedPair = Pair.of(hookContextPair.getKey(), curHookContext);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a setter here on the Pair to avoid to create Pairs twice for each Hook?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants