externalIdMapping: optionally allow using mapped unique index names

[lsmith77]

so the actual bug is more trivial and somehow my brain totally misfired in the initial analysis.

fixes #611

Summary by CodeRabbit

• New Features

  • Support for identifying models by compound external ID keys remains available.

• Bug Fixes

  • ID resolution for single-field unique mappings now resolves using the configured external ID field name (improves lookup consistency); compound-key behavior unchanged.
  • Public API signatures were not changed.

• Tests

  • Test schema and flows updated to add a short_title external ID for posts and to validate retrieval via external IDs.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Updates ID resolution so single-field external-id mappings return the external ID field name (e.g., short_title) instead of the constraint field name; compound-unique handling unchanged. Tests and test schema updated to add short_title on Post and map Post to that external ID.

Changes

Cohort / File(s)	Summary
REST API implementation packages/server/src/api/rest/index.ts	Adjust single-field external ID resolution to return the external-id field name (name) instead of the constraint field name (info.type). Compound-unique path unchanged. (1 line modified)
Tests & schema packages/server/test/api/rest.test.ts	Add short_title String @unique() to Post in test schema; extend externalIdMapping to map Post → short_title; update test flows to create/retrieve posts via external IDs and assert relationships by external ID paths.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I nibbled names beneath the moonlit light,

Swapped a path so single-fields fit right.
Compound pals still march in line,
Short titles now wear the proper sign.
A hop, a tweak — the IDs delight.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: enabling externalIdMapping to use mapped unique index names to resolve the underscore parsing ambiguity.
Linked Issues check	✅ Passed	The PR successfully addresses issue #611 by implementing support for mapped unique index names, which resolves the bug where single-field unique indexes with underscores were incorrectly parsed.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the external_id_mapping underscore parsing issue and implementing the feature to support mapped unique index names.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@packages/server/src/api/rest/index.ts`:
- Around line 1343-1356: The single-field unique mapping branch uses
infoTyped.type (which is the field type, not the field name) when calling
requireField, causing lookups like requireField(model, infoTyped.type) to fail;
change that call to use the unique key name (the loop variable name) i.e. call
requireField(model, name) for the single-field case in the block that checks
typeof infoTyped.type === 'string' (symbols: externalIdMapping, modelLower,
modelDef.uniqueFields, infoTyped.type, requireField).

🧹 Nitpick comments (1)

packages/server/test/api/rest.test.ts (1)

3094-3211: Add regression test for string mapping to a single unique field.

Current cases cover compound key names and array-based mappings, but not { User: 'email_with_underscore' } (string), which is expected behavior. Adding that case will prevent regressions.

✅ Suggested test addition

