externalIdMapping: optionally allow using array of column names #612
Conversation
📝 WalkthroughWalkthroughWidened externalIdMapping to accept Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
| } | ||
| } | ||
| } | ||
| // If not found, treat as a single column name (for single-column unique index with underscores) |
There was a problem hiding this comment.
should this fallback be included, ie. option 1)
lsmith77
force-pushed
the
externalIdMapping-array
branch
from
ce745ac to
4391157
Compare
lsmith77
changed the title
lsmith77
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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'); + });
lsmith77
force-pushed
the
externalIdMapping-array
branch
from
4391157 to
cd7a0cf
Compare
There was a problem hiding this comment.
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).
cd7a0cf to
a73882f
Compare
There was a problem hiding this comment.
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 hitsexternalIdMapping: { 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', }, });
…y of columns and not a unique key name
lsmith77
force-pushed
the
externalIdMapping-array
branch
from
a73882f to
4490ead
Compare
1 participant
make it optionally possible to define externalIdMapping using an array of columns and not a unique key name
fixes #611 using option 1) and 2)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.