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

Better support for inline schemas in parameters #12369

Merged
merged 11 commits into from
May 20, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.openapitools.codegen.api.TemplatingEngineAdapter;
import org.openapitools.codegen.config.GlobalSettings;
import org.openapitools.codegen.examples.ExampleGenerator;
import org.openapitools.codegen.languages.RustServerCodegen;
import org.openapitools.codegen.meta.FeatureSet;
import org.openapitools.codegen.meta.GeneratorMetadata;
import org.openapitools.codegen.meta.Stability;
Expand Down Expand Up @@ -4575,9 +4576,24 @@ public CodegenParameter fromParameter(Parameter parameter, Set<String> imports)
}

Schema parameterSchema;

// the parameter model name is obtained from the schema $ref
// e.g. #/components/schemas/list_pageQuery_parameter => toModelName(list_pageQuery_parameter)
String parameterModelName = null;

if (parameter.getSchema() != null) {
parameterSchema = parameter.getSchema();
CodegenProperty prop = fromProperty(parameter.getName(), parameterSchema);
parameterModelName = getParameterDataType(parameter, parameterSchema);
CodegenProperty prop;
if (this instanceof RustServerCodegen) {
// for rust server, we need to do somethings special as it uses
// $ref (e.g. #components/schemas/Pet) to determine whether it's a model
prop = fromProperty(parameter.getName(), parameterSchema);
} else if (getUseInlineModelResolver()) {
prop = fromProperty(parameter.getName(), ModelUtils.getReferencedSchema(openAPI, parameterSchema));
} else {
prop = fromProperty(parameter.getName(), parameterSchema);
}
codegenParameter.setSchema(prop);
} else if (parameter.getContent() != null) {
Content content = parameter.getContent();
Expand All @@ -4587,6 +4603,7 @@ public CodegenParameter fromParameter(Parameter parameter, Set<String> imports)
Map.Entry<String, MediaType> entry = content.entrySet().iterator().next();
codegenParameter.contentType = entry.getKey();
parameterSchema = entry.getValue().getSchema();
parameterModelName = getParameterDataType(parameter, parameterSchema);
} else {
parameterSchema = null;
}
Expand All @@ -4613,11 +4630,17 @@ public CodegenParameter fromParameter(Parameter parameter, Set<String> imports)

parameterSchema = unaliasSchema(parameterSchema, Collections.emptyMap());
if (parameterSchema == null) {
LOGGER.warn("warning! Schema not found for parameter \" {} \", using String", parameter.getName());
parameterSchema = new StringSchema().description("//TODO automatically added by openapi-generator due to missing type definition.");
LOGGER.warn("warning! Schema not found for parameter \" {} \"", parameter.getName());
finishUpdatingParameter(codegenParameter, parameter);
return codegenParameter;
}

if (getUseInlineModelResolver() && !(this instanceof RustServerCodegen)) {
// for rust server, we cannot run the following as it uses
// $ref (e.g. #components/schemas/Pet) to determine whether it's a model
parameterSchema = ModelUtils.getReferencedSchema(openAPI, parameterSchema);
}

ModelUtils.syncValidationProperties(parameterSchema, codegenParameter);
codegenParameter.setTypeProperties(parameterSchema);
codegenParameter.setComposedSchemas(getComposedSchemas(parameterSchema));
Expand Down Expand Up @@ -4717,9 +4740,8 @@ public CodegenParameter fromParameter(Parameter parameter, Set<String> imports)
//}
//codegenProperty.required = true;

