Fixes apollographql/apollo-server#4967

[arimus]

trafficstars

Separate GET / POST route configuration for the HAPI server plugin and allow payload options to make their way through to the POST side without creating errors on the GET side.

[apollo-cla]

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

[arimus]

Note: I did also create a v2 backport of this improvement if anyone wants me to take the time to create another pull request.

And sorry for the noise, had a bad version of the test that I checked in and then ran into prettier formatting issues.

[glasser]

Thanks, I'll try to make time to review this when I have time to refresh my memory of how Hapi works.

I'm not super clear on how this fixes #4967 which seems to want you to be able to specify GET and POST configs separately? Seems like this just fixes an error that happens if you specify a particular payload config? Do you think that's sufficient to count as fixing #4967?

Are there any docs changes needed?

[arimus]

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

I actually did already. There was an error in the checklist above, but then it went away after I filled it out. Looks like the CLA above is green. Should I do it again?

[arimus]

Thanks, I'll try to make time to review this when I have time to refresh my memory of how Hapi works.

I'm not super clear on how this fixes #4967 which seems to want you to be able to specify GET and POST configs separately? Seems like this just fixes an error that happens if you specify a particular payload config? Do you think that's sufficient to count as fixing #4967?

Are there any docs changes needed?

Looks like I got a message from you, landed here and then responded to the bot, lol. It's been that kind of day.

I see what you are saying. Technically, it solves some of the impacts of them not being separate configs, but does not actually create separate configs. Given the sheer number of options and not good documentation on which options will cause errors when specified for routes with the wrong method, perhaps separating them is the best thing to do.

However, that would require altering the applyMiddleware param contents, which would be a breaking change, unless we leave "route" alone and add "routeGet" and "routePost" options for each route definition that is generated internally and use route as a shared options variable (e.g. copy anything in "route" into both)?

Happy to adjust, if that's what you want to do. Seems like the most reasonable approach to not breaking things, but supporting this new configuration separation...

[arimus]

@glasser I've checked in some new changes which should fully fix the aforementioned ticket, with some better tests to verify them to boot. I did have to re-factor the createServer() method to return the Hapi server instance that gets created as well, so that I could test the routes after it was set up. Also adjusted the docs.

Let me know if you think we need any other adjustments.

[arimus]

Understood and makes sense. I'll address the issues you noted soon. Thanks for the review...not sure how I missed a couple of those.

On Thu, Jan 20, 2022, 9:56 PM David Glasser @.***> wrote:

@.**** requested changes on this pull request.

Sorry for the delay. As you might guess, the core maintainer team does not actually consist of people who are experts in all of different web frameworks that Apollo Server connects to; none of us ever use Hapi other than the bare minimum required to evaluate PRs like this one and we tend to forget everything about it in between PRs. The good news is that we are about to post our Apollo Server 4 roadmap, which will change Apollo Server to having a stable API for integrations with web frameworks and will allow packages like apollo-server-hapi to be maintained by a community of people who actually use Hapi rather than by the core team.

In docs/source/integrations/middleware.mdx https://github.com/apollographql/apollo-server/pull/5924#discussion_r789357529 :

@@ -376,6 +376,21 @@ async function startApolloServer(typeDefs, resolvers) {

 typeDefs,

 resolvers,

 plugins: [ApolloServerPluginStopHapiServer({ hapiServer: app })],

• // shared route options

This doesn't look right — these parameters are to applyMiddleware, not the constructor, right?

In packages/apollo-server-hapi/src/ApolloServer.ts https://github.com/apollographql/apollo-server/pull/5924#discussion_r789358040 :

•    const err = new Boom.Boom(error.message, {
  

•      statusCode: error.statusCode,
  

•    });
  

•    if (error.headers) {
  

•      Object.entries(error.headers).forEach(([headerName, value]) => {
  

•        err.output.headers[headerName] = value;
  

•      });
  

•    }
  

•    // Boom hides the error when status code is 500
  

•    err.output.payload.message = error.message;
  

•    throw err;
  

•  }
  

