Skip to content

Fix and test for issue #194#200

Closed
qqilihq wants to merge 4 commits intoexpressjs:masterfrom
qqilihq:master
Closed

Fix and test for issue #194#200
qqilihq wants to merge 4 commits intoexpressjs:masterfrom
qqilihq:master

Conversation

@qqilihq
Copy link
Copy Markdown

@qqilihq qqilihq commented Aug 12, 2015

Fix for error handling for fileSize limit; in case diskStorage was used, route was called when limit was hit, instead of error.

@LinusU
Copy link
Copy Markdown
Member

LinusU commented Aug 13, 2015

Would you mind fixing the following style issues so that the travis build will run 👍

standard: Use JavaScript Standard Style (https://github.com/feross/standard)
  test/express-integration.js:18:17: Missing space before function parentheses.
  test/express-integration.js:19:15: Expected error to be handled.
  test/express-integration.js:19:23: Missing space before function parentheses.
  test/express-integration.js:21:13: Extra semicolon.
  test/express-integration.js:22:7: Extra semicolon.
  test/express-integration.js:24:16: Missing space before function parentheses.
  test/express-integration.js:30:26: Trailing spaces not allowed.
  test/express-integration.js:31:22: Trailing spaces not allowed.
  test/express-integration.js:32:47: Missing space before value for key "destination".
  test/express-integration.js:32:59: Trailing spaces not allowed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not that important but I would prefer:

after(function (done) {
  rimraf(uploadDir, done)

@qqilihq
Copy link
Copy Markdown
Author

qqilihq commented Aug 13, 2015

Sorry, fixed now.

@LinusU
Copy link
Copy Markdown
Member

LinusU commented Aug 13, 2015

No problem, I'm trying to figure out why this works. I remember that I had the abort in an upper scope, such as you have now, but moved it into a tighter scope. I don't remember why thought. Would you mind explaining the code?

Also, maybe the check for aborting should be moved to indicateDone instead of in multer.on('finish')...

@qqilihq
Copy link
Copy Markdown
Author

qqilihq commented Aug 13, 2015

I moved the checking of the aborting flag, as suggested.

My problem was, that in case I was using the disk storage, and the limit was hit, the express route handler was getting called, instead of the error handler. That's why I introduced the !aborting check within the indicateDone (in case, the upload is canceled because of file size limit, the done(uploadError) callback (line 54) will be triggered.

Not sure why the CI test is failing now, works locally for me?

@LinusU
Copy link
Copy Markdown
Member

LinusU commented Sep 19, 2015

I restarted the CI job, it seems like it worked this time 😕

@qqilihq
Copy link
Copy Markdown
Author

qqilihq commented Sep 19, 2015

Hi Linus, not sure about the reason for the fail above. Running it locally several times gave no failure at all.

PS: Still wishful for getting that merged or fixed otherwise :)

@LinusU
Copy link
Copy Markdown
Member

LinusU commented Sep 19, 2015

This should be fixed by #205 now :)

@LinusU LinusU closed this Sep 19, 2015
@qqilihq
Copy link
Copy Markdown
Author

qqilihq commented Sep 27, 2015

Hi Linus, thank you, however I'm still having that problem. As soon as I use the diskStorage, the error event is not called when exceeding fileSizeLimit. In case of using the default storage, everything is fine. Test case demonstrating the issue is my first commit above.

Best,
Philipp

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.

2 participants