Closed jaydenseric closed 2 years ago
@mike-marcacci although this PR is a draft, please review these changes as well the changes in the next
branch, particularly the changes to test/abortingMultipartRequest.mjs
:
This PR is only a draft because I would like us to figure out if we need to add extra tests or not, as mentioned at the end of the PR description.
So, in our tests the only time the file stream error listener is called is for the abort tests, and in both cases fileError === exitError
:
There must be some other scenarios when busboy
emits an error on the file read stream that we can test. Maybe some sort of bad characters in the file or something?
Improved the
processRequest
function:Fixed ending requests from being handled incorrectly as aborting in edge cases. Closes #272.
I can't think of a way to reproduce the timing issues described in the above PR in a test. I wasn't even able to reproduce them locally. Considering we've never had reports before about such a thing, I wonder if there is something weird about the environment they were discovered in. Regardless, the new way we're doing it in this PR should solve any timing issues. It's also less listeners and the code is simpler and more obvious, so it's nicer to do it this more modern way anyway.
Some of these nicer new ways of doing things weren't possible with the Node.js APIs that existed when
graphql-upload
was worked on over the years;readable.readableEnded
was only added in Node.js v12.9.0 (which fits our new Node.js version support that begins at^12.20.0
).Fixed read streams created via the resolved
Upload
scalar valuecreateReadStream
method:error
event when the multipart request is aborted certain ways while the file is uploading.Previously our tests of aborting requests used
FormData
instances created viaform-data
directly with the Node.jsstream.Transform
andhttp.request
APIs:https://github.com/jaydenseric/graphql-upload/blob/17993a7f2a41de952cb1d99f2018d67503df9fe6/test/abortingMultipartRequest.mjs#L1-L2
The new way uses
FormData
instances created via the much more spec compliantformdata-node
, withform-data-encoder
,node-fetch
, andnode-abort-controller
:https://github.com/jaydenseric/graphql-upload/blob/b47970970cce60f46bd731abbd8255f5ad8ca8c9/test/abortingMultipartRequest.mjs#L1-L4
This new approach aligns better with web standards, is easier to understand, and less use of Node.js APIs will make a future Deno port of
graphql-upload
much easier. Some of the Node.js APIs that were used, i.e.request.abort()
, are deprecated. Also, GraphQL multipart requests are being aborted vianode-fetch
andAbortController
in the wild, so it's good the tests replicate that.The way the requests abort in the new tests, for some reason I haven't been able to figure out, result in different events and things going on in
processRequest
. The abort seems cleaner and doesn't seem to cause errors on the server side, which was causing the oldprocessRequest
function logic to wrap things up without emitting anerror
orend
event on the partly upload file’s read stream. This was causing this part of the tests to hang:https://github.com/jaydenseric/graphql-upload/blob/b47970970cce60f46bd731abbd8255f5ad8ca8c9/test/public/processRequest.test.mjs#L701
https://github.com/jaydenseric/graphql-upload/runs/4067595912?check_suite_focus=true#step:4:100
Perhaps this has something to do with the weirdness: https://github.com/mscdex/busboy/issues/73 .
error
event details for multipart request file field parse errors.I'm not sure why the code seems like it was wrong at first glance, but @mike-marcacci surely we did it that way for a reason? Please review this change really carefully. Here is a blame through time:
https://github.com/jaydenseric/graphql-upload/blame/master/public/processRequest.js#L297-L301
https://github.com/jaydenseric/graphql-upload/blame/7193c3ddc749eba8ada54d6226851aa0e8c935eb/lib/processRequest.js#L322-L332
This seems to be the last PR relating to this line, and it seems like we thought carefully about it at the time:
https://github.com/jaydenseric/graphql-upload/pull/180
It turns out that making the change I did in this current PR doesn't make a difference; all tests still pass the same. This makes me think we need to write a specific test that covers the logic in this error handler. Perhaps our tests do cover it, but by quirks of timing and such the results are the same either way we handle the error? If this is the case, I prefer the more obvious way in this PR.