feat: add GET /api/v1/check/{checkId} endpoint#245
Conversation
WalkthroughA new API endpoint, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Router
participant Store
Client->>API_Router: GET /api/v1/check/:checkId
API_Router->>API_Router: Validate checkId
alt checkId invalid
API_Router-->>Client: 400 Bad Request
else checkId valid
API_Router->>Store: getCheckById(checkId)
alt Check found
API_Router-->>Client: 200 OK (Check data)
else Check not found
API_Router-->>Client: 404 Not Found
end
end
alt Internal error
API_Router-->>Client: 500 Internal Server Error
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🧹 Nitpick comments (2)
src/httpServer/routers/apiV1.js (1)
148-149: Typo in log message
'Failed to retrieve Compliance Checks'was updated, but “Checks” is now plural whereas the corresponding singular route above still says “Check”. Align wording or factor a helper to avoid drift.__tests__/httpServer/apiV1.test.js (1)
418-424: Expectation couples test to middleware wordingThe test asserts the exact string
must be integer. If we ever tweak the OpenAPI-validator version (or remove redundant manual validation per previous comment) this will become brittle.Recommend asserting on status + presence of an error field, not the specific phrasing:
- expect(response.body.errors[0]).toHaveProperty('message', 'must be integer') + expect(response.body.errors[0].message).toMatch(/integer/i)Keeps the intent while decoupling from library internals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
__tests__/httpServer/apiV1.test.js(3 hunks)src/httpServer/routers/apiV1.js(2 hunks)src/httpServer/swagger/api-v1.yml(1 hunks)src/store/index.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Playwright Tests
🔇 Additional comments (1)
src/store/index.js (1)
252-254: ```shell
#!/bin/bash
echo "Checking definitions in src/store/index.js:"
rg "getAllComplianceChecks" -n src/store/index.js
rg "getAllChecks" -n src/store/index.jsecho
echo "Checking usages across repository:"
rg "getAllComplianceChecks(" -n .
rg "getAllChecks(" -n .</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| router.get('/check/:checkId', async (req, res) => { | ||
| try { | ||
| const checkId = parseInt(req.params.checkId, 10) | ||
| if (isNaN(checkId) || !Number.isInteger(checkId) || checkId <= 0) { | ||
| return res.status(400).json({ errors: [{ message: 'Invalid Compliance Check ID. Must be a positive integer.' }] }) | ||
| } | ||
|
|
||
| const check = await getCheckById(checkId) | ||
| if (!check) { | ||
| return res.status(404).json({ errors: [{ message: 'Compliance Check not found' }] }) | ||
| } | ||
| res.json(check) | ||
| } catch (error) { | ||
| logger.error(error) | ||
| res.status(500).json({ errors: [{ message: 'Failed to retrieve Compliance Check' }] }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redundant validation & message drift
The OpenAPI-validator middleware already guarantees checkId is an integer ≥ 0, so the manual parseInt block is never reached for invalid IDs.
Consequences:
- Dead code path → harder to test.
- Error message returned by middleware (“must be integer”) differs from the hard-coded one (“Invalid Compliance Check ID …”), creating confusion for API users and for the test suite.
Either:
A) Delete the manual check and rely on spec validation, or
B) Disable param validation for this route and keep the manual logic, but then adjust the spec & tests.
Option A is simpler:
- const checkId = parseInt(req.params.checkId, 10)
- if (isNaN(checkId) || !Number.isInteger(checkId) || checkId <= 0) {
- return res.status(400).json({ errors: [{ message: 'Invalid Compliance Check ID. Must be a positive integer.' }] })
- }
-
- const check = await getCheckById(checkId)
+ const checkId = req.params.checkId // already integer by validator
+ const check = await getCheckById(checkId)This also removes one source of truth for the 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.
| router.get('/check/:checkId', async (req, res) => { | |
| try { | |
| const checkId = parseInt(req.params.checkId, 10) | |
| if (isNaN(checkId) || !Number.isInteger(checkId) || checkId <= 0) { | |
| return res.status(400).json({ errors: [{ message: 'Invalid Compliance Check ID. Must be a positive integer.' }] }) | |
| } | |
| const check = await getCheckById(checkId) | |
| if (!check) { | |
| return res.status(404).json({ errors: [{ message: 'Compliance Check not found' }] }) | |
| } | |
| res.json(check) | |
| } catch (error) { | |
| logger.error(error) | |
| res.status(500).json({ errors: [{ message: 'Failed to retrieve Compliance Check' }] }) | |
| } | |
| }) | |
| router.get('/check/:checkId', async (req, res) => { | |
| try { | |
| const checkId = req.params.checkId // already integer by validator | |
| const check = await getCheckById(checkId) | |
| if (!check) { | |
| return res.status(404).json({ errors: [{ message: 'Compliance Check not found' }] }) | |
| } | |
| res.json(check) | |
| } catch (error) { | |
| logger.error(error) | |
| res.status(500).json({ errors: [{ message: 'Failed to retrieve Compliance Check' }] }) | |
| } | |
| }) |
🤖 Prompt for AI Agents
In src/httpServer/routers/apiV1.js around lines 124 to 140, remove the manual
parseInt and validation logic for checkId since the OpenAPI-validator middleware
already ensures checkId is a positive integer. This eliminates redundant code
and inconsistent error messages. Rely solely on the middleware validation and
keep the rest of the route handler unchanged.
UlisesGascon
force-pushed
the
ulises/v1-check-details
branch
from
9178c61 to
fb479bd
Compare
1 participant
Related #216
Summary by CodeRabbit
New Features
Tests