Skip to content

fix: allow diskStorage without options and handle string destination in template - #1471

Open
vaibhavmashal wants to merge 2 commits into
expressjs:mainfrom
vaibhavmashal:fix/disk-storage-optional-opts
Open

fix: allow diskStorage without options and handle string destination in template#1471
vaibhavmashal wants to merge 2 commits into
expressjs:mainfrom
vaibhavmashal:fix/disk-storage-optional-opts

Conversation

@vaibhavmashal

Copy link
Copy Markdown

Summary

  • Safely default \opts = opts || {}\ in \DiskStorage\ constructor so that calling \multer.diskStorage()\ without arguments does not throw a TypeError when accessing \opts.filename.
  • Update the custom storage template in \StorageEngine.md\ to safely handle optional \opts\ and support \destination\ passed as either a \string\ or a function (resolves Potential Issue with destination Parameter Type in Multer Custom Storage #1278).
  • Add unit test in \ est/disk-storage.js\ verifying \multer.diskStorage()\ with omitted options uses default destination and filename.

Verification


  • pm run lint\ passed.

  • pm test\ passed (133 passing).

Copilot AI lite review requested due to automatic review settings August 29, 2026 19:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@kilisamemarisaaa kilisamemarisaaa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we remove req.file.path before completing the new omitted-options test, and ideally clean up the adjacent default-destination test as well? I ran the new test alone on Windows while snapshotting os.tmpdir(): it passed, but left one new 1,778-byte file whose SHA-256 matched test/files/small0.dat. The full suite leaves two such files because successful uploads do not call _removeFile and the current afterEach only removes uploadDir.

@vaibhavmashal

Copy link
Copy Markdown
Author

Thanks for the thorough catch and review @kilisamemarisaaa!

I've updated both the new omitted-options test and the adjacent default-destination test in \ est/disk-storage.js\ to call \ s.unlink(req.file.path, done), ensuring any temporary files written to \os.tmpdir()\ are cleaned up before test completion.

Pushed in commit 4ebb5af — all 133 tests continue to pass cleanly.

@kilisamemarisaaa kilisamemarisaaa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed head 4ebb5af. The omitted-options DiskStorage regression is covered, both successful temp-directory tests now unlink req.file.path, and the custom storage template handles omitted opts plus string destinations. I ran npm test (133 passing, 2 pending) and npm run lint locally on Windows. No blocking findings.

@vaibhavmashal

Copy link
Copy Markdown
Author

Thank you for the thorough review and verification, @kilisamemarisaaa! Glad everything is clean and passing.

@vaibhavmashal

Copy link
Copy Markdown
Author

Just to note regarding the 2 pending tests on Windows: those are the Linux-specific file descriptor tests in \ est/abort-fd-leak.js\ and \ est/abort-unlink-after-close.js\ which check /proc/self/fd\ and gracefully \skip\ on non-Linux platforms. On Linux (and CI), all 135 tests execute and pass cleanly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Potential Issue with destination Parameter Type in Multer Custom Storage

3 participants