fix(core): mark form parameters required from the schema and its allOf chain

KH
21h ago
+534-6

DefaultCodegen.fromRequestBodyToFormParameters decides whether each parameter of a form request body (application/x-www-form-urlencoded or multipart/form-data) is required. For bodies composed with allOf it is wrong in both directions: it drops required flags that the spec sets, and it sets flags that the spec does not.

Reproducer

modules/openapi-generator/src/test/resources/3_0/form-allof-required.yaml (added by this PR), generated with the Kotlin client (-g kotlin --library jvm-okhttp4, normalizer at its defaults). The bug is in the core, so every generator that reflects required is affected; Kotlin shows it as String (required) versus String? = null.

Fixture path (form body)Spec says requiredOn masterWith this PR
/register (allOf of two members)query, user_id, prop-with-dashregister(query: kotlin.String, userId: kotlin.String? = null, propWithDash: kotlin.String? = null, nickname: kotlin.String? = null)register(query: kotlin.String, userId: kotlin.String, propWithDash: kotlin.String, nickname: kotlin.String? = null)
/register-wrapper (a single allOf member, required on the wrapper)wrapper_reqregisterWrapper(wrapperReq: kotlin.String? = null, wrapperOpt: kotlin.String? = null)registerWrapper(wrapperReq: kotlin.String, wrapperOpt: kotlin.String? = null)
/register-top (required and properties on the schema, plus an allOf member)top, member_reqregisterTop(top: kotlin.String, memberReq: kotlin.String? = null, memberOpt: kotlin.String? = null)registerTop(memberReq: kotlin.String, top: kotlin.String, memberOpt: kotlin.String? = null)
/register-mixed (required and properties on the schema, plus oneOf branches that have their own required)commonregisterMixed(common: kotlin.String, a: kotlin.String? = null, b: kotlin.String? = null)unchanged
/register-intersect (allOf of a member with required: [base_req] and a oneOf whose branches require a and b respectively; no top-level required)base_reqbase_req, a and b requiredonly base_req required
/register-allbranches and /register-anyof-common (allOf with a oneOf / anyOf where every branch requires x, partly via $ref)xx requiredx required (unchanged)

Causes, in the master code:

  1. Names are compared in the wrong form. The allOf lookup compares the collected required names with codegenParameter.paramName, the normalized name (user_id → userId, prop-with-dash → propWithDash), while the spec lists user_id and prop-with-dash. They never match. (/register)

  2. A wrapper's own required is lost. A body that is a single allOf member is unwrapped to the member, and only the member is searched, so the required on the wrapper is never read. (/register-wrapper)

  3. A schema's own required masks its allOf members. With schema.getRequired() != null, the code answers from that list alone and never looks at the required lists of the allOf members. (/register-top; member_req is also hit by cause 1.) The default normalizer rule REFACTOR_ALLOF_WITH_PROPERTIES_ONLY changes a schema that has its own properties next to allOf before it reaches this code, so for that shape the masking does not show through the CLI with default settings (checked with simple member names); it shows with --openapi-normalizer REFACTOR_ALLOF_WITH_PROPERTIES_ONLY=false, and in the unit test below, which parses without normalizing. With default settings the masking shows when the schema has a required list and allOf members but no properties of its own:

    schema:
  required: [x]
  allOf:
    - type: object
      required: [y]
      properties:
        x: {type: string}
        y: {type: string}
    - type: object
      properties:
        z: {type: string}

    On master (x: kotlin.String, y: kotlin.String? = null, z: kotlin.String? = null); with this PR (x: kotlin.String, y: kotlin.String, z: kotlin.String? = null).

  4. oneOf / anyOf alternatives force fields. The required names come from addProperties, which also unions the required lists of oneOf / anyOf alternatives. Those entries only hold if that alternative is chosen. For example, with a nested oneOf:

    schema:
  allOf:
    - type: object
      required: [base]
      properties:
        base: {type: string}
    - oneOf:
        - {type: object, required: [a], properties: {a: {type: string}}}
        - {type: object, required: [b], properties: {b: {type: string}}}

    On master (base: kotlin.String, a: kotlin.String, b: kotlin.String); with this PR (base: kotlin.String, a: kotlin.String? = null, b: kotlin.String? = null). The same happens for a schema with its own properties and a oneOf but no top-level required list. /register-mixed is correct on master only because its own required list takes the branch of cause 3.

Fix

  • required is matched against the schema property name (the map key, i.e. baseName), not the normalized paramName.
  • New collectRequiredFromAllOfChain(schema, required, visited) collects the schema's own required list and recurses into its allOf members, resolving $ref. A oneOf / anyOf met along that chain contributes only the names required by every branch (each branch resolved the same way): such a field is required whichever branch the caller picks, while a branch-only entry must not force the form parameter.
  • It starts from the original schema before the single-allOf unwrapping, so a wrapper's own required counts.
  • When a schema has both a $ref and its own required list (allowed in OpenAPI 3.1), the schema's own list is collected before the $ref is followed.
  • The separate schema.getRequired() != null branch is gone: one lookup covers the schema's own list and the allOf chain.
  • addProperties is unchanged and still collects the properties; the required list it fills is no longer used here.

