Improve the processRequest function

[jaydenseric]

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 value createReadStream method:

  • Not emitting the 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 via form-data directly with the Node.js stream.Transform and http.request APIs:

    graphql-upload/test/abortingMultipartRequest.mjs

    Lines 1 to 2 in 17993a7

    	import { request as httpRequest } from 'http';
    	import { Transform } from 'stream';

    The new way uses FormData instances created via the much more spec compliant formdata-node, with form-data-encoder, node-fetch, and node-abort-controller:

    graphql-upload/test/abortingMultipartRequest.mjs

    Lines 1 to 4 in b479709

    	import { Readable } from 'stream';
    	import { FormDataEncoder } from 'form-data-encoder';
    	import nodeAbortController from 'node-abort-controller';
    	import fetch, { AbortError } from 'node-fetch';

    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 via node-fetch and AbortController 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 old processRequest function logic to wrap things up without emitting an error or end event on the partly upload file’s read stream. This was causing this part of the tests to hang:

    graphql-upload/test/public/processRequest.test.mjs

    Line 701 in b479709

    await rejects(

    https://github.com/jaydenseric/graphql-upload/runs/4067595912?check_suite_focus=true#step:4:100

    Perhaps this has something to do with the weirdness: Invalid busboy behavior on incomplete multipart mscdex/busboy#73 .

  • Emitting incorrect 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:

    Remove redundant Node.js version compatibility logic #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.

[jaydenseric]

@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:

b479709#diff-bcc38e6adea970039237a8373729520e8219c640ec7777bc17becf0ec5a6e6b4R1

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.

[jaydenseric]

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?

[mike-marcacci]

Sorry for the long delay in reviewing this. I had forgotten about it until you made this comment.

These are excellent changes and fixes, and I have no notes or suggestions here!