Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support custom pathToRegexpModule #66

Merged
merged 1 commit into from
Jan 22, 2025
Merged

feat: support custom pathToRegexpModule #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions app/middleware/multipart.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
'use strict';

const pathMatching = require('egg-path-matching');

module.exports = options => {
module.exports = (options, app) => {
// normalize
const matchFn = options.fileModeMatch && pathMatching({ match: options.fileModeMatch });
const matchFn = options.fileModeMatch && pathMatching({
match: options.fileModeMatch,
pathToRegexpModule: app.options.pathToRegexpModule,
});

Comment on lines +5 to +8
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

return async function multipart(ctx, next) {
if (!ctx.is('multipart')) return next();
Expand Down
5 changes: 3 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"dependencies": {
"co-busboy": "^2.0.0",
"dayjs": "^1.11.5",
"egg-path-matching": "^1.0.1",
"egg-path-matching": "^1.2.0",
"humanize-bytes": "^1.0.1"
},
"devDependencies": {
Expand All @@ -62,6 +62,7 @@
"is-type-of": "^1.2.1",
"typescript": "5",
"urllib": "3",
"stream-wormhole": "^2.0.1"
"stream-wormhole": "^2.0.1",
"path-to-regexp-v8": "npm:path-to-regexp@8"
}
}
2 changes: 1 addition & 1 deletion test/dynamic-option.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,6 @@ describe('test/dynamic-option.test.js', () => {
});

assert(res.status === 413);
assert(res.data.toString().includes('Request_fileSize_limitError: Reach fileSize limit'));
assert.match(res.data.toString(), /Error: Reach fileSize limit/);
});
});
135 changes: 135 additions & 0 deletions test/enable-pathToRegexpModule.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
const assert = require('assert');
const formstream = require('formstream');
const urllib = require('urllib');
const path = require('path');
const mock = require('egg-mock');
const fs = require('fs').promises;

describe('test/enable-pathToRegexpModule.test.js', () => {
let app;
let server;
let host;
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);
Comment on lines +12 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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)


it('should upload match file mode work on /upload_file', async () => {
const form = formstream();
form.field('foo', 'fengmk2').field('love', 'egg');
form.file('file1', __filename, 'foooooooo.js');
form.file('file2', __filename);
// will ignore empty file
form.buffer('file3', Buffer.from(''), '', 'application/octet-stream');
form.file('bigfile', path.join(__dirname, 'fixtures', 'bigfile.js'));
// other form fields
form.field('work', 'with Node.js');

const headers = form.headers();
const res = await urllib.request(host + '/upload_file', {
method: 'POST',
headers,
stream: form,
});

assert(res.status === 200);
const data = JSON.parse(res.data);
assert.deepStrictEqual(data.body, { foo: 'fengmk2', love: 'egg', work: 'with Node.js' });
assert(data.files.length === 3);
assert(data.files[0].field === 'file1');
assert(data.files[0].filename === 'foooooooo.js');
assert(data.files[0].encoding === '7bit');
assert(data.files[0].mime === 'application/javascript');
assert(data.files[0].filepath.startsWith(app.config.multipart.tmpdir));

assert(data.files[1].field === 'file2');
assert(data.files[1].filename === 'enable-pathToRegexpModule.test.js');
assert(data.files[1].encoding === '7bit');
assert(data.files[1].mime === 'application/javascript');
assert(data.files[1].filepath.startsWith(app.config.multipart.tmpdir));

assert(data.files[2].field === 'bigfile');
assert(data.files[2].filename === 'bigfile.js');
assert(data.files[2].encoding === '7bit');
assert(data.files[2].mime === 'application/javascript');
assert(data.files[2].filepath.startsWith(app.config.multipart.tmpdir));
});

