Repository navigation
fix(core): mark form parameters required from the schema and its allOf chain - #25627
Open
khayashi4337 wants to merge 2 commits into
Open
khayashi4337 wants to merge 2 commits into
khayashi4337 wants to merge 2 commits into
Conversation
…f chain fromRequestBodyToFormParameters decided `required` for form parameters in two flawed ways: - the allOf branch compared the collected required names against the normalized paramName, so properties such as `prop-with-dash` or `user_id` lost the flag; - a top-level `required` list short-circuited the allOf lookup, and the collected list came from addProperties, which also unions the required entries of oneOf/anyOf alternatives. Collect required names only from the schema's own list and its allOf chain (resolving $ref, including a single-allOf wrapper) and match them against the schema property name. Within that collection, a oneOf/anyOf contributes only the required entries shared by every branch: a field required by all alternatives is required whichever the caller picks, while branch-only entries must not force the form parameter. New fixture form-allof-required.yaml and seven DefaultCodegenTest cases covering both over-required and under-required regressions. Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
guaranteedRequired returned early on a $ref, so a `required` list written alongside the reference (allowed in OAS 3.1 / JSON Schema 2020-12) was silently dropped and the corresponding form parameters stayed optional. The schema's own `required` is now collected before following the $ref, both when the pair sits on an allOf member and on a named schema reached through one. New 3.1 fixture form-ref-sibling-required.yaml with a DefaultCodegenTest case covering both placements. Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java:4033">
P2: The `$ref` early return still runs before the allOf/oneOf/anyOf handling. A `$ref` with sibling `allOf`/`oneOf`/`anyOf` (valid in OAS 3.1) therefore drops those members, even though sibling `required` is now honored. Consider walking the composition keywords before returning from the `$ref` branch, or confirm the parser never produces such siblings.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| if (schema == null || !visitedSchemas.add(schema)) { | ||
| return required; | ||
| } | ||
| // collect the schema's own `required` before following $ref: in OAS |
Contributor
There was a problem hiding this comment.
P2: The $ref early return still runs before the allOf/oneOf/anyOf handling. A $ref with sibling allOf/oneOf/anyOf (valid in OAS 3.1) therefore drops those members, even though sibling required is now honored. Consider walking the composition keywords before returning from the $ref branch, or confirm the parser never produces such siblings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java, line 4033:
<comment>The `$ref` early return still runs before the allOf/oneOf/anyOf handling. A `$ref` with sibling `allOf`/`oneOf`/`anyOf` (valid in OAS 3.1) therefore drops those members, even though sibling `required` is now honored. Consider walking the composition keywords before returning from the `$ref` branch, or confirm the parser never produces such siblings.</comment>
<file context>
@@ -4030,13 +4030,16 @@ private Set<String> guaranteedRequired(Schema schema, Set<Schema> visitedSchemas
if (schema == null || !visitedSchemas.add(schema)) {
return required;
}
+ // collect the schema's own `required` before following $ref: in OAS
+ // 3.1 a `required` list may sit next to $ref and would otherwise be
+ // dropped when only the referenced schema is visited
</file context>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DefaultCodegen.fromRequestBodyToFormParametersdecides whether each parameter of a form request body (application/x-www-form-urlencodedormultipart/form-data) is required. For bodies composed withallOfit is wrong in both directions: it dropsrequiredflags 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 reflectsrequiredis affected; Kotlin shows it asString(required) versusString? = null./register(allOfof 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 singleallOfmember,requiredon the wrapper)wrapper_reqregisterWrapper(wrapperReq: kotlin.String? = null, wrapperOpt: kotlin.String? = null)registerWrapper(wrapperReq: kotlin.String, wrapperOpt: kotlin.String? = null)/register-top(requiredandpropertieson the schema, plus anallOfmember)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(requiredandpropertieson the schema, plusoneOfbranches that have their ownrequired)commonregisterMixed(common: kotlin.String, a: kotlin.String? = null, b: kotlin.String? = null)/register-intersect(allOfof a member withrequired: [base_req]and aoneOfwhose branches requireaandbrespectively; no top-levelrequired)base_reqbase_req,aandbrequiredbase_reqrequired/register-allbranchesand/register-anyof-common(allOfwith aoneOf/anyOfwhere every branch requiresx, partly via$ref)xxrequiredxrequired (unchanged)Causes, in the master code:
Names are compared in the wrong form. The
allOflookup compares the collectedrequirednames withcodegenParameter.paramName, the normalized name (user_id→userId,prop-with-dash→propWithDash), while the spec listsuser_idandprop-with-dash. They never match. (/register)A wrapper's own
requiredis lost. A body that is a singleallOfmember is unwrapped to the member, and only the member is searched, so therequiredon the wrapper is never read. (/register-wrapper)A schema's own
requiredmasks itsallOfmembers. Withschema.getRequired() != null, the code answers from that list alone and never looks at therequiredlists of theallOfmembers. (/register-top;member_reqis also hit by cause 1.) The default normalizer ruleREFACTOR_ALLOF_WITH_PROPERTIES_ONLYchanges a schema that has its ownpropertiesnext toallOfbefore 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 arequiredlist andallOfmembers but nopropertiesof its own: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).oneOf/anyOfalternatives force fields. Therequirednames come fromaddProperties, which also unions therequiredlists ofoneOf/anyOfalternatives. Those entries only hold if that alternative is chosen. For example, with a nestedoneOf: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 ownpropertiesand aoneOfbut no top-levelrequiredlist./register-mixedis correct on master only because its ownrequiredlist takes the branch of cause 3.Fix
requiredis matched against the schema property name (the map key, i.e.baseName), not the normalizedparamName.collectRequiredFromAllOfChain(schema, required, visited)collects the schema's ownrequiredlist and recurses into itsallOfmembers, resolving$ref. AoneOf/anyOfmet 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.allOfunwrapping, so a wrapper's ownrequiredcounts.$refand its ownrequiredlist (allowed in OpenAPI 3.1), the schema's own list is collected before the$refis followed.schema.getRequired() != nullbranch is gone: one lookup covers the schema's own list and theallOfchain.addPropertiesis unchanged and still collects the properties; therequiredlist it fills is no longer used here.Behavior change
Regenerating a client for a form body composed with
allOfcan change the required flags:allOfmember, a wrapper'srequiredlist, or a name that the generator normalizes (user_id,prop-with-dash) are now required, for example KotlinuserId: kotlin.String? = null→userId: kotlin.String;requiredlist of someoneOf/anyOfalternatives are now optional, for examplea: 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
registerTopabove shows. Plain object bodies without composition and top-leveloneOf/anyOfbodies (all parameters optional before and after) are unchanged. A body with its ownpropertiesplusoneOfbranches but no top-levelrequiredcan change too, even withoutallOf: required entries that only some branches list no longer force those fields.Not changed here:
allOfthat wraps aoneOf/anyOfis unwrapped into a top-leveloneOf/anyOfbody, whose parameters stay optional by the existing design (so callers can choose which branch to send), even if the wrapper listsrequired;Tests
All eight tests call
fromRequestBodyToFormParametersof a plainDefaultCodegenon a spec parsed without normalization.testAllOfFormRequiredMatchesByBaseName(new)/register:query,user_idandprop-with-dashare required,nicknameis nottestTopLevelRequiredDoesNotMaskAllOfMemberRequired(new)/register-top:topandmember_reqare required,member_optis nottestSingleAllOfWrapperOwnRequired(new)/register-wrapper:wrapper_reqis required,wrapper_optis nottestOneOfBranchesDoNotForceFormRequired(new)/register-mixed:commonis required,aandb(required only insideoneOfbranches) 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 4testOneOfMemberInsideAllOfDoesNotForceBranchRequired(new)/register-intersect:base_reqis required,aandbare nottestRequiredSharedByAllOneOfBranchesInsideAllOfApplies(new)/register-allbranches:x(required by everyoneOfbranch, one of them via$ref) is required;only_aandonly_bare nottestRequiredSiblingOfRefApplies(new)3_1/form-ref-sibling-required.yaml: arequiredlist next to a$ref(in a component and in anallOfmember) appliestestRequiredSharedByAllAnyOfBranchesInsideAllOfApplies(new)/register-anyof-common:x2(required by everyanyOfbranch) is required;aaandbbare notCause 4 (the
oneOf/anyOfalternatives forcing fields) is covered bytestOneOfMemberInsideAllOfDoesNotForceBranchRequired, which fails on master../mvnw -nsu -Denforcer.skip=true -pl modules/openapi-generator -am -Dtest=DefaultCodegenTest -Dsurefire.failIfNoSpecifiedTests=false testDefaultCodegen.javachange reverted (tests kept; run limited to the new tests)testAllOfFormRequiredMatchesByBaseName,testTopLevelRequiredDoesNotMaskAllOfMemberRequired,testSingleAllOfWrapperOwnRequired,testOneOfMemberInsideAllOfDoesNotForceBranchRequired,testRequiredSharedByAllAnyOfBranchesInsideAllOfApplies;testOneOfBranchesDoNotForceFormRequiredandtestRequiredSharedByAllOneOfBranchesInsideAllOfAppliespass on master too and guard against regressions$ref-sibling handling removed (testRequiredSiblingOfRefAppliesrun on its own)testRequiredSiblingOfRefAppliesSamples
No sample changed (the diff has no
samples/file). Among the 271 distinct local input specs referenced bybin/configs, form bodies with a composed schema occur in two shapes only:/form/oneofin theecho_apispecs:type: objectwithoneOfbranches. All parameters are optional with and without this PR./countryin the C# petstore specs: a singleallOfmember that listscountryas required.countryis 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
export_docs_generators.shnot run: no generator option or metadata changes.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.
🤖 Generated with Claude Code