[core] Add type and format properties to model of inline response

[ybelenko]

I tried to copy original Schema instance, but failed I guess. Need a small help from a core team.

Closes #6150

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.

[ybelenko]

After a few experiments with schema:

definitions:
  Test:
    properties:
      propertyA:
        type: string
      proeprtyB:
        type: string

• Swagger Editor doesn't show any warnings.
• Builtin validate --recommend method doesn't show any warnings.

Well, let's consider output modelJson isn't broken. But I still insist that it's a bug, because input schema contains type: object while output doesn't.

Since it's a core bug I subscribe @OpenAPITools/generator-core-team

[jimschubert]

I don't think this is a bug necessarily, since any schema without a type defined is an object implicitly. Add a type: object to the output when it exists in the input makes sense, but consider that someone might think this is now a bug when they don't define type: object for input but it's now in the output.

I do think it makes sense to have this, however. OAS 3.1 will have a type: null and I wouldn't be surprised if that became the default.

[ybelenko]

Thanks for the review, Jim!

Add a type: object to the output when it exists in the input makes sense, but consider that someone might think this is now a bug when they don't define type: object for input but it's now in the output.

Is it possible to check that input definition contains type property? I tried to solve it, but it seems that my fix doesn't work, because there are files in samples with additional type field eg. openapi.yaml in go petstore. Can you briefly check the test I've made DefaultCodegenTest.java?

I've read OAS 3.0.3 spec yesterday and can't recall anything like "consider schema without type as object". Can you provide a ref from specification?

[ybelenko]

@jimschubert The reason why I ask about parsing schemas without type is because I have different answer from @sebastien-rosset. Maybe I'm wrong, but he said that parser should guess schema type from additional properties. Here is what he said in Slack conversation:

Sebastian Rosset
Your question is more about JSON schema, since OAI uses JSON schema. The relevant sections are https://tools.ietf.org/html/draft-handrews-json-schema-02#section-4.2.1, https://tools.ietf.org/html/draft-handrews-json-schema-02#section-9.3.2, https://json-schema.org/draft/2019-09/json-schema-validation.html#rfc.section.6.1.1. Since JSON schema is constraint based and has a principle to achieve keyword independance, I believe the constraints are applied whenever applicable. If you take a simpler example:

foo:
  # type: string commented out
  maxLength: 10

then what this means is 1) there is no constraint on the type (because type is not specified), i.e. the type can be null, string, number, object, boolean, array. 2) when the value is a string, then the maximum length of the string is 10.
I think it's the same for properties and type although that would be extremely unusual.

And maybe it's not relevant to current issue directly, but @richardwhiuk said:

Richard Whitehouse
Yes, but, note, the following is valid:

anyOf:
- type: string
- type: boolean

is valid.
However, the following isn't really valid:

allOf:
- type: string
- type: boolean

because there's no valid options.
Also, note, that while something might be valid OpenAPI specification, the OpenAPI specification is vast, and thus not all validators / generators may support it.

[sebastien-rosset]

I don't think this is a bug necessarily, since any schema without a type defined is an object implicitly.

I don't think a schema without type means it is an object implicitly. Where do you see that specified? I think no type means the type can be anything. For example below the value of any_prop can be anything because the type keyword has not been specified.

foo:
  type: object
  properties:
    int_prop:
      type: integer
    any_prop:
      description: a property whose value can be an integer, boolean, number, array or object.

Add a type: object to the output when it exists in the input makes sense, but consider that someone might think this is now a bug when they don't define type: object for input but it's now in the output.

I do think it makes sense to have this, however. OAS 3.1 will have a type: null and I wouldn't be surprised if that became the default.

[sebastien-rosset]

Here I agree type: object was missing. It it there in the original document (modules/openapi-generator/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml), it makes sense to add it.

[sebastien-rosset]

In JSON schema, allOf means a compliant validator must validate the payload against all of the nested schemas under allOf. Since Animal is already specified as a type: object, it's redundant and verbose to add type: object here.

[sebastien-rosset]