Behavior change

Regenerating a client for a form body composed with allOf can change the required flags:

  • parameters that the spec marks required through an allOf member, a wrapper's required list, or a name that the generator normalizes (user_id, prop-with-dash) are now required, for example Kotlin userId: kotlin.String? = null → userId: kotlin.String;
  • parameters that were required only because they appear in the required list of some oneOf / anyOf alternatives are now optional, for example a: kotlin.String → a: kotlin.String? = null. A name required by every alternative stays required.

Generators that list required parameters first (the Kotlin client does) may also change the positional order of the form parameters, as registerTop above shows. Plain object bodies without composition and top-level oneOf / anyOf bodies (all parameters optional before and after) are unchanged. A body with its own properties plus oneOf branches but no top-level required can change too, even without allOf: required entries that only some branches list no longer force those fields.

Not changed here:

  • a single allOf that wraps a oneOf / anyOf is unwrapped into a top-level oneOf / anyOf body, whose parameters stay optional by the existing design (so callers can choose which branch to send), even if the wrapper lists required;

Tests

All eight tests call fromRequestBodyToFormParameters of a plain DefaultCodegen on a spec parsed without normalization.

TestAsserts
testAllOfFormRequiredMatchesByBaseName (new)/register: query, user_id and prop-with-dash are required, nickname is not
testTopLevelRequiredDoesNotMaskAllOfMemberRequired (new)/register-top: top and member_req are required, member_opt is not
testSingleAllOfWrapperOwnRequired (new)/register-wrapper: wrapper_req is required, wrapper_opt is not
testOneOfBranchesDoNotForceFormRequired (new)/register-mixed: common is required, a and b (required only inside oneOf branches) are not. This test also passes on master (cause 3 hides cause 4 there), so it guards the new collection against regressing; it does not cover cause 4
testOneOfMemberInsideAllOfDoesNotForceBranchRequired (new)/register-intersect: base_req is required, a and b are not
testRequiredSharedByAllOneOfBranchesInsideAllOfApplies (new)/register-allbranches: x (required by every oneOf branch, one of them via $ref) is required; only_a and only_b are not
testRequiredSiblingOfRefApplies (new)3_1/form-ref-sibling-required.yaml: a required list next to a $ref (in a component and in an allOf member) applies
testRequiredSharedByAllAnyOfBranchesInsideAllOfApplies (new)/register-anyof-common: x2 (required by every anyOf branch) is required; aa and bb are not

Cause 4 (the oneOf / anyOf alternatives forcing fields) is covered by testOneOfMemberInsideAllOfDoesNotForceBranchRequired, which fails on master.

./mvnw -nsu -Denforcer.skip=true -pl modules/openapi-generator -am -Dtest=DefaultCodegenTest -Dsurefire.failIfNoSpecifiedTests=false test

Tests runResult
master + this PR1910 failures, 0 skipped
this PR with the DefaultCodegen.java change reverted (tests kept; run limited to the new tests)75 failures — testAllOfFormRequiredMatchesByBaseName, testTopLevelRequiredDoesNotMaskAllOfMemberRequired, testSingleAllOfWrapperOwnRequired, testOneOfMemberInsideAllOfDoesNotForceBranchRequired, testRequiredSharedByAllAnyOfBranchesInsideAllOfApplies; testOneOfBranchesDoNotForceFormRequired and testRequiredSharedByAllOneOfBranchesInsideAllOfApplies pass on master too and guard against regressions
this PR with only the $ref-sibling handling removed (testRequiredSiblingOfRefApplies run on its own)11 failure — testRequiredSiblingOfRefApplies

Samples

No sample changed (the diff has no samples/ file). Among the 271 distinct local input specs referenced by bin/configs, form bodies with a composed schema occur in two shapes only:

  • /form/oneof in the echo_api specs: type: object with oneOf branches. All parameters are optional with and without this PR.
  • /country in the C# petstore specs: a single allOf member that lists country as required. country is required with and without this PR.

Generating these two shapes (copied into a small spec) with and without the change gives identical signatures, so no sample output is expected to change.

cc @wing328

PR checklist

  • Read the contribution guidelines.
  • Run the following to build the project and update samples — not applicable beyond the build and tests: the change is in the generator core and the diff contains no sample changes (see Samples). export_docs_generators.sh not run: no generator option or metadata changes. 
    ./mvnw clean package || exit
./bin/generate-samples.sh ./bin/configs/*.yaml || exit
./bin/utils/export_docs_generators.sh || exit
    (For Windows users, please run the script in WSL)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request. (Not applicable: this changes the generator core, not a single language; only the maintainer is cc'd above.)

🤖 Generated with Claude Code