fix: Init resource relationship routes #219
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces new POST routes for establishing relationships between deployments and resources, as well as between resources themselves, in the web service API. It includes schema validation using the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@adityachoudhari26 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
apps/webservice/src/app/api/v1/relationship/deployment-to-resource/route.ts (3)
10-14: Consider adding constraints to resourceIdentifier fieldThe schema validation for
resourceIdentifiercould be more restrictive to prevent invalid inputs. Consider adding pattern matching or length constraints.const bodySchema = z.object({ workspaceId: z.string().uuid(), deploymentId: z.string().uuid(), - resourceIdentifier: z.string(), + resourceIdentifier: z.string().min(1).max(255), });
35-50: Enhance deployment error message for consistencySimilar to the resource error, include the deploymentId in the error message for better debugging.
if (!deployment) - return Response.json({ error: "Deployment not found" }, { status: 404 }); + return Response.json( + { error: `Deployment not found: ${body.deploymentId}` }, + { status: 404 } + );
16-19: Consider adding rate limitingSince this is an API endpoint that creates database records, consider adding rate limiting middleware to prevent abuse.
apps/webservice/src/app/api/v1/relationship/resource-to-resource/route.ts (1)
10-15: Consider enhancing the schema validation.While the basic schema is good, consider these improvements:
- Add validation for identifier format/length
- Prevent self-referential relationships (where fromIdentifier equals toIdentifier)
- Consider making relationship types extensible for future requirements
const bodySchema = z.object({ workspaceId: z.string().uuid(), - fromIdentifier: z.string(), - toIdentifier: z.string(), + fromIdentifier: z.string().min(1).max(255), + toIdentifier: z.string().min(1).max(255) + .refine(val => val !== bodySchema.shape.fromIdentifier, + "Self-referential relationships are not allowed"), type: z.enum(["associated_with", "depends_on"]), });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
apps/webservice/src/app/api/v1/relationship/deployment-to-resource/route.ts(1 hunks)apps/webservice/src/app/api/v1/relationship/openapi.ts(0 hunks)apps/webservice/src/app/api/v1/relationship/resource-to-resource/route.ts(1 hunks)apps/webservice/src/app/api/v1/relationship/route.ts(0 hunks)packages/db/src/schema/resource.ts(2 hunks)
💤 Files with no reviewable changes (2)
- apps/webservice/src/app/api/v1/relationship/openapi.ts
- apps/webservice/src/app/api/v1/relationship/route.ts
🔇 Additional comments (6)
apps/webservice/src/app/api/v1/relationship/deployment-to-resource/route.ts (1)
16-19: LGTM! Proper middleware setup
The route is properly secured with authentication and request validation.
apps/webservice/src/app/api/v1/relationship/resource-to-resource/route.ts (3)
1-9: LGTM! Well-organized imports with clear separation of concerns.
The imports are properly structured, bringing in necessary utilities for validation, database operations, and authentication.
17-20: LGTM! Well-structured route handler with proper middleware composition.
The route setup follows best practices with:
- Authentication enforcement
- Request body validation
- Type safety through Zod inference
1-62: Verify API specification and error handling.
Since the AI summary mentions removal of an OpenAPI specification file, ensure that:
- This new route is properly documented
- Error responses are consistent with the API's error handling patterns
✅ Verification successful
API specification and error handling are consistent with codebase patterns
The implementation follows the established patterns in the codebase:
- Error handling is consistent with other routes, using
Response.jsonwith appropriate status codes (404 for not found) - Input validation is handled through Zod schema and the shared
parseBodymiddleware which returns 400 for invalid requests - The route follows the same middleware pattern (
authn,parseBody) as other relationship routes - While this specific endpoint is not documented in OpenAPI, it follows the same structure as other documented endpoints, with similar request/response patterns
The code is well-aligned with the existing API implementation patterns and error handling approaches used throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for OpenAPI specs and error handling patterns
echo "Checking for OpenAPI specs..."
fd -e yaml -e json . | xargs rg -l "openapi:"
echo "Checking error handling patterns..."
rg -A 2 "Response\.json.*error" --type typescript
Length of output: 310
Script:
#!/bin/bash
# Let's try a different approach to check error handling patterns and API documentation
# Check for OpenAPI/Swagger specs
echo "Checking for API specs..."
rg -l "openapi|swagger" --type yaml --type json
# Check error handling patterns without relying on file type
echo -e "\nChecking error handling patterns in all files..."
rg -l "Response\.json.*error"
# Look for similar route handlers to understand the pattern
echo -e "\nChecking similar route handlers..."
rg -A 3 "export const (GET|POST|PUT|DELETE) = request\(\)"
# Check for any API documentation
echo -e "\nChecking for API documentation..."
fd README -t f
Length of output: 5956
Script:
#!/bin/bash
# Let's examine the OpenAPI spec and error handling patterns in more detail
# Check the OpenAPI spec content
echo "Checking OpenAPI spec content..."
cat openapi.v1.json
# Check error handling in the body parser since it's referenced in our route
echo -e "\nChecking body parser error handling..."
cat apps/webservice/src/app/api/v1/body-parser.tsx
# Check error handling in a similar relationship route
echo -e "\nChecking similar relationship route..."
cat apps/webservice/src/app/api/v1/relationship/deployment-to-resource/route.ts
Length of output: 53996
packages/db/src/schema/resource.ts (2)
34-34: LGTM!
The import is correctly placed and necessary for the foreign key reference in the new deploymentResourceRelationship table.
284-286: Verify the usage of resource.identifier as foreign key.
Using resource.identifier as a foreign key is unusual. Let's verify:
- If
identifieris used as a foreign key elsewhere - If
identifieris mutable (can be updated) - The rationale behind using
identifierinstead ofid
✅ Verification successful
Let me analyze the usage of resource.identifier further by checking for any updates to it.
Based on the analysis of the codebase, I can now provide a final response:
Using resource.identifier as foreign key is appropriate in this case
The analysis reveals that:
identifieris a non-null field that is part of a unique composite index withworkspaceId- There are no update operations found that modify the identifier field
- The field is used consistently for resource lookups in API routes and relationships
- The cascade delete behavior is appropriate as related records should be removed when the resource is deleted
The design choice is valid because:
- The identifier serves as a natural, stable key for resources within a workspace
- The composite uniqueness constraint (
identifier+workspaceId) ensures global uniqueness - The field is effectively immutable as no update operations were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of resource.identifier in the codebase
# Check if identifier is used as foreign key elsewhere
echo "Checking foreign key usage of resource.identifier:"
ast-grep --pattern 'references(() => resource.identifier'
# Check if identifier can be updated
echo -e "\nChecking if identifier can be updated:"
rg -A 5 'identifier.*=.*resource\.' --type ts
# Check commit history for context
echo -e "\nChecking commit history for context:"
git log -p --all -S "resource.identifier" --pretty=format:"%h %s"
Length of output: 33326
Script:
#!/bin/bash
# Check if identifier is mutable by looking for update operations
echo "Checking for identifier updates:"
ast-grep --pattern 'update($$$).set({ $$$identifier$$$})'
echo -e "\nChecking for direct assignments to identifier:"
rg "identifier = " --type ts
echo -e "\nChecking schema constraints on identifier:"
rg "identifier.*notNull" --type ts
Length of output: 493
| const resource = await db | ||
| .select() | ||
| .from(SCHEMA.resource) | ||
| .where( | ||
| and( | ||
| eq(SCHEMA.resource.identifier, body.resourceIdentifier), | ||
| eq(SCHEMA.resource.workspaceId, body.workspaceId), | ||
| ), | ||
| ) | ||
| .then(takeFirstOrNull); | ||
| if (!resource) | ||
| return Response.json({ error: "Resource not found" }, { status: 404 }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and add transaction support
Two suggestions for improvement:
- The error message could be more descriptive by including the resourceIdentifier.
- The entire operation should be wrapped in a transaction to ensure data consistency.
+ const result = await db.transaction(async (tx) => {
const resource = await db
.select()
.from(SCHEMA.resource)
.where(
and(
eq(SCHEMA.resource.identifier, body.resourceIdentifier),
eq(SCHEMA.resource.workspaceId, body.workspaceId),
),
)
.then(takeFirstOrNull);
if (!resource)
- return Response.json({ error: "Resource not found" }, { status: 404 });
+ return Response.json(
+ { error: `Resource not found: ${body.resourceIdentifier}` },
+ { status: 404 }
+ );Committable suggestion skipped: line range outside the PR's diff.
| await db | ||
| .insert(SCHEMA.deploymentResourceRelationship) | ||
| .values(body) | ||
| .returning(); | ||
|
|
||
| return Response.json(body); | ||
| }); |
There was a problem hiding this comment.
Add uniqueness check and improve response data
Several improvements needed for the relationship creation:
- Check for existing relationships to prevent duplicates
- Return the created record instead of the request body
- Add error handling for potential constraint violations
await db
+ .select()
+ .from(SCHEMA.deploymentResourceRelationship)
+ .where(
+ and(
+ eq(SCHEMA.deploymentResourceRelationship.deploymentId, body.deploymentId),
+ eq(SCHEMA.deploymentResourceRelationship.resourceIdentifier, body.resourceIdentifier)
+ )
+ )
+ .then(takeFirstOrNull)
+ .then((existing) => {
+ if (existing) {
+ return Response.json(
+ { error: "Relationship already exists" },
+ { status: 409 }
+ );
+ }
+ });
+
+ try {
+ const [created] = await db
.insert(SCHEMA.deploymentResourceRelationship)
.values(body)
- .returning();
+ .returning({
+ id: SCHEMA.deploymentResourceRelationship.id,
+ workspaceId: SCHEMA.deploymentResourceRelationship.workspaceId,
+ deploymentId: SCHEMA.deploymentResourceRelationship.deploymentId,
+ resourceIdentifier: SCHEMA.deploymentResourceRelationship.resourceIdentifier,
+ createdAt: SCHEMA.deploymentResourceRelationship.createdAt
+ });
- return Response.json(body);
+ return Response.json(created);
+ } catch (error) {
+ return Response.json(
+ { error: "Failed to create relationship" },
+ { status: 500 }
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await db | |
| .insert(SCHEMA.deploymentResourceRelationship) | |
| .values(body) | |
| .returning(); | |
| return Response.json(body); | |
| }); | |
| await db | |
| .select() | |
| .from(SCHEMA.deploymentResourceRelationship) | |
| .where( | |
| and( | |
| eq(SCHEMA.deploymentResourceRelationship.deploymentId, body.deploymentId), | |
| eq(SCHEMA.deploymentResourceRelationship.resourceIdentifier, body.resourceIdentifier) | |
| ) | |
| ) | |
| .then(takeFirstOrNull) | |
| .then((existing) => { | |
| if (existing) { | |
| return Response.json( | |
| { error: "Relationship already exists" }, | |
| { status: 409 } | |
| ); | |
| } | |
| }); | |
| try { | |
| const [created] = await db | |
| .insert(SCHEMA.deploymentResourceRelationship) | |
| .values(body) | |
| .returning({ | |
| id: SCHEMA.deploymentResourceRelationship.id, | |
| workspaceId: SCHEMA.deploymentResourceRelationship.workspaceId, | |
| deploymentId: SCHEMA.deploymentResourceRelationship.deploymentId, | |
| resourceIdentifier: SCHEMA.deploymentResourceRelationship.resourceIdentifier, | |
| createdAt: SCHEMA.deploymentResourceRelationship.createdAt | |
| }); | |
| return Response.json(created); | |
| } catch (error) { | |
| return Response.json( | |
| { error: "Failed to create relationship" }, | |
| { status: 500 } | |
| ); | |
| } |
| await db.insert(SCHEMA.resourceRelationship).values({ | ||
| sourceId: fromResource.id, | ||
| targetId: toResource.id, | ||
| type: body.type, | ||
| }); | ||
|
|
||
| return Response.json({ message: "Relationship created" }, { status: 200 }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance relationship creation with better error handling and response.
The relationship creation needs improvements in several areas:
- Wrap database operations in a transaction
- Validate circular dependencies for "depends_on" relationships
- Return created relationship details in response
- await db.insert(SCHEMA.resourceRelationship).values({
- sourceId: fromResource.id,
- targetId: toResource.id,
- type: body.type,
- });
-
- return Response.json({ message: "Relationship created" }, { status: 200 });
+ return await db.transaction(async (tx) => {
+ if (body.type === "depends_on") {
+ const hasCycle = await checkForDependencyCycle(
+ tx,
+ fromResource.id,
+ toResource.id
+ );
+ if (hasCycle) {
+ return Response.json(
+ { error: "Circular dependency detected" },
+ { status: 400 }
+ );
+ }
+ }
+
+ const [relationship] = await tx
+ .insert(SCHEMA.resourceRelationship)
+ .values({
+ sourceId: fromResource.id,
+ targetId: toResource.id,
+ type: body.type,
+ })
+ .returning();
+
+ return Response.json({
+ message: "Relationship created",
+ relationship: {
+ fromIdentifier: body.fromIdentifier,
+ toIdentifier: body.toIdentifier,
+ type: body.type,
+ }
+ }, { status: 201 });
+ });Committable suggestion skipped: line range outside the PR's diff.
| const fromResource = await db | ||
| .select() | ||
| .from(SCHEMA.resource) | ||
| .where( | ||
| and( | ||
| eq(SCHEMA.resource.identifier, body.fromIdentifier), | ||
| eq(SCHEMA.resource.workspaceId, body.workspaceId), | ||
| ), | ||
| ) | ||
| .then(takeFirstOrNull); | ||
| if (!fromResource) | ||
| return Response.json( | ||
| { error: `${body.fromIdentifier} not found` }, | ||
| { status: 404 }, | ||
| ); | ||
|
|
||
| const toResource = await db | ||
| .select() | ||
| .from(SCHEMA.resource) | ||
| .where( | ||
| and( | ||
| eq(SCHEMA.resource.identifier, body.toIdentifier), | ||
| eq(SCHEMA.resource.workspaceId, body.workspaceId), | ||
| ), | ||
| ) | ||
| .then(takeFirstOrNull); | ||
| if (!toResource) | ||
| return Response.json( | ||
| { error: `${body.toIdentifier} not found` }, | ||
| { status: 404 }, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Add transaction support and uniqueness validation.
The resource validation logic should be wrapped in a transaction, and you should check for existing relationships to prevent duplicates.
+ const existing = await db
+ .select()
+ .from(SCHEMA.resourceRelationship)
+ .where(
+ and(
+ eq(SCHEMA.resourceRelationship.sourceId, fromResource.id),
+ eq(SCHEMA.resourceRelationship.targetId, toResource.id),
+ eq(SCHEMA.resourceRelationship.type, body.type),
+ ),
+ )
+ .then(takeFirstOrNull);
+
+ if (existing) {
+ return Response.json(
+ { error: "Relationship already exists" },
+ { status: 409 },
+ );
+ }Committable suggestion skipped: line range outside the PR's diff.
packages/db/src/schema/resource.ts
Outdated
| export const deploymentResourceRelationship = pgTable( | ||
| "deployment_resource_relationship", | ||
| { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| workspaceId: uuid("workspace_id") | ||
| .references(() => workspace.id, { onDelete: "cascade" }) | ||
| .notNull(), | ||
| deploymentId: uuid("deployment_id") | ||
| .references(() => deployment.id, { onDelete: "cascade" }) | ||
| .notNull(), | ||
| resourceIdentifier: text("resource_identifier") | ||
| .references(() => resource.identifier, { onDelete: "cascade" }) | ||
| .notNull(), | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several improvements needed for the deploymentResourceRelationship table.
- Add a unique index to prevent duplicate relationships:
export const deploymentResourceRelationship = pgTable(
"deployment_resource_relationship",
{
id: uuid("id").primaryKey().defaultRandom(),
workspaceId: uuid("workspace_id")
.references(() => workspace.id, { onDelete: "cascade" })
.notNull(),
deploymentId: uuid("deployment_id")
.references(() => deployment.id, { onDelete: "cascade" })
.notNull(),
resourceIdentifier: text("resource_identifier")
.references(() => resource.identifier, { onDelete: "cascade" })
.notNull(),
},
+ (t) => ({
+ uniq: uniqueIndex().on(t.workspaceId, t.deploymentId, t.resourceIdentifier),
+ }),
);- Consider using
resource.idinstead ofresource.identifierfor the foreign key:
- resourceIdentifier: text("resource_identifier")
- .references(() => resource.identifier, { onDelete: "cascade" })
+ resourceId: uuid("resource_id")
+ .references(() => resource.id, { onDelete: "cascade" })- Add relations definition for better type safety and querying:
export const deploymentResourceRelationshipRelations = relations(
deploymentResourceRelationship,
({ one }) => ({
workspace: one(workspace, {
fields: [deploymentResourceRelationship.workspaceId],
references: [workspace.id],
}),
deployment: one(deployment, {
fields: [deploymentResourceRelationship.deploymentId],
references: [deployment.id],
}),
resource: one(resource, {
fields: [deploymentResourceRelationship.resourceId],
references: [resource.id],
}),
}),
);- Add schema validation like other tables:
export const createDeploymentResourceRelationship = createInsertSchema(
deploymentResourceRelationship,
).omit({ id: true });
export const updateDeploymentResourceRelationship =
createDeploymentResourceRelationship.partial();
export type DeploymentResourceRelationship = InferSelectModel<
typeof deploymentResourceRelationship
>;
Summary by CodeRabbit
New Features
Bug Fixes
Chores