From 6d841c5b5038a6bdf6a8184e0766caf439546b11 Mon Sep 17 00:00:00 2001 From: Valentin Cocaud Date: Fri, 26 Jul 2024 16:56:51 +0200 Subject: [PATCH 1/4] better error message for related errors --- .changeset/rude-squids-accept.md | 5 ++ .../use-check-graphql-query-params.ts | 52 ++++++++++++++++++- .../use-prevent-mutation-via-get.ts | 42 ++++----------- 3 files changed, 67 insertions(+), 32 deletions(-) create mode 100644 .changeset/rude-squids-accept.md diff --git a/.changeset/rude-squids-accept.md b/.changeset/rude-squids-accept.md new file mode 100644 index 0000000000..fd8cac4712 --- /dev/null +++ b/.changeset/rude-squids-accept.md @@ -0,0 +1,5 @@ +--- +'graphql-yoga': patch +--- + +Improve error messages in case of `operatinName` related errors. diff --git a/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts b/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts index ece4169e61..9b07ef6004 100644 --- a/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts +++ b/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts @@ -1,4 +1,5 @@ -import { createGraphQLError } from '@graphql-tools/utils'; +import { Kind } from 'graphql'; +import { createGraphQLError, isDocumentNode } from '@graphql-tools/utils'; import type { GraphQLParams } from '../../types.js'; import type { Plugin } from '../types.js'; @@ -138,6 +139,55 @@ export function useCheckGraphQLQueryParams(extraParamNames?: string[]): Plugin { onParams({ params }) { checkGraphQLQueryParams(params, extraParamNames); }, + onParse() { + return ({ + result, + context: { + request, + params: { operationName }, + }, + }) => { + // Run only if this is a Yoga request + // the `request` might be missing when using graphql-ws for example + // in which case throwing an error would abruptly close the socket + if (!request || !isDocumentNode(result)) { + return; + } + + let message: string | undefined; + + const operations = result.definitions.filter( + definition => definition.kind === Kind.OPERATION_DEFINITION, + ); + + if (operationName) { + const operationExists = operations.some( + operation => operation.name?.value === operationName, + ); + + if (!operationExists) { + if (operations.length === 1) { + message = `Operation name "${operationName}" doesn't match the name defined in the query.`; + } else { + message = `Could not determine what operation to execute. There is no operation "${operationName}" in the query.`; + } + } + } else if (operations.length > 1) { + message = + 'Could not determine what operation to execute. The query contains multiple operations, an operation name must be provided'; + } + + if (message) { + throw createGraphQLError(message, { + extensions: { + http: { + status: 400, + }, + }, + }); + } + }; + }, }; } diff --git a/packages/graphql-yoga/src/plugins/request-validation/use-prevent-mutation-via-get.ts b/packages/graphql-yoga/src/plugins/request-validation/use-prevent-mutation-via-get.ts index 45ceab7a6d..669dbc1671 100644 --- a/packages/graphql-yoga/src/plugins/request-validation/use-prevent-mutation-via-get.ts +++ b/packages/graphql-yoga/src/plugins/request-validation/use-prevent-mutation-via-get.ts @@ -1,29 +1,15 @@ -import { DocumentNode, getOperationAST, GraphQLError, OperationDefinitionNode } from 'graphql'; +import { DocumentNode, GraphQLError, Kind } from 'graphql'; import { Maybe } from '@envelop/core'; -import { createGraphQLError } from '@graphql-tools/utils'; +import { createGraphQLError, isDocumentNode } from '@graphql-tools/utils'; import type { YogaInitialContext } from '../../types.js'; import type { Plugin } from '../types.js'; -export function assertMutationViaGet( - method: string, - document: Maybe, - operationName?: string, -) { - const operation: OperationDefinitionNode | undefined = document - ? getOperationAST(document, operationName) ?? undefined - : undefined; +export function assertMutationViaGet(method: string, document: Maybe) { + const isMutation = + document?.definitions.find(def => def.kind === Kind.OPERATION_DEFINITION)?.operation === + 'mutation'; - if (!operation) { - throw createGraphQLError('Could not determine what operation to execute.', { - extensions: { - http: { - status: 400, - }, - }, - }); - } - - if (operation.operation === 'mutation' && method === 'GET') { + if (isMutation && method === 'GET') { throw createGraphQLError('Can only perform a mutation operation from a POST request.', { extensions: { http: { @@ -41,15 +27,7 @@ export function usePreventMutationViaGET(): Plugin { return { onParse() { // We should improve this by getting Yoga stuff from the hook params directly instead of the context - return ({ - result, - context: { - request, - // the `params` might be missing in cases where the user provided - // malformed context to getEnveloped (like `yoga.getEnveloped({})`) - params: { operationName } = {}, - }, - }) => { + return ({ result, context: { request } }) => { // Run only if this is a Yoga request // the `request` might be missing when using graphql-ws for example // in which case throwing an error would abruptly close the socket @@ -67,7 +45,9 @@ export function usePreventMutationViaGET(): Plugin { throw result; } - assertMutationViaGet(request.method, result, operationName); + if (isDocumentNode(result)) { + assertMutationViaGet(request.method, result); + } }; }, }; From 56eff8cb544874646b1e4ceb57152198411f2775 Mon Sep 17 00:00:00 2001 From: Valentin Cocaud Date: Fri, 26 Jul 2024 17:27:45 +0200 Subject: [PATCH 2/4] take review in account --- .../use-check-graphql-query-params.ts | 47 ++++++++++--------- .../use-prevent-mutation-via-get.ts | 32 ++++++++----- 2 files changed, 47 insertions(+), 32 deletions(-) diff --git a/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts b/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts index 9b07ef6004..a2b844beaf 100644 --- a/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts +++ b/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts @@ -154,38 +154,43 @@ export function useCheckGraphQLQueryParams(extraParamNames?: string[]): Plugin { return; } - let message: string | undefined; + let numberOfOperations = 0; - const operations = result.definitions.filter( - definition => definition.kind === Kind.OPERATION_DEFINITION, - ); - - if (operationName) { - const operationExists = operations.some( - operation => operation.name?.value === operationName, - ); + for (const definition of result.definitions) { + if (definition.kind === Kind.OPERATION_DEFINITION) { + if (definition.name?.value === operationName) { + return; + } - if (!operationExists) { - if (operations.length === 1) { - message = `Operation name "${operationName}" doesn't match the name defined in the query.`; - } else { - message = `Could not determine what operation to execute. There is no operation "${operationName}" in the query.`; + numberOfOperations++; + + if (operationName == null && numberOfOperations > 1) { + throw createGraphQLError( + 'Could not determine what operation to execute. The query contains multiple operations, an operation name must be provided', + { + extensions: { + http: { + status: 400, + }, + }, + }, + ); } } - } else if (operations.length > 1) { - message = - 'Could not determine what operation to execute. The query contains multiple operations, an operation name must be provided'; } - if (message) { - throw createGraphQLError(message, { + throw createGraphQLError( + numberOfOperations === 1 + ? `Operation name "${operationName}" doesn't match the name defined in the query.` + : `Could not determine what operation to execute. There is no operation "${operationName}" in the query.`, + { extensions: { http: { status: 400, }, }, - }); - } + }, + ); }; }, }; diff --git a/packages/graphql-yoga/src/plugins/request-validation/use-prevent-mutation-via-get.ts b/packages/graphql-yoga/src/plugins/request-validation/use-prevent-mutation-via-get.ts index 669dbc1671..bb381e6671 100644 --- a/packages/graphql-yoga/src/plugins/request-validation/use-prevent-mutation-via-get.ts +++ b/packages/graphql-yoga/src/plugins/request-validation/use-prevent-mutation-via-get.ts @@ -1,15 +1,19 @@ -import { DocumentNode, GraphQLError, Kind } from 'graphql'; +import { DocumentNode, getOperationAST, GraphQLError, OperationDefinitionNode } from 'graphql'; import { Maybe } from '@envelop/core'; -import { createGraphQLError, isDocumentNode } from '@graphql-tools/utils'; +import { createGraphQLError } from '@graphql-tools/utils'; import type { YogaInitialContext } from '../../types.js'; import type { Plugin } from '../types.js'; -export function assertMutationViaGet(method: string, document: Maybe) { - const isMutation = - document?.definitions.find(def => def.kind === Kind.OPERATION_DEFINITION)?.operation === - 'mutation'; +export function assertMutationViaGet( + method: string, + document: Maybe, + operationName?: string, +) { + const operation: OperationDefinitionNode | undefined = document + ? getOperationAST(document, operationName) ?? undefined + : undefined; - if (isMutation && method === 'GET') { + if (operation?.operation === 'mutation' && method === 'GET') { throw createGraphQLError('Can only perform a mutation operation from a POST request.', { extensions: { http: { @@ -27,7 +31,15 @@ export function usePreventMutationViaGET(): Plugin { return { onParse() { // We should improve this by getting Yoga stuff from the hook params directly instead of the context - return ({ result, context: { request } }) => { + return ({ + result, + context: { + request, + // the `params` might be missing in cases where the user provided + // malformed context to getEnveloped (like `yoga.getEnveloped({})`) + params: { operationName } = {}, + }, + }) => { // Run only if this is a Yoga request // the `request` might be missing when using graphql-ws for example // in which case throwing an error would abruptly close the socket @@ -45,9 +57,7 @@ export function usePreventMutationViaGET(): Plugin { throw result; } - if (isDocumentNode(result)) { - assertMutationViaGet(request.method, result); - } + assertMutationViaGet(request.method, result, operationName); }; }, }; From f0258a2de2c620900615ce9e11e722a29d546d0a Mon Sep 17 00:00:00 2001 From: Valentin Cocaud Date: Mon, 29 Jul 2024 16:57:51 +0200 Subject: [PATCH 3/4] refactor using generator --- .../use-check-graphql-query-params.ts | 77 ++++++++++--------- 1 file changed, 40 insertions(+), 37 deletions(-) diff --git a/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts b/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts index a2b844beaf..418e4459bf 100644 --- a/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts +++ b/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts @@ -1,4 +1,4 @@ -import { Kind } from 'graphql'; +import { Kind, type DocumentNode } from 'graphql'; import { createGraphQLError, isDocumentNode } from '@graphql-tools/utils'; import type { GraphQLParams } from '../../types.js'; import type { Plugin } from '../types.js'; @@ -140,57 +140,52 @@ export function useCheckGraphQLQueryParams(extraParamNames?: string[]): Plugin { checkGraphQLQueryParams(params, extraParamNames); }, onParse() { - return ({ - result, - context: { - request, - params: { operationName }, - }, - }) => { + return ({ result, context }) => { // Run only if this is a Yoga request // the `request` might be missing when using graphql-ws for example // in which case throwing an error would abruptly close the socket - if (!request || !isDocumentNode(result)) { + if (!context.request || !context.params || !isDocumentNode(result)) { return; } - let numberOfOperations = 0; + const { params: { operationName } = {} } = context; + const operations = listOperations(result); - for (const definition of result.definitions) { - if (definition.kind === Kind.OPERATION_DEFINITION) { - if (definition.name?.value === operationName) { + if (operationName != null) { + for (const operation of operations) { + if (operation.name?.value === operationName) { return; } + } - numberOfOperations++; - - if (operationName == null && numberOfOperations > 1) { - throw createGraphQLError( - 'Could not determine what operation to execute. The query contains multiple operations, an operation name must be provided', - { - extensions: { - http: { - status: 400, - }, - }, + throw createGraphQLError( + `Could not determine what operation to execute. There is no operation "${operationName}" in the query.`, + { + extensions: { + http: { + spec: true, + status: 400, }, - ); - } - } + }, + }, + ); } - throw createGraphQLError( - numberOfOperations === 1 - ? `Operation name "${operationName}" doesn't match the name defined in the query.` - : `Could not determine what operation to execute. There is no operation "${operationName}" in the query.`, - { - extensions: { - http: { - status: 400, + operations.next(); + // If there is no operation name, we should have only one operation + if (!operations.next().done) { + throw createGraphQLError( + `Could not determine what operation to execute. There is no operation "${operationName}" in the query.`, + { + extensions: { + http: { + spec: true, + status: 400, + }, }, }, - }, - ); + ); + } }; }, }; @@ -221,3 +216,11 @@ function extendedTypeof( function isObject(val: unknown): val is Record { return extendedTypeof(val) === 'object'; } + +function* listOperations(document: DocumentNode) { + for (const definition of document.definitions) { + if (definition.kind === Kind.OPERATION_DEFINITION) { + yield definition; + } + } +} From 66f8e744f2da857b4c90ecd61f474f87acf60c41 Mon Sep 17 00:00:00 2001 From: Valentin Cocaud Date: Mon, 29 Jul 2024 17:27:44 +0200 Subject: [PATCH 4/4] fix http tests --- .../__tests__/graphql-http.spec.ts | 2 +- .../use-check-graphql-query-params.ts | 109 ++++++++++++------ 2 files changed, 72 insertions(+), 39 deletions(-) diff --git a/packages/graphql-yoga/__tests__/graphql-http.spec.ts b/packages/graphql-yoga/__tests__/graphql-http.spec.ts index aa3be7f3a1..5da9daa0d8 100644 --- a/packages/graphql-yoga/__tests__/graphql-http.spec.ts +++ b/packages/graphql-yoga/__tests__/graphql-http.spec.ts @@ -15,7 +15,7 @@ for (const audit of serverAudits({ url: 'http://yoga/graphql', fetchFn: yoga.fetch, })) { - test(audit.name, async () => { + test(`[${audit.id}] ${audit.name}`, async () => { const result = await audit.fn(); if (result.status !== 'ok') { throw result.reason; diff --git a/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts b/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts index 418e4459bf..59290c76c1 100644 --- a/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts +++ b/packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts @@ -19,6 +19,21 @@ export function assertInvalidParams( }, }); } + + if ( + 'operationName' in params && + typeof params.operationName !== 'string' && + params.operationName != null + ) { + throw createGraphQLError(`Invalid operation name in the request body.`, { + extensions: { + http: { + status: 400, + }, + }, + }); + } + for (const paramKey in params) { if ((params as Record)[paramKey] == null) { continue; @@ -134,6 +149,61 @@ export function isValidGraphQLParams(params: unknown): params is GraphQLParams { } } +export function checkOperationName( + operationName: string | undefined, + document: DocumentNode, +): void { + const operations = listOperations(document); + + if (operationName != null) { + for (const operation of operations) { + if (operation.name?.value === operationName) { + return; + } + } + + throw createGraphQLError( + `Could not determine what operation to execute. There is no operation "${operationName}" in the query.`, + { + extensions: { + http: { + spec: true, + status: 400, + }, + }, + }, + ); + } + + operations.next(); + // If there is no operation name, we should have only one operation + if (!operations.next().done) { + throw createGraphQLError( + `Could not determine what operation to execute. The query contains multiple operation, an operation name should be provided.`, + { + extensions: { + http: { + spec: true, + status: 400, + }, + }, + }, + ); + } +} + +export function isValidOperationName( + operationName: string | undefined, + document: DocumentNode, +): boolean { + try { + checkOperationName(operationName, document); + return true; + } catch { + return false; + } +} + export function useCheckGraphQLQueryParams(extraParamNames?: string[]): Plugin { return { onParams({ params }) { @@ -148,44 +218,7 @@ export function useCheckGraphQLQueryParams(extraParamNames?: string[]): Plugin { return; } - const { params: { operationName } = {} } = context; - const operations = listOperations(result); - - if (operationName != null) { - for (const operation of operations) { - if (operation.name?.value === operationName) { - return; - } - } - - throw createGraphQLError( - `Could not determine what operation to execute. There is no operation "${operationName}" in the query.`, - { - extensions: { - http: { - spec: true, - status: 400, - }, - }, - }, - ); - } - - operations.next(); - // If there is no operation name, we should have only one operation - if (!operations.next().done) { - throw createGraphQLError( - `Could not determine what operation to execute. There is no operation "${operationName}" in the query.`, - { - extensions: { - http: { - spec: true, - status: 400, - }, - }, - }, - ); - } + checkOperationName(context.params.operationName, result); }; }, };