Inheritance using 'allOf' seems broken

[HaraldNordgren]

Problems with inheritance and overrides.

Q	A
Bug or feature request?	Bug
Which Swagger/OpenAPI version?	2.0
Which Swagger-UI version?	Using Swagger-UI 3.X, via https://editor.swagger.io
How did you install Swagger-UI?	N/A
Which browser & version?	Google Chrome 60.0.3112.113 (Official Build) (64-bit)
Which operating system?	macOS

Demonstration API definition

swagger: "2.0"

info:
  version: 1.0.0
  title: Test
securityDefinitions:
  basicAuth:
    type: basic

paths:
  /bban/validate:
    post:
      produces:
        - application/json
      responses:
        200:
          description: OK
          schema:
            allOf:
              - $ref: '#/definitions/SwedenBBANValidationResponse'
              - $ref: '#/definitions/NorwayBBANValidationResponse'

definitions:
  BBANValidationResponse:
    description: |
      A discriminated object with variants:
      - <a href="#SwedenBBANValidationResponse">SwedenBBANValidationResponse</a>
      - <a href="#NorwayBBANValidationResponse">NorwayBBANValidationResponse</a>
    type: object
    properties:
      common_property:
        type: boolean
      enums:
        type: string
        enum:
          - enum_override_1
          - enum_override_2
        description: Enum override

  SwedenBBANValidationResponse:
    type: object
    description: Specific information for the `Sweden` type of BBANValidationResponse
    allOf:
      - $ref: "#/definitions/BBANValidationResponse"
      - type: object
        properties:
          sweden_specific_property:
            type: string
          enums:
            type: string
            enum:
              - sweden_enum_1
              - sweden_enum_2
            description: Sweden description

  NorwayBBANValidationResponse:
    type: object
    description: Specific information for the `Norway` type of BBANValidationResponse
    allOf:
      - $ref: "#/definitions/BBANValidationResponse"
      - type: object
        properties:
          norway_specific_property:
            type: string
          enums:
            type: string
            enum:
              - norway_enum_1
              - norway_enum_2
            description: Norway description

Expected Behavior

I want 'Model' in the API definition to show the enums fields as a combination of the fields of its inherited definitions (SwedenBBANValidationResponse and NorwayBBANValidationResponse).

And allow an enums specification on the parent to override the children if used.

Current Behavior

Currently, enums shows values and description only from one of its children, seemingly arbitrarily chosen.

There is no way to override the bleed-through by specifying a values on the parent.

Possible Solution

Context

[HaraldNordgren]

[shockey]

@owenconti, can you take a look at this? Sounds like it may be related to your recent allOf changes.

[owenconti]

Looking now. Since it's the enum values that aren't merging (an array), I think it's a bug with this earlier implementation of merging arrays: swagger-api/swagger-js#1101

[owenconti]

Hi @HaraldNordgren - I believe I have a fix for this issue. The fix is in the swagger-js repository. You can track the PR here: #3674

[hkosova]

@HaraldNordgren It's not a bug. allOf cannot be used to override attributes (required, enum, default, etc.) of a submodel, and it does not merge enums. You need to define the complete enum in one of the submodels.

[HaraldNordgren]

@hkosova When using in production we did choose that work-around: To specify the combined enum in the parent instead.

But the problems with inheritance are a bit deeper than that.

As can be seen in the screenshot, the list in responses.200.schema.allOf is parsed so that the resultant model correctly shows

• sweden_specific_property
• norway_specific_property

but each property that exists in both children are taken only from the Norway model:

• description
• enums (with values)

So if the Norway model has a description, that is the only thing that will be shown for the aggregrate model.

[HaraldNordgren]

@owenconti Thanks a bunch for giving this attention!

Be aware that the issue is deeper than just the enum merging. The problem with description could likely not be explained by just an array merging bug.

[webron]

@HaraldNordgren I'm sorry, but @hkosova is right. allOf does not merge schemas, it checks each one of those individually. In effect, nothing would validate against any of the schemas you provided in your sample, except for BBANValidationResponse, because you have conflicting enum definitions in all the other allOf instances.

When we render allOfin Swagger-UI - it is indeed a 'merge' of a sort - this is because it's easier to display and understand, and in most cases, when there are no conflicts, works well. We do have a way of producing a non-merged version of it, even though that's not exposed right now.

I'm afraid there's nothing to fix here. @owenconti, your PR is not needed unfortunately.

[owenconti]

@webron @hkosova What should happen with nested required arrays? Back in July I was asked to fix this issue (#3328) with the required property, but my fix did not account for nested required properties.

Should allOf be merging these required properties at all? If so, this spec is currently broken without my PR:

openapi: 3.0.0
info:
  version: 0.0.0
  title: test

paths:

components:
  schemas:
    Admin:
      allOf:
        - $ref: "#/components/schemas/User"
        - type: object
          properties:
            subobject: 
              type: object
              required: ["a"]
              properties:
                a: 
                  type: "string"

    User:
      type: object
      properties:
        subobject: 
          type: object
          required: ["b"]
          properties:
            b: 
              type: "string"

However, my PR also merges the enum property, which may not be correct.

[webron]

It's a little bit tricky, but yes, both should be required. enums can never be merged though - on the contrary, only the values that are available in both of them should remain.

[lock]

Locking due to inactivity.

This is done to avoid resurrecting old issues and bumping long threads with new, possibly unrelated content.

If you think you're experiencing something similar to what you've found here: please open a new issue, follow the template, and reference this issue in your report.

Thanks!