fix: a few type errors #589
Conversation
WalkthroughThe changes introduce new utility functions and enhance existing middleware functionality across multiple files. Key additions include validation functions for resource IDs and dataset specifications, as well as modifications to improve error handling and data processing. The middleware now incorporates better checks for undefined properties, aiming to address TypeErrors identified in linked issues. The overall structure and logic of the middleware have been updated to ensure more robust handling of requests and responses. Changes
Assessment against linked issues
Possibly related PRs
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 (
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Error: Cannot read properties of undefined (reading '0') Trigger: GET /organisations/local-authority:SAL/tree/invalid%20date/entry-date/5000
Error: Cannot read properties of undefined (reading '0') Trigger: GET /organisations/local-authority:SAL/tree/invalid%20date/entry-date/5000 This is further fix, for the above: Added safe default values, so a page can't be displayed (but with missing data, so it's obviously looks broken). To take care of the "brokn look" we disallow going to pages that are missing data (e.g. no data to display returned from datasette)
...to help editor with hints
Error: Cannot read properties of undefined (reading 'resource') Trigger: GET /organisations/local-authority:MEN/brownfield-land We get an empty string id when fetching resource status and that causes things to fail further down. Upadted the code to skip attempted fetch of entity count and setting it to zero.
Error: Cannot read properties of undefined (reading 'fields')
Trigger: GET /organisations/local-authority:BUC/listed-building/overview
The fetched specification collecton doesn't containa a spec for
the dataset ('listed-building' in our example)
rosado
force-pushed
the
rosado/577-type-errors
branch
from
0a6b460 to
827c906
Compare
issues query needs a limit & offset (returns 1000 records max when not specified).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (5)
src/middleware/common.middleware.js (2)
47-52: Add JSDoc type definitions for logical operatorsThe logical operator functions are well-implemented but would benefit from type documentation to improve IDE support and type inference.
Add TypeScript-style JSDoc comments:
+/** + * Combines multiple request predicates with AND logic + * @param {...function(Request): boolean} args - Request predicate functions + * @returns {function(Request): boolean} Combined predicate function + */ export const and = (...args) => { return (req) => args.every(arg => arg(req)) } +/** + * Combines multiple request predicates with OR logic + * @param {...function(Request): boolean} args - Request predicate functions + * @returns {function(Request): boolean} Combined predicate function + */ export const or = (...args) => { return (req) => args.some(arg => arg(req)) }
Line range hint
84-94: Enhance error handling and type safety in query parameter validationThe validation error handling could provide more context to help debug issues. Also, the schema context type should be documented.
Consider these improvements:
/** * Middleware. Validates query params according to schema. * Short circuits with 400 error if validation fails. Potentially updates req with `parsedParams` * - * `this` needs: `{ schema }` + * @typedef {Object} ValidationContext + * @property {import('valibot').BaseSchema} schema - Valibot schema for validation + * + * @this {ValidationContext} * @param {Request} req * @param {Response} res * @param {NextFunction} next */ export function validateQueryParamsFn (req, res, next) { try { req.parsedParams = v.parse(this.schema || v.any(), req.params) next() } catch (error) { - res.status(400).render('errorPages/400', {}) + const validationError = error instanceof v.ValiError ? error.message : 'Invalid parameters' + logger.warn({ + message: `Parameter validation failed: ${validationError}`, + params: req.params, + type: types.App + }) + res.status(400).render('errorPages/400', { error: validationError }) } }test/unit/middleware/issueDetails.middleware.test.js (1)
Line range hint
139-150: Clarify the difference in entityCount between test cases.There's an inconsistency in
entityCount:
- First test uses
{ entity_count: 1 }- This test uses
{ entity_count: 3 }If this difference is intentional, please update the test description to clarify why different counts are being tested. If not, consider standardising the test data.
src/middleware/middleware.builders.js (1)
Line range hint
153-157: LGTM! Consider adding JSDoc type annotations.The change to make
elseFnoptional with a default value ofundefinedis a good fix for the type errors. This aligns well with the function's usage pattern where the else condition is not always needed.Consider adding TypeScript-style JSDoc annotations to improve type safety:
/** * Returns a middleware that will fetch data if condition is satisfied, * invoke `elseFn` otherwise. * * @param {(req) => boolean} condition predicate fn * @param {*} fetchFn fetch middleware - * @param {(req) => void} elseFn + * @param {((req) => void) | undefined} elseFn * @returns {(req, res, next) => Promise<void>} */ export const fetchIf = (condition, fetchFn, elseFn = undefined) => {src/middleware/issueDetails.middleware.js (1)
31-31: Consider makingissuesQueryLimitconfigurableHardcoding
issuesQueryLimitto 1000 may not be flexible for all use cases. Consider making this value configurable or documenting why this specific limit is appropriate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- src/middleware/common.middleware.js (3 hunks)
- src/middleware/datasetOverview.middleware.js (2 hunks)
- src/middleware/datasetTaskList.middleware.js (2 hunks)
- src/middleware/issueDetails.middleware.js (10 hunks)
- src/middleware/middleware.builders.js (1 hunks)
- src/services/performanceDbApi.js (3 hunks)
- test/unit/middleware/datasetOverview.middleware.test.js (3 hunks)
- test/unit/middleware/issueDetails.middleware.test.js (4 hunks)
🧰 Additional context used
📓 Learnings (2)
src/middleware/datasetOverview.middleware.js (1)
Learnt from: GeorgeGoodall-GovUk PR: digital-land/submit#510 File: test/unit/middleware/datasetOverview.middleware.test.js:1-3 Timestamp: 2024-10-21T08:57:17.338Z Learning: Tests for `pullOutDatasetSpecification` have been moved to `common.middleware.test`.test/unit/middleware/datasetOverview.middleware.test.js (1)
Learnt from: GeorgeGoodall-GovUk PR: digital-land/submit#510 File: test/unit/middleware/datasetOverview.middleware.test.js:1-3 Timestamp: 2024-10-21T08:57:17.338Z Learning: Tests for `pullOutDatasetSpecification` have been moved to `common.middleware.test`.
🪛 Biome
src/middleware/datasetTaskList.middleware.js
[error] 127-127: 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)
src/middleware/issueDetails.middleware.js
[error] 326-326: 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)
[error] 327-327: 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)
[error] 328-328: 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 (12)
test/unit/middleware/datasetOverview.middleware.test.js (2)
39-39: LGTM! Property rename aligns with middleware changesThe change from
specificationtodatasetSpecificationcorrectly reflects the middleware's new approach to handling dataset specifications. The test data structure remains comprehensive and effectively validates the template parameter preparation.
Line range hint
19-30: Consider moving these tests to common.middleware.testBased on previous changes in PR #510, the tests for
pullOutDatasetSpecificationshould reside incommon.middleware.test. Whilst the test implementation is correct, its location might need to be reconsidered for consistency.src/middleware/common.middleware.js (1)
Line range hint
42-94: Verify TypeErrors are properly handled across all endpointsThe changes introduce better type checking, but we should verify they address all TypeErrors mentioned in issue #577.
Let's check for potential remaining type-unsafe operations:
✅ Verification successful
The previous results show extensive property access patterns that could lead to TypeErrors. Let's verify the error handling mechanisms and type validation patterns to ensure they're properly implemented.
TypeErrors are properly handled with robust validation
The code implements comprehensive type safety measures:
- Parameter validation using
v.parse()with schema validation before accessing properties- Proper error handling in middleware functions
- Safe property access patterns with fallback policies in
fetchOneoperations- Type checking assertions where necessary
The changes align well with fixing TypeErrors as:
- Request parameters are validated before use via schema validation
- Resource fetching has proper fallback policies (
FetchOneFallbackPolicy.continue)- Property access is guarded by validation checks (e.g.
isResourceDataPresent)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct property access patterns that might cause TypeErrors ast-grep --pattern 'req.$_.$_' # Find nested property access ast-grep --pattern '$_?.()' # Find method calls that might be undefined # Search for error handling patterns rg -A 2 'catch.*TypeError' # Check existing TypeError handlersLength of output: 11679
Script:
#!/bin/bash # Search for potential TypeErrors in error handling rg -A 3 "try.*catch" src/middleware/common.middleware.js # Check for validation and type checking patterns ast-grep --pattern 'v.parse($_, $_)' ast-grep --pattern 'typeof $_' # Look for error handling in the fetchOne implementation rg -l "fetchOne" src/Length of output: 1102
src/middleware/datasetTaskList.middleware.js (2)
1-12: LGTM: Import additions enhance validation capabilitiesThe new imports
isResourceIdValidandandsupport the implementation of more robust validation checks, aligning well with the PR's objective of addressing type errors.
136-136: LGTM: Enhanced validation in middleware chainThe addition of
isResourceIdValidcheck combined withisResourceAccessibleusing theandcombinator provides more robust validation. The fallback tozeroEntityCountensures type safety by providing a default value.Let's verify the error handling implementation:
✅ Verification successful
The verification results show consistent error handling patterns across middleware files. The
zeroEntityCountfallback is used in bothissueDetails.middleware.jsanddatasetTaskList.middleware.js, following the same pattern. TheisResourceIdValidvalidation is properly defined incommon.middleware.jsand used in combination with other checks.Let me verify the middleware chain structure and error propagation:
Middleware chain validation is properly implemented
The verification confirms that the error handling and middleware chain structure are robust:
- The
fetchIfandonlyIfpatterns are consistently used across middleware files- Error handling via
logPageErroris present at the end of all middleware chains- The
zeroEntityCountfallback is properly integrated with other validation checks- The middleware builders handle both synchronous and asynchronous operations safely
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent error handling patterns across middleware files # Test: Look for similar validation patterns in other middleware files rg -A 3 "isResourceIdValid|zeroEntityCount" --type js # Test: Check for potential type-related error handlers ast-grep --pattern 'catch (error) { $$$}'Length of output: 2373
Script:
#!/bin/bash # Check middleware chain structure and error handling rg -A 5 "onlyIf|fetchIf" --type js # Verify error handling middleware rg "logPageError|handleError" --type jsLength of output: 6821
src/middleware/datasetOverview.middleware.js (2)
Line range hint
183-197: LGTM! Middleware chain order is correctThe placement of
pullOutDatasetSpecificationafterfetchSpecificationensures proper data flow, as it depends on the fetched specification data.
Line range hint
132-157: Improve code maintainability and documentationThe endpoint handling code could benefit from:
- Constants for HTTP status codes
- Resolution of uncertainty about documentation-url
- Clarification of the lastUpdated field mapping
Let's verify the documentation-url concern:
Consider this refactoring:
+ const HTTP_STATUS = { + OK_MIN: 200, + OK_MAX: 300 + } + const endpoints = sources.sort((a, b) => { - if (a.status >= 200 && a.status < 300) return -1 - if (b.status >= 200 && b.status < 300) return 1 + if (a.status >= HTTP_STATUS.OK_MIN && a.status < HTTP_STATUS.OK_MAX) return -1 + if (b.status >= HTTP_STATUS.OK_MIN && b.status < HTTP_STATUS.OK_MAX) return 1 return 0 }).map((source, index) => { let error - if (parseInt(source.status) < 200 || parseInt(source.status) >= 300) { + const status = parseInt(source.status) + if (status < HTTP_STATUS.OK_MIN || status >= HTTP_STATUS.OK_MAX) { error = { - code: parseInt(source.status), + code: status, exception: source.exception } } return { name: `Data Url ${index}`, endpoint: source.endpoint_url, documentation_url: source.documentation_url, lastAccessed: source.latest_log_entry_date, - lastUpdated: source.endpoint_entry_date, // not sure if this is the lastupdated + // TODO: Verify if endpoint_entry_date is the correct field for lastUpdated + lastUpdated: source.endpoint_entry_date, error } })test/unit/middleware/issueDetails.middleware.test.js (2)
83-86: Validate error message structure.The test should explicitly verify that:
errorHeadingbeingundefinedis the expected behaviour- The new error message format
mockMessageFor: 0follows a consistent patternThis would help prevent TypeErrors when accessing message properties.
#!/bin/bash # Check for consistent error message patterns across tests rg -A 2 "mockMessageFor:" test/
39-50: Consider adding edge cases for pagination.Whilst the changes properly test the happy path with
pageNumber: 1, consider adding test cases for:
- Page numbers outside valid range
- Non-numeric page numbers
- Missing page numbers
This would help ensure the TypeErrors mentioned in issue #577 are properly handled.
src/middleware/middleware.builders.js (1)
Line range hint
153-157: Verify usage patterns across the codebase.Let's ensure this change doesn't introduce any regressions by checking the current usage patterns.
✅ Verification successful
Based on the results, I can now generate the final response since I have sufficient information about the usage patterns of
fetchIfacross the codebase.The optional
elseFnparameter is being used correctly across the codebaseThe usage patterns show that
fetchIfis consistently used in two ways:
- With two parameters (condition and fetchFn)
- With three parameters (condition, fetchFn, and elseFn)
Examples of proper usage found:
fetchIf(isResourceIdInParams, fetchLatestResource, takeResourceIdFromParams)fetchIf(isResourceDataPresent, fetchIssues, emptyIssuesCollection)fetchIf(isResourceAccessible, fetchLatestResource)The implementation change maintains compatibility with both patterns, and the error handling remains robust as the function handles undefined
elseFngracefully.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage patterns of fetchIf to ensure compatibility with the optional elseFn # Test: Look for all fetchIf invocations echo "Checking fetchIf usage patterns:" rg "fetchIf\(" -A 2 # Test: Look for potential error handling patterns echo -e "\nChecking error handling patterns:" ast-grep --pattern 'fetchIf($condition, $fetchFn, $_)'Length of output: 2053
src/services/performanceDbApi.js (1)
368-378: 🛠️ Refactor suggestionEnhance pagination implementation
While the basic pagination is implemented correctly, consider these improvements:
- Make the page size configurable rather than hard-coding 1000
- Return total count for client-side pagination UI
- Add an index hint for
entry_numberif not already indexedConsider this enhanced implementation:
async getIssues ({ resource, issueType, issueField, - offset = 0 + offset = 0, + limit = 1000 }, database = 'digital-land') { const sql = ` - SELECT i.field, i.line_number, entry_number, message, issue_type, value + WITH issue_data AS ( + SELECT i.field, i.line_number, entry_number, message, issue_type, value + FROM issue i + WHERE resource = '${resource}' + AND issue_type = '${issueType}' + AND field = '${issueField}' + ) + SELECT + *, + (SELECT COUNT(*) FROM issue_data) as total_count FROM issue i WHERE resource = '${resource}' AND issue_type = '${issueType}' AND field = '${issueField}' ORDER BY entry_number ASC - LIMIT 1000 ${offset ? `OFFSET ${offset}` : ''} + LIMIT ${limit} ${offset ? `OFFSET ${offset}` : ''} `Let's check if there's an index on
entry_number:#!/bin/bash # Search for index definitions rg -i 'create.*index.*entry_number' --type sqlsrc/middleware/issueDetails.middleware.js (1)
45-48: Ensure proper handling ofpageNumberandoffsetThe calculation of
offsetbased onpageNumberassumes thatpageNumberis a valid number. Ensure thatpageNumberhas been successfully parsed and validated before this calculation to prevent potential errors.Run the following script to confirm that all usages of
pageNumberhandle parsing and validation correctly:✅ Verification successful
Let me analyze the validation of
pageNumbermore thoroughly by checking the validation schema and middleware implementation.
pageNumbervalidation is properly implementedThe code has robust validation and parsing of the
pageNumberparameter:
It's validated through the
IssueDetailsQueryParamsschema using valibot:
- Defined as optional with default value '1'
- Parsed from string to integer
- Validated to be minimum value of 1
v.pipe(v.string(), v.transform(parseInt), v.minValue(1))Additional range validation is performed in
isPageNumberInRangemiddleware:
- Checks if
pageNumber < 0- Checks if
pageNumberexceedsissueEntitiesCount- Returns 404 if out of range
The calculation of
offsetis safe aspageNumberis guaranteed to be a valid positive integer through these validations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that pageNumber is properly parsed and validated # Search for all instances where pageNumber is used rg 'pageNumber' src/ # Check that pageNumber is always obtained from req.parsedParams and is validatedLength of output: 3698
Script:
#!/bin/bash # Check the validation schema and middleware implementation ast-grep --pattern 'pageNumber: v.optional($$$)' # Check the middleware implementation for pageNumber validation ast-grep --pattern 'function validateParams($$$) { $$$ }'Length of output: 112
Script:
#!/bin/bash # Let's check the specific file for validation implementation cat src/middleware/issueDetails.middleware.jsLength of output: 10080
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/middleware/datasetOverview.middleware.js (1)
41-47: Enhance JSDoc documentation for type safety.
The JSDoc comments should include proper TypeScript-style parameter and return type definitions to prevent TypeErrors.
/**
* Middleware. Updates req with `datasetSpecification`
*
- * @param req
- * @param res
- * @param next
+ * @param {Object} req - Express request object
+ * @param {Object} req.specification - The specification object
+ * @param {Object} req.dataset - The dataset object
+ * @param {string} req.dataset.dataset - Dataset identifier
+ * @param {string} req.dataset.collection - Collection identifier
+ * @param {Object} res - Express response object
+ * @param {Function} next - Express next middleware function
+ * @returns {void}
*/src/middleware/issueDetails.middleware.js (2)
Line range hint 263-274: Specify radix in parseInt to avoid unexpected behaviour
Using parseInt without specifying a radix can lead to unexpected results when parsing strings that start with '0'. To ensure consistent behaviour, explicitly specify the radix.
Apply this diff to specify the radix:
current: pageNumber === parseInt(item)
+ current: pageNumber === parseInt(item, 10)Line range hint 164-164: Correct typo in console assertion message
There's a typo in the console.assert statement: 'precessEntryRow' should be 'processEntryRow'.
Apply this diff to fix the typo:
- console.assert(entryNumber, 'precessEntryRow(): entry_number not in row')
+ console.assert(entryNumber, 'processEntryRow(): entry_number not in row')📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/middleware/datasetOverview.middleware.js (3 hunks)
- src/middleware/issueDetails.middleware.js (10 hunks)
- src/services/performanceDbApi.js (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/performanceDbApi.js
🧰 Additional context used
📓 Learnings (1)
src/middleware/datasetOverview.middleware.js (1)
Learnt from: GeorgeGoodall-GovUk
PR: digital-land/submit#510
File: test/unit/middleware/datasetOverview.middleware.test.js:1-3
Timestamp: 2024-10-21T08:57:17.338Z
Learning: Tests for `pullOutDatasetSpecification` have been moved to `common.middleware.test`.
🪛 Biome
src/middleware/issueDetails.middleware.js
[error] 326-326: 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)
[error] 327-327: 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)
[error] 328-328: 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 (2)
src/middleware/datasetOverview.middleware.js (1)
5-6: LGTM: Necessary imports for error logging.
The addition of logger and logging types supports proper error handling and monitoring.
src/middleware/issueDetails.middleware.js (1)
23-23: LGTM!
The pageNumber validation correctly uses parseInt(s, 10) to ensure the radix is specified, preventing unexpected behaviour with inputs that start with '0'.
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
What type of PR is this? (check all applicable)
Description
This PR fixes the type errors by adding some sane default values, checks and in one update to the issues query and pagination code.
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Visit the preview instance with the below links to verify:
Added/updated tests?
Summary by CodeRabbit
New Features
fetchIffunction for more flexible usage in middleware.Bug Fixes
Documentation
Tests