Skip to content
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

Add generic cache for graphQL querys (as opt-in per query) #5343

Open
wants to merge 1 commit 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
6 changes: 4 additions & 2 deletions packages/cli-kit/src/private/node/conf-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,19 @@
export type PackageVersionKey = `npm-package-${string}`
export type NotificationsKey = `notifications-${string}`
export type NotificationKey = `notification-${string}`
export type GraphQLRequestKey = `q-${string}-${string}-${string}`
type MostRecentOccurrenceKey = `most-recent-occurrence-${string}`
type RateLimitKey = `rate-limited-occurrences-${string}`

type ExportedKey = IntrospectionUrlKey | PackageVersionKey | NotificationsKey | NotificationKey
type ExportedKey = IntrospectionUrlKey | PackageVersionKey | NotificationsKey | NotificationKey | GraphQLRequestKey

interface Cache {
[introspectionUrlKey: IntrospectionUrlKey]: CacheValue<string>
[packageVersionKey: PackageVersionKey]: CacheValue<string>
[notifications: NotificationsKey]: CacheValue<string>
[notification: NotificationKey]: CacheValue<string>
[MostRecentOccurrenceKey: MostRecentOccurrenceKey]: CacheValue<boolean>
[graphQLRequestKey: GraphQLRequestKey]: CacheValue<string>
[mostRecentOccurrenceKey: MostRecentOccurrenceKey]: CacheValue<boolean>
[rateLimitKey: RateLimitKey]: CacheValue<number[]>
}

Expand Down Expand Up @@ -101,7 +103,7 @@
}

export function cacheStore(key: ExportedKey, value: string, config = cliKitStore()): void {
const cache: Cache = config.get('cache') || {}

Check warning on line 106 in packages/cli-kit/src/private/node/conf-store.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/conf-store.ts#L106

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
cache[key] = {value, timestamp: Date.now()}
config.set('cache', cache)
}
Expand All @@ -112,7 +114,7 @@
* @returns The chache element.
*/
export function cacheRetrieve(key: ExportedKey, config = cliKitStore()): CacheValue<string> | undefined {
const cache: Cache = config.get('cache') || {}

Check warning on line 117 in packages/cli-kit/src/private/node/conf-store.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/conf-store.ts#L117

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
return cache[key]
}