IMO, type: object should be present when the schema is a non-composed object type, i.e. a type with declared properties such as Animal in the example. But if the schema is a composed schema (anyOf/oneOf/allOf) where the nested schemas are already of type object, I think adding type: object should not be done, it unnecessarily adds verbosity.

[sebastien-rosset]

In #6150, you state that type is required property, but that is not correct. The type keyword is not required.

[ybelenko]

In #6150, you state that type is required property, but that is not correct. The type keyword is not required.

Yeah, my mistake. Current OAS 3.0 Specification doesn't declare type as required property, but it doesn't say that type can be omitted either. At least two validators(Swagger Editor, builtin validate function) confirm your point which is enough for me.

For me(I need to parse OAS in my own tool) it means that I shouldn't throw exceptions when schema doesn't contain type prop. And I should guess which type is presented based on other properties.

[sebastien-rosset]

In #6150, you state that type is required property, but that is not correct. The type keyword is not required.

Yeah, my mistake. Current OAS 3.0 Specification doesn't declare type as required property, but it doesn't say that type can be omitted either. At least two validators(Swagger Editor, builtin validate function) confirm your point which is enough for me.

For me(I need to parse OAS in my own tool) it means that I shouldn't throw exceptions when schema doesn't contain type prop. And I should guess which type is presented based on other properties.

Just FYI, the relevant sections of the JSON schema are:

JSON Schema uses keywords to assert constraints on JSON instances

and

JSON Schema keywords fall into several general behavior categories. Assertions validate that an instance satisfies constraints, producing a boolean result. [...] Evaluating an instance against a schema involves processing all of the keywords in the schema against the appropriate locations within the instance. Typically, applicator keywords are processed until a schema object with no applicators (and therefore no subschemas) is reached.

So if a keyword is not present, there is no constraint assertion. There is nothing to guess.

Also here I think it's better to use the word keyword rather than property because "property" means something else in JSON schema.

[ybelenko]

So if a keyword is not present, there is no constraint assertion. There is nothing to guess.

My case is a bit different. I'm working on a data mocker. When mock tool gets input schema without type constraint should it generate null or free object?

[sebastien-rosset]

First, let me point out we make a distinction between "any object" versus free form object.
A free-form object is an object (i.e. a dictionary) schema that does not have any declared property AND it accepts arbitrary undeclared properties.
For example { "foo": "bar", "p1": true, "p2": { "a": "b" } } is a free-form object. However true, [ { "a": "b"}, true, "foo" ] and "abc" are not free-form objects because their value is not a dictionary.

The "any type" schema is even more generic than free-form. It's not necessarily an object. It could be an object, but also an array, the null value, an integer, number, string or boolean.

Are you saying the purpose of the data mocker is to automatically generate mock data based on the OpenAPI schema?

[ybelenko]

Are you saying the purpose of the data mocker is to automatically generate mock data based on the OpenAPI schema?

That's how I see it. The main advantage of OAS is auto generation of application parts. OAS perfectly describes how request and response looks like. I've spent so many time on writing server unit tests which can be autogenerated. Usually I need to write fixtures of valid and invalid requests. And that is the place where mock or faker engine fits perfectly.

There are developers that already shows interest in mock server. This feature helps to get demo server as soon as possible. #3545

Here is the tool I make btw - OpenAPI Data mocker.

[sebastien-rosset]

Are you saying the purpose of the data mocker is to automatically generate mock data based on the OpenAPI schema?

That's how I see it. The main advantage of OAS is auto generation of application parts. OAS perfectly describes how request and response looks like. I've spent so many time on writing server unit tests which can be autogenerated. Usually I need to write fixtures of valid and invalid requests. And that is the place where mock or faker engine fits perfectly.

There are developers that already shows interest in mock server. This feature helps to get demo server as soon as possible. #3545

Here is the tool I make btw - OpenAPI Data mocker.

I would consider the following scenarios that don't have the type keyword directly under the schema.

