express error handling#657
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rosado
force-pushed
the
rosado/express-error-handling
branch
from
bf3001d to
d56a04b
Compare
rosado
force-pushed
the
rosado/express-error-handling
branch
from
d56a04b to
ac7cd6b
Compare
WalkthroughThe changes primarily involve refactoring middleware functions across several files to enhance modularity and error handling. Key modifications include the introduction of new utility functions for handling promise rejections and updates to existing middleware to utilise these new functions. The structure of data being handled has also been modified, particularly in how counts are represented. Overall, the changes aim to streamline the middleware architecture and improve error management. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
Also: middleware to handle promise rejections for all cases where the generic fetchOne/fetchMany can't be used
rosado
force-pushed
the
rosado/express-error-handling
branch
from
ac7cd6b to
0c8875c
Compare
![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 (7)
src/serverSetup/errorHandlers.js (1)
Line range hint
1-45: Consider enhancing error template managementThe current implementation hard-codes error template paths. Consider extracting these to a configuration object for better maintainability and centralised error page management.
Example implementation:
const ERROR_TEMPLATES = { default: 'errorPages/500' }; // Then in the error handler: err.template = err.template || (err.status && ERROR_TEMPLATES[err.status]) || ERROR_TEMPLATES.default;src/middleware/overview.middleware.js (1)
Line range hint
158-163: Consider consistent error handling across all async operationsWhile
fetchEntityCountsis now protected, other async operations in the middleware chain might benefit from similar error handling. ParticularlyfetchLatestResourceswhich performs database operations.Consider applying
handleRejectionsto other async operations in the chain:export default [ fetchOrgInfo, - fetchLatestResources, + handleRejections(fetchLatestResources), handleRejections(fetchEntityCounts), fetchLpaOverview, prepareOverviewTemplateParams, getOverview, logPageError ]test/unit/middleware/middleware.builders.test.js (1)
155-161: Consider adding assertion for error absenceWhilst the test correctly verifies that the middleware passes through to
next()in the success case, it could be more explicit.Consider adding an assertion to explicitly verify that no error was passed:
expect(next).toHaveBeenCalledTimes(1) + expect(next).toHaveBeenCalledWith()test/unit/middleware/issueDetails.middleware.test.js (1)
Line range hint
11-276: Add test cases for error handling scenariosGiven that this PR focuses on improving error handling in Express middleware, consider adding test cases to verify:
- Promise rejection scenarios
- Error propagation through the middleware chain
- Edge cases where
issueEntitiesCountmight be undefined or nullWould you like me to help generate these additional test cases?
src/middleware/middleware.builders.js (3)
293-299: Consider enhancing thesafeFnimplementation with TypeScript annotations and documentation.The implementation is correct, but could benefit from additional clarity and type safety.
Consider applying these improvements:
+/** + * Wraps middleware execution in a try-catch block to ensure promise rejections + * are properly handled and passed to Express's error handling chain. + * @param {Request} req - Express request object + * @param {Response} res - Express response object + * @param {NextFunction} next - Express next function + * @returns {Promise<void>} + */ -async function safeFn (req, res, next) { +async function safeFn ( + req: Request, + res: Response, + next: NextFunction +): Promise<void> { try { await this.middleware(req, res, next) } catch (err) { + // Pass the error to Express's error handling chain next(err) } }
301-311: Enhance documentation for thehandleRejectionsfunction.The implementation is solid, but the documentation could be more comprehensive.
Consider enhancing the documentation:
/** * Express 4.x does not handle promise rejections in middleware on its own. The * @{fetchOne} and @{fetchMany} middleware handle that case but for any other async * code you can wrap it in this middleware to ensure rejections don't end up unhandled. * + * @example + * // Wrap an async middleware to handle rejections + * app.get('/api/data', handleRejections(async (req, res, next) => { + * const data = await fetchSomeData(); + * res.json(data); + * })); + * + * @template T + * @param {(req: Request, res: Response, next: NextFunction) => Promise<T>} middleware + * The async middleware function to wrap - * @returns {any} + * @returns {(req: Request, res: Response, next: NextFunction) => Promise<void>} + * A wrapped middleware that safely handles promise rejections */
292-311: Implementation effectively addresses promise rejection handling.The solution elegantly solves the unhandled promise rejection issue in Express 4.x middleware while maintaining compatibility with existing error handling patterns. The implementation is clean, reusable, and aligns perfectly with the PR objectives.
Consider documenting this pattern in your team's best practices guide to ensure consistent error handling across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/middleware/issueDetails.middleware.js(7 hunks)src/middleware/middleware.builders.js(1 hunks)src/middleware/overview.middleware.js(2 hunks)src/serverSetup/errorHandlers.js(2 hunks)src/services/performanceDbApi.js(3 hunks)test/unit/middleware/issueDetails.middleware.test.js(2 hunks)test/unit/middleware/middleware.builders.test.js(1 hunks)
🧰 Additional context used
🪛 Biome
src/middleware/issueDetails.middleware.js
[error] 283-283: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (15)
src/serverSetup/errorHandlers.js (2)
16-18: Excellent addition of the headersSent check!
This change properly prevents attempts to modify the response after headers have been sent, following Express.js best practices. It effectively resolves potential "Cannot set headers after they are sent" errors.
35-37: Good defensive programming with headersSent check
This addition ensures proper middleware chaining and prevents attempts to render the 404 page when the response has already been sent.
src/middleware/overview.middleware.js (2)
3-3: LGTM: Clean import addition
The addition of handleRejections import is well-placed alongside related middleware builders.
159-159: Verify error propagation in fetchEntityCounts
The addition of handleRejections wrapper aligns with the PR's goal of managing unhandled promise rejections. Let's verify the error handling chain.
✅ Verification successful
Let me gather more information about the error handling implementation.
Let me check the middleware builders implementation to complete the verification.
Error handling chain is properly implemented
The error handling implementation is complete and correct:
handleRejectionsmiddleware properly catches and forwards any promise rejections to Express error handlersperformanceDbApi.getEntityCountshas its own error handling that logs warnings and gracefully continues- The error handling chain aligns with the application's error handling patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling implementation across related files
# Check if performanceDbApi methods throw errors that need handling
rg -A 5 "getEntityCounts" --type js
# Verify consistent error handling pattern across middleware
rg "handleRejections\(" --type js
Length of output: 2288
Script:
#!/bin/bash
# Check handleRejections implementation
ast-grep --pattern 'function handleRejections($_) {
$$$
}'
# Check datasette.runQuery implementation for potential error throwing
rg -A 5 "runQuery" --type js
# Look for error handling patterns in middleware.builders
rg -A 10 "handleRejections" test/unit/middleware/middleware.builders.test.js
Length of output: 13783
Script:
#!/bin/bash
# Check handleRejections implementation in middleware builders
cat src/middleware/middleware.builders.js
# Check for any error handling in overview middleware
rg -A 5 "fetchEntityCounts" src/middleware/overview.middleware.js
Length of output: 9877
test/unit/middleware/middleware.builders.test.js (2)
141-143: Well-structured test setup with clear helper function
The test suite is properly organised with a helper function trouble() that consistently produces rejections for testing. This promotes code reuse and maintains consistency across test cases.
144-153: Comprehensive rejection handling test
The test effectively verifies that unhandled promise rejections are caught and properly passed to Express's error handling middleware via next(error). This aligns perfectly with the PR's objective of improving error handling for promise rejections.
src/middleware/issueDetails.middleware.js (6)
1-1: Import 'issuesQueryLimit' correctly
Adding issuesQueryLimit to the import statement ensures it is available for use within the module.
13-13: Middleware builders imported successfully
Including fetchIf, fetchMany, fetchOne, FetchOptions, handleRejections, and renderTemplate supports the refactored middleware functions and enhances modularity.
35-39: Refactor 'fetchIssues' using 'fetchMany'
Refactoring fetchIssues with fetchMany simplifies the middleware and reduces boilerplate code.
104-108: Simplify 'fetchIssueEntitiesCount' with 'fetchOne'
Using fetchOne for fetchIssueEntitiesCount improves code clarity and consistency in data fetching.
295-295: Enhance error handling by wrapping 'fetchEntry'
Wrapping fetchEntry with handleRejections ensures that any unhandled promise rejections are properly managed, enhancing the application's robustness.
176-177: Verify consistent handling of 'issueEntitiesCount'
Now that issueEntitiesCount is an object with a count property, ensure all usages of issueEntitiesCount expect a numeric value to prevent potential runtime errors.
Run the following script to locate all usages of issueEntitiesCount:
✅ Verification successful
Let me analyze the findings from the shell script results.
The search results show all usages of issueEntitiesCount in the codebase, and I can verify that:
-
In
src/middleware/issueDetails.middleware.js, the code correctly destructures the count from the object:- Line 176-177 shows:
const { count: issueEntitiesCount } = issueEntitiesCountObj - All subsequent uses in this file use the numeric value correctly
- Line 176-177 shows:
-
In test files:
test/unit/middleware/issueDetails.middleware.test.jsshows both object format{ count: 1 }and numeric usage1test/unit/issueDetailsPage.test.jsuses numeric values (10 and comparisons with 1)
-
In schema definition:
src/routes/schemas.jsexplicitly defines it asv.integer()
-
The middleware:
- Has a default handler
zeroIssueEntitiesCountthat sets{ count: 0 } - All numeric operations (comparisons, pagination) use the extracted count value
- Has a default handler
All usages are consistent with the new object format
The codebase correctly handles the transition between the object format { count: number } and the numeric value. The middleware extracts the count before any numeric operations, and the schema validation ensures type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of 'issueEntitiesCount' in JavaScript files.
# Expectation: Verify that 'issueEntitiesCount' is correctly used as a number throughout the codebase.
rg 'issueEntitiesCount' --type js --context 3
Length of output: 10461
src/services/performanceDbApi.js (3)
169-170: Define issuesQueryLimit for consistent pagination
The introduction of the issuesQueryLimit constant promotes consistency in pagination limits across the application. Ensure this constant is used wherever pagination is required to maintain uniformity.
313-326: Prevent SQL injection by parameterising queries in getIssuesQuery
Similar to the previous comment, the SQL query in getIssuesQuery directly embeds user-provided values, which can lead to SQL injection vulnerabilities. Use parameterised queries to safely include these variables.
367-372: Prevent SQL injection by parameterising queries in entityCountQuery
Embedding orgEntity directly into the SQL string can result in SQL injection attacks. Utilise parameterised queries to securely handle user input.
|
showing as done - but no end date? |
4 participants
What type of PR is this? (check all applicable)
Description
Due to the combination of the following:
fetchOne/fetchMany) don't handle promise rejectionsWe sometimes end up with unhandled errors (e.g. in
fetchEntry()).We can't switch to 5.x (at least not easily, the wizard library relies on something that broke between 4.x and 5.x), so we have to handle this explicitly. This PR adds a wrapper middleware to handle such errors for the few cases where
fetchOne()/fetchMany()isn't used. 500 error is returned now.Also, moves some of the data fetches to use generic middleware and removes some dead code.
Related Tickets & Documents
No ticket, but related Sentry error
Added/updated tests?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests