-
Notifications
You must be signed in to change notification settings - Fork 41
fix(cloudflare): tag Turnstile missing widgets as NotFound #303
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| { | ||
| "errors": { | ||
| "NotFound": [{ "status": 404 }] | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| { | ||
| "errors": { | ||
| "NotFound": [{ "status": 404 }] | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -256,10 +256,59 @@ function findMatchingError( | |
| return bestMatch; | ||
| } | ||
|
|
||
| // Some Cloudflare endpoints return bare HTTP failures instead of the standard | ||
| // `{ success: false, errors: [...] }` envelope. Generated operation schemas can | ||
| // still declare status-based error matchers, so try those before falling back | ||
| // to generic HTTP errors. | ||
| function matchOperationError( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, why is this necessary?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems to be to handle when there is no envelope from cloudflare; I found it weird when i first looked at this, but it seems fine
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a |
||
| errors: readonly ApiErrorClass[] | undefined, | ||
| code: number | undefined, | ||
| status: number, | ||
| message: string, | ||
| ): Effect.Effect<never, unknown> | undefined { | ||
| if (!errors || errors.length === 0) return undefined; | ||
|
|
||
| const errorSchemas = new Map<string, Schema.Top>(); | ||
| for (const errorSchema of errors) { | ||
| const identifier = extractTagFromAst( | ||
| (errorSchema as unknown as Schema.Top).ast, | ||
| ); | ||
| if (identifier) { | ||
| errorSchemas.set(identifier, errorSchema as unknown as Schema.Top); | ||
| } | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't do all this work on every request. Instead, it should create this map and return a function for matching, that way we cache the map in the closure once.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed, lmk what you think |
||
|
|
||
| const matched = findMatchingError(errorSchemas, code, status, message); | ||
| if (!matched) return undefined; | ||
|
|
||
| const errorData = { | ||
| _tag: matched.tag, | ||
| code: code ?? 0, | ||
| message, | ||
| }; | ||
| return Schema.decodeUnknownEffect(matched.schema)(errorData).pipe( | ||
| Effect.flatMap((decoded: unknown) => Effect.fail(decoded)), | ||
| Effect.catchIf( | ||
| (e: unknown) => | ||
| typeof e === "object" && | ||
| e !== null && | ||
| "_tag" in e && | ||
| (e as any)._tag === "SchemaError", | ||
| () => | ||
| Effect.fail( | ||
| new UnknownCloudflareError({ | ||
| code, | ||
| message, | ||
| }), | ||
| ), | ||
| ), | ||
| ) as Effect.Effect<never, unknown>; | ||
| } | ||
|
|
||
| /** | ||
| * Match a Cloudflare API error response using per-operation error schemas. | ||
| */ | ||
| const matchError = ( | ||
| export const matchCloudflareError = ( | ||
| status: number, | ||
| errorBody: unknown, | ||
| errors?: readonly ApiErrorClass[], | ||
|
|
@@ -272,6 +321,12 @@ const matchError = ( | |
| "_nonJsonError" in errorBody; | ||
| if (isNonJsonError) { | ||
| const message = String((errorBody as any).body); | ||
| // Some Cloudflare endpoints return bare HTTP errors without the standard | ||
| // `{ success: false, errors: [...] }` envelope. Give operation-specific | ||
| // status matchers a chance before falling back to generic HTTP errors. | ||
| const matched = matchOperationError(errors, undefined, status, message); | ||
| if (matched) return matched; | ||
|
|
||
| // For 5xx errors, return a properly categorized error so retries work | ||
| if (status >= 500) { | ||
| return Effect.fail(httpStatusError(status, message, headers)); | ||
|
|
@@ -316,6 +371,12 @@ const matchError = ( | |
| // For 5xx errors, return a properly categorized error so retries work | ||
| const bodyStr = | ||
| typeof errorBody === "string" ? errorBody : JSON.stringify(errorBody); | ||
| // Preserve per-operation error typing for endpoints whose error responses | ||
| // are JSON but not Cloudflare envelopes, such as Turnstile's missing-widget | ||
| // 404 response. | ||
| const matched = matchOperationError(errors, undefined, status, bodyStr); | ||
| if (matched) return matched; | ||
|
|
||
| if (status >= 500) { | ||
| return Effect.fail(httpStatusError(status, bodyStr, headers)); | ||
| } | ||
|
|
@@ -329,51 +390,8 @@ const matchError = ( | |
| ); | ||
| } | ||
|
|
||
| // Build error schema map from the per-operation errors | ||
| if (errors && errors.length > 0) { | ||
| const errorSchemas = new Map<string, Schema.Top>(); | ||
| for (const errorSchema of errors) { | ||
| const identifier = extractTagFromAst( | ||
| (errorSchema as unknown as Schema.Top).ast, | ||
| ); | ||
| if (identifier) { | ||
| errorSchemas.set(identifier, errorSchema as unknown as Schema.Top); | ||
| } | ||
| } | ||
|
|
||
| const matched = findMatchingError( | ||
| errorSchemas, | ||
| errorCode, | ||
| status, | ||
| errorMessage, | ||
| ); | ||
|
|
||
| if (matched) { | ||
| // Decode using the schema - properly instantiates TaggedError classes | ||
| const errorData = { | ||
| _tag: matched.tag, | ||
| code: errorCode ?? 0, | ||
| message: errorMessage, | ||
| }; | ||
| return Schema.decodeUnknownEffect(matched.schema)(errorData).pipe( | ||
| Effect.flatMap((decoded: unknown) => Effect.fail(decoded)), | ||
| Effect.catchIf( | ||
| (e: unknown) => | ||
| typeof e === "object" && | ||
| e !== null && | ||
| "_tag" in e && | ||
| (e as any)._tag === "SchemaError", | ||
| () => | ||
| Effect.fail( | ||
| new UnknownCloudflareError({ | ||
| code: errorCode, | ||
| message: errorMessage, | ||
| }), | ||
| ), | ||
| ), | ||
| ) as Effect.Effect<never, unknown>; | ||
| } | ||
| } | ||
| const matched = matchOperationError(errors, errorCode, status, errorMessage); | ||
| if (matched) return matched; | ||
|
|
||
| // Check global error codes before falling through to unknown | ||
| if (errorCode !== undefined && errorCode in GLOBAL_ERROR_CODE_MAP) { | ||
|
|
@@ -462,7 +480,7 @@ const _API = makeAPI<Credentials>({ | |
| credentials: Credentials as any, | ||
| getBaseUrl: (creds: any) => creds.apiBaseUrl, | ||
| getAuthHeaders: formatHeaders as any, | ||
| matchError, | ||
| matchError: matchCloudflareError, | ||
| ParseError: CloudflareDecodeError as any, | ||
| transformRequestParts: ({ pathTemplate, parts }) => | ||
| transformCloudflareRequestParts({ pathTemplate, parts }), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,16 @@ import * as T from "../traits.ts"; | |
| import type { Credentials } from "../credentials.ts"; | ||
| import { type DefaultErrors } from "../errors.ts"; | ||
|
|
||
| // ============================================================================= | ||
| // Errors | ||
| // ============================================================================= | ||
|
|
||
| export class NotFound extends Schema.TaggedErrorClass<NotFound>()("NotFound", { | ||
| code: Schema.Number, | ||
| message: Schema.String, | ||
| }) {} | ||
| T.applyErrorMatchers(NotFound, [{ status: 404 }]); | ||
|
Comment on lines
+16
to
+23
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these are generated files do not modify them directly. add a patch and then rerun generate
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed: moved this into |
||
|
|
||
| // ============================================================================= | ||
| // SecretWidget | ||
| // ============================================================================= | ||
|
|
@@ -201,7 +211,7 @@ export const GetWidgetResponse = /*@__PURE__*/ /*#__PURE__*/ Schema.Struct({ | |
| T.ResponsePath("result"), | ||
| ) as unknown as Schema.Schema<GetWidgetResponse>; | ||
|
|
||
| export type GetWidgetError = DefaultErrors; | ||
| export type GetWidgetError = DefaultErrors | NotFound; | ||
|
|
||
| export const getWidget: API.OperationMethod< | ||
| GetWidgetRequest, | ||
|
|
@@ -211,7 +221,7 @@ export const getWidget: API.OperationMethod< | |
| > = /*@__PURE__*/ /*#__PURE__*/ API.make(() => ({ | ||
| input: GetWidgetRequest, | ||
| output: GetWidgetResponse, | ||
| errors: [], | ||
| errors: [NotFound], | ||
| })); | ||
|
|
||
| export interface ListWidgetsRequest { | ||
|
|
@@ -690,7 +700,7 @@ export const DeleteWidgetResponse = /*@__PURE__*/ /*#__PURE__*/ Schema.Struct({ | |
| T.ResponsePath("result"), | ||
| ) as unknown as Schema.Schema<DeleteWidgetResponse>; | ||
|
|
||
| export type DeleteWidgetError = DefaultErrors; | ||
| export type DeleteWidgetError = DefaultErrors | NotFound; | ||
|
|
||
| export const deleteWidget: API.OperationMethod< | ||
| DeleteWidgetRequest, | ||
|
|
@@ -700,5 +710,5 @@ export const deleteWidget: API.OperationMethod< | |
| > = /*@__PURE__*/ /*#__PURE__*/ API.make(() => ({ | ||
| input: DeleteWidgetRequest, | ||
| output: DeleteWidgetResponse, | ||
| errors: [], | ||
| errors: [NotFound], | ||
| })); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we making changes to this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change is needed because Turnstile returns bare/non-envelope 404 responses for missing widgets. the generated operation patch can now declare
{ status: 404 }, butapi.tshas to try operation-specific matchers before falling back to genericCloudflareHttpErrorfor those non-envelope responses. added comments and regression tests for both non-json and non-envelope cases.