-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Changes from all commits
7f6e77b
572f498
f5a3cbe
19b5ff8
99a7d5f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
import gql from 'graphql-tag'; | ||
import { ApolloGateway } from '../../'; | ||
|
||
import * as accounts from '../__fixtures__/schemas/accounts'; | ||
import * as books from '../__fixtures__/schemas/books'; | ||
import * as inventory from '../__fixtures__/schemas/inventory'; | ||
import * as product from '../__fixtures__/schemas/product'; | ||
import * as reviews from '../__fixtures__/schemas/reviews'; | ||
|
||
describe('ApolloGateway executor', () => { | ||
it('validates requests prior to execution', async () => { | ||
const gateway = new ApolloGateway({ | ||
localServiceList: [accounts, books, inventory, product, reviews], | ||
}); | ||
|
||
const { executor } = await gateway.load(); | ||
|
||
const { errors } = await executor({ | ||
document: gql` | ||
query InvalidVariables($first: Int!) { | ||
topReviews(first: $first) { | ||
body | ||
} | ||
} | ||
`, | ||
request: { | ||
variables: { first: '3' }, | ||
}, | ||
queryHash: 'hashed', | ||
context: null, | ||
cache: {} as any, | ||
}); | ||
|
||
expect(errors![0].message).toMatch( | ||
'Variable "$first" got invalid value "3"; Expected type Int.', | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ import { | |
isIntrospectionType, | ||
GraphQLSchema, | ||
GraphQLError, | ||
VariableDefinitionNode, | ||
} from 'graphql'; | ||
import { GraphQLSchemaValidationError } from 'apollo-graphql'; | ||
import { composeAndValidate, ServiceDefinition } from '@apollo/federation'; | ||
|
@@ -38,6 +39,7 @@ import { serializeQueryPlan, QueryPlan, OperationContext } from './QueryPlan'; | |
import { GraphQLDataSource } from './datasources/types'; | ||
import { RemoteGraphQLDataSource } from './datasources/RemoteGraphQLDataSource'; | ||
import { HeadersInit } from 'node-fetch'; | ||
import { getVariableValues } from 'graphql/execution/values'; | ||
|
||
export type ServiceEndpointDefinition = Pick<ServiceDefinition, 'name' | 'url'>; | ||
|
||
|
@@ -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' | ||
>; | ||
|
||
export class ApolloGateway implements GraphQLService { | ||
public schema?: GraphQLSchema; | ||
protected serviceMap: ServiceMap = Object.create(null); | ||
|
@@ -439,10 +446,7 @@ export class ApolloGateway implements GraphQLService { | |
// are unlikely to show up as GraphQLErrors. Do we need to use | ||
// formatApolloErrors or something? | ||
public executor = async <TContext>( | ||
requestContext: WithRequired< | ||
GraphQLRequestContext<TContext>, | ||
'document' | 'operation' | 'queryHash' | ||
>, | ||
requestContext: RequestContext<TContext>, | ||
): Promise<GraphQLExecutionResult> => { | ||
const { request, document, queryHash } = requestContext; | ||
const queryPlanStoreKey = queryHash + (request.operationName || ''); | ||
|
@@ -451,7 +455,19 @@ export class ApolloGateway implements GraphQLService { | |
document, | ||
request.operationName, | ||
); | ||
let queryPlan; | ||
|
||
// No need to build a query plan if we know the request is invalid beforehand | ||
// In the future, this should be controlled by the requestPipeline | ||
const validationErrors = this.validateIncomingRequest( | ||
trevor-scheer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
requestContext, | ||
operationContext, | ||
); | ||
|
||
if (validationErrors.length > 0) { | ||
return { errors: validationErrors }; | ||
} | ||
|
||
let queryPlan: QueryPlan | undefined; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added types to this since it was previously inferred as |
||
if (this.queryPlanStore) { | ||
queryPlan = await this.queryPlanStore.get(queryPlanStoreKey); | ||
} | ||
|
@@ -510,6 +526,25 @@ export class ApolloGateway implements GraphQLService { | |
return response; | ||
}; | ||
|
||
protected validateIncomingRequest<TContext>( | ||
requestContext: RequestContext<TContext>, | ||
operationContext: OperationContext, | ||
) { | ||
// casting out of `readonly` | ||
const variableDefinitions = operationContext.operation | ||
trevor-scheer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.variableDefinitions as VariableDefinitionNode[] | undefined; | ||
|
||
if (!variableDefinitions) return []; | ||
|
||
const { errors } = getVariableValues( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I don't see any other references to |
||
operationContext.schema, | ||
variableDefinitions, | ||
requestContext.request.variables!, | ||
); | ||
|
||
return errors || []; | ||
} | ||
|
||
private initializeQueryPlanStore(): void { | ||
this.queryPlanStore = new InMemoryLRUCache<QueryPlan>({ | ||
// Create ~about~ a 30MiB InMemoryLRUCache. This is less than precise | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
operation
from theWithRequired
pick list as it's unused.