feat(core): pass bound domain to provider initialize and enforce domain-scoped binding#1433
feat(core): pass bound domain to provider initialize and enforce domain-scoped binding#1433jonathannorris wants to merge 8 commits into
Conversation
|
Warning Review limit reached
Next review available in: 28 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)
📝 WalkthroughWalkthroughProvider initialization now carries an optional domain through shared contracts, OpenFeature binding, and server/web MultiProvider paths. Tests add coverage for legacy single-argument providers and domain-scoped binding rules. ChangesProvider domain handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
jonathannorris
force-pushed
the
feat/ofrep-domain-cache-key
branch
from
0614414 to
ad42990
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK provider lifecycle to support domain-aware initialization by adding an optional domain argument to Provider.initialize, forwarding it through MultiProvider, and enforcing that domainScoped provider instances cannot be bound to multiple domains. It also adds conformance tests (web + server) and introduces a shared legacy-provider test helper to validate backwards compatibility with single-argument initialize implementations.
Changes:
- Extend
Provider.initializeto accept(context?, domain?)and pass the bound domain during provider registration/initialization. - Add
domainScopedprovider declaration and reject binding the same domain-scoped instance to more than one domain. - Forward
domainthrough web/serverMultiProvider.initializeand add conformance + legacy compatibility tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/provider/provider.ts | Adds domainScoped to provider interface and updates initialize signature/docs. |
| packages/shared/src/open-feature.ts | Enforces domain-scoped single-domain binding and forwards (context, domain) into provider initialization. |
| packages/web/src/provider/multi-provider/multi-provider-web.ts | Forwards domain through MultiProvider.initialize to child providers. |
| packages/server/src/provider/multi-provider/multi-provider.ts | Forwards domain through MultiProvider.initialize to child providers. |
| packages/shared/test/legacy-initialize-provider.ts | Adds reusable legacy single-arg initialize test provider helper for web/server suites. |
| packages/web/test/open-feature.spec.ts | Adds conformance tests for passing domain to initialize, enforcing domainScoped, and legacy init compatibility. |
| packages/web/test/multi-provider-web.spec.ts | Adds test asserting MultiProvider forwards domain without breaking legacy child providers. |
| packages/web/test/evaluation-context.spec.ts | Updates expectations to include (context, domain) in initialize calls. |
| packages/server/test/open-feature.spec.ts | Mirrors web conformance tests for server SDK behavior + legacy init compatibility. |
| packages/server/test/multi-provider.spec.ts | Mirrors web MultiProvider legacy-domain-forwarding test for server SDK. |
💡 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.
🧹 Nitpick comments (1)
packages/shared/test/legacy-initialize-provider.ts (1)
47-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd JSDoc
@param/@returnsto satisfyformat-lint.The
format-lintcheck warns that the JSDoc blocks onlegacyInitializeProvider(Line 47) andlegacyInitTestProvider(Line 78) are missing@paramand@returnsdeclarations.📝 Proposed JSDoc additions
/** * Provider with a single-argument initialize that ignores any extra arguments passed by the SDK. + * `@param` options provider configuration (paradigm, name, resolver mode) + * `@returns` a legacy single-argument initialize provider */ export function legacyInitializeProvider(options: LegacyInitializeProviderOptions): LegacyInitializeProvider {/** * Legacy initialize provider with the stubs MultiProvider expects on child providers. + * `@param` options provider configuration (paradigm, name, resolver mode) + * `@param` extras MultiProvider child stubs (events, hooks, track) + * `@returns` a legacy provider augmented with MultiProvider child stubs */ export function legacyInitTestProvider(🤖 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 `@packages/shared/test/legacy-initialize-provider.ts` around lines 47 - 50, Add the missing JSDoc tags on legacyInitializeProvider and legacyInitTestProvider so format-lint passes: update each function’s docblock to include an `@param` entry for the options argument and an `@returns` entry describing the provider returned. Keep the existing function names and doc comments intact, and apply the same JSDoc pattern to both legacyInitializeProvider and legacyInitTestProvider.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@packages/shared/test/legacy-initialize-provider.ts`:
- Around line 47-50: Add the missing JSDoc tags on legacyInitializeProvider and
legacyInitTestProvider so format-lint passes: update each function’s docblock to
include an `@param` entry for the options argument and an `@returns` entry
describing the provider returned. Keep the existing function names and doc
comments intact, and apply the same JSDoc pattern to both
legacyInitializeProvider and legacyInitTestProvider.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b35fb910-9cfc-4133-b796-ea1f346e78c5
📒 Files selected for processing (10)
packages/server/src/provider/multi-provider/multi-provider.tspackages/server/test/multi-provider.spec.tspackages/server/test/open-feature.spec.tspackages/shared/src/open-feature.tspackages/shared/src/provider/provider.tspackages/shared/test/legacy-initialize-provider.tspackages/web/src/provider/multi-provider/multi-provider-web.tspackages/web/test/evaluation-context.spec.tspackages/web/test/multi-provider-web.spec.tspackages/web/test/open-feature.spec.ts
MattIPv4
left a comment
MattIPv4
left a comment
There was a problem hiding this comment.
How does the MultiProvider handle domain-scoped providers? Or is wrapping a provider in MultiProvider essentially a way to bypass that restriction?
|
@MattIPv4 MultiProvider forwards Reusing the same domain-scoped child across multiple MultiProviders isn't caught at registration (children aren't in |
…in-scoped binding 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/ofrep-domain-cache-key
branch
from
199f03b to
50bdb4d
Compare
| * @param domain the bound domain, if any | ||
| */ | ||
| initialize?(context?: EvaluationContext): Promise<void>; | ||
| initialize?(context?: EvaluationContext, domain?: string): Promise<void>; |
There was a problem hiding this comment.
I wonder if we should make this an initialization config object so we can extend it in the future without breaking anything.
There was a problem hiding this comment.
I don't think it matters as much here due to overloading, but it's possible and maybe a good idea. We could have an InitializationContext (we already have a HookContext that is comparable). "Context" is already a bit overloaded, but this is mostly a provider author interface so I'm not that worried about that.
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.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 `@packages/web/test/open-feature.spec.ts`:
- Around line 120-129: The test expectation in OpenFeature provider resolution
is inverted for the named domain check. In the `it('MUST NOT bind a
domain-scoped default provider to a named domain'...)` case, update the
`OpenFeature.getProvider('domain-a')` assertion to verify it resolves to the
default provider instance rather than asserting it is different; use the
`OpenFeature.getProvider` and `provider.metadata.name` checks to confirm object
identity or matching metadata as appropriate.
🪄 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: c951b358-c438-4d73-a985-ca5b37391151
📒 Files selected for processing (6)
packages/server/test/multi-provider.spec.tspackages/server/test/open-feature.spec.tspackages/shared/src/open-feature.tspackages/shared/test/legacy-initialize-provider.tspackages/web/test/multi-provider-web.spec.tspackages/web/test/open-feature.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/server/test/multi-provider.spec.ts
- packages/server/test/open-feature.spec.ts
- packages/web/test/multi-provider-web.spec.ts
- packages/shared/src/open-feature.ts
| it('MUST NOT bind a domain-scoped default provider to a named domain', () => { | ||
| const provider = { ...mockProvider(), domainScoped: true } as Provider; | ||
|
|
||
| OpenFeature.setProvider(provider); | ||
| expect(() => OpenFeature.setProvider('domain-a', provider)).toThrow( | ||
| "Cannot bind domain-scoped provider 'mock-events-success' to more than one domain.", | ||
| ); | ||
| expect(OpenFeature.getProvider()).toBe(provider); | ||
| expect(OpenFeature.getProvider('domain-a').metadata.name).not.toBe(provider.metadata.name); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
ast-grep outline packages/shared/src/open-feature.ts --view expandedRepository: open-feature/js-sdk
Length of output: 671
🏁 Script executed:
sed -n '1,260p' packages/shared/src/open-feature.ts
rg -n "getProvider\\(|setAwaitableProvider|setProvider\\(" packages/shared/src/open-feature.ts packages/web -g '!**/dist/**'Repository: open-feature/js-sdk
Length of output: 21775
🏁 Script executed:
sed -n '1,280p' packages/web/src/open-feature.ts
sed -n '100,190p' packages/web/test/open-feature.spec.tsRepository: open-feature/js-sdk
Length of output: 16714
🏁 Script executed:
rg -n "getProviderForClient|_defaultProvider|_domainScopedProviders" packages/shared/src/open-feature.ts
sed -n '260,420p' packages/shared/src/open-feature.tsRepository: open-feature/js-sdk
Length of output: 7606
Flip the getProvider('domain-a') expectation
getProvider('domain-a') still resolves to the default provider here, so the metadata name matches the default provider’s name. Change this to toBe(...) (or assert object identity) instead of not.toBe(...).
Proposed fix
expect(OpenFeature.getProvider()).toBe(provider);
- expect(OpenFeature.getProvider('domain-a').metadata.name).not.toBe(provider.metadata.name);
+ expect(OpenFeature.getProvider('domain-a').metadata.name).toBe(provider.metadata.name);📝 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.
| it('MUST NOT bind a domain-scoped default provider to a named domain', () => { | |
| const provider = { ...mockProvider(), domainScoped: true } as Provider; | |
| OpenFeature.setProvider(provider); | |
| expect(() => OpenFeature.setProvider('domain-a', provider)).toThrow( | |
| "Cannot bind domain-scoped provider 'mock-events-success' to more than one domain.", | |
| ); | |
| expect(OpenFeature.getProvider()).toBe(provider); | |
| expect(OpenFeature.getProvider('domain-a').metadata.name).not.toBe(provider.metadata.name); | |
| }); | |
| it('MUST NOT bind a domain-scoped default provider to a named domain', () => { | |
| const provider = { ...mockProvider(), domainScoped: true } as Provider; | |
| OpenFeature.setProvider(provider); | |
| expect(() => OpenFeature.setProvider('domain-a', provider)).toThrow( | |
| "Cannot bind domain-scoped provider 'mock-events-success' to more than one domain.", | |
| ); | |
| expect(OpenFeature.getProvider()).toBe(provider); | |
| expect(OpenFeature.getProvider('domain-a').metadata.name).toBe(provider.metadata.name); | |
| }); |
🤖 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 `@packages/web/test/open-feature.spec.ts` around lines 120 - 129, The test
expectation in OpenFeature provider resolution is inverted for the named domain
check. In the `it('MUST NOT bind a domain-scoped default provider to a named
domain'...)` case, update the `OpenFeature.getProvider('domain-a')` assertion to
verify it resolves to the default provider instance rather than asserting it is
different; use the `OpenFeature.getProvider` and `provider.metadata.name` checks
to confirm object identity or matching metadata as appropriate.
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
| /** | ||
| * Provider with a single-argument initialize that ignores any extra arguments passed by the SDK. | ||
| * Pass optional extras when MultiProvider needs events, hooks, or track stubs on the child. | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * Provider with a single-argument initialize that ignores any extra arguments passed by the SDK. | |
| * Pass optional extras when MultiProvider needs events, hooks, or track stubs on the child. | |
| */ | |
| /** | |
| * Provider with a single-argument initialize that ignores any extra arguments passed by the SDK. | |
| * Pass optional extras when MultiProvider needs events, hooks, or track stubs on the child. | |
| * @param options provider configuration (paradigm, name, resolver mode) | |
| * @param extras optional MultiProvider child stubs (events, hooks, track) | |
| * @returns a single-argument-initialize provider | |
| */ |
Fixes a lint.
| @@ -241,6 +247,10 @@ export abstract class OpenFeatureCommonAPI< | |||
| throw new GeneralError(`Provider '${provider.metadata.name}' is intended for use on the ${provider.runsOn}.`); | |||
| } | |||
|
|
|||
| if (provider.domainScoped && this.allProviders.includes(provider)) { | |||
| throw new GeneralError(`Cannot bind domain-scoped provider '${provider.metadata.name}' to more than one domain.`); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Nit: I think the "domainScoped" check could be consolidated into a single upfront check that inspects both _defaultProvider and _domainScopedProviders directly for this instance and rejects if any binding differs from the current one, letting the no-op branch stay focused on being a no-op. Just for consideration; happy either way... I think it would be more lines of code, but it would lock it to a single place... maybe extracted to a method.
Totally optional, only do it if you agree.
There was a problem hiding this comment.
I'm approving because I see no fundamental problems, but consider mine and other's nits, and since this is a fundamentally new thing, let's leave it open for a few days at least.
I'm somewhat interested in doing what @beeme1mr said here, but I don't feel 100% sure. Interested in the opinions of others.
Summary
domainparameter to providerinitializeand pass the bound domain from the provider mutator (spec 1.1.2.2, 2.4.1)domainScopedprovider declaration and reject binding a domain-scoped instance to more than one domain (spec 2.4.3, 1.1.8.1)domainthrough web and server MultiProviderinitializeinitializebackward compatibilityMotivation
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.Notes
domainis an optional second parameter toinitializeinitializeruns once)initialize(context)remain compatible; the extradomainargument is ignored if unusedRelated Issues
Closes #1434
Relates to: open-feature/spec#393
Test plan
npx jest --selectProjects=shared --selectProjects=web --selectProjects=server