refactor: move common request validation to read function #600
Conversation
Phillip9587
requested review from
UlisesGascon,
bjohansebas,
ctcpip,
jonchurch and
wesleytodd
UlisesGascon
left a comment
UlisesGascon
left a comment
There was a problem hiding this comment.
Thanks for this work @Phillip9587 ❤️
|
Hey @jonchurch @wesleytodd, I’d appreciate your feedback on this PR. Planning to merge by the end of the month if no blockers come up. |
There was a problem hiding this comment.
Pull Request Overview
Refactors common request validation logic by moving it from individual parser functions into the centralized read function. This reduces code duplication and improves maintainability while adding consistent defaultCharset support across all parsers.
- Consolidates request validation logic (body parsing checks, content-type validation, charset handling) into the
readfunction - Adds
defaultCharsetoption support tonormalizeOptions()and the JSON parser - Removes duplicated validation code from all parser types (json, text, raw, urlencoded)
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/read.js | Adds common request validation logic including body existence checks, content-type validation, and charset handling |
| lib/utils.js | Adds defaultCharset option support to normalizeOptions() function |
| lib/types/*.js | Removes duplicated validation code and updates to use centralized read function with normalized options |
| test/utils.js | Adds test coverage for the new defaultCharset option in normalizeOptions() |
| README.md | Documents the new defaultCharset option for the JSON parser |
| HISTORY.md | Documents the refactoring changes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| encoding = getCharset(req) || options.defaultCharset | ||
|
|
||
| // validate charset | ||
| if (!!options?.isValidCharset && !options.isValidCharset(encoding)) { |
There was a problem hiding this comment.
The double negation !!options?.isValidCharset is unnecessary. Since isValidCharset is a function, you can check it directly: if (options?.isValidCharset && !options.isValidCharset(encoding))
| if (!!options?.isValidCharset && !options.isValidCharset(encoding)) { | |
| if (options?.isValidCharset && !options.isValidCharset(encoding)) { |
| var iconv = require('iconv-lite') | ||
| var onFinished = require('on-finished') | ||
| var zlib = require('node:zlib') | ||
| var hasBody = require('type-is').hasBody |
There was a problem hiding this comment.
[nitpick] Consider using destructuring assignment for consistency with other imports: var { hasBody } = require('type-is')
| var hasBody = require('type-is').hasBody | |
| var { hasBody } = require('type-is') |
3 participants
This PR moves the common request validation logic, previously duplicated across all parsers, into the
readfunction. This improves maintainability, reduces package size, and brings us closer to our goal of a generic body parser (see #22).It also adds support for the
defaultCharsetoption innormalizeOptions(). This option was already supported in theqsandtextparsers, and I’ve now added support in thejsonparser as well. While we’ll likely want to validate these options more thoroughly in a future major release, the current approach should suffice for now.There’s not much left before we can move the full middleware creation logic into its own function, but I wanted to keep this PR focused and manageable. Small steps toward our goals.