[Codegen] Convert "any type" to oneOf model #6051

sebastien-rosset · 2020-04-25T01:13:59Z

Issue 1:
In the JSON schema and OAS specifications, the array "items" attribute is NOT required. However I found out some language generators assume incorrectly that "items" is required. I am fixing the issue for:

go-experimental
python-experimental

https://json-schema.org/understanding-json-schema/reference/array.html

Issue 2
An OAS document may have a schema that does not specify the "type" attribute. For example, below notice the "type" attribute has not been set in the "annotation" property.

Pet:
  type: object
  properties:
    name:
      type: string
    annotations:
      description: an arbitrary element that can represent any JSON schema,
         The value may be null, integer, boolean, string, number, object, or array.

This is equivalent to writing:

anyOf:
  - type: 'null'
  - type: integer
  - type: string
  - type: number
  - type: object
  - type: array

To help with the code generation, I am proposing that codegen converts the "any type" to a anyOf composed schema. The alternative is every language generator has its own way to handle "any type", but it seems more generic to do this conversion. Potentially this could be controlled by a codegen runtime property.

PR checklist

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

sebastien-rosset · 2020-04-25T01:21:11Z

@wing328 , @jimschubert, what do you think? What is the proper way to handle "any type"? This is just a draft PR to get feedback before I go any further.

wing328 · 2020-04-25T16:00:19Z

Thanks for the PR. Just want to clarify a little bit, oneOf means only (and exactly) one type/schema is matched. If the value is 2 for example, it will match both integer and number so it will fail the match.

anyOf allows matching more than one schema defined in the anyOf schema list.

Ref: https://swagger.io/docs/specification/data-models/data-types/ => "Any Type"

cc @OpenAPITools/generator-core-team

…odegen-any-type-to-oneof-schema-2

sebastien-rosset · 2020-04-27T16:28:54Z

If the value is 2 for example, it will match both integer and number so it will fail the match.

Good point, I am changing to anyOf.

…odegen-any-type-to-oneof-schema-2

…s' attribute is not specified

sebastien-rosset · 2020-04-27T21:58:19Z

@spacether for Python-experimental bug fix.

sebastien-rosset · 2020-04-27T22:00:55Z

...erator/src/main/java/org/openapitools/codegen/languages/PythonClientExperimentalCodegen.java

+                // Per JSON schema specification, the array "items" attribute is optional.
+                // When "items" is not specified, the elements of the array
+                // may be anything at all.
+                return prefix + "[bool, date, datetime, dict, float, int, list, str, none_type]" + fullSuffix;


@spacether , can you help review this? Is this the right way to fix the issue?

So this will only happen if we are in an array, right?
Don't we instead need to cover the ModelUtils.isAnyTypeSchema(schema) case?
How about higher up at line 907 use:

Schema anyType = new Schema(); if (ModelUtils.isAnyTypeSchema(schema)): return prefix + "bool, date, datetime, dict, float, int, list, str, none_type" + fullSufix; } else if ((ModelUtils.isMapSchema(p) || "object".equals(p.getType())) && ... } else if (ModelUtils.isArraySchema(p)) ArraySchema ap = (ArraySchema) p; Schema inner = ap.getItems(); if (inner == null) { inner = anyType; } return prefix + "[" + getTypeString(inner, "", "") + "]" + fullSuffix; ...

We also need the if (ModelUtils.isAnyTypeSchema(schema)): in case we travel through an object(dict) to get there. In that case, our return would look like:
{str: (bool, date, datetime, dict, float, int, list, str, none_type)}

…s' attribute is not specified

sebastien-rosset · 2020-04-28T02:09:45Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

