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

Validate variables at the gateway level #3213

Merged
merged 5 commits into from
Aug 27, 2019

Conversation

trevor-scheer
Copy link
Member

Currently, the gateway depends on downstream services to report any
errors with graphql execution. In the case of invalid variables, it's
not necessary to make any fetches or even build a query plan since
we can perform that validation when the request is first received.

This also guarantees that we surface invalid queries to users,
independent of how the downstream services are implemented and what
kind of validation they perform on incoming requests as seen in this relevant issue: #3153

Fixes #3153

Currently, the gateway depends on downstream services to report any
errors with graphql execution. In the case of invalid variables, it's
not necessary to make any fetches or even build a query plan since
we can perform that validation when the request is first received.

This also guarantees that we surface invalid queries to users,
independent of how the downstream services are implemented and what
kind of validation _they_ perform on incoming requests.
@@ -132,6 +134,11 @@ export type UpdateServiceDefinitions = (

type Await<T> = T extends Promise<infer U> ? U : T;

type RequestContext<TContext> = WithRequired<
GraphQLRequestContext<TContext>,
'document' | 'queryHash'
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed operation from the WithRequired pick list as it's unused.

return { errors: validationErrors };
}

let queryPlan: QueryPlan | undefined;
Copy link
Member Author

Choose a reason for hiding this comment

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

Added types to this since it was previously inferred as any


if (!variableDefinitions) return [];

const { errors } = getVariableValues(
Copy link
Contributor

Choose a reason for hiding this comment

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

iirc, we get the variable values elsewhere in the query planner? I wonder if we can reuse this lookup and prevent having to do it twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you're referring to this section where we determine the variables needed for a particular Fetch node: https://github.com/apollographql/apollo-server/blob/master/packages/apollo-gateway/src/executeQueryPlan.ts#L214-L225

I don't see any other references to getVariableValues() within QP (or much interaction with our variables object at all, aside from ^ and our second graphql execution pass).

@trevor-scheer trevor-scheer merged commit e151191 into master Aug 27, 2019
@trevor-scheer trevor-scheer deleted the trevor/AS-3153-gateway-variable-validation branch August 27, 2019 22:49
@abernix abernix modified the milestones: Release 2.9.2, Release 2.9.1 Aug 29, 2019
abernix pushed a commit to apollographql/federation that referenced this pull request Sep 4, 2020
…3213)

Previously, the gateway depended on downstream services to report any
errors with graphql execution. In the case of invalid variables, it's
not necessary to make any fetches or even build a query plan since
we can perform that validation when the request is first received.

Validating at the gateway also guarantees that we surface invalid queries to users,
independent of how the downstream services are implemented and what
kind of validation _they_ perform on incoming requests.

Apollo-Orig-Commit-AS: apollographql/apollo-server@e151191
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 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.

Federation sometimes returns empty json with graphql-java
3 participants