feat: export EvaluationAPI and simplify provider/client its surface#511
Conversation
![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 refactors the OpenFeature Go SDK API by replacing the IEvaluation interface with a concrete EvaluationAPI struct, introducing a streamlined NewClient method with APIOption support, and renaming internal methods like ForEvaluation to resolveBinding. Feedback on these changes highlights two main issues: SetProviderAndWait does not respect context cancellation or timeouts when waiting for provider initialization, which could cause indefinite blocking, and the direct type conversion from apiOptions to ClientMetadata is fragile and should be replaced with explicit initialization.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #511 +/- ##
==========================================
+ Coverage 86.00% 86.55% +0.54%
==========================================
Files 22 22
Lines 2122 2119 -3
==========================================
+ Hits 1825 1834 +9
+ Misses 250 241 -9
+ Partials 47 44 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
erka
force-pushed
the
evaluation-api-type
branch
from
1cc9390 to
42e319b
Compare
|
Looks good overall. Found a few instances where doc comments could be improved. Real exported → unexported leak
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR exports the previously unexported ChangesEvaluationAPI Export & Domain Provider Refactor
Sequence Diagram(s)sequenceDiagram
participant Caller
participant openfeature as openfeature (package)
participant EvaluationAPI
participant domainProviders
participant Client
rect rgba(70, 130, 180, 0.5)
note over Caller,Client: Provider registration
Caller->>openfeature: SetNamedProviderAndWait(ctx, "my-domain", provider)
openfeature->>EvaluationAPI: SetProviderAndWait(ctx, provider, WithDomain("my-domain"))
EvaluationAPI->>EvaluationAPI: setDomainProvider("my-domain", provider)
EvaluationAPI->>domainProviders: domainProviders["my-domain"] = provider
end
rect rgba(60, 179, 113, 0.5)
note over Caller,Client: Client creation and flag evaluation
Caller->>openfeature: NewDefaultClient()
openfeature->>EvaluationAPI: NewClient(WithDomain("my-domain"))
EvaluationAPI-->>Caller: *Client with providerBinding
Caller->>Client: BooleanValue(ctx, "flag-key", false, ...)
Client->>Client: evaluate() calls providerBinding("my-domain")
Client->>domainProviders: resolveBinding("my-domain") returns provider
end
rect rgba(210, 105, 30, 0.5)
note over Caller,Client: Shutdown
Caller->>EvaluationAPI: Shutdown(ctx)
EvaluationAPI->>domainProviders: iterate and shutdown each domain provider
EvaluationAPI->>EvaluationAPI: unbindAllProvidersLocked()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openfeature/openfeature_api.go (2)
394-401:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Shutdown(ctx)can callShutdownmultiple times on the same provider instance.If one provider is bound as default and reused in one or more domains, it is shut down repeatedly. That is unsafe unless every provider implementation is strictly idempotent.
Suggested fix (dedupe providers before shutdown)
func (a *EvaluationAPI) Shutdown(ctx context.Context) error { a.mu.Lock() defer a.mu.Unlock() var errs []error + seen := map[uintptr]struct{}{} + + shouldSkip := func(p FeatureProvider) bool { + if k, ok := providerBindingKey(p); ok { + if _, exists := seen[k]; exists { + return true + } + seen[k] = struct{}{} + } + return false + } // Shutdown default provider if a.defaultProvider != nil { + if shouldSkip(a.defaultProvider) { + goto shutdownDomains + } if contextHandler, ok := a.defaultProvider.(ContextAwareStateHandler); ok { if err := contextHandler.ShutdownWithContext(ctx); err != nil { errs = append(errs, fmt.Errorf("default provider shutdown failed: %w", err)) } } else if stateHandler, ok := a.defaultProvider.(StateHandler); ok { stateHandler.Shutdown() } } +shutdownDomains: // Shutdown all named providers for name, provider := range a.domainProviders { + if shouldSkip(provider) { + continue + } if contextHandler, ok := provider.(ContextAwareStateHandler); ok { if err := contextHandler.ShutdownWithContext(ctx); err != nil { errs = append(errs, fmt.Errorf("named provider %q shutdown failed: %w", name, err)) }🤖 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/openfeature_api.go` around lines 394 - 401, The Shutdown method iterates through a.domainProviders and calls shutdown methods on each provider without deduplication. If the same provider instance is bound to multiple domains or is both a default provider and in domainProviders, it will be shut down multiple times, which is unsafe. Deduplicate the providers before the shutdown loop by collecting all unique provider instances from a.domainProviders into a map keyed by the provider instance itself, then iterate through only the unique instances when calling ShutdownWithContext or Shutdown to ensure each provider is shut down exactly once.
277-281:⚠️ Potential issue | 🟠 MajorUse pointer-key identity for provider comparison in
shutdownOld.Line 280 uses direct interface equality checks (
==andslices.Contains) that can panic whenFeatureProviderholds non-comparable values like maps or slices. The codebase already uses the safe pattern inunbindIfUnreferenced(lines 174-189) withproviderBindingKey()to avoid this. Apply the same approach here.🤖 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/openfeature_api.go` around lines 277 - 281, Replace the direct interface equality checks on line 280 in the shutdownOld function that compare oldProvider using == and slices.Contains with the safe pointer-key identity pattern already implemented in unbindIfUnreferenced. Instead of comparing oldProvider directly, convert both oldProvider and a.defaultProvider to their binding keys using providerBindingKey(), and similarly convert the namedProviders slice to binding keys before checking if the oldProvider's binding key is contained within it. This prevents panics when FeatureProvider holds non-comparable values like maps or slices.
♻️ Duplicate comments (1)
openfeature/openfeature_api.go (1)
89-94:⚠️ Potential issue | 🟠 MajorExported
APIOptionleaks an unexported type in its signature.
APIOption(line 90) is public, but its function type requires*apiOptions(line 92, unexported), exposing an internal type in the public API surface and generated documentation.Suggested fix (interface-with-unexported-method pattern)
-type APIOption func(*apiOptions) +type APIOption interface { + apply(*apiOptions) +} + +type apiOptionFunc func(*apiOptions) + +func (f apiOptionFunc) apply(o *apiOptions) { + f(o) +} @@ func WithDomain(domain string) APIOption { - return func(o *apiOptions) { + return apiOptionFunc(func(o *apiOptions) { o.domain = domain - } + }) }🤖 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/openfeature_api.go` around lines 89 - 94, The exported type `APIOption` has a function signature that directly references the unexported type `apiOptions`, which exposes an internal type in the public API. Refactor `APIOption` from a function type to an interface definition with unexported methods, allowing the implementation details to remain private while keeping the `APIOption` interface public. This way, callers work with the public interface rather than seeing unexported types in the API signature.
🧹 Nitpick comments (4)
openfeature/client.go (1)
52-54: ⚡ Quick winAlign
NewClientdoc with domain terminology.The comment still says “Name is a unique identifier” while the parameter and behavior are domain-scoped.
🤖 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/client.go` around lines 52 - 54, Update the documentation comment for the NewClient function to accurately reflect the domain parameter. Replace the phrase "Name is a unique identifier for this client" with wording that clarifies the domain parameter is a unique identifier for the domain scope, not a client name. Ensure the comment aligns with the actual parameter name "domain" and how it is used with the WithDomain function call.openfeature/openfeature.go (1)
53-55: ⚡ Quick winUpdate
NewDefaultClientdocs to concrete return semantics.The comment still describes the default-domain client as an
IClientinstance. This refactor now consistently returns*Client; docs should match that.🤖 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/openfeature.go` around lines 53 - 55, The documentation comment for the NewDefaultClient function still references IClient, but the function signature now returns *Client, a concrete type rather than an interface. Update the comment above NewDefaultClient to accurately reflect that it returns a *Client instance instead of describing it as an IClient instance. Ensure the documentation clearly states the concrete return type to match the actual function signature.openfeature/openfeature_api.go (1)
304-327: ⚡ Quick winRefresh stale doc comments after named→domain/client API refactor.
Comments in this block still mention “default metadata” for domain lookup, “named providers”, and
IClientreturn type, which no longer match behavior/signatures.🤖 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/openfeature_api.go` around lines 304 - 327, Update the doc comments for the three functions in this block to accurately reflect the current API after the refactor. For getDomainProviderMetadata, change the comment to reflect that it returns metadata for a domain-specific provider based on the provided name parameter, not a default provider. For getDomainProviders, update the comment to reference domain providers instead of named providers. For NewClient, correct the return type mentioned in the comment to specify that it returns a *Client (not IClient), and clarify that it binds to either a named domain provider or a default provider based on the APIOption parameters.openfeature/internal/factory/factory.go (1)
11-13: ⚡ Quick winFix stale symbol reference in comment.
The comment references
openfeature/isolated.CurrentAPI, which is not part of this refactored surface. Please point to the actual consumer(s) or keep the comment package-internal and symbol-agnostic.🤖 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/internal/factory/factory.go` around lines 11 - 13, The comment referencing CurrentAPI variable contains a stale symbol reference to openfeature/isolated.CurrentAPI which no longer exists in the refactored codebase. Update the comment for the CurrentAPI variable to either point to the actual current consumer(s) of this variable, or alternatively, keep the comment package-internal and generic by removing the specific symbol reference to openfeature/isolated.CurrentAPI and instead focus on describing what the CurrentAPI variable does without referencing external consumers that may not exist.
🤖 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/isolated_api_test.go`:
- Around line 24-31: The TestRequirement_1_8_2 function references an undefined
variable `a` in the reflect.TypeOf comparison on line 30, which causes a compile
failure. Before the comparison that checks if reflect.TypeOf(a) does not equal
reflect.TypeOf(b), you need to define and assign a value to variable `a`. Based
on the error message context mentioning "newAPI() and factory.NewAPI() returned
different types", assign `a` to the result of calling newAPI() so that the
comparison between the two can be properly evaluated.
---
Outside diff comments:
In `@openfeature/openfeature_api.go`:
- Around line 394-401: The Shutdown method iterates through a.domainProviders
and calls shutdown methods on each provider without deduplication. If the same
provider instance is bound to multiple domains or is both a default provider and
in domainProviders, it will be shut down multiple times, which is unsafe.
Deduplicate the providers before the shutdown loop by collecting all unique
provider instances from a.domainProviders into a map keyed by the provider
instance itself, then iterate through only the unique instances when calling
ShutdownWithContext or Shutdown to ensure each provider is shut down exactly
once.
- Around line 277-281: Replace the direct interface equality checks on line 280
in the shutdownOld function that compare oldProvider using == and
slices.Contains with the safe pointer-key identity pattern already implemented
in unbindIfUnreferenced. Instead of comparing oldProvider directly, convert both
oldProvider and a.defaultProvider to their binding keys using
providerBindingKey(), and similarly convert the namedProviders slice to binding
keys before checking if the oldProvider's binding key is contained within it.
This prevents panics when FeatureProvider holds non-comparable values like maps
or slices.
---
Duplicate comments:
In `@openfeature/openfeature_api.go`:
- Around line 89-94: The exported type `APIOption` has a function signature that
directly references the unexported type `apiOptions`, which exposes an internal
type in the public API. Refactor `APIOption` from a function type to an
interface definition with unexported methods, allowing the implementation
details to remain private while keeping the `APIOption` interface public. This
way, callers work with the public interface rather than seeing unexported types
in the API signature.
---
Nitpick comments:
In `@openfeature/client.go`:
- Around line 52-54: Update the documentation comment for the NewClient function
to accurately reflect the domain parameter. Replace the phrase "Name is a unique
identifier for this client" with wording that clarifies the domain parameter is
a unique identifier for the domain scope, not a client name. Ensure the comment
aligns with the actual parameter name "domain" and how it is used with the
WithDomain function call.
In `@openfeature/internal/factory/factory.go`:
- Around line 11-13: The comment referencing CurrentAPI variable contains a
stale symbol reference to openfeature/isolated.CurrentAPI which no longer exists
in the refactored codebase. Update the comment for the CurrentAPI variable to
either point to the actual current consumer(s) of this variable, or
alternatively, keep the comment package-internal and generic by removing the
specific symbol reference to openfeature/isolated.CurrentAPI and instead focus
on describing what the CurrentAPI variable does without referencing external
consumers that may not exist.
In `@openfeature/openfeature_api.go`:
- Around line 304-327: Update the doc comments for the three functions in this
block to accurately reflect the current API after the refactor. For
getDomainProviderMetadata, change the comment to reflect that it returns
metadata for a domain-specific provider based on the provided name parameter,
not a default provider. For getDomainProviders, update the comment to reference
domain providers instead of named providers. For NewClient, correct the return
type mentioned in the comment to specify that it returns a *Client (not
IClient), and clarify that it binds to either a named domain provider or a
default provider based on the APIOption parameters.
In `@openfeature/openfeature.go`:
- Around line 53-55: The documentation comment for the NewDefaultClient function
still references IClient, but the function signature now returns *Client, a
concrete type rather than an interface. Update the comment above
NewDefaultClient to accurately reflect that it returns a *Client instance
instead of describing it as an IClient instance. Ensure the documentation
clearly states the concrete return type to match the actual function signature.
🪄 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: aae0fc26-88bb-4f6e-8a15-86678bd3fff0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
go.modopenfeature/client.goopenfeature/client_test.goopenfeature/event_executor_test.goopenfeature/interfaces.goopenfeature/interfaces_mock.goopenfeature/internal/factory/factory.goopenfeature/isolated/isolated.goopenfeature/isolated/isolated_example_test.goopenfeature/isolated/isolated_test.goopenfeature/isolated_api_test.goopenfeature/main_test.goopenfeature/openfeature.goopenfeature/openfeature_api.goopenfeature/openfeature_api_test.goopenfeature/openfeature_test.go
💤 Files with no reviewable changes (2)
- go.mod
- openfeature/interfaces_mock.go
- Rename unexported evaluationAPI into exported EvaluationAPI - Replace evaluationImpl interface with providerBindingFn function type - Deprecated IEvaluation interface - Remove deprecated `GetApiInstance() and SetLogger() - Consolidate provider methods: SetProvider(ctx, provider, opts...) with WithDomain() replacing SetNamedProvider* and SetProviderWithContext* - Consolidate client creation: NewClient(opts...) replacing GetClient + GetNamedClient - Add tests for new API surface and isolated instance example Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Co-authored-by: Sahid Velji <sahidvelji@gmail.com> Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
erka
force-pushed
the
evaluation-api-type
branch
from
8db298d to
ae884a6
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openfeature/openfeature_api.go (1)
390-410:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent duplicate shutdown calls for shared provider instances.
On Line 402, shutdown is executed once for the default provider and again for each domain provider. If the same provider instance is bound in multiple places,
Shutdown/ShutdownWithContextcan run multiple times on the same object.💡 Suggested fix
func (a *EvaluationAPI) Shutdown(ctx context.Context) error { a.mu.Lock() defer a.mu.Unlock() var errs []error + seen := map[uintptr]struct{}{} + shutdownProvider := func(name string, provider FeatureProvider, isDefault bool) { + if provider == nil { + return + } + if key, ok := providerBindingKey(provider); ok { + if _, exists := seen[key]; exists { + return + } + seen[key] = struct{}{} + } + if contextHandler, ok := provider.(ContextAwareStateHandler); ok { + if err := contextHandler.ShutdownWithContext(ctx); err != nil { + if isDefault { + errs = append(errs, fmt.Errorf("default provider shutdown failed: %w", err)) + } else { + errs = append(errs, fmt.Errorf("domain provider %q shutdown failed: %w", name, err)) + } + } + return + } + if stateHandler, ok := provider.(StateHandler); ok { + stateHandler.Shutdown() + } + } - // Shutdown default provider - if a.defaultProvider != nil { - if contextHandler, ok := a.defaultProvider.(ContextAwareStateHandler); ok { - if err := contextHandler.ShutdownWithContext(ctx); err != nil { - errs = append(errs, fmt.Errorf("default provider shutdown failed: %w", err)) - } - } else if stateHandler, ok := a.defaultProvider.(StateHandler); ok { - stateHandler.Shutdown() - } - } + shutdownProvider("", a.defaultProvider, true) - // Shutdown all named providers for name, provider := range a.domainProviders { - if contextHandler, ok := provider.(ContextAwareStateHandler); ok { - if err := contextHandler.ShutdownWithContext(ctx); err != nil { - errs = append(errs, fmt.Errorf("named provider %q shutdown failed: %w", name, err)) - } - } else if stateHandler, ok := provider.(StateHandler); ok { - stateHandler.Shutdown() - } + shutdownProvider(name, provider, false) }🤖 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/openfeature_api.go` around lines 390 - 410, The current shutdown logic in the default provider and domain providers blocks can call Shutdown or ShutdownWithContext multiple times on the same provider instance if a provider is configured both as the default provider and as a named domain provider. To fix this, track which provider instances have already been shut down by comparing their identity. Before shutting down each domain provider in the loop over a.domainProviders, check if that provider instance is the same as a.defaultProvider and skip it if so, or maintain a set of already-processed provider instances to avoid duplicate shutdown calls on the same object.
🧹 Nitpick comments (2)
openfeature/openfeature_api.go (2)
365-370: ⚡ Quick winClone hook slices before returning to avoid shared mutable backing arrays.
getHooksandresolveBindingboth exposea.hksdirectly. Returning clones prevents accidental mutation of API-level hooks by downstream callers.💡 Suggested fix
func (a *EvaluationAPI) getHooks() []Hook { a.mu.RLock() defer a.mu.RUnlock() - return a.hks + return slices.Clone(a.hks) } func (a *EvaluationAPI) resolveBinding(domain string) (FeatureProvider, []Hook, EvaluationContext) { a.mu.RLock() defer a.mu.RUnlock() @@ - return provider, a.hks, a.evalCtx + return provider, slices.Clone(a.hks), a.evalCtx }Also applies to: 442-454
🤖 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/openfeature_api.go` around lines 365 - 370, The getHooks method returns a.hks directly, exposing the internal slice to potential mutation by callers. Create a clone of the a.hks slice and return the copy instead of the original reference to prevent accidental mutations of API-level hooks. Apply the same fix to the resolveBinding method (mentioned in the comment as also applying to lines 442-454) to ensure consistency across all methods that expose hook slices.
324-330: ⚡ Quick winReturn a defensive copy from
getDomainProviders.Line 329 currently returns the internal map directly, which allows callers to mutate API state after the lock is released.
💡 Suggested fix
func (a *EvaluationAPI) getDomainProviders() map[string]FeatureProvider { a.mu.RLock() defer a.mu.RUnlock() - return a.domainProviders + return maps.Clone(a.domainProviders) }🤖 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/openfeature_api.go` around lines 324 - 330, The getDomainProviders method in EvaluationAPI returns a direct reference to the internal a.domainProviders map, which allows callers to mutate the API's internal state even after the read lock is released. To fix this, create a defensive copy of the a.domainProviders map before returning it. Iterate through the original map and populate a new map with the same key-value pairs, then return the new map instead of the original reference. This ensures the returned map is independent and any modifications by callers will not affect the API's internal state.
🤖 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/internal/factory/factory.go`:
- Around line 11-13: The comment references openfeature/isolated.CurrentAPI
which is stale and does not reflect the current API surface. Update the comment
to reference the correct API that is actually available in the isolated package,
likely openfeature/isolated.NewAPI or another appropriate call site, ensuring
the documentation accurately reflects what maintainers should actually
reference.
In `@openfeature/openfeature_test.go`:
- Around line 406-414: The second resolveBinding assertion for "clientB" has a
hardcoded providerA reference in the error message instead of using the actual
resolved provider variable. In the t.Errorf call on line 413, replace the
hardcoded providerA.Metadata().Name with provider.Metadata().Name to show the
actual value that was resolved. Apply the same fix to the similar assertions
mentioned at lines 433-436 where the same copy-paste error likely exists.
---
Outside diff comments:
In `@openfeature/openfeature_api.go`:
- Around line 390-410: The current shutdown logic in the default provider and
domain providers blocks can call Shutdown or ShutdownWithContext multiple times
on the same provider instance if a provider is configured both as the default
provider and as a named domain provider. To fix this, track which provider
instances have already been shut down by comparing their identity. Before
shutting down each domain provider in the loop over a.domainProviders, check if
that provider instance is the same as a.defaultProvider and skip it if so, or
maintain a set of already-processed provider instances to avoid duplicate
shutdown calls on the same object.
---
Nitpick comments:
In `@openfeature/openfeature_api.go`:
- Around line 365-370: The getHooks method returns a.hks directly, exposing the
internal slice to potential mutation by callers. Create a clone of the a.hks
slice and return the copy instead of the original reference to prevent
accidental mutations of API-level hooks. Apply the same fix to the
resolveBinding method (mentioned in the comment as also applying to lines
442-454) to ensure consistency across all methods that expose hook slices.
- Around line 324-330: The getDomainProviders method in EvaluationAPI returns a
direct reference to the internal a.domainProviders map, which allows callers to
mutate the API's internal state even after the read lock is released. To fix
this, create a defensive copy of the a.domainProviders map before returning it.
Iterate through the original map and populate a new map with the same key-value
pairs, then return the new map instead of the original reference. This ensures
the returned map is independent and any modifications by callers will not affect
the API's internal state.
🪄 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: f6da3869-f5a9-4819-96ac-188337d23762
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
go.modopenfeature/client.goopenfeature/client_test.goopenfeature/event_executor_test.goopenfeature/interfaces.goopenfeature/interfaces_mock.goopenfeature/internal/factory/factory.goopenfeature/isolated/isolated.goopenfeature/isolated/isolated_example_test.goopenfeature/isolated/isolated_test.goopenfeature/isolated_api_test.goopenfeature/main_test.goopenfeature/openfeature.goopenfeature/openfeature_api.goopenfeature/openfeature_api_test.goopenfeature/openfeature_test.go
💤 Files with no reviewable changes (2)
- go.mod
- openfeature/interfaces_mock.go
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
| type APIOption interface { | ||
| apply(*apiOptions) | ||
| } | ||
|
|
||
| type ( | ||
| apiOptionFunc func(*apiOptions) | ||
| apiOptions struct { | ||
| domain string | ||
| } | ||
| ) | ||
|
|
||
| func (f apiOptionFunc) apply(o *apiOptions) { f(o) } |
There was a problem hiding this comment.
Initially I thought this was overbuilt, but I can see this way gives us control over apiOptions and will allow us to hide internal changes from customers. Nice.
| func SetProvider(provider FeatureProvider) error { | ||
| return api().SetProvider(provider) | ||
| return api().SetProvider(context.Background(), provider) |
There was a problem hiding this comment.
The v2 tracking issue #401 and #414 / #443 / #409 cover the bigger picture, but I don't see an open issue about the package-level setters (SetProvider, SetNamedProvider, etc.) getting the same options-shape change, which would let singleton users reach WithDomain (and any future option) without going to isolated.NewAPI(). Is that intentional (keep wrappers as legacy forever) or worth a v2 sub-issue for removal?
I'm guessing that's the intent, right?
There was a problem hiding this comment.
I would like to align these methods with the other SDKs (such as JavaScript and .NET), which do not have SetDomain....
JS as an example
// Registering the default provider
OpenFeature.setProvider(new TypedInMemoryProvider(myFlags));
// Registering a provider to a domain
OpenFeature.setProvider('my-domain', new TypedInMemoryProvider(someOtherFlags));I also don't like ...WithContext methods, so it's a trade-off in many ways with golang.
The v2 issues are outdated in some ways, and they would likely need to be revisited and rethought if v2 remains on the roadmap. I don’t see any community interest in v2 right now.
toddbaert
left a comment
toddbaert
left a comment
There was a problem hiding this comment.
I really like the shape. This feels better.
Please see: https://github.com/open-feature/go-sdk/pull/511/changes#r3466966882
Also, should we add README additions about the new isolated stuff and the associated new API here or in a separate PR? WDYT @erka @sahidvelji ?
Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
|
I believe none of the SDKs mention the isolated API in their README. |
3 participants
This PR
WithDomain() replacing SetNamedProviderand SetProviderWithContext
GetNamedClient