Warn on schema constructs that can't be represented in Swagger-Model

[tedepstein]

Support Ticket 137 describes some known limitations of the swagger-models library that defines the data structures for representing a Swagger spec in Java. swagger-models holds the in-memory object graph produced by the swagger parser.

This can cause significant problems downstream, because swagger-models is used by most of the Swagger-OpenAPI tools, including the Swagger-UI and code generators, as well as our own swagger-normalizer, live views, Xtend code gen framework, etc.

We hope there will be a fix for these problems. In the meantime, the validator should warn in cases where a schema construct, though allowed by the Swagger specification, is not handled properly by swagger-models.

Borrowing from @andylowry explanations:

allOf Schema with top-level properties

Here's an example:

definitions:
  person:
    type: object
    properties:
      firstname:
        type: string
      lastname:
        type: string

  employee:
    allOf:
      - $ref: "#/definitions/person"
    properties:
      employeeID:
        type: string
      hireDate:
        type: string

The problem is that, in that representation, the structure used for representing an allOf schema has no provision for direct properties of the allOf schema. So, contrary to the Swagger specification, the Swagger Parser is incapable of handling a schema with both allOf and properties.

Suggested warning text:

A schema with an 'allOf' constraint and a top-level properties list may not be handled correctly. Code generators and UI presentations may recognize properties inherited from schemas in the 'allOf' list, but ignore those in the top-level 'properties' list. Recommended workaround: add an inline object schema to the allOf list, and move the top-level properties into this schema.

allOf not recognized in certain contexts

Example:

  person:
    type: object
    properties:
      firstname:
        type: string
      lastname:
        type: string

  employeeList:
    type: array
    items:
      type: object
      allOf:
        - $ref: "#/definitions/person"
        - type: object
          properties:
            employeeID:
              type: string
            hireDate:
              type: string

There is another Swagger parser bug, also related to the "swagger-models" representation of swagger specs. The problem is that there are a number of contexts where Swagger allows a schema, but where internally, a restricted form of schema representation is used. These include:

• Responses (but not body parameters)
• Array item types
• Property types in object schemas

The restricted representation has no support at all for allOf, and any allOf specifications in your schemas that appear in these contexts are simply dropped silently by the parser.

This means, for example, that the employeeList schema is treated as an array of objects that have no defined properties. It's as if the definition of employeeList looked like this (because the allOf portion is simply ignored by the parser):

  employeeList:
    type: array
    items:
      type: object

Suggested warning text:

A schema in this context may not support the 'allOf' constraint. Code generators and UI presentations may omit properties and constraints defined in 'allOf' component schemas. Recommended workaround: define schema in the top-level 'definitions' object with the 'allOf' constraint, and reference that schema here.

additionalProperties not recognized

@andylowry , is there also a problem with recognition of additionalProperties in some cases? Or the combination of additionalProperties and statically defined properties?

[andylowry]

@tedepstein wrote:

swagger-models is used by most of the Swagger-OpenAPI tools, including the Swagger-UI and code generators

Not by swagger-ui. That's all javascript, and I'm sure it's got its own set of bugs, but I have far less insight into it.

@tedepstein asked:

@andylowry , is there also a problem with recognition of additionalProperties in some cases? Or the combination of additionalProperties and statically defined properties?

Two things:

1. A boolean value is not allowed. Swagger spec doesn't describe how a missing additionalProperties should be treated. JSON Schema states that if it's missing, it's as if an empty schema were supplied, and an empty JSON Schema validates against all instances. So this would suggest that leaving out additionalProperties is equivalent to setting it to true in JSON Schema, meaning that all additional properties are allowed, and their values are unconstrained. The equivalent of a false value for additionalProperties would be any unsatisfiable schema, e.g. an allOf schema with mutually exclusive member schemas.
2. The schema supplied with additionalProperties should be added to the list I gave of contexts in which schemas are captured in a restricted representation, where allOf is not allowed.

One additional failure comes to mind:

• minItems and maxItems are dropped, so all arrays appear to have '*' cardinality.

[andylowry]

Re swagger-ui, while what I said about javascript is correct, it's still completely true that in our product, the java libraries are relevant. This is because the editor content is run through our normalizer - which includes passing the structure through the Java swagger parser - before deserializing as the inline spec included in the generated HTML.

[tedepstein]

@andylowry, thanks for the additional details. BTW, I have raised the issue of inconsistency between additionalProperties default value of false, vs intended semantics of allOf. No reply from the OpenAPI TOC yet.

Relevant discussions:

• NullPointerException in parser with additionalProperties: false swagger-api/swagger-core#1437 (comment)
• JSON Schema Support Proposal OAI/OpenAPI-Specification#741 (comment)

[tedepstein]

@andylowry, note that my summary on the first issue points to an inability to combine additionalProperties with statically defined (regular) properties. That was my understanding, based on discussions related to troubleshooting that was happening at that time. Is that correct?

[andylowry]

Another thing that's disallowed in the "restricted" schema representation: regular properties and additionalProperties in the same schema. If additionalProperties is specified, all regular properties are silently ignored by the parser.

[andylowry]

As background info for anyone interested:

• The full-featured schema representation make use of the Model class and its subclasses in the swagger-models module of the swagger-core project.
  • ModelImpl is used for object schemas and also primitive type schemas
  • ArrayModel is used for array schemas
  • ComposedModel is used for allOf schemas and has no provision for directly contained properties
• The restricted schema representation makes use of Property and its subclasses, also in swagger-models
  • ObjectProperty represents object schemas with regular static properties
  • MappingProperty represents object schemas with additionalProperties and discards static properties.
  • ArrayProperty is used for array properties
  • Many subclasses exist for representing the various primitive types
• The code that notices an allOf and switches to ComposedModel (thereby silently discarding static properties) is in the swagger-core module of the swagger-core project, in util/ModelDeserializer.java
• Anywhere in the model where a schema should be permitted but you see a Java field of type Property constitutes a restricted schema context. E.g. in swagger-models in Response.java you see private Property schema, which means response schemas are restricted. In contrast, in parameters/BodyParameter.java you see Model schema which means body params are not a restricted context.

[andylowry]

@tedepstein wrote:

@andylowry, note that my summary on the first issue points to an inability to combine additionalProperties with statically defined (regular) properties. That was my understanding, based on discussions related to troubleshooting that was happening at that time. Is that correct?

Yes, in restricted contexts, which you properly described your post.