Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

subtypes & discriminator would benefit from more info #44

Closed
fehguy opened this issue Apr 15, 2014 · 12 comments
Closed

subtypes & discriminator would benefit from more info #44

fehguy opened this issue Apr 15, 2014 · 12 comments

Comments

@fehguy
Copy link
Contributor

fehguy commented Apr 15, 2014

Swagger 1.2 lets you specify a discriminator and possible subtypes, but it doesn't relate the value of the discriminator and the model that should be instantiated. It would be convenient if this could be expressed in the spec.

@Larry0ua
Copy link

Larry0ua commented Aug 6, 2014

I also vote up this issue. Working with inheritance at the model and have to make some fixes in swagger core and swagger ui because of that. It seems that no one have used this feature before (or failed to use because of some stoppers)

@mission-liao
Copy link
Contributor

+1.
Right now my understanding about the value of 'discriminator' is the property specified should be the id of the subtype, right?

ex. in Cat example, when accessing to Cat.type, it's value should be the value of Cat.id, which is 'Cat'.

Am I on the right track?

@mpoindexter
Copy link

The way I read the spec is the same as what @mission-liao says. I think this is a bit limiting to constrain the discriminator values to the model id. Perhaps if there were an optional "discriminatorValue" attribute on subtypes that controlled the value discriminator could take to signal that the data is an instance of that subtype.?

This would be closer what Jackson is doing which is an annotation that looks like:

@JsonSubTypes({ @type(value = Cat.class, name = "cat-discriminator"), @type(value = Dog.class, name = "dog-discriminator") })

@fehguy
Copy link
Contributor Author

fehguy commented Sep 19, 2014

The current composition model looks like such:

{
    "Pet": {
      "discriminator": "petType",
      "properties": {
        "name": {
          "type": "string"
        },
        "petType": {
          "type": "string"
        }
      },
      "required": [ "name", "petType" ]
    }
}

From this definition, the consumer will know that petType needs to be present to choose what type to instantiate.

somewhere we need to tell the client what class to instantiate. We would either need to list all the subtypes in the parent class--along with the discriminator. In the 1.2 spec, that was very ugly and frowned upon. What would a better structrue be?

@webron
Copy link
Member

webron commented Sep 19, 2014

I don't think we need to do anything. It's implicit. Any schema that uses allOf and includes the parent with the discriminator is applicable. There are two downsides to it - the first, clients would need to scan the definitions and build a tree (yes, a tree, since it can be of any level of hierarchy). The second - when it comes to spec generation, it may require deeper introspection. So both would require more work from the clients, but since this is not a simple feature, I don't think it's a major issue.

@webron
Copy link
Member

webron commented Mar 5, 2015

@fehguy - Is this still relevant?

@KrzysztofMadejski
Copy link

I'm wondering how to structure API schemas that use polymorphism and not to put all of them in swagger.json.

Specifying external schema urls in discriminator property would be a way to go: Following cat example, that would be a sample object:

{
  "name": "Felix",
  "petType": "http://example.org/schemas/cat.json"
}

That would be understandable for clients as well, even without building a tree of definitions. What do you think? Could such values be allowed?

Downside is that client won't be able to preload schemas. Another approach is to mention them in definitions and use "petType": "cat":

{"definitions": {
  "cat": {
    "$ref": "http://example.org/schemas/cat.json"
  }
}}

@webron
Copy link
Member

webron commented Mar 27, 2016

Closing it as it's related to 1.2. 2.0 changes things a bit, and we'll tackle the topic in the related modeling issue.

@webron webron closed this as completed Mar 27, 2016
@KrzysztofMadejski
Copy link

Could you provide the link to the related modelling issue?

@ePaul
Copy link
Contributor

ePaul commented Apr 14, 2016

@webron

I don't think we need to do anything. It's implicit. Any schema that uses allOf
and includes the parent with the discriminator is applicable.

That still doesn't tell me which of those schemas now is the one to choose, or do I miss something?

Ah, here:

Field Name Type Description
discriminator string Adds support for polymorphism. The discriminator is the schema property name that is used to differentiate between other schema that inherit this schema. The property name used MUST be defined at this schema and it MUST be in the required property list. When used, the value MUST be the name of this schema or any schema that inherits it.

So this discriminator property must match the name which is used as key in the definitions object for defining the schema (and inline schemas can't be subtypes). I guess the original request by @fehguy was to make this mapping a bit more flexible by not linking the schema keys (which otherwise can be swapped out without changing anything in the API, as long as the refs are changed too) directly to the on-the wire content.

@webron
Copy link
Member

webron commented Apr 14, 2016

@ePaul if you can remember full intent of comments you've made a year and a half ago, I salute you ;)

@natke
Copy link

natke commented Oct 30, 2016

I realize that this issue is closed but what was the resolution here?

Do the values of the discriminator field have to exactly match the name of the subType?

Otherwise, how does the client know specifically which value of the discriminator generates which subtype?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants