Fix getCacheKey path matching for graphql (#16647)

* fix getCacheKey path matching for graphql

* tweak  test

api/src/utils/get-cache-key.test.ts

@@ -1,9 +1,11 @@
 import { Request } from 'express';
-import { getCacheKey } from '../../src/utils/get-cache-key';
-import { describe, test, expect } from 'vitest';
+import { getCacheKey } from './get-cache-key';
+import * as getGraphqlQueryUtil from './get-graphql-query-and-variables';
+import { afterEach, beforeAll, describe, expect, SpyInstance, test, vi } from 'vitest';
 
-const restUrl = 'http://localhost/items/example';
-const graphQlUrl = 'http://localhost/graphql';
+const baseUrl = 'http://localhost';
+const restUrl = `${baseUrl}/items/example`;
+const graphQlUrl = `${baseUrl}/graphql`;
 const accountability = { user: '00000000-0000-0000-0000-000000000000' };
 const method = 'GET';
 
@@ -52,7 +54,32 @@ const requests = [
 
 const cases = requests.map(({ name, params, key }) => [name, params, key]);
 
+afterEach(() => {
+	vi.clearAllMocks();
+});
+
 describe('get cache key', () => {
+	describe('isGraphQl', () => {
+		let getGraphqlQuerySpy: SpyInstance;
+
+		beforeAll(() => {
+			getGraphqlQuerySpy = vi.spyOn(getGraphqlQueryUtil, 'getGraphqlQueryAndVariables');
+		});
+
+		test.each(['/items/test', '/items/graphql', '/collections/test', '/collections/graphql'])(
+			'path "%s" should not be interpreted as a graphql query',
+			(path) => {
+				getCacheKey({ originalUrl: `${baseUrl}${path}` } as Request);
+				expect(getGraphqlQuerySpy).not.toHaveBeenCalled();
+			}
+		);
+
+		test.each(['/graphql', '/graphql/system'])('path "%s" should be interpreted as a graphql query', (path) => {
+			getCacheKey({ originalUrl: `${baseUrl}${path}` } as Request);
+			expect(getGraphqlQuerySpy).toHaveBeenCalledOnce();
+		});
+	});
+
 	test.each(cases)('should create a cache key for %s', (_, params, key) => {
 		expect(getCacheKey(params as unknown as Request)).toEqual(key);
 	});

api/src/utils/get-cache-key.ts

@@ -1,17 +1,16 @@
 import { Request } from 'express';
-import url from 'url';
 import hash from 'object-hash';
-import { pick } from 'lodash';
+import url from 'url';
+import { getGraphqlQueryAndVariables } from './get-graphql-query-and-variables';
 
 export function getCacheKey(req: Request): string {
 	const path = url.parse(req.originalUrl).pathname;
-	const isGraphQl = path?.includes('/graphql');
-	const isGet = req.method?.toLowerCase() === 'get';
+	const isGraphQl = path?.startsWith('/graphql');
 
 	const info = {
 		user: req.accountability?.user || null,
 		path,
-		query: isGraphQl ? pick(isGet ? req.query : req.body, ['query', 'variables']) : req.sanitizedQuery,
+		query: isGraphQl ? getGraphqlQueryAndVariables(req) : req.sanitizedQuery,
 	};
 
 	const key = hash(info);

api/src/utils/get-graphql-query-and-variables.test.ts

@@ -0,0 +1,24 @@
+import { Request } from 'express';
+import { expect, test } from 'vitest';
+
+import { getGraphqlQueryAndVariables } from './get-graphql-query-and-variables';
+
+const query = `
+	query getProduct($id: ID!) {
+		products_by_id(id: $id) {
+			id
+		}
+	}
+`;
+const variables = JSON.stringify({ id: 1 });
+const additionalProperty = 'test';
+
+test('should get query from request query for GET method', async () => {
+	const request = { method: 'GET', query: { query, variables, additionalProperty } } as unknown as Request;
+	expect(getGraphqlQueryAndVariables(request)).toEqual({ query, variables });
+});
+
+test('should get query from request body for other methods', async () => {
+	const request = { method: 'POST', body: { query, variables, additionalProperty } } as unknown as Request;
+	expect(getGraphqlQueryAndVariables(request)).toEqual({ query, variables });
+});

api/src/utils/get-graphql-query-and-variables.ts

@@ -0,0 +1,7 @@
+import { Request } from 'express';
+import { pick } from 'lodash';
+
+export function getGraphqlQueryAndVariables(req: Request) {
+	const isGet = req.method?.toLowerCase() === 'get';
+	return pick(isGet ? req.query : req.body, ['query', 'variables']);
+}