+        it('works with id mapping (single column as string)', async () => {
+            client = await createTestClient(schema);
+            const _handler = new RestApiHandler({
+                schema: client.$schema,
+                endpoint: 'http://localhost/api',
+                externalIdMapping: {
+                    User: 'email_with_underscore',
+                },
+            });
+            handler = (args) => _handler.handleRequest({ ...args, url: new URL(`http://localhost/${args.path}`) });
+
+            await client.user.create({
+                data: { id: 4, name: 'User4', source: 'd', email_with_underscore: 'e4' },
+            });
+
+            const r = await handler({
+                method: 'get',
+                path: '/user/e4',
+                query: {},
+                client,
+            });
+            expect(r.status).toBe(200);
+            expect(r.body.data.attributes.email_with_underscore).toBe('e4');
+        });

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@packages/server/src/api/rest/index.ts`:
- Around line 1339-1369: The externalIdMapping resolution currently returns
fields (in the string fallback and Array paths) without ensuring they form a
unique or id constraint, which can cause downstream findUnique to fail; update
the logic in the externalIdMapping handling (the branches that return via
this.requireField(model, ...) and the Array.isArray(externalId) branch) to
validate that the resolved FieldDef names are either all part of
modelDef.idFields or match one of modelDef.uniqueFields compound keys (use
modelDef.uniqueFields entries and modelDef.idFields to check uniqueness), and if
they do not form a unique constraint throw a clear Error indicating the mapping
is not a unique/id key for the model (keep the existing unique-name handling
unchanged but add this validation before returning).

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

packages/server/src/api/rest/index.ts (1)

1336-1385: Uniqueness validation is order‑sensitive for array mappings.
If a caller provides ['source', 'name'] for a valid unique set defined as [name, source], this rejects it even though it’s still unique. Consider comparing sets (or document the required order).

✅ Possible fix (order‑insensitive uniqueness check)

-        const resolvedNames = resolved.map((f) => f.name);
+        const resolvedNames = resolved.map((f) => f.name);
+        const resolvedSet = new Set(resolvedNames);
+        if (resolvedSet.size !== resolvedNames.length) {
+            throw new Error(`Model ${model} externalIdMapping cannot contain duplicate fields`);
+        }
         const uniqueSets = this.getUniqueFieldSets(model);
         const isUnique =
-            uniqueSets.some(
-                (set) => set.length === resolvedNames.length && set.every((f, i) => f === resolvedNames[i]),
-            ) ||
-            (modelDef.idFields.length === resolvedNames.length &&
-                modelDef.idFields.every((f, i) => f === resolvedNames[i]));
+            uniqueSets.some(
+                (set) => set.length === resolvedSet.size && set.every((f) => resolvedSet.has(f)),
+            ) ||
+            (modelDef.idFields.length === resolvedSet.size &&
+                modelDef.idFields.every((f) => resolvedSet.has(f)));

🧹 Nitpick comments (2)

packages/server/src/api/rest/index.ts (1)

63-69: Remove the outdated externalIdMapping doc line.
Two consecutive doc blocks describe different semantics; the first still says “unique field name,” which is now misleading. Keep only the updated description.

packages/server/test/api/rest.test.ts (1)

3086-3211: Add coverage for the string form of single‑field underscore IDs.
The original bug also hits externalIdMapping: { User: 'email_with_underscore' }. The current test uses the array form (already covered by the compound‑array test). Consider switching this case (or adding a new one) to the string form.

🧪 Minimal test tweak

-            const _handler = new RestApiHandler({
+            const _handler = new RestApiHandler({
                 schema: client.$schema,
                 endpoint: 'http://localhost/api',
                 externalIdMapping: {
-                    User: ['email_with_underscore'],
+                    User: 'email_with_underscore',
                 },
             });

[lsmith77]

I will double check what is going wrong here.

[ymc9]

Hi @lsmith77 , thanks for working on this. I've left a few comments.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@packages/server/src/api/rest/index.ts`:
- Around line 1341-1374: The @@unique(map: ...) handling currently only
processes compound keys (fieldsArg.value.items.length > 1) and ignores
single-field mapped uniques; update the branch that inspects uniqueAttr and
fieldsArg so that when fieldsArg.value.items.length === 1 you extract the single
field name using the same logic used for compound items (inspect item.field or
item.value.name, validate against modelDef.fields, throw the same Invalid unique
field mapping error on failure) and then return this.requireField(model,
fieldName); ensure you reuse the same symbols (externalIdName, uniqueAttr,
fieldsArg, modelDef, requireField, model) and keep the error behavior consistent
with the existing compound-path.

🧹 Nitpick comments (1)

packages/server/test/api/rest.test.ts (1)

3296-3305: Test name implies array input, but the mapping is a string.

Consider renaming to reflect the mapped compound unique name to avoid confusion.

✏️ Suggested rename

-        it('works with externalIdMapping as array of fields (compound unique index)', async () => {
+        it('works with externalIdMapping using mapped compound unique name', async () => {

[lsmith77]

Hi @lsmith77 , thanks for working on this. I've left a few comments.

are the tests currently flaky?

BTW: I tried a different approach that felt cleaner than allowing both strings and arrays by adding support for mapped unique index names. It is a lot of code though as schema.ts doesn't compile down attributes.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@packages/server/src/api/rest/index.ts`:
- Around line 1343-1371: The leading comment "// 1. Model-level @@unique(map:
...) (compound keys only)" is now inaccurate because the code checks
fieldsArg.value.items.length >= 1 and thus supports single-field unique
mappings; update that comment near the block handling modelDef.attributes (the
uniqueAttr/fieldsArg logic) to indicate it supports single-field and compound
keys (e.g., "Model-level @@unique(map: ...) (single-field or compound keys)") so
future readers of the uniqueAttr / fieldsArg / requireField handling for model
and externalIdName see the correct behavior.

🧹 Nitpick comments (1)

packages/server/test/api/rest.test.ts (1)

3203-3222: Make the single‑field mapped‑name coverage explicit.
Because short_title is both the field name and the map name, this test would still pass without the new map‑resolution logic. Consider using a distinct map name so the test proves the new behavior.

🧪 Suggested tweak

-        short_title String `@unique`(map: "short_title")
+        short_title String `@unique`(map: "image_short_title")
@@
-                    Image: 'short_title',
+                    Image: 'image_short_title',

[lsmith77]

@ymc9 could you have another look at this PR?

[ymc9]

Hi @lsmith77 , sorry for the delay. Please check my additional comments.

[lsmith77]

@ymc9 I have reduced the scope of the PR to just the most obvious bug fix.

[ymc9]

@ymc9 I have reduced the scope of the PR to just the most obvious bug fix.

Awesome! Merging it and will release v3.3 shortly.