Skip to content

Commit 7a6f158

Browse files
committed
Merge branch 'main' into chore/CM-1349-cleanup-case-variant-identities
2 parents b8783e8 + ddfcebf commit 7a6f158

15 files changed

Lines changed: 242 additions & 109 deletions

File tree

backend/src/api/public/v1/members/createMember.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,10 @@ export async function createMember(req: Request, res: Response): Promise<void> {
5454
identities.map((identity) => ({
5555
...identity,
5656
memberId: dbMember.id,
57-
value: identity.value.trim().toLowerCase(),
57+
value:
58+
identity.type === MemberIdentityType.EMAIL
59+
? identity.value.trim().toLowerCase()
60+
: identity.value.trim(),
5861
})),
5962
true,
6063
true,

backend/src/api/public/v1/members/identities/createMemberIdentity.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,9 @@ export async function createMemberIdentity(req: Request, res: Response): Promise
4646
throw new NotFoundError('Member not found')
4747
}
4848

49-
// The data-sink writes identity values as trimmed lowercase, so normalize here
50-
// to keep idempotency checks reliable against existing rows.
51-
const normalizedValue = data.value.trim().toLowerCase()
49+
// Normalize emails to lowercase; keep username preferred casing from the caller.
50+
const normalizedValue =
51+
data.type === MemberIdentityType.EMAIL ? data.value.trim().toLowerCase() : data.value.trim()
5252

5353
let result!: IMemberIdentity
5454
let alreadyExisted = false
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
-- Enforce uniqueness on lower(value) instead of the original value.
2+
-- `value` preserves the source's preferred casing; email values are stored in lowercase.
3+
-- docs/adr/0015-how-cdp-stores-member-identities.md
4+
5+
create unique index concurrently if not exists "uix_memberIdentities_memberId_platform_type_lower_value"
6+
on "memberIdentities" ("memberId", platform, type, lower(value))
7+
where "deletedAt" is null;
8+
9+
create unique index concurrently if not exists "uix_memberIdentities_platform_type_lower_value_verified"
10+
on "memberIdentities" (platform, type, lower(value))
11+
where verified = true
12+
and "deletedAt" is null;
13+
14+
drop index concurrently if exists "uix_memberIdentities_memberId_platform_value_type";
15+
drop index concurrently if exists "uix_memberIdentities_platform_value_type_verified";

backend/src/services/member/memberIdentityService.ts

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,19 @@ import {
1313
updateMemberIdentity,
1414
} from '@crowd/data-access-layer/src/members'
1515
import { LoggerBase } from '@crowd/logging'
16-
import { IMemberIdentity, NewMemberIdentity } from '@crowd/types'
16+
import { IMemberIdentity, MemberIdentityType, NewMemberIdentity } from '@crowd/types'
1717

1818
import { IRepositoryOptions } from '@/database/repositories/IRepositoryOptions'
1919
import SequelizeRepository from '@/database/repositories/sequelizeRepository'
2020
import { optionsQx } from '@/database/sequelizeQueryExecutor'
2121

2222
import { IServiceOptions } from '../IServiceOptions'
2323

