feat: add endpoint POST /api/v1/generate-reports #223
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (4)
WalkthroughThis update refactors the HTTP server and report generation workflow to improve modularity and control over server lifecycle. The server startup and shutdown are now asynchronous, with explicit Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExpressApp
participant ApiRouter
participant ReportsModule
participant Database
participant Logger
Client->>ExpressApp: GET /__health
ExpressApp->>ApiRouter: /__health
ApiRouter-->>Client: { status: "ok", timestamp }
Client->>ExpressApp: GET /api/v1/generate-reports
ExpressApp->>ApiRouter: /generate-reports
ApiRouter->>ReportsModule: generateStaticReports(knex, { clearPreviousReports: true })
ReportsModule->>Database: Fetch data, etc.
ReportsModule-->>ApiRouter: Success/Failure
ApiRouter-->>Client: { status: "completed"/"failed", timestamp }
ApiRouter-->>Logger: Log errors if any
sequenceDiagram
participant Main
participant Server
participant Database
participant Logger
Main->>Server: start()
Server->>Database: checkDatabaseConnection()
Database-->>Server: Success/Failure
alt Success
Server->>Main: Server listening
else Failure
Server->>Logger: Log error
Server->>Main: Exit process
end
Main->>Server: stop()
Server->>Database: Destroy connection
Server->>Main: Server stopped
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new endpoint GET /api/v1/generate-reports to generate reports and integrates database connectivity checks at server startup. Key changes include:
- Adding an asynchronous database connection check utility.
- Refactoring the report generation function to clear previous reports if specified.
- Updating server initialization and API router configuration to use the new functions.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/index.js | Added checkDatabaseConnection utility and updated module exports |
| src/reports/index.js | Renamed generateReports to generateStaticReports with option handling |
| src/httpServer/index.js | Integrated database connection check and updated API router usage |
| src/httpServer/apiV1.js | Updated API router to use generateStaticReports and handle errors |
| src/cli/workflows.js | Updated CLI workflows to call generateStaticReports |
| server.js | Modified startup to asynchronously start and stop the server |
| tests/httpServer.test.js | Updated tests to accommodate asynchronous server start/stop and new API |
Comments suppressed due to low confidence (1)
src/reports/index.js:44
- The variable 'destinationFolder' is used without being declared. Please define 'destinationFolder' or pass it as a parameter to avoid runtime errors when clearing previous reports.
await rm(destinationFolder, { recursive: true, force: true })
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
UlisesGascon
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server.js (1)
1-6: Improved server lifecycle management.The server startup has been refactored to use an async IIFE pattern, which allows for better control over the server lifecycle. This approach properly handles the asynchronous nature of the server start process and aligns with the updated server interface.
Using an IIFE is the correct approach here since Node.js doesn't support top-level await in CommonJS modules, as discussed in previous comments.
🧹 Nitpick comments (7)
server.js (2)
3-6: Consider adding error handling.While the async initialization is good, it would be better to add error handling to catch and log any startup failures.
(async () => { const serverInstance = server() - await serverInstance.start() + try { + await serverInstance.start() + console.log('Server started successfully') + } catch (error) { + console.error('Failed to start server:', error) + process.exit(1) + } })()
1-6: Consider adding graceful shutdown.You should consider adding handlers for process termination signals (SIGTERM, SIGINT) to gracefully shut down the server.
(async () => { const serverInstance = server() await serverInstance.start() + + // Handle graceful shutdown + const shutdown = async (signal) => { + console.log(`Received ${signal}, shutting down gracefully...`) + try { + await serverInstance.stop() + console.log('Server stopped successfully') + process.exit(0) + } catch (error) { + console.error('Error during shutdown:', error) + process.exit(1) + } + } + + process.on('SIGTERM', () => shutdown('SIGTERM')) + process.on('SIGINT', () => shutdown('SIGINT')) })()src/httpServer/index.js (2)
55-58: Guardstop()when the server was never startedIf
start()failed beforeserver.listen()executed (e.g. DB down),server.close()will throw'Not running'.
Consider short-circuiting whenserver.listeningisfalse.
7-12: Instantiateknexlazily to improve test isolationBecause the
knexclient is created at module load time, requiring this module twice in the same process (common in Jest’s parallel runners) reuses a single connection pool and may trigger “pool destroyed” errors afterstop().
Moving theknexcreation insidestart()(and passing the instance to the router afterwards) avoids cross-test leakage.Also applies to: 18-18
__tests__/httpServer.test.js (2)
2-3: Import afterjest.mockfor clearer intentJest hoists
jest.mock, so the current code works, but importing the mocked module before the mock reads oddly to many contributors.
Switching the order clarifies test flow and avoids ESLint false-positives.Also applies to: 8-12
56-61: Use.getTime()for explicit date comparison
Dateobjects coerce to numbers, but the intent is clearer (and type-safe) if you compare epoch milliseconds.- expect(finishedAt >= startedAt).toBe(true) + expect(finishedAt.getTime()).toBeGreaterThanOrEqual(startedAt.getTime())Also applies to: 74-79
src/httpServer/apiV1.js (1)
11-28: Expose a status location to make 202 truly helpfulA
202 Acceptedwithout a way to poll progress forces clients to busy-wait or guess completion time.
Consider returning aLocationheader (orstatusUrlfield) that clients can query to obtain the final state.Example minimal change:
- res.status(202).json({ + res.status(202) + .set('Location', '/api/v1/generate-reports/status') // placeholder + .json({ status: 'completed', startedAt: startTs, finishedAt: new Date().toISOString() })You could also push the task ID into Redis / DB so the status endpoint can report ongoing or completed jobs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
__tests__/httpServer.test.js(1 hunks)server.js(1 hunks)src/cli/workflows.js(2 hunks)src/httpServer/apiV1.js(1 hunks)src/httpServer/index.js(2 hunks)src/reports/index.js(3 hunks)src/utils/index.js(2 hunks)
🔇 Additional comments (9)
src/utils/index.js (2)
126-134: Well-implemented database connection check utility.The
checkDatabaseConnectionfunction is a clean implementation of a database connectivity check. It follows good practices by using async/await with proper error handling and returning clear boolean responses.
145-145: Function exported correctly.The new utility function is properly added to the module exports.
src/reports/index.js (5)
3-3: Import updated to include the rm function.The
rmfunction fromnode:fs.promisesis now properly imported to support clearing previous reports.
40-46: Good implementation of report cleanup option.The renaming to
generateStaticReportsand addition of theclearPreviousReportsoption improves the function's flexibility. The implementation correctly removes existing reports when requested using thermfunction with appropriate options.
49-69: Performance improvement using Promise.all.Excellent optimization by running database queries in parallel with
Promise.allinstead of sequential awaits. This can significantly improve performance, especially with multiple independent database operations.
121-121: Added success log for better observability.Adding a success log message at the end of report generation improves observability and makes it easier to track execution flow.
125-125: Updated export to reflect function rename.The module export is correctly updated to use the new function name.
src/cli/workflows.js (2)
4-4: Import updated to use the renamed function.The import statement has been updated to reflect the renaming of
generateReportstogenerateStaticReports.
31-31: Workflow reference updated to use the renamed function.The
workflowproperty of thegenerate-reportscommand has been updated to use the renamed function.
Docstrings generation was requested by @UlisesGascon. * #223 (comment) The following files were modified: * `src/httpServer/apiV1.js`
|
Note Generated docstrings for this pull request at #224 |
Related #219
Summary by CodeRabbit
New Features
Improvements
Refactor
Tests