fix(typescript): match SDK request/auth shape in dynamic snippets - #17746
dvdaruri-art merged 18 commits into
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 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
Adds a wrapperProperty to dynamic-IR auth schemes (multi-auth nesting) and teaches the TS dynamic-snippet generator to flatten referenced request bodies under flattenRequestParameters. Implementation broadly mirrors the SDK generators; the main risks I see are the flattening path not handling optional/nullable body wrappers and the temporary structural type guard for wrapperProperty silently degrading if the field is ever renamed.
- 🟡 1 warning(s)
- 🔵 4 suggestion(s)
This review is complete for the current scope. Addressing all critical and warning findings above satisfies the review; suggestions are optional. Request another review only if subsequent changes introduce new behavior or materially expand the pull request's scope.
| const flattened = this.context.dynamicTypeLiteralMapper.convert({ | ||
| typeReference: body.bodyType.value, | ||
| value, | ||
| convertOpts: { isForRequest: true } | ||
| }); | ||
| const fields = flattened.getObjectFields(); |
There was a problem hiding this comment.
🔵 suggestion
dynamicTypeLiteralMapper.convert is called here and then again inside getReferencedRequestBodyPropertyObjectField when fields == null. Any errors the mapper records into the context (e.g. type mismatches on the request body) will therefore be reported twice in that fallback path. Worth caching the converted literal and reusing it instead of converting twice.
| function hasAuthWrapperProperty(auth: AuthFields): auth is AuthWithWrapperProperty { | ||
| return "wrapperProperty" in auth; | ||
| } | ||
|
|
||
| function getAuthWrapperProperty(auth: AuthFields): FernIr.dynamic.Name | undefined { | ||
| return hasAuthWrapperProperty(auth) ? auth.wrapperProperty : undefined; | ||
| } |
There was a problem hiding this comment.
🔵 suggestion
hasAuthWrapperProperty is a structural "wrapperProperty" in auth check against a pinned older dynamic-ir-sdk. If the IR field is ever renamed this degrades silently to "no wrapper" rather than failing to compile. Since the PR description already plans a follow-up dep bump, please leave a // TODO(<owner>): remove once @fern-api/dynamic-ir-sdk >= 67.26.0 comment here (and in the mirrored guard in authWrapperPropertyDynamic.test.ts) so the temporary shim doesn't outlive its purpose.
| const response = await generator.generate(REQUEST); | ||
|
|
||
| expect(response.snippet).toContain("token:"); | ||
| expect(response.snippet).not.toContain("bearerAuth"); |
There was a problem hiding this comment.
🔵 suggestion
not.toContain("bearerAuth") is a fairly loose assertion — it would also pass (or fail) for unrelated occurrences of that substring elsewhere in the snippet. Asserting on the exact expected constructor snippet (or at least not.toContain("bearerAuth: {")) makes the intent clearer and less brittle.
| modifyIr?: ( | ||
| ir: import("@fern-api/dynamic-ir-sdk").FernIr.dynamic.DynamicIntermediateRepresentation | ||
| ) => import("@fern-api/dynamic-ir-sdk").FernIr.dynamic.DynamicIntermediateRepresentation; |
There was a problem hiding this comment.
🔵 suggestion
Inline import(...) type references are unusual here given the file already imports from @fern-api/dynamic-ir-sdk-adjacent modules. A top-level import type { FernIr } from "@fern-api/dynamic-ir-sdk"; and modifyIr?: (ir: FernIr.dynamic.DynamicIntermediateRepresentation) => FernIr.dynamic.DynamicIntermediateRepresentation; would read better.
| modifyIr?: ( | |
| ir: import("@fern-api/dynamic-ir-sdk").FernIr.dynamic.DynamicIntermediateRepresentation | |
| ) => import("@fern-api/dynamic-ir-sdk").FernIr.dynamic.DynamicIntermediateRepresentation; | |
| modifyIr?: ( | |
| ir: FernIr.dynamic.DynamicIntermediateRepresentation | |
| ) => FernIr.dynamic.DynamicIntermediateRepresentation; |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| const wrapperProperty = | ||
| auth.requirement === "ANY" || auth.requirement === "ENDPOINT_SECURITY" | ||
| ? this.fullCasingsGenerator.generateName(scheme.key) |
There was a problem hiding this comment.
🟡 Endpoint-specific auth uses wrong scheme
When an ENDPOINT_SECURITY endpoint excludes the first global scheme, wrapperProperty still selects that scheme. convertEndpoint assigns this auth to every endpoint, producing unusable credentials.
Learn more
Endpoint security lets each endpoint select a subset or combination of the API's global auth schemes. The converter selects auth.schemes[0] once, builds one dynamic auth object, and assigns it to every endpoint in convertEndpoint. The new wrapper therefore preserves the same globally selected scheme even when an endpoint routes through another provider. A generated snippet can construct the client successfully but omit the credential required for that endpoint.
Example: An API declares Bearer first and ApiKey second. An endpoint permits only ApiKey. Its dynamic snippet still emits { bearer: { token: "..." } }, so the SDK's ApiKey route has no credential.
Recommended fix: Derive each endpoint's dynamic auth and example auth values from that endpoint's security requirements. Preserve the selected scheme key in wrapperProperty, and define deterministic handling for endpoint requirements containing multiple alternatives or combined schemes.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pre-existing: the converter has always picked auth.schemes[0] once and attached it to every endpoint; this PR only adds the wrapper name for that same scheme, so it doesn't regress anything. Deriving per-endpoint auth from endpoint security requirements is a larger change to the dynamic IR (Endpoint.auth would need to be per-endpoint-resolved) and is out of scope here — happy to file it as a follow-up.
…elds in snippets 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>
Docs Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on
Docs generation runs |
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 |
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>
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>
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>
…ody-auth-wrapper 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>
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>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… of serde config 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>
…-body-auth-wrapper
…7746) * fix(typescript): match SDK request/auth shape in dynamic snippets Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * chore(typescript): address review nits on dynamic snippet auth/flatten Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(typescript): drop path params that collide with flattened body fields in snippets Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(cli): update auth wrapperProperty snapshots Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(cli): add TypeScript flattening fixture snapshots Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(ts-sdk): add flattening seed fixture output Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(typescript): cover real fixture dynamic snippets Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * refactor(typescript): centralize auth wrapper property handling Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test: fix fixture generators.yml branch names Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(test): add response examples to TypeScript fixture Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(cli): refresh IR snapshots after main merge Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test: align ts-flatten-request-any-auth example ids for wire tests Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * chore(typescript): point dynamic-ir-sdk compat TODO at IR 67.27.0 Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * chore: rerun CI Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(typescript): use camelCase scheme key for auth wrapper regardless of serde config Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(typescript): match SDK oauth wrapper key casing in dynamic snippets Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: dakshesh.daruri <dakshesh.daruri@postman.com> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Dak <dakshesh@buildwithfern.com>
…7746) * fix(typescript): match SDK request/auth shape in dynamic snippets Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * chore(typescript): address review nits on dynamic snippet auth/flatten Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(typescript): drop path params that collide with flattened body fields in snippets Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(cli): update auth wrapperProperty snapshots Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(cli): add TypeScript flattening fixture snapshots Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(ts-sdk): add flattening seed fixture output Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(typescript): cover real fixture dynamic snippets Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * refactor(typescript): centralize auth wrapper property handling Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test: fix fixture generators.yml branch names Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(test): add response examples to TypeScript fixture Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(cli): refresh IR snapshots after main merge Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test: align ts-flatten-request-any-auth example ids for wire tests Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * chore(typescript): point dynamic-ir-sdk compat TODO at IR 67.27.0 Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * chore: rerun CI Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(typescript): use camelCase scheme key for auth wrapper regardless of serde config Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(typescript): match SDK oauth wrapper key casing in dynamic snippets Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: dakshesh.daruri <dakshesh.daruri@postman.com> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Dak <dakshesh@buildwithfern.com>
Description
Refs Pylon issue 23380 (Payabli): TypeScript API-reference code samples don't match the generated TypeScript SDK.
Two independent mismatches between the TS dynamic snippet generator and the TS SDK generator:
flattenRequestParameters— the SDK spreads a referenced request body's properties into the request wrapper when the body resolves to a named object type (GeneratedRequestWrapperImpl.getFlattenedReferencedRequestBodyProperties), but snippets always emittedbody: { ... }.auth.requirementofANYorENDPOINT_SECURITYthe SDK nests each scheme's constructor options under a camelCased scheme-key property (SdkGenerator.shouldUseWrapper), but snippets emitted them flat.Note on scope of the flatten fix: the path/body collision only exists when the path parameter is declared inside the endpoint's
request:block (inlined request). An endpoint-levelpath-parameters:with a directly referenced body is emitted positionally by both the SDK and the snippet generator (updateUserProfile("user-1", { ... })), so nothing changes there. The newts-flatten-request-any-authfixture pins both shapes.Changes Made
baseUrlEnvVarwhile this was open): newdynamic.BaseAuth { wrapperProperty: optional<Name> }extended byBasicAuth/BearerAuth/HeaderAuth/OAuth/InferredAuth; regeneratedpackages/ir-sdk/src/sdk, dated changelog entry. Kept language-neutral (aName, not a TS-specific flag).DynamicSnippetsConverter.convertAuth: newgetAuthWrapperProperty(auth, scheme)(exhaustiveswitchonauth.requirement+assertNever) returnsgenerateName(scheme.key)forANY/ENDPOINT_SECURITY,undefinedforALL. Dynamic test-definition JSONs regenerated (wrapperProperty: null/set).EndpointSnippetGenerator:referencedbody +customConfig.flattenRequestParameters === true+ body type resolves to anobject→ emit the object's fields directly (viaTypeLiteral.getObjectFields(), new accessor intypescript-v2/ast). Non-object bodies (bytes, primitives, aliases,optional<...>) and default config are unchanged — same rule as the SDK.getCollidingPathParameterPropertyNames).getConstructor*AuthArgsgoes throughwrapAuthFields, which nests fields underwrapperPropertywhen present. The wrapper key is alwayscamelCase(scheme.key)(with the SDK'sOAuth → oauthspecial case), notcontext.getPropertyName(...)— undernoSerdeLayer/retainOriginalCasingthat would emit the original scheme name (BearerAuth: {...}), which the SDK rejects (*AuthProviderGenerator.getWrapperPropertyNameusestoCamelCaseunconditionally).@fern-api/dynamic-ir-sdk@67.21.0, which doesn't have the field yet, so it readswrapperPropertythrough a named type guard (hasWrapperPropertyField, marked TODO). Serialized fixtures always carrywrapperProperty: null, so the guard is about the field existing;getAuthWrapperPropertymapsnull → undefined. Follow-up once the new IR is published: bump the dep and delete the guard.test-definitions/fern/apis/ts-flatten-request-any-auth(ts-prefixed → seed runs it for ts-sdk only):auth: any: [BearerAuth, ApiKey];PUT /users/{id}with the path param declared in therequest:block and a referencedUpdateUser { id, name }body (deliberateidcollision);PUT /users/{id}/profilewith an endpoint-level path param. Seed output committed underseed/ts-sdk/ts-flatten-request-any-auth/{no-custom-config,flatten-request-parameters}— the generated SDK is the ground truth the snippet tests assert against (UpdateUserRequest { id; name },AuthOptions = { bearerAuth?: { token? } }, positionalupdateUserProfile(id, request)).user-1) for the pathidand bodyid. With the collision the generated SDK necessarily sends the body'sidin the URL, but the TS wire-test generator mocks the URL from the example's path-parameter value — with differing values the wire test can't pass. That wire-test-generator gap is pre-existing and out of scope; the snippet unit tests still use distinctpath-id/body-idvalues (they don't read example values) so precedence is still asserted.packages/cli/cli/changes/unreleased/dynamic-auth-wrapper-property.yml,generators/typescript/sdk/changes/unreleased/dynamic-snippets-flatten-body-and-auth-wrapper.yml.Testing
generators/typescript-v2/dynamic-snippets/src/__test__/FlattenRequestParameters.test.ts— modified-exhaustivecases (flatten on / default / non-object body / path-param collision) plus unmodified real-fixture cases: flatten on →id: "body-id",name: "Ada", nobody:, no"path-id", exactly oneid:; flatten off →id: "path-id"+body: {; endpoint-level path param →updateUserProfile("path-id", {.generators/typescript-v2/dynamic-snippets/src/__test__/AuthWrapperProperty.test.ts— nested vs flat, the real fixture →bearerAuth: { token: "<token>" }, the same fixture undernoSerdeLayer: trueandretainOriginalCasing: true→ stillbearerAuth(neverBearerAuth), and OAuth viajava-endpoint-security-token-subpackageundernoSerdeLayer→oauth: { clientId, clientSecret }.packages/cli/generation/ir-generator-tests/src/dynamic-snippets/__test__/authWrapperPropertyDynamic.test.ts— single scheme → unset;it.eachoverany-auth(ANY) andendpoint-security-auth(ENDPOINT_SECURITY) →Bearer/bearer; real fixture →BearerAuth/bearerAuthand asserts the IR shape the collision logic relies on (inlined request,referencedbody, path paramid).ts-flatten-request-any-auth.jsonas-is, so CLI converter → TS generator run together.pnpm ir:generate+@fern-api/ir-sdkcompile; turbo compile forir-generator,ir-generator-tests,typescript-dynamic-snippets; TS dynamic-snippets suite 50/50; CLI dynamic-snippets suite 253/253; snapshot regeneration 543 passed; seed ts-sdk for the new fixture (2/2, validator passed); Biome format/lint clean.mainvs this branch for the fixture above;mainemitsnew AcmeClient({ token })+{ id: "path-id", body: {...} }, this branch emitsnew AcmeClient({ bearerAuth: { token } })+{ id: "body-id", name: "Ada" }.endpoint-securitywithBearerAuthOAuth client-credentials +APIKeyAuth;flattenRequestParameters: true,noSerdeLayer: true,inlinePathParameters: false) onmainvs this branch, with their TS SDK generated via seed from the same spec as ground truth:POST /v2/MoneyIn/getpaidgoes fromgetpaidv2({ body: {...} })togetpaidv2({ paymentDetails, paymentMethod, ... })(matchesRequestPaymentV2), and the constructor goes from{ clientId, clientSecret }to{ bearerAuth: { clientId, clientSecret } }(matchesOAuthAuthProvider.ClientCredentials = { bearerAuth?: {...} }). Positional path-param endpoints (PUT /Customer/{customerId},PUT /Subscription/{subId}) are unchanged. The only diff across all 268 endpoints is the auth nesting.Link to Devin session: https://app.devin.ai/sessions/0d0077731be1453cbf57f84b5f9ba2ec
Open in Devin Desktop: https://app.devin.ai/desktop/session/0d0077731be1453cbf57f84b5f9ba2ec?variant=devin