test: add Xquik OpenAPI 3.1 fixture#30
Conversation
PR SummaryMedium Risk Overview Compiler: HTTP: Env lookups for tokens and OAuth2 client credentials use Cache: Tests: OAuth2 compiler cases, scoped OAuth2 HTTP test, Xquik YAML fixture, and integration test for dual auth, query enums, and response schemas. Reviewed by Cursor Bugbot for commit b8bb04f. Bugbot is set up for automated code reviews on this repo. Configure here. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const doc = await loadOpenApiDocument("test/fixtures/xquik-openapi.yaml"); | ||
| const operations = compileOperations(doc); |
There was a problem hiding this comment.
📝 Info: Test relies on $ref resolution happening inside loadOpenApiDocument
The output schema assertion at test/compiler.test.ts:168-171 accesses tweets.type directly, which requires that the $ref: '#/components/schemas/PaginatedTweets' in the fixture's 200 response has been fully dereferenced before compileOperations sees the document. This works because loadOpenApiDocument (src/openapi.ts:17) uses @apidevtools/json-schema-ref-parser to dereference all $refs. If someone were to pass the raw parsed YAML to compileOperations without going through loadOpenApiDocument, the response schema would still contain unresolved $refs and the output schema would lack properties like tweets. This is by design but represents a coupling that isn't immediately obvious from the test alone.
Was this helpful? React with 👍 or 👎 to provide feedback.
kriptoburak
force-pushed
the
codex/add-xquik-openapi31-fixture
branch
from
f0706b2 to
f3424f4
Compare
|
Addressed the two Devin readability notes in Validation after the update:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Addressed the current Devin finding by preserving the OpenAPI security-scheme key separately from the API key wire name. The compiler now keeps the scheme key for credential lookup and OpenAPI reconstruction while using Validation:
|
| key: name, | ||
| name: scheme.type === "apiKey" && typeof scheme.name === "string" ? scheme.name : name, |
There was a problem hiding this comment.
📝 Info: Non-apiKey schemes still use registry key as name, creating redundancy with key
For non-apiKey schemes (http, oauth2, openIdConnect), getSecuritySchemes sets both key and name to the same value—the registry key (src/compiler.ts:675-676). This means scheme.key ?? scheme.name always resolves to scheme.key for compiler-produced schemes. The redundancy is harmless but worth being aware of: scheme.name for http/oauth2 schemes is just the registry key, not a protocol-level name. If a future change needs to distinguish these (e.g., displaying a human-readable name), the name field's semantics would need to be revisited.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Addressed the new Devin OAuth2 prefix finding in Validation:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
📝 Info: Behavioral change: apiKey header name now comes from the OpenAPI spec's name field instead of the dictionary key
Before this PR, apiKey schemes used the security scheme dictionary key (e.g., "apiKey") as the HTTP header name at src/http.ts:425. Now it uses the OpenAPI spec's name field (e.g., "X-API-Key"). This is a correctness fix — the OpenAPI specification says the name property of an apiKey scheme is the actual header/query/cookie parameter name — but it is a behavioral change for any spec where the dictionary key differs from the spec's name field (like the xquik fixture where key is "apiKey" but name is "X-API-Key"). Previously such specs would send the wrong header name.
(Refers to line 425)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
📝 Info: Existing tests create SecurityScheme without the new key field
Several existing tests in test/http.test.ts construct SecurityScheme objects directly without the key field (e.g., lines 180, 217-219, 259, 269, 313). This works due to the scheme.key ?? scheme.name fallback pattern used consistently in src/http.ts:417 and src/http.ts:462. However, this means these tests don't exercise the new key-vs-name distinction at all. Only the new compiler test (test/compiler.test.ts:156) verifies the split behavior. Integration-level http tests that compile from an OpenAPI doc with divergent scheme key and apiKey name would provide stronger coverage.
(Refers to lines 214-222)
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| const cacheKey = `${scheme.name}:${scheme.tokenUrl}`; | ||
| const credentialName = scheme.key ?? scheme.name; | ||
| const cacheKey = `${envPrefix}:${credentialName}:${scheme.tokenUrl}`; |
There was a problem hiding this comment.
📝 Info: OAuth2 cache key format changed — stale entries from prior runs will miss
The OAuth2 token cache key changed from ${scheme.name}:${scheme.tokenUrl} to ${envPrefix}:${credentialName}:${scheme.tokenUrl} (src/http.ts:463). Since the cache is a module-level Map (src/http.ts:34), this means any cached tokens from before this change (in a long-running process that gets hot-reloaded) would become orphaned. In practice this is harmless — it just means a single extra token fetch after upgrading — but worth noting that the cache is now partitioned by envPrefix, which is the intended improvement (prevents cross-scope cache collisions).
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Addressed the Cursor compile-cache finding in Validation:
|
1 similar comment
|
Addressed the Cursor compile-cache finding in Validation:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Addressed the latest Devin OAuth2 flow finding in Validation:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Addressed the latest Cursor OAuth2 flow finding in Validation:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Addressed the latest Cursor OAuth2 scopes finding in Validation:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| out[name] = { | ||
| name, | ||
| key: name, | ||
| name: scheme.type === "apiKey" && typeof scheme.name === "string" ? scheme.name : name, | ||
| type: String(scheme.type), | ||
| in: isValidIn(scheme.in) ? scheme.in : undefined, | ||
| scheme: typeof scheme.scheme === "string" ? scheme.scheme : undefined, | ||
| tokenUrl: typeof scheme.tokenUrl === "string" ? scheme.tokenUrl : undefined, | ||
| scopes: isObject(scheme.scopes) ? (scheme.scopes as Record<string, string>) : undefined | ||
| tokenUrl: oauth2Flow.tokenUrl ?? (typeof scheme.tokenUrl === "string" ? scheme.tokenUrl : undefined), | ||
| scopes: oauth2Flow.scopes ?? normalizeScopes(scheme.scopes) | ||
| }; |
There was a problem hiding this comment.
📝 Info: Semantic change to SecurityScheme.name for apiKey types
Previously scheme.name was always the component key from the OpenAPI securitySchemes map. Now for apiKey schemes, scheme.name is the actual parameter name (e.g., "X-API-Key") while scheme.key holds the component key (e.g., "apiKey"). This is a semantic change to a shared interface (SecurityScheme in src/types.ts:17-25). All internal usages were updated: src/http.ts:417 uses scheme.key ?? scheme.name for credential lookup, while src/http.ts:423-425 correctly uses scheme.name for the actual header/query/cookie name. The src/index.ts:204 also uses scheme.key ?? scheme.name for security requirement reconstruction. External consumers loading cached OperationModels that were serialized before this change would get the old name semantics, but the cache version bump (2→6) ensures those caches are invalidated.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function getOAuth2FlowAuth(scheme: Record<string, unknown>): Pick<SecurityScheme, "tokenUrl" | "scopes"> { | ||
| const flows = isObject(scheme.flows) ? (scheme.flows as Record<string, unknown>) : {}; | ||
| let firstScopes: Record<string, string> | undefined; | ||
| let selectedTokenUrl: string | undefined; | ||
| let selectedScopes: Record<string, string> | undefined; | ||
| for (const flowName of ["clientCredentials", "authorizationCode", "password", "implicit"]) { | ||
| const rawFlow = flows[flowName]; | ||
| if (!isObject(rawFlow)) { | ||
| continue; | ||
| } | ||
|
|
||
| const flow = rawFlow as Record<string, unknown>; | ||
| const scopes = normalizeScopes(flow.scopes); | ||
| if (scopes) { | ||
| firstScopes ??= scopes; | ||
| } | ||
|
|
||
| if (!selectedTokenUrl && typeof flow.tokenUrl === "string") { | ||
| selectedTokenUrl = flow.tokenUrl; | ||
| selectedScopes = scopes; | ||
| } | ||
| } | ||
|
|
||
| if (selectedTokenUrl) { | ||
| const auth: Pick<SecurityScheme, "tokenUrl" | "scopes"> = { tokenUrl: selectedTokenUrl }; | ||
| const scopes = selectedScopes ?? firstScopes; | ||
| if (scopes) { | ||
| auth.scopes = scopes; | ||
| } | ||
| return auth; | ||
| } | ||
|
|
||
| if (firstScopes) { | ||
| return { scopes: firstScopes }; | ||
| } | ||
|
|
||
| return {}; | ||
| } |
There was a problem hiding this comment.
📝 Info: getOAuth2FlowAuth silently ignores non-clientCredentials flows
The getOAuth2FlowAuth function at src/compiler.ts:689-710 only extracts metadata from flows.clientCredentials. Other OAuth2 flows (authorizationCode, implicit, password) are intentionally ignored because the system only supports the client_credentials grant type (see getOAuth2AccessToken at src/http.ts:472 which hardcodes grant_type: "client_credentials"). This is consistent and tested at test/compiler.test.ts:184-228. However, specs that use only non-clientCredentials flows will get no tokenUrl or scopes unless they happen to have those fields at the top level of the scheme object (non-standard). This is a reasonable limitation but worth noting in documentation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function normalizeScopes(value: unknown): Record<string, string> | undefined { | ||
| if (!isObject(value)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const scopes: Record<string, string> = {}; | ||
| for (const [name, description] of Object.entries(value as Record<string, unknown>)) { | ||
| if (typeof description === "string") { | ||
| scopes[name] = description; | ||
| } | ||
| } | ||
|
|
||
| return Object.keys(scopes).length > 0 ? scopes : undefined; | ||
| } |
There was a problem hiding this comment.
📝 Info: normalizeScopes changes behavior for empty scopes objects
The old code (isObject(scheme.scopes) ? scheme.scopes as Record<string, string> : undefined) would return {} for an empty scopes object. The new normalizeScopes function returns undefined when there are no string-valued entries. Downstream in getOAuth2AccessToken (src/http.ts:471), Object.keys(scheme.scopes ?? {}).join(" ") produces an empty string either way, so the scope parameter is omitted from the token request in both cases. No observable difference in behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
| import type { CompileOptions, OperationModel } from "./types.js"; | ||
|
|
||
| const CACHE_FORMAT_VERSION = 2; | ||
| const CACHE_FORMAT_VERSION = 6; |
There was a problem hiding this comment.
🚩 Cache format version jump from 2 to 7 instead of 3
The CACHE_FORMAT_VERSION jumps from 2 to 7 (src/compile-cache.ts:8). This is functionally correct — any non-matching version invalidates the cache — but the gap suggests either parallel development branches or an intent to reserve intermediate versions. Not a bug, but the non-sequential numbering is unusual and could be confusing for future maintainers.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Addressed the latest Cursor OAuth2 grant finding in b8bb04f by limiting compiled token acquisition metadata to the clientCredentials flow, matching the runtime client_credentials grant. Non-client-credentials token URLs are now ignored so auth-code/password endpoints are not used for client-credentials token requests. Validation:
|
1 participant
Summary
Validation
Duplicate checks: no existing Xquik references in repo tree, open PRs, or issues.