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

core: return PersistedQueryNotSupported for Apollo Persisted Queries #982

Merged
merged 1 commit into from
Apr 24, 2018

Conversation

glasser
Copy link
Member

@glasser glasser commented Apr 23, 2018

Apollo Persisted Queries is a standard for sending queries as short hashes
instead of full strings, designed to work well with GET requests. It is
implemented by servers including the Apollo Engine Proxy, and by the
apollo-link-persisted-query client.

A common configuration is to set up persisted queries on production servers but
not in development. It is still convenient to leave apollo-link-persisted-query
always on, though. While apollo-link-persisted-query can detect that servers
don't support PQs, it works better if the server actually says it doesn't
support the PQ, instead of trying to process a request without a query and
potentially printing a confusing stack trace. This commit makes apollo-server
directly return PersistedQueryNotSupported instead of allowing confusing stack
traces to occur.

Apollo Persisted Queries is a standard for sending queries as short hashes
instead of full strings, designed to work well with GET requests. It is
implemented by servers including the Apollo Engine Proxy, and by the
apollo-link-persisted-query client.

A common configuration is to set up persisted queries on production servers but
not in development. It is still convenient to leave apollo-link-persisted-query
always on, though. While apollo-link-persisted-query can detect that servers
don't support PQs, it works better if the server actually says it doesn't
support the PQ, instead of trying to process a request without a query and
potentially printing a confusing stack trace.  This commit makes apollo-server
directly return PersistedQueryNotSupported instead of allowing confusing stack
traces to occur.
@glasser glasser requested a review from jbaxleyiii April 23, 2018 23:02
@jbaxleyiii jbaxleyiii merged commit e2df79d into master Apr 24, 2018
@jbaxleyiii jbaxleyiii deleted the glasser/persistedquerynotsupported branch April 24, 2018 10:29
@glasser
Copy link
Member Author

glasser commented Apr 24, 2018

Thanks!

In the future I prefer to merge my own PRs on repos where I have permissions, as l'esprit de l'escalier often strikes me. (I was at least intending to add a CHANGELOG note here.)

@glasser
Copy link
Member Author

glasser commented Apr 24, 2018

This has been published as apollo-server-* 1.3.6.

@billnbell
Copy link

billnbell commented Apr 25, 2018

OK When calling to get a persisted query I get an error when Graphql does not have Engine defined.

1:

http://localhost:4001/graphql?query={__typename}&extensions={%22persistedQuery%22:{%22version%22:1,%22sha256Hash%22:%22ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b38%22}}

Then:

http://localhost:4001/graphql?extensions={%22persistedQuery%22:{%22version%22:1,%22sha256Hash%22:%22ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b38%22}}

{
errors: [
{
message: "Cannot read property 'definitions' of undefined"
}
]
}

Stack dump is:

graphql_1 | TypeError: Cannot read property 'definitions' of undefined
graphql_1 | at Object.getOperationAST (/usr/src/app/node_modules/graphql/utilities/getOperationAST.js:17:35)
graphql_1 | at isQueryOperation (/usr/src/app/node_modules/apollo-server-core/src/runHttpQuery.ts:26:24)
graphql_1 | at /usr/src/app/node_modules/apollo-server-core/src/runHttpQuery.ts:86:16
graphql_1 | at Array.map ()
graphql_1 | at Object. (/usr/src/app/node_modules/apollo-server-core/src/runHttpQuery.ts:77:59)
graphql_1 | at step (/usr/src/app/node_modules/apollo-server-core/dist/runHttpQuery.js:42:23)
graphql_1 | at Object.next (/usr/src/app/node_modules/apollo-server-core/dist/runHttpQuery.js:23:53)
graphql_1 | at fulfilled (/usr/src/app/node_modules/apollo-server-core/dist/runHttpQuery.js:14:58)
graphql_1 | at propagateAslWrapper (/usr/src/app/node_modules/async-listener/index.js:478:23)
graphql_1 | at /usr/src/app/node_modules/async-listener/glue.js:188:31

It should probably say:

message: "Graphql Apollo Engine APQ is not enabled"

@glasser
Copy link
Member Author

glasser commented Apr 25, 2018

You are definitely running [email protected]?

@billnbell
Copy link

I am running

"apollo-engine": "^1.1.0",
"apollo-server-express": "^1.3.6",

@glasser
Copy link
Member Author

glasser commented Apr 27, 2018

But what version of apollo-server-core (according to, say, npm ls | grep apollo-server-core)?

@billnbell
Copy link

npm ls | grep apollo-server-core
│ ├─┬ [email protected]

@glasser
Copy link
Member Author

glasser commented Apr 27, 2018

That's strange — this PR really should fix that. Can you share a small reproduction git repo that shows this happening?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants