Skip to content

feat(ofrep-web): ADR-0009 domain-aware cache key + domainScoped#1569

Open
jonathannorris wants to merge 7 commits into
mainfrom
feat/ofrep-domain-cache-key
Open

feat(ofrep-web): ADR-0009 domain-aware cache key + domainScoped#1569
jonathannorris wants to merge 7 commits into
mainfrom
feat/ofrep-domain-cache-key

Conversation

@jonathannorris

@jonathannorris jonathannorris commented Jun 26, 2026

Copy link
Copy Markdown
Member

Summary

  • Replace cacheKeyPrefix with a cacheKeyGenerator per the protocol ADR-0009 amendment. The default generator returns JSON-encoded key material from baseUrl, auth credential, bound domain, and targetingKey; the provider hashes that into cacheKeyHash.
  • Declare domainScoped: true and create Storage only in initialize(context, domain?), so nothing touches localStorage before the bound domain is known.
  • Derive auth for the cache key from known auth headers only (Authorization, Api-Key, X-Api-Key, X-Auth-Token, X-Access-Token), matched case-insensitively and serialized with lowercased header names.
  • Persist under schema v2 (ofrep-web-provider:v2:{hash}); v1 entries are discarded on read.
  • Document caching, cache-key customization, and domain scoping in the provider README.

Motivation

OFREP web persistence needs the bound OpenFeature domain at init time to scope cache keys per ADR-0009. This supersedes #1566 (URL + full eval context keying).

Blocked on open-feature/js-sdk#1433 for runtime domain forwarding via OpenFeature.setProvider('domain', provider).

Notes

  • Use cacheKeyGenerator to namespace instances, drop auth for rotating tokens, or include stable context fields.
  • cacheKeyPrefix is removed.

Related Issues

Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The OFREP web provider now derives persisted cache keys from base URL, auth credentials, domain, prefix, and targeting key, stores them in a v2 schema, and defers storage initialization until initialize(context, domain). The README, option docs, and tests were updated for the new caching flow.

Changes

OFREP web persistent caching

Layer / File(s) Summary
Cache-key contract
libs/providers/ofrep-web/src/lib/store/cache-key.ts, libs/providers/ofrep-web/src/lib/store/cache-key.spec.ts, libs/providers/ofrep-web/src/lib/store/storage.ts, libs/providers/ofrep-web/src/lib/store/storage.spec.ts
Cache-key helpers serialize supported auth headers and JSON-encode the cache key parts, and Storage hashes that encoded input under schema v2; tests cover prefix handling, case-insensitive auth matching, and persisted-entry expectations.
Domain-scoped provider
libs/providers/ofrep-web/src/lib/ofrep-web-provider.ts, libs/providers/ofrep-web/src/lib/ofrep-web-provider.spec.ts
OFREPWebProvider is marked domain-scoped, defers Storage creation to initialize(context, domain), and guards cache access when storage is unset; tests cover domain-scoped initialization and persisted-cache seeding with the v2 entry shape.
Caching docs
libs/providers/ofrep-web/README.md, libs/providers/ofrep-web/src/lib/model/ofrep-web-provider-options.ts
The README and cacheKeyPrefix docs describe persistent localStorage caching, cache-mode and TTL semantics, cache-key components, domain scoping, and auth-header participation.

Sequence Diagram(s)

sequenceDiagram
  participant OpenFeature
  participant OFREPWebProvider
  participant deriveAuthCredential
  participant Storage
  participant localStorage

  OpenFeature->>OFREPWebProvider: initialize(context, domain)
  OFREPWebProvider->>Storage: new Storage(cacheMode, baseUrl, getAuthCredential, domain, cacheKeyPrefix, logger)
  Storage->>deriveAuthCredential: await getAuthCredential()
  deriveAuthCredential-->>Storage: serialized auth credential
  Storage->>localStorage: read/write persisted entries
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: ADR-0009 domain-aware cache keying and domainScoped support.
Description check ✅ Passed The description is directly related to the changeset and accurately describes the caching, domain scoping, and persistence updates.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@MattIPv4

