feat(ir): model request-body omittability separately from its type - #17360
feat(ir): model request-body omittability separately from its type#17360fern-support wants to merge 14 commits into
Conversation
![nitpickybot[bot]](https://avatars.githubusercontent.com/in/2901896?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Adds an optional flag to referenced request bodies in the Fern definition and a corresponding required field on HttpRequestBodyReference in the IR, so omittability is modelled separately from optional<T>. The core change is small (buildEndpoint + schema + IR version bump) with a large snapshot/seed footprint. Main concerns: a hardcoded schemaIdsToExclude: [] in the new early return, a changelog that claims "no-op" while seed output for C#/TS clearly changes, and four required: undefined placeholders where the OpenAPI requestBody.required is already in hand.
- 🟡 2 warning(s)
- 🔵 2 suggestion(s)
| return { | ||
| schemaIdsToExclude: [], | ||
| value: canCollapse ? { body: requestValue.body } : requestValue | ||
| }; |
There was a problem hiding this comment.
🟡 warning
This new early return hardcodes schemaIdsToExclude: [], whereas the fall-through return just below computes it. If the optional-body path can ever reach a request whose schema should be excluded (e.g. an inlined/aliased body schema), those schema ids will now leak back into the generated types. Prefer setting requestValue.body and letting control fall through to the existing return (adjusting only the canCollapse branch), rather than duplicating the return with an empty exclusion list.
There was a problem hiding this comment.
Checked this one — the leak cannot happen, because the fall-through return also hardcodes schemaIdsToExclude: []; neither branch computes it. The referenced-body path never excludes schema ids (only the inlined path below does, via maybeSchemaId).
That said the duplicated return was worth removing, so I collapsed both into a single return with the collapse decision expressed inline.
| This is a no-op for generated SDKs today: the body type generators see is unchanged, so output | ||
| is identical until a generator opts into reading the new field. It removes the need for each | ||
| generator to unwrap `optional<T>` before deciding how to shape the request parameter, which is | ||
| what made enabling the setting change calling conventions in some languages. |
There was a problem hiding this comment.
🟡 warning
"This is a no-op for generated SDKs today" isn't accurate — the seed diffs in this same PR show C# (ISeedApiClient.cs, SeedApiClient.cs, removed Example6.cs, removed mock-server test) and, per the description, TypeScript (request?: Body → request: Body) changing shape for users of respect-optional-request-body. That's a user-visible narrowing for anyone already on the setting. Please state that explicitly in the changelog so consumers aren't surprised.
There was a problem hiding this comment.
Fair — the changelog overclaimed. Rewritten to separate the two populations: definitions without the setting are byte-identical, and definitions with it do change shape. It now names which languages move and in which direction, including that Python/Java/Rust return to their non-setting shape while TypeScript and C# lose the optional parameter until those generators opt in.
| // the direct OpenAPI->IR path does not thread requestBody.required yet | ||
| required: undefined, |
There was a problem hiding this comment.
🔵 suggestion
The OpenAPI requestBody.required is available on this converter, so the four required: undefined placeholders are dropping information that's already in hand. If threading it is out of scope for this PR, mark them // TODO(<issue>) so they're greppable; otherwise a reader will assume undefined is intentional semantics rather than a gap.
There was a problem hiding this comment.
Added TODO(#17360) at all four sites. Threading requestBody.required through the direct OpenAPI→IR path is genuinely out of scope here — this PR is about the IR model and the fern-definition path — but you are right that a bare undefined reads as intentional semantics rather than a gap.
| * | ||
| * This is read by example validation only: it is not carried into the IR, so it | ||
| * does not make the request parameter optional in a generated SDK. |
There was a problem hiding this comment.
🔵 suggestion
Worth calling out the asymmetry explicitly: optional on the inline body is example-validation-only, while the identically named field on HttpReferencedRequestBodySchema now maps to IR required: false. Same key, two different behaviours depending on which body form is used — a sentence pointing at HttpReferencedRequestBodySchema here would save the next reader a bisect.
There was a problem hiding this comment.
Added. The inline field's docs now point at HttpReferencedRequestBodySchema and explain why they differ: an inline body has no single type to mark omittable, so its optional stays validation-only while the referenced one reaches the IR as required: false.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
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 |
ed86ca9 to
8cdd3e2
Compare
respect-optional-request-body expressed "the caller may omit this body" by wrapping requestBodyType in optional<T>. Every generator branches on the container shape to pick a calling convention, so the wrapper changed far more than optionality: Python stopped flattening the body into kwargs, Java swapped Body for Optional<Body> in its overloads, Rust went from &Body to &Option<Body>, and C# produced a nullable parameter with no default. Those are different facts. optional<T> says the value may be null; this says the call may omit the body, in which case the request carries neither content nor a Content-Type header. A body that is sent is always a T. HttpRequestBodyReference gains an optional `required`. Absent means required, so a generator that does not read it sees the same type it always has and emits the same output. Co-Authored-By: Claude <noreply@anthropic.com>
Python, Java and Rust now see a plain body type and fall back to their normal calling conventions, which is the breakage this fixes. TypeScript loses the optional parameter until it opts into reading `required`; restoring it is a widening. Co-Authored-By: Claude <noreply@anthropic.com>
Moving omittability off the type broke the two places that still inferred it from the type, so an example with no request body was silently dropped: convertTypeReferenceExample threw on the undefined example and the throw was swallowed by the catch in convertHttpService, and fern check reported a fatal "requires a request body". Both now treat `optional: true` on a referenced body the way they already treat it on an inline one. The bodyless example is back in the IR, and the Go snippet no longer sends a body it was never given. Also addresses review: changelog no longer claims the change is a no-op for generated SDKs, the four unwired `required` placeholders are marked TODO, the inline/referenced asymmetry on `optional` is documented, and the duplicated return in buildEndpoint is collapsed. Co-Authored-By: Claude <noreply@anthropic.com>
The importer keeps emitting optional<Foo> under respect-optional-request-body. Switching it to the new marker made the flag inert for generators that have not adopted `required` yet, which produced non-compiling Java: the example says "no body" while the wrapper's staged builder requires one, so RefundBody.builder().build() has no build() to call. PR 1 now only adds the model — the IR field, the fern-definition marker, the mapping, and example handling that honours it. Generators adopt `required` one at a time while the importer still emits the old spelling; a later CLI change flips the importer once enough have landed. Seed output and importer snapshots are byte-identical to main. Co-Authored-By: Claude <noreply@anthropic.com>
dependencies.test.ts asserts the byte length of the serialised IR, which grows by 226 bytes now that request body references carry `required`. I reverted this twice while isolating unrelated ETE churn: my filter looked for `"required": null` in the diff, and a length change is a bare number, so it read as noise. Co-Authored-By: Claude <noreply@anthropic.com>
Nothing exercised the new `optional: true` path: every regenerated fixture in the PR shows `"required": null`, so the mapping to `required: false`, the example handling, and the validator branch were all untested. - ir-generator: add an `optionalReferencedBody` endpoint to the omit-request-body fixture and assert the contract that motivates the change — `required` is false while `requestBodyType` stays `named`, where `optional<T>` leaves `required` unset and wraps the type in a container. - validator: endpoint `i` is endpoint `h` plus `optional: true`, so the omitted-request example that violates on `h` passes on `i`, while a present request is still checked against the body type. Co-Authored-By: Claude <noreply@anthropic.com>
A snippet generator only ever sees the dynamic IR, so `required` on `HttpRequestBodyReference` is invisible to it. It also cannot infer omittability from `BodyRequest.body`: an absent `body` means the endpoint has no body at all, which is a different fact from "the caller may skip one". Without its own copy, snippets would keep rendering a body argument for a call that omits it once generators adopt `required`. - `dynamic.BodyRequest` gains an optional `bodyRequired`, folded into the same 67.21.0 minor bump. Absent means required, so snippet output is unchanged until a generator reads it. - `DynamicSnippetsConverter` carries it over from the SDK IR's reference body. - New `optionalRequestBodyDynamic` test pins both directions against the shared fixture. Also corrects two comments this change made misleading: - `RequestBodyConverter` claimed the OpenAPI `requestBody.required` "is not read yet" at four copy-pasted sites. It is read — `this.required` drives the bytes body. The real reason the reference paths leave it unset is that threading it would hand optional-body semantics to every `--from-openapi` user, bypassing the `respect-optional-request-body` opt-in. Replaced with one named constant carrying that explanation. - The gRPC comment now says absent means required, so it no longer reads as a mismatch with `required: undefined`. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The OpenAPI importer no longer wraps a non-required referenced body in optional<Foo>. It describes the body as its own type with `optional: true`, which the IR carries as `required: false`, so the IR says what the spec says regardless of any setting. `respect-optional-request-body` is now a no-op that warns: reading omittability is opt-in per SDK generator, through that generator's own configuration, so a language adopts it when it is ready instead of every language flipping at once. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
a49dd77 to
da52bfd
Compare
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…etting 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>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…loops The csharp-grpc-proto fixtures already run 55-63s on CI runners against a 60s budget, on main as well as this branch. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
3 participants
Description
Refs #17348, #17356
respect-optional-request-bodyexpressed "the caller may omit this body" by wrappingrequestBodyTypeinoptional<T>. Every generator branches on the container shape of the request body to pick a calling convention, so the wrapper changed far more than optionality:optional<T>bodyrequest=objectalpha(String, Body)alpha(String, Optional<Body>)&Body&Option<Body>Body requestBody? requestwith no defaultEach of those is a breaking change to an existing caller, and they were found one at a time because four generators independently re-derive "is this body omittable" from the type shape.
They are two different facts.
optional<T>says the body's value may be null. What the setting means is that the call may omit the body, in which case the request carries neither content nor aContent-Typeheader. A body that is sent is always aT, never an optional of one.Scope
The importer now always carries omittability into the IR. A request body that OpenAPI does not mark
required: trueis described as omittable, the body type is no longer wrapped inoptional<Foo>, and the spec-levelrespect-optional-request-bodysetting is deprecated and inert (it warns onfern check). The on/off switch moves into each SDK generator's own configuration, so a generator that has not adoptedrequiredemits exactly what it did before.An earlier revision left the importer emitting
optional<Foo>and deferred the flip. That ordering produced non-compiling Java: the flag went inert for generators that had not adoptedrequired, so the example said "no body" while the wrapper's staged builder still required one, leavingRefundBody.builder().build()with nobuild()to call. Carrying omittability in the IR and gating on per-generator config avoids that split.Changes Made
HttpRequestBodyReferencegains an optionalrequired. Absent means required, so a generator that does not read it sees the same type it always has and emits the same output.VERSION→67.21.0(minor, additive optional field) + changelog entry. No IR migration needed —ir-migrationsis keyed per major.HttpReferencedRequestBodySchemagainsoptional, which maps torequired: false. This is deliberately distinct frombody: optional<Foo>, which keeps its existing meaning.convertReferenceHttpRequestBodymapsoptional: true→required: false; example conversion and thevalid-example-endpoint-callrule both accept an example that omitsrequestfor such a body.dynamic.BodyRequestgains an optionalbodyRequired, mirroring the field above. A snippet generator only ever sees the dynamic IR and cannot infer omittability frombody, since an absentbodymeans the endpoint has no body at all rather than that the caller may skip one.requiredexplicitly:openapi-to-ir(the--from-openapipath, 4 call sites) andprotoc-gen-fern. Neither threadsrequestBody.required, so both passundefinedand behave exactly as before. Inopenapi-to-irthis is a deliberate gate, not missing wiring —this.requiredis available there and already drives the bytes body; threading it into the reference bodies would hand optional-body semantics to every--from-openapiuser without the opt-in.A note on naming
The concept changes both its name and its default across the stack, which is worth knowing when reading the diff:
requiredfalse)optionalrequiredbodyRequiredThe definition layer says
optionalto match the vocabulary it already uses (optional<T>, and the inline body's existingoptional). The IR saysrequireddeliberately: a field namedoptionalsitting beside arequestBodyTypethat may itself beoptional<Foo>is the exact collision this PR exists to break apart.The default flip is the one to watch. Omitting the key in OpenAPI means optional; omitting the IR field means required. That is forced — the IR field has to default to the old behaviour so generators that ignore it are unaffected — but it means a naive
required: requestBody.requiredin the importer would silently make every OpenAPI body optional. Hence the gate described above.optionalreaches the IR only for referenced bodiesoptional: trueon a referenced body maps torequired: falseand will eventually change generated output. On an inline body the same key is read by example validation and then dropped; it never reaches a generator. An inline body has no single named type to hang omittability off, which is the reason, but it is an implementation constraint leaking into the definition language — same key, same spelling, different reach. Worth resolving before more people write it.Effect on users
respect-optional-request-bodyrequired: true— those now reach the IR as omittablerespect-optional-request-bodyconfig:block, per languagerequired, and an omittable body is no longer wrapped inoptional<T>The one behavioural difference is strictly more permissive: object parsing defaults to
unrecognizedObjectKeys: "fail", sobody: { type: Foo, optional: true }previously failed both arms of the undiscriminated union and errored infern check. It now parses.Per-generator adoption is the point: the generator version becomes the opt-in, so no new config flag is needed and no customer gets a calling-convention change they did not ask for.
Testing
pnpm compileclean across the monorepo. The ripple surfaced in three stages —ir-generator, thenopenapi-to-ir, thenprotoc-gen-fern— each only visible once the previous was fixed.seed/changes, so no generator's output moved — including for therespect-optional-request-bodyfixture.body: optional<Foo>regenerates byte-identical.745c813):optionalReferencedBodyin the omit-request-body IR fixture asserts the contract that motivates the change —requiredisfalsewhilerequestBodyTypestaysnamed, whereoptional<T>leavesrequiredunset and wraps the type in a container.valid-example-endpoint-callendpointiis endpointhplusoptional: true: the omitted-request example that violates onhpasses oni, while a present request is still validated against the body type.Follow-ups
requiredone at a time: TypeScript feat(typescript): let the caller omit an optional request body #17368, Python feat(python): let the caller omit an optional request body #17370, Java feat(java): let the caller omit an optional request body #17372, C# feat(csharp): let the caller omit an optional request body #17373. Go, Ruby, PHP and Rust are not covered yet.67.21.0, which is only publishable once this merges. A stale pin fails differently per language, so this is easy to misread as "CI is red until the IR lands":@fern-api/dynamic-ir-sdk—java-v2andtypescript-v2at66.1.0,csharpat67.12.0. Fails to compile:TS2339: Property 'bodyRequired' does not exist on type 'BodyRequest'.fern_fern_ir_v67 = 67.15.0ingenerators/python/pyproject.toml. Fails at generation time, not build time:AttributeError: 'HttpRequestBodyReference' object has no attribute 'required'.com.fern.fern:irV67:67.21.0published to Maven.bodyRequired(per-generator, alongside the SDK signature change).Content-Typerendered on a bodyless example comes fromtoSnippetHttpRequestinpackages/fdr-sdk/src/api-definition/snippets/SnippetHttpRequest.ts, which sets the header fromendpoint.requests[0].contentTypewithout checking whether the example sends a body. Gating it there is sufficient and needs noisOptionalon FDR'sRegisterJsonRequestBody. The docs SDK snippets are likewise rendered by fern-platform's own@fern-api/snippetsworkspace package, so no generator PR in this repo can change them.