refactor: canonical ID unification across API and models#2626
refactor: canonical ID unification across API and models#2626
Conversation
SafeDep Report Summary
Package Details
This report is generated by SafeDep Github App |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR refactors the core server to standardize on a canonical id field (vs MongoDB _id) across transport, controllers/services, lean results, and related utilities, backed by a new shared ID module and expanded test coverage.
Changes:
- Introduces shared branded ObjectId string utilities/schemas and updates Zod/DTO boundaries to use them.
- Reworks lean/serialization normalization to expose
idand strip/avoid_id, plus broad controller/service updates to remove_idfallbacks. - Updates tests and some dependencies (notably
better-auth) to align with the new identifier contract.
Reviewed changes
Copilot reviewed 72 out of 73 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updates lockfile for dependency upgrades (incl. better-auth ecosystem). |
| docs/superpowers/specs/2026-04-11-canonical-id-refactor-design.md | Adds the canonical-id design spec and invariants. |
| docs/superpowers/plans/2026-04-11-canonical-id-refactor-implementation.md | Adds an execution plan and test strategy for the refactor. |
| apps/core/test/src/shared/model/lean-id.spec.ts | Adds unit tests for document/lean id normalization behavior. |
| apps/core/test/src/shared/id/id.util.spec.ts | Adds unit tests for shared ID parsing/branding and conversions. |
| apps/core/test/src/modules/topic/topic.controller.e2e-spec.ts | Updates topic translation e2e to use id instead of _id. |
| apps/core/test/src/modules/serverless/serverless.service.spec.ts | Adds/updates serverless tests around owner identity shape. |
| apps/core/test/src/modules/search/search.service.spec.ts | Updates search tests to rely on canonical id. |
| apps/core/test/src/modules/search/search-document.util.spec.ts | Updates search document builder tests to require id. |
| apps/core/test/src/modules/recently/recently.model.spec.ts | Adds tests for RecentlyModel.refId behavior with normalized refs. |
| apps/core/test/src/modules/recently/recently.controller.e2e-spec.ts | Adds an e2e assertion that /recently/latest omits _id. |
| apps/core/test/src/modules/owner/owner.service.spec.ts | Adds unit coverage ensuring owner info returns id only. |
| apps/core/test/src/modules/note/note.translation-entry.e2e-spec.ts | Updates translation-entry e2e to use topic.id. |
| apps/core/test/src/modules/note/note.service.spec.ts | Updates note service tests to expect canonical id. |
| apps/core/test/src/modules/file/file.controller.spec.ts | Adds coverage for orphan file listing using canonical ids. |
| apps/core/test/src/modules/draft/draft.service.spec.ts | Expands draft tests to validate canonical-id flows in deletes. |
| apps/core/test/src/modules/comment/comment-lifecycle.spec.ts | Adds coverage for canonical-id link building in comment lifecycle. |
| apps/core/test/src/modules/comment/comment-anchor.spec.ts | Updates anchor-resolution tests to use canonical ids. |
| apps/core/test/src/modules/category/category.service.spec.ts | Adds unit coverage for canonical id usage in category service. |
| apps/core/test/src/modules/category/category.controller.spec.ts | Adds controller test ensuring translation uses canonical ids only. |
| apps/core/test/src/modules/ai/translation-entry.service.spec.ts | Updates translation-entry service tests to use id. |
| apps/core/test/src/modules/ai/translation-entry.interceptor.spec.ts | Updates interceptor tests to idField: 'id' and id payloads. |
| apps/core/test/src/modules/ai/ai-summary.service.spec.ts | Adds coverage for summary grouping using canonical ids. |
| apps/core/test/src/modules/ai/ai-slug-backfill.service.spec.ts | Updates slug backfill tests to work with id on lean results. |
| apps/core/test/src/modules/aggregate/aggregate.service.spec.ts | Adds unit coverage for aggregate top-articles returning canonical ids. |
| apps/core/src/shared/model/plugins/lean-id.ts | Implements recursive id normalization for lean (and shared utility usage). |
| apps/core/src/shared/model/base.model.ts | Applies canonical normalization in toJSON/toObject transforms. |
| apps/core/src/shared/id/index.ts | Adds shared ID module barrel exports. |
| apps/core/src/shared/id/id.util.ts | Implements parsing, branding, normalization, and ObjectId conversions. |
| apps/core/src/shared/id/id.type.ts | Defines branded ObjectIdString and entity ID aliases. |
| apps/core/src/shared/id/id.schema.ts | Defines Zod schema + pattern for ObjectId strings. |
| apps/core/src/shared/dto/id.dto.ts | Updates DTO typing/regex paths to align with canonical id branding. |
| apps/core/src/processors/gateway/web/visitor-event-dispatch.service.ts | Removes _id fallback when dispatching visitor events. |
| apps/core/src/modules/topic/topic.controller.ts | Switches translation rules from _id to id. |
| apps/core/src/modules/serverless/serverless.service.ts | Normalizes owner data and adjusts returned identity fields. |
| apps/core/src/modules/search/search.service.ts | Normalizes inputs before building search documents. |
| apps/core/src/modules/search/search-document.util.ts | Requires id and derives refId solely from id. |
| apps/core/src/modules/recently/recently.service.ts | Normalizes aggregate/lookup results and uses canonical ids in queries. |
| apps/core/src/modules/recently/recently.model.ts | Updates refId getter to prefer id. |
| apps/core/src/modules/post/post.controller.ts | Switches translation id fields to id and removes _id fallback usage. |
| apps/core/src/modules/page/page.controller.ts | Removes _id fallback usage in translation flows. |
| apps/core/src/modules/owner/owner.service.ts | Normalizes reader owner identity and removes _id from output model. |
| apps/core/src/modules/owner/owner.model.ts | Removes _id from the owner model contract. |
| apps/core/src/modules/note/note.service.ts | Removes _id fallback reads in service logic; returns canonical ids. |
| apps/core/src/modules/note/note.controller.ts | Enforces id-only list items and translation lookup keys. |
| apps/core/src/modules/markdown/markdown.service.ts | Adjusts category id handling to use canonical id. |
| apps/core/src/modules/link/link.service.ts | Uses canonical id when updating by _id. |
| apps/core/src/modules/link/link-avatar.service.ts | Uses canonical id in logging and avatar migration flows. |
| apps/core/src/modules/file/file.controller.ts | Returns id instead of _id in orphan file responses. |
| apps/core/src/modules/draft/draft.service.ts | Uses canonical id when removing references. |
| apps/core/src/modules/comment/comment.service.ts | Extends id handling to accept id and updates several _id usages. |
| apps/core/src/modules/comment/comment.lifecycle.service.ts | Removes _id fallback in reply flow and uses canonical ids in URLs. |
| apps/core/src/modules/comment/comment.controller.ts | Removes _id fallback usage when calling lifecycle and deletes. |
| apps/core/src/modules/category/category.service.ts | Uses canonical id for counting/posts and normalizes emitted payloads. |
| apps/core/src/modules/category/category.controller.ts | Uses idField: 'id' and canonical ids for tag/category flows. |
| apps/core/src/modules/auth/auth.service.ts | Normalizes Mongo-driver docs to consistently return canonical ids. |
| apps/core/src/modules/ai/ai-writer/ai-slug-backfill.service.ts | Updates note updates to use note.id instead of note._id. |
| apps/core/src/modules/ai/ai-translation/translation-entry.service.ts | Collects translation lookup keys via canonical id. |
| apps/core/src/modules/ai/ai-translation/ai-translation.types.ts | Adjusts event payload typing toward id-centric payloads. |
| apps/core/src/modules/ai/ai-translation/ai-translation.service.ts | Removes _id extraction fallback but keeps some legacy payload handling. |
| apps/core/src/modules/ai/ai-translation/ai-translation-event-handler.service.ts | Types handler payloads and switches to canonical id usage. |
| apps/core/src/modules/ai/ai-summary/ai-summary.service.ts | Uses canonical ids for matched ref IDs. |
| apps/core/src/modules/aggregate/aggregate.service.ts | Replaces _id picks/returns with id. |
| apps/core/src/modules/aggregate/aggregate.controller.ts | Removes _id translation fallbacks and uses id in translation inputs. |
| apps/core/src/modules/activity/activity.service.ts | Normalizes docs and maps by canonical ids. |
| apps/core/src/modules/activity/activity.controller.ts | Removes _id-based shaping and uses canonical ids for translation. |
| apps/core/src/migration/version/v10.4.3.ts | Normalizes docs before building search index documents. |
| apps/core/src/common/zod/primitives.ts | Replaces zMongoId with shared zObjectIdString and tweaks hex regex. |
| apps/core/src/common/zod/index.ts | Re-exports zObjectIdString from shared id module. |
| apps/core/src/common/interceptors/translation-entry.interceptor.ts | Restricts entity translations to idField: 'id'. |
| apps/core/src/common/decorators/translate-fields.decorator.ts | Narrows idField to 'id' only. |
| apps/core/package.json | Upgrades better-auth and related packages. |
| apps/core/nest-cli.json | Disables Nest CLI builder type checking. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "compilerOptions": { | ||
| "builder": "swc", | ||
| "typeCheck": true, | ||
| "typeCheck": false, |
There was a problem hiding this comment.
typeCheck is turned off for the Nest CLI SWC builder. This removes a build-time safety net and can let type errors ship even if CI doesn’t run pnpm typecheck for this app. If there’s a specific performance reason to disable it, consider keeping it enabled in CI builds (or re-enabling it here) and relying on pnpm -C apps/core run typecheck for local speed instead.
| "typeCheck": false, | |
| "typeCheck": true, |
| get refId() { | ||
| return (this.ref as any)?._id ?? this.ref | ||
| return (this.ref as any)?.id ?? (this.ref as any)?._id ?? this.ref | ||
| } |
There was a problem hiding this comment.
refId still falls back to (this.ref as any)?._id, which keeps _id-based compatibility logic in application-layer models. This undermines the goal of making _id a persistence-only detail and makes it harder to enforce/grep for regressions. If legacy populated refs are expected, prefer normalizing refs at the query boundary (or in the lean/serialization layer) so RecentlyModel.refId can be return (this.ref as any)?.id ?? this.ref without _id access.
| .then(async (res) => { | ||
| res.docs.forEach((doc) => normalizeDocumentIds(doc)) | ||
| const translationInputs: ArticleTranslationInput[] = [] | ||
| for (const doc of res.docs) { |
There was a problem hiding this comment.
In the aggregate-paginate pipeline, $lookup injects category documents that only have _id, but the controller only calls normalizeDocumentIds(doc) (no schema), which normalizes the root _id but won’t recurse into the nested category. Since @TranslateFields now uses idField: 'id' for docs[].category.name, translations will fail to collect/replace category values because category.id won’t exist. Pass the Post schema to normalizeDocumentIds (e.g., normalizeDocumentIds(doc, this.postService.model.schema)) or project category.id in the aggregation pipeline and remove category._id before returning.
| return { | ||
| id: owner._id.toString(), | ||
| _id: owner._id, | ||
| username: owner.username ?? owner.handle ?? '', | ||
| name: owner.name, | ||
| // Preserve the published sandbox contract while exposing canonical id. | ||
| _id: ownerId, | ||
| id: ownerId, | ||
| username: |
There was a problem hiding this comment.
mockGetOwner() returns both id and legacy _id in the public payload. This conflicts with the canonical-id contract described in the added spec (no _id in transport/payloads) and reintroduces dual-shape identifier handling. If _id must remain for an external sandbox contract, consider isolating it behind an explicit versioned endpoint/DTO (or a separate “legacy” response type) so the rest of the application can stay strictly id-only.
| if (typeof id === 'object' && id && '_id' in id) { | ||
| return this.toObjectId((id as { _id?: unknown })._id as any) | ||
| } | ||
|
|
||
| if (typeof id === 'object' && id && 'id' in id) { |
There was a problem hiding this comment.
toObjectId still reads from _id/id on arbitrary objects, which keeps mixed-shape identifier handling inside the service layer. For a strict canonical-id refactor, prefer accepting only a canonical id (ideally the shared branded type) and keep any _id/ObjectId coercion strictly at the persistence boundary using the shared ~/shared/id utilities.
| if ('data' in event && typeof event.data === 'string') { | ||
| return event.data | ||
| } |
There was a problem hiding this comment.
extractIdFromEvent still accepts the legacy { data: string } payload shape. If this refactor is meant to enforce a single canonical { id } event contract, keeping data here preserves compatibility paths and makes event typing/enforcement harder. Consider removing the data branch and narrowing payloads to { id: ... } (or centralizing any one-off legacy adaptation strictly at the event boundary).
| if ('data' in event && typeof event.data === 'string') { | |
| return event.data | |
| } |
| if ('_id' in value) { | ||
| if (preserveOriginalId) { | ||
| hidePropertyFromEnumeration(value, '_id') | ||
| } else { | ||
| Reflect.deleteProperty(value, '_id') |
There was a problem hiding this comment.
For lean query results, _id is preserved as a non-enumerable property instead of being removed. This means values still “contain” _id (e.g., '_id' in obj is true), which enables accidental fallbacks and conflicts with the invariant that lean results should not contain _id. Consider deleting _id even in the lean path, or making retention an explicit opt-in for the few persistence-only call sites that truly need it.
![chatgpt-codex-connector[bot]](https://avatars.gh.lumenfield.work/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c4bed381d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| path: 'docs[].category.name', | ||
| keyPath: 'category.name', | ||
| idField: '_id', | ||
| idField: 'id', |
There was a problem hiding this comment.
_id in post list
getPaginate builds docs via aggregation + $lookup, then calls normalizeDocumentIds(doc) without schema context (line 164), which only normalizes the root document and leaves doc.category._id unchanged. With idField: 'id' here, TranslationEntryInterceptor cannot collect IDs for docs[].category.name, so category-name translations silently stop applying on GET /posts when lang is set. Either keep this rule on _id for this aggregated shape or explicitly normalize nested category before interception.
Useful? React with 👍 / 👎.
The $lookup-produced category subdocs only had _id, not id. Without a schema, normalizeDocumentIds could not recurse into nested refs, causing TranslationEntryInterceptor to silently skip category-name translations when idField was changed to 'id'.
Summary
Unifies identifier handling across the codebase: shared
idutilities (id.schema,id.type,id.util), extendedlean-idplugin behavior, and updates across controllers/services.Includes migration
v10.4.3, Zod primitive tweaks, and RFC notes underrfcs/.Testing
Run
pnpm testbefore merge.