components:
  freeFormSchema:
    description: free form object (must be a dictionary).
      It would be very unusual to write the schema that way, but since the schema
      must match ALL of the allOf nested schema, that means the top-level schema
      must be an object.
    allOf:
      - type: object
    additionalProperties: true
 anyTypeSchema:
   description: any type, the payload can be anything (boolean, integer, null, number,
      string, object, array). Ideally the mock server would be able to generate all of
      these values.
 unusualSchemaWithConstraintsAndNoType:
   description: this is (imo) a very unusual way to specify a schema.
      The type can be anything (boolean, integer, null, number,
      string, object, array)
      When the value is a number, it cannot be more than 10.
      If the value is a string, the length of the string cannot be more than 20.
      Any boolean value is ok.
      If the value is an array, it cannot have more than 30 items.
   maximum: 10
   maxLength: 20
   maxItems: 30
 nothingSchema:
  description: no JSON payload can ever match this schema
   allOf:
   - type: string
   - type: boolean
 composedSchemaAllOf:
  description: The type of this schema depends on the type of the nested schemas.
    The nested schemas must be of the same type, except the scenario when A is
    an integer and B is a number.
   allOf:
   - $ref: '#/components/schemas/A'
   - $ref: '#/components/schemas/B'
 composedSchemaOneOf:
  description: The type of this schema depends on the type of the nested schemas
   oneOf:
   - $ref: '#/components/schemas/A'
   - $ref: '#/components/schemas/B'
 composedSchema:
  description: The type of this schema depends on the type of the nested schemas.
     The nested schemas may have different types.
   anyOf:
   - $ref: '#/components/schemas/A'
   - $ref: '#/components/schemas/B'

[ybelenko]

nothingSchema:
  description: no JSON payload can ever match this schema
   allOf:
   - type: string
   - type: boolean

I don't think so. As far as I remember encoding JSON.stringify("hello world") returns "hello world". So string, boolean or any scalar can be JSON theoretically. Or maybe you mean something else here.

[sebastien-rosset]

nothingSchema:
  description: no JSON payload can ever match this schema
   allOf:
   - type: string
   - type: boolean

I don't think so. As far as I remember encoding JSON.stringify("hello world") returns "hello world". So string, boolean or any scalar can be JSON theoretically. Or maybe you mean something else here.

What I mean is suppose the payload is:

"hello"

A compliant validator must validate the payload against all allOf schemas. So it may start with the first nested schema, it is type: string so validation passes. Then it validates the payload against the next nested allOf schema, which is type: boolean. The payload does not match that type because it is a string. So overall validation fails.

If the payload is the value true, then that payload will also fail because it's not a string.

[ybelenko]

Ohhh, now I see. Thanks for so many test cases. I asked community for weeks with no response at all. Thanks again Sebastien!

[ybelenko]

So, in a conclusion I need to fix behaviour with 4 keywords(allOf, anyOf, oneOf, not). When type keyword is missed in inline schema which contains allOf/anyOf/oneOf/not keyword, I need to search keyword in subschemas.

1. allOf. Can be omitted if any subschema already contains the same type:

Model:
  # type can be omitted because of allOf[1] schema
  type: object
  allOf:
  - properties:
      foobar:
        type: string
  - type: object
    properties:
      foobaz:
        type: string

2. Same with anyOf:

Model:
  # type can be omitted because of allOf[1] schema
  type: object
  anyOf:
  - type: string
  - type: object

3. Same for oneOf:

Model:
  # type can be omitted because of allOf[1] schema
  type: object
  oneOf:
  - type: integer
  - type: object

4. Cannot imagine example with not. I think parent schema type cannot be stripped when not keyword presented.

[sebastien-rosset]

So, in a conclusion I need to fix behaviour with 4 keywords(allOf, anyOf, oneOf, not). When type keyword is missed in inline schema which contains allOf/anyOf/oneOf/not keyword, I need to search keyword in subschemas.

This can get tricky because it recursive, so you can't just look at the direct subschema. Example:

 A:
   allOf:
   - $ref: '#/components/schemas/B'
 B:
   allOf:
   - $ref: '#/components/schemas/C'
 C:
   allOf:
   - $ref: '#/components/schemas/D'

Also, if you are looking for corner cases, here is one that you probably won't see very often :-)

