Skip to content

feat: Add Hooks class#118

Open
NeaguGeorgiana23 wants to merge 22 commits into
mainfrom
hooks
Open

feat: Add Hooks class#118
NeaguGeorgiana23 wants to merge 22 commits into
mainfrom
hooks

Conversation

@NeaguGeorgiana23

Copy link
Copy Markdown
Contributor

This PR

Introduces the foundation for Hooks. It defines the base interfaces for hooks and the evaluation details structure passed to them during the evaluation lifecycle.

  • Base Hook Interface: Adds BaseHook, a non-templated base class, allowing different types of hooks to be stored together.
  • Typed Hook Interface: Adds the Hook<T> template class defining the lifecycle methods: Before, After, Error, and Finally.
  • Flag Evaluation Details: Adds FlagEvaluationDetails<T>, which extends ResolutionDetails<T> by adding the flag_key to associate the evaluation results with the requested flag.
  • Type Aliases: Provides convenient type aliases for common types (BoolHook, StringHook, IntHook, DoubleHook, ObjectHook and their corresponding FlagEvaluationDetails counterparts).
  • Unit Tests: Adds comprehensive unit tests for both FlagEvaluationDetails and the Hook lifecycle methods.

Fixes #117

NeaguGeorgiana23 and others added 15 commits July 7, 2026 13:19
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
@NeaguGeorgiana23
NeaguGeorgiana23 requested review from a team as code owners July 14, 2026 09:50
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@NeaguGeorgiana23, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 42 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 39bf7d37-c5a9-4ec2-84f1-05ae01a3ec81

📥 Commits

Reviewing files that changed from the base of the PR and between 3b93d77 and a0ca155.

📒 Files selected for processing (2)
  • openfeature/base_hook.h
  • test/hook_test.cpp

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details
putComment timed out

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (2)
test/hook_test.cpp (1)

5-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add missing <vector> include.

The test uses std::vector (Line 222) but relies on transitive inclusion. Including it explicitly prevents unexpected compilation failures if downstream headers are refactored.

♻️ Proposed fix
 `#include` <any>
 `#include` <memory>
 `#include` <optional>
 `#include` <stdexcept>
 `#include` <string>
+#include <vector>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/hook_test.cpp` around lines 5 - 10, Add an explicit <vector>
standard-library include alongside the existing headers in hook_test.cpp, so the
std::vector usage in the test is directly supported rather than relying on
transitive includes.
openfeature/base_hook.h (1)

12-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Delete move operations for polymorphic base class.

BaseHook deletes copy semantics but explicitly defaults move semantics. Since derived classes like Hook explicitly delete move operations, it is safer and more consistent to delete move semantics at the base class level to entirely prevent unintended slicing or partial moves.

♻️ Proposed refactor
-  BaseHook(const BaseHook&) = delete;
-  BaseHook(BaseHook&&) = default;
-  BaseHook& operator=(const BaseHook&) = delete;
-  BaseHook& operator=(BaseHook&&) = default;
+  BaseHook(const BaseHook&) = delete;
+  BaseHook(BaseHook&&) = delete;
+  BaseHook& operator=(const BaseHook&) = delete;
+  BaseHook& operator=(BaseHook&&) = delete;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openfeature/base_hook.h` around lines 12 - 15, Delete the move constructor
and move assignment operator declarations in BaseHook, replacing the defaulted
move operations while preserving its existing deleted copy operations. This
ensures the polymorphic base class cannot be moved or partially transferred,
consistent with derived classes such as Hook.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@openfeature/hook_data.h`:
- Around line 22-29: Add a const overload of HookData::GetAs matching the
existing non-const lookup and typed any_cast behavior, returning a const T
pointer from const HookData instances. Preserve the current missing-key behavior
by returning nullptr.

In `@openfeature/hook.h`:
- Around line 34-38: Make the Before hook accept HookContext<T> as a const
reference, matching the immutability contract and the existing After, Error, and
Finally signatures. Update the corresponding mocked override in
test/hook_test.cpp to use the same const-reference signature; both affected
sites require this change.

---

Nitpick comments:
In `@openfeature/base_hook.h`:
- Around line 12-15: Delete the move constructor and move assignment operator
declarations in BaseHook, replacing the defaulted move operations while
preserving its existing deleted copy operations. This ensures the polymorphic
base class cannot be moved or partially transferred, consistent with derived
classes such as Hook.

