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

Conversation

isaacroldan
Copy link
Contributor

@isaacroldan isaacroldan commented Feb 3, 2025

WHY are these changes introduced?

Improve performance by adding caching capabilities to GraphQL requests in the CLI.

WHAT is this pull request doing?

Adds caching support for GraphQL requests with configurable TTL options ranging from 1 hour to 30 days. The cache implementation:

  • Adds cacheTTL and cacheExtraKey parameters to GraphQL request functions
  • Implements cache key generation using query and variable hashing
  • Provides predefined TTL options (1h, 6h, 12h, 1d, 3d, 7d, 14d, 30d)

How to test your changes?

You'd need test in dev mode and adding some logs/breakpoints:
(or just test everything in the next PR in the stack that already enables Caching for some queries)

  1. Modify a GraphQL request to enable caching:
await graphqlRequest({
  query: "...",
  token: "...",
  {
    cacheTTL: "1h",
    cacheExtraKey: "optional-key"
  }  
})
  1. Make the same request again within the TTL period
  2. Verify that the second request returns cached data without making a network call

NOTE: The cache is not enabled right now for any existing query, that will happen in the following PR

Measuring impact

  • Existing analytics will cater for this addition (network timing metrics will reflect cached responses)

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Copy link
Contributor Author

isaacroldan commented Feb 3, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@isaacroldan isaacroldan changed the title Add generic cache support for any graphQL query Add generic cache for any graphQL query (as opt-in per query) Feb 3, 2025
@isaacroldan isaacroldan changed the title Add generic cache for any graphQL query (as opt-in per query) Add generic cache for graphQL querys (as opt-in per query) Feb 3, 2025
Copy link
Contributor

github-actions bot commented Feb 3, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.43% (-0.02% 🔻)
9025/11965
🟡 Branches
70.59% (-0.05% 🔻)
4402/6236
🟡 Functions
75.25% (-0.01% 🔻)
2366/3144
🟡 Lines
75.95% (-0.02% 🔻)
8527/11227
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
95.18% (-1.2% 🔻)
86.49% (-2.7% 🔻)
95.45% 100%
🟡
... / graphql.ts
74.47% (-9.53% 🔻)
45.45% (+7.95% 🔼)
90% (-10% 🔻)
73.91% (-9.42% 🔻)

Test suite run success

2038 tests passing in 910 suites.

Report generated by 🧪jest coverage report action from c7d964b

@isaacroldan isaacroldan force-pushed the graphql-cache branch 2 times, most recently from 5f488a1 to 162fead Compare February 3, 2025 15:48
Copy link
Contributor

github-actions bot commented Feb 3, 2025

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/private/node/conf-store.d.ts
@@ -7,15 +7,17 @@ export type IntrospectionUrlKey = ;
 export type PackageVersionKey = ;
 export type NotificationsKey = ;
 export type NotificationKey = ;
+export type GraphQLRequestKey = ;
 type MostRecentOccurrenceKey = ;
 type RateLimitKey = ;
-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[]>;
 }
 export interface ConfSchema {
packages/cli-kit/dist/public/node/api/app-management.d.ts
@@ -1,4 +1,4 @@
-import { GraphQLResponse } from './graphql.js';
+import { CacheOptions, GraphQLResponse } from './graphql.js';
 import { TypedDocumentNode } from '@graphql-typed-document-node/core';
 import { Variables } from 'graphql-request';
 /**
@@ -8,9 +8,10 @@ import { Variables } from 'graphql-request';
  * @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 declare function appManagementRequestDoc<TResult, TVariables extends Variables>(orgId: string, query: TypedDocumentNode<TResult, TVariables>, token: string, variables?: TVariables): Promise<TResult>;
+export declare function appManagementRequestDoc<TResult, TVariables extends Variables>(orgId: string, query: TypedDocumentNode<TResult, TVariables>, token: string, variables?: TVariables, cacheOptions?: CacheOptions): Promise<TResult>;
 /**
  * Sets the next deprecation date from [GraphQL response extensions](https://www.apollographql.com/docs/resources/graphql-glossary/#extensions)
  * if  objects contain a  (ISO 8601-formatted string).
packages/cli-kit/dist/public/node/api/business-platform.d.ts
@@ -1,4 +1,4 @@
-import { Exact, GraphQLVariables } from './graphql.js';
+import { CacheOptions, Exact, GraphQLVariables } from './graphql.js';
 import { TypedDocumentNode } from '@graphql-typed-document-node/core';
 import { Variables } from 'graphql-request';
 /**
@@ -7,18 +7,20 @@ import { Variables } from 'graphql-request';
  * @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 declare function businessPlatformRequest<T>(query: string, token: string, variables?: GraphQLVariables): Promise<T>;
+export declare function businessPlatformRequest<T>(query: string, token: string, variables?: GraphQLVariables, cacheOptions?: CacheOptions): Promise<T>;
 /**
  * Executes a GraphQL query against the Business Platform Destinations API. Uses typed documents.
  *
  * @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 declare function businessPlatformRequestDoc<TResult, TVariables extends Variables>(query: TypedDocumentNode<TResult, TVariables>, token: string, variables?: TVariables): Promise<TResult>;
+export declare function businessPlatformRequestDoc<TResult, TVariables extends Variables>(query: TypedDocumentNode<TResult, TVariables>, token: string, variables?: TVariables, cacheOptions?: CacheOptions): Promise<TResult>;
 /**
  * Executes a GraphQL query against the Business Platform Organizations API.
  *
packages/cli-kit/dist/public/node/api/graphql.d.ts
@@ -9,6 +9,11 @@ export interface GraphQLVariables {
     [key: string]: any;
 }
 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;
@@ -17,6 +22,8 @@ interface GraphQLRequestBaseOptions<TResult> {
         [header: string]: string;
     };
     responseOptions?: GraphQLResponseOptions<TResult>;
+    cacheTTL?: CacheTTL;
+    cacheExtraKey?: string;
 }
 export type GraphQLRequestOptions<T> = GraphQLRequestBaseOptions<T> & {
     query: RequestDocument;

@isaacroldan isaacroldan marked this pull request as ready for review February 3, 2025 16:26
@isaacroldan isaacroldan requested a review from a team as a code owner February 3, 2025 16:26
Copy link
Contributor

github-actions bot commented Feb 3, 2025

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

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)

// 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 variablesHash = nonRandomUUID(JSON.stringify(variables ?? {}))
const cacheKey: GraphQLRequestKey = `q-${queryHash}-${variablesHash}-${cacheExtraKey ?? ''}`

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.

@@ -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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants