feat: supply bound domain to provider initialization#1982
feat: supply bound domain to provider initialization#1982jonathannorris wants to merge 10 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughAdds domain-aware provider initialization, exposes domain-scoped provider metadata, propagates bound domains through provider setup and multi-provider initialization, enforces scoped binding rules, and expands compatibility and integration tests. ChangesDomain-scoped initialization support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1982 +/- ##
============================================
+ Coverage 92.31% 92.97% +0.65%
- Complexity 706 727 +21
============================================
Files 60 60
Lines 1705 1736 +31
Branches 195 201 +6
============================================
+ Hits 1574 1614 +40
+ Misses 82 73 -9
Partials 49 49
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:
|
jonathannorris
force-pushed
the
feat/domain-scoped-provider-init
branch
2 times, most recently
from
e7ad946 to
4a23ee9
Compare
There was a problem hiding this comment.
Pull request overview
Adds domain-awareness to provider initialization and introduces an opt-in “domain-scoped” provider declaration that the SDK enforces to prevent a single provider instance from being bound to multiple domains, aligning the Java SDK with updated OpenFeature spec requirements.
Changes:
- Add
FeatureProvider.initialize(EvaluationContext, @Nullable String domain)default method andisDomainScoped()declaration. - Pass the bound domain through provider initialization paths (repository/state manager, MultiProvider) and enforce domain-scoped single-domain binding.
- Add/adjust conformance and backward-compatibility tests for legacy single-arg providers and domain-scoped binding rules.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/dev/openfeature/sdk/FeatureProvider.java | Adds optional domain initialize overload + isDomainScoped() declaration. |
| src/main/java/dev/openfeature/sdk/ProviderRepository.java | Passes domain into initialization and enforces domain-scoped binding constraints. |
| src/main/java/dev/openfeature/sdk/FeatureProviderStateManager.java | Forwards domain into provider initialization while preserving one-time init behavior. |
| src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java | Adds two-arg initialize and forwards domain during child initialization. |
| src/test/java/dev/openfeature/sdk/InitializeBehaviorSpecTest.java | Updates spec tests to assert domain (or null) is passed on init. |
| src/test/java/dev/openfeature/sdk/DomainScopedProviderSpecTest.java | Adds spec tests for domain-scoped binding restrictions and domain receipt. |
| src/test/java/dev/openfeature/sdk/ProviderInitializeBackwardCompatibilityTest.java | Adds explicit backward-compat coverage for legacy single-arg providers. |
| src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java | Updates repository tests to expect two-arg initialize with proper domain/null. |
| src/test/java/dev/openfeature/sdk/vmlens/ProviderRepositoryCT.java | Updates concurrency tests to stub/verify two-arg initialize. |
| src/test/java/dev/openfeature/sdk/OpenFeatureAPITest.java | Updates API tests to verify two-arg initialize is called with null domain. |
| src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderTest.java | Adds tests for MultiProvider domain forwarding and legacy child providers. |
| src/test/java/dev/openfeature/sdk/FeatureProviderStateManagerTest.java | Adds test ensuring domain is forwarded into delegate init. |
| src/test/java/dev/openfeature/sdk/e2e/steps/ProviderSteps.java | Updates e2e mocking to stub two-arg initialize for named clients. |
| src/test/java/dev/openfeature/sdk/fixtures/ProviderFixture.java | Updates fixture to throw from two-arg initialize (nullable domain). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🧹 Nitpick comments (5)
src/test/java/dev/openfeature/sdk/InitializeBehaviorSpecTest.java (2)
93-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate test body flagged by SonarCloud (named-provider case).
mustPassBoundDomainWhenInitializingANamedProviderduplicatesmustCallInitializeFunctionOfTheNewlyRegisteredNamedProviderBeforeUsingItForFlagEvaluationverbatim. Same consolidation suggestion as the default-provider pair above applies here.Also applies to: 109-115
🤖 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 `@src/test/java/dev/openfeature/sdk/InitializeBehaviorSpecTest.java` around lines 93 - 101, The named-provider test body is duplicated and should be consolidated to match the shared pattern used for the default-provider case. Update mustPassBoundDomainWhenInitializingANamedProvider and mustCallInitializeFunctionOfTheNewlyRegisteredNamedProviderBeforeUsingItForFlagEvaluation in InitializeBehaviorSpecTest so only one test covers the initialize-on-set behavior, or extract the common setup/assertion into a shared helper and keep distinct assertions only where behavior differs.Source: Linters/SAST tools
40-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate test body flagged by SonarCloud.
mustPassNullDomainWhenInitializingTheDefaultProvideris byte-identical tomustCallInitializeFunctionOfTheNewlyRegisteredProviderBeforeUsingItForFlagEvaluationabove it. Consider extracting the shared assertion into a private helper so both@Specification-tagged tests stay for traceability without duplicated code.♻️ Suggested consolidation
+ private void assertInitializeCalledWithDomain(FeatureProvider featureProvider, Object domainMatcher) { + // shared verification helper + }Or simply merge both
@Specificationannotations onto a single test method.Also applies to: 55-61
🤖 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 `@src/test/java/dev/openfeature/sdk/InitializeBehaviorSpecTest.java` around lines 40 - 47, The two `@Specification` tests in InitializeBehaviorSpecTest are duplicate bodies, so consolidate the shared setup/assertion into a private helper or merge the annotations onto one test method. Keep the existing test names or metadata only as needed for traceability, and update both mustCallInitializeFunctionOfTheNewlyRegisteredProviderBeforeUsingItForFlagEvaluation and mustPassNullDomainWhenInitializingTheDefaultProvider to delegate to the shared logic instead of repeating the same mock/verify sequence.Source: Linters/SAST tools
src/test/java/dev/openfeature/sdk/DomainScopedProviderSpecTest.java (2)
95-110: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't verify DOMAIN_B initialization was actually invoked.
allowsBindingNonDomainScopedProviderToMultipleDomainsonly verifiesinitialize(any(), eq(DOMAIN_A)). To actually confirm the provider was initialized for both domains (the point of the test), also assert the DOMAIN_B call occurred.♻️ Suggested fix
assertThat(api.getProvider(DOMAIN_A)).isSameAs(provider); assertThat(api.getProvider(DOMAIN_B)).isSameAs(provider); verify(provider, times(1)).initialize(any(), eq(DOMAIN_A)); + verify(provider, times(1)).initialize(any(), eq(DOMAIN_B));🤖 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 `@src/test/java/dev/openfeature/sdk/DomainScopedProviderSpecTest.java` around lines 95 - 110, The test allows a non-domain-scoped provider to be bound to two domains, but it only verifies initialization for DOMAIN_A. Update allowsBindingNonDomainScopedProviderToMultipleDomains in DomainScopedProviderSpecTest to also assert that the provider’s initialize path was invoked for DOMAIN_B, using the existing provider mock and initialization verification so the test confirms both domain bindings.
27-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing teardown to shut down
OpenFeatureAPIinstances.Each test constructs a new
OpenFeatureAPI()instance in@BeforeEach, andsetProviderdispatches initialization work to an internal executor owned by that instance. Without an@AfterEachcallingapi.shutdown(), each test leaves behind an unshut-down executor/thread pool for the lifetime of the JVM running the suite.♻️ Suggested fix
`@BeforeEach` void setupTest() { api = new OpenFeatureAPI(); api.setProvider(new NoOpProvider()); } + + `@AfterEach` + void tearDown() { + api.shutdown(); + }🤖 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 `@src/test/java/dev/openfeature/sdk/DomainScopedProviderSpecTest.java` around lines 27 - 31, The test setup creates a new OpenFeatureAPI and assigns a provider, but the instance-owned executor is never shut down, leaving background threads running after each test. Add an `@AfterEach` teardown in DomainScopedProviderSpecTest that calls api.shutdown() on the same api initialized in setupTest, and ensure this cleanup runs even when setup or assertions fail. Use the existing OpenFeatureAPI and setupTest symbols to place the teardown alongside the current lifecycle methods.src/test/java/dev/openfeature/sdk/ProviderInitializeBackwardCompatibilityTest.java (1)
54-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMisleading test name: asserts count == 2, not "once".
singleArgOverrideIsOnlyInvokedOncecallsprovider.initialize(ctx, DOMAIN)twice directly (bypassing theFeatureProviderStateManageridempotency guard) and asserts the count is2. The name suggests the opposite of what's asserted, and it's easy to conflate with the similarly-namedonlyInvokesLegacySingleArgProviderOncePerStateManagerInittest (lines 142-152), which asserts count== 1via the state-manager guard. Consider renaming to clarify that each direct call to the default two-arg method triggers exactly one delegated single-arg call (e.g.,eachCallDelegatesExactlyOnceToSingleArgOverride).✏️ Suggested rename
- `@DisplayName`("single-arg override is only invoked once per initialization") - void singleArgOverrideIsOnlyInvokedOnce() throws Exception { + `@DisplayName`("each call to the two-arg default delegates exactly once to the single-arg override") + void eachCallDelegatesExactlyOnceToSingleArgOverride() throws Exception {🤖 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 `@src/test/java/dev/openfeature/sdk/ProviderInitializeBackwardCompatibilityTest.java` around lines 54 - 63, Rename the misleading test in ProviderInitializeBackwardCompatibilityTest so its name matches the asserted behavior: the current singleArgOverrideIsOnlyInvokedOnce method calls initialize(new ImmutableContext(), DOMAIN) twice and expects singleArgInitCount() to be 2, so update the test name to reflect that each direct two-arg initialize call delegates exactly once to the single-arg override. Make sure it stays clearly distinct from the state-manager-backed onlyInvokesLegacySingleArgProviderOncePerStateManagerInit 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 `@src/main/java/dev/openfeature/sdk/FeatureProvider.java`:
- Line 5: The production code in FeatureProvider imports
javax.annotation.Nullable, but the project is missing a compile-scope dependency
that provides it, which can break the main module build. Update pom.xml to add
jsr305 or an equivalent compile-time dependency so the Nullable import in
FeatureProvider resolves during production compilation and is available without
relying on test-only or transitive scope.
---
Nitpick comments:
In `@src/test/java/dev/openfeature/sdk/DomainScopedProviderSpecTest.java`:
- Around line 95-110: The test allows a non-domain-scoped provider to be bound
to two domains, but it only verifies initialization for DOMAIN_A. Update
allowsBindingNonDomainScopedProviderToMultipleDomains in
DomainScopedProviderSpecTest to also assert that the provider’s initialize path
was invoked for DOMAIN_B, using the existing provider mock and initialization
verification so the test confirms both domain bindings.
- Around line 27-31: The test setup creates a new OpenFeatureAPI and assigns a
provider, but the instance-owned executor is never shut down, leaving background
threads running after each test. Add an `@AfterEach` teardown in
DomainScopedProviderSpecTest that calls api.shutdown() on the same api
initialized in setupTest, and ensure this cleanup runs even when setup or
assertions fail. Use the existing OpenFeatureAPI and setupTest symbols to place
the teardown alongside the current lifecycle methods.
In `@src/test/java/dev/openfeature/sdk/InitializeBehaviorSpecTest.java`:
- Around line 93-101: The named-provider test body is duplicated and should be
consolidated to match the shared pattern used for the default-provider case.
Update mustPassBoundDomainWhenInitializingANamedProvider and
mustCallInitializeFunctionOfTheNewlyRegisteredNamedProviderBeforeUsingItForFlagEvaluation
in InitializeBehaviorSpecTest so only one test covers the initialize-on-set
behavior, or extract the common setup/assertion into a shared helper and keep
distinct assertions only where behavior differs.
- Around line 40-47: The two `@Specification` tests in InitializeBehaviorSpecTest
are duplicate bodies, so consolidate the shared setup/assertion into a private
helper or merge the annotations onto one test method. Keep the existing test
names or metadata only as needed for traceability, and update both
mustCallInitializeFunctionOfTheNewlyRegisteredProviderBeforeUsingItForFlagEvaluation
and mustPassNullDomainWhenInitializingTheDefaultProvider to delegate to the
shared logic instead of repeating the same mock/verify sequence.
In
`@src/test/java/dev/openfeature/sdk/ProviderInitializeBackwardCompatibilityTest.java`:
- Around line 54-63: Rename the misleading test in
ProviderInitializeBackwardCompatibilityTest so its name matches the asserted
behavior: the current singleArgOverrideIsOnlyInvokedOnce method calls
initialize(new ImmutableContext(), DOMAIN) twice and expects
singleArgInitCount() to be 2, so update the test name to reflect that each
direct two-arg initialize call delegates exactly once to the single-arg
override. Make sure it stays clearly distinct from the state-manager-backed
onlyInvokesLegacySingleArgProviderOncePerStateManagerInit 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: 0adbf73e-e6cd-4ba9-bd5d-9c956a5fc975
📒 Files selected for processing (14)
src/main/java/dev/openfeature/sdk/FeatureProvider.javasrc/main/java/dev/openfeature/sdk/FeatureProviderStateManager.javasrc/main/java/dev/openfeature/sdk/ProviderRepository.javasrc/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.javasrc/test/java/dev/openfeature/sdk/DomainScopedProviderSpecTest.javasrc/test/java/dev/openfeature/sdk/FeatureProviderStateManagerTest.javasrc/test/java/dev/openfeature/sdk/InitializeBehaviorSpecTest.javasrc/test/java/dev/openfeature/sdk/OpenFeatureAPITest.javasrc/test/java/dev/openfeature/sdk/ProviderInitializeBackwardCompatibilityTest.javasrc/test/java/dev/openfeature/sdk/ProviderRepositoryTest.javasrc/test/java/dev/openfeature/sdk/e2e/steps/ProviderSteps.javasrc/test/java/dev/openfeature/sdk/fixtures/ProviderFixture.javasrc/test/java/dev/openfeature/sdk/multiprovider/MultiProviderTest.javasrc/test/java/dev/openfeature/sdk/vmlens/ProviderRepositoryCT.java
|
Addressing the remaining CodeRabbit nitpicks from the review body:
All three inline review threads (Copilot identity check, MultiProvider PR description, |
| * @param evaluationContext the global evaluation context | ||
| * @param domain the bound domain, or {@code null} for the default provider | ||
| */ | ||
| default void initialize(EvaluationContext evaluationContext, @Nullable String domain) throws Exception { |
There was a problem hiding this comment.
Same as JS, we could consider using an object here to future-proof (something like InitializationContext, comparable to HookContext). IMO not a blocker but worth thinking about before we commit to the two-arg overload.
| } | ||
| } | ||
|
|
||
| private void validateDomainScopedBinding(@Nullable String domain, FeatureProvider newProvider) { |
There was a problem hiding this comment.
Again, same as JS, might be better to extract this and implement it in one check. Something like a single upfront predicate that inspects defaultStateManger and stateManagers for this exact instance and rejects if any existing binding differs from the incoming one. More lines but keeps the rule in one place. Totally optional.
| assertThatThrownBy(() -> api.setProvider(DOMAIN_B, provider)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessageContaining("Domain-scoped provider cannot be bound to more than one domain"); | ||
| } |
There was a problem hiding this comment.
The three rejection tests (rejectsBindingDomainScopedProviderToSecondNamedDomain, rejectsBindingDomainScopedNamedProviderAsDefault, rejectsBindingDomainScopedDefaultProviderToNamedDomain) only verify the exception is thrown. IMO we should also assert initialize is not re-run after the rejection.
|
|
||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import javax.annotation.Nullable; |
There was a problem hiding this comment.
We removed javax.annotation.* in main sources a while back (see #921), and there are no such imports anywhere in src/main/java/ on main today (there's one in /test I think we should even remove). The runtime generally ignores the annotation if it's not available on classpath, but there are still ways to get ClassNotFounds if no jsr305 impl is found.
jsr305 is abandoned, so I'd rather not have FeatureProvider reference it at all. The Javadoc at line 56 already documents that domain can be null, which is what we do elsewhere in src/main.
I'd suggest dropping the annotation here (and the internal ones in ProviderRepository / FeatureProviderStateManager / MultiProvider as well).
| } | ||
|
|
||
| /** | ||
| * Initialize the provider with the bound domain, if any. |
There was a problem hiding this comment.
These docs belong in the interface
| * @param evaluationContext the global evaluation context | ||
| * @param domain the bound domain, or {@code null} for the default provider | ||
| */ | ||
| default void initialize(EvaluationContext evaluationContext, @Nullable String domain) throws Exception { |
There was a problem hiding this comment.
Are we sure that the addition of this default method is not a breaking change on a byte code level?
There was a problem hiding this comment.
Rebased against our recent additions for byecode/sourcecode change detection, and passes, so I guess no problem with this.
| } | ||
| } | ||
|
|
||
| private void validateDomainScopedBinding(@Nullable String domain, FeatureProvider newProvider) { |
There was a problem hiding this comment.
Is the functionality of this method already covered by a VmLens test? If not, we should add one to guard against registering two providers to the same domain concurrently
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
toddbaert
force-pushed
the
feat/domain-scoped-provider-init
branch
from
68fa5da to
362bcce
Compare
|
Summary
domainparameter to providerinitializeand pass the bound domain from the provider mutator (spec 1.1.2.2, 2.4.1)isDomainScoped()provider declaration and reject binding a domain-scoped instance to more than one domain (spec 2.4.3, 1.1.8.1)domainthroughMultiProviderinitializeto each child providerinitializeMotivation
Unblocks OFREP static-context providers that need the bound
domainat init time to scope persisted cache keys per open-feature/spec#393 and protocol ADR 0009.Parallel implementation in js-sdk: open-feature/js-sdk#1433.
Notes
domainis an optional second parameter via a default method onFeatureProviderinitializeruns once)initialize(context)remain compatible; the extradomainargument is ignored if unusednulldomain; named clients receive the bound domain stringRelated Issues
Fixes #1981
Relates to: open-feature/spec#393, open-feature/js-sdk#1433
Test plan
InitializeBehaviorSpecTestverifies domain is passed at init (2.4.1)DomainScopedProviderSpecTestcovers 1.1.8.1 and 2.4.3mvn testpasses after rebase ontomain