Skip to content

better error message for related errors #3389

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/rude-squids-accept.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'graphql-yoga': patch
---

Improve error messages in case of `operatinName` related errors.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was the error message before?
I also don't see any new tests that ensure that the new error message is used and a regression in the future is avoided.

2 changes: 1 addition & 1 deletion packages/graphql-yoga/__tests__/graphql-http.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { createGraphQLError } from '@graphql-tools/utils';
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';

Expand All @@ -18,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,
},
},
});
}

Comment on lines +23 to +35
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This part was missing but required in the http spec. It was caught before by chance...

Copy link
Collaborator

Choose a reason for hiding this comment

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

How can we ensure we don't introduce a regression? How was this cattched by chance?

for (const paramKey in params) {
if ((params as Record<string, unknown>)[paramKey] == null) {
continue;
Expand Down Expand Up @@ -133,11 +149,78 @@ 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;
}
}

Comment on lines +195 to +205
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure it's needed, just wanted to implement the same pattern that was used for the check of the params

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this function used?

export function useCheckGraphQLQueryParams(extraParamNames?: string[]): Plugin {
return {
onParams({ params }) {
checkGraphQLQueryParams(params, extraParamNames);
},
onParse() {
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 (!context.request || !context.params || !isDocumentNode(result)) {
return;
}

checkOperationName(context.params.operationName, result);
};
},
};
}

Expand Down Expand Up @@ -166,3 +249,11 @@ function extendedTypeof(
function isObject(val: unknown): val is Record<PropertyKey, unknown> {
return extendedTypeof(val) === 'object';
}

function* listOperations(document: DocumentNode) {
for (const definition of document.definitions) {
if (definition.kind === Kind.OPERATION_DEFINITION) {
yield definition;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,7 @@ export function assertMutationViaGet(
? getOperationAST(document, operationName) ?? undefined
: undefined;

if (!operation) {
throw createGraphQLError('Could not determine what operation to execute.', {
extensions: {
http: {
status: 400,
},
},
});
}

if (operation.operation === 'mutation' && method === 'GET') {
if (operation?.operation === 'mutation' && method === 'GET') {
throw createGraphQLError('Can only perform a mutation operation from a POST request.', {
extensions: {
http: {
Expand Down
Loading