Skip to content

feat: preserve trait key casing in API responses - #416

Open
Zaimwa9 wants to merge 4 commits into
mainfrom
feat/preserve-trait-key-casing
Open

feat: preserve trait key casing in API responses#416
Zaimwa9 wants to merge 4 commits into
mainfrom
feat/preserve-trait-key-casing

Conversation

@Zaimwa9

@Zaimwa9 Zaimwa9 commented Aug 28, 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

  • Stop lowercasing trait keys from API responses — re-sending them created
    duplicate traits (appVersion + appversion). Keys are stored verbatim;
    spaces still become underscores.
  • getTrait matches exactly, then case-insensitively, so existing reads keep working.

How did you test this code?

  • New test/trait-key-casing.test.ts; full suite and tsc pass.

khvn26 and others added 3 commits August 6, 2026 17:48
Trait keys returned by the API were stored lowercased, then re-sent to the
identities endpoint on subsequent calls (setTrait, identify without traits),
creating duplicate traits server-side for any camelCase key. Store keys
verbatim (spaces are still replaced with underscores) and keep getTrait
case-insensitive via an exact-then-fallback lookup.
@Zaimwa9
Zaimwa9 requested a review from a team as a code owner August 28, 2026 15:17
@Zaimwa9
Zaimwa9 requested review from talissoncosta and removed request for a team August 28, 2026 15:17
@Zaimwa9

Zaimwa9 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

@themis-blindfold review

Comment thread flagsmith-core.ts
Comment thread flagsmith-core.ts
@themis-blindfold

Copy link
Copy Markdown

⚖️ Themis review: 🟠 Fix before merge

The completed test check passed, but this casing fix does not account for lowercased traits already persisted in client caches, and its fallback lookup changes legacy reads when case-distinct traits coexist. Those paths can still re-send or return the wrong trait value after upgrade.

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

🟠 Majors

  • flagsmith-core.ts:131 — Existing cached traits can still recreate duplicate server traits after an SDK upgrade (unverified).
  • flagsmith-core.ts:728 — A mixed-case read can return a different distinct trait than before (unverified).
📝 Walkthrough
  • Identity response handling - API trait-key casing is retained while spaces remain canonicalised.
  • Trait reads - exact key lookup now precedes a case-insensitive fallback.
  • Regression coverage - identity-response, mutation, and same-case-fold tests cover fresh API state.
🧪 How to verify
  1. Seed storage with a state written by the previous SDK for an appVersion trait, initialise with cacheFlags and skipAPI, then set an unrelated trait; confirm the POST does not include appversion.
  2. Return appVersion followed by appversion, call getTrait('APPVERSION'), and confirm it preserves the pre-change last-response result.
  3. Run npm test -- --runInBand test/trait-key-casing.test.ts test/cache.test.ts.
  4. Run npm run typecheck.
    Automate: Add cache-upgrade and no-exact-match case-collision regressions to the trait-casing suite.

Product take: This is a solid compatibility repair for camel-case traits, but it needs to cover already-installed clients before it stops duplicate records reliably.

🧭 Assumptions & unverified claims
  • Unverified: The identity endpoint can return and persist trait keys that differ only by case; confirm with an API integration test or published endpoint contract.

Camel case needs to survive the trip home, including through the cache · reviewed at bcdc220

Lowercased trait keys persisted by pre-9.0.1 SDK versions are dropped on
cache load and never re-sent, and a lookup matching no exact key resolves to
the first case-insensitive match.
@Zaimwa9

Zaimwa9 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

@themis-blindfold review

Comment thread flagsmith-core.ts
});
traits.forEach(trait => {
userTraits[trait.trait_key.toLowerCase().replace(/ /g, '_')] = {
userTraits[trait.trait_key.replace(/ /g, '_')] = {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 Major · ⚡ Quick win

(unverified) Case-insensitive reads are not paired with case-insensitive writes.

Observed: This stores appVersion as a key, while setTrait('appversion', ...) and setTraits({ appversion: ... }) merge the supplied spelling and the identity POST serialises every key. The resulting payload contains both appVersion and appversion.

Predicted: If the identities API treats casing variants as separate traits—the duplicate scenario this change addresses—an application that keeps its previous lowercase write spelling would recreate the duplicate when updating the trait. Resolve an incoming update to an existing case-insensitive key (exact match first) before merging it, and cover both update methods.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@themis-blindfold working as intended: trait keys are case-sensitive, so a write with a different casing is a different trait, the SDK stores and sends exactly what the caller provides, matching the API and every other SDK.

Case-folding writes would silently rewrite user-chosen keys; the case-insensitive read fallback remains as a backwards-compatibility

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Confirmed. setTrait and setTraits preserve caller-provided keys (flagsmith-core.ts:773, flagsmith-core.ts:794), while identity POST serialises those keys unchanged (flagsmith-core.ts:213). Case variants are intentionally distinct writes; the finding is withdrawn.

@themis-blindfold

Copy link
Copy Markdown

⚖️ Themis review: 🟠 Fix before merge

TL;DR: Preserving API casing fixes the re-send path, but callers that continue using the lowercased key with setTrait or setTraits now construct a payload containing both spellings. The completed test check passed.

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

🟠 Majors

⚖️ Acknowledged

  • Cached lowercased identity traits are not migrated for cache-only operation — thread resolved by @Zaimwa9
  • A mixed-case lookup without an exact match returns the first colliding key — thread resolved by @Zaimwa9
📝 Walkthrough
  • Trait response handling - retains API casing while continuing to convert spaces to underscores.
  • Trait access and updates - reads fall back case-insensitively, while update payloads retain the supplied key spelling.
  • Regression coverage - exercises response casing, cache refresh, lookup collisions, and re-sends.
🧪 How to verify
  1. Initialise against an identity whose response contains appVersion.
  2. Call setTrait('appversion', '2.16.1') and assert the identities POST has one appVersion entry, with the updated value.
  3. Repeat with setTraits({ appversion: '2.16.1' }).
  4. Assert an exact-case update still selects the exact trait when a response contains both casing variants.
    Automate: Add the two update cases to test/trait-key-casing.test.ts and inspect the posted trait keys.

Product take: This fixes a real trait-integrity problem for camelCase keys, but existing applications commonly retain the lowercased write spelling produced by earlier SDK versions. The remaining write path produces a payload containing both spellings.

🧭 Assumptions & unverified claims

The identities API examples document trait_key payloads but do not specify the handling of two casing variants in one request; the predicted duplicate consequence depends on that provider behaviour.

A casing fix that still speaks lowercase on writes needs one more translation layer · reviewed at f7d98fc

@themis-blindfold

Copy link
Copy Markdown

Confirmed. Case-sensitive trait writes are intentional; no change is needed. The case-insensitive lookup is a read-only compatibility fallback.

🧠 Learning recorded — lands in .themis/learnings.jsonl via the next digest PR.

@talissoncosta talissoncosta left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me!!!

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