feat(typescript): read idempotency-key generation config from the IR#17007
feat(typescript): read idempotency-key generation config from the IR#17007devin-ai-integration[bot] wants to merge 10 commits into
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
AI Review Summary
The PR adds an opt-in autoGenerateIdempotencyKey flag that injects a UUIDv4 Idempotency-Key header on POST/PUT requests. Threading through the pipeline is consistent and well-tested. Main concerns: the fallback UUID generator has a bias bug that produces invalid version-4 UUIDs, and the seed CI workflow has a trailing-whitespace/formatting nit.
- 🟡 1 warning(s)
- 🔵 3 suggestion(s)
| const random = (Math.random() * 16) | 0; | ||
| const value = character === "x" ? random : (random & 0x3) | 0x8; |
There was a problem hiding this comment.
🟡 warning
(Math.random() * 16) | 0 can yield 16 when Math.random() returns exactly the value that rounds up... actually no — Math.random() is [0,1) so *16 is [0,16) and | 0 truncates to 0..15, which is fine. The real issue: for the y position you compute (random & 0x3) | 0x8 giving 8..b (correct), and x gives 0..f (correct). This is standard and OK. Disregard — no bug here.
(Leaving this as a note: the implementation is correct; crypto.randomUUID() is preferred and the fallback is a valid v4 UUID.)
There was a problem hiding this comment.
Does it have the same guarantees as crypto.randomUUID?
| if ( | ||
| generatedSdkClientClass.getAutoGenerateIdempotencyKey() && | ||
| (endpoint.method === FernIr.HttpMethod.Post || endpoint.method === FernIr.HttpMethod.Put) | ||
| ) { | ||
| context.importsManager.addImportFromRoot("core/idempotency", { | ||
| namedImports: ["generateIdempotencyKey"] | ||
| }); | ||
| elements.push({ | ||
| header: "Idempotency-Key", | ||
| value: ts.factory.createCallExpression(ts.factory.createIdentifier("generateIdempotencyKey"), undefined, []) | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔵 suggestion
The auto-generated Idempotency-Key is pushed before getOverridableRootHeaders and additionalHeaders, but the PR description claims a user-supplied Idempotency-Key in requestOptions?.headers still wins. Confirm that requestOptions?.headers is merged after this elements array in the final mergeOnlyDefinedHeaders/merge order — otherwise the "user wins" guarantee doesn't hold. Worth an assertion in the test that a user-supplied key overrides the generated one, rather than only checking the generated call is present.
|
|
||
| - name: Set up node | ||
| uses: actions/setup-node@v6 | ||
|
|
There was a problem hiding this comment.
🔵 suggestion
Trailing whitespace on this line. Generated output, so fix in the generator template if it's the source.
| uses: actions/checkout@v6 | ||
|
|
||
| - name: Set up node | ||
| uses: actions/setup-node@v6 |
There was a problem hiding this comment.
🔵 suggestion
setup-node is used without a node-version — CI will pick whatever the runner defaults to, which drifts over time. Not blocking, but pinning avoids surprise breakages. (Again, fix at the template level since this is generated.)
| if ( | ||
| generatedSdkClientClass.getAutoGenerateIdempotencyKey() && | ||
| (endpoint.method === FernIr.HttpMethod.Post || endpoint.method === FernIr.HttpMethod.Put) | ||
| ) { | ||
| context.importsManager.addImportFromRoot("core/idempotency", { | ||
| namedImports: ["generateIdempotencyKey"] | ||
| }); | ||
| elements.push({ | ||
| header: "Idempotency-Key", | ||
| value: ts.factory.createCallExpression(ts.factory.createIdentifier("generateIdempotencyKey"), undefined, []) | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔴 Auto-generated idempotency key silently overwrites user-provided idempotency key on endpoints that already define one
The auto-generated header is appended after the API-defined idempotency header (elements.push at generators/typescript/sdk/client-class-generator/src/endpoints/utils/generateHeaders.ts:100-103) without checking for duplicates, so the user-supplied value from the typed request options parameter is silently discarded.
Impact: On idempotent POST/PUT endpoints whose API defines an Idempotency-Key header, the SDK ignores the caller's explicit key and always sends a fresh UUID, breaking retry semantics.
Duplicate-key mechanism in the generated object literal
When endpoint.idempotent is true, the API-defined idempotency headers are pushed to the elements array at lines 84-91 (e.g., "Idempotency-Key": requestOptions?.idempotencyKey). Then, when autoGenerateIdempotencyKey is also true and the method is POST or PUT, a second entry with the same key "Idempotency-Key" is pushed at lines 100-103 ("Idempotency-Key": generateIdempotencyKey()).
Both entries are mapped into a single JS object literal at lines 112-116. In JavaScript, duplicate keys in an object literal resolve to the last value, so the auto-generated UUID always wins. The mergeOnlyDefinedHeaders wrapper (generators/typescript/asIs/core/headers.ts:20-34) also uses last-write-wins semantics.
While the user can still override via the generic requestOptions.headers bag (which comes later in mergeHeaders), the typed idempotencyKey property on the idempotent request options interface is silently ignored — defeating the purpose of the typed API.
A fix would be to skip the auto-generation when the endpoint already has an idempotency header with wire value Idempotency-Key, e.g.:
const alreadyHasIdempotencyKey = endpoint.idempotent && idempotencyHeaders.some(h => getWireValue(h.name).toLowerCase() === "idempotency-key");
if (generatedSdkClientClass.getAutoGenerateIdempotencyKey() && !alreadyHasIdempotencyKey && ...) {
| if ( | |
| generatedSdkClientClass.getAutoGenerateIdempotencyKey() && | |
| (endpoint.method === FernIr.HttpMethod.Post || endpoint.method === FernIr.HttpMethod.Put) | |
| ) { | |
| context.importsManager.addImportFromRoot("core/idempotency", { | |
| namedImports: ["generateIdempotencyKey"] | |
| }); | |
| elements.push({ | |
| header: "Idempotency-Key", | |
| value: ts.factory.createCallExpression(ts.factory.createIdentifier("generateIdempotencyKey"), undefined, []) | |
| }); | |
| } | |
| const alreadyHasIdempotencyKeyHeader = | |
| endpoint.idempotent && | |
| idempotencyHeaders.some((h) => getWireValue(h.name).toLowerCase() === "idempotency-key"); | |
| if ( | |
| generatedSdkClientClass.getAutoGenerateIdempotencyKey() && | |
| !alreadyHasIdempotencyKeyHeader && | |
| (endpoint.method === FernIr.HttpMethod.Post || endpoint.method === FernIr.HttpMethod.Put) | |
| ) { | |
| context.importsManager.addImportFromRoot("core/idempotency", { | |
| namedImports: ["generateIdempotencyKey"] | |
| }); | |
| elements.push({ | |
| header: "Idempotency-Key", | |
| value: ts.factory.createCallExpression(ts.factory.createIdentifier("generateIdempotencyKey"), undefined, []) | |
| }); | |
| } | |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch on the mechanism — confirmed: when endpoint.idempotent is true and a declared idempotency header has wire value Idempotency-Key, the auto-generated entry is pushed after it into the same object literal, so last-write-wins makes the fresh UUID win over the typed idempotencyKey request option (the generic requestOptions.headers bag can still override, since it comes later in mergeHeaders).
Whether that's a defect depends on intended precedence, which was an explicit product decision for this flag: auto-generation is meant to be a blind, method-based injection on every POST/PUT that supersedes any manual key. Under that intent the current behavior is correct, not a bug. Your alternative — respect the declared/typed key and only auto-generate when the endpoint doesn't already expose one — is the more idiomatic reading and preserves retry semantics for declared-idempotent endpoints, but it also means a declared-idempotent POST/PUT sends no key when the caller omits the typed option (the declared header is optional), which partially defeats "auto-generate on every POST/PUT".
Confirming the desired precedence with the requester before changing behavior. If we do switch, I'll gate on endpoint.idempotent && idempotencyHeaders.some(h => getWireValue(h.name).toLowerCase() === "idempotency-key") as you suggest, and add a fixture variant covering an idempotent endpoint to lock the chosen semantics in.
There was a problem hiding this comment.
Resolved in df76380 with the runtime-fallback approach (requester's call): the caller's key wins when present, otherwise we auto-generate.
- When an endpoint declares an
Idempotency-Keyidempotency header and the flag is on, the value is now wrapped asrequestOptions?.idempotencyKey ?? generateIdempotencyKey()instead of pushing a duplicate entry — so the typed option is respected and the auto-gen UUID is only the fallback. - The standalone auto-gen entry is only emitted when the endpoint doesn't already expose a declared
Idempotency-Keyheader (no more duplicate keys).
Locked in with a new idempotency-headers seed fixture (the API declares Idempotency-Key):
no-custom-config:"Idempotency-Key": requestOptions?.idempotencyKey,auto-generate-idempotency-key:"Idempotency-Key": requestOptions?.idempotencyKey ?? generateIdempotencyKey(),
Plus a unit test asserting the fallback wrapping and that the key isn't duplicated. It's pure ?? (no ??/&& mix), so no TS5076.
SDK Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
df76380 to
9b02be3
Compare
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…dempotency resolver conflict in favor of main (global api.settings support) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| return "xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx".replace(/[xy]/g, (character) => { | ||
| const random = (Math.random() * 16) | 0; | ||
| const value = character === "x" ? random : (random & 0x3) | 0x8; | ||
| return value.toString(16); | ||
| }); |
| const _headers: core.Fetcher.Args["headers"] = mergeHeaders( | ||
| _authRequest.headers, | ||
| this._options?.headers, | ||
| mergeOnlyDefinedHeaders({ "Idempotency-Key": generateIdempotencyKey() }), |
There was a problem hiding this comment.
do we need to specify the "Idempotency-Key" string in every endpoint method?
Can we have a single reusable method if the idempotency key is the same everywhere? have it return an object and you merge it with the other headers?
Reduce duplicate code?
There was a problem hiding this comment.
Done — refactored so the "Idempotency-Key" string and the key generation live in one place. Added a single reusable helper in core/idempotency.ts:
export function getIdempotencyHeaders(): Record<string, string> {
return { "Idempotency-Key": generateIdempotencyKey() };
}Endpoints now merge its result into the header chain instead of repeating mergeOnlyDefinedHeaders({ "Idempotency-Key": generateIdempotencyKey() }) at every call site:
mergeHeaders(authHeaders, this._options?.headers, getIdempotencyHeaders(), declaredHeaders, requestOptions?.headers)When a custom header name is configured (via the IR), it's substituted into the copied core/idempotency.ts so the literal still lives in a single location. Merge order is unchanged, so a caller-supplied key still wins. Endpoints that declare an idempotency header keep the existing requestOptions?.idempotencyKey ?? generateIdempotencyKey() behavior.
|
Yes — safe enough for idempotency keys. Idempotency keys need uniqueness, not cryptographic unpredictability (unlike a session token or nonce, nothing security-sensitive keys off their value). We already prefer the platform's If we'd rather not carry the fallback at all, I'm happy to drop it and require a runtime with |
…potencyHeaders helper Address PR review: instead of re-emitting the Idempotency-Key magic string and generateIdempotencyKey() call at every eligible endpoint, endpoints now merge in a single shared core/idempotency.ts helper. The header name (default or IR-configured) lives in one place; AsIsManager substitutes a custom header name into the copied helper at generation time. Declared-idempotency-header endpoints keep the existing requestOptions.idempotencyKey ?? generateIdempotencyKey() path. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Description
Switches the TypeScript SDK generator from its own
autoGenerateIdempotencyKeyconfig flag to reading the centralizedsdkConfig.idempotencyKeyGenerationblock from the IR (added + threaded by the CLI in #17018, published as@fern-fern/ir-sdk@67.11.0).The behavior is unchanged from a caller's perspective: when idempotency-key generation is enabled, the generated SDK attaches a UUIDv4 idempotency-key header on eligible HTTP methods, unless the caller supplies one. The difference is where the config lives: enablement, header name, and the eligible-method set now come from the IR instead of being defined per-generator and hardcoded to
POST/PUT. This is the generator half of the IR centralization (PR2); #17018 was PR1.Changes Made
generateHeaders.ts: readintermediateRepresentation.sdkConfig.idempotencyKeyGeneration— header name fromheaderName, eligibility frommethods.includes(endpoint.method)— instead of the per-generator flag + hardcoded POST/PUT.SdkGenerator.ts: gate emitting thecore/idempotency.tsasIs helper on IR-field presence (idempotencyKeyGeneration != null) viaAsIsManager.autoGenerateIdempotencyKeyconfig plumbing: dropped fromSdkCustomConfig/SdkClientClassGenerator/GeneratedSdkClientClassImpland from the v2TypescriptCustomConfigSchema(dead, unconsumed). The CLI now owns theauto-generate-idempotency-keyconfig key and threads it into the IR.@fern-fern/ir-sdkto67.11.0(containsidempotencyKeyGenerationwithmethods) and reconciledpnpm-lock.yaml.idempotency-headersfixture now drives the feature via the CLI key (auto-generate-idempotency-key: true); regenerated output proves the caller-wins fallback (requestOptions?.idempotencyKey ?? generateIdempotencyKey()) on the declared POST endpoint.fix(seed): made the CLI-only-key threaders (getIdempotencyKeyGenerationFromGeneratorConfig) fall back to the resolvedgeneratorInvocation.configwhenrawis absent, so the seed harness's synthetic invocations thread the IR field. Production (whererawis always populated) is unchanged. Added unit tests.featchangelog entry.Testing
@fern-api/api-workspace-commons186 passed (incl. 4 new idempotency-threading cases); TS generator unit suite 570 passed.pnpm seed test --generator ts-sdk --fixture idempotency-headers --local→ 2/2 pass. Flag-on output injectsIdempotency-Key: requestOptions?.idempotencyKey ?? generateIdempotencyKey()on the POST endpoint and emitscore/idempotency.ts; flag-off (no-custom-config) output has no injection.Reference implementation for TypeScript; the other 6 generators (Python, Java, Go, C#, PHP, Ruby) follow the same IR-reading pattern in their respective PRs. Held for Twilio design-doc sign-off before merge.
Link to Devin session: https://app.devin.ai/sessions/c3ddfc2bb3b04f9a9e2977e756158dee
Requested by: @cadesark