037 Nov 5 Add Swagger for Section #201
Conversation
WalkthroughThe changes in this pull request primarily 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 (
|
|
@HarshP4585 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
Servers/controllers/section.ctrl.ts (2)
Line range hint
131-169: Remove debug log and extract validation logic.
- The debug console.log statement should be removed or enhanced
- The validation logic is duplicated from createSection
- Error handling could be more consistent
Consider extracting the validation logic into a shared function:
// Add this utility function function validateSectionData( assessment_tracker_id: number, total_questions: number, completed_questions: number ): { isValid: boolean; error?: string } { if (!Number.isInteger(assessment_tracker_id) || assessment_tracker_id <= 0) { return { isValid: false, error: "assessment_tracker_id must be a positive integer" }; } if (!Number.isInteger(total_questions) || total_questions <= 0) { return { isValid: false, error: "total_questions must be a positive integer" }; } if (!Number.isInteger(completed_questions) || completed_questions < 0) { return { isValid: false, error: "completed_questions must be a non-negative integer" }; } if (completed_questions > total_questions) { return { isValid: false, error: "completed_questions cannot exceed total_questions" }; } return { isValid: true }; }Then update the function:
export async function updateSectionById(req: Request, res: Response): Promise<any> { - console.log("updateSectionById"); try { const sectionId = parseInt(req.params.id); const { assessment_tracker_id, name, total_questions, completed_questions } = req.body; + const validation = validateSectionData( + assessment_tracker_id, + total_questions, + completed_questions + ); + + if (!validation.isValid) { + return res.status(400).json(STATUS_CODE[400]({ message: validation.error })); + } // Rest of the function... } catch (error) { + console.error('Error in updateSectionById:', error); return res.status(500).json(STATUS_CODE[500]((error as Error).message)); } }
Line range hint
72-169: Consider implementing request validation middleware.Both functions would benefit from middleware validation using a schema validation library like Joi or Zod. This would:
- Centralize validation logic
- Provide consistent error messages
- Reduce code duplication
- Improve type safety
Would you like me to provide an example implementation using Joi or Zod?
Servers/swagger.yaml (1)
2813-2834: Standardize error response structure.The 503 error response uses the same schema as 500. Consider differentiating them to provide more specific error information.
"503": description: internal server error content: application/json: type: object properties: message: type: string - example: service unavailable + example: database connection error error: type: object + properties: + code: + type: string + example: DB_CONNECTION_ERROR + details: + type: string + example: Unable to connect to the database
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Servers/controllers/section.ctrl.ts(3 hunks)Servers/swagger.yaml(2 hunks)Servers/utils/section.util.ts(1 hunks)
🔇 Additional comments (4)
Servers/controllers/section.ctrl.ts (1)
Line range hint 72-169: Verify Swagger documentation alignment.
The changes to the request body parameters in both functions should be reflected in the Swagger documentation.
Let's verify the Swagger documentation:
✅ Verification successful
Swagger documentation is aligned with the implementation
The Swagger documentation correctly reflects all required request body parameters for both POST and PUT endpoints:
assessment_tracker_id(integer)name(string)total_questions(integer)completed_questions(integer)
The schema definition and endpoint documentation match the implementation in section.ctrl.ts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the Swagger documentation matches the new parameters
# Test: Search for section-related endpoints in swagger.yaml
rg -A 10 "/sections:" Servers/swagger.yaml
# Test: Search for section schema definition
rg -A 10 "Section:" Servers/swagger.yaml
Length of output: 533
Script:
#!/bin/bash
# Let's check more context around the Section schema to see all properties
rg -A 15 "Section:" Servers/swagger.yaml
# Let's also check the POST and PUT endpoints documentation
rg -A 20 "post:|put:" Servers/swagger.yaml
Length of output: 12988
Servers/swagger.yaml (1)
Line range hint 2734-3254: LGTM! The sections API implementation is well-structured.
The implementation follows REST best practices, maintains consistency with other endpoints, and provides comprehensive documentation. The suggested improvements will enhance data validation and schema completeness.
Servers/utils/section.util.ts (2)
17-20: Parameters in createNewSectionQuery are correctly defined.
The required fields assessment_tracker_id, name, total_questions, and completed_questions are appropriately included in the function parameters.
24-25: Ensure the order of placeholders matches the values in the INSERT statement.
The placeholders $1, $2, $3, $4 correspond to the fields in the INSERT statement and the values array. Verify that they are correctly ordered to prevent any data mismatch.
To confirm, you can run the following script:
This script searches for the INSERT query pattern and allows you to verify the placeholders and values.
✅ Verification successful
This will help us see the full context of the INSERT statement and the corresponding values array to verify their alignment.
Placeholders and values are correctly ordered in the INSERT statement
The order of placeholders perfectly matches the values array:
$1→section.assessment_tracker_id$2→section.name$3→section.total_questions$4→section.completed_questions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the placeholders in the INSERT statement match the values provided.
# Test: Check the order of fields and values in the INSERT query.
ast-grep --pattern $'pool.query(
"INSERT INTO sections ($_fields) VALUES ($_placeholders) RETURNING *",
$_values
)'
Length of output: 120
Script:
#!/bin/bash
# Let's try a different approach to find the query pattern
rg -U "INSERT INTO sections.*VALUES.*RETURNING" -A 2
Length of output: 385
| const { | ||
| assessment_tracker_id, | ||
| name, | ||
| total_questions, | ||
| completed_questions | ||
| } = req.body; | ||
|
|
||
| if ( | ||
| !assessment_tracker_id || | ||
| !name || | ||
| !total_questions || | ||
| !completed_questions | ||
| ) { | ||
| return res | ||
| .status(400) | ||
| .json( | ||
| STATUS_CODE[400]({ message: "name and description are required" }) | ||
| STATUS_CODE[400]({ message: "assessment_tracker_id, name, total_questions and completed_questions are required" }) | ||
| ); | ||
| } | ||
|
|
||
| if (MOCK_DATA_ON === "true") { | ||
| const newSection = createMockSection({ name, description }); | ||
| const newSection = createMockSection({ | ||
| assessment_tracker_id, | ||
| name, | ||
| total_questions, | ||
| completed_questions | ||
| }); | ||
|
|
||
| if (newSection) { | ||
| return res.status(201).json(STATUS_CODE[201](newSection)); | ||
| } | ||
|
|
||
| return res.status(503).json(STATUS_CODE[503]({})); | ||
| } else { | ||
| const newSection = await createNewSectionQuery({ name, description }); | ||
| const newSection = await createNewSectionQuery({ | ||
| assessment_tracker_id, | ||
| name, | ||
| total_questions, | ||
| completed_questions | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and improve error handling.
The function needs additional validation and error handling improvements:
- Add type and range validation for numeric fields
- Add structured error responses
- Add error logging for debugging
Consider applying these improvements:
export async function createSection(req: Request, res: Response): Promise<any> {
try {
const {
assessment_tracker_id,
name,
total_questions,
completed_questions
} = req.body;
+ // Validate numeric fields
+ if (!Number.isInteger(assessment_tracker_id) || assessment_tracker_id <= 0) {
+ return res.status(400).json(
+ STATUS_CODE[400]({ message: "assessment_tracker_id must be a positive integer" })
+ );
+ }
+
+ if (!Number.isInteger(total_questions) || total_questions <= 0) {
+ return res.status(400).json(
+ STATUS_CODE[400]({ message: "total_questions must be a positive integer" })
+ );
+ }
+
+ if (!Number.isInteger(completed_questions) || completed_questions < 0) {
+ return res.status(400).json(
+ STATUS_CODE[400]({ message: "completed_questions must be a non-negative integer" })
+ );
+ }
+
+ if (completed_questions > total_questions) {
+ return res.status(400).json(
+ STATUS_CODE[400]({ message: "completed_questions cannot exceed total_questions" })
+ );
+ }
// Rest of the function...
} catch (error) {
+ console.error('Error in createSection:', error);
return res.status(500).json(STATUS_CODE[500]((error as Error).message));
}
}📝 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 { | |
| assessment_tracker_id, | |
| name, | |
| total_questions, | |
| completed_questions | |
| } = req.body; | |
| if ( | |
| !assessment_tracker_id || | |
| !name || | |
| !total_questions || | |
| !completed_questions | |
| ) { | |
| return res | |
| .status(400) | |
| .json( | |
| STATUS_CODE[400]({ message: "name and description are required" }) | |
| STATUS_CODE[400]({ message: "assessment_tracker_id, name, total_questions and completed_questions are required" }) | |
| ); | |
| } | |
| if (MOCK_DATA_ON === "true") { | |
| const newSection = createMockSection({ name, description }); | |
| const newSection = createMockSection({ | |
| assessment_tracker_id, | |
| name, | |
| total_questions, | |
| completed_questions | |
| }); | |
| if (newSection) { | |
| return res.status(201).json(STATUS_CODE[201](newSection)); | |
| } | |
| return res.status(503).json(STATUS_CODE[503]({})); | |
| } else { | |
| const newSection = await createNewSectionQuery({ name, description }); | |
| const newSection = await createNewSectionQuery({ | |
| assessment_tracker_id, | |
| name, | |
| total_questions, | |
| completed_questions | |
| }); | |
| const { | |
| assessment_tracker_id, | |
| name, | |
| total_questions, | |
| completed_questions | |
| } = req.body; | |
| // Validate numeric fields | |
| if (!Number.isInteger(assessment_tracker_id) || assessment_tracker_id <= 0) { | |
| return res.status(400).json( | |
| STATUS_CODE[400]({ message: "assessment_tracker_id must be a positive integer" }) | |
| ); | |
| } | |
| if (!Number.isInteger(total_questions) || total_questions <= 0) { | |
| return res.status(400).json( | |
| STATUS_CODE[400]({ message: "total_questions must be a positive integer" }) | |
| ); | |
| } | |
| if (!Number.isInteger(completed_questions) || completed_questions < 0) { | |
| return res.status(400).json( | |
| STATUS_CODE[400]({ message: "completed_questions must be a non-negative integer" }) | |
| ); | |
| } | |
| if (completed_questions > total_questions) { | |
| return res.status(400).json( | |
| STATUS_CODE[400]({ message: "completed_questions cannot exceed total_questions" }) | |
| ); | |
| } | |
| if ( | |
| !assessment_tracker_id || | |
| !name || | |
| !total_questions || | |
| !completed_questions | |
| ) { | |
| return res | |
| .status(400) | |
| .json( | |
| STATUS_CODE[400]({ message: "assessment_tracker_id, name, total_questions and completed_questions are required" }) | |
| ); | |
| } | |
| if (MOCK_DATA_ON === "true") { | |
| const newSection = createMockSection({ | |
| assessment_tracker_id, | |
| name, | |
| total_questions, | |
| completed_questions | |
| }); | |
| if (newSection) { | |
| return res.status(201).json(STATUS_CODE[201](newSection)); | |
| } | |
| return res.status(503).json(STATUS_CODE[503]({})); | |
| } else { | |
| const newSection = await createNewSectionQuery({ | |
| assessment_tracker_id, | |
| name, | |
| total_questions, | |
| completed_questions | |
| }); |
| assessment_tracker_id: | ||
| type: integer | ||
| example: 1 | ||
| name: | ||
| type: string | ||
| example: "Network Security" | ||
| total_questions: | ||
| type: integer | ||
| example: 20 | ||
| completed_questions: | ||
| type: integer | ||
| example: 20 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add field validations in the request body schema.
Consider adding constraints to ensure data integrity:
assessment_tracker_id:
type: integer
example: 1
+ minimum: 1
name:
type: string
example: "Network Security"
+ minLength: 1
+ maxLength: 255
total_questions:
type: integer
example: 20
+ minimum: 0
completed_questions:
type: integer
example: 20
+ minimum: 0
+ maximum: {total_questions}Committable suggestion skipped: line range outside the PR's diff.
| Section: | ||
| type: object | ||
| properties: | ||
| id: | ||
| type: integer | ||
| example: 1 | ||
| assessment_tracker_id: | ||
| type: integer | ||
| example: 1 | ||
| name: | ||
| type: string | ||
| example: "Network Security" | ||
| total_questions: | ||
| type: integer | ||
| example: 20 | ||
| completed_questions: | ||
| type: integer | ||
| example: 20 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the Section schema definition.
The schema should be consistent with other entities and include additional metadata:
Section:
type: object
+ description: Represents a section within an assessment tracker
properties:
id:
type: integer
example: 1
+ readOnly: true
assessment_tracker_id:
type: integer
example: 1
+ minimum: 1
name:
type: string
example: "Network Security"
+ minLength: 1
+ maxLength: 255
total_questions:
type: integer
example: 20
+ minimum: 0
completed_questions:
type: integer
example: 20
+ minimum: 0
+ created_at:
+ type: string
+ format: date-time
+ example: "2024-10-22T10:15:00Z"
+ readOnly: true
+ updated_at:
+ type: string
+ format: date-time
+ example: "2024-10-23T12:30:00Z"
+ readOnly: true
+ required:
+ - assessment_tracker_id
+ - name
+ - total_questions
+ - completed_questions📝 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.
| Section: | |
| type: object | |
| properties: | |
| id: | |
| type: integer | |
| example: 1 | |
| assessment_tracker_id: | |
| type: integer | |
| example: 1 | |
| name: | |
| type: string | |
| example: "Network Security" | |
| total_questions: | |
| type: integer | |
| example: 20 | |
| completed_questions: | |
| type: integer | |
| example: 20 | |
| Section: | |
| type: object | |
| description: Represents a section within an assessment tracker | |
| properties: | |
| id: | |
| type: integer | |
| example: 1 | |
| readOnly: true | |
| assessment_tracker_id: | |
| type: integer | |
| example: 1 | |
| minimum: 1 | |
| name: | |
| type: string | |
| example: "Network Security" | |
| minLength: 1 | |
| maxLength: 255 | |
| total_questions: | |
| type: integer | |
| example: 20 | |
| minimum: 0 | |
| completed_questions: | |
| type: integer | |
| example: 20 | |
| minimum: 0 | |
| created_at: | |
| type: string | |
| format: date-time | |
| example: "2024-10-22T10:15:00Z" | |
| readOnly: true | |
| updated_at: | |
| type: string | |
| format: date-time | |
| example: "2024-10-23T12:30:00Z" | |
| readOnly: true | |
| required: | |
| - assessment_tracker_id | |
| - name | |
| - total_questions | |
| - completed_questions |
| if(section.assessment_tracker_id) { | ||
| fields.push("assessment_tracker_id = $1") | ||
| values.push(section.assessment_tracker_id) | ||
| } |
There was a problem hiding this comment.
Adjust placeholder numbering to handle optional fields gracefully.
Hardcoded placeholders like $1, $2, etc., can cause errors if some optional fields are not provided. Dynamically generating placeholders based on the number of values ensures accurate SQL queries.
Refactored Code:
let query = "UPDATE sections SET ";
if (section.assessment_tracker_id) {
- fields.push("assessment_tracker_id = $1");
+ fields.push(`assessment_tracker_id = \$${values.length + 1}`);
values.push(section.assessment_tracker_id);
}
if (section.name) {
- fields.push("name = $2");
+ fields.push(`name = \$${values.length + 1}`);
values.push(section.name);
}
if (section.total_questions) {
- fields.push("total_questions = $3");
+ fields.push(`total_questions = \$${values.length + 1}`);
values.push(section.total_questions);
}
if (section.completed_questions) {
- fields.push("completed_questions = $4");
+ fields.push(`completed_questions = \$${values.length + 1}`);
values.push(section.completed_questions);
}This approach updates the placeholder numbers based on the current length of the values array, maintaining correct alignment between placeholders and values.
Also applies to: 48-51, 52-54, 56-58
| section: { | ||
| assessment_tracker_id?: number | ||
| name?: string | ||
| total_questions?: number | ||
| completed_questions?: number | ||
| } |
There was a problem hiding this comment.
Review use of optional parameters in updateSectionByIdQuery.
The function accepts optional fields for updating a section. However, hardcoding placeholder numbers may lead to incorrect parameter binding when some fields are omitted.
Suggested Fix: Dynamically assign placeholder numbers.
Modify the code to assign placeholders based on the current length of the values array, ensuring accurate mapping between placeholders and values.
const fields = [];
const values = [];
let query = "UPDATE sections SET ";
if (section.assessment_tracker_id) {
- fields.push("assessment_tracker_id = $1");
+ fields.push(`assessment_tracker_id = \$${values.length + 1}`);
values.push(section.assessment_tracker_id);
}
if (section.name) {
- fields.push("name = $2");
+ fields.push(`name = \$${values.length + 1}`);
values.push(section.name);
}
if (section.total_questions) {
- fields.push("total_questions = $3");
+ fields.push(`total_questions = \$${values.length + 1}`);
values.push(section.total_questions);
}
if (section.completed_questions) {
- fields.push("completed_questions = $4");
+ fields.push(`completed_questions = \$${values.length + 1}`);
values.push(section.completed_questions);
}This change ensures that each placeholder $n correctly corresponds to the nth value in the values array.
Committable suggestion skipped: line range outside the PR's diff.
| } | ||
|
|
||
| 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.
Correct the placeholder for id in the WHERE clause.
The placeholder $5 assumes that there are always four fields before id, which may not be the case with optional fields. Dynamically setting this placeholder ensures the query remains valid.
Suggested Fix:
- query += fields.join(", ") + " WHERE id = $5 RETURNING *";
+ query += fields.join(", ") + ` WHERE id = \$${values.length + 1} RETURNING *`;
values.push(id);This change aligns the placeholder for id with the next available position in the values array.
Committable suggestion skipped: line range outside the PR's diff.
Affected Issue: #155
Summary by CodeRabbit
New Features
assessment_tracker_id,total_questions, andcompleted_questions.Bug Fixes
Documentation
Sectionin the API documentation to clarify the required properties.