[develop 2.0] support allOf in models

[fehguy]

Currently the 2.0 spec supports allOf constructs but the js library does not.

swagger-api/swagger-ui#723

[webron]

Does this include the discriminator property for inheritance or just the allOf for composition?

[fehguy]

the intent of this issue is to support the spec, for purposes of composition which means adding the discriminator.

[webron]

Technically, you can use allOf without a discriminator just for composition, which implies a model that includes the properties of another, but there's no direct hierarchy between them.

Though if this issue includes both, that's great.

[DavidBiesack]

yes, we'd like to use it for composition, to reuse schema instead of copy/paste (and to model property inheritance).

[wodka]

I just gave it a try in #264 - this seems to be working:

definitions:
    AbstractObject:
        properties:
            id:
                type: string
    MyObject:
        allOf:
            -
                $ref: "#/definitions/AbstractObject"
            -
                type: object
                required:
                    - name
                properties:
                    name:
                        type: string
    ListOfMyObject:
        type: array
        items:
            $ref: "#/definitions/MyObject"

Only errors I see so far is that the Model is not shown in ListOfMyObject - but the ModelSchema is shown correctly.
Also the datatype only reads "undefined"...

[webron]

I wonder how we would end up displaying it, as technically this is not a single model but a set of models to be checked against.

[whitlockjc]

How do you think this should be displayed? A lot of work recently has been done to properly support arrays, inline schemas and references but to fix this we need to know just how to display ancestors/included models.

[m8rge]

@whitlockjc, I think, we just need to get all schemas from allOf property and merge them all together into result model.

[webron]

@m8rge - unfortunately, allOf is not really a merge, and the problem will come when there are conflicting schema.

The solution should probably be closer to an array of schema possibilities.

[m8rge]

@webron This is spec-related question: https://github.com/swagger-api/swagger-spec/blob/master/versions/2.0.md#composition-and-inheritance-polymorphism

[webron]

What's the question?

[m8rge]

@webron

the problem will come when there are conflicting schema

Answer to this question must be in spec: https://github.com/swagger-api/swagger-spec/blob/master/versions/2.0.md#composition-and-inheritance-polymorphism

[webron]

It's not a question, it's a statement. The spec can't deal with people's mistakes.

[m8rge]

If developer make mistake in schema - this is his fault. This is his pain, not swgger-js developers. I am right?
Why we can't add support for allOf statement?

[webron]

Who said we can't add support to it? I just said the meaning of allOf is not "merge", so we can't "merge" it to a single schema.

[whitlockjc]

We could definitely make this work but we need to figure out the best way to display this stuff. If we could agree on that, we'd be able to do the work.

[cristian0]

+1

[mgttlinger]

As far as displaying I think a nice option would be to list the super-class/schema and all children-schemes when the used type is of the super-schema (similar to what is done with compound types). I would mark all child-schemes with "inherits from ..." or something in that manner.
In case of the used type being a sub-schema I would list every field alongside every inherited field and mark those fields with "inherited from ...".

[jharmn]

IMO the discriminator keyword adds all the complexity to this scenario.
For both inheritance and composition:

• Merge properties from each referenced schema in allOf
• If property conflicts exist, error

With that in mind: I'd recommend splitting these issues up.

1. Support for inheritance and composition with allOf

• Seems like no issues, this could be implemented ASAP
  1. Support for polymorphism with discriminator
• Thunderdome tournament begins on rendering preferences, who knows how long that takes

[webron]

If only allOf was equivalent to merging... :(

[Smolations]

@webron Well, it sort of is, there is just a tiny conditional: if merging 2 properties with the same name, their schema definitions must match. From a JSON schema resource:

Note that it’s quite easy to create schemas that are logical impossibilities with allOf. The following example creates a schema that won’t validate against anything (since something may not be both a string and a number at the same time):

{
  "allOf": [
    { "type": "string" },
    { "type": "number" }
  ]
}

Though there may be some cases where one of the schemas is almost the same as the other and, when merged, might not present conflicts for which validation fails, it would appear that, in general, overlapping schema properties would need to match in order to render correctly after a merge.

I must concede, however, that I am in agreement with @m8rge about implementation mistakes being the developer's fault.

[webron]

@Smolations - thanks, that's indeed a good reference. But it also goes to show you have much allOf is not 'sort of' merging, but rather that it's not merging at all.

There are many subtle technicalities when it comes to JSON Schema, allOf being one of them. The actual meaning of it is given a JSON structure that's to be validated against a JSON Schema containing allOf, that JSON structure must be validated against each of the schema described by the allOf array separately. The example provided in your reference is one example to create a schema against nothing would validate, but it is not the only one.

While we can't (and shouldn't) deal with use errors, this does impose some difficulties in what we end up representing in the UI.

Please note I'm not looking to argue the semantics of merging, it just really isn't merging at all. For what it's worth, draft 5 currently has a proposal for $merge which as you can guess would mean that, but I don't know its details nor will it affect the current version of the Swagger specification.

[Smolations]

The example provided in your reference is one example to create a schema against nothing would validate, but it is not the only one.

@webron Totally. So while merging the schemas for use in the UI might work for many, if not most, cases, some validator would need to be written in the library and I can understand the hesitation with a lift like that.

In any case, as long as "allOf", "anyOf", "oneOf", and "not" eventually get implemented, I'll be happy. 😄

[webron]

Well, can't comment regarding the other keywords other than allOfas they are not supported by the spec and can't say if/when they will be.

[yonderblue]

👍 to get this working

[fehguy]

allOf is supported in devleop_2.0. discriminator support is tracked here:

#469

[phiz71]

would it be possible to get inspired by swagger-editor for the display?

[dcinadr]

This is still not working but I have found a fix that can be added to the source code. If you place this code in between lines 491 and 492 of the signature.js file it will fix the issue.

_.forEach(schema.properties, function(property, name){ model.definition.properties[name] = property; });