generateSchemaHash doesn't await execution result

[wmertens]

There's a problem here:

apollo-server/packages/apollo-server-core/src/utils/schemaHash.ts

Line 11 in 6bd73b1

const result = execute(schema, documentAST) as ExecutionResult;

in graphql-js, this can return a Promise:
https://github.com/graphql/graphql-js/blob/f529809f5408fb9a343e669ea4d4851add3df004/src/execution/execute.js#L141-L144

So generateSchemaHash has to always return a Promise, too.

This probably causes all the graphql-middleware issues, like #1934

[wmertens]

I can confirm that by awaiting the execution result, and doing a delayed storing of the result like this works:

apollo-server-core/dist/utils/schemaHash.js line 15:

async function generateSchemaHash(schema) {
	const introspectionQuery = utilities_1.getIntrospectionQuery()
	const documentAST = language_1.parse(introspectionQuery)
	const result = await execution_1.execute(schema, documentAST)

apollo-server-core/dist/ApolloServer.js: line 226

		this.schemaHash = ''
		schemaHash_1
			.generateSchemaHash(this.schema)
			.then(
				hash => (this.schemaHash = hash),
				err => console.error('Warning: Could not generated schema hash', err)
			)

[townmulti]

TypeError: utilities_1.getIntrospectionQuery is not a function

...is the error I'm getting.

[wmertens]

@martijnwalraven This is not fixed in 2.2.1, right?

[melkir]

The problem is still present in 2.2.1

[martijnwalraven]

Hmmm, this is surprising, because executing an introspection query is normally guaranteed to be synchronous. If graphql-middleware changes this to return an asynchronous result, that seems like a design issue with that library to me. But it would probably make sense to put a workaround in place to avoid errors.

[wmertens]

The workaround I describe above worked fine for me

[martijnwalraven]

Yep, I'm not saying that workaround doesn't work, but it's unfortunate it's required because introspection is meant to be synchronous.

[abernix]

Just to be clear, this problem only manifests itself if when graphql-middleware is used. The current contract for schemaHash necessitates that it be set synchronously. The work-around above fails to meet the requirements of this contract and provides no guarantee that it will be set by the time it's needed. For example, it will not be resolved by its use on this line:

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

Line 317 in 26d6c73

schemaHash: this.schemaHash,

I'm happy to revisit this in the future but, as I've indicated in the commit message for #1955's 45bdcd9, the more correct solution here would be to maintain the synchronicity of introspection query execution by changing graphql-middleware to maintain the execution dynamics of the resolvers it is wrapping. I've made the error more clear via #1955, but I'm not currently willing to believe that an asynchronous introspection query is desired behavior and by working around this, we'd be encouraging that behavior.

[wmertens]

@abernix I'm encountering this too and I don't use graphql-middleware, I don't even have it in my node_modules.

[wmertens]

Furthermore, this behavior breaks existing code with a minor release :( The only thing this adds is the schemaHash which is not crucial for operation.

[abernix]

Ok, thanks for reporting back @wmertens! Then something else is breaking that contract then! I hadn't been able to reproduce this without graphql-middleware, but I'm willing to dig into it and figure it out!

[wmertens]

I do make my own schema using the objects, and I wrap the resolvers, but I can't think of any place where I asynchronously define the types

[abernix]

@wmertens If you have the ability to try to figure out a minimum reproduction, I'd really appreciate it!

[wmertens]

@abernix no real time to spend on this in the coming days :( Do you happen to know how the executor decides that the result should be a promise?

[abernix]

@wmertens Sure! It occurs within graphql/execution's executeFields via execute. Basically, if a field resolves with a Promise (which I really believe should not happen in an introspection query), then the entire execution is Promise-ified.

[abernix]

@wmertens Can you share a brief example of how you wrap the resolvers?

[wmertens]

@abernix see https://gist.github.com/wmertens/c291e074c74b88a482b2c61abe5b9947

[nicklaros]

confirmed this issue after update apollo-server in one of my project today

[wmertens]

Still, since this is a minor release, and it breaks previously working code, can't there be a deprecation and making it throw in the next major release?

[ascott1]

I think that this has been implied, but not stated explicitly. It appears that this also means that the Middleware examples found in the README and documentation (Express, etc.) throw an error when a user attempts to run the given example:

.../node_modules/apollo-server-core/dist/utils/schemaHash.js:12
    const introspectionQuery = utilities_1.getIntrospectionQuery();
                                           ^

TypeError: utilities_1.getIntrospectionQuery is not a function

[abernix]

@ascott1 I'm not finding that to be true. This reproduction of the example you linked to above doesn't seem to be affected. Are you sure you're not wrapping otherwise synchronous resolvers with Promises?

@wmertens We're working on addressing the problem you're encountering, but I'm not sure what you're suggesting be deprecated.

[ascott1]

@abernix Running that README example was where I originally ran into the error. I am now unable to replicate it, so it may have been user error on my end.

[abernix]

@wmertens Just to be clear, we haven't done anything to change a schema to return a Promise and we aren't deprecating (or suggesting that Promises be deprecated or disallowed). Operations will resolve with a Promise thanks to this existing (unchanged) behavior in graphql. If an operation implements asynchronous behavior, then the entire operation will return return a Promise.

In the case of an introspection query, it has — by design — no asynchronous behavior. This is corroborated by graphql/graphql-js#1120 in graphql-js. If the introspection query is changing to become asynchronous, it's because something is wrapping types which are otherwise supposed to be (by expectation) synchronous. This is not an execution dynamic which Apollo Server changes, so it must be happening in user-land (or via user-land plugins, like graphql-middleware). We also maintain that it shouldn't happen for introspection types — something which could be prevented by checking the type against graphql/introspection's isIntrospectionType predicate function prior to wrapping.

Looking at the code you've provided in #1935 (comment), I don't see anything problematic. However, the Gist you linked to at https://gist.github.com/wmertens/c291e074c74b88a482b2c61abe5b9947#file-makeschema-js-L44-L67 could definitely be problematic if you used (your) instrument function against any of the GraphQL introspection types (again, those listed here) since it would change their behavior from synchronous to asynchronous.

[wmertens]

@abernix Hmm, I rewrote everything so that it maintains resolver async status, but the error is still present. I'm not using any other wrappers AFAIK.

What I mean by deprecating is that, until 2.0.5, people could have incorrect schemas that worked, and by upgrading a minor Apollo version, their code will break. You could make the schemahash async until the next major version and in the meantime deprecate those incorrect async schemas.

[wmertens]

🤷‍♂️ very odd, I tracked it down to kadirahq/graphql-errors#18 even though I had disabled it before. Anyway, I got 2.2.0 to work now.

@abernix I guess this issue is a "wontfix" now, unless you go the deprecation route? Feel free to close.

[BrendanBall]

What is the deal with this? The following basic example even has this problem:
"apollo-server": "^2.3.1"

import { ApolloServer, gql } from 'apollo-server'

const typeDefs = gql`
  type Query {
    hello: String
  }
`
const resolvers = {
  Query: {
    hello: () => 'world'
  }
}

const server = new ApolloServer({ typeDefs, resolvers })

server.listen(3000, () => console.log('running graphql server on port 3000'))

So apollo is completely broken in the latest version?

[abernix]

@BrendanBall The reproduction you've included — if it is failing — seems to be missing clarity on some other attributes which are contributing to your particular failure because it reproduces fine using the above code, as seen here: https://glitch.com/edit/#!/joyous-pressure

I will close this for now because, as stated above, Apollo Server seems to be in compliance with the expectations of the GraphQL reference implementation and test-suite: that is, the introspection query execution should operate in a synchronous behavior.

If you continue to experience this issue, please open a new issue with a full reproduction (though keep in mind if it's still using graphql-middleware, we'll probably end up back over at maticzav/graphql-middleware#75).

[stringbeans]

this might be useful for someone, but i was struggling this for a while and then realized that graphql was not installed as a dependency and that was causing the error

[danielalves]

Maybe this will help someone. I got to this thread because I was getting the following error:

UnhandledPromiseRejectionWarning: TypeError: utilities_1.introspectionQuery is not a function

which I tracked down to apollo-server-cores's schemaHash.ts file. The transpiled version of the file looks like that:

// ...
const utilities_1 = require("graphql/utilities");
const json_stable_stringify_1 = __importDefault(require("json-stable-stringify"));
const crypto_1 = require("crypto");
function generateSchemaHash(schema) {
    const introspectionQuery = utilities_1.getIntrospectionQuery();
    // ...

But the thing is getIntrospectionQuery does not exist. What exists is a getter method called introspectionQuery. So updating the file to

const utilities_1 = require("graphql/utilities");
const json_stable_stringify_1 = __importDefault(require("json-stable-stringify"));
const crypto_1 = require("crypto");
function generateSchemaHash(schema) {
    const introspectionQuery = utilities_1.introspectionQuery;
    // ...

got me rid of the first error, but then I started to get

UnhandledPromiseRejectionWarning: Error: The introspection query is resolving asynchronously; execution of an introspection query is not expected to return a `Promise`.

which I'm still unable to fix.

[eric-burel]

Hi, just for documentation, we can repro this issue on a Meteor app. Still unsure why or what we are expected to fix or change though even after reading this thread. We do not use graphql-middleware.

Edit: if you need reproduction simply install Vulcan apollo2 branch (it's a Meteor app as usual) and run npm run test (it's not a minimal example though): https://github.com/VulcanJS/Vulcan/tree/apollo2

[eric-burel]

Hi, I've updated my test. I can still reproduce this issue with a minimal schema (the example schema defined here in the official doc).

If someone wants to take a look, the server is defined here: https://github.com/VulcanJS/Vulcan/tree/apollo2/packages/vulcan-lib/lib/server/apollo-server

It only happens in test environment though, the server works fine otherwise.

[wmertens]

@eric-burel It's probably not this one, but you have to look for lines like https://github.com/VulcanJS/Vulcan/blob/b2e3aaaeb54a266a060c539f45fbf0f9fd208e96/packages/vulcan-lib/lib/server/intl.js#L49 where it's wrapping a resolver with an async version without checking if the resolver returns a promise…

[abernix]

@eric-burel The assessment and identification of the issue by @wmertens here is correct. (Thanks, @wmertens!)

The VulcanJS resolver wrapper should maintain the existing execution synchronicity dynamics of the resolver its wrapping. One way to do this could be to check if the existing resolver is then-able (i.e. it has a then attribute which is a function), or by checking if the type is one of the introspection types identified by graphql-js's isIntrospectionType, as graphql-middleware has done here.

I would suggest submitting a PR to the Vulcan project which adds such a guard.

[eric-burel]

@wmertens Thanks a lot for pointing me out to this example directly in the code, it makes things clearer to me. I still don't get why I get this issue on a minimal schema but it will help me to investigate this (and also avoid many potential problems).
@abernix Thanks a lot too for this suggestion!

[eric-burel]

Hi, I've tried again using a really minimal schema, I can still reproduce in Meteor test mode. Here is the example test:

    it('init any server', function () {
      const apolloServer = new ApolloServer({
        typeDefs: `
      type Author {
        id: Int!
      }

      type Query {
        author(id: Int!): Author
      }
    `});
    });

I don't get what happens honestly. First, why is an introspection query triggered? Is this supposed to happen when the server is first created? Then, why would it fail? The schema is really minimal, so this could only come from how Meteor works, maybe related to how it runs queries? I am running apollo-server-express 2.3.3, I think this is the last version.

Sorry to bother or if it is not the appropriate thread, it's just a bit unsettling. It happens only during test but I am worried that this could happen with no reason (or at least no reason I can grasp) in production too. I still try my best to solve this.

[eric-burel]

Also, @abernix you say that adding a isIntrospectionType would avoid this issue, which I understand, but you you also say that the wrapper should maintain the resolver synchronicity. I am not sure I get this second part correctly. What if I want to write a directive that makes sync fields async? E.g if I want to query a translation from a distant server, and apply it to the field, I transform a sync resolver in an async resolver.
I guess that when you say that we should maintain the underlying resolver synchronicity, you mean for introspection queries only?

[wmertens]

Async resolvers are fine, but field types must be able to be introspected synchronously. So any wrapper that walks the schema and wraps resolvers must maintain synchronicity Your configuration contains something that makes this fail. Wout.

…

On Tue, Jan 29, 2019, 6:44 PM Eric Burel ***@***.*** wrote: Also, @abernix <https://github.com/abernix> you say that adding a isIntrospectionType would avoid this issue, which I understand, but you you also say that the wrapper should maintain the resolver synchronicity. I am not sure I get this second part correctly. What if I want to write a directive that makes sync fields async? E.g if I want to query a translation from a distant server, and apply it to the field, I transform a sync resolver in an async resolver. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1935 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADWluIfpLBnC3Vzg0TqG5x22_w2zHruks5vIIiYgaJpZM4YUdg4> .

[abernix]

@eric-burel If you can provide a minimal reproduction, I can try to take a look. I wouldn't assume it's anything related to Meteor (as a maintainer on the Meteor project, I can't think of anything that would do this. ☄️ ).

First, why is an introspection query triggered?

Apollo Server introspects itself when it starts up. But that's just the symptom, not the cause. (And in this case, is a great smoke test which ensures that your introspection schema is behaving synchronously, as it is intended.)

What if I want to write a directive that makes sync fields async?

You can do that, so long as you don't make an introspection type (e.g. __schema) behave asynchronously. The logic to put eyes 👀 on would be anything which loops over all types in a schema (say, via getTypes) and wraps them all without checking if they are introspection types. Making fields which were sync become async is fine, so long as making that change doesn't conflict with the desires of the underlying graphql-js execution library.

[eric-burel]

Hi guys,

I finally figured out what happens.
We actually mixed up 2 different issues.

@ascott1 had a TypeError: utilities_1.getIntrospectionQuery is not a function error, which magically disappeared after an update. I got the exact same issue!
This may simply be related to how things were exported in graphql/utilities, getIntrospectionQuery was not exported somehow. I had a similar issue with apollo-server-express and the ApolloServer constructor, which disappeared afterward.
A google search pointed me to this issue, hence my confusion.

Thanks a lot for taking time to answer me though, this will definitely help me to avoid potential future issues with introspection!

[dev-reactjs]

const introspectionQuery = utilities_1.getIntrospectionQuery();
                                       ^

TypeError: utilities_1.getIntrospectionQuery is not a function

I am getting this error while using code from this link https://github.com/jaydenseric/apollo-upload-examples in different project and i am unable to correct this issue.
Please help ..

[eric-burel]

@dev-reactjs can you check your graphql version with npm list graphql? It should be the last version (14.1.1)

[OlivierJM]

I think It would be good to specifically mention that the graphql version should be updated to the latest version during an upgrade of apollo-server

[oorestisime]

Hit this up today after reworking on an old project. i confirm upgrading fixes the issue.

[joshduck]

This is what peer dependencies are for. Why is graphql not listed?

[abernix]

@joshduck graphql is listed within Apollo Server's peerDependencies with the appropriate range.

I'm going to lock this issue because I feel like multiple issues are starting to get conflated. We do recommend updating to newer versions of graphql, but we do actually try to maintain compatibility with the existing range. Please note that we will certainly be dropping support for versions of graphql before 14.x in Apollo Server 3.x though.

If anyone continues to experience problems, please do open a new issue with a runnable reproduction. Thanks!