feat: Update global api#123
Conversation
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>
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>
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>
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds C++ hook contracts, evaluation detail types, provider hook access, and global hook management. New Bazel targets and tests cover lifecycle callbacks, hook hints, polymorphism, evaluation details, and API registration behavior. ChangesHook infrastructure
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant OpenFeatureAPI
participant HookStorage
Client->>OpenFeatureAPI: AddHook or AddHooks
OpenFeatureAPI->>HookStorage: append non-null hooks
Client->>OpenFeatureAPI: GetHooks
OpenFeatureAPI->>HookStorage: read hooks under shared lock
HookStorage-->>OpenFeatureAPI: hook snapshot
OpenFeatureAPI-->>Client: return hook list
Client->>OpenFeatureAPI: Shutdown
OpenFeatureAPI->>HookStorage: clear hooks under exclusive lock
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/openfeature_api_test.cpp (1)
19-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winEnsure test isolation matches the comment.
The comment indicates that the singleton is shut down before and after each test to reset its state. However,
SetUp()is currently empty. To prevent potential state leakage from other test suites that might run beforehand in the same process, consider applying the cleanup logic inSetUp()as well.🛠️ Proposed fix
// To ensure test isolation for the singleton, we shut it down before and // after each test, to reset it to its default state. - void SetUp() override {} + void SetUp() override { + api.Shutdown(); + api.SetEvaluationContext(EvaluationContext::Builder().build()); + } void TearDown() override { api.Shutdown(); api.SetEvaluationContext(EvaluationContext::Builder().build()); }🤖 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/openfeature_api_test.cpp` around lines 19 - 25, Update the test fixture’s SetUp method to perform the same singleton cleanup as TearDown: call api.Shutdown() and reset the evaluation context with a newly built default EvaluationContext. Keep the existing TearDown cleanup unchanged so isolation occurs both before and after each test.
🤖 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/flag_evaluation_details.cpp`:
- Around line 3-5: Add a direct <utility> include in flag_evaluation_details.cpp
alongside the existing standard-library includes so the translation unit
explicitly provides std::move without relying on transitive includes.
In `@openfeature/openfeature_api.cpp`:
- Around line 80-84: Update OpenFeatureAPI::AddHooks to skip null shared
pointers before inserting elements into hooks_, matching the validation
performed by AddHook. Preserve moving valid hooks into the collection while
ensuring nullptr entries are never stored.
In `@openfeature/provider.h`:
- Line 28: Update FeatureProvider::GetHooks() to provide a default empty
implementation instead of remaining pure virtual, preserving source
compatibility for downstream providers while allowing providers to override it
when needed.
---
Outside diff comments:
In `@test/openfeature_api_test.cpp`:
- Around line 19-25: Update the test fixture’s SetUp method to perform the same
singleton cleanup as TearDown: call api.Shutdown() and reset the evaluation
context with a newly built default EvaluationContext. Keep the existing TearDown
cleanup unchanged so isolation occurs both before and after each test.
🪄 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: a865e274-ec3d-4840-9f04-56e938238aab
📒 Files selected for processing (22)
openfeature/BUILDopenfeature/base_hook.hopenfeature/evaluation_options.hopenfeature/flag_evaluation_details.cppopenfeature/flag_evaluation_details.hopenfeature/hook.cppopenfeature/hook.hopenfeature/memory_provider/BUILDopenfeature/memory_provider/in_memory_provider.cppopenfeature/memory_provider/in_memory_provider.hopenfeature/noop_provider.cppopenfeature/noop_provider.hopenfeature/openfeature.hopenfeature/openfeature_api.cppopenfeature/openfeature_api.hopenfeature/provider.htest/BUILDtest/evaluation_options_test.cpptest/flag_evaluation_details_test.cpptest/hook_test.cpptest/mocks/mock_feature_provider.htest/openfeature_api_test.cpp
| #include <optional> | ||
| #include <string> | ||
| #include <string_view> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
ast-grep outline openfeature/flag_evaluation_details.cpp --view expandedRepository: open-feature/cpp-sdk
Length of output: 575
🏁 Script executed:
sed -n '1,120p' openfeature/flag_evaluation_details.cppRepository: open-feature/cpp-sdk
Length of output: 1561
🏁 Script executed:
sed -n '1,200p' openfeature/flag_evaluation_details.h && printf '\n---\n' && sed -n '1,200p' openfeature/resolution_details.hRepository: open-feature/cpp-sdk
Length of output: 3727
Include <utility> for std::move. This translation unit depends on transitive includes here; add the direct include so it stays portable.
Proposed fix
`#include` <string_view>
+#include <utility>📝 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.
| #include <optional> | |
| #include <string> | |
| #include <string_view> | |
| `#include` <optional> | |
| `#include` <string> | |
| `#include` <string_view> | |
| `#include` <utility> |
🤖 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/flag_evaluation_details.cpp` around lines 3 - 5, Add a direct
<utility> include in flag_evaluation_details.cpp alongside the existing
standard-library includes so the translation unit explicitly provides std::move
without relying on transitive includes.
| public: | ||
| virtual ~FeatureProvider() = default; | ||
| virtual Metadata GetMetadata() const = 0; | ||
| virtual std::vector<std::shared_ptr<BaseHook>> GetHooks() const = 0; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo root: '; pwd
printf '\nFiles of interest:\n'
git ls-files | rg '^openfeature/(provider\.h|.*provider.*|.*hook.*|.*mock.*|.*test.*|README|CHANGELOG|VERSION|docs/)'
printf '\n--- openfeature/provider.h (numbered) ---\n'
cat -n openfeature/provider.h
printf '\n--- Search for FeatureProvider implementations / subclasses ---\n'
rg -n "class\s+\w+\s*:\s*public\s+FeatureProvider|struct\s+\w+\s*:\s*public\s+FeatureProvider|FeatureProvider\s*{" openfeature . --glob '!**/build/**' --glob '!**/dist/**' --glob '!**/node_modules/**'
printf '\n--- Search for GetHooks usage/overrides ---\n'
rg -n "GetHooks\s*\(" openfeature .
printf '\n--- Search for version / breaking-change notes ---\n'
rg -n "breaking change|major version|semver|compatib|migration|GetHooks|hooks" README* CHANGELOG* docs openfeature . --glob '!**/build/**' --glob '!**/dist/**' --glob '!**/node_modules/**'Repository: open-feature/cpp-sdk
Length of output: 3873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- FeatureProvider subclasses in repo ---\n'
rg -n "public FeatureProvider" openfeature
printf '\n--- GetHooks overrides/usages ---\n'
rg -n "GetHooks\s*\(" openfeature
printf '\n--- Provider implementations ---\n'
sed -n '1,220p' openfeature/noop_provider.h
printf '\n--- openfeature/noop_provider.cpp ---\n'
sed -n '1,240p' openfeature/noop_provider.cpp
printf '\n--- openfeature/memory_provider/in_memory_provider.h ---\n'
sed -n '1,220p' openfeature/memory_provider/in_memory_provider.h
printf '\n--- openfeature/memory_provider/in_memory_provider.cpp ---\n'
sed -n '1,260p' openfeature/memory_provider/in_memory_provider.cpp
printf '\n--- Docs mentioning compatibility / breaking changes / hooks ---\n'
rg -n "breaking change|major version|semver|compatib|migration|hooks" README* CHANGELOG* docs openfeature .Repository: open-feature/cpp-sdk
Length of output: 25219
Avoid a breaking FeatureProvider change.
Adding GetHooks() as pure virtual makes downstream FeatureProvider implementations abstract and breaks source compatibility. If that break is intentional, document it as a major API change and add migration guidance; otherwise provide a default empty implementation.
🤖 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/provider.h` at line 28, Update FeatureProvider::GetHooks() to
provide a default empty implementation instead of remaining pure virtual,
preserving source compatibility for downstream providers while allowing
providers to override it when needed.
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/openfeature_api.cpp`:
- Around line 88-95: Update OpenFeatureAPI::AddHooks to acquire hooks_mutex_
with a std::unique_lock before reserving or appending to hooks_, matching the
synchronization used by AddHook; keep the existing null filtering and move
behavior unchanged.
🪄 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: a729b14d-7df5-4c6e-aedb-9f2fd1dbdc86
📒 Files selected for processing (4)
openfeature/openfeature.hopenfeature/openfeature_api.cppopenfeature/openfeature_api.htest/openfeature_api_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- openfeature/openfeature.h
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
This PR
Shutdownmethod to also clean hooks when it is called.SetEvaluationContextandGetEvaluationContextsince they were missing.Fixes #122