Skip to content

Commit

Permalink
[Core/Rust Server] Check references in additionalProperties correctly…
Browse files Browse the repository at this point in the history
… when checking freeForm status (OpenAPITools#19605)

* Check references in additionalProperties correctly

Handle references in additionalProperties correctly when determining free-form status

* Update samples
  • Loading branch information
richardwhiuk authored and nicolas-vivot committed Sep 23, 2024
1 parent aef33d7 commit e09b516
Show file tree
Hide file tree
Showing 16 changed files with 209 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2424,7 +2424,7 @@ private String getPrimitiveType(Schema schema) {
return schema.getFormat();
}
return "string";
} else if (ModelUtils.isFreeFormObject(schema)) {
} else if (ModelUtils.isFreeFormObject(schema, openAPI)) {
// Note: the value of a free-form object cannot be an arbitrary type. Per OAS specification,
// it must be a map of string to values.
return "object";
Expand Down Expand Up @@ -2833,7 +2833,7 @@ protected void updateModelForObject(CodegenModel m, Schema schema) {
if (ModelUtils.isMapSchema(schema)) {
// an object or anyType composed schema that has additionalProperties set
addAdditionPropertiesToCodeGenModel(m, schema);
} else if (ModelUtils.isFreeFormObject(schema)) {
} else if (ModelUtils.isFreeFormObject(schema, openAPI)) {
// non-composed object type with no properties + additionalProperties
// additionalProperties must be null, ObjectSchema, or empty Schema
addAdditionPropertiesToCodeGenModel(m, schema);
Expand Down Expand Up @@ -3039,7 +3039,7 @@ public CodegenModel fromModel(String name, Schema schema) {
m.isNullable = Boolean.TRUE;
}

m.setTypeProperties(schema);
m.setTypeProperties(schema, openAPI);
m.setFormat(schema.getFormat());
m.setComposedSchemas(getComposedSchemas(schema));
if (ModelUtils.isArraySchema(schema)) {
Expand Down Expand Up @@ -3698,7 +3698,7 @@ protected void updatePropertyForMap(CodegenProperty property, Schema p) {
}

protected void updatePropertyForObject(CodegenProperty property, Schema p) {
if (ModelUtils.isFreeFormObject(p)) {
if (ModelUtils.isFreeFormObject(p, openAPI)) {
// non-composed object type with no properties + additionalProperties
// additionalProperties must be null, ObjectSchema, or empty Schema
property.isFreeFormObject = true;
Expand Down Expand Up @@ -4022,7 +4022,7 @@ public CodegenProperty fromProperty(String name, Schema p, boolean required, boo
property.datatypeWithEnum = property.dataType;
}

property.setTypeProperties(p);
property.setTypeProperties(p, openAPI);
property.setComposedSchemas(getComposedSchemas(p));
if (ModelUtils.isIntegerSchema(p)) { // integer type
updatePropertyForInteger(property, p);
Expand Down Expand Up @@ -4068,7 +4068,7 @@ public CodegenProperty fromProperty(String name, Schema p, boolean required, boo
!ModelUtils.isComposedSchema(p) &&
p.getAdditionalProperties() == null && p.getNot() == null && p.getEnum() == null);

if (!ModelUtils.isArraySchema(p) && !ModelUtils.isMapSchema(p) && !ModelUtils.isFreeFormObject(p) && !isAnyTypeWithNothingElseSet) {
if (!ModelUtils.isArraySchema(p) && !ModelUtils.isMapSchema(p) && !ModelUtils.isFreeFormObject(p, openAPI) && !isAnyTypeWithNothingElseSet) {
/* schemas that are not Array, not ModelUtils.isMapSchema, not isFreeFormObject, not AnyType with nothing else set
* so primitive schemas int, str, number, referenced schemas, AnyType schemas with properties, enums, or composition
*/
Expand Down Expand Up @@ -4865,7 +4865,7 @@ public CodegenResponse fromResponse(String responseCode, ApiResponse response) {
}
}

r.setTypeProperties(responseSchema);
r.setTypeProperties(responseSchema, openAPI);
r.setComposedSchemas(getComposedSchemas(responseSchema));
if (ModelUtils.isArraySchema(responseSchema)) {
r.simpleType = false;
Expand Down Expand Up @@ -4925,7 +4925,7 @@ public CodegenResponse fromResponse(String responseCode, ApiResponse response) {
r.isDouble = Boolean.TRUE;
}
} else if (ModelUtils.isTypeObjectSchema(responseSchema)) {
if (ModelUtils.isFreeFormObject(responseSchema)) {
if (ModelUtils.isFreeFormObject(responseSchema, openAPI)) {
r.isFreeFormObject = true;
} else {
r.isModel = true;
Expand Down Expand Up @@ -5189,7 +5189,7 @@ public CodegenParameter fromParameter(Parameter parameter, Set<String> imports)
}

ModelUtils.syncValidationProperties(parameterSchema, codegenParameter);
codegenParameter.setTypeProperties(parameterSchema);
codegenParameter.setTypeProperties(parameterSchema, openAPI);
codegenParameter.setComposedSchemas(getComposedSchemas(parameterSchema));

if (Boolean.TRUE.equals(parameterSchema.getNullable())) { // use nullable defined in the spec
Expand Down Expand Up @@ -5239,7 +5239,7 @@ public CodegenParameter fromParameter(Parameter parameter, Set<String> imports)
if (ModelUtils.isMapSchema(parameterSchema)) { // for map parameter
updateParameterForMap(codegenParameter, parameterSchema, imports);
}
if (ModelUtils.isFreeFormObject(parameterSchema)) {
if (ModelUtils.isFreeFormObject(parameterSchema, openAPI)) {
codegenParameter.isFreeFormObject = true;
}
addVarsRequiredVarsAdditionalProps(parameterSchema, codegenParameter);
Expand Down Expand Up @@ -7115,7 +7115,7 @@ public CodegenParameter fromFormProperty(String name, Schema propertySchema, Set

Schema ps = unaliasSchema(propertySchema);
ModelUtils.syncValidationProperties(ps, codegenParameter);
codegenParameter.setTypeProperties(ps);
codegenParameter.setTypeProperties(ps, openAPI);
codegenParameter.setComposedSchemas(getComposedSchemas(ps));
if (ps.getPattern() != null) {
codegenParameter.pattern = toRegularExpression(ps.getPattern());
Expand Down Expand Up @@ -7206,7 +7206,7 @@ public CodegenParameter fromFormProperty(String name, Schema propertySchema, Set
codegenParameter.isPrimitiveType = false;
codegenParameter.items = codegenProperty.items;
codegenParameter.mostInnerItems = codegenProperty.mostInnerItems;
} else if (ModelUtils.isFreeFormObject(ps)) {
} else if (ModelUtils.isFreeFormObject(ps, openAPI)) {
codegenParameter.isFreeFormObject = true;
}
} else if (ModelUtils.isNullType(ps)) {
Expand Down Expand Up @@ -7382,7 +7382,7 @@ protected void updateRequestBodyForMap(CodegenParameter codegenParameter, Schema
if (StringUtils.isBlank(name)) {
useModel = false;
} else {
if (ModelUtils.isFreeFormObject(schema)) {
if (ModelUtils.isFreeFormObject(schema, openAPI)) {
useModel = ModelUtils.shouldGenerateFreeFormObjectModel(name, this);
} else if (ModelUtils.isMapSchema(schema)) {
useModel = ModelUtils.shouldGenerateMapModel(schema);
Expand Down Expand Up @@ -7461,7 +7461,7 @@ protected void updateRequestBodyForObject(CodegenParameter codegenParameter, Sch
if (ModelUtils.isMapSchema(schema)) {
// Schema with additionalproperties: true (including composed schemas with additionalproperties: true)
updateRequestBodyForMap(codegenParameter, schema, name, imports, bodyParameterName);
} else if (ModelUtils.isFreeFormObject(schema)) {
} else if (ModelUtils.isFreeFormObject(schema, openAPI)) {
// non-composed object type with no properties + additionalProperties
// additionalProperties must be null, ObjectSchema, or empty Schema
codegenParameter.isFreeFormObject = true;
Expand Down Expand Up @@ -7724,7 +7724,7 @@ public CodegenParameter fromRequestBody(RequestBody body, Set<String> imports, S
schema = ModelUtils.getReferencedSchema(this.openAPI, schema);

ModelUtils.syncValidationProperties(unaliasedSchema, codegenParameter);
codegenParameter.setTypeProperties(unaliasedSchema);
codegenParameter.setTypeProperties(unaliasedSchema, openAPI);
codegenParameter.setComposedSchemas(getComposedSchemas(unaliasedSchema));
// TODO in the future switch al the below schema usages to unaliasedSchema
// because it keeps models as refs and will not get their referenced schemas
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ void generateModels(List<File> files, List<ModelMap> allModels, List<String> unu
if (schema.getExtensions() != null && Boolean.TRUE.equals(schema.getExtensions().get("x-internal"))) {
LOGGER.info("Model {} not generated since x-internal is set to true", name);
continue;
} else if (ModelUtils.isFreeFormObject(schema)) { // check to see if it's a free-form object
} else if (ModelUtils.isFreeFormObject(schema, openAPI)) { // check to see if it's a free-form object
if (!ModelUtils.shouldGenerateFreeFormObjectModel(name, config)) {
LOGGER.info("Model {} not generated since it's a free-form object", name);
continue;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.openapitools.codegen.meta.features.SchemaSupportFeature;
import org.openapitools.codegen.utils.ModelUtils;

import io.swagger.v3.oas.models.OpenAPI;
import io.swagger.v3.oas.models.media.Schema;

public interface IJsonSchemaValidationProperties {
Expand Down Expand Up @@ -283,7 +284,7 @@ public interface IJsonSchemaValidationProperties {
*
* @param p the schema which contains the type info
*/
default void setTypeProperties(Schema p) {
default void setTypeProperties(Schema p, OpenAPI openAPI) {
if (ModelUtils.isModelWithPropertiesOnly(p)) {
setIsModel(true);
} else if (ModelUtils.isArraySchema(p)) {
Expand Down Expand Up @@ -336,7 +337,7 @@ default void setTypeProperties(Schema p) {
setIsNull(true);
} else if (ModelUtils.isAnyType(p)) {
setIsAnyType(true);
} else if (ModelUtils.isFreeFormObject(p)) {
} else if (ModelUtils.isFreeFormObject(p, openAPI)) {
setIsFreeFormObject(true);
// TODO: remove below later after updating generators to properly use isFreeFormObject
setIsMap(true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1117,7 +1117,7 @@ protected void updateModelForObject(CodegenModel m, Schema schema) {
addAdditionPropertiesToCodeGenModel(m, schema);
} else {
m.setIsMap(false);
if (ModelUtils.isFreeFormObject(schema)) {
if (ModelUtils.isFreeFormObject(schema, openAPI)) {
// non-composed object type with no properties + additionalProperties
// additionalProperties must be null, ObjectSchema, or empty Schema
addAdditionPropertiesToCodeGenModel(m, schema);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1702,7 +1702,7 @@ protected void updateModelForObject(CodegenModel m, Schema schema) {
addAdditionPropertiesToCodeGenModel(m, schema);
} else {
m.setIsMap(false);
if (ModelUtils.isFreeFormObject(schema)) {
if (ModelUtils.isFreeFormObject(schema, openAPI)) {
// non-composed object type with no properties + additionalProperties
// additionalProperties must be null, ObjectSchema, or empty Schema
addAdditionPropertiesToCodeGenModel(m, schema);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1123,7 +1123,7 @@ protected void updateModelForObject(CodegenModel m, Schema schema) {
addAdditionPropertiesToCodeGenModel(m, schema);
} else {
m.setIsMap(false);
if (ModelUtils.isFreeFormObject(schema)) {
if (ModelUtils.isFreeFormObject(schema, openAPI)) {
// non-composed object type with no properties + additionalProperties
// additionalProperties must be null, ObjectSchema, or empty Schema
addAdditionPropertiesToCodeGenModel(m, schema);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ public String toDefaultValue(Schema p) {
return "new " + toModelName(ModelUtils.getSimpleRef(p.get$ref())) + "()";
} else if (ModelUtils.isStringSchema(p)) {
return "utility::conversions::to_string_t(\"\")";
} else if (ModelUtils.isFreeFormObject(p)) {
} else if (ModelUtils.isFreeFormObject(p, openAPI)) {
return "new Object()";
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ public String toDefaultValue(Schema p) {
if (ModelUtils.isMapSchema(p)) {
String inner = getSchemaType(ModelUtils.getAdditionalProperties(p));
return "Map[String, " + inner + "]() ";
} else if (ModelUtils.isFreeFormObject(p)) {
} else if (ModelUtils.isFreeFormObject(p, openAPI)) {
// We're opinionated in this template to use ujson
return "ujson.Null";
}
Expand All @@ -143,7 +143,7 @@ public String toDefaultValue(Schema p) {

@Override
public String getSchemaType(Schema p) {
if (ModelUtils.isFreeFormObject(p)) {
if (ModelUtils.isFreeFormObject(p, openAPI)) {
// We're opinionated in this template to use ujson
return "Value";
}
Expand Down Expand Up @@ -829,7 +829,7 @@ public CodegenProperty fromProperty(String name, Schema schema) {
CodegenProperty property = super.fromProperty(name, schema);

// Customize type for freeform objects
if (ModelUtils.isFreeFormObject(schema)) {
if (ModelUtils.isFreeFormObject(schema, openAPI)) {
property.dataType = "Value";
property.baseType = "Value";
}
Expand All @@ -839,7 +839,7 @@ public CodegenProperty fromProperty(String name, Schema schema) {

@Override
public String getTypeDeclaration(Schema schema) {
if (ModelUtils.isFreeFormObject(schema)) {
if (ModelUtils.isFreeFormObject(schema, openAPI)) {
return "Value";
}
return super.getTypeDeclaration(schema);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -844,9 +844,10 @@ public static boolean hasValidation(Schema sc) {
* The value can be any type except the 'null' value.
*
* @param schema potentially containing a '$ref'
* @param openAPI document containing the Schema.
* @return true if it's a free-form object
*/
public static boolean isFreeFormObject(Schema schema) {
public static boolean isFreeFormObject(Schema schema, OpenAPI openAPI) {
if (schema == null) {
// TODO: Is this message necessary? A null schema is not a free-form object, so the result is correct.
once(LOGGER).error("Schema cannot be null in isFreeFormObject check");
Expand Down Expand Up @@ -905,6 +906,8 @@ public static boolean isFreeFormObject(Schema schema) {
if (addlProps == null) {
return true;
} else {
addlProps = getReferencedSchema(openAPI, addlProps);

if (addlProps instanceof ObjectSchema) {
ObjectSchema objSchema = (ObjectSchema) addlProps;
// additionalProperties defined as {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ public void testIsModelAllowsEmptyBaseModel() {
Schema commandSchema = ModelUtils.getSchema(openAPI, "Command");

Assert.assertTrue(ModelUtils.isModel(commandSchema));
Assert.assertFalse(ModelUtils.isFreeFormObject(commandSchema));
Assert.assertFalse(ModelUtils.isFreeFormObject(commandSchema, openAPI));
}

@Test
Expand Down Expand Up @@ -247,23 +247,23 @@ public void testIsFreeFormObject() {
OpenAPI openAPI = new OpenAPI().openapi("3.0.0");
// Create initial "empty" object schema.
ObjectSchema objSchema = new ObjectSchema();
Assert.assertTrue(ModelUtils.isFreeFormObject(objSchema));
Assert.assertTrue(ModelUtils.isFreeFormObject(objSchema, openAPI));

// Set additionalProperties to an empty ObjectSchema.
objSchema.setAdditionalProperties(new ObjectSchema());
Assert.assertTrue(ModelUtils.isFreeFormObject(objSchema));
Assert.assertTrue(ModelUtils.isFreeFormObject(objSchema, openAPI));

// Add a single property to the schema (no longer a free-form object).
Map<String, Schema> props = new HashMap<>();
props.put("prop1", new StringSchema());
objSchema.setProperties(props);
Assert.assertFalse(ModelUtils.isFreeFormObject(objSchema));
Assert.assertFalse(ModelUtils.isFreeFormObject(objSchema, openAPI));

// Test a non-object schema
Assert.assertFalse(ModelUtils.isFreeFormObject(new StringSchema()));
Assert.assertFalse(ModelUtils.isFreeFormObject(new StringSchema(), openAPI));

// Test a null schema
Assert.assertFalse(ModelUtils.isFreeFormObject(null));
Assert.assertFalse(ModelUtils.isFreeFormObject(null, openAPI));
}

@Test
Expand Down Expand Up @@ -326,9 +326,9 @@ public void test30Schemas() {
Assert.assertTrue(ModelUtils.isMapSchema((Schema) misc.getProperties().get("map1")));

// test free form object
Assert.assertTrue(ModelUtils.isFreeFormObject((Schema) misc.getProperties().get("free_form_object_1")));
Assert.assertTrue(ModelUtils.isFreeFormObject((Schema) misc.getProperties().get("free_form_object_2")));
Assert.assertTrue(ModelUtils.isFreeFormObject((Schema) misc.getProperties().get("free_form_object_3")));
Assert.assertTrue(ModelUtils.isFreeFormObject((Schema) misc.getProperties().get("free_form_object_1"), openAPI));
Assert.assertTrue(ModelUtils.isFreeFormObject((Schema) misc.getProperties().get("free_form_object_2"), openAPI));
Assert.assertTrue(ModelUtils.isFreeFormObject((Schema) misc.getProperties().get("free_form_object_3"), openAPI));

// test oneOf
Assert.assertTrue(ModelUtils.isOneOf((Schema) misc.getProperties().get("oneof1")));
Expand Down Expand Up @@ -360,9 +360,9 @@ public void test31Schemas() {
Assert.assertTrue(ModelUtils.isMapSchema((Schema) misc.getProperties().get("map1")));

// test free form object
Assert.assertTrue(ModelUtils.isFreeFormObject((Schema) misc.getProperties().get("free_form_object_1")));
Assert.assertTrue(ModelUtils.isFreeFormObject((Schema) misc.getProperties().get("free_form_object_2")));
Assert.assertTrue(ModelUtils.isFreeFormObject((Schema) misc.getProperties().get("free_form_object_3")));
Assert.assertTrue(ModelUtils.isFreeFormObject((Schema) misc.getProperties().get("free_form_object_1"), openAPI));
Assert.assertTrue(ModelUtils.isFreeFormObject((Schema) misc.getProperties().get("free_form_object_2"), openAPI));
Assert.assertTrue(ModelUtils.isFreeFormObject((Schema) misc.getProperties().get("free_form_object_3"), openAPI));

// test oneOf property
Assert.assertTrue(ModelUtils.isOneOf((Schema) misc.getProperties().get("oneof1")));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,11 @@ components:
$ref: '#/components/schemas/12345AnyOfObject'
required:
- requiredAnyOf
AdditionalPropertiesReferencedAnyOfObject:
description: Check that an object with only additional properties that references another object (e.g. an anyOf object) isn't treated as freeForm
type: object
additionalProperties:
$ref: '#/components/schemas/AnyOfProperty'
AnyOfObject:
description: Test a model containing an anyOf
anyOf:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ Cargo.toml
README.md
api/openapi.yaml
bin/cli.rs
docs/AdditionalPropertiesReferencedAnyOfObject.md
docs/AdditionalPropertiesWithList.md
docs/AdditionalPropertiesWithNullable.md
docs/AnotherXmlArray.md
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ Method | HTTP request | Description

## Documentation For Models

- [AdditionalPropertiesReferencedAnyOfObject](docs/AdditionalPropertiesReferencedAnyOfObject.md)
- [AdditionalPropertiesWithList](docs/AdditionalPropertiesWithList.md)
- [AdditionalPropertiesWithNullable](docs/AdditionalPropertiesWithNullable.md)
- [AnotherXmlArray](docs/AnotherXmlArray.md)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,12 @@ components:
$ref: '#/components/schemas/12345AnyOfObject'
required:
- requiredAnyOf
AdditionalPropertiesReferencedAnyOfObject:
additionalProperties:
$ref: '#/components/schemas/AnyOfProperty'
description: Check that an object with only additional properties that references
another object (e.g. an anyOf object) isn't treated as freeForm
type: object
AnyOfObject:
anyOf:
- $ref: '#/components/schemas/AnyOfObject_anyOf'
Expand Down
Loading

0 comments on commit e09b516

Please sign in to comment.