Skip to content

fix: fetch flags only when storage holds another identity's flags - #95

Merged
Zaimwa9 merged 1 commit into
mainfrom
fix/identity-scoped-flag-lookup
Oct 6, 2026
Merged

Zaimwa9 merged 1 commit into
mainfrom
fix/identity-scoped-flag-lookup

Conversation

@Zaimwa9

@Zaimwa9 Zaimwa9 commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Thanks for submitting a PR! Please check the boxes below:

  • I have read the Contributing Guide.
  • I have added information to docs/ if required so people know about the feature.
  • I have filled in the "Changes" section below.
  • I have filled in the "How did you test this code" section below.

Changes

Follow-up to #94.

The client now remembers which identity the stored flags were fetched for (null for environment flags, reset() and a fresh process). Lookups that take a user fetch from the API only when that identity differs from the stored one, and read storage otherwise. An explicit reload still wins.

  • getExperimentFlag(user:) no longer hits /identities on every call; same identity as the last fetch is served from storage, a different identity is fetched. Exposures keep the guarantee from feat: surface experiment metadata on flags and add event tracking #94: never attributed against another identity's assignment.
  • hasFeatureFlag, isFeatureFlagEnabled and getFeatureFlagValue share the same rule through _getFlagByName. Previously a user different from the last fetch silently returned the previous identity's flags from storage; now it fetches once. Same identity or no user: unchanged, no request.

Behaviour change to the three existing methods only on the path that returned the wrong identity's flags. They also no longer forget cachedUser when called without a user, and getExperimentFlag fetches whenever traits are supplied so they reach the server.

How did you test this code?

flutter analyze clean, flutter test 169 passing. New cases: same identity makes no request; environment flags overwriting storage trigger a fetch for the cached identity; hasFeatureFlag with a switched identity fetches once and then reads storage; traits force a fetch; a read without user keeps the identity; clearStore and initStore(clear: true) drop the marker; a transient evaluation is never reused as a baseline.

Base automatically changed from feat/experimentation to main September 21, 2026 12:20
@Zaimwa9
Zaimwa9 force-pushed the fix/identity-scoped-flag-lookup branch from 41e8c0a to afa7586 Compare October 5, 2026 10:11
@Zaimwa9

Zaimwa9 commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

@themis-blindfold review

@Zaimwa9
Zaimwa9 force-pushed the fix/identity-scoped-flag-lookup branch 2 times, most recently from d3363ca to d954910 Compare October 5, 2026 14:25
Comment thread lib/src/flagsmith_client.dart Outdated
Comment thread lib/src/flagsmith_client.dart
@themis-blindfold

Copy link
Copy Markdown

⚖️ Themis review: 🟠 Fix before merge

The identity marker fixes the ordinary identity-switch path, but two cache-validity gaps remain: trait-bearing experiment reads can ignore the traits, and reinitialising storage can retain a marker for flags that were removed. CI passed: Dart Publish Package Test, Dart Analyze, and Flutter Test. Flutter is unavailable in this environment, so I could not rerun them locally.

Area Score
🎯 Correctness 2/5
🧪 Test coverage 3/5
📐 Code quality 3/5
🚀 Product impact 3/5

🟠 Majors

  • lib/src/flagsmith_client.dart:396 — trait-bearing experiment lookups can skip the supplied traits after a same-identity fetch.
  • lib/src/flagsmith_client.dart:255 — reinitialising storage can retain an identity marker for flags no longer in storage.
📝 Walkthrough
  • identity-aware storage - records the identity that last populated stored flags and uses it for user-specific reads.
  • experiments - reuses same-identity flags for exposure reads and adds coverage for switching identities and environment flags.
🧪 How to verify
  1. Fetch an identity without traits, then call getExperimentFlag for that identity with transient traits and assert a second identity request includes those traits.
  2. Fetch identity A, call initStore(clear: true, seeds: ...), then read a flag for A and assert it refetches rather than using the seed.
  3. Run flutter analyze.
  4. Run flutter test.
    Automate: Add regressions for both cache invalidation paths above.

Product take: This is a solid request-reduction improvement, but stale evaluation is especially costly for targeting and experiment exposure data. Fixing the two invalidation cases keeps the optimisation transparent to users.

🧭 Assumptions & unverified claims

No unverified assumptions or claims.

The cache now knows its owner; it just needs to notice when the furniture moves. · reviewed at afa7586

@Zaimwa9
Zaimwa9 force-pushed the fix/identity-scoped-flag-lookup branch from d954910 to bfbc092 Compare October 5, 2026 14:26
@Zaimwa9

Zaimwa9 commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

@themis-blindfold review

