fix: lowercase email-shaped identity values (CM-1349) - #4412
Conversation
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Normalizes email-shaped member identity and activity relation values while preserving non-email username casing.
Changes:
- Adds shared identity normalization.
- Applies normalization across DAL, APIs, and data-sink paths.
- Updates the identity-storage ADR.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
services/libs/common/src/member.ts |
Adds shared normalization helper. |
services/libs/data-access-layer/src/members/identities.ts |
Normalizes identity writes. |
services/libs/data-access-layer/src/activities/sql.ts |
Normalizes activity relation usernames. |
services/apps/data_sink_worker/src/service/member.service.ts |
Normalizes and deduplicates incoming identities. |
services/apps/data_sink_worker/src/service/activity.service.ts |
Normalizes activity usernames. |
backend/src/services/member/memberIdentityService.ts |
Uses shared normalization in UI identity operations. |
backend/src/api/public/v1/members/identities/createMemberIdentity.ts |
Normalizes public identity creation. |
backend/src/api/public/v1/members/createMember.ts |
Normalizes identities during member creation. |
docs/adr/0015-how-cdp-stores-member-identities.md |
Clarifies casing rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
docs/adr/0015-how-cdp-stores-member-identities.md:34
- The ADR names
isEmail, but the implementation deliberately uses the whole-valueisValidEmailvalidator. This documentation now points readers to the unanchored helper that caused the prior bug and misstates the storage rule.
Identity equality in CDP is `(platform, type, lower(value))`. Email vs username for storage casing is inferred from the value with `isEmail`, not from `type` (git often stores emails as `type=username`). Non-email usernames keep preferred casing from the source.
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
docs/adr/0015-how-cdp-stores-member-identities.md:13
- The ADR format rule requires
## Contextto contain 2–5 sentences (.claude/rules/adr-format.md:25), but this section now has six. Condense the final rationale into the preceding paragraph to keep the ADR compliant.
- **GitHub / GitLab**: usernames are case-insensitive for uniqueness, but case-preserving for display (`WillsonHG` and `willsonhg` are the same account; APIs return preferred casing).
- **Discord** (new usernames): forced lowercase.
- **Email**: stored and compared lowercase in practice.
backend/src/services/member/memberIdentityService.ts:138
- Each identity is inserted after trimming in the DAL, but the preflight conflict check uses the untrimmed value. Whitespace-padded input can therefore miss an existing identity here; because the later bulk insert uses
ON CONFLICT DO NOTHING, the requested identity is silently ignored and this method reports success instead of its intendedError409.
for (const identity of data) {
const conflict = await findMemberIdentityConflict(qx, {
value: identity.value,
platform: identity.platform,
type: identity.type,
services/apps/data_sink_worker/src/service/member.service.ts:343
- Normalization can remove every identity (for example, an all-whitespace username or an invalid
type=emailvalue), but the non-empty check runs before this call. The method then creates and returns a member with no identities, leaving an orphan that cannot be resolved. Recheck the invariant after normalization.
data.identities = normalizeMemberIdentities(data.identities, {
dropInvalidEmails: true,
})
services/apps/data_sink_worker/src/service/activity.service.ts:301
- For a whitespace-only supplied username, normalization produces
''but this truthy guard does not write it back. The laterif (!activity.username)therefore sees the original whitespace as truthy and skips the valid member-identity fallback, so the activity is processed with a blank username instead of its available identity.
const username = activity.username
? normalizeMemberIdentityValue(activity.username)
: undefined
if (username) {
activity.username = username
}
backend/src/services/member/memberIdentityService.ts:214
- When
data.valueis whitespace-padded, this conflict check uses the raw value whileupdateMemberIdentitytrims it before writing. The lookup can miss a verified identity owned by another member, after which the update hits the global unique index rather than producing the service's structured conflict response.
const value = data.value ?? currentIdentity.value
backend/src/api/public/v1/members/identities/createMemberIdentity.ts:58
- This lookup no longer uses the same trimmed value that the DAL inserts. For whitespace-padded input, an identity already on this member is missed; the subsequent normalized insert raises a conflict instead of taking the endpoint's idempotent
exactMatchpath (and its verified-state synchronization).
const existing = await findMemberIdentitiesByValue(tx, memberId, data.value, {
backend/src/services/member/memberIdentityService.ts:63
- The DAL trims before inserting, but this conflict lookup now receives the raw value. A request such as
" user@example.com "misses an existing normalized identity here; the laterON CONFLICT DO NOTHINGsilently skips the insert, so this create method reports success instead of its intendedError409conflict.
This issue also appears on line 134 of the same file.
value: data.value,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
backend/src/services/member/memberIdentityService.ts:136
- Each preflight conflict query now uses the untrimmed input, but the bulk insert trims values. A whitespace-padded duplicate can evade this check and then be silently skipped by
ON CONFLICT DO NOTHING, allowing a partially appliedcreateMultiplecall instead of the existing all-or-conflict behavior.
for (const identity of data) {
const conflict = await findMemberIdentityConflict(qx, {
value: identity.value,
backend/src/services/member/memberIdentityService.ts:214
- The conflict lookup now receives an untrimmed candidate, whereas
updateMemberIdentitytrims before persisting. A whitespace-padded duplicate can miss this check and fail later at the unique index with a raw database error rather than the service'sError409. Keep the lookup value aligned with the write normalization.
const value = data.value ?? currentIdentity.value
services/apps/data_sink_worker/src/service/activity.service.ts:342
- This now checks the unnormalized field rather than
username. For a whitespace-only input,usernameis''butactivity.usernameremains the original truthy whitespace (the assignment above only runs whenusernameis truthy), so the identity fallback is skipped and the activity retains an empty-shaped username. Use the normalized local for this decision.
if (!activity.username) {
services/apps/data_sink_worker/src/service/member.service.ts:343
- Normalization can remove every identity (for example, a payload containing only whitespace handles or invalid
type=emailvalues), but the non-empty check runs before this call. The create flow then persists a member with no identities, despite its own “Member must have at least one identity” invariant. Re-check after normalization.
data.identities = normalizeMemberIdentities(data.identities, {
dropInvalidEmails: true,
})
backend/src/api/public/v1/members/identities/createMemberIdentity.ts:58
- The DAL lookup is case-insensitive but does not trim its parameter. Since this endpoint accepts untrimmed strings, a request such as
" User@example.com "now misses the existing normalized row; the subsequent insert trims it ininsertMemberIdentitiesand hits the unique index, turning the endpoint's idempotent200path into a conflict. Trim before this lookup (as the removed normalization did).
const existing = await findMemberIdentitiesByValue(tx, memberId, data.value, {
backend/src/services/member/memberIdentityService.ts:63
- This conflict check no longer trims the value, while
insertMemberIdentitiestrims it before writing. An input such as" Foo "can therefore miss an existingFoo; because this insert usesON CONFLICT DO NOTHING, the create call silently succeeds instead of returning the priorError409contract. Normalize at least whitespace before checking.
This issue also appears in the following locations of the same file:
- line 134
- line 214
value: data.value,
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
Summary
Make member identity casing durable: email-shaped values are always stored lowercase (even when
typeisusername, e.g. git), usernames keep preferred casing, and equality/uniqueness stay on(platform, type, lower(value)).Changes
@crowd/common:normalizeMemberIdentityValue(isValidEmailshape check) andnormalizeMemberIdentities(normalize + same-batch case-variant dedupe, prefer verified)insertMemberIdentities/updateMemberIdentity(plus the legacyaddAsUnverifiedIdentityraw insert)normalizeMemberIdentities(replaces the misleadingvalidateEmailspath)usernameandobjectMemberUsernamethe same way; AR fallback lookups uselower(value)