038 Nov 5 Add Swagger for Subrequirements #204
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…elevant body params
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![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 (3)
Servers/utils/subrequirement.util.ts (1)
24-25: Consider wrapping creation in a transaction.The query is well-structured, but consider wrapping it in a transaction to ensure referential integrity with the requirements table.
export const createNewSubrequirementQuery = async (subrequirement: { requirement_id: number; name: string; description: string; status: string; }): Promise<Subrequirement> => { console.log("createNewSubrequirement", subrequirement); + const client = await pool.connect(); + try { + await client.query('BEGIN'); + // Verify requirement exists + const reqCheck = await client.query( + 'SELECT id FROM requirements WHERE id = $1', + [subrequirement.requirement_id] + ); + if (reqCheck.rows.length === 0) { + throw new Error('Referenced requirement does not exist'); + } + const result = await client.query( "INSERT INTO subrequirements (requirement_id, name, description, status) VALUES ($1, $2, $3, $4) RETURNING *", [subrequirement.requirement_id, subrequirement.name, subrequirement.description, subrequirement.status] ); + await client.query('COMMIT'); return result.rows[0]; + } catch (e) { + await client.query('ROLLBACK'); + throw e; + } finally { + client.release(); + } };Servers/controllers/subrequirement.ctrl.ts (2)
79-89: Consider adding field validations.While the presence check is good, consider adding:
- Type validations (requirement_id should be a number)
- Status value validation (should be from a predefined set)
- Existence check for the referenced requirement_id
Example validation:
+ const validStatuses = ['PENDING', 'IN_PROGRESS', 'COMPLETED']; // Add your actual status values if ( !requirement_id || !name || !description || !status ) { return res .status(400) .json( STATUS_CODE[400]({ message: "requirement_id, name, description and status are required" }) ); } + if (typeof requirement_id !== 'number') { + return res.status(400).json(STATUS_CODE[400]({ message: "requirement_id must be a number" })); + } + if (!validStatuses.includes(status)) { + return res.status(400).json(STATUS_CODE[400]({ message: `status must be one of: ${validStatuses.join(', ')}` })); + }
Line range hint
127-127: Remove debug console.log statement.Debug logging should be removed before merging.
- console.log("updateSubrequirementById");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Servers/controllers/subrequirement.ctrl.ts(3 hunks)Servers/swagger.yaml(2 hunks)Servers/utils/subrequirement.util.ts(1 hunks)
🔇 Additional comments (2)
Servers/utils/subrequirement.util.ts (1)
32-37: 🛠️ Refactor suggestion
Ensure consistent validation with create operation.
The optional fields are well-defined, but ensure that the same validation rules from the create operation (once implemented) are applied here for status and requirement_id.
#!/bin/bash
# Check if update endpoint in controller has validation
ast-grep --pattern 'async function updateSubrequirementById($$$) {
$$$
// Look for validation logic
if ($$$status$$$) {
$$$
}
$$$
}'Servers/controllers/subrequirement.ctrl.ts (1)
Line range hint 131-169: Consider applying the same field validations as suggested for create.
The validation suggestions from the create function should also be applied here for consistency.
Comment on lines
+44
to
+65
| if(subrequirement.requirement_id) { | ||
| fields.push("requirement_id = $1") | ||
| values.push(subrequirement.requirement_id) | ||
| } | ||
| if(subrequirement.name) { | ||
| fields.push("name = $2") | ||
| values.push(subrequirement.name) | ||
| } | ||
| if(subrequirement.description) { | ||
| fields.push("description = $3") | ||
| values.push(subrequirement.description) | ||
| } | ||
| if (subrequirement.description) { | ||
| fields.push("description = $2"); | ||
| values.push(subrequirement.description); | ||
| if(subrequirement.status) { | ||
| fields.push("status = $4") | ||
| values.push(subrequirement.status) | ||
| } | ||
|
|
||
| if (fields.length === 0) { | ||
| throw new Error("No fields to update"); | ||
| } | ||
|
|
||
| query += fields.join(", ") + " WHERE id = $3 RETURNING *"; | ||
| query += fields.join(", ") + " WHERE id = $5 RETURNING *"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve query parameter handling robustness.
The current implementation has several potential issues:
- Hardcoded parameter indices ($1, $2, etc.) are brittle and error-prone
- No validation that values array matches the number of parameters
- Query construction could be more maintainable
Consider this more robust approach:
export const updateSubrequirementByIdQuery = async (
id: number,
subrequirement: {
requirement_id?: number;
name?: string;
description?: string;
status?: string;
}
): Promise<Subrequirement | null> => {
console.log("updateSubrequirementById", id, subrequirement);
const updates: { field: string; value: any }[] = [];
if (subrequirement.requirement_id !== undefined) {
updates.push({ field: 'requirement_id', value: subrequirement.requirement_id });
}
if (subrequirement.name !== undefined) {
updates.push({ field: 'name', value: subrequirement.name });
}
if (subrequirement.description !== undefined) {
updates.push({ field: 'description', value: subrequirement.description });
}
if (subrequirement.status !== undefined) {
updates.push({ field: 'status', value: subrequirement.status });
}
if (updates.length === 0) {
throw new Error("No fields to update");
}
const setClause = updates
.map((update, index) => `${update.field} = $${index + 1}`)
.join(', ');
const values = updates.map(update => update.value);
values.push(id);
const query = `UPDATE subrequirements SET ${setClause} WHERE id = $${values.length} RETURNING *`;
const result = await pool.query(query, values);
return result.rows.length ? result.rows[0] : null;
};
Comment on lines
+17
to
+20
| requirement_id: number; | ||
| name: string; | ||
| description: string; | ||
| status: string; |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Add type safety for subrequirement status values
The verification reveals:
- The database schema uses
varchar(50)for status without constraints - The controller validates presence but not values of status
- No enum/type exists for subrequirement status, unlike other entities (e.g., MitigationStatus)
Recommendations:
- Add a
SubrequirementStatusenum in a shared location - Add value validation in the controller
- Consider adding a CHECK constraint in the database
🔗 Analysis chain
Consider adding input validation and type safety.
While adding requirement_id and status fields is good, consider:
- Adding an enum for valid status values
- Validating that the referenced requirement exists before insertion
Consider adding this type definition:
export enum SubrequirementStatus {
PENDING = 'PENDING',
IN_PROGRESS = 'IN_PROGRESS',
COMPLETED = 'COMPLETED'
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's a status enum or type definition
rg -t typescript "type.*Status|enum.*Status"
# Check if requirement validation exists in the controller
ast-grep --pattern 'async function createSubrequirement($$$) {
$$$
// Look for requirement validation
const requirement = $$$
$$$
}'
Length of output: 307
Script:
#!/bin/bash
# Let's try a different approach to find status-related types/enums
rg -t typescript "status.*=.*['\"](PENDING|IN_PROGRESS|COMPLETED)" -B2 -A2
# Check for requirement validation in controller or service layer
rg -t typescript "createSubrequirement.*\{" -A 10
# Look for any status validation
rg -t typescript "status.*validation|validateStatus|checkStatus" -B2 -A2
# Check database schema or type definitions for status constraints
fd -e sql -e prisma -e ts "schema|type|interface" --exec cat {}
Length of output: 378
Script:
#!/bin/bash
# Search without file type restriction since typescript extension isn't recognized
rg "status.*=.*['\"](PENDING|IN_PROGRESS|COMPLETED)" -B2 -A2
# Look for requirement validation in any relevant files
rg "createSubrequirement.*\{" -A 10
# Check for any status-related types or enums
rg "enum.*Status|type.*Status|interface.*Status" -B1 -A3
# Look for schema definitions
rg "CREATE TABLE.*subrequirement|CREATE TYPE.*status" -i
Length of output: 4864
Script:
#!/bin/bash
# Let's check the subrequirements table definition
rg -A 10 "CREATE TABLE subrequirements\("
# Let's also check if there's any validation in the controller
rg -A 15 "!requirement_id \|\|" Servers/controllers/subrequirement.ctrl.ts
# Check for any existing status constants or validation
rg "const.*status.*=.*\[" -A 5
Length of output: 2296
Comment on lines
+72
to
+111
| const { | ||
| requirement_id, | ||
| name, | ||
| description, | ||
| status | ||
| } = req.body; | ||
|
|
||
| if ( | ||
| !requirement_id || | ||
| !name || | ||
| !description || | ||
| !status | ||
| ) { | ||
| return res | ||
| .status(400) | ||
| .json( | ||
| STATUS_CODE[400]({ message: "name and description are required" }) | ||
| STATUS_CODE[400]({ message: "requirement_id, name, description and status are required" }) | ||
| ); | ||
| } | ||
|
|
||
| if (MOCK_DATA_ON === "true") { | ||
| const newSubrequirement = createMockSubrequirement({ name, description }); | ||
| const newSubrequirement = createMockSubrequirement({ | ||
| requirement_id, | ||
| name, | ||
| description, | ||
| status | ||
| }); | ||
|
|
||
| if (newSubrequirement) { | ||
| return res.status(201).json(STATUS_CODE[201](newSubrequirement)); | ||
| } | ||
|
|
||
| return res.status(503).json(STATUS_CODE[503]({})); | ||
| } else { | ||
| const newSubrequirement = await createNewSubrequirementQuery({ name, description }); | ||
| const newSubrequirement = await createNewSubrequirementQuery({ | ||
| requirement_id, | ||
| name, | ||
| description, | ||
| status | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Add requirement_id validation to prevent orphaned subrequirements
The code review reveals that while the requirement_id is properly added as a required field, there's no validation to ensure it references an existing requirement. This could lead to orphaned subrequirements in the database.
- Add a check in
createSubrequirementto verify the requirement exists before creating the subrequirement - Consider adding a foreign key constraint in the database schema
- Add validation for the
statusfield to ensure it only accepts valid values
Example validation to add before creating the subrequirement:
const requirement = await getRequirementByIdQuery(requirement_id);
if (!requirement) {
return res.status(404).json(
STATUS_CODE[404]({ message: "Requirement not found" })
);
}🔗 Analysis chain
LGTM! The implementation looks clean and consistent.
The changes properly handle the new required fields and maintain consistency between mock and real database implementations.
Let's verify the requirement_id references:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if requirement_id is properly validated against existing requirements
# Test: Search for requirement validation logic
rg -A 5 "requirement_id.*exist|validate.*requirement"
Length of output: 55
Script:
#!/bin/bash
# Let's try a broader search to understand requirement validation
# First, check for any requirement-related queries or validations
rg -l "requirement" --type ts
# Then look for database queries related to requirements
ast-grep --pattern 'async function $name($_) {
$$$
requirement
$$$
}'
Length of output: 1578
Script:
#!/bin/bash
# Let's check the requirement model and validation logic
cat Servers/models/Subrequirement.ts
# Also check the database query implementation
cat Servers/utils/subrequirement.util.ts
# And the requirement controller for any validation patterns
cat Servers/controllers/requirement.ctrl.ts
Length of output: 7755
Comment on lines
+1680
to
+1940
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| requirement_id: | ||
| type: integer | ||
| example: 1 | ||
| name: | ||
| type: string | ||
| example: "Privacy Policy Update" | ||
| description: | ||
| type: string | ||
| example: "Ensure the privacy policy reflects changes regarding user consent." | ||
| status: | ||
| type: string | ||
| example: "Completed" | ||
| required: true | ||
| responses: | ||
| "202": | ||
| description: subrequirement | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| message: | ||
| type: string | ||
| example: accepted | ||
| data: | ||
| $ref: "#components/schemas/SubRequirement" | ||
| "404": | ||
| description: no content to display | ||
| content: | ||
| application/json: | ||
| type: object | ||
| properties: | ||
| message: | ||
| type: string | ||
| example: not found | ||
| data: | ||
| type: object | ||
| "500": | ||
| description: internal server error | ||
| content: | ||
| application/json: | ||
| type: object | ||
| properties: | ||
| message: | ||
| type: string | ||
| example: internal server error | ||
| error: | ||
| type: object | ||
| delete: | ||
| tags: [subrequirements] | ||
| security: | ||
| - JWTAuth: [] | ||
| parameters: | ||
| - in: path | ||
| name: id | ||
| schema: | ||
| type: integer | ||
| required: true | ||
| description: id of the subrequirement to delete | ||
| responses: | ||
| "202": | ||
| description: subrequirement | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| message: | ||
| type: string | ||
| example: accepted | ||
| data: | ||
| type: boolean | ||
| example: true | ||
| "404": | ||
| description: no content to display | ||
| content: | ||
| application/json: | ||
| type: object | ||
| properties: | ||
| message: | ||
| type: string | ||
| example: not found | ||
| data: | ||
| type: object | ||
| "500": | ||
| description: internal server error | ||
| content: | ||
| application/json: | ||
| type: object | ||
| properties: | ||
| message: | ||
| type: string | ||
| example: internal server error | ||
| error: | ||
| type: object |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the subrequirements API endpoints with additional specifications.
While the basic CRUD operations are well-defined, consider the following improvements:
/subrequirements:
get:
tags: [subrequirements]
security:
- JWTAuth: []
+ parameters:
+ - in: query
+ name: status
+ schema:
+ type: string
+ enum: ["Pending", "In Progress", "Completed", "Failed"]
+ required: false
+ description: Filter subrequirements by status
+ - in: query
+ name: requirement_id
+ schema:
+ type: integer
+ required: false
+ description: Filter subrequirements by requirement ID
+ - in: query
+ name: page
+ schema:
+ type: integer
+ minimum: 1
+ default: 1
+ required: false
+ description: Page number for pagination
+ - in: query
+ name: limit
+ schema:
+ type: integer
+ minimum: 1
+ maximum: 100
+ default: 20
+ required: false
+ description: Number of items per page
responses:
"200":
content:
application/json:
schema:
properties:
data:
+ type: object
+ properties:
+ items:
+ type: array
+ items:
+ $ref: "#components/schemas/SubRequirement"
+ total:
+ type: integer
+ description: Total number of items
+ page:
+ type: integer
+ description: Current page number
+ totalPages:
+ type: integer
+ description: Total number of pages
post:
requestBody:
content:
application/json:
schema:
properties:
requirement_id:
type: integer
+ description: ID of the parent requirement
+ minimum: 1
+ required: ["requirement_id", "name", "status"]Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+2106
to
+2123
| SubRequirement: | ||
| type: object | ||
| properties: | ||
| id: | ||
| type: integer | ||
| example: 1 | ||
| requirement_id: | ||
| type: integer | ||
| example: 1 | ||
| name: | ||
| type: string | ||
| example: "Privacy Policy Update" | ||
| description: | ||
| type: string | ||
| example: "Ensure the privacy policy reflects changes regarding user consent." | ||
| status: | ||
| type: string | ||
| example: "Completed" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the SubRequirement schema with additional constraints and timestamps.
The schema needs more specific constraints and additional fields for proper data validation and tracking.
SubRequirement:
type: object
properties:
id:
type: integer
example: 1
+ readOnly: true
requirement_id:
type: integer
example: 1
+ minimum: 1
name:
type: string
example: "Privacy Policy Update"
+ minLength: 1
+ maxLength: 255
description:
type: string
example: "Ensure the privacy policy reflects changes regarding user consent."
+ maxLength: 1000
status:
type: string
example: "Completed"
+ enum: ["Pending", "In Progress", "Completed", "Failed"]
+ created_at:
+ type: string
+ format: date-time
+ example: "2024-01-01T00:00:00Z"
+ readOnly: true
+ updated_at:
+ type: string
+ format: date-time
+ example: "2024-01-01T00:00:00Z"
+ readOnly: true
+ required:
+ - requirement_id
+ - name
+ - status📝 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.
Suggested change
| SubRequirement: | |
| type: object | |
| properties: | |
| id: | |
| type: integer | |
| example: 1 | |
| requirement_id: | |
| type: integer | |
| example: 1 | |
| name: | |
| type: string | |
| example: "Privacy Policy Update" | |
| description: | |
| type: string | |
| example: "Ensure the privacy policy reflects changes regarding user consent." | |
| status: | |
| type: string | |
| example: "Completed" | |
| SubRequirement: | |
| type: object | |
| properties: | |
| id: | |
| type: integer | |
| example: 1 | |
| readOnly: true | |
| requirement_id: | |
| type: integer | |
| example: 1 | |
| minimum: 1 | |
| name: | |
| type: string | |
| example: "Privacy Policy Update" | |
| minLength: 1 | |
| maxLength: 255 | |
| description: | |
| type: string | |
| example: "Ensure the privacy policy reflects changes regarding user consent." | |
| maxLength: 1000 | |
| status: | |
| type: string | |
| example: "Completed" | |
| enum: ["Pending", "In Progress", "Completed", "Failed"] | |
| created_at: | |
| type: string | |
| format: date-time | |
| example: "2024-01-01T00:00:00Z" | |
| readOnly: true | |
| updated_at: | |
| type: string | |
| format: date-time | |
| example: "2024-01-01T00:00:00Z" | |
| readOnly: true | |
| required: | |
| - requirement_id | |
| - name | |
| - status |
|
🔃 |
|
⛔️❌️ |
MuhammadKhalilzadeh
approved these changes
Nov 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Affected Issue: #155
Summary by CodeRabbit
New Features
requirement_id,name,description, andstatus.Bug Fixes
Documentation