@Zaimwa9
Zaimwa9 marked this pull request as ready for review October 5, 2026 14:28
@Zaimwa9
Zaimwa9 requested a review from a team as a code owner October 5, 2026 14:28
@Zaimwa9
Zaimwa9 requested review from gagantrivedi and kyle-ssg and removed request for a team October 5, 2026 14:28
Comment thread lib/src/flagsmith_client.dart
Comment thread lib/src/flagsmith_client.dart
@themis-blindfold

Copy link
Copy Markdown

⚖️ Themis review: 🟠 Fix before merge

The identity-cache optimisation tracks only an identifier, and some storage-reset paths never invalidate that marker. Fix these cache-owner transitions before merging. Dart Analyze completed successfully; Flutter Test was still running.

Area Score
🎯 Correctness 2/5
🧪 Test coverage 3/5
📐 Code quality 3/5
🚀 Product impact 3/5

🟠 Majors

  • lib/src/flagsmith_client.dart: Predicted: transient evaluations would be reused for an ordinary identity read.
  • lib/src/flagsmith_client.dart: Predicted: clearStore() would leave the deleted store marked as the previous identity.
  • lib/src/flagsmith_client.dart: Predicted: initStore(clear: true) would label seeded flags as the previous identity.

⚖️ Acknowledged

  • Trait-bearing experiment reads now force a server fetch — thread resolved by @Zaimwa9
📝 Walkthrough
  • FlagsmithClient - tracks the identity associated with persisted flags so normal reads can avoid redundant identity requests.
  • Flag lookup helpers - use that owner marker to reload only after an identity change.
  • Experiment lookup - now reloads when traits are supplied and preserves the cached identity for exposure events.
  • Regression tests - cover same-identity reuse, identity changes, environment replacement, traits, and cached-user retention.
🧪 How to verify
  1. Fetch flags for identity A, call clearStore(), then read a flag for A and assert that /identities is requested.
  2. Fetch flags for identity A, call initStore(clear: true, seeds: ...), then read for A and assert that the identity response replaces the seed flags.
  3. Fetch A with a transient identity or transient trait that changes a segment, then read A without that context and assert a fresh request and baseline variant.
  4. Run flutter analyze and flutter test.
    Automate: add the three cache-owner lifecycle cases to test/fg/flagsmith_experiments_test.dart.

Product take: Avoiding redundant identity calls is a solid improvement. Predicted: a stale evaluation context could serve and record the wrong experiment variant, making these fixes important for identity-targeted flags.

🧭 Assumptions & unverified claims

No unverified assumptions or claims.

A good cache needs to remember when it has forgotten · reviewed at bfbc092

@Zaimwa9
Zaimwa9 force-pushed the fix/identity-scoped-flag-lookup branch from bfbc092 to 8afcac2 Compare October 5, 2026 14:47
@Zaimwa9

Zaimwa9 commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

@themis-blindfold review

@themis-blindfold

Copy link
Copy Markdown

⚖️ Themis review: ✅ Ship it

The identity marker now tracks the stored assignment through environment refreshes, resets, clears, and transient evaluations. The completed Flutter Test and Dart Analyze checks passed.

Area Score
🎯 Correctness 5/5
🧪 Test coverage 5/5
📐 Code quality 4/5
🚀 Product impact 3/5

⚖️ Acknowledged

  • Trait-bearing experiment reads refresh from the API — thread resolved by @Zaimwa9
📝 Walkthrough
  • Identity-aware reads - reuse storage only for the identity that produced it, while a changed identity or environment result refreshes it.
  • Storage lifecycle - clearing, reset, and replacement with seeded values invalidate the identity marker.
  • Experiment evaluation - supplied traits force a server evaluation, and transient evaluations are not reused as a normal assignment.
🧪 How to verify
  1. Run flutter test test/fg/flagsmith_experiments_test.dart.
  2. Fetch flags for identity A, then query identity B and confirm the first B query reaches /identities while the next one does not.
  3. Refresh environment flags or clear the store, then query the cached identity and confirm it refetches.
  4. Evaluate a transient identity, then perform a plain read for the same identifier and confirm it refetches.
    Automate: run flutter test and flutter analyze in CI.

Product take: A solid correctness and efficiency improvement for identity-based flag and experiment reads, avoiding redundant evaluations without reusing another identity's assignment.

🧭 Assumptions & unverified claims
  • flutter was unavailable locally; confidence in execution relies on the completed Flutter Test and Dart Analyze checks.

The flag cache now knows whose coat it is wearing · reviewed at 8afcac2

@Zaimwa9
Zaimwa9 merged commit b848eff into main Oct 6, 2026
4 checks passed
@Zaimwa9
Zaimwa9 deleted the fix/identity-scoped-flag-lookup branch October 6, 2026 06:10
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.

2 participants