Model:
  anyOf:
   - allOf:
      - type: string
         maxLength: 15
   - type: boolean
   - $ref: '#/components/schemas/D'
  allOf:
  - type: string
    minLength: 10
  - pattern: [a-z]{3,50}
  oneOf:
  - type: string

This is a complicated way to specify the value must be a string between length 10 and 15, and all characters must be lowercase a-z, with possibly some constraints coming from D

[ybelenko]

Small offtopic.

This PR should be merged as temporarily solution from my point of view. I think that we'll end up with parsing spec file directly anyway. It's hard to store all spec data in codegen variables if it's even possible(I don't know how to store deeply nested values in mustache variables). Maybe server framework should be able to parse full spec(yaml|json) and process it directly. There might be issues with referenced schemas if spec divided into many files, but it's doable.

For instance we have stale issue #4513 which cannot be solved because of codegen {{variables}}. These variables cannot show difference between OR and AND logic which presented in original spec.

[ybelenko]

@wing328 or @jimschubert could you check and merge? It's 3 months already.

[jimschubert]

@ybelenko this has been open for 3 months, but you have commits up to 15 days ago. Realize when you make additional changes and force-push, things have to be reviewed again.

If @sebastien-rosset is content with your recent changes, I'm fine to have this merged.

[ybelenko]

@ybelenko this has been open for 3 months, but you have commits up to 15 days ago. Realize when you make additional changes and force-push, things have to be reviewed again.

Sure, but I didn't drop any commit. I rebase and force push to:

1. Fix CI errors
2. Test on latest master.

I've requested your review again at June 13th.

[jimschubert]

Re-approving but awaiting feedback from others whether their concerns have been addressed.

[jimschubert]

@ybelenko just curious, why was rebase necessary rather than merging master up to your branch? The rebase and force-push will often make inline review comments and their resolutions no longer accessible. It also makes it so that contributors can't tell whether this was just a master sync or if there was new work that requires another review.

Reviewing each of your 4 force pushes, I now see there were no changes since Jun 9, but it's unlikely that any contributors are going to dig into force push history when re-reviewing a pull request.

My question still remains that if your last true change was June 9, and concerns were still voiced on June 10, whether or not those concerns are blockers before merge.

[ybelenko]

@ybelenko just curious, why was rebase necessary rather than merging master up to your branch? The rebase and force-push will often make inline review comments and their resolutions no longer accessible. It also makes it so that contributors can't tell whether this was just a master sync or if there was new work that requires another review.

I've been in a project where rebase was the major requirement to all feature branches. Team Lead said: "Keep clean commit history and always rebase to latest master/develop and run tests before merge." and he also said "Never merge master/develop to feature branch". In our project I rebase branch because of samples commit. At first, it's much easier to resolve merge conflicts in samples if I drop Refresh samples commit and generate them again. Also if I don't rebase I have commit history like:

1. Fix 1
2. Fix 2
3. Refresh samples
4. Fix 3
5. Refresh samples

Beside, it was the first time I've changed my solution completely in the middle of PR. I agree that changes was hard to review. My bad.

My question still remains that if your last true change was June 9, and concerns were still voiced on June 10, whether or not those concerns are blockers before merge.

I don't know whether my fix solves @dang-gyg issue because there is no details. I think that @sebastien-rosset concerns solved because I decided to fix it inside InlineModelResolver and not modify {{modelJson}} property directly. So type property saved in model only when it presented in original inline schema.

[sebastien-rosset]

Re-approving but awaiting feedback from others whether their concerns have been addressed.

The changes look good. I did make a couple of suggestions to add code comments. This is to avoid the element of surprise/confusion when future code maintainers will read the source code. It's not immediately obvious the format keyword is legal for object types.

[ybelenko]

Thanks @sebastien-rosset. I'll add those comments and push. Then I'll call everybody to check again 😄

[ybelenko]

Obviously CI failure isn't related to changes, because I modified comments only.

[sebastien-rosset]

thanks! lgtm

[jimschubert]

CircleCI was a timeout I have the type/format change and 20 other attributes were weren't copying in #7106, so I'll merge this and retain your comments when I merge it up into my PR.