feat(server): upsert support for rest api handler #1863
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I refactored this to treat upserts as peers to creates etc. I'm much happier with this version. |
![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
🧹 Outside diff range and nitpick comments (1)
packages/server/src/api/rest/index.ts (1)
312-333: Consider typing the request body for better type safetyCurrently, the request body is cast to
any. Defining a specific type for the request body can enhance type safety and catch potential errors at compile time.Apply this diff to improve type safety:
- const body = requestBody as any; + interface RequestMeta { + operation?: string; + matchFields?: string[]; + } + + interface RequestBody { + meta?: RequestMeta; + } + + const body = requestBody as RequestBody;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/server/src/api/rest/index.ts(3 hunks)packages/server/tests/api/rest.test.ts(2 hunks)
🧰 Additional context used
🪛 eslint
packages/server/tests/api/rest.test.ts
[error] 9-9: 'query' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (6)
packages/server/src/api/rest/index.ts (2)
860-865: Review the exclusion of match fields from the update data
In the upsert operation, match fields are excluded from the update data. There might be cases where match fields need to be updated. Consider allowing match fields to be updated if it aligns with the business logic.
1402-1419: Verify the construction of the 'where' clause for compound IDs
Ensure that the makeUpsertWhere method correctly constructs the where clause for models with compound IDs. Incorrect construction may lead to unexpected behavior during upsert operations.
Run the following script to verify the correctness:
✅ Verification successful
Based on the comprehensive analysis of the codebase, I can now provide a final response:
The compound ID handling in makeUpsertWhere is correctly implemented
The implementation correctly handles both single and compound IDs by:
- Creating a base
whereobject from all match fields - For compound IDs (when
typeInfo.idFields.length > 1), it properly wraps the fields using Prisma's compound ID format withprismaIdDivider('_') - The implementation is validated by test cases in
packages/server/tests/api/rest.test.tswhich include models with compound IDs likePostLikeandUser
The code is consistent with Prisma's compound ID requirements and is properly tested with various scenarios including:
- Models with
@@id([field1, field2]) - Proper handling of the
prismaIdDividerconstant - Integration with Prisma's query format
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of 'makePrismaIdKey' and ensure correct 'where' clause construction.
# Search for 'makePrismaIdKey' usages in the codebase
rg 'makePrismaIdKey' -A 5 -B 5
# Search for models with compound IDs
grep -r '@@id' ./prisma/schema.prisma
# Verify upsert operations on models with compound IDs
# (Assuming test cases are available)
cd tests && npm run test-upsert-compound-id
Length of output: 5694
Script:
#!/bin/bash
# Let's analyze the implementation and usage of makeUpsertWhere more thoroughly
# First, let's find the definition of prismaIdDivider constant
rg "prismaIdDivider" -B 2 -A 2
# Find all model definitions to check for compound IDs
fd "schema.prisma" --type f
# Look for test files related to upsert operations
fd "upsert" --type f
# Look for actual usage of makeUpsertWhere
rg "makeUpsertWhere" -B 5 -A 5
# Look for model info type definition
rg "type ModelInfo" -A 10
Length of output: 5704
Script:
#!/bin/bash
# Let's examine the implementation of compound IDs in the test projects' schema files
# Check the schema files for compound ID definitions
cat packages/plugins/trpc/tests/projects/nuxt-trpc-v10/prisma/schema.prisma
cat packages/plugins/trpc/tests/projects/nuxt-trpc-v11/prisma/schema.prisma
cat packages/plugins/trpc/tests/projects/t3-trpc-v10/prisma/schema.prisma
cat packages/plugins/trpc/tests/projects/t3-trpc-v11/prisma/schema.prisma
# Look for test files that might be using compound IDs
rg "@@id" --type ts -B 5 -A 5
# Look for the FieldInfo type definition to understand ID field structure
rg "type FieldInfo" -A 10
Length of output: 63505
packages/server/tests/api/rest.test.ts (4)
1805-1841: Test case for upserting a new entity is correctly implemented
The test case effectively verifies the upsert operation when creating a new entity. It asserts that the entity is created and the response matches the expected structure.
1843-1874: Test case for upserting an existing entity works as expected
This test correctly checks that the upsert operation updates an existing entity when the match field matches, ensuring that the existing entity is updated with new attributes.
1876-1912: Test case accurately captures upsert failure due to non-unique matchFields
The test verifies that an upsert operation fails with a proper error when the specified matchFields are not unique, ensuring the system correctly handles such cases.
1914-1937: Test case confirms upsert operation with compound IDs functions correctly
This test ensures that the upsert operation supports entities with compound IDs using multiple matchFields, confirming that the feature works as intended.
There was a problem hiding this comment.
Hi @thomassnielsen , overall it looks very good to me. Please check the few comments I left. Thanks!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
packages/server/src/api/rest/index.ts (2)
853-853: Consider using 'upsert' mode in processRequestBody.The method currently uses 'create' mode when processing the request body. Consider adding an 'upsert' mode to properly validate upsert-specific payload structures.
-const { error, attributes, relationships } = this.processRequestBody(type, requestBody, zodSchemas, 'create'); +const { error, attributes, relationships } = this.processRequestBody(type, requestBody, zodSchemas, 'upsert');
916-921: Consider different status codes for create vs update.The method always returns 201 (Created), but for updates, 200 (OK) might be more appropriate. Consider differentiating based on the operation performed.
+const isCreate = !await prisma[type].findFirst({ where: upsertPayload.where, select: { [typeInfo.idFields[0].name]: true } }); const entity = await prisma[type].upsert(upsertPayload); return { - status: 201, + status: isCreate ? 201 : 200, body: await this.serializeItems(type, entity), };packages/server/tests/api/rest.test.ts (4)
1803-1840: Consider verifying database persistence after upsert operation.In the test
'upsert a new entity', after performing the upsert, consider querying the database to ensure that the new user has been correctly created. This adds an extra layer of validation to confirm that the upsert operation behaves as expected.
1842-1873: Ensure only the intended fields are updated during upsert.In the test
'upsert an existing entity', after the upsert operation, consider fetching the user from the database to verify that only the
1875-1911: Add assertions for specific error codes and messages.In the test
'upsert fails if matchFields are not unique', to enhance robustness, consider asserting that the error response contains the expected error code and message. This ensures that the test accurately verifies that the failure is due to non-uniquematchFields.
1913-1936: Expand assertions to verify response data after upsert with compound ID.In the test
'upsert works with compound id', currently only the response status is checked. Consider adding assertions to verify that the response body contains the correct data, and optionally, fetch the entity from the database to confirm that it has been inserted or updated as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/server/src/api/rest/index.ts(4 hunks)packages/server/tests/api/rest.test.ts(1 hunks)
🔇 Additional comments (4)
packages/server/src/api/rest/index.ts (4)
212-217: LGTM! Well-defined schema for upsert metadata.
The Zod schema correctly validates the upsert operation metadata, ensuring type safety and proper validation of match fields.
319-341: LGTM! Clean integration of upsert handling.
The implementation elegantly integrates upsert support while maintaining backward compatibility with existing create operations.
1411-1427: LGTM! Well-implemented where clause construction.
The helper method correctly handles both simple and compound IDs, with proper null value handling for match fields.
888-899: Review the relationship update strategy.
Using set for collection relationships in the update case will replace all existing relationships. Consider if this is the intended behavior or if connect would be more appropriate to preserve existing relationships.
✅ Verification successful
Let's analyze the implementation context and test coverage to better understand the relationship update behavior.
The context shows an interesting pattern where collection relationships use set in update but connect in create, while non-collection relationships use connect for both. Let's check the tests and documentation to understand if this is intentional.
The test file shows extensive testing of update operations, but let's check the documentation and specific relationship update tests to understand the intended behavior.
Based on the comprehensive test coverage and implementation context, I can now provide a final response:
Using set for collection relationships is the correct behavior.
The current implementation using set for collection relationships in the update case is intentional and correct. Here's why:
- For collection relationships,
setoperation replaces all existing relationships with the new set, which is the expected behavior when updating a collection through a REST API endpoint. - The implementation maintains consistency by using
connectfor creation (adding new relationships) andsetfor updates (replacing the entire collection). - The test suite extensively verifies this behavior through various relationship update scenarios, showing this is a well-tested pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any existing tests that verify the relationship update behavior
rg -A 5 "relationship.*update.*set" "tests/"
Length of output: 44
Script:
#!/bin/bash
# Let's search for tests related to relationship updates more broadly
rg -l "\.update.*connect|\.update.*set" "tests/"
# Also search for the actual implementation context
rg -B 5 -A 5 "upsertPayload\.update\[key\]" "packages/server/src/"
Length of output: 2380
Script:
#!/bin/bash
# Let's examine one of the test files to understand the expected behavior
cat "tests/integration/tests/enhancements/with-policy/refactor.test.ts"
# Also look for any documentation about relationship handling
fd -e md -e txt . | xargs rg -l "relationship|relation"
Length of output: 41980
Script:
#!/bin/bash
# Let's check the README files for any documentation about relationship handling
cat "packages/schema/README-global.md" "README.md"
# Also let's look for any relationship-related tests specifically for updates
rg -A 5 "update.*relation" "tests/"
Length of output: 26822
|
Looking into test failure |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/server/src/api/rest/index.ts (2)
212-217: Consider enhancing matchFields validation.The schema correctly validates the structure of upsert metadata, but consider adding validation for the field names to ensure they exist in the model.
private upsertMetaSchema = z.object({ meta: z.object({ operation: z.literal('upsert'), - matchFields: z.array(z.string()).min(1), + matchFields: z.array(z.string()).min(1).refine( + (fields) => fields.every((field) => this.typeMap[type]?.fields[field]), + "All match fields must exist in the model" + ), }), });
840-922: Well-structured implementation with room for improvements.The implementation correctly handles both attributes and relationships for upsert operations, with proper validation of unique constraints.
Consider these improvements:
- Add validation for required match fields:
const matchFields = this.upsertMetaSchema.parse(requestBody).meta.matchFields; + +// Validate that all match fields have values +const missingFields = matchFields.filter(field => attributes[field] === undefined); +if (missingFields.length > 0) { + return this.makeError( + 'invalidPayload', + `Missing values for match fields: ${missingFields.join(', ')}`, + 400 + ); +}
- Extract relationship processing to reduce duplication:
+private processUpsertRelationship( + relationInfo: RelationshipInfo, + data: any, + mode: 'create' | 'update' +) { + if (relationInfo.isCollection) { + return { + [mode === 'create' ? 'connect' : 'set']: enumerate(data.data).map((item: any) => + this.makeIdConnect(relationInfo.idFields, item.id) + ), + }; + } else { + return { + connect: this.makeIdConnect(relationInfo.idFields, data.data.id), + }; + } +}Then use it in the relationship processing loop:
-if (relationInfo.isCollection) { - upsertPayload.create[key] = { - connect: enumerate(data.data).map((item: any) => - this.makeIdConnect(relationInfo.idFields, item.id) - ), - }; - upsertPayload.update[key] = { - set: enumerate(data.data).map((item: any) => - this.makeIdConnect(relationInfo.idFields, item.id) - ), - }; -} else { - upsertPayload.create[key] = { - connect: this.makeIdConnect(relationInfo.idFields, data.data.id), - }; - upsertPayload.update[key] = { - connect: this.makeIdConnect(relationInfo.idFields, data.data.id), - }; -} +upsertPayload.create[key] = this.processUpsertRelationship(relationInfo, data, 'create'); +upsertPayload.update[key] = this.processUpsertRelationship(relationInfo, data, 'update');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/server/src/api/rest/index.ts(4 hunks)
🔇 Additional comments (1)
packages/server/src/api/rest/index.ts (1)
319-341: LGTM! Clean implementation of operation routing.
The code elegantly determines whether to handle the request as an upsert or create operation, maintaining consistency with the existing error handling patterns.
Fixed. This is now ready for re-review. |
|
Hi @thomassnielsen , I'm merging this PR and the other one you made. Will publish a 2.9.4 release shortly. Thanks for your continuous contribution! |
ymc9
changed the title
Adds support for upsert operations in the RESTAPIHandler via metadata.
We may also want to add documentation about this to the OpenAPI generator.
Discussion in Discord