feat: implement hooks lifecycle (spec section 4)#214
Conversation
Summary of ChangesHello, 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 significantly enhances the OpenFeature SDK by introducing a robust and spec-compliant hooks lifecycle. It allows developers to inject custom logic at various stages of flag evaluation, from before the evaluation to after, error handling, and final cleanup. This feature provides greater flexibility and control over how feature flags are processed, enabling advanced use cases like context modification, metric collection, and error recovery, all while maintaining performance with an optimized execution path. Highlights
Changelog
Activity
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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #214 +/- ##
==========================================
+ Coverage 99.59% 99.64% +0.05%
==========================================
Files 22 26 +4
Lines 491 571 +80
==========================================
+ Hits 489 569 +80
Misses 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request implements the full OpenFeature hooks lifecycle, which is a major feature. The implementation is well-structured, following the specification closely. New components like HookExecutor, HookContext, and the Hook module are introduced, and they are integrated into the Client's flag evaluation process. The inclusion of a fast path for when no hooks are present is a good performance consideration. The code is accompanied by a comprehensive suite of unit and specification compliance tests, which is excellent. I have one suggestion regarding the immutability of default_value in the HookContext to fully align with the specification. Overall, this is a high-quality contribution.
Introduce the core hook abstractions per OpenFeature spec section 4: - Hook module (hooks/hook.rb): provides default no-op implementations for before, after, error, and finally stages. Hooks include this module and override the stages they care about (spec 4.3.1). - HookContext class (hooks/hook_context.rb): immutable flag_key and flag_value_type with mutable evaluation_context, plus optional client_metadata and provider_metadata (spec 4.1.1-4.1.5). - Barrel require file (hooks.rb): loads all hooks components. - Move `require "delegate"` into hints.rb where it is actually needed. Closes #52 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Jose Colella <jose.colella@gusto.com>
Implements the hook execution engine per OpenFeature spec 4.4: - Before hooks run in order: API → Client → Invocation → Provider - After/Error/Finally hooks run in reverse order - Before hooks may return EvaluationContext for merging (spec 4.3.4) - Error in before/after → runs error hooks, returns default (spec 4.4.5-7) - Error/finally hook failures are logged, remaining hooks still run (spec 4.4.3-4) - Uses reverse_each to avoid intermediate array allocations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Jose Colella <jose.colella@gusto.com>
Wire hooks into the evaluation flow: - Add `require_relative "hooks"` to api.rb - Update Client method signatures to accept hooks: and hook_hints: - Assemble hooks in spec order: API → Client → Invocation → Provider - Wrap provider calls with HookExecutor when hooks are present - Fast path: skip hook ceremony entirely when no hooks are registered - Extract evaluate_flag method shared between fast and hooks paths - Use frozen EMPTY_HINTS constant for the common no-hints case - Use splat array assembly to avoid intermediate allocations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Jose Colella <jose.colella@gusto.com>
Add comprehensive test coverage for the hooks feature: Unit tests: - hook_spec.rb: default no-ops, selective override behavior - hook_context_spec.rb: immutability, optional fields, mutable context - hook_executor_spec.rb: ordering, error handling, context propagation Specification compliance (hooks_spec.rb): - 4.1.x: HookContext fields, immutability, mutability - 4.3.x: Stage execution timing, context merging, error/finally - 4.4.x: Hook ordering, error isolation, default value on failure - 4.5.x: API/Client/invocation hook registration Also: - Replace pending tests for Requirements 1.1.4 and 1.2.1 - Extract shared recording_hook helper to spec/support/ Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Jose Colella <jose.colella@gusto.com>
a310240 to
7cf9c76
Compare
Address review feedback: - Freeze default_value in HookContext to enforce immutability per spec 4.1.3, preventing hooks from mutating mutable default values like Hash or Array. - Add test for frozen default_value with a mutable object type. - Add test for hook_hints Hash conversion to cover the Hints construction branch flagged by Codecov. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Jose Colella <jose.colella@gusto.com>
Add test for passing a pre-built Hooks::Hints instance as hook_hints, covering line 82 in client.rb where the Hints object is passed through directly without conversion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Jose Colella <jose.colella@gusto.com>
Tip
Best reviewed commit-by-commit
Summary
Hookinterface module,HookContextclass, andHookExecutorengineClientflag evaluation with proper ordering (API → Client → Invocation → Provider for before; reverse for after/error/finally)Closes #52
Test plan
bundle exec rake)spec/open_feature/sdk/hooks/spec/specification/hooks_spec.rbspec/open_feature/sdk/client_spec.rb,spec/specification/flag_evaluation_api_spec.rb🤖 Jose's AI agent