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.

Sign up for GitHub

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

feat: retry invalid/expired refs #356

Merged
merged 7 commits into from
Oct 30, 2024
Merged

feat: retry invalid/expired refs #356

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c062cb6
feat: retry invalid/expired refs
angeloashmore Oct 29, 2024
fc131d1
fix: remove duplicate ref retry logic and support `getFirst`
angeloashmore Oct 30, 2024
d10ebe6
test: invalid ref retry
angeloashmore Oct 30, 2024
345ff2b
feat: allow up to 3 retries before throwing
angeloashmore Oct 30, 2024
ef49bdc
feat: use a new master ref once a known-stale ref is used
angeloashmore Oct 30, 2024
1499cd1
test: simplify test title
angeloashmore Oct 30, 2024
f3ae551
docs: update const description
angeloashmore Oct 30, 2024
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
37 changes: 36 additions & 1 deletion src/Client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>()

/**
* The Prismic repository's name.
*/
Expand Down Expand Up @@ -531,7 +537,36 @@ export class Client<
): Promise<Query<TDocument>> {
const url = await this.buildQueryURL(params)

return await this.fetch<Query<TDocument>>(url, params)
try {
return await this.fetch<Query<TDocument>>(url, params)
} catch (error) {
if (!(error instanceof RefNotFoundError)) {
throw error
}

const latestRef = error.message.match(/Master ref is: (?<ref>.*)$/)
?.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)
Copy link
Member

@lihbr lihbr Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ #issue: I'm afraid this could create some concurrency issues when we dedupe requests:

  • T0
    • Page Foo fetches nav document
    • Page Bar fetches nav document
  • T1
    • Requests get deduped (only one is made) and fail at the same time with the same error
  • T2
    • Page Foo retries the request with the latest ref
    • Page Bar cannot retries the request with the latest ref because it's already being retried by Foo
    • Request cannot be deduped

...or when multiple requests (e.g. Promise.all(...)) fail at the same time, which I believe is symptomatic of this kind of error(?)

Maybe we should just retry directly instead of recursively calling the function after that?

- return await this.get({ ...params, ref: latestRef })
+ const url = await this.buildQueryURL({ ...params, ref: latestRef })

+ return await this.fetch<Query<TDocument>>(url, { ...params, ref: latestRef })

Or base the retry off whether or not we had an explicit ref parameter(?)

💡 #idea: Maybe we should clear the repository cache when this happens so that other requests (during the cache's 5s lifespan) can fetch a fresh ref?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right; it would likely cause some issues when queries are deduplicated. I removed the retriedRefs array and check and simply trust that the provided refs won't cause a loop.

Here's the comment I sent to you in Slack:

I removed the logic that prevents the same ref from being retried more than once. You are right about concurrency issues since the retry logic is in the query method, not in the deduplicated fetch job. I'd like to keep the retry in the method so it is close to its intended usage.

I'm wondering if we should still include some kind of loop prevention. I refactored the implementation you reviewed yesterday such that we now have a private _get() method that preforms the retry. Since we have that now, it's easy for us to track how many retries and halt if we get too deep.

I think we should include some kind of loop protection to be safe, but I wanted to get your thoughts first.

Maybe we should just retry directly instead of recursively calling the function after that?

I'd rather not retry directly since it means we can only follow the API's response once. If a website has a very stale cache with multiple chained invalid ref responses, we should be able to use each of them.

We log a warning each time we retry, so devs will still be nudged to fix their caching issue.

Or base the retry off whether or not we had an explicit ref parameter(?)

A retry may be necessary whether or not an explicit ref is provided. Websites may have a stale /api/v2, which would result in a stale master ref being used implicitly.

💡 #idea: Maybe we should clear the repository cache when this happens so that other requests (during the cache's 5s lifespan) can fetch a fresh ref?

Maybe! This fix is really intended for Next.js' indefinite cache, which works outside the client's internal 5s cache.

I can't think of any downside to clearing the internal cache. It would resolve the tiny chance that a burst of requests from the same client results uses a stale, internally cached ref. If you can't think of any downsides, I'll add it in.

Copy link
Member

@lihbr lihbr Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the clarification!

  • I agree we should have some kind of loop prevention yes ♻️
  • I'm aligned with clearing the cache, it's mostly free and will clear some edge cases 🧹


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 })
}
}

/**
Expand Down
88 changes: 88 additions & 0 deletions test/__testutils__/testInvalidRefRetry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<prismic.Client["get"]>[0],
) => Promise<unknown>
}

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()
})
}
5 changes: 5 additions & 0 deletions test/client-get.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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),
})
Expand Down
Loading