String parameterDataType = this.getParameterDataType(parameter, parameterSchema);
if (parameterDataType != null) {
codegenParameter.dataType = parameterDataType;
if (parameterModelName != null) {
codegenParameter.dataType = parameterModelName;
Copy link
Contributor

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?

Copy link
Member Author

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.

if (ModelUtils.isObjectSchema(parameterSchema)) {
codegenProperty.complexType = codegenParameter.dataType;
}
Expand Down Expand Up @@ -4771,13 +4793,17 @@ public CodegenParameter fromParameter(Parameter parameter, Set<String> imports)
// https://swagger.io/docs/specification/serialization/
if (schema != null) {
Map<String, Schema<?>> properties = schema.getProperties();
codegenParameter.items.vars =
properties.entrySet().stream()
.map(entry -> {
CodegenProperty property = fromProperty(entry.getKey(), entry.getValue());
property.baseName = codegenParameter.baseName + "[" + entry.getKey() + "]";
return property;
}).collect(Collectors.toList());
if (properties != null) {
codegenParameter.items.vars =
properties.entrySet().stream()
.map(entry -> {
CodegenProperty property = fromProperty(entry.getKey(), entry.getValue());
property.baseName = codegenParameter.baseName + "[" + entry.getKey() + "]";
return property;
Copy link
Contributor

@spacether spacether Oct 9, 2022

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.

}).collect(Collectors.toList());
} else {
//LOGGER.error("properties is null: {}", schema);
}
} else {
LOGGER.warn(
"No object schema found for deepObject parameter{} deepObject won't have specific properties",
Expand All @@ -4790,17 +4816,17 @@ public CodegenParameter fromParameter(Parameter parameter, Set<String> imports)
}

/**
* Returns the data type of a parameter.
* Returns the data type of parameter.
* Returns null by default to use the CodegenProperty.datatype value
*
* @param parameter Parameter
* @param schema Schema
* @return data type
*/
protected String getParameterDataType(Parameter parameter, Schema schema) {
if (parameter.get$ref() != null) {
String refName = ModelUtils.getSimpleRef(parameter.get$ref());
return toModelName(refName);
Schema unaliasSchema = ModelUtils.unaliasSchema(openAPI, schema);
if (unaliasSchema.get$ref() != null) {
return toModelName(ModelUtils.getSimpleRef(unaliasSchema.get$ref()));
}
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,9 @@ public interface IJsonSchemaValidationProperties {
default void setTypeProperties(Schema p) {
if (ModelUtils.isTypeObjectSchema(p)) {
setIsMap(true);
if (ModelUtils.isModelWithPropertiesOnly(p)) {
setIsModel(true);
}
} else if (ModelUtils.isArraySchema(p)) {
setIsArray(true);
} else if (ModelUtils.isFileSchema(p) && !ModelUtils.isStringSchema(p)) {
Expand Down Expand Up @@ -219,6 +222,9 @@ default void setTypeProperties(Schema p) {
setIsNull(true);
} else if (ModelUtils.isAnyType(p)) {
setIsAnyType(true);
if (ModelUtils.isModelWithPropertiesOnly(p)) {
setIsModel(true);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -408,10 +408,10 @@ private void flattenRequestBody(String modelName, Operation operation) {
/**
* Flatten inline models in parameters
*
* @param pathname target pathname
* @param modelName model name
* @param operation target operation
*/
private void flattenParameters(String pathname, Operation operation) {
private void flattenParameters(String modelName, Operation operation) {
List<Parameter> parameters = operation.getParameters();
if (parameters == null) {
return;
Expand All @@ -422,39 +422,19 @@ private void flattenParameters(String pathname, Operation operation) {
continue;
}

Schema model = parameter.getSchema();
if (model instanceof ObjectSchema) {
Schema obj = model;
if (obj.getType() == null || "object".equals(obj.getType())) {
if (obj.getProperties() != null && obj.getProperties().size() > 0) {
flattenProperties(openAPI, obj.getProperties(), pathname);
String modelName = resolveModelName(obj.getTitle(), parameter.getName());
modelName = addSchemas(modelName, model);
parameter.$ref(modelName);
}
}
} else if (model instanceof ArraySchema) {
ArraySchema am = (ArraySchema) model;
Schema inner = am.getItems();
if (inner instanceof ObjectSchema) {
ObjectSchema op = (ObjectSchema) inner;
if (op.getProperties() != null && op.getProperties().size() > 0) {
flattenProperties(openAPI, op.getProperties(), pathname);
String modelName = resolveModelName(op.getTitle(), parameter.getName());
Schema innerModel = modelFromProperty(openAPI, op, modelName);
String existing = matchGenerated(innerModel);
if (existing != null) {
Schema schema = new Schema().$ref(existing);
schema.setRequired(op.getRequired());
am.setItems(schema);
} else {
modelName = addSchemas(modelName, innerModel);
Schema schema = new Schema().$ref(modelName);
schema.setRequired(op.getRequired());
am.setItems(schema);
}
}
}
Schema parameterSchema = parameter.getSchema();

if (parameterSchema == null) {
continue;
}
String schemaName = resolveModelName(parameterSchema.getTitle(),
(operation.getOperationId() == null ? modelName : operation.getOperationId()) + "_" + parameter.getName() + "_parameter");
// Recursively gather/make inline models within this schema if any
gatherInlineModels(parameterSchema, schemaName);
if (isModelNeeded(parameterSchema)) {
// If this schema should be split into its own model, do so
Schema refSchema = this.makeSchemaInComponents(schemaName, parameterSchema);
parameter.setSchema(refSchema);
}
}
}
Expand Down Expand Up @@ -564,29 +544,7 @@ private void flattenComponents() {
flattenComposedChildren(modelName + "_oneOf", m.getOneOf());
} else if (model instanceof Schema) {
gatherInlineModels(model, modelName);
} /*else if (ModelUtils.isArraySchema(model)) {
ArraySchema m = (ArraySchema) model;
Schema inner = m.getItems();
if (inner instanceof ObjectSchema) {
ObjectSchema op = (ObjectSchema) inner;
if (op.getProperties() != null && op.getProperties().size() > 0) {
String innerModelName = resolveModelName(op.getTitle(), modelName + "_inner");
Schema innerModel = modelFromProperty(openAPI, op, innerModelName);
String existing = matchGenerated(innerModel);
if (existing == null) {
openAPI.getComponents().addSchemas(innerModelName, innerModel);
addGenerated(innerModelName, innerModel);
Schema schema = new Schema().$ref(innerModelName);
schema.setRequired(op.getRequired());
m.setItems(schema);
} else {
Schema schema = new Schema().$ref(existing);
schema.setRequired(op.getRequired());
m.setItems(schema);
}
}
}
}*/
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import io.swagger.v3.oas.models.media.FileSchema;
import io.swagger.v3.oas.models.media.Schema;
import io.swagger.v3.oas.models.media.XML;
import io.swagger.v3.oas.models.parameters.Parameter;
import io.swagger.v3.oas.models.parameters.RequestBody;
import io.swagger.v3.oas.models.responses.ApiResponse;
import io.swagger.v3.oas.models.servers.Server;
Expand Down Expand Up @@ -1741,4 +1742,13 @@ protected void updatePropertyForAnyType(CodegenProperty property, Schema p) {

@Override
public GeneratorLanguage generatorLanguage() { return GeneratorLanguage.RUST; }

@Override
protected String getParameterDataType(Parameter parameter, Schema schema) {
if (parameter.get$ref() != null) {
String refName = ModelUtils.getSimpleRef(parameter.get$ref());
return toModelName(refName);
}
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -725,14 +725,33 @@ public static boolean isModel(Schema schema) {
}

// has properties
if (null != schema.getProperties()) {
if (null != schema.getProperties() && !schema.getProperties().isEmpty()) {
return true;
}

// composed schema is a model, consider very simple ObjectSchema a model
return schema instanceof ComposedSchema || schema instanceof ObjectSchema;
}

/**
* Check to see if the schema is a model with properties only (non-composed model)
*
* @param schema potentially containing a '$ref'
* @return true if it's a model with at least one properties
*/
public static boolean isModelWithPropertiesOnly(Schema schema) {
Copy link
Contributor

@spacether spacether May 15, 2022

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?

Copy link
Member Author

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.

Copy link
Contributor

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

Copy link
Contributor

@spacether spacether May 18, 2022

Choose a reason for hiding this comment

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

Copy link
Contributor

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?

if (schema == null) {
return false;
}

if (null != schema.getProperties() && !schema.getProperties().isEmpty() && // has properties
(schema.getAdditionalProperties() == null || // no additionalProperties is set
(schema.getAdditionalProperties() instanceof Boolean && !(Boolean)schema.getAdditionalProperties()))) {
return true;
}
return false;
}

public static boolean hasValidation(Schema sc) {
return (
sc.getMaxItems() != null ||
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2061,11 +2061,12 @@ public void objectQueryParamIdentifyAsObject() {
CodegenParameter parameter = codegen.fromParameter(openAPI.getPaths().get("/pony").getGet().getParameters().get(0), imports);

// TODO: This must be updated to work with flattened inline models
Assert.assertEquals(parameter.dataType, "PageQuery1");
Assert.assertEquals(parameter.dataType, "ListPageQueryParameter");
Assert.assertEquals(imports.size(), 1);
Assert.assertEquals(imports.iterator().next(), "PageQuery1");
Assert.assertEquals(imports.iterator().next(), "ListPageQueryParameter");

Assert.assertNotNull(parameter.getSchema());
Assert.assertEquals(parameter.getSchema().dataType, "Object");
Assert.assertEquals(parameter.getSchema().baseType, "object");
}

Expand Down Expand Up @@ -3167,8 +3168,8 @@ public void testVarsAndRequiredVarsPresent() {

// CodegenOperation puts the inline schema into schemas and refs it
assertTrue(co.responses.get(0).isModel);
assertEquals(co.responses.get(0).baseType, "objectData");
modelName = "objectData";
assertEquals(co.responses.get(0).baseType, "objectWithOptionalAndRequiredProps_request");
modelName = "objectWithOptionalAndRequiredProps_request";
sc = openAPI.getComponents().getSchemas().get(modelName);
cm = codegen.fromModel(modelName, sc);
assertEquals(cm.vars, vars);
Expand All @@ -3180,7 +3181,7 @@ public void testVarsAndRequiredVarsPresent() {
cm = codegen.fromModel(modelName, sc);
CodegenProperty cp = cm.getVars().get(0);
assertTrue(cp.isModel);
assertEquals(cp.complexType, "objectData");
assertEquals(cp.complexType, "objectWithOptionalAndRequiredProps_request");
}

@Test
Expand Down Expand Up @@ -4005,7 +4006,9 @@ public void testRequestParameterContent() {
CodegenMediaType mt = content.get("application/json");
assertNull(mt.getEncoding());
CodegenProperty cp = mt.getSchema();
// TODO need to revise the test below
assertTrue(cp.isMap);
assertTrue(cp.isModel);
assertEquals(cp.complexType, "object");
assertEquals(cp.baseName, "SchemaForRequestParameterCoordinatesInlineSchemaApplicationJson");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ export interface FakeEnumRequestGetInlineRequest {
}

export interface FakeEnumRequestGetRefRequest {
stringEnum?: StringEnum;
stringEnum?: FakeEnumRequestGetRefStringEnumEnum;
nullableStringEnum?: StringEnum | null;
numberEnum?: NumberEnum;
numberEnum?: FakeEnumRequestGetRefNumberEnumEnum;
nullableNumberEnum?: NumberEnum | null;
}

Expand Down Expand Up @@ -210,3 +210,21 @@ export const FakeEnumRequestGetInlineNumberEnumEnum = {
NUMBER_3: 3
} as const;
export type FakeEnumRequestGetInlineNumberEnumEnum = typeof FakeEnumRequestGetInlineNumberEnumEnum[keyof typeof FakeEnumRequestGetInlineNumberEnumEnum];
/**
* @export
*/
export const FakeEnumRequestGetRefStringEnumEnum = {
One: 'one',
Two: 'two',
Three: 'three'
} as const;
export type FakeEnumRequestGetRefStringEnumEnum = typeof FakeEnumRequestGetRefStringEnumEnum[keyof typeof FakeEnumRequestGetRefStringEnumEnum];
/**
* @export
*/
export const FakeEnumRequestGetRefNumberEnumEnum = {
NUMBER_1: 1,
NUMBER_2: 2,
NUMBER_3: 3
} as const;
export type FakeEnumRequestGetRefNumberEnumEnum = typeof FakeEnumRequestGetRefNumberEnumEnum[keyof typeof FakeEnumRequestGetRefNumberEnumEnum];
Loading