fix: Job resource relationship endpoint#229
Conversation
WalkthroughThis pull request introduces new OpenAPI specifications and API routes for managing relationships between jobs and resources within the Ctrlplane API. It includes a POST endpoint for creating these relationships, along with structured error handling. Additionally, it modifies the database schema to enhance relational capabilities, incorporating new relationships between jobs, resources, and release job triggers. The changes ensure that the API is well-defined, with clear request and response formats, and that the database structure supports the new functionality. 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (5)
apps/webservice/src/app/api/v1/relationship/job-to-resource/openapi.ts (2)
4-5: Enhance API information documentationConsider adding more descriptive information to the API documentation to improve its usability.
openapi: "3.0.0", - info: { title: "Ctrlplane API", version: "1.0.0" }, + info: { + title: "Ctrlplane API", + version: "1.0.0", + description: "API for managing relationships between jobs and resources in Ctrlplane", + contact: { + name: "Ctrlplane Support", + url: "https://github.com/your-org/your-repo/issues" + } + },
7-10: Add more detailed operation metadataThe endpoint would benefit from additional OpenAPI metadata to improve API documentation.
"/v1/relationship/job-to-resource": { post: { summary: "Create a relationship between a job and a resource", + description: "Associates a job with a resource by creating a relationship between them. This relationship enables tracking and management of resource allocation to specific jobs.", + tags: ["Relationships"], operationId: "createJobToResourceRelationship",apps/webservice/src/app/api/v1/relationship/resource-to-resource/openapi.ts (1)
4-5: Consider more specific API metadata.The API title "Ctrlplane API" could be more descriptive, and the version number should align with your versioning strategy.
openapi: "3.0.0", - info: { title: "Ctrlplane API", version: "1.0.0" }, + info: { + title: "Ctrlplane Resource Management API", + version: "1.0.0", + description: "API for managing relationships between resources and jobs in Ctrlplane" + },apps/webservice/src/app/api/v1/relationship/resource-to-resource/route.ts (1)
71-73: Enhance error handling robustness and clarityConsider the following improvements to error handling:
- Use a more robust method to detect duplicate key errors (e.g., specific error codes or types)
- Include identifiers of conflicting resources in the error message
- Wrap database operations in a transaction
Example improvement:
- if (error instanceof Error && error.message.includes("duplicate key")) - return Response.json( - { error: "Relationship already exists" }, - { status: 409 }, - ); + if (error instanceof Error && error.message.includes("duplicate key")) + return Response.json({ + error: "Relationship already exists", + details: `Relationship between ${body.fromIdentifier} and ${body.toIdentifier} of type ${body.type} already exists`, + status: 409, + });packages/db/src/schema/release.ts (1)
205-217: Consider adding bidirectional relations for improved querying.While the current relations are correctly defined, consider adding reverse relations in the
jobandresourceschema files. This would enable easier querying from both directions, improving the API's flexibility.Example structure for reverse relations:
// In job schema: export const jobRelations = relations(job, ({ one }) => ({ releaseTrigger: one(releaseJobTrigger, { fields: [job.id], references: [releaseJobTrigger.jobId], }), })); // In resource schema: export const resourceRelations = relations(resource, ({ many }) => ({ releaseTriggers: many(releaseJobTrigger), }));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
apps/webservice/src/app/api/v1/relationship/job-to-resource/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/relationship/job-to-resource/route.ts(1 hunks)apps/webservice/src/app/api/v1/relationship/resource-to-resource/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/relationship/resource-to-resource/route.ts(1 hunks)packages/db/src/schema/job.ts(3 hunks)packages/db/src/schema/release.ts(1 hunks)packages/db/src/schema/resource.ts(2 hunks)
🔇 Additional comments (10)
apps/webservice/src/app/api/v1/relationship/job-to-resource/openapi.ts (1)
7-7: Verify endpoint path consistency
Let's ensure the endpoint path matches the router implementation and there are no conflicts.
✅ Verification successful
Let me gather more information about the router implementation.
Let me try a different approach to find the router implementation.
Let me check the content of the route.ts file to verify the endpoint implementation.
Endpoint path is correctly implemented
The endpoint path /v1/relationship/job-to-resource in the OpenAPI specification matches the implementation in route.ts. The file uses Next.js routing conventions where the file location defines the route path, and the POST handler is properly implemented with authentication, body parsing, and error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for route definitions matching this endpoint
rg -t ts "/v1/relationship/job-to-resource"
# Search for potential duplicate OpenAPI definitions
ast-grep --pattern 'paths: {
$$$
"/v1/relationship/job-to-resource": {
$$$
}
$$$
}'
Length of output: 254
Script:
#!/bin/bash
# Search for router implementation files
fd router.ts -t f
# Search for route handler implementation
rg -t ts "router\.(get|post|put|delete|patch)\s*\(['\"].*job-to-resource" -A 2
Length of output: 102
Script:
#!/bin/bash
# Look for files in the job-to-resource directory
fd . apps/webservice/src/app/api/v1/relationship/job-to-resource/
# Search for route handler implementation with a broader pattern
rg -t ts "router.*job.*resource" -A 2
Length of output: 247
Script:
#!/bin/bash
# Check the content of route.ts file
cat apps/webservice/src/app/api/v1/relationship/job-to-resource/route.ts
# Also search for any imports of this route in parent router files
rg -t ts "import.*job-to-resource.*route" -A 2
Length of output: 2093
apps/webservice/src/app/api/v1/relationship/resource-to-resource/openapi.ts (1)
7-8: Verify consistency with other relationship endpoints.
Let's ensure this endpoint follows the same patterns as other relationship endpoints in the API.
✅ Verification successful
Endpoint follows consistent patterns with other relationship endpoints
The resource-to-resource relationship endpoint follows the same patterns as the existing job-to-resource endpoint:
- Both use POST method for creation
- Both have similar path structure
/v1/relationship/{type} - Both have consistent response codes (200, 400, 404, 409, 500)
- Both have similar OpenAPI structure with required requestBody and proper schema definitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other relationship endpoint definitions
rg -t ts "relationship.*post.*summary" apps/webservice/src/app/api/v1/
Length of output: 72
Script:
#!/bin/bash
# Let's try a broader search for relationship endpoints
rg -t ts "/relationship" apps/webservice/src/app/api/v1/ -A 5
Length of output: 1413
Script:
#!/bin/bash
# Search for OpenAPI endpoint definitions in the relationship directory
fd openapi.ts$ apps/webservice/src/app/api/v1/relationship -x cat {}
Length of output: 2615
apps/webservice/src/app/api/v1/relationship/resource-to-resource/route.ts (2)
72-72: LGTM! Appropriate status code for conflict scenarios
The change from 400 to 409 status code is correct. HTTP 409 Conflict is the proper status code for cases where the request conflicts with the current state (existing relationship).
Line range hint 10-16: Verify schema consistency across layers
Let's ensure the relationship types and schema structure are consistent with the database schema and OpenAPI specification.
✅ Verification successful
Schema is consistent across database and API layers
The verification confirms that the relationship schema is consistent:
- Database schema in
packages/db/src/schema/resource.tsdefines the same relationship types ("associated_with", "depends_on") viaresourceRelationshipType - The schema structure matches with required fields (source, target, type) and proper constraints
- The relationship types are consistently used across the codebase as shown in the API routes and database schema
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in relationship type definitions across the codebase
# Check OpenAPI schema
echo "Checking OpenAPI schema for relationship types..."
rg -A 5 "associated_with|depends_on" --type ts
# Check database schema
echo "Checking database schema for relationship types..."
fd "schema.(ts|js)" -x rg -A 5 "resourceRelationship"
Length of output: 2907
Script:
#!/bin/bash
# Let's check the database schema definition for resource relationships
ast-grep --pattern 'export const resourceRelationship = pgTable(
"resource_relationship",
{
$$$
}
)'
Length of output: 112
Script:
#!/bin/bash
# Let's try a simpler pattern to find the resource relationship schema
rg -A 15 "resourceRelationship = pgTable" --type ts
Length of output: 1150
packages/db/src/schema/job.ts (3)
21-21: LGTM: Import added correctly
The relations import from 'drizzle-orm' is properly added alongside other Drizzle ORM imports.
49-49: LGTM: Import updated correctly
The import statement correctly adds releaseJobTrigger from the release module.
Let's verify the imported entity exists:
✅ Verification successful
Import verified: releaseJobTrigger is correctly exported and defined
The releaseJobTrigger entity is properly exported from release.ts as a pg table definition with appropriate schema and relationships.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that releaseJobTrigger is exported from release.js
# Check if releaseJobTrigger is exported in release.js
ast-grep --pattern 'export const releaseJobTrigger = $$$'
Length of output: 1855
98-100: Please verify the relationship design
The relationship is defined as a many-to-many between job and releaseJobTrigger. Please confirm:
- Is this the intended cardinality?
- Should there be an explicit join table for this relationship?
- Are there any constraints needed on this relationship?
Let's check the database schema and related entities:
packages/db/src/schema/release.ts (1)
205-217: LGTM! The relations are properly defined.
The releaseJobTriggerRelations correctly establishes one-to-one relationships with both job and resource tables, using appropriate field mappings and references. This aligns well with the PR objective of fixing job resource relationship endpoints.
Let's verify the relationship consistency:
✅ Verification successful
Relations are correctly defined and consistent with the schema
The verification confirms that:
- The
jobtable exists withidas UUID primary key - The
resourcetable exists withidas UUID primary key - The
releaseJobTriggertable is properly defined - The relations are bi-directional and consistent:
jobRelationsincludes a many-to-one relationship withreleaseJobTriggerreleaseJobTriggerRelationscorrectly maps to bothjobandresourcetables using proper foreign key references
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the referenced tables and fields exist and are properly defined
# Expected: Find matching field definitions in job and resource tables
# Check for job.id field definition
ast-grep --pattern 'export const job = pgTable("job", { id: uuid("id").$$$'
# Check for resource.id field definition
ast-grep --pattern 'export const resource = pgTable("resource", { id: uuid("id").$$$'
# Check for any other relations to releaseJobTrigger to ensure consistency
rg "relations.*releaseJobTrigger" -A 5
Length of output: 206
Script:
#!/bin/bash
# Let's try a broader search to find the table definitions and relations
# Find job table definition
rg "const job = pgTable" -A 5
# Find resource table definition
rg "const resource = pgTable" -A 5
# Find releaseJobTrigger table definition
rg "const releaseJobTrigger = pgTable" -A 5
# Find all relations definitions to understand the schema better
rg "export const \w+Relations = relations\(" -A 5
Length of output: 11956
packages/db/src/schema/resource.ts (2)
35-35: LGTM!
The import statement follows the existing pattern and correctly imports the required dependency.
80-80: Verify the many-to-many relationship implementation.
The releaseTrigger: many(releaseJobTrigger) relation appears to be setting up a many-to-many relationship, but there's no explicit join table defined. Many-to-many relationships typically require an intermediate table to store the associations.
Let's verify the schema implementation:
Please ensure that:
- A join table is properly defined to handle the many-to-many relationship
- The inverse relation is defined in the
releaseJobTriggerschema
✅ Verification successful
The many-to-many relationship is correctly implemented using foreign keys and Drizzle ORM relations
The implementation is correct and follows Drizzle ORM's relationship patterns:
-
The
releaseJobTriggertable serves as the junction table with proper foreign keys:resourceIdreferencesresource.idreleaseIdreferencesrelease.idjobIdreferencesjob.id
-
The relations are properly defined in both directions:
resourceRelationsdefinesmanyrelationship to handle multiple triggersreleaseJobTriggerRelationsdefines the inverseonerelationship to resource
The implementation doesn't require an explicit separate join table as the releaseJobTrigger table itself serves this purpose while also storing additional metadata like type and causedById.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for join table definition and related schema
# Check for any join table definitions related to resources and release triggers
rg -A 5 "releaseJobTrigger.*resource|resource.*releaseJobTrigger"
# Check the release.js schema file for the other side of the relation
cat src/schema/release.js 2>/dev/null || cat src/schema/release.ts 2>/dev/null
Length of output: 12259
Script:
#!/bin/bash
# Let's check the schema definitions for both resource and releaseJobTrigger
# Check releaseJobTrigger schema definition
ast-grep --pattern 'export const releaseJobTrigger = pgTable'
# Check resource schema definition
ast-grep --pattern 'export const resource = pgTable'
Length of output: 118
Script:
#!/bin/bash
# Let's try with ripgrep to find the schema definitions
# Find releaseJobTrigger schema definition
rg -A 10 "export const releaseJobTrigger = " packages/db/src/schema/
# Find resource schema definition
rg -A 10 "export const resource = " packages/db/src/schema/
Length of output: 1522
Script:
#!/bin/bash
# Let's see the complete releaseJobTrigger schema to check for foreign keys and relations
rg -A 20 "export const releaseJobTrigger = " packages/db/src/schema/release.ts
# Also check for any relations defined in the Drizzle schema
rg "relations" packages/db/src/schema/
Length of output: 4490
Script:
#!/bin/bash
# Let's check the complete releaseJobTriggerRelations and resourceRelations definitions
rg -A 10 "export const releaseJobTriggerRelations = relations" packages/db/src/schema/release.ts
rg -A 10 "export const resourceRelations = relations" packages/db/src/schema/resource.ts
Length of output: 863
| schema: { | ||
| type: "object", | ||
| properties: { | ||
| jobId: { type: "string", format: "uuid" }, | ||
| resourceIdentifier: { type: "string" }, | ||
| }, | ||
| required: ["jobId", "resourceIdentifier"], | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance request body schema with constraints and examples
The schema should include property descriptions, examples, and appropriate constraints.
schema: {
type: "object",
properties: {
- jobId: { type: "string", format: "uuid" },
- resourceIdentifier: { type: "string" },
+ jobId: {
+ type: "string",
+ format: "uuid",
+ description: "Unique identifier of the job",
+ example: "123e4567-e89b-12d3-a456-426614174000"
+ },
+ resourceIdentifier: {
+ type: "string",
+ description: "Unique identifier of the resource to be associated with the job",
+ maxLength: 255,
+ example: "resource-123"
+ },
},
required: ["jobId", "resourceIdentifier"],
+ additionalProperties: false
},📝 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.
| schema: { | |
| type: "object", | |
| properties: { | |
| jobId: { type: "string", format: "uuid" }, | |
| resourceIdentifier: { type: "string" }, | |
| }, | |
| required: ["jobId", "resourceIdentifier"], | |
| }, | |
| schema: { | |
| type: "object", | |
| properties: { | |
| jobId: { | |
| type: "string", | |
| format: "uuid", | |
| description: "Unique identifier of the job", | |
| example: "123e4567-e89b-12d3-a456-426614174000" | |
| }, | |
| resourceIdentifier: { | |
| type: "string", | |
| description: "Unique identifier of the resource to be associated with the job", | |
| maxLength: 255, | |
| example: "resource-123" | |
| }, | |
| }, | |
| required: ["jobId", "resourceIdentifier"], | |
| additionalProperties: false | |
| }, |
| responses: { | ||
| "200": { description: "Relationship created" }, | ||
| "400": { description: "Invalid request body" }, | ||
| "404": { description: "Job or resource not found" }, | ||
| "409": { description: "Relationship already exists" }, | ||
| "500": { description: "Internal server error" }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add detailed response schemas for error cases
The response definitions should include structured error schemas to help API consumers handle errors effectively.
responses: {
- "200": { description: "Relationship created" },
- "400": { description: "Invalid request body" },
- "404": { description: "Job or resource not found" },
- "409": { description: "Relationship already exists" },
- "500": { description: "Internal server error" },
+ "200": {
+ description: "Relationship created successfully",
+ content: {
+ "application/json": {
+ schema: {
+ type: "object",
+ properties: {
+ message: {
+ type: "string",
+ example: "Relationship created successfully"
+ }
+ }
+ }
+ }
+ }
+ },
+ "400": {
+ description: "Invalid request body",
+ content: {
+ "application/json": {
+ schema: {
+ type: "object",
+ properties: {
+ error: {
+ type: "string",
+ example: "Invalid jobId format"
+ }
+ }
+ }
+ }
+ }
+ },
+ "404": {
+ description: "Job or resource not found",
+ content: {
+ "application/json": {
+ schema: {
+ type: "object",
+ properties: {
+ error: {
+ type: "string",
+ example: "Job with specified ID not found"
+ }
+ }
+ }
+ }
+ }
+ },
+ "409": {
+ description: "Relationship already exists",
+ content: {
+ "application/json": {
+ schema: {
+ type: "object",
+ properties: {
+ error: {
+ type: "string",
+ example: "Relationship between job and resource already exists"
+ }
+ }
+ }
+ }
+ }
+ },
+ "500": {
+ description: "Internal server error",
+ content: {
+ "application/json": {
+ schema: {
+ type: "object",
+ properties: {
+ error: {
+ type: "string",
+ example: "Internal server error occurred"
+ }
+ }
+ }
+ }
+ }
+ }📝 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.
| responses: { | |
| "200": { description: "Relationship created" }, | |
| "400": { description: "Invalid request body" }, | |
| "404": { description: "Job or resource not found" }, | |
| "409": { description: "Relationship already exists" }, | |
| "500": { description: "Internal server error" }, | |
| }, | |
| responses: { | |
| "200": { | |
| description: "Relationship created successfully", | |
| content: { | |
| "application/json": { | |
| schema: { | |
| type: "object", | |
| properties: { | |
| message: { | |
| type: "string", | |
| example: "Relationship created successfully" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "400": { | |
| description: "Invalid request body", | |
| content: { | |
| "application/json": { | |
| schema: { | |
| type: "object", | |
| properties: { | |
| error: { | |
| type: "string", | |
| example: "Invalid jobId format" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "404": { | |
| description: "Job or resource not found", | |
| content: { | |
| "application/json": { | |
| schema: { | |
| type: "object", | |
| properties: { | |
| error: { | |
| type: "string", | |
| example: "Job with specified ID not found" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "409": { | |
| description: "Relationship already exists", | |
| content: { | |
| "application/json": { | |
| schema: { | |
| type: "object", | |
| properties: { | |
| error: { | |
| type: "string", | |
| example: "Relationship between job and resource already exists" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "500": { | |
| description: "Internal server error", | |
| content: { | |
| "application/json": { | |
| schema: { | |
| type: "object", | |
| properties: { | |
| error: { | |
| type: "string", | |
| example: "Internal server error occurred" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, |
| "/v1/relationship/resource-to-resource": { | ||
| post: { | ||
| summary: "Create a relationship between two resources", | ||
| operationId: "createResourceToResourceRelationship", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security scheme and API grouping.
The endpoint lacks security definitions and API grouping tags which are important for documentation and access control.
"/v1/relationship/resource-to-resource": {
post: {
summary: "Create a relationship between two resources",
operationId: "createResourceToResourceRelationship",
+ tags: ["Resource Relationships"],
+ security: [{ bearerAuth: [] }],Also, consider adding at the root level:
components: {
securitySchemes: {
bearerAuth: {
type: "http",
scheme: "bearer",
bearerFormat: "JWT"
}
}
}| schema: { | ||
| type: "object", | ||
| properties: { | ||
| workspaceId: { type: "string", format: "uuid" }, | ||
| fromIdentifier: { type: "string" }, | ||
| toIdentifier: { type: "string" }, | ||
| type: { type: "string" }, | ||
| }, | ||
| required: [ | ||
| "workspaceId", | ||
| "fromIdentifier", | ||
| "toIdentifier", | ||
| "type", | ||
| ], | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance request body schema with constraints and descriptions.
The schema could benefit from additional constraints and documentation:
- Add descriptions for each field
- Add pattern/length constraints for identifiers
- Define allowed values for the
typefield
schema: {
type: "object",
properties: {
- workspaceId: { type: "string", format: "uuid" },
- fromIdentifier: { type: "string" },
- toIdentifier: { type: "string" },
- type: { type: "string" },
+ workspaceId: {
+ type: "string",
+ format: "uuid",
+ description: "Unique identifier of the workspace"
+ },
+ fromIdentifier: {
+ type: "string",
+ minLength: 1,
+ maxLength: 255,
+ description: "Identifier of the source resource"
+ },
+ toIdentifier: {
+ type: "string",
+ minLength: 1,
+ maxLength: 255,
+ description: "Identifier of the target resource"
+ },
+ type: {
+ type: "string",
+ enum: ["depends_on", "triggers", "references"],
+ description: "Type of relationship between the resources"
+ },
},Committable suggestion skipped: line range outside the PR's diff.
| responses: { | ||
| "200": { description: "Relationship created" }, | ||
| "400": { description: "Invalid request body" }, | ||
| "404": { description: "Resource not found" }, | ||
| "409": { description: "Relationship already exists" }, | ||
| "500": { description: "Internal server error" }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add detailed response schemas.
The response definitions should include structured response bodies, especially for error cases, to provide more detailed information to API consumers.
responses: {
- "200": { description: "Relationship created" },
- "400": { description: "Invalid request body" },
- "404": { description: "Resource not found" },
- "409": { description: "Relationship already exists" },
- "500": { description: "Internal server error" },
+ "200": {
+ description: "Relationship created",
+ content: {
+ "application/json": {
+ schema: {
+ type: "object",
+ properties: {
+ id: { type: "string", format: "uuid" },
+ created_at: { type: "string", format: "date-time" }
+ }
+ }
+ }
+ }
+ },
+ "400": {
+ description: "Invalid request body",
+ content: {
+ "application/json": {
+ schema: {
+ type: "object",
+ properties: {
+ error: { type: "string" },
+ details: { type: "array", items: { type: "string" } }
+ }
+ }
+ }
+ }
+ },| const matchesIdentifer = eq( | ||
| SCHEMA.resource.identifier, | ||
| body.resourceIdentifier, | ||
| ); |
There was a problem hiding this comment.
Fix typo in variable name 'matchesIdentifer'
There's a typo in the variable name matchesIdentifer; it should be matchesIdentifier for correct spelling and improved readability.
Apply this diff to correct the typo:
- const matchesIdentifer = eq(
+ const matchesIdentifier = eq(
SCHEMA.resource.identifier,
body.resourceIdentifier,
);📝 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.
| const matchesIdentifer = eq( | |
| SCHEMA.resource.identifier, | |
| body.resourceIdentifier, | |
| ); | |
| const matchesIdentifier = eq( | |
| SCHEMA.resource.identifier, | |
| body.resourceIdentifier, | |
| ); |
| .catch((e: Error) => | ||
| e.message.includes("duplicate key value") | ||
| ? NextResponse.json( | ||
| { error: "Relationship already exists" }, | ||
| { status: 409 }, | ||
| ) | ||
| : NextResponse.json({ error: e.message }, { status: 500 }), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle specific database errors instead of parsing error messages
Parsing error messages to identify duplicate key violations is not reliable. It's better to handle specific error types or codes provided by the database driver to accurately detect such cases.
For example, if your database driver provides specific error codes or classes for unique constraint violations, you can modify the error handling as follows:
.catch((e: Error) =>
- e.message.includes("duplicate key value")
+ e.code === '23505' // PostgreSQL unique violation error code
? NextResponse.json(
{ error: "Relationship already exists" },
{ status: 409 },
)
: NextResponse.json(
{ error: "Internal server error" },
{ status: 500 },
),
);Ensure you import any necessary error classes or check the appropriate properties provided by your database library.
Committable suggestion skipped: line range outside the PR's diff.
Avoid exposing internal error messages to clients
Returning internal error messages in the response may reveal sensitive information. It's safer to return a generic error message to the client and log the actual error internally.
Apply this diff to provide a generic error message:
.catch((e: Error) =>
e.message.includes("duplicate key value")
? NextResponse.json(
{ error: "Relationship already exists" },
{ status: 409 },
)
: NextResponse.json(
- { error: e.message },
+ { error: "Internal server error" },
{ status: 500 },
),
);Consider logging the error e using your preferred logging mechanism for debugging purposes.
📝 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.
| .catch((e: Error) => | |
| e.message.includes("duplicate key value") | |
| ? NextResponse.json( | |
| { error: "Relationship already exists" }, | |
| { status: 409 }, | |
| ) | |
| : NextResponse.json({ error: e.message }, { status: 500 }), | |
| ); | |
| .catch((e: Error) => | |
| e.message.includes("duplicate key value") | |
| ? NextResponse.json( | |
| { error: "Relationship already exists" }, | |
| { status: 409 }, | |
| ) | |
| : NextResponse.json( | |
| { error: "Internal server error" }, | |
| { status: 500 }, | |
| ), | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/webservice/src/app/api/v1/relationship/job-to-resource/openapi.ts (2)
7-10: Add tags for better API documentation organization.Tags help group related endpoints in the API documentation.
Add tags to the operation:
"/v1/relationship/job-to-resource": { post: { summary: "Create a relationship between a job and a resource", operationId: "createJobToResourceRelationship", + tags: ["Job Relationships"], requestBody: {
37-119: Consider reusing error response schemas.The error response schemas (400, 404, 409, 500) share the same structure. Consider defining a reusable error schema component to reduce duplication.
Extract common error schema to components:
components: { securitySchemes: { bearerAuth: { type: "http", scheme: "bearer", bearerFormat: "JWT" } }, + schemas: { + Error: { + type: "object", + properties: { + error: { + type: "string" + } + } + } + } }, paths: { // ... existing paths responses: { "400": { description: "Invalid request body", content: { "application/json": { schema: { - type: "object", - properties: { - error: { - type: "string", - example: "Invalid jobId format" - } - } + allOf: [ + { $ref: "#/components/schemas/Error" }, + { + example: { + error: "Invalid jobId format" + } + } + ] } } } }, // Apply similar changes to other error responses
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/webservice/src/app/api/v1/relationship/job-to-resource/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/relationship/job-to-resource/route.ts(1 hunks)apps/webservice/src/app/api/v1/relationship/resource-to-resource/openapi.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webservice/src/app/api/v1/relationship/job-to-resource/route.ts
- apps/webservice/src/app/api/v1/relationship/resource-to-resource/openapi.ts
🔇 Additional comments (1)
apps/webservice/src/app/api/v1/relationship/job-to-resource/openapi.ts (1)
11-36: Request body schema looks good!
The schema is well-defined with appropriate constraints, descriptions, and examples. Previous review suggestions have been properly implemented.
| export const openapi: Swagger.SwaggerV3 = { | ||
| openapi: "3.0.0", | ||
| info: { title: "Ctrlplane API", version: "1.0.0" }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security scheme definition to protect the endpoint.
Consider adding authentication/authorization requirements to the OpenAPI specification. This endpoint creates relationships between resources and should be properly secured.
Add security schemes and requirements:
export const openapi: Swagger.SwaggerV3 = {
openapi: "3.0.0",
info: { title: "Ctrlplane API", version: "1.0.0" },
+ components: {
+ securitySchemes: {
+ bearerAuth: {
+ type: "http",
+ scheme: "bearer",
+ bearerFormat: "JWT"
+ }
+ }
+ },
+ security: [
+ { bearerAuth: [] }
+ ],
paths: {📝 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.
| export const openapi: Swagger.SwaggerV3 = { | |
| openapi: "3.0.0", | |
| info: { title: "Ctrlplane API", version: "1.0.0" }, | |
| export const openapi: Swagger.SwaggerV3 = { | |
| openapi: "3.0.0", | |
| info: { title: "Ctrlplane API", version: "1.0.0" }, | |
| components: { | |
| securitySchemes: { | |
| bearerAuth: { | |
| type: "http", | |
| scheme: "bearer", | |
| bearerFormat: "JWT" | |
| } | |
| } | |
| }, | |
| security: [ | |
| { bearerAuth: [] } | |
| ], | |
| paths: { |
Summary by CodeRabbit
New Features
Bug Fixes
409 Conflictstatus.Chores