Bug/#3267 hapi upload #3268

swashcap · 2019-09-08T23:04:27Z

Problem: File uploads in apollo-server-hapi appear broken (see File uploads in apollo-server-hapi appear broken #3267). This appears to be due to request.mime not being set here:

apollo-server/packages/apollo-server-hapi/src/ApolloServer.ts

Line 22 in 10402c4

request.mime === 'multipart/form-data'

, which is probably due to the hapi not fully parsing the request on onRequest extensions.
Solution: Move the file upload handling to the route registered in the hapi plugin.

Testing: Just run the relevant tests:

$ npx jest packages/apollo-server-hapi/src/__tests__/*.ts

This addresses broken file upload handling by moving `processFileUploads` and related checks to the GraphQL route.

apollo-cla · 2019-09-08T23:04:29Z

@swashcap: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

swashcap · 2019-09-08T23:09:01Z

Huh, seeing this error on the server from the un-ignored test:

Missing multipart field ‘operations’ (https://github.com/jaydenseric/graphql-multipart-request-spec).

The server is returning a 500. Interesting!

swashcap · 2019-09-08T23:56:09Z

Ah, I see: moving upload handling to the route handler in c63bcad ensures that hapi parses the request and sets request.payload and request.mime, However, this reads the request stream entirely, allowing hapi to do some file upload handling. graphql-upload's processRequest needs the request stream to be unread!

Kinda a catch 22: request.mime isn't set unless the payload is parsed, but the payload can't be parsed for graphql-upload.

swashcap · 2019-09-09T01:18:24Z

Unfortunately, changing the content-type check here doesn't work:

apollo-server/packages/apollo-server-hapi/src/ApolloServer.ts

Line 22 in 10402c4

request.mime === 'multipart/form-data'

…to something like this:

- request.mime === 'multipart/form-data'
+ /^multipart\/form-data/.test(request.headers['content-type'])

payload isn't configurable, yet hapi mutates it here:

It looks like hapi re-assigns request.payload here: https://github.com/hapijs/hapi/blob/020065db61964f46e6af32293709f96ad61c6da7/lib/route.js#L401

We could change configurable to true, but then we run into another 500 error, this time:

POST body missing. Did you forget use body-parser middleware?

From here:

apollo-server/packages/apollo-server-core/src/runHttpQuery.ts

Lines 199 to 204 in 10402c4

    
           if (!httpRequest.query || Object.keys(httpRequest.query).length === 0) { 
        
             throw new HttpQueryError( 
        
               500, 
        
               'POST body missing. Did you forget use body-parser middleware?', 
        
             ); 
        
           }

Which seems to indicate some request payload parsing problems on the hapi side.

swashcap · 2019-09-09T05:48:10Z

I'm looking into the idiomatic hapi way to handle this and ran across hapijs/hapi#586, which helpfully points to the server.inject method, which references route.options.isInternal, a route configuration that registers the route for internal use only. I believe we can register an “internal only” route for multipart/form-data requests, then use request.setUrl to this in the server.ext('onRoute') handler. Going to give it a try.

swashcap · 2019-09-09T07:01:23Z

It looks like server.inject isn't able to forward the original request. Merely passing the original request.payload yields the same "Missing multipart field ‘operations’ (https://github.com/jaydenseric/graphql-multipart-request-spec)." error, likely because the injection library, shot, isn't transforming the original payload into a proper http.IncomingMessage?. 😕

BREAKING CHANGE: This alters the `options.route` configuration object for the POST route. This separates GET and POST routes to facilitate extra route parsing configuration for the POST route. (hapi throws an error for this configuration when the route handler serves both methods.) POST route handling now parses the request manually to correct multipart/form-data requests through graphql-upload, and manually parses requests with hapijs/subtext for non-form requests.

swashcap · 2019-09-09T08:29:31Z

packages/apollo-server-hapi/src/ApolloServer.ts

-  processFileUploads,
-} from 'apollo-server-core';
-
-function handleFileUploads(uploadsConfig: FileUploadOptions) {


This logic/handling moved to the hapi plugin.

swashcap · 2019-09-09T08:29:56Z

packages/apollo-server-hapi/src/__tests__/ApolloServer.test.ts

@@ -409,7 +409,7 @@ const port = 0;
        });
      });
      describe('file uploads', () => {
-        xit('enabled uploads', async () => {
+        it('enabled uploads', async () => {


Test the thing!

swashcap · 2019-09-09T08:30:34Z

packages/apollo-server-hapi/src/ApolloServer.ts

@@ -138,6 +111,7 @@ export class ApolloServer extends ApolloServerBase {
            : {
                cors: cors !== undefined ? cors : true,
              },
+        uploadsConfig: this.uploadsConfig,


Pass the upload configuration to the plugin for use within the route handler (see comments below).

swashcap · 2019-09-09T08:31:46Z

packages/apollo-server-hapi/src/hapiApollo.ts

-      path: options.path || '/graphql',
-      vhost: options.vhost || undefined,
-      options: options.route || {},
-      handler: async (request, h) => {


This handler function's body – with some query assignment changes – is copied to the handler function.

swashcap · 2019-09-09T08:34:21Z

packages/apollo-server-hapi/src/hapiApollo.ts

+          // The request is unparsed due to the 'POST' route's config. Use
+          // Subtext, hapi's default parser, to parse the request.
+          // https://github.com/hapijs/subtext
+          const { payload } = await Subtext.parse(request.raw.req, null, {


Subtext does the route payload parsing in hapi: https://github.com/hapijs/hapi/blob/03ca9133d016945ecc8642cc73fa7b05864ec605/lib/route.js#L429

hapi users will already have this library installed.

swashcap · 2019-09-09T08:36:52Z

packages/apollo-server-hapi/src/hapiApollo.ts

+        path,
+        vhost: options.vhost || undefined,
+        handler,
+        options: {


Here's where it gets a little tricky: This patch necessarily adjusts the options.route behavior for the POST route (hapi throws when the options.payload is applied to a GET route). This could be considered a breaking change.

What this does: prevents hapi from parsing the http.IncomingMessage, and passes the raw stream to the route handler as a readable stream.

See:

options.payload.output: https://hapi.dev/api/?v=18.3.2#route.options.payload.output

options.payload.parse: https://hapi.dev/api/?v=18.3.2#route.options.payload.parse

This means the route handler must manually parse the payload (thus the use of subtext).

Could you help me understand the breaking change aspect of this a bit more?

Trying to recall what I did here 😅

This package's ApolloServer#applyMiddleware has an optional route configuration that's passed directly to the hapi plugin if it's defined. This PR introduces a change in how the hapi plugin's route handler accepts uploads by changing the route's options.

Before:

apollo-server/packages/apollo-server-hapi/src/hapiApollo.ts

Line 41 in 0926b20

options: options.route || {},

After (abbreviated):

server.route([ { method: 'GET', // ... options: options.route || {}, }, { method: 'POST', // ... options: { ...options.route, payload: { output: 'stream', parse: false, }, } } ])

Notice how a user's configuration for payload.options isn't passed through to the route. This is a necessary change: graphql-upload's processRequest (via processFileUploads) expects an unread stream such that it can handle the parsing. This is the only hapi route.options.payload config that doesn't read the incoming message (that I found, anway). Unfortunately, allowing a user to override this breaks uploads.

This modifies how apollo-server-hapi passes user config down to the underlying route: should it be marked as a breaking change?

I guess we could change it to:

{ options: { ...options.route, payload: { + ...(options.route || {}).payload, output: 'stream', parse: false, } } }

to give users that extra control.

Hmmm, it looks like users were never able to set route.payload. I made a demo on Glitch to test this out: https://glitch.com/edit/#!/scratch-animantarx?path=server.js:44:4

Even with route.payload set to empty object, hapi seems to error:

AssertionError [ERR_ASSERTION]: Cannot set payload settings on HEAD or GET request: GET /graphql at new AssertionError (internal/assert.js:269:11) # ...

The app never seems to start. If you "Remix" (fork in Glitch terms? first time trying it out) and find that "Logs" button in the bottom right you'll see the error.

I think the user was never able to pass payload, so this wouldn't be a breaking change at all 🎉

I can confirm that I'm getting this error trying to override payload, because payload should only be set on POST requests, and not HEAD or GET. But currently if you pass payload through as route options it will set on the GET route of /graphql too.

So yes it's not a breaking change

swashcap · 2019-09-09T08:38:28Z

types/subtext/index.d.ts

@@ -0,0 +1,18 @@
+declare module 'subtext' {


@hapi/subtext has types, but not subtext. This should do until apollo-server upgrades to the @hapi-scoped packages.

I'm wrong, there aren't official types: https://www.npmjs.com/package/@types/hapi__subtext. Perhaps I should contribute these back to the @types repo.

swashcap · 2019-09-09T08:45:06Z

Looks like the Node 6 tests fail:

    Details:

    /home/circleci/project/node_modules/subtext/lib/index.js:25
    exports.parse = async function (req, tap, options) {
                          ^^^^^^^^

    SyntaxError: Unexpected token function



      at ScriptTransformer._transformAndBuildScript (../../node_modules/@jest/core/node_modules/@jest/transform/build/ScriptTransformer.js:537:17)
      at ScriptTransformer.transform (../../node_modules/@jest/core/node_modules/@jest/transform/build/ScriptTransformer.js:579:25)
      at Object.<anonymous> (src/hapiApollo.ts:1850:42)

— https://circleci.com/gh/apollographql/apollo-server/50505?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

What do you think about dropping Node.js 6 for apollo-server-hapi? hapi@^17 only supports Node.js >=8.x.x: https://github.com/hapijs/hapi/blob/v17-commercial/.travis.yml

abernix

@swashcap I really appreciate your attention to fixing this and the comments you've left throughout. Very helpful!

Side-note / Relatedly: I will mention here that we will be unable to actually apply the more major version bump to Hapi to v18 that you opened in #3273. Others have tried, and we've tried to accommodate as well, but it's just not something that we can do within Apollo Server 2.x. Additionally, Apollo Server 3.x will remove the close coupling with Hapi (and all web frameworks) as a concern from this repository with #3184 being the intended replacement. As you're familiar with Hapi, your feedback appreciated on that issue would be very much appreciated, but the idea is that there would be:

Many less baked-in middleware which can't be removed, swapped out or customized; and

A slim adapter between the Apollo Server transport (which we will provide) and frameworks (e.g. Hapi). I hope this will work much better and free us from this close version coupling and the constraints it puts on both Apollo Server users and maintainers.

Back to the PR at hand though: I support this, since it fixes existing functionality.

And since, as you correctly point out, Hapi did in fact never officially test Node.js v6 going into its v17 release line (see the v17.0.0 tag's .travis.yml), I would support dropping Node.js 6 tests in the same way we do already here:

apollo-server/packages/apollo-server-hapi/src/__tests__/hapiApollo.test.ts

Line 11 in 6dc132e

(NODE_MAJOR_VERSION < 8 ? describe.skip : describe)('integration:Hapi', () => {

If you can help me understand how the potential breaking change you mentioned below might be or not be a breaking change, I'd like to think about it briefly before finalizing this. If it is, in fact, a breaking a change, it may be the case that we simply cannot land this, but I hope that's not the case.

abernix · 2019-09-20T11:41:58Z

packages/apollo-server-hapi/src/hapiApollo.ts

+        path,
+        vhost: options.vhost || undefined,
+        handler,
+        options: {


Could you help me understand the breaking change aspect of this a bit more?

swashcap · 2019-09-24T05:32:37Z

Had another look at the test failure:

    Details:

    /home/circleci/project/node_modules/subtext/lib/index.js:25
    exports.parse = async function (req, tap, options) {
                          ^^^^^^^^

    SyntaxError: Unexpected token function

— https://circleci.com/gh/apollographql/apollo-server/50505?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Huh. Hapi doesn't transpile for older versions of Node.js. Probably means a little test matrix reworking, huh?

swashcap · 2019-09-24T06:03:41Z

Apollo Server 3.x will remove the close coupling with Hapi (and all web frameworks) as a concern from this repository with #3184 being the intended replacement. As you're familiar with Hapi, your feedback appreciated on that issue would be very much appreciated, but the idea is that there would be:

Many less baked-in middleware which can't be removed, swapped out or customized; and
A slim adapter between the Apollo Server transport (which we will provide) and frameworks (e.g. Hapi). I hope this will work much better and free us from this close version coupling and the constraints it puts on both Apollo Server users and maintainers.

I haven't had a chance to read through the proposal, but this sounds like a great idea: I believe hapi developers would appreciate owning some more boilerplate in exchange for the ability to keep up with the latest version. Personally, I've found that packages that wrap top-level dependencies in this fashion lead to some difficulty in upgrades, so I think this a good move from a maintenance standpoint. I'll read through it and mention it to my hapi-enthusiast coworkers to get some feedback.

wzrdtales · 2020-06-05T15:50:07Z

so this is not going to be merged and fixed anytime soon?

wzrdtales · 2020-06-05T17:12:47Z

works perfectly fine for me @wzrdtales/apollo-server-hapi, using this module as replacement right now with this pull request and it works with hapi 19 perfectly fine

swashcap · 2020-06-07T20:20:11Z

so this is not going to be merged and fixed anytime soon?

Been a while since I opened this PR. I believe we'd need some hapi-specific test matrix changes to get this working (remove NodeJS 6?). Looks like both 6 was turned off in c68c577. I'll have a look at rebasing.

wzrdtales · 2020-06-24T20:28:19Z

why was this closed?

b-jam · 2020-08-11T04:05:25Z

Is this just a case of disabling Node 6 tests and merging? Keen to use this fix as I'm running into the same errors with GET/HEAD payload mentioned above.

TdyP · 2020-12-18T09:03:41Z

Hi guys!
Any plan on merging this PR? I'm facing the Must provide query string issue and @wzrdtales fix works perfectly fine for me.

Thanks a lot! :)

glasser · 2022-10-04T22:34:19Z

File upload integration was removed from Apollo Server in AS3 last year. Sorry for the delay!

swashcap added 2 commits September 8, 2019 15:58

chore(apollo-server-hapi): un-ignore hapi upload test

6b85833

fix(apollo-server-hapi): move upload handling to hapi plugin

c63bcad

This addresses broken file upload handling by moving `processFileUploads` and related checks to the GraphQL route.

swashcap commented Sep 9, 2019

View reviewed changes

abernix added the 🖇️ hapi label Sep 20, 2019

abernix reviewed Sep 20, 2019

View reviewed changes

swashcap mentioned this pull request Sep 24, 2019

updated hapi to match current @hapi/* #3320

Closed

abernix mentioned this pull request Oct 4, 2019

Update apollo-server-hapi to latest versions. #3374

Closed

abernix closed this Jun 24, 2020

abernix reopened this Jun 25, 2020

abernix changed the base branch from master to main June 25, 2020 09:04

abernix added the 🧬 uploads label Dec 31, 2020

wzrdtales mentioned this pull request May 26, 2022

Support Hapi jaydenseric/graphql-upload#239

Closed

glasser closed this Oct 4, 2022

github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug/#3267 hapi upload #3268

Bug/#3267 hapi upload #3268

swashcap commented Sep 8, 2019

apollo-cla commented Sep 8, 2019

swashcap commented Sep 8, 2019 •

edited

Loading

swashcap commented Sep 8, 2019

swashcap commented Sep 9, 2019 •

edited

Loading

swashcap commented Sep 9, 2019

swashcap commented Sep 9, 2019 •

edited

Loading

swashcap Sep 9, 2019

swashcap Sep 9, 2019

swashcap Sep 9, 2019

swashcap Sep 9, 2019

swashcap Sep 9, 2019

swashcap Sep 9, 2019

abernix Sep 20, 2019

swashcap Sep 24, 2019

swashcap Sep 24, 2019

swashcap Sep 24, 2019 •

edited

Loading

b-jam Aug 11, 2020

swashcap Sep 9, 2019

swashcap Sep 9, 2019

swashcap commented Sep 9, 2019

abernix left a comment

abernix Sep 20, 2019

swashcap commented Sep 24, 2019

swashcap commented Sep 24, 2019

wzrdtales commented Jun 5, 2020

wzrdtales commented Jun 5, 2020

swashcap commented Jun 7, 2020

wzrdtales commented Jun 24, 2020

b-jam commented Aug 11, 2020

TdyP commented Dec 18, 2020

glasser commented Oct 4, 2022

Bug/#3267 hapi upload #3268

Bug/#3267 hapi upload #3268

Conversation

swashcap commented Sep 8, 2019

apollo-cla commented Sep 8, 2019

swashcap commented Sep 8, 2019 • edited Loading

swashcap commented Sep 8, 2019

swashcap commented Sep 9, 2019 • edited Loading

swashcap commented Sep 9, 2019

swashcap commented Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swashcap Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swashcap commented Sep 9, 2019

abernix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swashcap commented Sep 24, 2019

swashcap commented Sep 24, 2019

wzrdtales commented Jun 5, 2020

wzrdtales commented Jun 5, 2020

swashcap commented Jun 7, 2020

wzrdtales commented Jun 24, 2020

b-jam commented Aug 11, 2020

TdyP commented Dec 18, 2020

glasser commented Oct 4, 2022

swashcap commented Sep 8, 2019 •

edited

Loading

swashcap commented Sep 9, 2019 •

edited

Loading

swashcap commented Sep 9, 2019 •

edited

Loading

swashcap Sep 24, 2019 •

edited

Loading