From 92f6fa00741d691e2f34670c0d574c9588f1de58 Mon Sep 17 00:00:00 2001 From: Jason Kuhrt Date: Sat, 3 Feb 2024 13:48:35 -0500 Subject: [PATCH 1/2] fix: no GET on mutations --- package.json | 1 + pnpm-lock.yaml | 24 +++++++ src/classes/GraphQLClient.ts | 21 +++--- src/entrypoints/main.ts | 2 +- src/helpers/resolveRequestDocument.ts | 63 ++++++++++-------- src/helpers/runRequest.ts | 94 +++++++++++++++++---------- src/lib/graphql-ws.ts | 6 +- src/lib/graphql.ts | 16 +++++ tests/httpMethod.test.ts | 24 +++++++ 9 files changed, 175 insertions(+), 76 deletions(-) create mode 100644 tests/httpMethod.test.ts diff --git a/package.json b/package.json index a3f7cf02c..94deb3671 100644 --- a/package.json +++ b/package.json @@ -91,6 +91,7 @@ "happy-dom": "^13.3.1", "json-bigint": "^1.0.0", "prettier": "^3.2.4", + "tsx": "^4.7.0", "type-fest": "^4.10.1", "typescript": "^5.3.3", "vitest": "^1.2.1", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 1d97c1f64..0bc9a6064 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -103,6 +103,9 @@ devDependencies: prettier: specifier: ^3.2.4 version: 3.2.4 + tsx: + specifier: ^4.7.0 + version: 4.7.0 type-fest: specifier: ^4.10.1 version: 4.10.1 @@ -2635,6 +2638,12 @@ packages: engines: {node: '>=16'} dev: true + /get-tsconfig@4.7.2: + resolution: {integrity: sha512-wuMsz4leaj5hbGgg4IvDU0bqJagpftG5l5cXIAvo8uZrqn0NJqwtfupTN00VnkQJPcIRrxYrm1Ue24btpCha2A==} + dependencies: + resolve-pkg-maps: 1.0.0 + dev: true + /getpass@0.1.7: resolution: {integrity: sha512-0fzj9JxOLfJ+XGLhR8ze3unN0KZCgZwiSSDz168VERjK8Wl8kVSdcu2kspd4s4wtAa1y/qrVRiAA0WclVsu0ng==} dependencies: @@ -3779,6 +3788,10 @@ packages: engines: {node: '>=4'} dev: true + /resolve-pkg-maps@1.0.0: + resolution: {integrity: sha512-seS2Tj26TBVOC2NIc2rOe2y2ZO7efxITtLZcGSOnHHNOQ7CkiUBfw0Iw2ck6xkIhPwLhKNLS8BO+hEpngQlqzw==} + dev: true + /resolve@1.19.0: resolution: {integrity: sha512-rArEXAgsBG4UgRGcynxWIWKFvh/XZCcS8UJdHhwy91zwAvCZIbcs+vAbflgBnNjYMs/i/i+/Ux6IZhML1yPvxg==} dependencies: @@ -4138,6 +4151,17 @@ packages: typescript: 5.3.3 dev: true + /tsx@4.7.0: + resolution: {integrity: sha512-I+t79RYPlEYlHn9a+KzwrvEwhJg35h/1zHsLC2JXvhC2mdynMv6Zxzvhv5EMV6VF5qJlLlkSnMVvdZV3PSIGcg==} + engines: {node: '>=18.0.0'} + hasBin: true + dependencies: + esbuild: 0.19.12 + get-tsconfig: 4.7.2 + optionalDependencies: + fsevents: 2.3.3 + dev: true + /tunnel-agent@0.6.0: resolution: {integrity: sha512-McnNiV1l8RYeY8tBgEpuodCC1mLUdbSN+CYBL7kJsJNInOP8UjDDEwdk6Mw60vdLLrr5NHKZhMAOSrR2NZuQ+w==} dependencies: diff --git a/src/classes/GraphQLClient.ts b/src/classes/GraphQLClient.ts index d4311e81d..5500ea5d7 100644 --- a/src/classes/GraphQLClient.ts +++ b/src/classes/GraphQLClient.ts @@ -2,7 +2,7 @@ import type { BatchRequestDocument, BatchRequestsOptions, BatchResult } from '.. import { parseBatchRequestArgs } from '../functions/batchRequests.js' import { parseRawRequestArgs } from '../functions/rawRequest.js' import { parseRequestArgs } from '../functions/request.js' -import { resolveRequestDocument } from '../helpers/resolveRequestDocument.js' +import { analyzeDocument } from '../helpers/resolveRequestDocument.js' import { runRequest } from '../helpers/runRequest.js' import type { RequestDocument, RequestOptions, VariablesAndRequestHeadersArgs } from '../helpers/types.js' import { @@ -45,14 +45,13 @@ export class GraphQLClient { fetchOptions.signal = rawRequestOptions.signal } - const { operationName } = resolveRequestDocument(rawRequestOptions.query, excludeOperationName) + const document = analyzeDocument(rawRequestOptions.query, excludeOperationName) const response = await runRequest({ url, request: { _tag: `Single`, - operationName, - query: rawRequestOptions.query, + document, variables: rawRequestOptions.variables, }, headers: { @@ -105,14 +104,13 @@ export class GraphQLClient { fetchOptions.signal = requestOptions.signal } - const { query, operationName } = resolveRequestDocument(requestOptions.document, excludeOperationName) + const analyzedDocument = analyzeDocument(requestOptions.document, excludeOperationName) const response = await runRequest({ url, request: { - operationName, _tag: `Single`, - query, + document: analyzedDocument, variables: requestOptions.variables, }, headers: { @@ -152,9 +150,11 @@ export class GraphQLClient { fetchOptions.signal = batchRequestOptions.signal } - const queries = batchRequestOptions.documents.map( - ({ document }) => resolveRequestDocument(document, excludeOperationName).query + const analyzedDocuments = batchRequestOptions.documents.map( + ({ document }) => analyzeDocument(document, excludeOperationName) ) + const expressions = analyzedDocuments.map(({ expression }) => expression) + const hasMutations = analyzedDocuments.some(({ isMutation }) => isMutation) const variables = batchRequestOptions.documents.map(({ variables }) => variables) const response= await runRequest({ @@ -162,7 +162,8 @@ export class GraphQLClient { request: { _tag:`Batch`, operationName: undefined, - query: queries, + query: expressions, + hasMutations, variables, }, headers: { diff --git a/src/entrypoints/main.ts b/src/entrypoints/main.ts index 54ef1294f..c77f814a4 100644 --- a/src/entrypoints/main.ts +++ b/src/entrypoints/main.ts @@ -12,7 +12,7 @@ export { GraphQLClient } from '../classes/GraphQLClient.js' export { batchRequests } from '../functions/batchRequests.js' export { gql } from '../functions/gql.js' export { rawRequest } from '../functions/rawRequest.js' -export { resolveRequestDocument } from '../helpers/resolveRequestDocument.js' +export { analyzeDocument } from '../helpers/resolveRequestDocument.js' export { GraphQLWebSocketClient } from '../lib/graphql-ws.js' export { BatchRequestDocument, diff --git a/src/helpers/resolveRequestDocument.ts b/src/helpers/resolveRequestDocument.ts index 666069b9d..0deeaeadc 100644 --- a/src/helpers/resolveRequestDocument.ts +++ b/src/helpers/resolveRequestDocument.ts @@ -1,11 +1,12 @@ +import { isOperationDefinitionNode } from '../lib/graphql.js' +import { tryCatch } from '../lib/prelude.js' import type { RequestDocument } from './types.js' /** * Refactored imports from `graphql` to be more specific, this helps import only the required files (100KiB) * instead of the entire package (greater than 500KiB) where tree-shaking is not supported. * @see https://github.com/jasonkuhrt/graphql-request/pull/543 */ -import type { DocumentNode, OperationDefinitionNode } from 'graphql/language/ast.js' -import { Kind } from 'graphql/language/kinds.js' +import { type DocumentNode, OperationTypeNode } from 'graphql/language/ast.js' import { parse } from 'graphql/language/parser.js' import { print } from 'graphql/language/printer.js' @@ -16,41 +17,47 @@ import { print } from 'graphql/language/printer.js' const extractOperationName = (document: DocumentNode): string | undefined => { let operationName = undefined - const operationDefinitions = document.definitions.filter( - (definition) => definition.kind === Kind.OPERATION_DEFINITION, - ) as OperationDefinitionNode[] + const defs = document.definitions.filter(isOperationDefinitionNode) - if (operationDefinitions.length === 1) { - operationName = operationDefinitions[0]?.name?.value + if (defs.length === 1) { + operationName = defs[0]!.name?.value } return operationName } -export const resolveRequestDocument = ( +const extractIsMutation = (document: DocumentNode): boolean => { + let isMutation = false + + const defs = document.definitions.filter(isOperationDefinitionNode) + + if (defs.length === 1) { + isMutation = defs[0]!.operation === OperationTypeNode.MUTATION + } + + return isMutation +} + +export const analyzeDocument = ( document: RequestDocument, excludeOperationName?: boolean, -): { query: string; operationName?: string } => { - if (typeof document === `string`) { - if (excludeOperationName) { - return { query: document } - } - - let operationName = undefined - - try { - const parsedDocument = parse(document) - operationName = extractOperationName(parsedDocument) - } catch (err) { - // Failed parsing the document, the operationName will be undefined - } - - return { query: document, operationName } - } +): { expression: string; operationName: string | undefined; isMutation: boolean } => { + const expression = typeof document === `string` ? document : print(document) + + let isMutation = false + let operationName = undefined + if (excludeOperationName) { - return { query: print(document) } + return { expression, isMutation, operationName } } - const operationName = extractOperationName(document) - return { query: print(document), operationName } + const docNode = tryCatch(() => (typeof document === `string` ? parse(document) : document)) + if (docNode instanceof Error) { + return { expression, isMutation, operationName } + } + + operationName = extractOperationName(docNode) + isMutation = extractIsMutation(docNode) + + return { expression, operationName, isMutation } } diff --git a/src/helpers/runRequest.ts b/src/helpers/runRequest.ts index 0d078dfad..4a2577a8e 100644 --- a/src/helpers/runRequest.ts +++ b/src/helpers/runRequest.ts @@ -20,9 +20,14 @@ import type { Variables, } from './types.js' -interface Params { +interface Input { url: string - method: HTTPMethodInput + /** + * The HTTP method to use for queries. Note that mutations are ALWAYS sent as POST requests ([per spec](https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md?rgh-link-date=2022-06-02T09%3A30%3A53Z)). + * + * @defaultValue `'POST'` + */ + method?: HTTPMethodInput fetch: Fetch fetchOptions: FetchOptions headers?: HeadersInit @@ -30,41 +35,56 @@ interface Params { request: | { _tag: 'Single' - query: string - operationName?: string variables?: Variables + document: { + expression: string + isMutation: boolean + operationName?: string + } } | { _tag: 'Batch' query: string[] operationName?: undefined + hasMutations: boolean variables?: BatchVariables } } // @ts-expect-error todo -export const runRequest = async (params: Params): Promise> => { - const $params = { - ...params, - method: uppercase(params.method ?? `post`), +export const runRequest = async (input: Input): Promise> => { + // todo make a Config type + const config = { + ...input, + method: + input.request._tag === `Single` + ? input.request.document.isMutation + ? `POST` + : uppercase(input.method ?? `post`) + : input.request.hasMutations + ? `POST` + : uppercase(input.method ?? `post`), fetchOptions: { - ...params.fetchOptions, - errorPolicy: params.fetchOptions.errorPolicy ?? `none`, + ...input.fetchOptions, + errorPolicy: input.fetchOptions.errorPolicy ?? `none`, }, } - const fetcher = createFetcher($params.method) - const fetchResponse = await fetcher($params) + const fetcher = createFetcher(config.method) + const fetchResponse = await fetcher(config) if (!fetchResponse.ok) { return new ClientError( { status: fetchResponse.status, headers: fetchResponse.headers }, - { query: params.request.query, variables: params.request.variables }, + { + query: input.request._tag === `Single` ? input.request.document.expression : input.request.query, + variables: input.request.variables, + }, ) } const result = await parseResultFromResponse( fetchResponse, - params.fetchOptions.jsonSerializer ?? defaultJsonSerializer, + input.fetchOptions.jsonSerializer ?? defaultJsonSerializer, ) if (result instanceof Error) throw result // todo something better @@ -74,8 +94,8 @@ export const runRequest = async (params: Params): Promise (executionResult: GraphQLExecutionResultSingle) => { + ($params: Input) => (executionResult: GraphQLExecutionResultSingle) => { return { extensions: executionResult.extensions, data: executionResult.data, @@ -126,7 +146,7 @@ const parseResultFromResponse = async (response: Response, jsonSerializer: JsonS } } -const createFetcher = (method: 'GET' | 'POST') => async (params: Params) => { +const createFetcher = (method: 'GET' | 'POST') => async (params: Input) => { const headers = new Headers(params.headers) let queryParams = `` let body = undefined @@ -150,11 +170,14 @@ const createFetcher = (method: 'GET' | 'POST') => async (params: Params) => { let urlResolved = params.url let initResolved = init if (params.middleware) { - const { - url, - request: { variables, operationName }, - } = params - const result = await Promise.resolve(params.middleware({ ...init, url, operationName, variables })) + const result = await Promise.resolve( + params.middleware({ + ...init, + url: params.url, + operationName: params.request._tag === `Single` ? params.request.document.operationName : undefined, + variables: params.request.variables, + }), + ) const { url: urlNew, ...initNew } = result urlResolved = urlNew initResolved = initNew @@ -166,10 +189,13 @@ const createFetcher = (method: 'GET' | 'POST') => async (params: Params) => { return await $fetch(urlResolved, initResolved) } -const buildBody = (params: Params) => { +const buildBody = (params: Input) => { if (params.request._tag === `Single`) { - const { query, variables, operationName } = params.request - return { query, variables, operationName } + const { + variables, + document: { expression, operationName }, + } = params.request + return { query: expression, variables, operationName } } else if (params.request._tag === `Batch`) { return zip(params.request.query, params.request.variables ?? []).map(([query, variables]) => ({ query, @@ -180,15 +206,15 @@ const buildBody = (params: Params) => { } } -const buildQueryParams = (params: Params): string => { +const buildQueryParams = (params: Input): string => { const $jsonSerializer = params.fetchOptions.jsonSerializer ?? defaultJsonSerializer if (params.request._tag === `Single`) { - const search: string[] = [`query=${encodeURIComponent(cleanQuery(params.request.query))}`] + const search: string[] = [`query=${encodeURIComponent(cleanQuery(params.request.document.expression))}`] if (params.request.variables) { search.push(`variables=${encodeURIComponent($jsonSerializer.stringify(params.request.variables))}`) } - if (params.request.operationName) { - search.push(`operationName=${encodeURIComponent(params.request.operationName)}`) + if (params.request.document.operationName) { + search.push(`operationName=${encodeURIComponent(params.request.document.operationName)}`) } return search.join(`&`) } else if (params.request._tag === `Batch`) { diff --git a/src/lib/graphql-ws.ts b/src/lib/graphql-ws.ts index b211ed5c4..f3b2b1300 100644 --- a/src/lib/graphql-ws.ts +++ b/src/lib/graphql-ws.ts @@ -1,5 +1,5 @@ /* eslint-disable */ -import { resolveRequestDocument } from '../helpers/resolveRequestDocument.js' +import { analyzeDocument } from '../helpers/resolveRequestDocument.js' import type { RequestDocument, Variables } from '../helpers/types.js' import { ClientError } from '../classes/ClientError.js' import { TypedDocumentNode } from '@graphql-typed-document-node/core' @@ -247,8 +247,8 @@ export class GraphQLWebSocketClient { subscriber: GraphQLSubscriber, variables?: V, ): UnsubscribeCallback { - const { query, operationName } = resolveRequestDocument(document, this.excludeOperationName) - return this.makeSubscribe(query, operationName, subscriber, variables) + const { expression, operationName } = analyzeDocument(document, this.excludeOperationName) + return this.makeSubscribe(expression, operationName, subscriber, variables) } rawSubscribe( diff --git a/src/lib/graphql.ts b/src/lib/graphql.ts index 3cb586067..ab3f8392e 100644 --- a/src/lib/graphql.ts +++ b/src/lib/graphql.ts @@ -1,5 +1,12 @@ import { CONTENT_TYPE_GQL, CONTENT_TYPE_JSON } from './http.js' import { isPlainObject } from './prelude.js' +import { Kind } from 'graphql' +/** + * Refactored imports from `graphql` to be more specific, this helps import only the required files (100KiB) + * instead of the entire package (greater than 500KiB) where tree-shaking is not supported. + * @see https://github.com/jasonkuhrt/graphql-request/pull/543 + */ +import type { OperationDefinitionNode } from 'graphql/language/ast.js' /** * Clean a GraphQL document to send it via a GET query @@ -88,3 +95,12 @@ export const isRequestResultHaveErrors = (result: GraphQLRequestResult) => export const isExecutionResultHaveErrors = (result: GraphQLExecutionResultSingle) => Array.isArray(result.errors) ? result.errors.length > 0 : Boolean(result.errors) + +export const isOperationDefinitionNode = (definition: unknown): definition is OperationDefinitionNode => { + return ( + typeof definition === `object` && + definition !== null && + `kind` in definition && + definition.kind === Kind.OPERATION_DEFINITION + ) +} diff --git a/tests/httpMethod.test.ts b/tests/httpMethod.test.ts new file mode 100644 index 000000000..8fe2d538d --- /dev/null +++ b/tests/httpMethod.test.ts @@ -0,0 +1,24 @@ +import { gql, GraphQLClient } from '../src/entrypoints/main.js' +import type { RequestConfig } from '../src/helpers/types.js' +import { CONTENT_TYPE_HEADER, statusCodes } from '../src/lib/http.js' +import { expect, test, vitest } from 'vitest' + +test(`mutation forces a POST method even if input wants GET for query`, async () => { + const fetch = vitest.fn().mockImplementation((_: string, requestConfig: RequestConfig) => { + expect(requestConfig.method).toBe(`POST`) + return new Response(JSON.stringify({ data: { a: { result: `ok` } } }), { + headers: new Headers({ [CONTENT_TYPE_HEADER]: `application/json; charset=utf-8` }), + status: statusCodes.success, + }) + }) + const client = new GraphQLClient(`https://foobar`, { fetch, method: `GET` }) + const document = gql` + mutation { + a { + result + } + } + ` + await client.request(document) + expect(fetch.mock.calls.length).toBe(1) +}) From f13fa8f7977d7514768375deec016a607a24038c Mon Sep 17 00:00:00 2001 From: Jason Kuhrt Date: Sat, 3 Feb 2024 13:49:45 -0500 Subject: [PATCH 2/2] refactor --- src/classes/GraphQLClient.ts | 2 +- src/entrypoints/main.ts | 2 +- src/helpers/{resolveRequestDocument.ts => analyzeDocument.ts} | 0 src/lib/graphql-ws.ts | 2 +- 4 files changed, 3 insertions(+), 3 deletions(-) rename src/helpers/{resolveRequestDocument.ts => analyzeDocument.ts} (100%) diff --git a/src/classes/GraphQLClient.ts b/src/classes/GraphQLClient.ts index 5500ea5d7..eae6e8094 100644 --- a/src/classes/GraphQLClient.ts +++ b/src/classes/GraphQLClient.ts @@ -2,7 +2,7 @@ import type { BatchRequestDocument, BatchRequestsOptions, BatchResult } from '.. import { parseBatchRequestArgs } from '../functions/batchRequests.js' import { parseRawRequestArgs } from '../functions/rawRequest.js' import { parseRequestArgs } from '../functions/request.js' -import { analyzeDocument } from '../helpers/resolveRequestDocument.js' +import { analyzeDocument } from '../helpers/analyzeDocument.js' import { runRequest } from '../helpers/runRequest.js' import type { RequestDocument, RequestOptions, VariablesAndRequestHeadersArgs } from '../helpers/types.js' import { diff --git a/src/entrypoints/main.ts b/src/entrypoints/main.ts index c77f814a4..262c81c8f 100644 --- a/src/entrypoints/main.ts +++ b/src/entrypoints/main.ts @@ -12,7 +12,7 @@ export { GraphQLClient } from '../classes/GraphQLClient.js' export { batchRequests } from '../functions/batchRequests.js' export { gql } from '../functions/gql.js' export { rawRequest } from '../functions/rawRequest.js' -export { analyzeDocument } from '../helpers/resolveRequestDocument.js' +export { analyzeDocument } from '../helpers/analyzeDocument.js' export { GraphQLWebSocketClient } from '../lib/graphql-ws.js' export { BatchRequestDocument, diff --git a/src/helpers/resolveRequestDocument.ts b/src/helpers/analyzeDocument.ts similarity index 100% rename from src/helpers/resolveRequestDocument.ts rename to src/helpers/analyzeDocument.ts diff --git a/src/lib/graphql-ws.ts b/src/lib/graphql-ws.ts index f3b2b1300..7c1f5d121 100644 --- a/src/lib/graphql-ws.ts +++ b/src/lib/graphql-ws.ts @@ -1,5 +1,5 @@ /* eslint-disable */ -import { analyzeDocument } from '../helpers/resolveRequestDocument.js' +import { analyzeDocument } from '../helpers/analyzeDocument.js' import type { RequestDocument, Variables } from '../helpers/types.js' import { ClientError } from '../classes/ClientError.js' import { TypedDocumentNode } from '@graphql-typed-document-node/core'