Expand Down Expand Up @@ -146,7 +148,7 @@
task: () => Promise<void>,
config = cliKitStore(),
): Promise<boolean> {
const cache: Cache = config.get('cache') || {}

Check warning on line 151 in packages/cli-kit/src/private/node/conf-store.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/conf-store.ts#L151

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const cacheKey: MostRecentOccurrenceKey = `most-recent-occurrence-${key}`
const cached = cache[cacheKey]

Expand Down Expand Up @@ -197,7 +199,7 @@
*/
export async function runWithRateLimit(options: RunWithRateLimitOptions, config = cliKitStore()): Promise<boolean> {
const {key, limit, timeout, task} = options
const cache: Cache = config.get('cache') || {}

Check warning on line 202 in packages/cli-kit/src/private/node/conf-store.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/private/node/conf-store.ts#L202

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const cacheKey: RateLimitKey = `rate-limited-occurrences-${key}`
const cached = cache[cacheKey]
const now = Date.now()
Expand Down
7 changes: 6 additions & 1 deletion packages/cli-kit/src/public/node/api/app-management.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {GraphQLResponse, graphqlRequestDoc} from './graphql.js'
import {CacheOptions, GraphQLResponse, graphqlRequestDoc} from './graphql.js'
import {appManagementFqdn} from '../context/fqdn.js'
import {setNextDeprecationDate} from '../../../private/node/context/deprecations-store.js'
import Bottleneck from 'bottleneck'
Expand Down Expand Up @@ -32,24 +32,29 @@ async function setupRequest(orgId: string, token: string) {
* @param query - GraphQL query to execute.
* @param token - Partners token.
* @param variables - GraphQL variables to pass to the query.
* @param cacheOptions - Cache options for the request. If not present, the request will not be cached.
* @returns The response of the query of generic type <T>.
*/
export async function appManagementRequestDoc<TResult, TVariables extends Variables>(
orgId: string,
query: TypedDocumentNode<TResult, TVariables>,
token: string,
variables?: TVariables,
cacheOptions?: CacheOptions,
): Promise<TResult> {
const result = limiter.schedule<TResult>(async () =>
graphqlRequestDoc<TResult, TVariables>({
...(await setupRequest(orgId, token)),
query,
variables,
cacheTTL: cacheOptions?.cacheTTL,
cacheExtraKey: cacheOptions?.cacheExtraKey,
}),
)

return result
}

interface Deprecation {
supportedUntilDate?: string
}
Expand Down
10 changes: 9 additions & 1 deletion packages/cli-kit/src/public/node/api/business-platform.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {Exact, GraphQLVariables, graphqlRequest, graphqlRequestDoc} from './graphql.js'
import {CacheOptions, Exact, GraphQLVariables, graphqlRequest, graphqlRequestDoc} from './graphql.js'
import {handleDeprecations} from './partners.js'
import {businessPlatformFqdn} from '../context/fqdn.js'
import {TypedDocumentNode} from '@graphql-typed-document-node/core'
Expand Down Expand Up @@ -27,17 +27,21 @@ async function setupRequest(token: string) {
* @param query - GraphQL query to execute.
* @param token - Business Platform token.
* @param variables - GraphQL variables to pass to the query.
* @param cacheOptions - Cache options for the request. If not present, the request will not be cached.
* @returns The response of the query of generic type <T>.
*/
export async function businessPlatformRequest<T>(
query: string,
token: string,
variables?: GraphQLVariables,
cacheOptions?: CacheOptions,
): Promise<T> {
return graphqlRequest<T>({
...(await setupRequest(token)),
query,
variables,
cacheTTL: cacheOptions?.cacheTTL,
cacheExtraKey: cacheOptions?.cacheExtraKey,
})
}

Expand All @@ -47,17 +51,21 @@ export async function businessPlatformRequest<T>(
* @param query - GraphQL query to execute.
* @param token - Business Platform token.
* @param variables - GraphQL variables to pass to the query.
* @param cacheOptions - Cache options for the request. If not present, the request will not be cached.
* @returns The response of the query of generic type <TResult>.
*/
export async function businessPlatformRequestDoc<TResult, TVariables extends Variables>(
query: TypedDocumentNode<TResult, TVariables>,
token: string,
variables?: TVariables,
cacheOptions?: CacheOptions,
): Promise<TResult> {
return graphqlRequestDoc<TResult, TVariables>({
...(await setupRequest(token)),
query,
variables,
cacheTTL: cacheOptions?.cacheTTL,
cacheExtraKey: cacheOptions?.cacheExtraKey,
})
}

Expand Down
77 changes: 77 additions & 0 deletions packages/cli-kit/src/public/node/api/graphql.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as debugRequest from '../../../private/node/api/graphql.js'
import {buildHeaders} from '../../../private/node/api/headers.js'
import {requestIdsCollection} from '../../../private/node/request-ids.js'
import * as metadata from '../metadata.js'
import {cacheRetrieveOrRepopulate} from '../../../private/node/conf-store.js'
import {GraphQLClient} from 'graphql-request'
import {test, vi, describe, expect, beforeEach} from 'vitest'
import {TypedDocumentNode} from '@graphql-typed-document-node/core'
Expand All @@ -29,6 +30,10 @@ vi.mock('graphql-request', async () => {
})
vi.spyOn(debugRequest, 'debugLogRequestInfo').mockResolvedValue(undefined)

vi.mock('../../../private/node/conf-store.js', () => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible to test without the mock? Maybe if we could set the file system location?

cacheRetrieveOrRepopulate: vi.fn(),
}))

const mockedAddress = 'http://localhost:3000'
const mockVariables = {some: 'variables'}
const mockToken = 'token'
Expand Down Expand Up @@ -149,3 +154,75 @@ describe('graphqlRequestDoc', () => {
)
})
})

describe('graphqlRequest with caching', () => {
test('uses cache when TTL is provided', async () => {
// Given
const mockQueryHash = '84f38895-31ed-05b5-0d7b-dbf2f1eecd46e2580db0'
const mockVariablesHash = 'e6959ad8-4a7c-c23d-e7b8-be1ae774e05751514949'

vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({data: 'cached-response'}))

// When
await graphqlRequest({
query: 'query',
api: 'mockApi',
url: mockedAddress,
token: mockToken,
variables: mockVariables,
cacheTTL: '1h',
cacheExtraKey: 'extra',
})

// Then
expect(cacheRetrieveOrRepopulate).toHaveBeenCalledWith(
`q-${mockQueryHash}-${mockVariablesHash}-extra`,
expect.any(Function),
1000 * 60 * 60,
)
})

test('uses cache key when no extra key provided', async () => {
// Given
const mockQueryHash = '84f38895-31ed-05b5-0d7b-dbf2f1eecd46e2580db0'
const mockVariablesHash = 'e6959ad8-4a7c-c23d-e7b8-be1ae774e05751514949'

vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({data: 'cached-response'}))

// When
await graphqlRequest({
query: 'query',
api: 'mockApi',
url: mockedAddress,
token: mockToken,
variables: mockVariables,
cacheTTL: '1d',
})

// Then
expect(cacheRetrieveOrRepopulate).toHaveBeenCalledWith(
`q-${mockQueryHash}-${mockVariablesHash}-`,
expect.any(Function),
1000 * 60 * 60 * 24,
)
})

