-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Better support for inline schemas in parameters #12369
Conversation
* @param schema potentially containing a '$ref' | ||
* @return true if it's a model with at least one properties | ||
*/ | ||
public static boolean isModelWithPropertiesOnly(Schema schema) { |
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.
- Do you want this to be true for AnyType models with properties and additionalProperties set? Right now it is true for that case.
- Do you want this to be true for array/string/number models with properties and additionalProperties set? Right now it is true for that case.
How about limiting this to type object and AnyType models only?
Also type object with additionalProperties unset is the same as type object with additionalProperties true so why is this code returning different results for those same use cases?
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.
Thanks for the review.
I previously didn't check getAdditionalProperties()
but got a few issues (change in samples). Adding the check seems to fix it but you're right that it needs to revised further.
I simply want to set the isModel flag correctly because now isMap is set to true for a simple model. I've not touched the code in modules/openapi-generator/src/main/java/org/openapitools/codegen/IJsonSchemaValidationProperties.java
for a while so I welcome suggestions on other way to fix the problem.
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.
Let me look into what is happening with python experimental. I think it is changing because the component schema changed
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.
Root cause investigation is here: https://github.com/OpenAPITools/openapi-generator/pull/12369/files#r876380534
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.
Should this be true if the schema also has anyOf or allOf definitions?
...penapi-generator/src/main/java/org/openapitools/codegen/IJsonSchemaValidationProperties.java
Outdated
Show resolved
Hide resolved
if (parameter.getSchema() != null) { | ||
parameterSchema = parameter.getSchema(); | ||
parameterModelName = getParameterDataType(parameter ,parameter.getSchema()); | ||
parameterSchema = ModelUtils.getReferencedSchema(openAPI, parameter.getSchema()); |
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.
Something odd is happening with unaliasSchema
The original code extracts the schema, unaliases it, then uses that unaliased schemas to set properties.
This code instead gets the schema from the ref automatically.
So why is unaliasSchema not handling this case automatically originally?
if (parameterDataType != null) { | ||
codegenParameter.dataType = parameterDataType; | ||
if (parameterModelName != null) { | ||
codegenParameter.dataType = parameterModelName; |
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.
Why should codegenParameters handle dataType and complexType differently than codegenProperty?
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.
The change above simply uses a different (better) name in the code block as parameterDataType
originally aims to store the model name so no change in code logic if I remember correctly.
* Uses unaliasSchema rather than ModelUtils.getReferencedSchema * Fixes python-experimental, delays param schema setting * Samples regenerated * Adds parameterModelName setting back in * Samples regenerated * removes needToSetSchema * Sets schema differently depending on if inline model resolver is used * Adds step for getting ref schema * Samples regen
…Tools/openapi-generator into inline-parameter-enhance
Hi guys. Will this PR fixes #11151 ? |
@Dimon70007 please pull the latest master to give it a try. |
Thanks. I'll try it this evening. |
* master: update gson to newer version in kotlin templates (OpenAPITools#12425) update gson to 2.8.9 in android httpclient (OpenAPITools#12423) update android dependencies to newer versions (OpenAPITools#12421) [Java] Update rest-assured dependencies (OpenAPITools#12420) update cwiki samples Add - Status colors in Wiki, Example per parameter (OpenAPITools#12394) Better support for inline schemas in parameters (OpenAPITools#12369) Upgrade Spring Boot to 2.5.14 / 2.7.0 (OpenAPITools#12408) update java samples Remove javadoc comment for unthrown IOException (OpenAPITools#12401) remove errorObjectSubtype from java client genreator (OpenAPITools#12405) [java][Micronaut] generator fixes (OpenAPITools#11803) Apply style and explode values from encoding for form-encoded request body parameters (OpenAPITools#12162)
Have tried to build from source with
but there were failed tests
Did i do something wrong? |
succeed package jar with |
.map(entry -> { | ||
CodegenProperty property = fromProperty(entry.getKey(), entry.getValue()); | ||
property.baseName = codegenParameter.baseName + "[" + entry.getKey() + "]"; | ||
return property; |
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.
Why is property.baseName being modified here?
Can this be made to work without modifying the baseName?
Why not modify name instead or another string property.
baseName is historically used to store the spec case property name.
isModelWithPropertiesOnly
methodFYI. @OpenAPITools/generator-core-team
PR checklist
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*
.For Windows users, please run the script in Git BASH.