In `@test/hook_test.cpp`:
- Around line 5-10: Add an explicit <vector> standard-library include alongside
the existing headers in hook_test.cpp, so the std::vector usage in the test is
directly supported rather than relying on transitive includes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 381c79ae-6a33-413a-ad1c-0990a4889791

📥 Commits

Reviewing files that changed from the base of the PR and between ce590f4 and 3b93d77.

📒 Files selected for processing (17)
  • openfeature/BUILD
  • openfeature/base_hook.h
  • openfeature/flag_evaluation_details.cpp
  • openfeature/flag_evaluation_details.h
  • openfeature/flag_type_value.h
  • openfeature/hook.cpp
  • openfeature/hook.h
  • openfeature/hook_context.cpp
  • openfeature/hook_context.h
  • openfeature/hook_data.cpp
  • openfeature/hook_data.h
  • openfeature/hook_hints.h
  • test/BUILD
  • test/flag_evaluation_details_test.cpp
  • test/hook_context_test.cpp
  • test/hook_data_test.cpp
  • test/hook_test.cpp

Comment thread openfeature/hook_data.h
Comment on lines +22 to +29
template <typename T>
T* GetAs(std::string_view key) {
auto it_key = data_.find(std::string(key));
if (it_key != data_.end()) {
return std::any_cast<T>(&it_key->second);
}
return nullptr;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Add a const overload for GetAs.

The standard OpenFeature Hook lifecycle methods (After, Error, and Finally) receive a const HookContext<T>& ctx. This likely limits state retrieval to a const HookData&. Without a const overload for GetAs, hook authors will not be able to use typed retrieval during these critical stages to read the state they initialized in the Before hook.

🛠️ Proposed fix
   template <typename T>
   T* GetAs(std::string_view key) {
     auto it_key = data_.find(std::string(key));
     if (it_key != data_.end()) {
       return std::any_cast<T>(&it_key->second);
     }
     return nullptr;
   }
+
+  template <typename T>
+  const T* GetAs(std::string_view key) const {
+    auto it_key = data_.find(std::string(key));
+    if (it_key != data_.end()) {
+      return std::any_cast<T>(&it_key->second);
+    }
+    return nullptr;
+  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
template <typename T>
T* GetAs(std::string_view key) {
auto it_key = data_.find(std::string(key));
if (it_key != data_.end()) {
return std::any_cast<T>(&it_key->second);
}
return nullptr;
}
template <typename T>
T* GetAs(std::string_view key) {
auto it_key = data_.find(std::string(key));
if (it_key != data_.end()) {
return std::any_cast<T>(&it_key->second);
}
return nullptr;
}
template <typename T>
const T* GetAs(std::string_view key) const {
auto it_key = data_.find(std::string(key));
if (it_key != data_.end()) {
return std::any_cast<T>(&it_key->second);
}
return nullptr;
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openfeature/hook_data.h` around lines 22 - 29, Add a const overload of
HookData::GetAs matching the existing non-const lookup and typed any_cast
behavior, returning a const T pointer from const HookData instances. Preserve
the current missing-key behavior by returning nullptr.

Comment thread openfeature/hook.h
Comment on lines +34 to +38
// Runs before the flag evaluation occurs.
virtual std::optional<EvaluationContext> Before(HookContext<T>& ctx,
const HookHints& hints) {
return std::nullopt;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Enforce HookContext immutability in the Before hook.

The OpenFeature specification requires that the before hook must not alter the hook context object. Passing ctx as a non-const reference violates this contract and introduces an inconsistency with the After, Error, and Finally methods which correctly accept it as a const reference. The underlying HookData state remains cleanly modifiable since it is stored via std::shared_ptr.

  • openfeature/hook.h#L34-L38: Update the signature to accept const HookContext<T>& ctx.
  • test/hook_test.cpp#L27-L38: Update the mocked override signature to match const HookContext<T>& ctx.
📍 Affects 2 files
  • openfeature/hook.h#L34-L38 (this comment)
  • test/hook_test.cpp#L27-L38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openfeature/hook.h` around lines 34 - 38, Make the Before hook accept
HookContext<T> as a const reference, matching the immutability contract and the
existing After, Error, and Finally signatures. Update the corresponding mocked
override in test/hook_test.cpp to use the same const-reference signature; both
affected sites require this change.

Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <115723925+NeaguGeorgiana23@users.noreply.github.com>
This was referenced Jul 15, 2026
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.

Implement Hook interface

1 participant