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

[codegen][validation] Add support for 'null' type #5290

Merged
merged 28 commits into from
Feb 12, 2020
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
eb31144
initial commit for null type
sebastien-rosset Feb 1, 2020
56da622
handle scenario when one element in composed schema is 'null' type
sebastien-rosset Feb 1, 2020
00718d4
support 'null' type
sebastien-rosset Feb 1, 2020
8ff9236
fix null pointer exception while evaluating recommendations
sebastien-rosset Feb 2, 2020
7cbcba0
Merge branch 'parameter-npe' into null-type
sebastien-rosset Feb 2, 2020
30111a5
add validation for null type and type array
sebastien-rosset Feb 2, 2020
68bbe2d
add validation for null type and type array
sebastien-rosset Feb 2, 2020
ed14375
Merge remote-tracking branch 'origin' into null-type
sebastien-rosset Feb 4, 2020
2059035
Add openAPI attribute to validation and recommendation
sebastien-rosset Feb 5, 2020
ca257f4
add OpenAPI attribute to validation
sebastien-rosset Feb 5, 2020
bb91104
add validation for nullable and null
sebastien-rosset Feb 5, 2020
57f4a0e
add validation for nullable and null
sebastien-rosset Feb 5, 2020
36b8d62
revert log statement modification
sebastien-rosset Feb 5, 2020
71363d5
support null type
sebastien-rosset Feb 6, 2020
660e4dc
support null type
sebastien-rosset Feb 6, 2020
0f140cc
improve code comments
sebastien-rosset Feb 6, 2020
8ba134d
improve code comments. the warning used to notify the users to use i…
sebastien-rosset Feb 6, 2020
23c8bcc
add code comments
sebastien-rosset Feb 6, 2020
ae76186
code formatting
sebastien-rosset Feb 6, 2020
3e52b6f
add code comments, refactor method
sebastien-rosset Feb 6, 2020
e18ddc3
Merge remote-tracking branch 'origin' into null-type
sebastien-rosset Feb 6, 2020
3b61fb9
fix call to isNullable to handle and 'null' type
sebastien-rosset Feb 7, 2020
68ad45d
add OAS document for test purpose
sebastien-rosset Feb 9, 2020
4d6e09d
add unit tests for null type
sebastien-rosset Feb 10, 2020
364d355
add 'null' type validation
sebastien-rosset Feb 11, 2020
a9d212d
sync from master
sebastien-rosset Feb 11, 2020
e825bbc
fix javadoc issues
sebastien-rosset Feb 11, 2020
023cd47
Add validation rule for the supported values of the 'type' attribute
sebastien-rosset Feb 12, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@

import java.util.*;

