fix(java): order OAuth token-supplier builder calls for staged builders - #17259
fix(java): order OAuth token-supplier builder calls for staged builders#17259cadesark wants to merge 2 commits into
Conversation
Generated OAuthTokenSupplier.java failed to compile when the OAuth token-exchange request type had a required field that forces a staged builder whose first stage is not the setter the supplier called first (e.g. a required plain-string grant_type with optional client_id/ client_secret). The supplier emitted a hardcoded flat order (.clientId().clientSecret()...), so it called .clientId() on the grantType stage and javac failed with "cannot find symbol". The supplier now orders its builder setter calls to match the generated request type's staged builder: required-field setters first, in the request-type property declaration order, then the optional setters. Required detection mirrors BuilderGenerator (optional/nullable/collection types are not required); literal properties are skipped. Output is byte-identical for token requests whose required order already matched. Adds seed fixture java-oauth-token-required-grant-type reproducing the staged-builder ordering bug. Co-Authored-By: Claude <noreply@anthropic.com>
![nitpickybot[bot]](https://avatars.githubusercontent.com/in/2901896?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
The PR fixes a real staged-builder ordering bug in OAuthTokenSupplierGenerator by ordering builder setter calls to match the generated request type. The core logic mostly mirrors BuilderGenerator.isRequired, but there are a few edge-case concerns around required-order matching and property matching by wire value.
- 🟡 2 warning(s)
- 🔵 1 suggestion(s)
| List<BuilderSetter> required = new ArrayList<>(); | ||
| for (String wireValue : requiredWireValuesInOrder) { | ||
| for (BuilderSetter setter : setters) { | ||
| if (wireValue.equals(setter.wireValue)) { | ||
| required.add(setter); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 warning
orderSettersForBuilder orders required setters by requiredWireValuesInOrder, but if a required property from the request type has no corresponding setter in setters (e.g. a required field the supplier doesn't emit), it's silently skipped. In that case the generated builder is still a staged builder expecting that setter, and .build() won't compile because a required stage was never satisfied. Worth asserting/handling that every required wire value maps to a setter, or at least documenting that assumption.
| private boolean isRequiredBuilderProperty(TypeReference valueType) { | ||
| TypeName poetTypeName = clientGeneratorContext.getPoetTypeNameMapper().convertToTypeName(false, valueType); | ||
| if (poetTypeName instanceof ParameterizedTypeName) { | ||
| ClassName rawType = ((ParameterizedTypeName) poetTypeName).rawType; | ||
| ClassName optionalNullableClassName = | ||
| clientGeneratorContext.getPoetClassNameFactory().getOptionalNullableClassName(); | ||
| return !rawType.equals(ClassName.get(Optional.class)) | ||
| && !rawType.equals(optionalNullableClassName) | ||
| && !rawType.equals(ClassName.get(java.util.Map.class)) | ||
| && !rawType.equals(ClassName.get(List.class)) | ||
| && !rawType.equals(ClassName.get(java.util.Set.class)); | ||
| } | ||
| return true; |
There was a problem hiding this comment.
🟡 warning
isRequiredBuilderProperty returns true for any non-parameterized TypeName, including primitives and enums — that matches BuilderGenerator, but a nullable-wrapped alias or a Nullable<T> that resolves to a raw ClassName (not ParameterizedTypeName) would be misclassified as required. Confirm nullable and aliased-optional types always resolve to a ParameterizedTypeName via the poet mapper; otherwise the ordering will be wrong for those cases.
| List<BuilderSetter> required = new ArrayList<>(); | ||
| for (String wireValue : requiredWireValuesInOrder) { | ||
| for (BuilderSetter setter : setters) { | ||
| if (wireValue.equals(setter.wireValue)) { |
There was a problem hiding this comment.
🔵 suggestion
Matching setters to properties purely by wireValue assumes wire values are unique within the request body. If two distinct properties (e.g. a header and a body field) share a wire value, both setters get pulled into required for a single required property. Consider matching on the actual property identity rather than wire value if collisions are possible.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| private boolean isRequiredBuilderProperty(TypeReference valueType) { | ||
| TypeName poetTypeName = clientGeneratorContext.getPoetTypeNameMapper().convertToTypeName(false, valueType); | ||
| if (poetTypeName instanceof ParameterizedTypeName) { | ||
| ClassName rawType = ((ParameterizedTypeName) poetTypeName).rawType; | ||
| ClassName optionalNullableClassName = | ||
| clientGeneratorContext.getPoetClassNameFactory().getOptionalNullableClassName(); | ||
| return !rawType.equals(ClassName.get(Optional.class)) | ||
| && !rawType.equals(optionalNullableClassName) | ||
| && !rawType.equals(ClassName.get(java.util.Map.class)) | ||
| && !rawType.equals(ClassName.get(List.class)) | ||
| && !rawType.equals(ClassName.get(java.util.Set.class)); | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🟡 Nullable token-request fields can be mis-ordered, breaking generated OAuth code compilation
A token-request field is classified as required (isRequiredBuilderProperty at generators/java/sdk/src/main/java/com/fern/java/client/generators/OAuthTokenSupplierGenerator.java:516-529) purely from its mapped Java type, so a nullable field (or an alias of a nullable field) that maps to a plain type is treated as required even though the request type's builder treats it as optional, so its setter can be emitted in the wrong position and the generated OAuth file fails to compile.
Impact: For an OAuth token request that has a nullable field the generated token supplier can fail to compile.
Divergence from BuilderGenerator.isRequired
The method is documented to mirror BuilderGenerator.isRequired (generators/java/generator-utils/src/main/java/com/fern/java/generators/object/BuilderGenerator.java:1245-1261), but BuilderGenerator.isRequired first short-circuits to false when the property is nullable() or aliasOfNullable(), regardless of the poet type. isRequiredBuilderProperty only inspects convertToTypeName(false, valueType) and returns true for any non-ParameterizedTypeName.
Under use-nullable-annotation, PoetTypeNameMapper.visitNullable returns the inner (non-parameterized) type (generators/java/generator-utils/src/main/java/com/fern/java/PoetTypeNameMapper.java:306-307), and an alias-of-nullable named type resolves to a ClassName. In both cases isRequiredBuilderProperty returns true while the request type's staged builder places the property on _FinalStage (optional). If such a field also has a supplier setter, getRequiredBuilderPropertyWireValuesInOrder (:435-491) includes its wire value, and orderSettersForBuilder (:407-427) places the setter among the required-stage calls, producing a .field(...) call on an intermediate stage that does not expose it — a cannot find symbol compile error, the same failure class this PR fixes.
No existing java OAuth fixture exercises a nullable/alias-of-nullable request property (the nullable fixtures are on responses), so this is latent.
Prompt for agents
isRequiredBuilderProperty in OAuthTokenSupplierGenerator.java is documented to mirror BuilderGenerator.isRequired, but it omits the early short-circuit that BuilderGenerator applies: BuilderGenerator.isRequired returns false whenever the property is nullable() or aliasOfNullable() before it ever inspects the poet type. isRequiredBuilderProperty instead only checks convertToTypeName(false, valueType) and treats any non-ParameterizedTypeName as required. Under use-nullable-annotation config (PoetTypeNameMapper.visitNullable returns the raw inner type) or for alias-of-nullable named types (which resolve to a ClassName), the two classifiers disagree: BuilderGenerator puts the field on _FinalStage (optional) while isRequiredBuilderProperty marks it required and orders its setter among the required-stage calls, which can produce a cannot-find-symbol compile error in the generated OAuthTokenSupplier. Consider detecting the nullable / alias-of-nullable case from the IR ObjectProperty (e.g. via the property's TypeReference / resolved alias) and treating those as not-required, matching BuilderGenerator.isRequired. This is latent today because no OAuth request fixture uses a nullable request property, but it undermines the stated correctness guarantee.
Was this helpful? React with 👍 or 👎 to provide feedback.
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 |
- Add missing ir-to-jsonschema snapshot for the new java-oauth-token-required-grant-type fixture (fixes the failing convertIRtoJsonSchema test in CI). - Share staged-builder required-property detection as a single source of truth (EnrichedObjectProperty.isRequiredForBuilder) used by both BuilderGenerator and OAuthTokenSupplierGenerator so they can never drift. This also fixes misclassification of nullable/aliased-optional token-request properties that resolve to a raw ClassName under use-nullable-annotation. - Throw a clear generation-time error (instead of silently skipping) when a required token-request property has no emitted setter, since the resulting staged builder would not compile. - Match at most one setter per required wire value and document the wire-value-uniqueness assumption. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Description
Generated
OAuthTokenSupplier.javafails to compile when the OAuth token-exchange request type has a required field that forces a Java staged builder whose first stage is not the setter the supplier calls first.When a token-request schema has a required (non-nullable, non-container, non-literal) field, Java codegen (
BuilderGenerator) emits a staged builder:builder() -> <firstRequired>Stage -> ... -> _FinalStage, where each required stage exposes ONLY its own setter (returning the next stage). The token-supplier generator (OAuthTokenSupplierGenerator.buildFetchTokenMethod) previously emitted a hardcoded flat order —.clientId(...),.clientSecret(...), then custom properties — with no awareness of which fields are required.Existing OAuth fixtures pass because their required fields (
client_id/client_secret) happen to be exactly what the supplier emits first, so the flat order coincidentally matched. But when, for example, a required plain-stringgrant_typecomes first (with optionalclient_id/client_secret), the supplier calls.clientId(...)on theGrantTypeStageand javac fails:Changes Made
OAuthTokenSupplierGenerator:buildFetchTokenMethodnow orders the emitted builder setter calls to match the generated request type's builder — required-field setters first, in the request-type property declaration order (required-only), then the optional setters, then.build(). Required detection mirrorsBuilderGenerator.isRequired(a property is required unless it resolves to optional/nullable or a list/set/map). Literal properties are skipped (baked into the type, not builder setters). The request body's declaration order is read from the endpoint (InlinedRequestBody, referenced object type, or file-upload), matching how the request type's builder is generated. Output is byte-identical for token requests whose required order already matched (e.g. requiredclient_id/client_secret), so existing seeds do not churn.java-oauth-token-required-grant-type:oauthclient-credentialsscheme with aget-tokenendpoint whose request has a requiredgrant_type(plain string) and optionalclient_id/client_secret. Reproduces the staged-builder ordering bug and now compiles.seed/java-sdk/seed.yml.generators/java/sdk/changes/unreleased/.Testing
javacfailure on the new fixture via Docker (eclipse-temurin:17-jdk+./gradlew compileJava)..grantType(...).clientId(...).clientSecret(...).build()and./gradlew compileJavapasses.oauth-client-credentials,oauth-client-credentials-mandatory-auth,java-oauth-token-optional);OAuthTokenSupplier.javaoutput is byte-identical (no generator-output changes).generators/java :sdk:test) pass;spotlessApplyapplied to source and fixture.Generated with Claude Code