feat: support custom pathToRegexpModule #66
Conversation
WalkthroughThe pull request introduces enhancements to the multipart file upload middleware in an Egg.js application. The changes include updating the middleware to accept an additional application context parameter, modifying dependency versions, and expanding test coverage for file upload scenarios. The modifications aim to improve the flexibility of file upload handling, particularly with path matching and file mode configurations. Changes
Sequence DiagramsequenceDiagram
participant Client
participant MultipartMiddleware
participant Controller
participant FileSystem
Client->>MultipartMiddleware: Send multipart file upload
MultipartMiddleware->>MultipartMiddleware: Process file upload with path matching
MultipartMiddleware->>Controller: Forward processed files
Controller->>FileSystem: Save uploaded files
Controller-->>Client: Return upload response
Poem
✨ Finishing Touches
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 (
|
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #66 +/- ##
=======================================
Coverage 98.63% 98.63%
=======================================
Files 6 6
Lines 512 513 +1
Branches 101 101
=======================================
+ Hits 505 506 +1
Misses 7 7 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/config/config.default.js (1)
1-4: LGTM! Consider adding JSDoc comments.The multipart configuration is well-structured for testing the custom pathToRegexpModule feature. The fileModeMatch pattern will effectively test path matching functionality.
Consider adding JSDoc comments to document the expected behavior of the glob pattern:
/** * @type {import('egg-multipart').EggMultipartConfig} * @property {string} fileModeMatch - Matches paths like /upload_file, /upload_file/foo */ exports.multipart = {test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/config/config.unittest.js (1)
1-1: Remove redundant 'use strict' directive.Since this is an ES module, the 'use strict' directive is unnecessary as modules are automatically in strict mode.
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/app/controller/upload.js (1)
1-1: Remove redundant 'use strict' directive.Since this is an ES module, the 'use strict' directive is unnecessary as modules are automatically in strict mode.
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/app/controller/save.js (1)
1-1: Remove redundant 'use strict' directive.Since this is an ES module, the 'use strict' directive is unnecessary as modules are automatically in strict mode.
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/app/router.js (1)
1-8: LGTM! Test fixture mirrors base implementation with pathToRegexpModule.The routes are correctly mirrored to provide comparative testing between default and custom pathToRegexpModule implementations.
Optional: Remove redundant 'use strict' directive as modules are automatically in strict mode.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
test/stream-mode-with-filematch-glob.test.js (1)
73-112: Consider reducing code duplication in test cases.This test case duplicates most of the code from the previous test. Consider extracting the common test logic into a helper function.
+ function testFileUpload(endpoint) { + it(`should upload match file mode work on ${endpoint}`, async () => { + const form = formstream(); + form.field('foo', 'fengmk2').field('love', 'egg'); + form.file('file1', __filename, 'foooooooo.js'); + form.file('file2', __filename); + form.buffer('file3', Buffer.from(''), '', 'application/octet-stream'); + form.file('bigfile', path.join(__dirname, 'fixtures', 'bigfile.js')); + form.field('work', 'with Node.js'); + + const headers = form.headers(); + const res = await urllib.request(host + endpoint, { + method: 'POST', + headers, + stream: form, + }); + + assert(res.status === 200); + const data = JSON.parse(res.data); + // ... rest of the assertions + }); + } + + testFileUpload('/upload_file'); + testFileUpload('/upload_file/foo');test/enable-pathToRegexpModule.test.js (1)
31-111: Consider reducing code duplication in test cases.Similar to the previous file, these test cases contain significant code duplication. Consider extracting the common test logic into a helper function.
test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/app/views/home.html (1)
1-8: Enhance form accessibility and security.Consider the following improvements:
- Add labels with
forattributes for better accessibility- Add
acceptattributes to file inputs to restrict file types- Add
aria-labelattributes for screen readers-<form method="POST" action="/upload_file?_csrf={{ ctx.csrf | safe }}" enctype="multipart/form-data"> - title: <input name="title" /> - file1: <input name="file1" type="file" /> - file2: <input name="file2" type="file" /> - file3: <input name="file3" type="file" /> - other: <input name="other" /> - <button type="submit">上传</button> +<form method="POST" action="/upload_file?_csrf={{ ctx.csrf | safe }}" enctype="multipart/form-data" aria-label="File Upload Form"> + <label for="title">title:</label> + <input id="title" name="title" aria-label="Title" /> + <label for="file1">file1:</label> + <input id="file1" name="file1" type="file" accept=".js,.json" aria-label="File 1" /> + <label for="file2">file2:</label> + <input id="file2" name="file2" type="file" accept=".js,.json" aria-label="File 2" /> + <label for="file3">file3:</label> + <input id="file3" name="file3" type="file" accept=".js,.json" aria-label="File 3" /> + <label for="other">other:</label> + <input id="other" name="other" aria-label="Other" /> + <button type="submit" aria-label="Upload">上传</button>package.json (1)
65-66: Review dependencies placementConsider the following suggestions:
stream-wormholeappears to be a runtime dependency for handling file streams. Should it be moved todependencies?- If
path-to-regexp-v8is required for the custom pathToRegexpModule feature at runtime, it should also be independencies."dependencies": { "co-busboy": "^2.0.0", "dayjs": "^1.11.5", "egg-path-matching": "^1.2.0", - "humanize-bytes": "^1.0.1" + "humanize-bytes": "^1.0.1", + "path-to-regexp-v8": "npm:path-to-regexp@8", + "stream-wormhole": "^2.0.1" }, "devDependencies": { "@eggjs/tsconfig": "^1.2.0", "@types/node": "^20.14.2", "coffee": "^5.4.0", "egg": "^3.9.0", "egg-bin": "6", "egg-mock": "^5.4.0", "eslint": "^8.23.1", "eslint-config-egg": "^12", "formstream": "^1.5.1", "is-type-of": "^1.2.1", "typescript": "5", - "urllib": "3", - "stream-wormhole": "^2.0.1", - "path-to-regexp-v8": "npm:path-to-regexp@8" + "urllib": "3" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/middleware/multipart.js(1 hunks)package.json(2 hunks)test/dynamic-option.test.js(1 hunks)test/enable-pathToRegexpModule.test.js(1 hunks)test/file-mode.test.js(5 hunks)test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/app/controller/save.js(1 hunks)test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/app/controller/upload.js(1 hunks)test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/app/router.js(1 hunks)test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/app/views/home.html(1 hunks)test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/config/config.default.js(1 hunks)test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/config/config.unittest.js(1 hunks)test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/package.json(1 hunks)test/fixtures/apps/fileModeMatch-glob/app/router.js(1 hunks)test/stream-mode-with-filematch-glob.test.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/config/config.unittest.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/app/controller/upload.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/app/controller/save.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/app/router.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/enable-pathToRegexpModule.test.js
[error] 19-22: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 26-26: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 27-27: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
🔇 Additional comments (7)
test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/config/config.unittest.js (1)
3-8: LGTM! Clean test configuration.The logging configuration is well-structured for the test environment, with console logging disabled to keep test output clean.
app/middleware/multipart.js (1)
3-3: LGTM! Function signature updated to support application context.The addition of the
appparameter allows access to application-wide options, which is necessary for the custom pathToRegexpModule support.test/fixtures/apps/fileModeMatch-glob/app/router.js (1)
6-6: LGTM! Test route added for nested path matching.The new route
/upload_file/fooprovides coverage for testing nested path patterns with the custom pathToRegexpModule.test/dynamic-option.test.js (1)
44-44: LGTM! Improved error message assertion.The change to use
assert.match()with a regex pattern makes the test more resilient to minor variations in error messages while maintaining the core validation.test/stream-mode-with-filematch-glob.test.js (1)
Line range hint
32-72: LGTM! Comprehensive test coverage for file upload functionality.The test case thoroughly verifies the file upload behavior, including field values, file metadata, and proper file handling.
test/file-mode.test.js (1)
190-190: LGTM! Improved error message assertions.The switch to regex matching for error messages makes the tests more maintainable and flexible to minor changes in error message formatting.
Also applies to: 208-208, 223-223, 240-240, 259-259
package.json (1)
49-49: Verify compatibility with egg-path-matching@1.2.0The minor version bump of egg-path-matching (1.0.1 → 1.2.0) might introduce new features or changes. Let's verify the changelog for any breaking changes.
✅ Verification successful
Version upgrade to egg-path-matching@1.2.0 is safe and beneficial
The upgrade adds support for custom path-to-regexp modules and maintains backward compatibility. The changes align perfectly with the PR's objective to support custom pathToRegexpModule.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the changelog and git history of egg-path-matching for breaking changes between 1.0.1 and 1.2.0 # Fetch the package info and changelog curl -s https://registry.npmjs.org/egg-path-matching | jq -r '.versions | keys[]' # List all versions curl -s https://raw.githubusercontent.com/eggjs/egg-path-matching/master/CHANGELOG.md # Check changelogLength of output: 1956
Script:
#!/bin/bash # Check the actual code changes between 1.0.1 and 1.2.0 curl -s https://raw.githubusercontent.com/eggjs/egg-path-matching/v1.0.1/index.js > v1.0.1.js curl -s https://raw.githubusercontent.com/eggjs/egg-path-matching/v1.2.0/index.js > v1.2.0.js diff v1.0.1.js v1.2.0.js || trueLength of output: 2008
| module.exports = async ctx => { | ||
| ctx.body = { | ||
| body: ctx.request.body, | ||
| files: ctx.request.files, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and type checking.
The controller should include error handling and type validation for robust file upload handling.
Consider this enhanced implementation:
-module.exports = async ctx => {
+module.exports = async ctx => {
+ try {
+ // Validate that files were actually uploaded
+ if (!ctx.request.files || Object.keys(ctx.request.files).length === 0) {
+ ctx.status = 400;
+ ctx.body = { error: 'No files uploaded' };
+ return;
+ }
+
ctx.body = {
body: ctx.request.body,
files: ctx.request.files,
};
+ } catch (err) {
+ ctx.status = 500;
+ ctx.body = { error: 'File upload failed' };
+ // Log the error for debugging
+ ctx.logger.error('[upload] Error processing file upload:', err);
+ }
};Committable suggestion skipped: line range outside the PR's diff.
| module.exports = async ctx => { | ||
| await ctx.saveRequestFiles(); | ||
| ctx.body = { | ||
| body: ctx.request.body, | ||
| files: ctx.request.files, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file saving operation.
The controller should handle potential errors during the file saving process.
Consider this enhanced implementation:
-module.exports = async ctx => {
+module.exports = async ctx => {
+ try {
+ // Validate that files were actually uploaded
+ if (!ctx.request.files || Object.keys(ctx.request.files).length === 0) {
+ ctx.status = 400;
+ ctx.body = { error: 'No files uploaded' };
+ return;
+ }
+
await ctx.saveRequestFiles();
ctx.body = {
body: ctx.request.body,
files: ctx.request.files,
};
+ } catch (err) {
+ ctx.status = 500;
+ ctx.body = { error: 'Failed to save uploaded files' };
+ // Log the error for debugging
+ ctx.logger.error('[save] Error saving uploaded files:', err);
+ }
};Committable suggestion skipped: line range outside the PR's diff.
| const matchFn = options.fileModeMatch && pathMatching({ | ||
| match: options.fileModeMatch, | ||
| pathToRegexpModule: app.options.pathToRegexpModule, | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Documentation needed for pathToRegexpModule option
The option is well-tested with comprehensive test coverage, but lacks documentation. Please add documentation in README.md explaining:
- Purpose of pathToRegexpModule option
- Configuration examples
- Supported values (e.g., path-to-regexp-v8)
🔗 Analysis chain
Verify pathToRegexpModule configuration.
The integration looks good, but let's ensure the pathToRegexpModule option is properly documented and tested.
Run this script to check for documentation and test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for documentation of pathToRegexpModule option
rg -l "pathToRegexpModule" README.md docs/
# Verify test coverage
rg -l "pathToRegexpModule" test/
Length of output: 235
Script:
#!/bin/bash
# Check test file content
cat test/enable-pathToRegexpModule.test.js
# Look for JSDoc or comments about pathToRegexpModule in source files
rg "pathToRegexpModule" -B 2 -A 2
# Check test fixture package.json
cat test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/package.json
Length of output: 7507
| before(() => { | ||
| app = mock.app({ | ||
| baseDir: 'apps/fileModeMatch-glob-with-pathToRegexpModule', | ||
| pathToRegexpModule: require.resolve('path-to-regexp-v8'), | ||
| }); | ||
| return app.ready(); | ||
| }); | ||
| before(() => { | ||
| server = app.listen(); | ||
| host = 'http://127.0.0.1:' + server.address().port; | ||
| }); | ||
| after(() => { | ||
| return fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true }); | ||
| }); | ||
| after(() => app.close()); | ||
| after(() => server.close()); | ||
| beforeEach(() => app.mockCsrf()); | ||
| afterEach(mock.restore); |
There was a problem hiding this comment.
Consolidate duplicate before/after hooks.
The test setup contains duplicate before/after hooks which should be consolidated.
- before(() => {
- app = mock.app({
- baseDir: 'apps/fileModeMatch-glob-with-pathToRegexpModule',
- pathToRegexpModule: require.resolve('path-to-regexp-v8'),
- });
- return app.ready();
- });
- before(() => {
- server = app.listen();
- host = 'http://127.0.0.1:' + server.address().port;
- });
+ before(async () => {
+ app = mock.app({
+ baseDir: 'apps/fileModeMatch-glob-with-pathToRegexpModule',
+ pathToRegexpModule: require.resolve('path-to-regexp-v8'),
+ });
+ await app.ready();
+ server = app.listen();
+ host = 'http://127.0.0.1:' + server.address().port;
+ });
- after(() => app.close());
- after(() => server.close());
+ after(async () => {
+ await app.close();
+ await server.close();
+ });📝 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.
| before(() => { | |
| app = mock.app({ | |
| baseDir: 'apps/fileModeMatch-glob-with-pathToRegexpModule', | |
| pathToRegexpModule: require.resolve('path-to-regexp-v8'), | |
| }); | |
| return app.ready(); | |
| }); | |
| before(() => { | |
| server = app.listen(); | |
| host = 'http://127.0.0.1:' + server.address().port; | |
| }); | |
| after(() => { | |
| return fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true }); | |
| }); | |
| after(() => app.close()); | |
| after(() => server.close()); | |
| beforeEach(() => app.mockCsrf()); | |
| afterEach(mock.restore); | |
| before(async () => { | |
| app = mock.app({ | |
| baseDir: 'apps/fileModeMatch-glob-with-pathToRegexpModule', | |
| pathToRegexpModule: require.resolve('path-to-regexp-v8'), | |
| }); | |
| await app.ready(); | |
| server = app.listen(); | |
| host = 'http://127.0.0.1:' + server.address().port; | |
| }); | |
| after(() => { | |
| return fs.rm(app.config.multipart.tmpdir, { force: true, recursive: true }); | |
| }); | |
| after(async () => { | |
| await app.close(); | |
| await server.close(); | |
| }); | |
| beforeEach(() => app.mockCsrf()); | |
| afterEach(mock.restore); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 19-22: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 26-26: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 27-27: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[skip ci] ## [3.5.0](v3.4.0...v3.5.0) (2025-01-22) ### Features * support custom pathToRegexpModule ([#66](#66)) ([d474eec](d474eec))
part of eggjs/core#290
Summary by CodeRabbit
Dependencies
egg-path-matchingto version 1.2.0path-to-regexp-v8dependencyMiddleware
Testing
Configuration