/**
* Describes a single operation parameter in the OAS specification.
* A unique parameter is defined by a combination of a name and location.
* Parameters may be located in a path, query, header or cookie.
*/
public class CodegenParameter implements IJsonSchemaValidationProperties {
public boolean isFormParam, isQueryParam, isPathParam, isHeaderParam,
isCookieParam, isBodyParam, hasMore, isContainer,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1744,8 +1744,8 @@ protected Schema<?> getSchemaItems(ArraySchema schema) {
}

/**
* Return the name of the allOf schema
*
* Return the name of the 'allOf' composed schema.
*
* @param names List of names
* @param composedSchema composed schema
* @return name of the allOf schema
Expand All @@ -1758,8 +1758,7 @@ public String toAllOfName(List<String> names, ComposedSchema composedSchema) {
} else if (names.size() == 1) {
return names.get(0);
} else {
LOGGER.warn("allOf with multiple schemas defined. Using only the first one: {}. " +
"To fully utilize allOf, please use $ref instead of inline schema definition", names.get(0));
LOGGER.warn("allOf with multiple schemas defined. Using only the first one: {}", names.get(0));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewer: this was discussed with @wing328:

If I remember correctly, that refers to InlineModelResolver (https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java) not supporting allOf so the warning notifies the users to use $ref instead of defining the schema inline in allOf . We should have improved the InlineModelResolver to better handle inline schema allOf so I think it's ok to remove the warning.

Copy link
Member

@jimschubert jimschubert Feb 12, 2020

Choose a reason for hiding this comment

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

I'm not aware of the discussion, but I don't know if it's related to InlineModelResolver. This method is only called from here:

getSchemaType will call ModelUtils.getInterfaces on a composed schema, then iterate all the schemas and get the first defined schema as the "type" of the composed schema. I don't think that behavior is correct because it assumes or suggests that composed schemas have some concept of "interfaces".

InlineModelResolver has other issues, in that it tries to do a best-guess of models defined inline and resolve them to Schemas defined at the top of the document. This goes against requirements in the spec which claim that inline models should not be considered for composed schemas. At a certain point, we have no information whether our model was initially inline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is also a non-obvious point: the 'allOf' name is constructed from the first element in the "allOf" list. If two separate authors want to model class inheritance, one OAS author may think it's better to put the "parent" class first, and another author may think it's better to put the "parent" last.

return names.get(0);
}
}
Expand Down Expand Up @@ -1818,13 +1817,20 @@ private String getSingleSchemaType(Schema schema) {
/**
* Return the OAI type (e.g. integer, long, etc) corresponding to a schema.
* <pre>$ref</pre> is not taken into account by this method.
*
* If the schema is free-form (i.e. 'type: object' with no properties) or inline
* schema, the returned OAI type is 'object'.
*
* @param schema
* @return type
*/
private String getPrimitiveType(Schema schema) {
if (schema == null) {
throw new RuntimeException("schema cannot be null in getPrimitiveType");
} else if (ModelUtils.isNullType(schema)) {
// The 'null' type is allowed in OAS 3.1 and above. It is not supported by OAS 3.0.x,
// though this tooling supports it.
return "null";
} else if (ModelUtils.isStringSchema(schema) && "number".equals(schema.getFormat())) {
// special handle of type: string, format: number
return "BigDecimal";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ private void flattenRequestBody(OpenAPI openAPI, String pathname, Operation oper
ObjectSchema op = (ObjectSchema) inner;
if (op.getProperties() != null && op.getProperties().size() > 0) {
flattenProperties(op.getProperties(), pathname);
// Generate a unique model name based on the title.
String modelName = resolveModelName(op.getTitle(), null);
Schema innerModel = modelFromProperty(op, modelName);
String existing = matchGenerated(innerModel);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,11 @@ private String toExampleValueRecursive(Schema schema, List<String> included_sche
for (int i=0 ; i< indentation ; i++) indentation_string += " ";
String example = super.toExampleValue(schema);

if (ModelUtils.isNullType(schema) && null != example) {
// The 'null' type is allowed in OAS 3.1 and above. It is not supported by OAS 3.0.x,
// though this tooling supports it.
return "None";
}
// correct "true"s into "True"s, since super.toExampleValue uses "toString()" on Java booleans
if (ModelUtils.isBooleanSchema(schema) && null!=example) {
if ("false".equalsIgnoreCase(example)) example = "False";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -805,13 +805,25 @@ public String getSimpleTypeDeclaration(Schema schema) {
return oasType;
}

/**
* Return a string representation of the Python types for the specified schema.
* Primitive types in the OAS specification are implemented in Python using the corresponding
* Python primitive types.
* Composed types (e.g. allAll, oneOf, anyOf) are represented in Python using list of types.
*
* @param p The OAS schema.
* @param prefix prepended to the returned value.
* @param suffix appended to the returned value.
* @return a string representation of the Python types
*/
public String getTypeString(Schema p, String prefix, String suffix) {
// this is used to set dataType, which defines a python tuple of classes
String fullSuffix = suffix;
if (")".equals(suffix)) {
fullSuffix = "," + suffix;
}
if (ModelUtils.isNullable(p)) {
// Resolve $ref because ModelUtils.isXYZ methods do not automatically resolve references.
if (ModelUtils.isNullable(ModelUtils.getReferencedSchema(this.openAPI, p))) {
fullSuffix = ", none_type" + suffix;
}
if (ModelUtils.isFreeFormObject(p) && ModelUtils.getAdditionalProperties(p) == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,18 @@ private static void visitContent(OpenAPI openAPI, Content content, OpenAPISchema
}
}

/**
* Invoke the specified visitor function for every schema that matches mimeType in the OpenAPI document.
*
* To avoid infinite recursion, referenced schemas are visited only once. When a referenced schema is visited,
* it is added to visitedSchemas.
*
* @param openAPI the OpenAPI document that contains schema objects.
* @param schema the root schema object to be visited.
* @param mimeType the mime type. TODO: does not seem to be used in a meaningful way.
* @param visitedSchemas the list of referenced schemas that have been visited.
* @param visitor the visitor function which is invoked for every visited schema.
*/
private static void visitSchema(OpenAPI openAPI, Schema schema, String mimeType, List<String> visitedSchemas, OpenAPISchemaVisitor visitor) {
visitor.visit(schema, mimeType);
if (schema.get$ref() != null) {
Expand Down Expand Up @@ -648,13 +660,41 @@ public static Schema getSchema(OpenAPI openAPI, String name) {
return getSchemas(openAPI).get(name);
}

/**
* Return a Map of the schemas defined under /components/schemas in the OAS document.
* The returned Map only includes the direct children of /components/schemas in the OAS document; the Map
* does not include inlined schemas.
*
* @param openAPI the OpenAPI document.
* @return a map of schemas in the OAS document.
*/
public static Map<String, Schema> getSchemas(OpenAPI openAPI) {
if (openAPI != null && openAPI.getComponents() != null && openAPI.getComponents().getSchemas() != null) {
return openAPI.getComponents().getSchemas();
}
return Collections.emptyMap();
}

/**
* Return the list of all schemas in the 'components/schemas' section of an openAPI specification,
* including inlined schemas and children of composed schemas.
*
* @param openAPI OpenAPI document
* @return a list of schemas
*/
public static List<Schema> getAllSchemas(OpenAPI openAPI) {
List<Schema> allSchemas = new ArrayList<Schema>();
List<String> refSchemas = new ArrayList<String>();
getSchemas(openAPI).forEach((key, schema) -> {
// Invoke visitSchema to recursively visit all schema objects, included inlined and composed schemas.
// Use the OpenAPISchemaVisitor visitor function
visitSchema(openAPI, schema, null, refSchemas, (s, mimetype) -> {
allSchemas.add(s);
});
});
return allSchemas;
}

/**
* If a RequestBody contains a reference to an other RequestBody with '$ref', returns the referenced RequestBody if it is found or the actual RequestBody in the other cases.
*
Expand Down Expand Up @@ -971,7 +1011,8 @@ public static List<Schema> getInterfaces(ComposedSchema composed) {
*/
public static String getParentName(ComposedSchema composedSchema, Map<String, Schema> allSchemas) {
List<Schema> interfaces = getInterfaces(composedSchema);

int nullSchemaChildrenCount = 0;
boolean hasAmbiguousParents = false;
List<String> refedWithoutDiscriminator = new ArrayList<>();

if (interfaces != null && !interfaces.isEmpty()) {
Expand All @@ -988,19 +1029,34 @@ public static String getParentName(ComposedSchema composedSchema, Map<String, Sc
return parentName;
} else {
// not a parent since discriminator.propertyName is not set
hasAmbiguousParents = true;
refedWithoutDiscriminator.add(parentName);
}
} else {
// not a ref, doing nothing
// not a ref, doing nothing, except counting the number of times the 'null' type
// is listed as composed element.
if (ModelUtils.isNullType(schema)) {
// If there are two interfaces, and one of them is the 'null' type,
// then the parent is obvious and there is no need to warn about specifying
// a determinator.
nullSchemaChildrenCount++;
}
}
}
if (refedWithoutDiscriminator.size() == 1 && nullSchemaChildrenCount == 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Just a heads up that I don't think this is intended to always indicate polymorphism in the specification. An allOf ref with discriminator is always supposed to indicate polymporphism, but the opposite isn't clear. The case that you've defined here, and that I'm assuming was intended to match the refedWithoutDiscriminator.size() == 1 condition below is a "legacy" implementation from when we'd began implementing allOf support but it wasn't fully supported by the generator.

Details:

  • discriminator document starts with "Adds support for polymorphism."
  • Composition and Inheritance (Polymorphism) states: "To support polymorphism, the OpenAPI Specification adds the discriminator field. When used, the discriminator will be the name of the property that decides which schema definition validates the structure of the model."

It doesn't explain this well enough, but from these two it seems like they're defining subtype polymorphism since other types of polymorphism are either irrelevant or don't make much sense in this usage.

More discussion (we could turn it into an issue as well):

There are additional details of OpenAPI Spec which aren't explained very well with allOf. For example, the discriminator for allOf can be in the $ref schema and not in the schema containing the allOf definition. In this case, the discriminator will use the schema name of the schema with the allOf and $ref. Example:

Pet:
  …
  discriminator:
    propertyName: type
  properties:
    name: type
    type: String
Cat:
  allOf:
    - $ref: #/components/schemas/Pet

Even without the discriminator, I don't think the LOGGER.warn message is totally accurate. For example, the json-schema combining doc shows this example:

{
  "definitions": {
    "address": {
      "type": "object",
      "properties": {
        "street_address": { "type": "string" },
        "city":           { "type": "string" },
        "state":          { "type": "string" }
      },
      "required": ["street_address", "city", "state"]
    }
  },

  "allOf": [
    { "$ref": "#/definitions/address" },
    { "properties": {
        "type": { "enum": [ "residential", "business" ] }
      }
    }
  ]
}

Unfortunately, this doesn't define whether a codegen should model:

Address:
  + StreetAddress: String
  + City: String
  + State: String

TypedAddress <Address>:
    Enum: [ Residential, Business ]

or:

Address:
  + StreetAddress: String
  + City: String
  + State: String
    Enum: [ Residential, Business ]

In this issue, there's no clarification: OAI/OpenAPI-Specification#1467

In fact, if you consider from a REST input modeling perspective, it might make sense that all allOf without discriminator are unique types with properties of the $ref.

I wonder if it may make sense for us to support a vendor extension as a hint whether the user wants to use composition or inheritance for these one-off allOf without a discriminator. Something like:

  • x-codegen-prefer-polymorphism: true. When no discriminator is defined, we'll try to create a parent and child relationship if the target language/generator supports it
  • x-codegen-prefer-polymorphism: false. When no discriminator is defined, we'll merge all properties into one "super" class.

The latter might be challenging to support the spec requirement that these are validated independently like in the case where two refs have the same property with different validation characteristics. This isn't a case we handle now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the quick feedback. Let me digest this and get back to you tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, this doesn't define whether a codegen should model:

Address:
  + StreetAddress: String
  + City: String
  + State: String

TypedAddress <Address>:
    Enum: [ Residential, Business ]

or:

Address:
  + StreetAddress: String
  + City: String
  + State: String
    Enum: [ Residential, Business ]

Above did you mean? 1) the generated code; 2) the serialization format of the JSON document; or 3) both? If (1), I agree it's not defined in the OAS spec, but OAS does not mandate how code generators should be implemented. If (2), isn't https://tools.ietf.org/html/draft-handrews-json-schema-02 section 9.2.1.1 mandating that it must be the second case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • x-codegen-prefer-polymorphism: true. When no discriminator is defined, we'll try to create a parent and child relationship if the target language/generator supports it
  • x-codegen-prefer-polymorphism: false. When no discriminator is defined, we'll merge all properties into one "super" class.

The latter might be challenging to support the spec requirement that these are validated independently like in the case where two refs have the same property with different validation characteristics. This isn't a case we handle now.

I see there are related discussions about defined "subclassOf", but it does not seem to make much progress.

I agree with all your points, they will have to be addressed in separate PRs. For example, I had an issue with inlined schema exactly as you mentioned, and I ended up doing a workaround by using a $ref.

Copy link
Member

Choose a reason for hiding this comment

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

Oh. I wasn't suggesting any additional changes here. I just wanted to comment on the allOf change because of the ambiguity, and hoped it'd generate discussion. There's a lot to be done for allOf support, and I'm hoping we'll make great progress in the 5.0 release.

Regarding your comment:

If (2), isn't https://tools.ietf.org/html/draft-handrews-json-schema-02 section 9.2.1.1 mandating that it must be the second case?

The issue is that JSON Schema focuses on validation only and not on structure. This is further complicated because OpenAPI Specification disassociates it's allOf from JSON Schema:

allOf - Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema.

I think the only sane way to support it cleanly is by having good defaults and vendor extensions to modify each model's behavior. We may also want to consider a configuration option at generation to define a "global" default.

// One schema is a $ref and the other is the 'null' type, so the parent is obvious.
// In this particular case there is no need to specify a discriminator.
hasAmbiguousParents = false;
}
}

// parent name only makes sense when there is a single obvious parent
if (refedWithoutDiscriminator.size() == 1) {
LOGGER.warn("[deprecated] inheritance without use of 'discriminator.propertyName' is deprecated " +
"and will be removed in a future release. Generating model for composed schema name: {}. Title: {}",
composedSchema.getName(), composedSchema.getTitle());
if (hasAmbiguousParents) {
LOGGER.warn("[deprecated] inheritance without use of 'discriminator.propertyName' is deprecated " +
"and will be removed in a future release. Generating model for composed schema name: {}. Title: {}",
composedSchema.getName(), composedSchema.getTitle());
}
return refedWithoutDiscriminator.get(0);
}

Expand Down Expand Up @@ -1080,6 +1136,24 @@ else if (schema instanceof ComposedSchema) {
return false;
}

/**
* Return true if the 'nullable' attribute is set to true in the schema, i.e. if the value
* of the property can be the null value.
*
* In addition, if the OAS document is 3.1 or above, isNullable returns true if the input
* schema is a 'oneOf' composed document with at most two children, and one of the children
* is the 'null' type.
*
* The caller is responsible for resolving schema references before invoking isNullable.
* If the input schema is a $ref and the referenced schema has 'nullable: true', this method
* returns false (because the nullable attribute is defined in the referenced schema).
*
* The 'nullable' attribute was introduced in OAS 3.0.
* The 'nullable' attribute is deprecated in OAS 3.1. In a OAS 3.1 document, the preferred way
* to specify nullable properties is to use the 'null' type.
*
* @param schema the OAS schema.
*/
public static boolean isNullable(Schema schema) {
if (schema == null) {
return false;
Expand All @@ -1092,7 +1166,59 @@ public static boolean isNullable(Schema schema) {
if (schema.getExtensions() != null && schema.getExtensions().get("x-nullable") != null) {
return Boolean.valueOf(schema.getExtensions().get("x-nullable").toString());
}
// In OAS 3.1, the recommended way to define a nullable property or object is to use oneOf.
if (schema instanceof ComposedSchema) {
return isNullableComposedSchema(((ComposedSchema) schema));
}
return false;
}

/**
* Return true if the specified composed schema is 'oneOf', contains one or two elements,
* and at least one of the elements is the 'null' type.
*
* The 'null' type is supported in OAS 3.1 and above.
* In the example below, the 'OptionalOrder' can have the null value because the 'null'
* type is one of the elements under 'oneOf'.
*
* OptionalOrder:
* oneOf:
* - type: 'null'
* - $ref: '#/components/schemas/Order'
*
* @param schema the OAS composed schema.
* @return true if the composed schema is nullable.
*/
public static boolean isNullableComposedSchema(ComposedSchema schema) {
List<Schema> oneOf = schema.getOneOf();
if (oneOf != null && oneOf.size() <= 2) {
for (Schema s : oneOf) {
if (isNullType(s)) {
return true;
}
}
}
return false;
}

/**
* isNullType returns true if the input schema is the 'null' type.
*
* The 'null' type is supported in OAS 3.1 and above. It is not supported
* in OAS 2.0 and OAS 3.0.x.
*
* For example, the "null" type could be used to specify that a value must
* either be null or a specified type:
*
* OptionalOrder:
* oneOf:
* - type: 'null'
* - $ref: '#/components/schemas/Order'
*/
public static boolean isNullType(Schema schema) {
if ("null".equals(schema.getType())) {
return true;
}
return false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,10 @@ public ValidationResult validate(OpenAPI specification) {
ModelUtils.getUnusedSchemas(specification).forEach(schemaName -> validationResult.addResult(Validated.invalid(unusedSchema, "Unused model: " + schemaName)));
}

Map<String, Schema> schemas = ModelUtils.getSchemas(specification);
schemas.forEach((key, schema) -> {
// Get list of all schemas under /components/schemas, including nested schemas defined inline and composed schema.
// The validators must be able to validate every schema defined in the OAS document.
List<Schema> schemas = ModelUtils.getAllSchemas(specification);
schemas.forEach(schema -> {
SchemaWrapper wrapper = new SchemaWrapper(specification, schema);
validationResult.consume(schemaValidations.validate(wrapper));
});
Expand Down
Loading