test('skips cache when no TTL is provided', async () => {
const retryAwareSpy = vi
.spyOn(api, 'retryAwareRequest')
.mockImplementation(async () => ({status: 200, headers: new Headers()}))

// When
await graphqlRequest({
query: 'query',
api: 'mockApi',
url: mockedAddress,
token: mockToken,
variables: mockVariables,
})

// Then
expect(cacheRetrieveOrRepopulate).not.toHaveBeenCalled()
expect(retryAwareSpy).toHaveBeenCalled()
})
})
79 changes: 68 additions & 11 deletions packages/cli-kit/src/public/node/api/graphql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import {debugLogRequestInfo, errorHandler} from '../../../private/node/api/graph
import {addPublicMetadata, runWithTimer} from '../metadata.js'
import {retryAwareRequest} from '../../../private/node/api.js'
import {requestIdsCollection} from '../../../private/node/request-ids.js'
import {nonRandomUUID} from '../crypto.js'
import {cacheRetrieveOrRepopulate, GraphQLRequestKey} from '../../../private/node/conf-store.js'
import {
GraphQLClient,
rawRequest,
Expand All @@ -23,12 +25,20 @@ export interface GraphQLVariables {

export type GraphQLResponse<T> = Awaited<ReturnType<typeof rawRequest<T>>>

export interface CacheOptions {
cacheTTL?: CacheTTL
cacheExtraKey?: string
}
export type CacheTTL = '1h' | '6h' | '12h' | '1d' | '3d' | '7d' | '14d' | '30d'

interface GraphQLRequestBaseOptions<TResult> {
api: string
url: string
token?: string
addedHeaders?: {[header: string]: string}
responseOptions?: GraphQLResponseOptions<TResult>
cacheTTL?: CacheTTL
Copy link
Contributor

Choose a reason for hiding this comment

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

base on the if below I think this is really

responseOptions?: GraphQLResponseOptions<TResult>
} & (
  {} |
  {cacheTTL: CacheTTL, cacheExtraKey?: string}
)

(we only use extra key & caching in general if TTL provided)

cacheExtraKey?: string
}

type PerformGraphQLRequestOptions<TResult> = GraphQLRequestBaseOptions<TResult> & {
Expand Down Expand Up @@ -88,19 +98,43 @@ async function performGraphQLRequest<TResult>(options: PerformGraphQLRequestOpti
}
}

return runWithTimer('cmd_all_timing_network_ms')(async () => {
const response = await retryAwareRequest(
{request: performRequest, url},
responseOptions?.handleErrors === false ? undefined : errorHandler(api),
unauthorizedHandler,
)
const executeWithTimer = () =>
runWithTimer('cmd_all_timing_network_ms')(async () => {
const response = await retryAwareRequest(
{request: performRequest, url},
responseOptions?.handleErrors === false ? undefined : errorHandler(api),
unauthorizedHandler,
)

if (responseOptions?.onResponse) {
responseOptions.onResponse(response)
}
if (responseOptions?.onResponse) {
responseOptions.onResponse(response)
}

return response.data
})
return response.data
})

const {cacheTTL, cacheExtraKey} = options

// If there is no cache config for this query, just execute it and return the result.
if (cacheTTL === undefined) {
return executeWithTimer()
}

// The cache key is a combination of the hashed query and variables, with an optional extra key provided by the user.
const queryHash = nonRandomUUID(queryAsString)
const variablesHash = nonRandomUUID(JSON.stringify(variables ?? {}))
const cacheKey: GraphQLRequestKey = `q-${queryHash}-${variablesHash}-${cacheExtraKey ?? ''}`
Copy link
Contributor

Choose a reason for hiding this comment

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

add the CLI version? what do you think?


const result = await cacheRetrieveOrRepopulate(
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is an existing function. But where are these being stored? Should they go into a sub-folder in .shopify? Or some typical system location? etc. -- mainly thinking about debugging. If I have a weird problem, finding and deleting my cache feels like a reasonable step.

cacheKey,
async () => {
const result = await executeWithTimer()
return JSON.stringify(result)
},
cacheTTLToMs(cacheTTL),
)

return JSON.parse(result) as TResult
}

async function logLastRequestIdFromResponse(response: GraphQLResponse<unknown>) {
Expand Down Expand Up @@ -143,3 +177,26 @@ export async function graphqlRequestDoc<TResult, TVariables extends Variables>(
queryAsString: resolveRequestDocument(options.query).query,
})
}

function cacheTTLToMs(cacheTTL: CacheTTL) {
const oneHour = 1000 * 60 * 60
const oneDay = 1000 * 60 * 60 * 24
switch (cacheTTL) {
case '1h':
return oneHour
case '6h':
return oneHour * 6
case '12h':
return oneHour * 12
case '1d':
return oneDay
case '3d':
return oneDay * 3
case '7d':
return oneDay * 7
case '14d':
return oneDay * 14
case '30d':
return oneDay * 30
}
}
Loading