-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
Default to OAS-compliant handling of additionalProperties #21170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
7c220fc
to
b38c2fb
Compare
@wing328 I took a crack at swapping the default value of |
@@ -5,3 +5,4 @@ templateDir: modules/openapi-generator/src/main/resources/python-fastapi | |||
sourceFolder: "src" | |||
additionalProperties: | |||
hideGenerationTimestamp: "true" | |||
disallowAdditionalPropertiesIfNotPresent: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wing328 In the bin/configs/python-fastapi.yaml
, as in many other sample configs, I added disallowAdditionalPropertiesIfNotPresent: true
to avoid a PR that has massive changes across all examples. I expected that the samples would have 0 changes as a result, because true
was the default value prior to this PR.
# raise errors for additional fields in the input | ||
for _key in obj.keys(): | ||
if _key not in cls.__properties: | ||
raise ValueError("Error due to additional fields (not defined in Order) in the input: " + _key) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the sample code corresponding to the config I mentioned in #21170 (comment).
This code is being updated to throw errors when there are additional properties, but since the old default value was disallowAdditionalPropertiesIfNotPresent=true
, these models should have already been throwing errors before I made my change. I'm not sure what I'm missing here. Is there some hidden third case for some languages when disallowAdditionalPropertiesIfNotPresent
isn't explicitly set to a true or false?
@@ -152,7 +152,7 @@ Authentication schemes defined for the API: | |||
- apiName: Api | |||
- caseInsensitiveResponseHeaders: | |||
- conditionalSerialization: false | |||
- disallowAdditionalPropertiesIfNotPresent: | |||
- disallowAdditionalPropertiesIfNotPresent: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change in the README for one of the C# samples seems to reinforce the idea that the default value for disallowAdditionalPropertiesIfNotPresent
is a hidden third option rather than true
.
9b01374
to
69520f0
Compare
…erting them all at once
69520f0
to
9ef6192
Compare
9ef6192
to
2d4fb51
Compare
Map<String, String> disallowAdditionalPropertiesIfNotPresentOpts = new HashMap<>(); | ||
disallowAdditionalPropertiesIfNotPresentOpts.put("false", | ||
"The 'additionalProperties' implementation is compliant with the OAS and JSON schema specifications."); | ||
disallowAdditionalPropertiesIfNotPresentOpts.put("true", | ||
"Keep the old (incorrect) behaviour that 'additionalProperties' is set to false by default."); | ||
disallowAdditionalPropertiesIfNotPresentOpt.setEnum(disallowAdditionalPropertiesIfNotPresentOpts); | ||
cliOptions.add(disallowAdditionalPropertiesIfNotPresentOpt); | ||
this.setDisallowAdditionalPropertiesIfNotPresent(true); | ||
this.setDisallowAdditionalPropertiesIfNotPresent(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious: why also set the default value here? Does the default value not flow through automatically via the CliOption
?
This changes the default value for
disallowAdditionalPropertiesIfNotPresent
fromtrue
tofalse
, which means that the generator will now, by default, produce code that complies OpenAPI standards with respect to the handling ofadditionalProperties
.Note that a number of example configs did not have to be updated as a result of this change, which indicates that the
disallowAdditionalPropertiesIfNotPresent
option had no effect on the generators used in those examples.Closes #21169
PR checklist
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.
master
(upcoming7.x.0
minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)