Copy link
Copy Markdown
Member

👀 I will flag the same other concern I had in #1566 here, which is that while we're now passing all of this in as input to the cache key, it is still being truncated down to just the first 16 chars, and that concerns me that collisions will happen unexpectedly and result in unsafe behaviour still?

Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Comment thread libs/providers/ofrep-web/src/lib/store/cache-key.ts
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the ofrep-web provider’s persistent cache behavior to align with ADR-0009 by making persisted cache keys domain-aware and derived from a structured (JSON-encoded) identity that includes baseUrl, selected auth headers, bound OpenFeature domain, and targetingKey. It also shifts persistence initialization into initialize(...) so localStorage is not accessed before the provider’s bound domain is known.

Changes:

  • Introduces ADR-0009 cache key encoding + auth-credential derivation (restricted to known auth headers).
  • Updates persistent storage schema to v2 and makes the storage key depend on baseUrl/auth/domain/targetingKey (+ optional prefix).
  • Declares domainScoped = true, updates provider lifecycle to accept initialize(context, domain?), and documents the behavior in the README.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
libs/providers/ofrep-web/src/lib/store/storage.ts Updates persistent storage schema to v2 and hashes ADR-0009 cache key inputs including baseUrl/auth/domain.
libs/providers/ofrep-web/src/lib/store/storage.spec.ts Updates storage tests for v2 keys and verifies baseUrl/domain/auth affect key derivation.
libs/providers/ofrep-web/src/lib/store/cache-key.ts Adds auth-credential derivation from known auth headers and JSON encoding for cache key inputs.
libs/providers/ofrep-web/src/lib/store/cache-key.spec.ts Adds tests for cache key encoding and auth header serialization.
libs/providers/ofrep-web/src/lib/ofrep-web-provider.ts Makes provider domain-scoped and instantiates persistence only during initialize with the bound domain.
libs/providers/ofrep-web/src/lib/ofrep-web-provider.spec.ts Adds domain-scoping tests and updates persisted-cache tests for schema v2.
libs/providers/ofrep-web/src/lib/model/ofrep-web-provider-options.ts Updates cacheKeyPrefix option docs to reflect ADR-0009 composition.
libs/providers/ofrep-web/README.md Documents ADR-0009 cache key, domain scoping, and auth header participation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread libs/providers/ofrep-web/src/lib/ofrep-web-provider.spec.ts Outdated
Comment thread libs/providers/ofrep-web/src/lib/ofrep-web-provider.spec.ts Outdated
Comment thread libs/providers/ofrep-web/src/lib/store/cache-key.ts
@jonathannorris

jonathannorris commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

@MattIPv4 re #1569 (comment)

I will flag the same other concern I had in #1566 here, which is that while we're now passing all of this in as input to the cache key, it is still being truncated down to just the first 16 chars, and that concerns me that collisions will happen unexpectedly and result in unsafe behaviour still?

The 16-char truncation is inherited from the original ADR implementation (first 64 bits of SHA-256). Worth being explicit about what that means in practice.

Collision risk depends on how many entries coexist in localStorage on the same origin, not how large the targetingKey space is. A deployment might have millions of possible targeting keys, but this provider clears the previous entry on targetingKey change and typically only holds one active entry per url/auth/domain binding. In practice that's a small number of keys per website.

For n simultaneous entries, collision probability scales roughly with n² / (2 × 2⁶⁴):

Coexisting entries Approx. collision risk
5 ~5 × 10⁻¹⁹
10 ~2 × 10⁻¹⁸
100 ~3 × 10⁻¹⁶

At that scale a collision would mean serving the wrong cached flags, but the odds are not something I'd optimize for here. Open to bumping to 32 hex chars if you feel strongly.

@MattIPv4

Copy link
Copy Markdown
Member

👍 Not strongly held at all, just want to make sure that has been thought through, which it seems it has!

…h seeds

Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
@jonathannorris jonathannorris marked this pull request as ready for review June 26, 2026 19:48
@jonathannorris jonathannorris requested review from a team as code owners June 26, 2026 19:48
…R-0009

Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants