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://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
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? |
|
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
|
Hi @lsmith77 , could you merge from the latest code from the branch? I've fixed some environment issues that were causing test failures. |
…T API this makes it possible to use a natural key even while also using surrogate primary keys
|
@ymc9 ok looks better now! 3 topics remain:
I have taken care of 1) and if you provide a hint for 3) I will work on this in a followup PR. |
|
Ok figured out 3) First up, it seems like you consider an invalid resource ID to be a 400 anyway, so maybe there is nothing to fix here: I see two possible fixes.
|
I searched for references and it seems the "invalidId" error is never used ... not sure why but maybe the usage was removed with a previous refactor. If the reason comes from Prisma rejecting an invalid unique filter, I believe the fix is larger, because a real fix should really be systematically validate the entire input object that's passed to Prisma. I suggest we can get this PR merged and worry about the larger fix when it hurts. Btw, do you want to work on the documentation for this new feature too? It'd be great if you have time! |
|
Ok great. Yeah I will add a doc PR next once this is merged, ok? |
Sounds good. Thank you! The related doc is here: https://github.com/zenstackhq/zenstack-docs/blob/main/docs/reference/server-adapters/api-handlers/rest.mdx I'm merging this PR @lsmith77 |
|
I have created #2211 |
|
here is the doc PR zenstackhq/zenstack-docs#480 |
2 participants
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)