• };

• // POST ROUTE

• let postOptions = Object.assign({}, routePost);

These can be consts rather than lets, no?

In packages/apollo-server-hapi/src/ApolloServer.ts https://github.com/apollographql/apollo-server/pull/5924#discussion_r789358371 :

•      statusCode: error.statusCode,
  

•    });
  

•    if (error.headers) {
  

•      Object.entries(error.headers).forEach(([headerName, value]) => {
  

•        err.output.headers[headerName] = value;
  

•      });
  

•    }
  

•    // Boom hides the error when status code is 500
  

•    err.output.payload.message = error.message;
  

•    throw err;
  

•  }
  

• };

• // POST ROUTE

• let postOptions = Object.assign({}, routePost);

• postOptions = Object.assign(postOptions, route);

Doesn't this mean that things on route will take precedence over things on routePost if there are conflicts? Seems a bit backwards.

Also it's a bit easier to write {...routePost, ...route} (or the reverse if I'm correct) rather than use Object.assign these days.

In packages/apollo-server-hapi/src/ApolloServer.ts https://github.com/apollographql/apollo-server/pull/5924#discussion_r789358478 :

•        err.output.headers[headerName] = value;
  

•      });
  

•    }
  

•    // Boom hides the error when status code is 500
  

•    err.output.payload.message = error.message;
  

•    throw err;
  

•  }
  

• };

• // POST ROUTE

• let postOptions = Object.assign({}, routePost);

• postOptions = Object.assign(postOptions, route);

• // if we have post route options which contain no cors options, but have

• // specified cors options separately, then merge them in

• if (!!postOptions && !!cors && postOptions.cors == null) {

!!postOptions is always true here.

In docs/source/integrations/middleware.mdx https://github.com/apollographql/apollo-server/pull/5924#discussion_r789358833 :

@@ -396,6 +411,10 @@ You must await server.start() before calling server.applyMiddleware. You c

• onHealthCheck

• disableHealthCheck

+applyMiddleware also accepts the following options:

+* routeGet - get specific route options

in the description here, capitalize GET and POST — so it's clear that we're talking about the methods rather than saying that this is how you get route options. Or perhaps "route options for GET requests"?

— Reply to this email directly, view it on GitHub https://github.com/apollographql/apollo-server/pull/5924#pullrequestreview-859105423, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXYKQNRP2367LOFWDOASDUXDQ3PANCNFSM5I47D4FA . You are receiving this because you were mentioned.Message ID: @.***>

[glasser]

Thanks. Also don't forget to rebase when you do so (I think the conflicts are just spelling fixes.)

[codesandbox-ci[bot]]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 93fe678c800a4c7eefe6751cb83e12dec5cd8c4e:

Sandbox	Source
Apollo Server Typescript	Configuration
Apollo Server	Configuration

[arimus]

@glasser Sorry for the delay. Got sick and then slammed on projects. Let me know if you would like any other changes

[netlify[bot]]

✅ Deploy Preview for apollo-server-docs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	93fe678c800a4c7eefe6751cb83e12dec5cd8c4e
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-server-docs/deploys/62b781082437a80008ace327
😎 Deploy Preview	https://deploy-preview-5924--apollo-server-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[glasser]

Sorry for the huge delay once again. We're about to release Apollo Server 4, which has a much simpler HTTP integration API and won't have a core-maintained Hapi integration. It looks like you're hoping to find the time to build out that integration, or perhaps another community member will. Because we're about to stop actively maintaining apollo-server-hapi (and let's be honest, it's not like we have been particularly exceptional at efficiently maintaining the Hapi-specific parts of that package, which is why we're excited to turn over this responsibility to actual Hapi users) I don't think we'll be evaluating and merging this PR.

[arimus]

Fair enough. I'll hobble along on my custom build until we get AS4 + HAPI integrations going.