it('should upload match file mode work on /upload_file/*', async () => {
const form = formstream();
form.field('foo', 'fengmk2').field('love', 'egg');
form.file('file1', __filename, 'foooooooo.js');
form.file('file2', __filename);
// will ignore empty file
form.buffer('file3', Buffer.from(''), '', 'application/octet-stream');
form.file('bigfile', path.join(__dirname, 'fixtures', 'bigfile.js'));
// other form fields
form.field('work', 'with Node.js');

const headers = form.headers();
const res = await urllib.request(host + '/upload_file/foo', {
method: 'POST',
headers,
stream: form,
});

assert.equal(res.status, 200);
const data = JSON.parse(res.data);
assert.deepStrictEqual(data.body, { foo: 'fengmk2', love: 'egg', work: 'with Node.js' });
assert(data.files.length === 3);
assert(data.files[0].field === 'file1');
assert(data.files[0].filename === 'foooooooo.js');
assert(data.files[0].encoding === '7bit');
assert(data.files[0].mime === 'application/javascript');
assert(data.files[0].filepath.startsWith(app.config.multipart.tmpdir));

assert(data.files[1].field === 'file2');
assert(data.files[1].filename === 'enable-pathToRegexpModule.test.js');
assert(data.files[1].encoding === '7bit');
assert(data.files[1].mime === 'application/javascript');
assert(data.files[1].filepath.startsWith(app.config.multipart.tmpdir));

assert(data.files[2].field === 'bigfile');
assert(data.files[2].filename === 'bigfile.js');
assert(data.files[2].encoding === '7bit');
assert(data.files[2].mime === 'application/javascript');
assert(data.files[2].filepath.startsWith(app.config.multipart.tmpdir));
});

it('should upload not match file mode', async () => {
const form = formstream();
form.field('foo', 'fengmk2').field('love', 'egg');
form.file('file1', __filename, 'foooooooo.js');
form.file('file2', __filename);
// will ignore empty file
form.buffer('file3', Buffer.from(''), '', 'application/octet-stream');
form.file('bigfile', path.join(__dirname, 'fixtures', 'bigfile.js'));
// other form fields
form.field('work', 'with Node.js');

const headers = form.headers();
const res = await urllib.request(host + '/upload', {
method: 'POST',
headers,
stream: form,
});

assert(res.status === 200);
const data = JSON.parse(res.data);
assert.deepStrictEqual(data, { body: {} });
});
});
10 changes: 5 additions & 5 deletions test/file-mode.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ describe('test/file-mode.test.js', () => {
});

assert(res.status === 413);
assert(res.data.toString().includes('Request_fields_limitError: Reach fields limit'));
assert.match(res.data.toString(), /Error: Reach fields limit/);
});

it('should throw error when request files limit', async () => {
Expand All @@ -205,7 +205,7 @@ describe('test/file-mode.test.js', () => {
});

assert(res.status === 413);
assert(res.data.toString().includes('Request_files_limitError: Reach files limit'));
assert.match(res.data.toString(), /Error: Reach files limit/);
});

it('should throw error when request field size limit', async () => {
Expand All @@ -220,7 +220,7 @@ describe('test/file-mode.test.js', () => {
});

assert(res.status === 413);
assert(res.data.toString().includes('Request_fieldSize_limitError: Reach fieldSize limit'));
assert.match(res.data.toString(), /Error: Reach fieldSize limit/);
});

// fieldNameSize is TODO on busboy
Expand All @@ -237,7 +237,7 @@ describe('test/file-mode.test.js', () => {
});

assert(res.status === 413);
assert(res.data.toString().includes('Request_fieldSize_limitError: Reach fieldSize limit'));
assert.match(res.data.toString(), /Error: Reach fieldSize limit/);
});

it('should throw error when request file size limit', async () => {
Expand All @@ -256,7 +256,7 @@ describe('test/file-mode.test.js', () => {
});

assert(res.status === 413);
assert(res.data.toString().includes('Request_fileSize_limitError: Reach fileSize limit'));
assert.match(res.data.toString(), /Error: Reach fileSize limit/);
});

