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

Access context in rewriteHeaders #454

Merged

Conversation

alex-parra
Copy link
Contributor

Fix #431

On gateway setups, query context was not being exposed to rewriteHeaders.
This PR fixes the issue by passing the context to every call to serviceDefinition.sendRequest which then exposes it to rewriteHeaders as an additional parameter.

@@ -85,7 +85,8 @@ async function getRemoteSchemaDefinition (serviceConfig, initHeaders) {
}
`
}),
headers
headers,
context: {} // No context to pass at this stage
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there's no context, do we need to provide a fake one then or would leaving it undefined be a more sensible default than providing an empty object then?

@@ -0,0 +1,101 @@
'use strict'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe I'm missing it, but I don't see where this file tests that rewriteHeaders is actually called and, more importantly, that its return value is actually used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests to rewriteHeaders are at line 64 and 65

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Collaborator

@simoneb simoneb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, see inline comment.

docs/federation.md Outdated Show resolved Hide resolved
alex-parra and others added 2 commits April 9, 2021 09:44
Co-authored-by: Simone Busoli <simone.busoli@gmail.com>
@mcollina mcollina merged commit 9bde5b4 into mercurius-js:master Apr 9, 2021
@alex-parra alex-parra deleted the issue-431-context-in-rewriteHeaders branch April 9, 2021 10:58
@Santinell
Copy link

@alex-parra @mcollina
I thought that in rewriteHeaders I will have the same context as generated in context method of gateway.
Like this:

 const rewriteHeaders = (headers, context) => {
    // I expect that here I will have the same context as in method `context` followed below
    return { context: JSON.stringify(context) };
  };
  app.register(mercurius, {
    context: async req => {
      const ctx = await authByHeaders(req.headers);
      return ctx;
    },
    gateway: {
      pollingInterval: 3000,
      services: [
        { name: 'host', url: 'http://localhost:3001/', rewriteHeaders },
        { name: 'dict', url: 'http://localhost:3002/', rewriteHeaders }
      ]
    }
  });

My use-case is auth on gateway side and sending context to services via rewriteHeaders

For now I have to implement auth on all services, even which don't use database for its methods. It's a huge overhead in packages and runtime, when one query using 3-5 services and every of them should auth.

Is it possible to get data from context method to rewriteHeaders?
Converting method rewriteHeaders to asynchronous is also can make it working.

@mcollina
Copy link
Collaborator

I think it should be the same context. Please open a new issue with a full reproducible example. Thanks.

@Santinell
Copy link

Santinell commented Apr 28, 2021

I am sorry, problem was because sometimes context is undefined. I haven't expected such value and it led to error on request its fields.
When I made it empty object by default this problem was resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access context in rewriteHeaders
4 participants