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

Call formatError when passed in options to runHttpQuery, fixes #1439 #1662

Closed

Conversation

tgriesser
Copy link
Contributor

@tgriesser tgriesser commented Sep 13, 2018

Another pass at #1660

The formatError option is being ignored if it's added in the options outside of the ApolloServer constructor (in runHttpQuery). This is fairly common use in contexts like lambda, and seems like a pretty big regression. This PR moves the code into doRunQuery and unshifts onto the extension stack, so it runs before the engine reporting agent, as the comments suggest it should.

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@ghost ghost added the 🪲 bug label Sep 13, 2018
@abernix abernix self-assigned this Sep 20, 2018
Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

I think this fix might end up being a short-term solution to this problem since @martijnwalraven is working on a refactor of the existing pipelines (of which there are more than one) in an effort to unify them and to avoid exactly this type of oversight in the future.

So, LGTM, assuming @martijnwalraven agrees that this aligns with that refactor.

@abernix
Copy link
Member

abernix commented Nov 16, 2018

Sorry for the delay on this.

The doRunQuery function no longer exists and has been rolled up into the new request pipeline (see #1795). We don't recommend calling runHttpQuery directly which, unless you explicitly passed your own extension which implemented behavior which reacted to formatError as well, would still exhibit this same behavior. That said, I agree that it's weird that runHttpQuery still seems to accept formatError and does nothing with it, but I also believe it to be an internal API (and always has been, as far as I'm aware). We'll should still try to fix this, but it's not a high priority, and I'd still not recommend using it.

Luckily, your concern should be addressed — even on Lambda — by constructing an instance of ApolloServer (per the documentation) and, in the case of Lambda, using the result of that instantiated ApolloServer's createHandler method:

const { ApolloServer, gql } = require("apollo-server-lambda");

const typeDefs = gql`
  type Query {
    hello: String
  }
`;

const resolvers = {
  Query: {
    hello: (parent, args, context) => {
      return "Hello, world!";
    }
  }
};

const server = new ApolloServer({
  typeDefs,
  resolvers,
  formatError(err) {
    err.message = "Manipulated";
    return err;
  },
});

exports.handler = server.createHandler();

I suspect this should work properly for you. If you're still struggling with it, please do ping for a re-open!

@abernix abernix closed this Nov 16, 2018
@abernix
Copy link
Member

abernix commented Nov 16, 2018

I should add, I'm very grateful for you taking the time to look at this — twice!

@tgriesser
Copy link
Contributor Author

@abernix I'm mainly looking to be able to access the request's context within formatError, is this possible with the approach you mentioned?

@tgriesser tgriesser deleted the extensions-formatError branch November 16, 2018 18:53
@gajus
Copy link

gajus commented Mar 2, 2019

Was this ever released?

As far as I can tell, formatError is never called.

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

Successfully merging this pull request may close these issues.

3 participants