it('should throw error when file name invalid', async () => {
Expand Down
9 changes: 9 additions & 0 deletions test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/app/controller/save.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
'use strict';

module.exports = async ctx => {
await ctx.saveRequestFiles();
ctx.body = {
body: ctx.request.body,
files: ctx.request.files,
};
};
Comment on lines +3 to +9
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

8 changes: 8 additions & 0 deletions test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/app/controller/upload.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
'use strict';

module.exports = async ctx => {
ctx.body = {
body: ctx.request.body,
files: ctx.request.files,
};
};
Comment on lines +3 to +8
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

8 changes: 8 additions & 0 deletions test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/app/router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
'use strict';

module.exports = app => {
app.post('/upload', app.controller.upload);
app.post('/upload_file', app.controller.upload);
app.post('/upload_file/foo', app.controller.upload);
app.post('/save', app.controller.save);
};
8 changes: 8 additions & 0 deletions test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/app/views/home.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<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>
6 changes: 6 additions & 0 deletions test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/config/config.default.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
exports.multipart = {
mode: 'stream',
fileModeMatch: '/upload_file{/:paths}'
};

exports.keys = 'multipart';
8 changes: 8 additions & 0 deletions test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/config/config.unittest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
'use strict';

exports.logger = {
consoleLevel: 'NONE',
coreLogger: {
// consoleLevel: 'DEBUG',
},
};
3 changes: 3 additions & 0 deletions test/fixtures/apps/fileModeMatch-glob-with-pathToRegexpModule/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"name": "fileModeMatch-glob-with-pathToRegexpModule"
}
1 change: 1 addition & 0 deletions test/fixtures/apps/fileModeMatch-glob/app/router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@
module.exports = app => {
app.post('/upload', app.controller.upload);
app.post('/upload_file', app.controller.upload);
app.post('/upload_file/foo', app.controller.upload);
app.post('/save', app.controller.save);
};
43 changes: 42 additions & 1 deletion test/stream-mode-with-filematch-glob.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('test/stream-mode-with-filematch-glob.test.js', () => {
beforeEach(() => app.mockCsrf());
afterEach(mock.restore);

it('should upload match file mode', async () => {
it('should upload match file mode work on /upload_file', async () => {
const form = formstream();
form.field('foo', 'fengmk2').field('love', 'egg');
form.file('file1', __filename, 'foooooooo.js');
Expand Down Expand Up @@ -70,6 +70,47 @@ describe('test/stream-mode-with-filematch-glob.test.js', () => {
assert(data.files[2].filepath.startsWith(app.config.multipart.tmpdir));
});

it('should upload match file mode work on /upload_file/*', async () => {
const form = formstream();
form.field('foo', 'fengmk2').field('love', 'egg');
form.file('file1', __filename, 'foooooooo.js');
form.file('file2', __filename);
// will ignore empty file
form.buffer('file3', Buffer.from(''), '', 'application/octet-stream');
form.file('bigfile', path.join(__dirname, 'fixtures', 'bigfile.js'));
// other form fields
form.field('work', 'with Node.js');

const headers = form.headers();
const res = await urllib.request(host + '/upload_file/foo', {
method: 'POST',
headers,
stream: form,
});

assert.equal(res.status, 200);
const data = JSON.parse(res.data);
assert.deepStrictEqual(data.body, { foo: 'fengmk2', love: 'egg', work: 'with Node.js' });
assert(data.files.length === 3);
assert(data.files[0].field === 'file1');
assert(data.files[0].filename === 'foooooooo.js');
assert(data.files[0].encoding === '7bit');
assert(data.files[0].mime === 'application/javascript');
assert(data.files[0].filepath.startsWith(app.config.multipart.tmpdir));

assert(data.files[1].field === 'file2');
assert(data.files[1].filename === 'stream-mode-with-filematch-glob.test.js');
assert(data.files[1].encoding === '7bit');
assert(data.files[1].mime === 'application/javascript');
assert(data.files[1].filepath.startsWith(app.config.multipart.tmpdir));

assert(data.files[2].field === 'bigfile');
assert(data.files[2].filename === 'bigfile.js');
assert(data.files[2].encoding === '7bit');
assert(data.files[2].mime === 'application/javascript');
assert(data.files[2].filepath.startsWith(app.config.multipart.tmpdir));
});

it('should upload not match file mode', async () => {
const form = formstream();
form.field('foo', 'fengmk2').field('love', 'egg');
Expand Down
Loading