From 7ef423a4b8396f24eb78365bc8a2f7e8ca784d4d Mon Sep 17 00:00:00 2001 From: damour Date: Mon, 19 Feb 2018 14:49:57 +0200 Subject: [PATCH 1/2] Don't validate if query is already an AST. --- CHANGELOG.md | 2 + .../apollo-server-core/src/runHttpQuery.ts | 4 +- packages/apollo-server-core/src/runQuery.ts | 25 ++++++------ .../src/apolloServerHttp.test.ts | 38 +++++++++++++++++++ 4 files changed, 57 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e2b71896b15..3926387b280 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ All of the packages in the `apollo-server` repo are released with the same versi ### vNEXT +* Don't validate if query is already an AST. [PR #839](https://github.com/apollographql/apollo-server/pull/839) + ### v1.3.6 * Recognize requests with Apollo Persisted Queries and return `PersistedQueryNotSupported` to the client instead of a confusing error. [PR #982](https://github.com/apollographql/apollo-server/pull/982) diff --git a/packages/apollo-server-core/src/runHttpQuery.ts b/packages/apollo-server-core/src/runHttpQuery.ts index f6eb87c9d6e..00fa9e0f4ae 100644 --- a/packages/apollo-server-core/src/runHttpQuery.ts +++ b/packages/apollo-server-core/src/runHttpQuery.ts @@ -102,6 +102,7 @@ export async function runHttpQuery( try { let query = requestParams.query; let extensions = requestParams.extensions; + const isQueryString = typeof query === 'string'; if (isGetRequest && extensions) { // For GET requests, we have to JSON-parse extensions. (For POST @@ -138,7 +139,7 @@ export async function runHttpQuery( } if (isGetRequest) { - if (typeof query === 'string') { + if (isQueryString) { // preparse the query incase of GET so we can assert the operation. // XXX This makes the type of 'query' in this function confused // which has led to us accidentally supporting GraphQL AST over @@ -193,6 +194,7 @@ export async function runHttpQuery( query: query, variables: variables, context, + isQueryString, rootValue: optionsObject.rootValue, operationName: operationName, logFunction: optionsObject.logFunction, diff --git a/packages/apollo-server-core/src/runQuery.ts b/packages/apollo-server-core/src/runQuery.ts index c99e94d1b38..672237a9931 100644 --- a/packages/apollo-server-core/src/runQuery.ts +++ b/packages/apollo-server-core/src/runQuery.ts @@ -72,6 +72,7 @@ export interface QueryOptions { debug?: boolean; tracing?: boolean; cacheControl?: boolean | CacheControlExtensionOptions; + isQueryString?: boolean; } export function runQuery(options: QueryOptions): Promise { @@ -171,17 +172,19 @@ function doRunQuery(options: QueryOptions): Promise { documentAST = options.query as DocumentNode; } - let rules = specifiedRules; - if (options.validationRules) { - rules = rules.concat(options.validationRules); - } - logFunction({ action: LogAction.validation, step: LogStep.start }); - const validationErrors = validate(options.schema, documentAST, rules); - logFunction({ action: LogAction.validation, step: LogStep.end }); - if (validationErrors.length) { - return Promise.resolve({ - errors: format(validationErrors, options.formatError), - }); + if (options.isQueryString !== false) { + let rules = specifiedRules; + if (options.validationRules) { + rules = rules.concat(options.validationRules); + } + logFunction({ action: LogAction.validation, step: LogStep.start }); + const validationErrors = validate(options.schema, documentAST, rules); + logFunction({ action: LogAction.validation, step: LogStep.end }); + if (validationErrors.length) { + return Promise.resolve({ + errors: format(validationErrors, options.formatError), + }); + } } if (extensionStack) { diff --git a/packages/apollo-server-express/src/apolloServerHttp.test.ts b/packages/apollo-server-express/src/apolloServerHttp.test.ts index 1255a388249..407dc35b5c7 100644 --- a/packages/apollo-server-express/src/apolloServerHttp.test.ts +++ b/packages/apollo-server-express/src/apolloServerHttp.test.ts @@ -37,7 +37,9 @@ import { GraphQLScalarType, GraphQLError, BREAK, + parse, } from 'graphql'; +import { LogAction } from 'apollo-server-core'; const QueryRootType = new GraphQLObjectType({ name: 'QueryRoot', @@ -538,4 +540,40 @@ describe(`GraphQL-HTTP (apolloServer) tests for ${version} express`, () => { }); }); }); + + describe('Query is an AST', () => { + it('Do not validate if query is already an AST.', async () => { + const app = express(); + let validationCalled = false; + + app.use('/graphql', bodyParser.json()); + app.use('/graphql', (req, res, next) => { + req.body.query = parse(req.body.query); + + next(); + }); + app.use( + '/graphql', + graphqlExpress({ + schema: TestSchema, + logFunction: ({ action }) => { + if (action == LogAction.validation) { + validationCalled = true; + } + }, + }), + ); + + const response = await request(app) + .post('/graphql') + .send({ + query: '{ test(who: "World") }', + }); + + expect( + validationCalled, + 'Validation should not be called if query is already an AST', + ).to.equal(false); + }); + }); }); From 52f158939c251f566014c2e789918430581f7a28 Mon Sep 17 00:00:00 2001 From: damour Date: Mon, 19 Feb 2018 20:00:20 +0200 Subject: [PATCH 2/2] Skip validation option. --- CHANGELOG.md | 2 +- README.md | 3 ++ .../apollo-server-core/src/runHttpQuery.ts | 4 +- packages/apollo-server-core/src/runQuery.ts | 4 +- .../src/apolloServerHttp.test.ts | 38 ------------------- .../src/index.ts | 33 ++++++++++++++++ 6 files changed, 40 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3926387b280..22427a6c995 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ All of the packages in the `apollo-server` repo are released with the same versi ### vNEXT -* Don't validate if query is already an AST. [PR #839](https://github.com/apollographql/apollo-server/pull/839) +* Add skipValidation option [PR #839](https://github.com/apollographql/apollo-server/pull/839) ### v1.3.6 diff --git a/README.md b/README.md index cabf5547225..b16220a15e1 100644 --- a/README.md +++ b/README.md @@ -214,6 +214,7 @@ Apollo Server can be configured with an options object with the following fields * **context**: the context value passed to resolvers during GraphQL execution * **rootValue**: the value passed to the first resolve function * **formatError**: a function to apply to every error before sending the response to clients +* **skipValidation**: skip query validation (increase performance, use carefully, only with whitelisting) * **validationRules**: additional GraphQL validation rules to be applied to client-specified queries * **formatParams**: a function applied for each query in a batch to format parameters before execution * **formatResponse**: a function applied to each response after execution @@ -228,6 +229,7 @@ All options except for `schema` are optional. ### Whitelisting The `formatParams` function can be used in combination with the `OperationStore` to enable whitelisting. +In this case query parsing and validation will be called only once when saving to store. ```js const store = new OperationStore(Schema); @@ -236,6 +238,7 @@ graphqlOptions = { schema: Schema, formatParams(params) { params['query'] = store.get(params.operationName); + params['skipValidation'] = true; return params; }, }; diff --git a/packages/apollo-server-core/src/runHttpQuery.ts b/packages/apollo-server-core/src/runHttpQuery.ts index 00fa9e0f4ae..f6eb87c9d6e 100644 --- a/packages/apollo-server-core/src/runHttpQuery.ts +++ b/packages/apollo-server-core/src/runHttpQuery.ts @@ -102,7 +102,6 @@ export async function runHttpQuery( try { let query = requestParams.query; let extensions = requestParams.extensions; - const isQueryString = typeof query === 'string'; if (isGetRequest && extensions) { // For GET requests, we have to JSON-parse extensions. (For POST @@ -139,7 +138,7 @@ export async function runHttpQuery( } if (isGetRequest) { - if (isQueryString) { + if (typeof query === 'string') { // preparse the query incase of GET so we can assert the operation. // XXX This makes the type of 'query' in this function confused // which has led to us accidentally supporting GraphQL AST over @@ -194,7 +193,6 @@ export async function runHttpQuery( query: query, variables: variables, context, - isQueryString, rootValue: optionsObject.rootValue, operationName: operationName, logFunction: optionsObject.logFunction, diff --git a/packages/apollo-server-core/src/runQuery.ts b/packages/apollo-server-core/src/runQuery.ts index 672237a9931..fadb6240856 100644 --- a/packages/apollo-server-core/src/runQuery.ts +++ b/packages/apollo-server-core/src/runQuery.ts @@ -72,7 +72,7 @@ export interface QueryOptions { debug?: boolean; tracing?: boolean; cacheControl?: boolean | CacheControlExtensionOptions; - isQueryString?: boolean; + skipValidation?: boolean; } export function runQuery(options: QueryOptions): Promise { @@ -172,7 +172,7 @@ function doRunQuery(options: QueryOptions): Promise { documentAST = options.query as DocumentNode; } - if (options.isQueryString !== false) { + if (options.skipValidation !== true) { let rules = specifiedRules; if (options.validationRules) { rules = rules.concat(options.validationRules); diff --git a/packages/apollo-server-express/src/apolloServerHttp.test.ts b/packages/apollo-server-express/src/apolloServerHttp.test.ts index 407dc35b5c7..1255a388249 100644 --- a/packages/apollo-server-express/src/apolloServerHttp.test.ts +++ b/packages/apollo-server-express/src/apolloServerHttp.test.ts @@ -37,9 +37,7 @@ import { GraphQLScalarType, GraphQLError, BREAK, - parse, } from 'graphql'; -import { LogAction } from 'apollo-server-core'; const QueryRootType = new GraphQLObjectType({ name: 'QueryRoot', @@ -540,40 +538,4 @@ describe(`GraphQL-HTTP (apolloServer) tests for ${version} express`, () => { }); }); }); - - describe('Query is an AST', () => { - it('Do not validate if query is already an AST.', async () => { - const app = express(); - let validationCalled = false; - - app.use('/graphql', bodyParser.json()); - app.use('/graphql', (req, res, next) => { - req.body.query = parse(req.body.query); - - next(); - }); - app.use( - '/graphql', - graphqlExpress({ - schema: TestSchema, - logFunction: ({ action }) => { - if (action == LogAction.validation) { - validationCalled = true; - } - }, - }), - ); - - const response = await request(app) - .post('/graphql') - .send({ - query: '{ test(who: "World") }', - }); - - expect( - validationCalled, - 'Validation should not be called if query is already an AST', - ).to.equal(false); - }); - }); }); diff --git a/packages/apollo-server-integration-testsuite/src/index.ts b/packages/apollo-server-integration-testsuite/src/index.ts index 201145e8820..3ca17a45e68 100644 --- a/packages/apollo-server-integration-testsuite/src/index.ts +++ b/packages/apollo-server-integration-testsuite/src/index.ts @@ -19,6 +19,7 @@ const request = require('supertest'); import { GraphQLOptions } from 'apollo-server-core'; import * as GraphiQL from 'apollo-server-module-graphiql'; import { OperationStore } from 'apollo-server-module-operation-store'; +import { LogAction } from '../../apollo-server-core/dist'; const personType = new GraphQLObjectType({ name: 'PersonType', @@ -1054,6 +1055,38 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => { expect(res.body).to.deep.equal(expected); }); }); + + it('do not validate if query is already an AST', async () => { + const store = new OperationStore(schema); + let validationCalled = false; + store.put('query testquery{ testString }'); + app = await createApp({ + graphqlOptions: { + schema, + formatParams(params) { + params['query'] = store.get(params.operationName); + params['skipValidation'] = true; + return params; + }, + logFunction: ({ action }) => { + if (action == LogAction.validation) { + validationCalled = true; + } + }, + }, + }); + const req = request(app) + .post('/graphql') + .send({ + operationName: 'testquery', + }); + return req.then(res => { + return expect( + validationCalled, + 'Validation should not be called if skipValidation option provided', + ).to.equal(false); + }); + }); }); describe('server setup', () => {