24+
function normalizeIdentityValue(type: string, value: string): string {
25+
const trimmed = value.trim()
26+
return type === MemberIdentityType.EMAIL ? trimmed.toLowerCase() : trimmed
27+
}
28+
2429
export default class MemberIdentityService extends LoggerBase {
2530
options: IServiceOptions
2631

@@ -58,11 +63,16 @@ export default class MemberIdentityService extends LoggerBase {
5863

5964
const qx = SequelizeRepository.getQueryExecutor(repoOptions)
6065

66+
const identityData = {
67+
...data,
68+
value: normalizeIdentityValue(data.type, data.value),
69+
}
70+
6171
// Check if identity already exists
6272
const conflict = await findMemberIdentityConflict(qx, {
63-
value: data.value,
64-
platform: data.platform,
65-
type: data.type,
73+
value: identityData.value,
74+
platform: identityData.platform,
75+
type: identityData.type,
6676
})
6777

6878
if (conflict) {
@@ -77,7 +87,7 @@ export default class MemberIdentityService extends LoggerBase {
7787
}
7888

7989
// Create member identity
80-
await insertMemberIdentities(qx, [{ ...data, memberId }])
90+
await insertMemberIdentities(qx, [{ ...identityData, memberId }])
8191

8292
await touchMemberUpdatedAt(qx, memberId)
8393

@@ -131,7 +141,12 @@ export default class MemberIdentityService extends LoggerBase {
131141
const qx = SequelizeRepository.getQueryExecutor(repoOptions)
132142

133143
// Check if any of the identities already exist
134-
for (const identity of data) {
144+
const normalizedData = data.map((identity) => ({
145+
...identity,
146+
value: normalizeIdentityValue(identity.type, identity.value),
147+
}))
148+
149+
for (const identity of normalizedData) {
135150
const conflict = await findMemberIdentityConflict(qx, {
136151
value: identity.value,
137152
platform: identity.platform,
@@ -153,7 +168,7 @@ export default class MemberIdentityService extends LoggerBase {
153168
// Create member identities
154169
await insertMemberIdentities(
155170
qx,
156-
data.map((identity) => ({ ...identity, memberId })),
171+
normalizedData.map((identity) => ({ ...identity, memberId })),
157172
)
158173

159174
await touchMemberUpdatedAt(qx, memberId)
@@ -211,7 +226,10 @@ export default class MemberIdentityService extends LoggerBase {
211226
throw new Error404(this.options.language, 'errors.notFound.message')
212227
}
213228

214-
const value = data.value ?? currentIdentity.value
229+
const value = normalizeIdentityValue(
230+
data.type ?? currentIdentity.type,
231+
data.value ?? currentIdentity.value,
232+
)
215233
const platform = data.platform ?? currentIdentity.platform
216234
const type = data.type ?? currentIdentity.type
217235

@@ -234,7 +252,10 @@ export default class MemberIdentityService extends LoggerBase {
234252
}
235253

236254
// Update member identity with new data
237-
await updateMemberIdentity(qx, memberId, id, data)
255+
await updateMemberIdentity(qx, memberId, id, {
256+
...data,
257+
...(data.value !== undefined ? { value } : {}),
258+
})
238259

239260
await touchMemberUpdatedAt(qx, memberId)
240261

backend/src/utils/err.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ type ConflictFactory = (context?: Record<string, unknown>) => Error
55
const DB_CONFLICT_MAP: Record<string, ConflictFactory> = {
66
uix_memberIdentities_platform_value_type_verified: (context) =>
77
new ConflictError('Identity already exists on another member', context),
8+
uix_memberIdentities_platform_type_lower_value_verified: (context) =>
9+
new ConflictError('Identity already exists on another member', context),
810
}
911

1012
export function rethrowDbConflict(error: unknown, context?: Record<string, unknown>): never {
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
# ADR-0015: How CDP stores member identities
2+
3+
**Date**: 2026-07-28
4+
**Status**: accepted
5+
**Deciders**: Yeganathan S
6+
**Related**: CM-1349
7+
8+
## Context
9+
10+
CDP treats member identities as case-insensitive when resolving people (`lower(value)` in lookups and many DAL queries), which matches how major platforms behave:
11+
12+
- **GitHub / GitLab**: usernames are case-insensitive for uniqueness, but case-preserving for display (`WillsonHG` and `willsonhg` are the same account; APIs return preferred casing).
13+
- **Discord** (new usernames): forced lowercase.
14+
- **Email**: stored and compared lowercase in practice.
15+
16+
Historically, `memberIdentities` uniqueness was defined on raw `value` (case-sensitive):
17+
18+
- `uix_memberIdentities_memberId_platform_value_type`
19+
- `uix_memberIdentities_platform_value_type_verified`
20+
21+
Lookups used `lower(value)`, but inserts often did not. Data-sink `mergeData` matched with exact `value ===`, so a self-serve lowercase `willsonhg` plus a later GitHub ingest of `WillsonHG` produced two rows for the same identity — often both verified on the same member. Prod had tens of thousands of GitHub case-variant groups.
22+
23+
Ticket CM-1349 proposed soft-deleting / auto-verifying case variants at verification time. That treats a write-path bug as a product special case.
24+
25+
## Decision
26+
27+
**Mental model**
28+
29+
| Kind | Store | Compare / unique on |
30+
| --- | --- | --- |
31+
| username | preferred casing from the source/integration | `lower(value)` |
32+
| email | always lowercase | `lower(value)` (same as stored) |
33+
34+
Identity equality in CDP is `(platform, type, lower(value))`. The `value` column keeps what the source sent for usernames; we do not rewrite GitHub `login` casing on ingest.
35+
36+
**Enforcement**
37+
38+
1. **Write paths** match and upsert with case-insensitive equality (`isSameMemberIdentity` / `lower(value)`). Do not insert a second row that only differs by casing.
39+
2. **DB uniqueness** uses expression unique indexes on `lower(value)` (partial on `deletedAt is null`, and verified-only for the global verified owner index). See migration `V1785255019__member_identities_case_insensitive_unique_indexes.sql`.
40+
3. **Existing duplicates** are cleaned with a one-time script before the unique indexes can be applied: same-member case variants → keep one (prefer verified + `verifiedBy`, else most recent integration casing) and soft-delete the rest; cross-member unverified variants of a verified identity → soft-delete the unverified; both verified across members → merge / existing capitalization-merge workflows, not blind soft-delete.
41+
4. **Verification** does not need special “soft-delete case siblings” logic once the invariant holds — verifying finds the one row.
42+
43+
## Alternatives Considered
44+
45+
### Alternative 1: Soft-delete / auto-verify case variants at identity verification time (CM-1349 as written)
46+
47+
- **Pros**: Fixes the user-visible self-serve pain quickly; no schema change.
48+
- **Cons**: Case variants keep being inserted by ingest/enrichment; verify path becomes a mop; duplicates still break uniqueness and analytics.
49+
- **Why not**: Papers over the root cause. If case variants should not exist, stop creating them and clean existing data.
50+
51+
### Alternative 2: Always store usernames lowercase (like emails / Discord)
52+
53+
- **Pros**: Simplest storage; uniqueness on `value` works without expression indexes.
54+
- **Cons**: Throws away GitHub/GitLab preferred casing; diverges from source payloads; confuses display and support (“CDP shows lowercase but GitHub shows mixed”).
55+
- **Why not**: We want GitHub-style case-preserving storage. Uniqueness belongs on `lower(value)`, not on mutating the stored handle.
56+
57+
### Alternative 3: Keep case-sensitive unique indexes; only fix app-layer matching
58+
59+
- **Pros**: No migration; no cleanup required to change indexes.
60+
- **Cons**: App bugs or races can still insert case variants; DB does not enforce the domain invariant.
61+
- **Why not**: At this scale, durable invariants need to live in the database, not only in callers.
62+
63+
### Alternative 4: Update stored casing on every ingest when preferred casing differs
64+
65+
- **Pros**: `value` always mirrors latest source casing.
66+
- **Cons**: Unsafe while same-member case-variant pairs still exist (updating both rows to the same `value` hits the old unique index). Extra write noise.
67+
- **Why not**: Deferred until after cleanup. Preventing duplicate inserts is enough for the durable fix; optional casing refresh can come later.
68+
69+
## Consequences
70+
71+
### Positive
72+
73+
- One clear rule: same platform + type + lower(value) ⇒ same identity.
74+
- Lookups, writes, and uniqueness agree.
75+
- Self-serve / GitHub / enrichment stop creating `WillsonHG` + `willsonhg` pairs.
76+
- Preferred username casing from integrations is preserved.
77+
78+
### Negative
79+
80+
- Cleanup must run before the unique-index migration, or `create unique index` fails (and can leave an `INVALID` index).
81+
- Expression unique indexes are slightly less obvious than column-only uniques; callers must keep using `lower(value)` (or `isSameMemberIdentity`) consistently.
82+
- Conflict handlers need to recognize both old and new constraint names during rollout.
83+
84+
### Risks
85+
86+
- **Migration applied before cleanup** — mitigated by documenting order: write-path fix → cleanup script → unique-index migration.
87+
- **Cross-member verified case variants** — rare; require merge, not soft-delete. Existing `findAndMergeMembersWithSamePlatformIdentitiesDifferentCapitalization` covers part of this.
88+
- **Incomplete write-path coverage** — mitigated by DB unique indexes as the backstop once cleanup is done; shared `isSameMemberIdentity` for app equality.

docs/adr/README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ Use the `/adr` skill in Claude Code to record new ADRs or query past decisions.
2121
| [ADR-0012](./0012-api-e2e-test-architecture.md) | API e2e test architecture | accepted | 2026-07-25 |
2222
| [ADR-0013](./0013-api-e2e-test-suite-design.md) | API e2e test suite design | accepted | 2026-07-24 |
2323
| [ADR-0014](./0014-collaboration-track-record-signal.md) | Collaboration track record signal | accepted | 2026-07-28 |
24+
| [ADR-0015](./0015-how-cdp-stores-member-identities.md) | How CDP stores member identities | accepted | 2026-07-28 |
2425

2526
## Why ADRs?
2627

services/apps/data_sink_worker/src/service/activity.service.ts

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -521,10 +521,16 @@ export default class ActivityService extends LoggerBase {
521521
const toEraseMemberIdentities = toErase.filter((e) =>
522522
member.identities.some((i) => {
523523
if (i.type === MemberIdentityType.EMAIL) {
524-
return e.type === i.type && e.value === i.value
524+
return (
525+
e.type === i.type && e.value.trim().toLowerCase() === i.value.trim().toLowerCase()
526+
)
525527
}
526528

527-
return e.type === i.type && e.value === i.value && e.platform === i.platform
529+
return (
530+
e.type === i.type &&
531+
e.value.trim().toLowerCase() === i.value.trim().toLowerCase() &&
532+
e.platform === i.platform
533+
)
528534
}),
529535
)
530536

@@ -550,7 +556,7 @@ export default class ActivityService extends LoggerBase {
550556
const maybeToErase = toEraseMemberIdentities.find(
551557
(e) =>
552558
e.type === i.type &&
553-
e.value === i.value &&
559+
e.value.trim().toLowerCase() === i.value.trim().toLowerCase() &&
554560
(e.type === MemberIdentityType.EMAIL || e.platform === i.platform),
555561
)
556562

@@ -1761,7 +1767,8 @@ export default class ActivityService extends LoggerBase {
17611767
error.constructor &&
17621768
error.constructor.name === 'DatabaseError' &&
17631769
error.constraint &&
1764-
error.constraint === 'uix_memberIdentities_platform_value_type_verified'
1770+
(error.constraint === 'uix_memberIdentities_platform_value_type_verified' ||
1771+
error.constraint === 'uix_memberIdentities_platform_type_lower_value_verified')
17651772
) {
17661773
return true
17671774
}
@@ -2004,7 +2011,10 @@ export default class ActivityService extends LoggerBase {
20042011

20052012
for (const i1 of m1Identities) {
20062013
for (const i2 of m2Identities) {
2007-
if (i1.type === i2.type && i1.value === i2.value) {
2014+
if (
2015+
i1.type === i2.type &&
2016+
i1.value.trim().toLowerCase() === i2.value.trim().toLowerCase()
2017+
) {
20082018
return true
20092019
}
20102020
}

services/apps/data_sink_worker/src/service/dataSink.service.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,10 @@ export default class DataSinkService extends LoggerBase {
128128
// allowing retries to grow without bound. Now we respect the retry limit so the row
129129
// eventually reaches ERROR state instead of cycling forever.
130130
if (
131-
errorData.errorMessage.includes('uix_memberIdentities_platform_value_type_verified') &&
131+
(errorData.errorMessage.includes('uix_memberIdentities_platform_value_type_verified') ||
132+
errorData.errorMessage.includes(
133+
'uix_memberIdentities_platform_type_lower_value_verified',
134+
)) &&
132135
resultInfo.retries + 1 <= WORKER_SETTINGS().maxStreamRetries
133136
) {
134137
const delaySeconds = Math.floor(Math.random() * (120 - 10 + 1) + 10) * 60

0 commit comments

Comments
 (0)