+        cs.addAnyOfItem(new ArraySchema());
+        if (schema != null) {
+            cs.setTitle(schema.getTitle());
+            cs.setDescription(schema.getDescription());


Initially I thought I should create a single "AnyType" schema, but in reality there may be corner cases where constraints other than "type" have been specified:
title
pattern
required
enum
minimum
maximum
exclusiveMinimum
exclusiveMaximum
multipleOf
minLength
maxLength
minItems
maxItems
uniqueItems
minProperties
maxProperties

But aren't those constraints specific to types? If you had minItems it would only apply to dict and array, right? My take is if they have any of those constraints then they should be fully explicit and list all types. We are just trying to cover this one super general yoy said it could be anything case. What do you think?

That way that constraint question is the headache of specific generators. I agree with you about adding this model/schema once and then using it multiple places if the writers used it multiple places.

But aren't those constraints specific to types? If you had minItems it would only apply to dict and array, right? My take is if they have any of those constraints then they should be fully explicit and list all types. We are just trying to cover this one super general yoy said it could be anything case. What do you think?

I am planning to handle these edge cases later. Right now just trying to make the simple case work, i.e. no OAS attribute whatsoever is defined in the OAS schema, it's really any type. Even this simple case is not so simple.

Also, the good news is most of these constraints are specific to a type. The only constraints that apply to all types are type, enum and const: https://tools.ietf.org/html/draft-handrews-json-schema-validation-02

spacether · 2020-04-28T02:32:39Z

If the value is 2 for example, it will match both integer and number so it will fail the match.

Good point, I am changing to anyOf.

Shouldn't it be oneOf? Why should it be anyOf? null can never be a Dict or List.

sebastien-rosset · 2020-04-28T02:38:18Z

If the value is 2 for example, it will match both integer and number so it will fail the match.

Good point, I am changing to anyOf.

Shouldn't it be oneOf? Why should it be anyOf? null can never be a Dict or List.

Because 2 can be both Integer and Number.

spacether · 2020-04-28T17:32:43Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

+        if (ModelUtils.isAnyTypeSchema(schema)) {
+            // "Any type" means the payload can be any type, e.g. integer, number, object, array...
+            return getAnyTypeModel(name, schema);
+        }


This code only covers the case when the user include AnyType as a model schema.
Below I see that you you also handle the Model property case, but that code does not create a schema/model that the property can refer to.

What if we have AnyTypeSchema in:

a Model property

a Model

Inline in a response definition?

For example if AnyType is only in a model property AND/or an inline response definition there is no Model/Schema that it should be $ref connected to. In my opinion, we should make this model so it can be handled by all generators.

How about instead in the InlineModelResolver

pre-processing the the openapi document to check for the locations that need to be fixed

then insert qty 1 anyTypeModel into the openapi document

AnyType: anyOf: - type: "null" - type: integer - type: array ...

link all references that require it to that $ref
Then we don't need any specific logic like you are adding in python-experimental.

wing328 · 2020-04-29T07:14:14Z

In the JSON schema and OAS specifications, the array "items" attribute is NOT required. However I found out some language generators assume incorrectly that "items" is required. I am fixing the issue for:

Just want to clarify a bit. Currently, items is still required:

items - Value MUST be an object and not an array. Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema. items MUST be present if the type is array.

Ref: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.3.md#properties

Next version of OpenAPI Spec should align better with JSON schema: OAI/OpenAPI-Specification#1977

To model array of any type, please use the following instead:

        array_any_value:
          items: {}
        array_any_value_with_desc:
          items: 
            description: inline any value
        array_any_value_nullable:
          items:
            nullable: true
            description: inline any value nullable

(I've a pending PR for better support of any type: #6091)

sebastien-rosset · 2020-04-29T15:51:30Z

In the JSON schema and OAS specifications, the array "items" attribute is NOT required. However I found out some language generators assume incorrectly that "items" is required. I am fixing the issue for:

Just want to clarify a bit. Currently, items is still required:

items - Value MUST be an object and not an array. Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema. items MUST be present if the type is array.

Doesn't this state that if "items" is present, then items must be an object? It does not actually state that items must be present.

Ref: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.3.md#properties

Next version of OpenAPI Spec should align better with JSON schema: OAI/OpenAPI-Specification#1977

To model array of any type, please use the following instead:
        array_any_value:
          items: {}
        array_any_value_with_desc:
          items: 
            description: inline any value
        array_any_value_nullable:
          items:
            nullable: true
            description: inline any value nullable
(I've a pending PR for better support of any type: #6091)

spacether · 2020-04-29T16:47:33Z

But it says:
items MUST be present if the type is array.
Which means items is required for arrays.

sebastien-rosset · 2020-04-29T17:10:00Z

But it says:
items MUST be present if the type is array.
Which means items is required for arrays.

Sorry, good catch, I missed that. So to expand "any type", it would have to be something like this:

AnyType:
  anyOf:
  - type: 'null'
  - type: integer
  - type: number
  - type: string
  - type: object
  - type: array
     items:
       $ref: '#components/schema/AnyType'

spacether · 2020-04-29T17:25:10Z

Yes, I think that that will work.
@wing328 what do you think?

sebastien-rosset · 2020-05-01T04:18:46Z

But it says:
items MUST be present if the type is array.
Which means items is required for arrays.

Sorry, good catch, I missed that. So to expand "any type", it would have to be something like this:
AnyType:
  anyOf:
  - type: 'null'
  - type: integer
  - type: number
  - type: string
  - type: object
  - type: array
     items:
       $ref: '#components/schema/AnyType'

Actually, that statement was removed in the 3.1 specification, to be more inline with JSON schema. So it is no longer required to have items under arrray.

wing328 · 2020-05-01T13:07:11Z

cc @OpenAPITools/generator-core-team as it impacts default codegen

sebastien-rosset added 2 commits April 24, 2020 18:13

Early draft to handle conversion from any type to oneOf schema

0c3f479

Early draft to handle conversion from any type to oneOf schema

b1d60d5

Merge branch 'master' of github.com:CiscoM31/openapi-generator into c…

5d746ee

…odegen-any-type-to-oneof-schema-2

sebastien-rosset added 4 commits April 27, 2020 10:00

Replace oneOf with anyOf

2710959

Support any type

fcc47b8

Merge branch 'master' of github.com:CiscoM31/openapi-generator into c…

9ebddb5

…odegen-any-type-to-oneof-schema-2

fix issue in go-experimental and python-experimental when array 'item…

9045d65

…s' attribute is not specified

sebastien-rosset commented Apr 27, 2020

View reviewed changes

sebastien-rosset added 2 commits April 27, 2020 17:19

Fix unit tests to handle any type

1791d16

fix issue in go-experimental and python-experimental when array 'item…

6097031

…s' attribute is not specified

sebastien-rosset commented Apr 28, 2020

View reviewed changes

Convert any type to anyOf

af6496f

spacether reviewed Apr 28, 2020

View reviewed changes

spacether added Client: Go Client: Python labels May 1, 2020

sebastien-rosset closed this May 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Codegen] Convert "any type" to oneOf model #6051

[Codegen] Convert "any type" to oneOf model #6051

sebastien-rosset commented Apr 25, 2020 •

edited

Loading

sebastien-rosset commented Apr 25, 2020

wing328 commented Apr 25, 2020

sebastien-rosset commented Apr 27, 2020

sebastien-rosset commented Apr 27, 2020

sebastien-rosset Apr 27, 2020 •

edited

Loading

spacether Apr 28, 2020

spacether Apr 28, 2020

sebastien-rosset Apr 28, 2020 •

edited

Loading

spacether Apr 28, 2020

spacether Apr 28, 2020

sebastien-rosset Apr 28, 2020 •

edited

Loading

spacether commented Apr 28, 2020 •

edited

Loading

sebastien-rosset commented Apr 28, 2020

spacether Apr 28, 2020 •

edited

Loading

wing328 commented Apr 29, 2020

sebastien-rosset commented Apr 29, 2020

spacether commented Apr 29, 2020

sebastien-rosset commented Apr 29, 2020

spacether commented Apr 29, 2020 •

edited

Loading

sebastien-rosset commented May 1, 2020

wing328 commented May 1, 2020

[Codegen] Convert "any type" to oneOf model #6051

[Codegen] Convert "any type" to oneOf model #6051

Conversation

sebastien-rosset commented Apr 25, 2020 • edited Loading

PR checklist

sebastien-rosset commented Apr 25, 2020

wing328 commented Apr 25, 2020

sebastien-rosset commented Apr 27, 2020

sebastien-rosset commented Apr 27, 2020

sebastien-rosset Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

spacether Apr 28, 2020

Choose a reason for hiding this comment

spacether Apr 28, 2020

Choose a reason for hiding this comment

sebastien-rosset Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

spacether Apr 28, 2020

Choose a reason for hiding this comment

spacether Apr 28, 2020

Choose a reason for hiding this comment

sebastien-rosset Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

spacether commented Apr 28, 2020 • edited Loading

sebastien-rosset commented Apr 28, 2020

spacether Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

wing328 commented Apr 29, 2020

sebastien-rosset commented Apr 29, 2020

spacether commented Apr 29, 2020

sebastien-rosset commented Apr 29, 2020

spacether commented Apr 29, 2020 • edited Loading

sebastien-rosset commented May 1, 2020

wing328 commented May 1, 2020

sebastien-rosset commented Apr 25, 2020 •

edited

Loading

sebastien-rosset Apr 27, 2020 •

edited

Loading

sebastien-rosset Apr 28, 2020 •

edited

Loading

sebastien-rosset Apr 28, 2020 •

edited

Loading

spacether commented Apr 28, 2020 •

edited

Loading

spacether Apr 28, 2020 •

edited

Loading

spacether commented Apr 29, 2020 •

edited

Loading