feat: supply bound domain to provider initialization#1982
feat: supply bound domain to provider initialization#1982jonathannorris wants to merge 10 commits into
Conversation
|
Warning Review limit reached
Next review available in: 31 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 (1)
📝 WalkthroughWalkthroughAdds domain-aware provider initialization, exposes domain-scoped provider metadata, propagates bound domains through provider setup, and updates tests for the new two-argument initialization contract. ChangesDomain-scoped initialization support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ProviderRepository
participant FeatureProviderStateManager
participant FeatureProvider
Caller->>ProviderRepository: setProvider(domain, provider)
ProviderRepository->>ProviderRepository: validateDomainScopedBinding(domain, provider)
alt provider already bound to another domain
ProviderRepository-->>Caller: throw IllegalArgumentException
else binding allowed
ProviderRepository->>FeatureProviderStateManager: initialize(context, domain)
FeatureProviderStateManager->>FeatureProvider: initialize(context, domain)
end
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1982 +/- ##
============================================
+ Coverage 92.23% 93.33% +1.10%
- Complexity 669 692 +23
============================================
Files 59 59
Lines 1635 1666 +31
Branches 186 192 +6
============================================
+ Hits 1508 1555 +47
+ Misses 80 65 -15
+ Partials 47 46 -1
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
from
e2beebe to
e7ad946
Compare
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>
jonathannorris
force-pushed
the
feat/domain-scoped-provider-init
branch
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
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>
|
|
Addressing the remaining CodeRabbit nitpicks from the review body:
All three inline review threads (Copilot identity check, MultiProvider PR description, |
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