Skip to content
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

Consider ComposedSchema in DefaultCodegen#fromRequestBody(..) #358

Merged
merged 1 commit into from
Jun 22, 2018

Conversation

jmini
Copy link
Member

@jmini jmini commented Jun 20, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.1.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

This is a fix for #192.

In DefaultCodegen#fromRequestBody(..) the composed-schema case was going through the default else case.

} else {
CodegenProperty codegenProperty = fromProperty("property", schema);
if (schema.getAdditionalProperties() != null) {// http body is map
LOGGER.error("Map should be supported. Please report to openapi-generator github repo about the issue.");
} else if (codegenProperty != null) {
LOGGER.warn("The following schema has undefined (null) baseType. " +
"It could be due to form parameter defined in OpenAPI v2 spec with incorrect consumes. " +
"A correct 'consumes' for form parameters should be " +
"'application/x-www-form-urlencoded' or 'multipart/form-data'");
LOGGER.warn("schema: " + schema);
LOGGER.warn("Defaulting baseType to UNKNOWN_BASE_TYPE");
codegenProperty.baseType = "UNKNOWN_BASE_TYPE";
codegenParameter.baseName = codegenProperty.baseType;
codegenParameter.baseType = codegenProperty.baseType;
codegenParameter.dataType = codegenProperty.dataType;
codegenParameter.description = codegenProperty.description;
codegenParameter.paramName = toParamName(codegenProperty.baseType);
if (codegenProperty.complexType != null) {
imports.add(codegenProperty.complexType);
}
}
setParameterBooleanFlagWithCodegenProperty(codegenParameter, codegenProperty);
}

In my opinion, the composed-schema is more like the object case. This is why I have added || ModelUtils.isComposedSchema(schema) to the else condition here:

} else if (ModelUtils.isObjectSchema(schema)) {
CodegenModel codegenModel = null;
if (StringUtils.isNotBlank(name)) {
schema.setName(name);
codegenModel = fromModel(name, schema, schemas);
}
if (codegenModel != null && !codegenModel.emptyVars) {
if (StringUtils.isEmpty(bodyParameterName)) {
codegenParameter.baseName = codegenModel.classname;
} else {
codegenParameter.baseName = bodyParameterName;
}
codegenParameter.paramName = toParamName(codegenParameter.baseName);
codegenParameter.baseType = codegenModel.classname;
codegenParameter.dataType = getTypeDeclaration(codegenModel.classname);
codegenParameter.description = codegenModel.description;
imports.add(codegenParameter.baseType);
} else {

A unit test with the spec provided in #192 is added to the change to demonstrate the effect of the change.

@jmini
Copy link
Member Author

jmini commented Jun 20, 2018

Technical Committee for DefaultCodegen is the core Team Members:
@wing328 (2015/07)
@jimschubert (2016/05)
@cbornet (2016/05)
@jaz-ah (2016/05)
@ackintosh (2018/02)
@JFCote (2018/03)
@jmini (2018/04)

Copy link
Member

@JFCote JFCote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I will test it with my big spec and let you know.

@JFCote
Copy link
Member

JFCote commented Jun 21, 2018

@jmini Like I said in the issue itself, the changes work in my spec. So If you want, I can merge this right now.
@wing328 Is this ok with you?

@jmini
Copy link
Member Author

jmini commented Jun 22, 2018

I am also in favor of this change (of course I proposed it), and I think 2 core team members are enough to validate the change.

@jmini jmini merged commit 8f4df45 into OpenAPITools:master Jun 22, 2018
@wing328 wing328 added this to the 3.0.3 milestone Jun 25, 2018
@wing328
Copy link
Member

wing328 commented Jun 25, 2018

I'm very busy these days and didn't have a chance to test it yet. I trust the change made by @jmini is of top quality and the review from you guys are sufficient. I'll take a look later and let you guys know if I've any questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants