Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Operation headers are not interpolated when context.req.headers is a HeaderMap #5226

Closed
3 of 4 tasks
pmrotule opened this issue Mar 16, 2023 · 10 comments
Closed
3 of 4 tasks

Comments

@pmrotule
Copy link

pmrotule commented Mar 16, 2023

Issue workflow progress

Progress of the issue based on the
Contributor Workflow

  • 1. The issue provides a reproduction available on Github
  • 2. A failing test has been provided
  • 3. A local solution has been provided
  • 4. A pull request is pending review

Describe the bug

Context

Since we are using GraphQL Mesh together with Apollo Server and Express, upgrading some of the mesh dependencies to the latest versions became a challenge because we couldn't find instructions for Express in your documentation like we used to. We finally dug a bit deeper and found the nextjs-apollo-example which made us understand how to upgrade using an executor in Apollo Server.

The issue

We managed to upgrade all our graphql-mesh dependencies and everything was working as expected besides one thing: the operationHeaders based on request headers were not interpolated because context.req.headers was not an object anymore, but a HeaderMap (you need to call headers.get('key')).

Screenshot 2023-03-15 at 14 21 22

Using "{context.req.headers.get('access-token')}" instead of "{context.req.headers['access-token']}" doesn't fix the issue because of the way graph-mesh is interpolating the string value: it only allows to access the property of an object as the last bit of the interpolated string (see here).

We were able to work around the issue (see workaround branch) by creating a new object with one property and access it right away:

"{({ x: context.req.headers.get('access-token') }).x}"

Nevertheless, the workaround is only working in our real GraphQL Mesh integration. It was also working in the issue reproduction repo at first with only one public GraphQL API, but once I made it a monorepo to add a local account service, the workaround stopped working and the interpolation was broken. It was returning x: context.req.headers.get('access-token') }).x} like it would consider the opening bracket of the object being the start of a string interpolation. We had the same issue when I tried to write integration tests using apollo-server-integration-testing.

To Reproduce

Find the steps to reproduce in the README of the issue reproduction repo:
https://github.com/pmrotule/graphql-mesh-headers-not-interpolated#issue-reproduction

Expected behavior

"{context.req.headers.get('access-token')}" would be interpolated as expected instead of returning an empty string.

Environment:

  • OS: MacOS Monterey 12.6.1
  • @graphql-mesh/string-interpolation: 0.4.2 (latest)
  • NodeJS: 18.14.0
  • express: 4.18.2 (latest)
  • @apollo/server: 4.5.0 (latest)

Suggested solution

See #5227 for details.

🚀 See it in action by checking out the fixed branch of the issue reproduction repo.

@ardatan
Copy link
Owner

ardatan commented Mar 16, 2023

Did you try headers directly?

@pmrotule
Copy link
Author

You mean context.headers instead of context.req.headers? Same issue unfortunately. It seems to be the same object:

image

@ardatan
Copy link
Owner

ardatan commented Mar 20, 2023

We don't recommend using Apollo Server with Mesh because Apollo Server doesn't allow us to hook into the different phases of GraphQL Execution then we cannot use all the features of Envelop which is the plugin system Mesh uses under the hood.
Mesh's default server uses GraphQL Yoga which we believe is a better alternative for GraphQL Mesh.
You can easily use the default server impl with Express
https://the-guild.dev/graphql/mesh/docs/getting-started/deploy-mesh-gateway#mesh-as-an-express-route

But if you still want to use Apollo Server with Mesh, you have to make sure request object is received properly in this line;
pmrotule/graphql-mesh-headers-not-interpolated@a831432#diff-a4c65ede64197e1a112899a68bf994485b889c4b143198bac4af53425b38406fR33

@pmrotule
Copy link
Author

@ardatan I think we went with Apollo Server because it was the natural fit with our frontend that uses the ApolloClient, but now we have quite a few Apollo Server plugins which makes it harder to switch to GraphQL Yoga.

But if you still want to use Apollo Server with Mesh, you have to make sure request object is received properly in this line;
pmrotule/graphql-mesh-headers-not-interpolated@a831432#diff-a4c65ede64197e1a112899a68bf994485b889c4b143198bac4af53425b38406fR33

What do you mean by that? I re-used the code you wrote in nextjs-apollo-example and when I add a debugger, requestContext.request.http is indeed a request object with headers being a Symbol map:

image

@ardatan
Copy link
Owner

ardatan commented Mar 21, 2023

Could you try the following?

const { schema, execute, contextFactory } = getEnveloped({
-   req: requestContext.request.http,
+   request: requestContext.request.http,
});

@pmrotule
Copy link
Author

I'm not sure I understand what we're trying to do here. The request object is passed as expected, the issue is just that the headers property of that object is a HeaderMap (or Symbole map) so we need to access the value of specific headers with .get('specific-header') which is not interpolated as expected by the string-interpolation package. No matter if I pass the request object as req or request, it would still refer to the same initial object requestContext.request.http with the HeaderMap.

I tried what you suggested, but I have the same result:

operationHeaders: {
  'access-token': "{context.request.headers.get('access-token')}",
},

image

@ardatan
Copy link
Owner

ardatan commented Mar 21, 2023

Mesh has this plugin that converts header maps to regular objects so you can access them like {headers['x-foo']}
https://github.com/Urigo/graphql-mesh/blob/master/packages/runtime/src/get-mesh.ts#L182
Notice I didn't add {req. or {request. at the beginning.

@pmrotule
Copy link
Author

Got it, thanks. I checked and this plugin is only running on this url https://unknown-url.invalid/, it doesn't hit the debugger when I send graphql queries.

image

Nevertheless, the headers property is still not converted to a regular object because getHeadersObj doesn't convert it if there is no 'forEach' in headers and a HeaderMap doesn't have a forEach method.

https://github.com/Urigo/graphql-mesh/blob/master/packages/utils/src/getHeadersObj.ts#L10

image

@ardatan
Copy link
Owner

ardatan commented Mar 21, 2023

Hmm it seems Apollo doesn't use the standard Headers. But if it implements a Map interface, we can support it actually. I'll create a PR for this change.

@ardatan
Copy link
Owner

ardatan commented Mar 21, 2023

#5240
After this release, it should work with { headers. . I added tests for Apollo's header implementation together with the regular Headers.

@ardatan ardatan closed this as completed Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants