From c062cb636fe04cb6ca261a6197121194362d16cd Mon Sep 17 00:00:00 2001 From: Angelo Ashmore Date: Mon, 28 Oct 2024 16:39:07 -1000 Subject: [PATCH 1/7] feat: retry invalid/expired refs --- src/Client.ts | 37 +++++++++- test/__testutils__/testInvalidRefRetry.ts | 88 +++++++++++++++++++++++ test/client-get.test.ts | 5 ++ 3 files changed, 129 insertions(+), 1 deletion(-) create mode 100644 test/__testutils__/testInvalidRefRetry.ts diff --git a/src/Client.ts b/src/Client.ts index 22fcd565..dd0b31b5 100644 --- a/src/Client.ts +++ b/src/Client.ts @@ -261,6 +261,12 @@ export class Client< > extends BaseClient { #repositoryName: string | undefined + /** + * Keep a list of all refs we retried so we know which refs not to retry + * again. We should only retry once per ref. + */ + #retriedRefs = new Set() + /** * The Prismic repository's name. */ @@ -531,7 +537,36 @@ export class Client< ): Promise> { const url = await this.buildQueryURL(params) - return await this.fetch>(url, params) + try { + return await this.fetch>(url, params) + } catch (error) { + if (!(error instanceof RefNotFoundError)) { + throw error + } + + const latestRef = error.message.match(/Master ref is: (?.*)$/) + ?.groups?.ref + if (!latestRef) { + throw error + } + + if (this.#retriedRefs.has(latestRef)) { + // We only allow retrying a ref once. We'll + // likely get the same response on future + // retries and don't want to get stuck in a + // loop. + throw error + } + + this.#retriedRefs.add(latestRef) + + const invalidRef = new URL(url).searchParams.get("ref") + console.warn( + `The ref (${invalidRef}) was invalid or expired. Now retrying with the latest master ref (${latestRef}). If you were previewing content, the response will not include draft content.`, + ) + + return await this.get({ ...params, ref: latestRef }) + } } /** diff --git a/test/__testutils__/testInvalidRefRetry.ts b/test/__testutils__/testInvalidRefRetry.ts new file mode 100644 index 00000000..fc3192e7 --- /dev/null +++ b/test/__testutils__/testInvalidRefRetry.ts @@ -0,0 +1,88 @@ +import { expect, it, vi } from "vitest" + +import { rest } from "msw" + +import { createTestClient } from "./createClient" +import { getMasterRef } from "./getMasterRef" +import { mockPrismicRestAPIV2 } from "./mockPrismicRestAPIV2" + +import * as prismic from "../../src" + +type TestInvalidRefRetryArgs = { + run: ( + client: prismic.Client, + params?: Parameters[0], + ) => Promise +} + +export const testInvalidRefRetry = ( + description: string, + args: TestInvalidRefRetryArgs, +): void => { + it.concurrent(description, async (ctx) => { + const client = createTestClient({ ctx }) + + const triedRefs: string[] = [] + + const repositoryResponse = ctx.mock.api.repository() + repositoryResponse.refs = [ctx.mock.api.ref({ isMasterRef: true })] + + const latestRef = ctx.mock.api.ref().ref + + mockPrismicRestAPIV2({ ctx, repositoryResponse }) + + const queryEndpoint = new URL( + "documents/search", + `${client.documentAPIEndpoint}/`, + ).toString() + + ctx.server.use( + rest.get(queryEndpoint, (req, res, ctx) => { + const ref = req.url.searchParams.get("ref") + if (ref) { + triedRefs.push(ref) + } + + if (triedRefs.length <= 1) { + return res( + ctx.status(404), + ctx.json({ + type: "api_notfound_error", + message: `Ref not found. Ensure you have the correct ref and try again. Master ref is: ${latestRef}`, + }), + ) + } + }), + ) + + const consoleWarnSpy = vi + .spyOn(console, "warn") + .mockImplementation(() => void 0) + + await args.run(client) + + expect(triedRefs).toStrictEqual([ + getMasterRef(repositoryResponse), + latestRef, + ]) + + // Check that refs are not retried more than once. + ctx.server.use( + rest.get(queryEndpoint, (_req, res, ctx) => { + return res( + ctx.status(404), + ctx.json({ + type: "api_notfound_error", + message: `Ref not found. Ensure you have the correct ref and try again. Master ref is: ${triedRefs[0]}`, + }), + ) + }), + ) + + await expect(async () => { + await args.run(client) + }).rejects.toThrow(prismic.RefNotFoundError) + + consoleWarnSpy.mockRestore() + }) +} diff --git a/test/client-get.test.ts b/test/client-get.test.ts index 61893155..d3db5f57 100644 --- a/test/client-get.test.ts +++ b/test/client-get.test.ts @@ -7,6 +7,7 @@ import { mockPrismicRestAPIV2 } from "./__testutils__/mockPrismicRestAPIV2" import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetMethod } from "./__testutils__/testAnyGetMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetMethod("resolves a query", { run: (client) => client.get(), @@ -92,6 +93,10 @@ it("uses cached repository metadata within the client's repository cache TTL", a ) }) +testInvalidRefRetry("retries on expired refs", { + run: (client, params) => client.get(params), +}) + testFetchOptions("supports fetch options", { run: (client, params) => client.get(params), }) From fc131d11e82cee32cc2d0357330f4785ea7c9487 Mon Sep 17 00:00:00 2001 From: Angelo Ashmore Date: Tue, 29 Oct 2024 15:10:37 -1000 Subject: [PATCH 2/7] fix: remove duplicate ref retry logic and support `getFirst` --- src/Client.ts | 97 +++++++++++++++++++++++++++++---------------------- 1 file changed, 55 insertions(+), 42 deletions(-) diff --git a/src/Client.ts b/src/Client.ts index dd0b31b5..0d8b54ac 100644 --- a/src/Client.ts +++ b/src/Client.ts @@ -261,12 +261,6 @@ export class Client< > extends BaseClient { #repositoryName: string | undefined - /** - * Keep a list of all refs we retried so we know which refs not to retry - * again. We should only retry once per ref. - */ - #retriedRefs = new Set() - /** * The Prismic repository's name. */ @@ -535,38 +529,9 @@ export class Client< async get( params?: Partial & FetchParams, ): Promise> { - const url = await this.buildQueryURL(params) - - try { - return await this.fetch>(url, params) - } catch (error) { - if (!(error instanceof RefNotFoundError)) { - throw error - } - - const latestRef = error.message.match(/Master ref is: (?.*)$/) - ?.groups?.ref - if (!latestRef) { - throw error - } + const { data } = await this._get(params) - if (this.#retriedRefs.has(latestRef)) { - // We only allow retrying a ref once. We'll - // likely get the same response on future - // retries and don't want to get stuck in a - // loop. - throw error - } - - this.#retriedRefs.add(latestRef) - - const invalidRef = new URL(url).searchParams.get("ref") - console.warn( - `The ref (${invalidRef}) was invalid or expired. Now retrying with the latest master ref (${latestRef}). If you were previewing content, the response will not include draft content.`, - ) - - return await this.get({ ...params, ref: latestRef }) - } + return data } /** @@ -581,8 +546,9 @@ export class Client< * * @typeParam TDocument - Type of the Prismic document returned. * - * @param params - Parameters to filter, sort, and paginate results. @returns - * The first result of the query, if any. + * @param params - Parameters to filter, sort, and paginate results. + * + * @returns The first result of the query, if any. */ async getFirst( params?: Partial & FetchParams, @@ -591,10 +557,9 @@ export class Client< if (!(params && params.page) && !params?.pageSize) { actualParams.pageSize = this.defaultParams?.pageSize ?? 1 } - const url = await this.buildQueryURL(actualParams) - const result = await this.fetch>(url, params) + const { data, url } = await this._get(actualParams) - const firstResult = result.results[0] + const firstResult = data.results[0] if (firstResult) { return firstResult @@ -1713,6 +1678,54 @@ export class Client< return findMasterRef(cachedRepository.refs).ref } + /** + * The private implementation of `this.get`. It returns the API response and + * the URL used to make the request. The URL is sometimes used in the public + * method to include in thrown errors. + * + * This method retries requests that throw `RefNotFoundError` or + * `RefExpiredError`. It contains special logic to retry with the latest + * master ref, provided in the API's error message. + * + * @typeParam TDocument - Type of Prismic documents returned. + * + * @param params - Parameters to filter, sort, and paginate results. + * + * @returns An object containing the paginated response containing the result + * of the query and the URL used to make the API request. + */ + private async _get( + params?: Partial & FetchParams, + ): Promise<{ data: Query; url: string }> { + const url = await this.buildQueryURL(params) + + try { + const data = await this.fetch>(url, params) + + return { data, url } + } catch (error) { + if ( + !(error instanceof RefNotFoundError || error instanceof RefExpiredError) + ) { + throw error + } + + const masterRef = error.message.match(/Master ref is: (?.*)$/) + ?.groups?.ref + if (!masterRef) { + throw error + } + + const badRef = new URL(url).searchParams.get("ref") + const issue = error instanceof RefNotFoundError ? "invalid" : "expired" + console.warn( + `The ref (${badRef}) was ${issue}. Now retrying with the latest master ref (${masterRef}). If you were previewing content, the response will not include draft content.`, + ) + + return await this._get({ ...params, ref: masterRef }) + } + } + /** * Performs a network request using the configured `fetch` function. It * assumes all successful responses will have a JSON content type. It also From d10ebe6d8fe3f5d5461e3e565d8d4c93370e60ba Mon Sep 17 00:00:00 2001 From: Angelo Ashmore Date: Tue, 29 Oct 2024 15:10:55 -1000 Subject: [PATCH 3/7] test: invalid ref retry --- test/__testutils__/testInvalidRefRetry.ts | 148 ++++++++++++---------- test/client-get.test.ts | 2 +- test/client-getAllByEveryTag.test.ts | 5 + test/client-getAllByIDs.test.ts | 5 + test/client-getAllBySomeTags.test.ts | 5 + test/client-getAllByTag.test.ts | 5 + test/client-getAllByType.test.ts | 5 + test/client-getAllByUIDs.test.ts | 6 + test/client-getByEveryTag.test.ts | 5 + test/client-getByID.test.ts | 5 + test/client-getByIDs.test.ts | 5 + test/client-getBySomeTags.test.ts | 5 + test/client-getByTag.test.ts | 5 + test/client-getByType.test.ts | 5 + test/client-getByUID.test.ts | 5 + test/client-getByUIDs.test.ts | 5 + test/client-getFirst.test.ts | 5 + test/client-getSingle.test.ts | 5 + 18 files changed, 161 insertions(+), 70 deletions(-) diff --git a/test/__testutils__/testInvalidRefRetry.ts b/test/__testutils__/testInvalidRefRetry.ts index fc3192e7..05c428b2 100644 --- a/test/__testutils__/testInvalidRefRetry.ts +++ b/test/__testutils__/testInvalidRefRetry.ts @@ -3,10 +3,9 @@ import { expect, it, vi } from "vitest" import { rest } from "msw" import { createTestClient } from "./createClient" -import { getMasterRef } from "./getMasterRef" import { mockPrismicRestAPIV2 } from "./mockPrismicRestAPIV2" -import * as prismic from "../../src" +import type * as prismic from "../../src" type TestInvalidRefRetryArgs = { run: ( @@ -15,74 +14,85 @@ type TestInvalidRefRetryArgs = { ) => Promise } -export const testInvalidRefRetry = ( - description: string, - args: TestInvalidRefRetryArgs, -): void => { - it.concurrent(description, async (ctx) => { - const client = createTestClient({ ctx }) - - const triedRefs: string[] = [] - - const repositoryResponse = ctx.mock.api.repository() - repositoryResponse.refs = [ctx.mock.api.ref({ isMasterRef: true })] - - const latestRef = ctx.mock.api.ref().ref - - mockPrismicRestAPIV2({ ctx, repositoryResponse }) - - const queryEndpoint = new URL( - "documents/search", - `${client.documentAPIEndpoint}/`, - ).toString() - - ctx.server.use( - rest.get(queryEndpoint, (req, res, ctx) => { - const ref = req.url.searchParams.get("ref") - if (ref) { - triedRefs.push(ref) - } - - if (triedRefs.length <= 1) { - return res( - ctx.status(404), +export const testInvalidRefRetry = (args: TestInvalidRefRetryArgs): void => { + it.concurrent( + "retries with the master ref when an invalid ref is used", + async (ctx) => { + const client = createTestClient({ ctx }) + const badRef = ctx.mock.api.ref().ref + const masterRef = ctx.mock.api.ref().ref + const queryResponse = ctx.mock.api.query({ + documents: [ctx.mock.value.document()], + }) + + const triedRefs: (string | null)[] = [] + + mockPrismicRestAPIV2({ ctx, queryResponse }) + const endpoint = new URL( + "documents/search", + `${client.documentAPIEndpoint}/`, + ).toString() + ctx.server.use( + rest.get(endpoint, (req) => { + triedRefs.push(req.url.searchParams.get("ref")) + }), + rest.get(endpoint, (_req, res, ctx) => + res.once( ctx.json({ type: "api_notfound_error", - message: `Ref not found. Ensure you have the correct ref and try again. Master ref is: ${latestRef}`, + message: `Master ref is: ${masterRef}`, }), - ) - } - }), - ) - - const consoleWarnSpy = vi - .spyOn(console, "warn") - .mockImplementation(() => void 0) - - await args.run(client) - - expect(triedRefs).toStrictEqual([ - getMasterRef(repositoryResponse), - latestRef, - ]) - - // Check that refs are not retried more than once. - ctx.server.use( - rest.get(queryEndpoint, (_req, res, ctx) => { - return res( - ctx.status(404), - ctx.json({ - type: "api_notfound_error", - message: `Ref not found. Ensure you have the correct ref and try again. Master ref is: ${triedRefs[0]}`, - }), - ) - }), - ) - - await expect(async () => { - await args.run(client) - }).rejects.toThrow(prismic.RefNotFoundError) - - consoleWarnSpy.mockRestore() - }) + ctx.status(404), + ), + ), + ) + + const consoleWarnSpy = vi + .spyOn(console, "warn") + .mockImplementation(() => void 0) + await args.run(client, { ref: badRef }) + consoleWarnSpy.mockRestore() + + expect(triedRefs).toStrictEqual([badRef, masterRef]) + }, + ) + + it.concurrent( + "retries with the master ref when an expired ref is used", + async (ctx) => { + const client = createTestClient({ ctx }) + const badRef = ctx.mock.api.ref().ref + const masterRef = ctx.mock.api.ref().ref + const queryResponse = ctx.mock.api.query({ + documents: [ctx.mock.value.document()], + }) + + const triedRefs: (string | null)[] = [] + + mockPrismicRestAPIV2({ ctx, queryResponse }) + const endpoint = new URL( + "documents/search", + `${client.documentAPIEndpoint}/`, + ).toString() + ctx.server.use( + rest.get(endpoint, (req) => { + triedRefs.push(req.url.searchParams.get("ref")) + }), + rest.get(endpoint, (_req, res, ctx) => + res.once( + ctx.json({ message: `Master ref is: ${masterRef}` }), + ctx.status(410), + ), + ), + ) + + const consoleWarnSpy = vi + .spyOn(console, "warn") + .mockImplementation(() => void 0) + await args.run(client, { ref: badRef }) + consoleWarnSpy.mockRestore() + + expect(triedRefs).toStrictEqual([badRef, masterRef]) + }, + ) } diff --git a/test/client-get.test.ts b/test/client-get.test.ts index d3db5f57..ecadaa38 100644 --- a/test/client-get.test.ts +++ b/test/client-get.test.ts @@ -93,7 +93,7 @@ it("uses cached repository metadata within the client's repository cache TTL", a ) }) -testInvalidRefRetry("retries on expired refs", { +testInvalidRefRetry({ run: (client, params) => client.get(params), }) diff --git a/test/client-getAllByEveryTag.test.ts b/test/client-getAllByEveryTag.test.ts index ffca2b71..73a45ce2 100644 --- a/test/client-getAllByEveryTag.test.ts +++ b/test/client-getAllByEveryTag.test.ts @@ -2,6 +2,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetAllMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetAllMethod("returns all documents by every tag from paginated response", { run: (client) => client.getAllByEveryTag(["foo", "bar"]), @@ -29,6 +30,10 @@ testFetchOptions("supports fetch options", { run: (client, params) => client.getAllByEveryTag(["foo", "bar"], params), }) +testInvalidRefRetry({ + run: (client, params) => client.getAllByEveryTag(["foo", "bar"], params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getAllByEveryTag(["foo", "bar"], params), }) diff --git a/test/client-getAllByIDs.test.ts b/test/client-getAllByIDs.test.ts index fbe027a3..44fd5e73 100644 --- a/test/client-getAllByIDs.test.ts +++ b/test/client-getAllByIDs.test.ts @@ -2,6 +2,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetAllMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetAllMethod("returns all documents by IDs from paginated response", { run: (client) => client.getAllByIDs(["id1", "id2"]), @@ -29,6 +30,10 @@ testFetchOptions("supports fetch options", { run: (client, params) => client.getAllByIDs(["id1", "id2"], params), }) +testInvalidRefRetry({ + run: (client, params) => client.getAllByIDs(["id1", "id2"], params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getAllByIDs(["id1", "id2"], params), }) diff --git a/test/client-getAllBySomeTags.test.ts b/test/client-getAllBySomeTags.test.ts index 002f92b0..e13ca5cd 100644 --- a/test/client-getAllBySomeTags.test.ts +++ b/test/client-getAllBySomeTags.test.ts @@ -2,6 +2,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetAllMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetAllMethod("returns all documents by some tags from paginated response", { run: (client) => client.getAllBySomeTags(["foo", "bar"]), @@ -29,6 +30,10 @@ testFetchOptions("supports fetch options", { run: (client, params) => client.getAllBySomeTags(["foo", "bar"], params), }) +testInvalidRefRetry({ + run: (client, params) => client.getAllBySomeTags(["foo", "bar"], params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getAllBySomeTags(["foo", "bar"], params), }) diff --git a/test/client-getAllByTag.test.ts b/test/client-getAllByTag.test.ts index d2b81bd4..488afdac 100644 --- a/test/client-getAllByTag.test.ts +++ b/test/client-getAllByTag.test.ts @@ -2,6 +2,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetAllMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetAllMethod("returns all documents by tag from paginated response", { run: (client) => client.getAllByTag("tag"), @@ -29,6 +30,10 @@ testFetchOptions("supports fetch options", { run: (client, params) => client.getAllByTag("tag", params), }) +testInvalidRefRetry({ + run: (client, params) => client.getAllByTag("tag", params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getAllByTag("tag", params), }) diff --git a/test/client-getAllByType.test.ts b/test/client-getAllByType.test.ts index 2af6e770..d1045604 100644 --- a/test/client-getAllByType.test.ts +++ b/test/client-getAllByType.test.ts @@ -2,6 +2,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetAllMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetAllMethod("returns all documents by type from paginated response", { run: (client) => client.getAllByType("type"), @@ -29,6 +30,10 @@ testFetchOptions("supports fetch options", { run: (client, params) => client.getAllByType("type", params), }) +testInvalidRefRetry({ + run: (client, params) => client.getAllByType("type", params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getAllByType("type", params), }) diff --git a/test/client-getAllByUIDs.test.ts b/test/client-getAllByUIDs.test.ts index c1ddb21b..9d6d2b7d 100644 --- a/test/client-getAllByUIDs.test.ts +++ b/test/client-getAllByUIDs.test.ts @@ -2,6 +2,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetAllMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetAllMethod("returns all documents by UIDs from paginated response", { run: (client) => client.getAllByUIDs("type", ["uid1", "uid2"]), @@ -36,6 +37,11 @@ testFetchOptions("supports fetch options", { client.getAllByUIDs("type", ["uid1", "uid2"], params), }) +testInvalidRefRetry({ + run: (client, params) => + client.getAllByUIDs("type", ["uid1", "uid2"], params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getAllByUIDs("type", ["uid1", "uid2"], params), diff --git a/test/client-getByEveryTag.test.ts b/test/client-getByEveryTag.test.ts index fa5b34a5..1ee3a19b 100644 --- a/test/client-getByEveryTag.test.ts +++ b/test/client-getByEveryTag.test.ts @@ -2,6 +2,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetMethod("queries for documents by tag", { run: (client) => client.getByEveryTag(["foo", "bar"]), @@ -29,6 +30,10 @@ testFetchOptions("supports fetch options", { run: (client, params) => client.getByEveryTag(["foo", "bar"], params), }) +testInvalidRefRetry({ + run: (client, params) => client.getByEveryTag(["foo", "bar"], params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getByEveryTag(["foo", "bar"], params), }) diff --git a/test/client-getByID.test.ts b/test/client-getByID.test.ts index 1914dffa..1559a9a3 100644 --- a/test/client-getByID.test.ts +++ b/test/client-getByID.test.ts @@ -2,6 +2,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetFirstMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetFirstMethod("queries for document by ID", { run: (client) => client.getByID("id"), @@ -29,6 +30,10 @@ testFetchOptions("supports fetch options", { run: (client, params) => client.getByID("id", params), }) +testInvalidRefRetry({ + run: (client, params) => client.getByID("id", params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getByID("id", params), }) diff --git a/test/client-getByIDs.test.ts b/test/client-getByIDs.test.ts index 1bbfbad9..b73e21e0 100644 --- a/test/client-getByIDs.test.ts +++ b/test/client-getByIDs.test.ts @@ -2,6 +2,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetMethod("queries for documents by IDs", { run: (client) => client.getByIDs(["id1", "id2"]), @@ -29,6 +30,10 @@ testFetchOptions("supports fetch options", { run: (client, params) => client.getByIDs(["id1", "id2"], params), }) +testInvalidRefRetry({ + run: (client, params) => client.getByIDs(["id1", "id2"], params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getByIDs(["id1", "id2"], params), }) diff --git a/test/client-getBySomeTags.test.ts b/test/client-getBySomeTags.test.ts index b90922be..7adef377 100644 --- a/test/client-getBySomeTags.test.ts +++ b/test/client-getBySomeTags.test.ts @@ -2,6 +2,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetMethod("queries for documents by some tags", { run: (client) => client.getBySomeTags(["foo", "bar"]), @@ -29,6 +30,10 @@ testFetchOptions("supports fetch options", { run: (client, params) => client.getBySomeTags(["foo", "bar"], params), }) +testInvalidRefRetry({ + run: (client, params) => client.getBySomeTags(["foo", "bar"], params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getBySomeTags(["foo", "bar"], params), }) diff --git a/test/client-getByTag.test.ts b/test/client-getByTag.test.ts index fa6d0567..e12d19d4 100644 --- a/test/client-getByTag.test.ts +++ b/test/client-getByTag.test.ts @@ -2,6 +2,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetMethod("queries for documents by tag", { run: (client) => client.getByTag("tag"), @@ -29,6 +30,10 @@ testFetchOptions("supports fetch options", { run: (client, params) => client.getByTag("tag", params), }) +testInvalidRefRetry({ + run: (client, params) => client.getByTag("tag", params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getByTag("tag", params), }) diff --git a/test/client-getByType.test.ts b/test/client-getByType.test.ts index 9ef415b2..42cf032c 100644 --- a/test/client-getByType.test.ts +++ b/test/client-getByType.test.ts @@ -2,6 +2,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetMethod("queries for documents by type", { run: (client) => client.getByType("type"), @@ -29,6 +30,10 @@ testFetchOptions("supports fetch options", { run: (client, params) => client.getByType("type", params), }) +testInvalidRefRetry({ + run: (client, params) => client.getByType("type", params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getByType("type", params), }) diff --git a/test/client-getByUID.test.ts b/test/client-getByUID.test.ts index 27fa358a..4f4a49c8 100644 --- a/test/client-getByUID.test.ts +++ b/test/client-getByUID.test.ts @@ -2,6 +2,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetFirstMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetFirstMethod("queries for document by UID", { run: (client) => client.getByUID("type", "uid"), @@ -29,6 +30,10 @@ testFetchOptions("supports fetch options", { run: (client, params) => client.getByUID("type", "uid", params), }) +testInvalidRefRetry({ + run: (client, params) => client.getByUID("type", "uid", params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getByUID("type", "uid", params), }) diff --git a/test/client-getByUIDs.test.ts b/test/client-getByUIDs.test.ts index 45ac76ac..00f89b69 100644 --- a/test/client-getByUIDs.test.ts +++ b/test/client-getByUIDs.test.ts @@ -2,6 +2,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetMethod("queries for documents by UIDs", { run: (client) => client.getByUIDs("type", ["uid1", "uid2"]), @@ -35,6 +36,10 @@ testFetchOptions("supports fetch options", { run: (client, params) => client.getByUIDs("type", ["uid1", "uid2"], params), }) +testInvalidRefRetry({ + run: (client, params) => client.getByUIDs("type", ["uid1", "uid2"], params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getByUIDs("type", ["uid1", "uid2"], params), }) diff --git a/test/client-getFirst.test.ts b/test/client-getFirst.test.ts index 48634f67..8235233b 100644 --- a/test/client-getFirst.test.ts +++ b/test/client-getFirst.test.ts @@ -8,6 +8,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetFirstMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" import * as prismic from "../src" @@ -98,6 +99,10 @@ testFetchOptions("supports fetch options", { run: (client, params) => client.getFirst(params), }) +testInvalidRefRetry({ + run: (client, params) => client.getFirst(params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getFirst(params), }) diff --git a/test/client-getSingle.test.ts b/test/client-getSingle.test.ts index 8e123578..b1fa3f51 100644 --- a/test/client-getSingle.test.ts +++ b/test/client-getSingle.test.ts @@ -2,6 +2,7 @@ import { testAbortableMethod } from "./__testutils__/testAbortableMethod" import { testGetFirstMethod } from "./__testutils__/testAnyGetMethod" import { testConcurrentMethod } from "./__testutils__/testConcurrentMethod" import { testFetchOptions } from "./__testutils__/testFetchOptions" +import { testInvalidRefRetry } from "./__testutils__/testInvalidRefRetry" testGetFirstMethod("queries for singleton document", { run: (client) => client.getSingle("type"), @@ -29,6 +30,10 @@ testFetchOptions("supports fetch options", { run: (client, params) => client.getSingle("type", params), }) +testInvalidRefRetry({ + run: (client, params) => client.getSingle("type", params), +}) + testAbortableMethod("is abortable with an AbortController", { run: (client, params) => client.getSingle("type", params), }) From 345ff2b414a2cc1479abee272e28a1bb30bd0a32 Mon Sep 17 00:00:00 2001 From: Angelo Ashmore Date: Wed, 30 Oct 2024 09:33:31 -1000 Subject: [PATCH 4/7] feat: allow up to 3 retries before throwing --- src/Client.ts | 15 ++++++-- test/__testutils__/testInvalidRefRetry.ts | 44 ++++++++++++++++++++++- 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/src/Client.ts b/src/Client.ts index 0d8b54ac..b064ec2f 100644 --- a/src/Client.ts +++ b/src/Client.ts @@ -73,6 +73,13 @@ export const GET_ALL_QUERY_DELAY = 500 */ const DEFUALT_RETRY_AFTER_MS = 1000 +/** + * The maximum number of attemps to retry a query with an invalid ref before + * halting. We allow multiple attemps as each attemp may return a different ref. + * Capping the number of attemps prevents infinite loops. + */ +const MAX_INVALID_REF_RETRY_ATTEMPS = 3 + /** * Extracts one or more Prismic document types that match a given Prismic * document type. If no matches are found, no extraction is performed and the @@ -1696,6 +1703,7 @@ export class Client< */ private async _get( params?: Partial & FetchParams, + attemptCount = 0, ): Promise<{ data: Query; url: string }> { const url = await this.buildQueryURL(params) @@ -1705,7 +1713,10 @@ export class Client< return { data, url } } catch (error) { if ( - !(error instanceof RefNotFoundError || error instanceof RefExpiredError) + !( + error instanceof RefNotFoundError || error instanceof RefExpiredError + ) || + attemptCount >= MAX_INVALID_REF_RETRY_ATTEMPS - 1 ) { throw error } @@ -1722,7 +1733,7 @@ export class Client< `The ref (${badRef}) was ${issue}. Now retrying with the latest master ref (${masterRef}). If you were previewing content, the response will not include draft content.`, ) - return await this._get({ ...params, ref: masterRef }) + return await this._get({ ...params, ref: masterRef }, attemptCount + 1) } } diff --git a/test/__testutils__/testInvalidRefRetry.ts b/test/__testutils__/testInvalidRefRetry.ts index 05c428b2..8ef46c43 100644 --- a/test/__testutils__/testInvalidRefRetry.ts +++ b/test/__testutils__/testInvalidRefRetry.ts @@ -5,7 +5,7 @@ import { rest } from "msw" import { createTestClient } from "./createClient" import { mockPrismicRestAPIV2 } from "./mockPrismicRestAPIV2" -import type * as prismic from "../../src" +import * as prismic from "../../src" type TestInvalidRefRetryArgs = { run: ( @@ -95,4 +95,46 @@ export const testInvalidRefRetry = (args: TestInvalidRefRetryArgs): void => { expect(triedRefs).toStrictEqual([badRef, masterRef]) }, ) + + it.concurrent( + "throws if the maximum number of retries when an invalid ref is used is reached", + async (ctx) => { + const client = createTestClient({ ctx }) + const queryResponse = ctx.mock.api.query({ + documents: [ctx.mock.value.document()], + }) + + const triedRefs: (string | null)[] = [] + + mockPrismicRestAPIV2({ ctx, queryResponse }) + const endpoint = new URL( + "documents/search", + `${client.documentAPIEndpoint}/`, + ).toString() + ctx.server.use( + rest.get(endpoint, (req) => { + triedRefs.push(req.url.searchParams.get("ref")) + }), + rest.get(endpoint, (_req, res, requestCtx) => + res( + requestCtx.json({ + type: "api_notfound_error", + message: `Master ref is: ${ctx.mock.api.ref().ref}`, + }), + requestCtx.status(404), + ), + ), + ) + + const consoleWarnSpy = vi + .spyOn(console, "warn") + .mockImplementation(() => void 0) + await expect(async () => { + await args.run(client) + }).rejects.toThrow(prismic.RefNotFoundError) + consoleWarnSpy.mockRestore() + + expect(triedRefs.length).toBe(3) + }, + ) } From ef49bdc4ee6638971c88442146d7ddc77f089bd7 Mon Sep 17 00:00:00 2001 From: Angelo Ashmore Date: Wed, 30 Oct 2024 09:53:00 -1000 Subject: [PATCH 5/7] feat: use a new master ref once a known-stale ref is used --- src/Client.ts | 8 + test/__testutils__/testInvalidRefRetry.ts | 268 ++++++++++++---------- 2 files changed, 157 insertions(+), 119 deletions(-) diff --git a/src/Client.ts b/src/Client.ts index b064ec2f..548493f6 100644 --- a/src/Client.ts +++ b/src/Client.ts @@ -1721,6 +1721,14 @@ export class Client< throw error } + // If no explicit ref is given (i.e. the master ref from + // /api/v2 is used), clear the cached repository value. + // Clearing the cached value prevents other methods from + // using a known-stale ref. + if (!params?.ref) { + this.cachedRepository = undefined + } + const masterRef = error.message.match(/Master ref is: (?.*)$/) ?.groups?.ref if (!masterRef) { diff --git a/test/__testutils__/testInvalidRefRetry.ts b/test/__testutils__/testInvalidRefRetry.ts index 8ef46c43..9406f1eb 100644 --- a/test/__testutils__/testInvalidRefRetry.ts +++ b/test/__testutils__/testInvalidRefRetry.ts @@ -15,126 +15,156 @@ type TestInvalidRefRetryArgs = { } export const testInvalidRefRetry = (args: TestInvalidRefRetryArgs): void => { - it.concurrent( - "retries with the master ref when an invalid ref is used", - async (ctx) => { - const client = createTestClient({ ctx }) - const badRef = ctx.mock.api.ref().ref - const masterRef = ctx.mock.api.ref().ref - const queryResponse = ctx.mock.api.query({ - documents: [ctx.mock.value.document()], - }) - - const triedRefs: (string | null)[] = [] - - mockPrismicRestAPIV2({ ctx, queryResponse }) - const endpoint = new URL( - "documents/search", - `${client.documentAPIEndpoint}/`, - ).toString() - ctx.server.use( - rest.get(endpoint, (req) => { - triedRefs.push(req.url.searchParams.get("ref")) - }), - rest.get(endpoint, (_req, res, ctx) => - res.once( - ctx.json({ - type: "api_notfound_error", - message: `Master ref is: ${masterRef}`, - }), - ctx.status(404), - ), + it("retries with the master ref when an invalid ref is used", async (ctx) => { + const client = createTestClient({ ctx }) + const badRef = ctx.mock.api.ref().ref + const masterRef = ctx.mock.api.ref().ref + const queryResponse = ctx.mock.api.query({ + documents: [ctx.mock.value.document()], + }) + + const triedRefs = new Set() + + mockPrismicRestAPIV2({ ctx, queryResponse }) + const endpoint = new URL( + "documents/search", + `${client.documentAPIEndpoint}/`, + ).toString() + ctx.server.use( + rest.get(endpoint, (req) => { + triedRefs.add(req.url.searchParams.get("ref")) + }), + rest.get(endpoint, (_req, res, ctx) => + res.once( + ctx.json({ + type: "api_notfound_error", + message: `Master ref is: ${masterRef}`, + }), + ctx.status(404), ), - ) - - const consoleWarnSpy = vi - .spyOn(console, "warn") - .mockImplementation(() => void 0) - await args.run(client, { ref: badRef }) - consoleWarnSpy.mockRestore() - - expect(triedRefs).toStrictEqual([badRef, masterRef]) - }, - ) - - it.concurrent( - "retries with the master ref when an expired ref is used", - async (ctx) => { - const client = createTestClient({ ctx }) - const badRef = ctx.mock.api.ref().ref - const masterRef = ctx.mock.api.ref().ref - const queryResponse = ctx.mock.api.query({ - documents: [ctx.mock.value.document()], - }) - - const triedRefs: (string | null)[] = [] - - mockPrismicRestAPIV2({ ctx, queryResponse }) - const endpoint = new URL( - "documents/search", - `${client.documentAPIEndpoint}/`, - ).toString() - ctx.server.use( - rest.get(endpoint, (req) => { - triedRefs.push(req.url.searchParams.get("ref")) - }), - rest.get(endpoint, (_req, res, ctx) => - res.once( - ctx.json({ message: `Master ref is: ${masterRef}` }), - ctx.status(410), - ), + ), + ) + + const consoleWarnSpy = vi + .spyOn(console, "warn") + .mockImplementation(() => void 0) + await args.run(client, { ref: badRef }) + consoleWarnSpy.mockRestore() + + expect([...triedRefs]).toStrictEqual([badRef, masterRef]) + }) + + it("retries with the master ref when an expired ref is used", async (ctx) => { + const client = createTestClient({ ctx }) + const badRef = ctx.mock.api.ref().ref + const masterRef = ctx.mock.api.ref().ref + const queryResponse = ctx.mock.api.query({ + documents: [ctx.mock.value.document()], + }) + + const triedRefs = new Set() + + mockPrismicRestAPIV2({ ctx, queryResponse }) + const endpoint = new URL( + "documents/search", + `${client.documentAPIEndpoint}/`, + ).toString() + ctx.server.use( + rest.get(endpoint, (req) => { + triedRefs.add(req.url.searchParams.get("ref")) + }), + rest.get(endpoint, (_req, res, ctx) => + res.once( + ctx.json({ message: `Master ref is: ${masterRef}` }), + ctx.status(410), ), - ) - - const consoleWarnSpy = vi - .spyOn(console, "warn") - .mockImplementation(() => void 0) - await args.run(client, { ref: badRef }) - consoleWarnSpy.mockRestore() - - expect(triedRefs).toStrictEqual([badRef, masterRef]) - }, - ) - - it.concurrent( - "throws if the maximum number of retries when an invalid ref is used is reached", - async (ctx) => { - const client = createTestClient({ ctx }) - const queryResponse = ctx.mock.api.query({ - documents: [ctx.mock.value.document()], - }) - - const triedRefs: (string | null)[] = [] - - mockPrismicRestAPIV2({ ctx, queryResponse }) - const endpoint = new URL( - "documents/search", - `${client.documentAPIEndpoint}/`, - ).toString() - ctx.server.use( - rest.get(endpoint, (req) => { - triedRefs.push(req.url.searchParams.get("ref")) - }), - rest.get(endpoint, (_req, res, requestCtx) => - res( - requestCtx.json({ - type: "api_notfound_error", - message: `Master ref is: ${ctx.mock.api.ref().ref}`, - }), - requestCtx.status(404), - ), + ), + ) + + const consoleWarnSpy = vi + .spyOn(console, "warn") + .mockImplementation(() => void 0) + await args.run(client, { ref: badRef }) + consoleWarnSpy.mockRestore() + + expect([...triedRefs]).toStrictEqual([badRef, masterRef]) + }) + + it("throws if the maximum number of retries when an invalid ref is used is reached", async (ctx) => { + const client = createTestClient({ ctx }) + const queryResponse = ctx.mock.api.query({ + documents: [ctx.mock.value.document()], + }) + + const triedRefs = new Set() + + mockPrismicRestAPIV2({ ctx, queryResponse }) + const endpoint = new URL( + "documents/search", + `${client.documentAPIEndpoint}/`, + ).toString() + ctx.server.use( + rest.get(endpoint, (req) => { + triedRefs.add(req.url.searchParams.get("ref")) + }), + rest.get(endpoint, (_req, res, requestCtx) => + res( + requestCtx.json({ + type: "api_notfound_error", + message: `Master ref is: ${ctx.mock.api.ref().ref}`, + }), + requestCtx.status(404), ), - ) - - const consoleWarnSpy = vi - .spyOn(console, "warn") - .mockImplementation(() => void 0) - await expect(async () => { - await args.run(client) - }).rejects.toThrow(prismic.RefNotFoundError) - consoleWarnSpy.mockRestore() - - expect(triedRefs.length).toBe(3) - }, - ) + ), + ) + + const consoleWarnSpy = vi + .spyOn(console, "warn") + .mockImplementation(() => void 0) + await expect(async () => { + await args.run(client) + }).rejects.toThrow(prismic.RefNotFoundError) + consoleWarnSpy.mockRestore() + + expect(triedRefs.size).toBe(3) + }) + + it("fetches a new master ref on subsequent queries if an invalid ref is used", async (ctx) => { + const client = createTestClient({ ctx }) + const queryResponse = ctx.mock.api.query({ + documents: [ctx.mock.value.document()], + }) + + const triedRefs = new Set() + + mockPrismicRestAPIV2({ ctx, queryResponse }) + const endpoint = new URL( + "documents/search", + `${client.documentAPIEndpoint}/`, + ).toString() + ctx.server.use( + rest.get(endpoint, (req) => { + triedRefs.add(req.url.searchParams.get("ref")) + }), + rest.get(endpoint, (_req, res, requestCtx) => + res.once( + requestCtx.json({ + type: "api_notfound_error", + message: `Master ref is: ${ctx.mock.api.ref().ref}`, + }), + requestCtx.status(404), + ), + ), + ) + + const consoleWarnSpy = vi + .spyOn(console, "warn") + .mockImplementation(() => void 0) + await args.run(client) + consoleWarnSpy.mockRestore() + + await args.run(client) + + expect(triedRefs.size).toBe(3) + }) } From 1499cd13cfed588050cc5c3fbd8e83e851d2af6d Mon Sep 17 00:00:00 2001 From: Angelo Ashmore Date: Wed, 30 Oct 2024 09:55:55 -1000 Subject: [PATCH 6/7] test: simplify test title --- test/__testutils__/testInvalidRefRetry.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/__testutils__/testInvalidRefRetry.ts b/test/__testutils__/testInvalidRefRetry.ts index 9406f1eb..1a53cbe6 100644 --- a/test/__testutils__/testInvalidRefRetry.ts +++ b/test/__testutils__/testInvalidRefRetry.ts @@ -90,7 +90,7 @@ export const testInvalidRefRetry = (args: TestInvalidRefRetryArgs): void => { expect([...triedRefs]).toStrictEqual([badRef, masterRef]) }) - it("throws if the maximum number of retries when an invalid ref is used is reached", async (ctx) => { + it("throws if the maximum number of retries with invalid refs is reached", async (ctx) => { const client = createTestClient({ ctx }) const queryResponse = ctx.mock.api.query({ documents: [ctx.mock.value.document()], From f3ae551cd9da9dee493971db58c8c4f003f8ee87 Mon Sep 17 00:00:00 2001 From: Angelo Ashmore Date: Wed, 30 Oct 2024 09:59:13 -1000 Subject: [PATCH 7/7] docs: update const description --- src/Client.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Client.ts b/src/Client.ts index 548493f6..e722384a 100644 --- a/src/Client.ts +++ b/src/Client.ts @@ -74,9 +74,9 @@ export const GET_ALL_QUERY_DELAY = 500 const DEFUALT_RETRY_AFTER_MS = 1000 /** - * The maximum number of attemps to retry a query with an invalid ref before - * halting. We allow multiple attemps as each attemp may return a different ref. - * Capping the number of attemps prevents infinite loops. + * The maximum number of attemps to retry a query with an invalid ref. We allow + * multiple attempts since each attempt may use a different (and possibly + * invalid) ref. Capping the number of attemps prevents infinite loops. */ const MAX_INVALID_REF_RETRY_ATTEMPS = 3