feat: support natural keys in the API #2188
Conversation
📝 Walkthrough## Walkthrough
This change introduces optional support for external ID mapping in the REST API request handler. It adds an optional `externalIdMapping` option, a new private method for ID field resolution that prioritizes external mappings, and updates all internal usages to leverage the new logic, affecting ID extraction and serialization processes.
## Changes
| Cohort / File(s) | Change Summary |
|------------------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------|
| **REST API Handler: External ID Mapping** <br> `packages/server/src/api/rest/index.ts` | Adds optional `externalIdMapping` option, introduces private `getIdFields` method with external mapping support, replaces direct global calls with method calls, and normalizes mapping keys. |
| **REST API Handler Tests: External ID Mapping** <br> `packages/server/tests/api/rest.test.ts` | Adds new test suite verifying REST API behavior with external ID mapping, including compound unique index usage and related entity resolution. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant RequestHandler
participant ModelMeta
Client->>RequestHandler: API request with model
RequestHandler->>RequestHandler: getIdFields(modelMeta, model)
alt External ID mapping exists
RequestHandler->>ModelMeta: Lookup unique constraints for external key
ModelMeta-->>RequestHandler: Return ID fields
else No external ID mapping
RequestHandler->>Global: getIdFields(modelMeta, model)
Global-->>RequestHandler: Return ID fields
end
RequestHandler-->>Client: Processed response using resolved ID fieldsEstimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
|
lsmith77
changed the title
![coderabbitai[bot]](https://app.smartcontext.ai/default/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/schema/src/res/stdlib.zmodel (1)
741-744: Enhance documentation with usage examples and validation guidance.The new
@@externalIdattribute definition is syntactically correct, but the documentation could be more comprehensive to help developers understand its intended usage.Consider expanding the documentation to include:
- Usage examples showing how it relates to unique constraints
- Clarification that the
nameparameter should match an existing unique constraint name- Explanation of how this integrates with the runtime behavior
/** - * Specifies an external identifier for the model other than a primary key + * Specifies an external identifier for the model other than a primary key. + * The name parameter should match an existing unique constraint name that will be used + * for external API interactions instead of the primary key. + * + * @param name: The name of the unique constraint to use as external identifier + * + * @example + * model Tag { + * id Int @id @default(autoincrement()) + * name String @unique + * + * @@externalId("name") + * } */ attribute @@externalId(_ name: String)packages/runtime/src/cross/utils.ts (1)
59-61: Consider adding type safety for attribute argument access.The current implementation assumes a specific structure for the attribute arguments without type checking.
Consider adding type validation:
- if (externalId && externalId.args[0] !== undefined) { - entries = entries.filter((entry) => entry.name === externalId.args[0].value); + if (externalId && externalId.args?.[0]?.value && typeof externalId.args[0].value === 'string') { + const externalIdName = externalId.args[0].value as string; + entries = entries.filter((entry) => entry.name === externalIdName);This ensures the external ID name is actually a string before using it for filtering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime/src/cross/utils.ts(1 hunks)packages/schema/src/res/stdlib.zmodel(1 hunks)
There was a problem hiding this comment.
Hi @lsmith77 ,
I have two main concerns with introducing the @externalId attribute:
- A toplevel attribute ideally should have a meaning from data modeling point of view. From Prisma ORM API point view, one can already address an entity either with an id or a unique field, so such a configuration feels redundant.
- The
getIdFieldshelper is called in many places in the code base, and there might be places where it's assumed it returns ID fields ... I'm not sure though, just feeling changing its semantic can be quite dangerous.
Do you feel again this can be a configuration just at the rest api handler (and open api) level? Since its effect is just there.
|
I can certainly refactor it to be a Handler option like the model name mapping. That being said, the argument you bring forth to some degree also applies to the As for changing the semantics of Your proposal would rather be we do not call this method but rather receive a list of fields from the Handler config, which is then only checked at runtime against the defined fields (in order to map the model attributes to RDBMS field names to handle if the model used I guess this would end up duplicating a fair bit of code which comes with its own set of risks. The advantage is of course it should prevent unwanted side-effects and whatever bugs it introduces would be isolated to this feature. If you feel more comfortable with this approach I am happy to implement it. |
|
FTR I just tried to quickly implement this but it appears that it is not enough to override all calls to So it seems like further investigation is needed where all getIdFields() needs to be overridden to get the desired change in behavior. Which however then might lead to additional complexity introduced in even more places :-/ |
Hi @lsmith77 , sorry I didn't put it clearly in my previous comments. What I meant was The rules are effective in manually written backend code: const db = enhance(prisma, { user: getAuthUser() });
// only posts allowed to be read are returned
const posts = await db.post.findMany();, and when requested from automatic http api: // requesting "/api/model/post" will only return readable posts
app.use(
'/api/model',
ZenStackMiddleware({
handler: RestApiHandler(...),
getPrisma: (request) => enhance(prisma, { user: getAuthUser(request) }),
})
); |
|
OK thx for this additional explanation. |
Yes, an option in the rest handler sounds good to me. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/server/src/api/rest/index.ts (1)
1179-1195: Good proof of concept implementation with room for future enhancements.This implementation successfully demonstrates the external ID concept by leveraging existing unique constraints. As mentioned in the PR objectives, future enhancements could include:
- Validation at schema compilation time to ensure
@@externalIdreferences an existing unique constraint- Storing external ID metadata in the model to avoid runtime lookups
- Adding support for composite external IDs if needed
The current approach is clean and appropriate for validating the concept.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime/src/cross/utils.ts(1 hunks)packages/server/src/api/rest/index.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/runtime/src/cross/utils.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ymc9
PR: zenstackhq/zenstack#2187
File: packages/server/src/api/rest/index.ts:287-289
Timestamp: 2025-07-16T03:09:04.699Z
Learning: In REST API URL pattern matching with model name mapping, only the `type` parameter (representing model names) should be mapped using `modelNameMapping`. The `relationship` parameter represents field names on models and should not be mapped, as field names are distinct from model names in the API structure.
packages/server/src/api/rest/index.ts (1)
Learnt from: ymc9
PR: #2187
File: packages/server/src/api/rest/index.ts:287-289
Timestamp: 2025-07-16T03:09:04.699Z
Learning: In REST API URL pattern matching with model name mapping, only the type parameter (representing model names) should be mapped using modelNameMapping. The relationship parameter represents field names on models and should not be mapped, as field names are distinct from model names in the API structure.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: dependency-review
- GitHub Check: OSSAR-Scan
- GitHub Check: build-test (20.x)
🔇 Additional comments (3)
packages/server/src/api/rest/index.ts (3)
57-57: Good architectural choice for the external ID mapping option.Making
externalIdMappinga required option ensures explicit configuration and follows the same pattern as other mapping options in the handler.
259-262: Consistent normalization of external ID mapping.The normalization follows the same pattern as
modelNameMapping, ensuring consistent handling of model names throughout the handler.
1200-1200: All ID field retrievals correctly updated to use the new method.The consistent replacement of the global
getIdFieldsfunction with the instance method ensures that external ID mappings are applied uniformly across all operations including type mapping, serialization, and relationship handling.Also applies to: 1216-1216, 1244-1244, 1296-1296, 1336-1336
lsmith77
force-pushed
the
externalid
branch
2 times, most recently
from
3d5022e to
20410b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/server/src/api/rest/index.ts (1)
1179-1195: Improve error handling and fix potential runtime errorsThe implementation has several issues that need to be addressed:
Potential undefined access: The code doesn't handle the case where
modelMeta.models[modelLower]is undefined before accessinguniqueConstraints.Inconsistent model name usage: Using
modelin error messages andrequireFieldwhile usingmodelLowerfor metadata lookup could cause confusion.Apply this diff to fix these issues:
private getIdFields(modelMeta: ModelMeta, model: string): FieldInfo[] { const modelLower = lowerCaseFirst(model); if (!(modelLower in this.externalIdMapping)) { return getIdFields(modelMeta, model); } - const metaData = modelMeta.models[modelLower] ?? {}; + const metaData = modelMeta.models[modelLower]; + if (!metaData) { + throw new Error(`Model ${modelLower} not found in metadata`); + } const externalIdName = this.externalIdMapping[modelLower]; const uniqueConstraints = metaData.uniqueConstraints ?? {}; + const availableKeys = Object.keys(uniqueConstraints); for (const [name, constraint] of Object.entries(uniqueConstraints)) { if (name === externalIdName) { - return constraint.fields.map((f) => requireField(modelMeta, model, f)); + return constraint.fields.map((f) => requireField(modelMeta, modelLower, f)); } } - throw new Error(`Model ${model} does not have unique key ${externalIdName}`); + throw new Error(`Model ${modelLower} does not have unique key "${externalIdName}". Available unique keys: ${availableKeys.join(', ') || 'none'}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server/src/api/rest/index.ts(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/server/src/api/rest/index.ts (1)
Learnt from: ymc9
PR: #2187
File: packages/server/src/api/rest/index.ts:287-289
Timestamp: 2025-07-16T03:09:04.699Z
Learning: In REST API URL pattern matching with model name mapping, only the type parameter (representing model names) should be mapped using modelNameMapping. The relationship parameter represents field names on models and should not be mapped, as field names are distinct from model names in the API structure.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: dependency-review
🔇 Additional comments (2)
packages/server/src/api/rest/index.ts (2)
244-244: LGTM!The initialization correctly handles the optional mapping and normalizes keys consistently with the model name mapping pattern.
Also applies to: 259-263
1200-1200: Consistent application of external ID mappingAll calls to retrieve ID fields have been correctly updated to use the new instance method, ensuring external ID mapping is applied uniformly throughout the handler.
Also applies to: 1216-1216, 1244-1244, 1296-1296, 1336-1336
| private getIdFields(modelMeta: ModelMeta, model: string): FieldInfo[] { | ||
| const modelLower = lowerCaseFirst(model); | ||
| if (!(modelLower in this.externalIdMapping)) { | ||
| return getIdFields(modelMeta, model); | ||
| } | ||
|
|
||
| const metaData = modelMeta.models[modelLower] ?? {}; | ||
| const externalIdName = this.externalIdMapping[modelLower]; | ||
| const uniqueConstraints = metaData.uniqueConstraints ?? {}; | ||
| for (const [name, constraint] of Object.entries(uniqueConstraints)) { | ||
| if (name === externalIdName) { | ||
| return constraint.fields.map((f) => requireField(modelMeta, model, f)); | ||
| } | ||
| } | ||
|
|
||
| throw new Error(`Model ${model} does not have unique key ${externalIdName}`); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Document the implications of external ID mapping
This feature creates a separation between API-level IDs (natural keys) and database-level IDs (surrogate keys). This could lead to confusion when debugging or writing custom backend code.
Consider:
- Adding comprehensive documentation about this behavior
- Providing utility functions to convert between external and internal IDs
- Ensuring error messages clearly indicate which type of ID is expected
To verify potential issues with ID mismatches, let's check if there are any direct ID references in error messages or logging:
🏁 Script executed:
#!/bin/bash
# Search for places where IDs are used in error messages or logging
rg -A 3 -B 3 "resourceId|item\.id|\.id\b" packages/server/src/api/rest/index.ts | grep -E "(error|Error|log|Log|throw|makeError)"
# Check if there are any other files that might be affected by this change
fd -e ts -e js . packages/server/src | xargs rg "getIdFields.*modelMeta"Length of output: 1198
Document and Enhance External ID Mapping Support
The REST handler now accepts natural keys at the API boundary while Prisma continues to use surrogate IDs, which can be confusing when diving into database operations or writing custom backend logic. Let’s make this explicit and provide tooling around it.
• Add documentation (e.g. in docs/api.md or inline JSDoc) noting that
– API endpoints use “external”/natural IDs
– all Prisma calls still use the internal surrogate key
• Create utility functions (e.g. toExternalId(id: number): string, toInternalId(natural: string): number) and export them from packages/server/src/api/rest/utils.ts
• In packages/server/src/api/rest/index.ts (around getIdFields):
– Update thrown errors to specify “external ID” vs. “internal ID”
– Add a brief comment above getIdFields summarizing its role in mapping between ID types
This will ensure maintainers and integrators understand the dual-ID model, avoid debugging confusion, and have ready-to-use conversion helpers.
🤖 Prompt for AI Agents
In packages/server/src/api/rest/index.ts around lines 1179 to 1195, add a brief
JSDoc comment above the getIdFields method explaining that it maps between
external (natural) IDs used at the API boundary and internal surrogate IDs used
by Prisma. Update the error message thrown to clearly distinguish that the
missing key is an external ID, not an internal one. Additionally, create and
export utility functions named toExternalId and toInternalId in
packages/server/src/api/rest/utils.ts to convert between internal numeric IDs
and external string IDs. Finally, add documentation in docs/api.md or inline
noting that API endpoints use external IDs while Prisma uses internal surrogate
keys to clarify this dual-ID model for maintainers and integrators.
…T API this makes it possible to use a natural key even while also using surrogate primary keys
lsmith77
force-pushed
the
externalid
branch
2 times, most recently
from
a682d40 to
3b4a931
Compare
|
@ymc9 ok got it to work. the issue came from the second commit is not necessary but felt like a sensible optimization but I am happy to revert if you prefer. |
|
I will now look into updating the openapi plugin so that Actually upon further investigation it appears that right now you just list all the compound id fields as separate filters, so I guess there isn't really anything to do here. We will likely just add our own logic to remove |
|
Hmm we would however like to not include the What would you advise here? How could be prevent exposing 
|
|
ok found a solution using |
|
@ymc9 could you have another look if this is now the right direction and what is left to do to get this merged? |
| private getIdFields(modelMeta: ModelMeta, model: string): FieldInfo[] { | ||
| const modelLower = lowerCaseFirst(model); | ||
|
|
||
| if (this.typeMap && modelLower in this.typeMap) { |
There was a problem hiding this comment.
this is a performance optimization but not sure if its really needed
There was a problem hiding this comment.
I believe after typeMap is built, every model with proper id fields will have an entry in it. Will this code block then completely mute the "externalIdMapping" check below?
There was a problem hiding this comment.
Yes that was the idea as my assumption was that there is no way things could change at run time.
| private getIdFields(modelMeta: ModelMeta, model: string): FieldInfo[] { | ||
| const modelLower = lowerCaseFirst(model); | ||
|
|
||
| if (this.typeMap && modelLower in this.typeMap) { |
There was a problem hiding this comment.
I believe after typeMap is built, every model with proper id fields will have an entry in it. Will this code block then completely mute the "externalIdMapping" check below?
| const metaData = modelMeta.models[modelLower] ?? {}; | ||
| const externalIdName = this.externalIdMapping[modelLower]; | ||
| const uniqueConstraints = metaData.uniqueConstraints ?? {}; | ||
| for (const [name, constraint] of Object.entries(uniqueConstraints)) { |
There was a problem hiding this comment.
There's some complexity here due to how Prisma handles multi-field id fields or unique fields.
If a model has id like:
model Model {
...
@@id([x, y])
}The model meta's uniqueConstraint for this model will have an entry like:
{
...
x_y: { name: 'x_y', fields: ['x', 'y']
}, and when querying, the synthetic compound field "x_y" must be used as a unique filter.
db.model.findUnique({ where: { x_y: { x: 1, y: 2 } });For this new feature, do you plan to support mapping multi-field id/unique fields? That'll result in some more complexity. However, if not, we can have a check in the loop and only match unique constraint that has a single field.
I don't personally see too much value to support multi field.
There was a problem hiding this comment.
For @id fields, this feature is not needed, since the current code already handles this. so I am only supporting model (not field level) @unique. I have added an example at the very top of this PR.
There was a problem hiding this comment.
OK, there is still an issue here.
If I map the unique index name to something arbitrary, it doesn't work as the current code calling findUnqiue() apparently always then uses the fieldA_fieldB in the where rather than the unique index name, causing an error.
But digging into the source code I found that this is a known limitation:
https://github.com/zenstackhq/zenstack/blob/main/packages/server/src/api/rest/index.ts#L1415
|
Hi @lsmith77 , I've left a few review comments. Please take a look when you have time. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/server/src/api/rest/index.ts(9 hunks)packages/server/tests/api/rest.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/src/api/rest/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build-test (20.x)
- GitHub Check: OSSAR-Scan
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: dependency-review
| it('works with id mapping', async () => { | ||
| await prisma.user.create({ | ||
| data: { id: 1, name: 'User1', email: 'user1@abc.com' }, | ||
| }); | ||
|
|
||
| // user is no longer exposed using the `id` field | ||
| let r = await handler({ | ||
| method: 'get', | ||
| path: '/user/1', | ||
| query: {}, | ||
| prisma, | ||
| }); | ||
|
|
||
| expect(r.status).toBe(404); | ||
|
|
||
| // user is exposed using the fields from the `name__email` multi-column unique index | ||
| r = await handler({ | ||
| method: 'get', | ||
| path: '/user/User1_user1@abc.com', | ||
| query: {}, | ||
| prisma, | ||
| }); | ||
|
|
||
| expect(r.status).toBe(200); | ||
| expect(r.body.data.attributes.email).toBe('user1@abc.com'); | ||
| expect(r.body.data.attributes.name).toBe('User1'); | ||
|
|
||
| await prisma.post.create({ | ||
| data: { id: 1, title: 'Title1', authorId: 1 }, | ||
| }); | ||
|
|
||
| // post is exposed using the `id` field | ||
| r = await handler({ | ||
| method: 'get', | ||
| path: '/post/1', | ||
| query: {}, | ||
| prisma, | ||
| }); | ||
|
|
||
| expect(r.status).toBe(200); | ||
| expect(r.body.data.attributes.title).toBe('Title1'); | ||
| expect(r.body.data.attributes.author.name).toBe('User1'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for external ID mapping feature.
The current test only covers GET operations. To ensure comprehensive testing of the external ID mapping feature, consider adding tests for:
- POST operations with relationships using external IDs
- PUT operations on resources with external IDs
- DELETE operations on resources with external IDs
- Relationship operations (POST, PATCH, DELETE on relationships)
- Error scenarios (malformed external IDs, non-existent resources)
Example additional test cases:
// Test POST with external ID relationship
it('creates post with external ID relationship', async () => {
await prisma.user.create({
data: { id: '1', name: 'User1', email: 'user1@abc.com' },
});
const r = await handler({
method: 'post',
path: '/post',
requestBody: {
data: {
type: 'post',
attributes: { title: 'New Post' },
relationships: {
author: { data: { type: 'user', id: 'User1_user1@abc.com' } }
}
}
},
prisma,
});
expect(r.status).toBe(201);
});
// Test PUT with external ID
it('updates user using external ID', async () => {
await prisma.user.create({
data: { id: '1', name: 'User1', email: 'user1@abc.com' },
});
const r = await handler({
method: 'put',
path: '/user/User1_user1@abc.com',
requestBody: {
data: {
type: 'user',
attributes: { name: 'Updated User' }
}
},
prisma,
});
expect(r.status).toBe(200);
expect(r.body.data.attributes.name).toBe('Updated User');
});🤖 Prompt for AI Agents
In packages/server/tests/api/rest.test.ts around lines 3149 to 3191, the
existing test only covers GET requests for external ID mapping. To improve
coverage, add tests for POST, PUT, DELETE methods using external IDs, including
creating resources with relationships via external IDs, updating resources by
external ID, deleting resources by external ID, relationship operations (POST,
PATCH, DELETE), and error cases like malformed or non-existent external IDs.
Implement these tests by simulating requests with appropriate method, path, and
requestBody fields referencing external IDs, and assert expected status codes
and response data.
7a409b2 to
9f1d3a4
Compare
| prisma, | ||
| }); | ||
|
|
||
| expect(r.status).toBe(400); |
There was a problem hiding this comment.
this here imho should be a 404 but it causes a 400 due to "detail": "\nInvalid prisma.post.findUnique() invocation as it expects a _ separated string and therefore lacks the number of parameters for this method call.
that being said, the same happens when using the existing compound id feature. so this issue be addressed in another PR, though I am not entirely sure where. if you give me a hint I can look into this in another PR.
this makes it possible to use a natural key even while also using surrogate primary keys
https://www.prisma.io/dataguide/datamodeling/correctness-constraints#the-primary-key
Using the surrogate key might be sensible since it is more compact for foreign keys. Also the tag name might be editable. But inside the API it makes no sense to use the surrogate key.
ie.
/tags/1makes no sense./tags/blueis the much better developer experience.In order to make use of this a unique key needs to be defined and referenced in the RESTApiHandler Options:
ie. in the model
This will result in a compound id in the API based on the fields in the unique index similar to when using a compound
@id.Note as shown in the above example, if the surrogate key should be hidden in the API response, it is possible to do this via
@deny('all', true)