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

Discriminator field (unnecessarily) required #708

Closed
dmejia opened this issue Jun 2, 2016 · 11 comments
Closed

Discriminator field (unnecessarily) required #708

dmejia opened this issue Jun 2, 2016 · 11 comments

Comments

@dmejia
Copy link

dmejia commented Jun 2, 2016

Is it possible to remove the restriction of making the field that acts as discriminator required (and assume that when not returned, the object is just of the 'base' type defined in the schema)?

i.e (using the Pet example from the docs)

Schema:

(...)
properties: 
    pets: 
       type: array
       items:
           type: Pet

Payload:

{
   pets: [
   {
      name: "Some generic pet" 
   },
   {
      @type: Dog
      name: "Some dog"
      packSize: ...
   },
   {
      @type: Cat
      name: "Some cat"
      huntingSkill: ...
   },
   {
       name: "Some other generic pet"
   }
}

The first and third items are just of type "Pet" which would be explicit from the schema of the "pets" field. "Pet" would therefore define the @type field but it won't be required.
We use schema.org and rely on polymorphic schemas extensively but we currently only return a type discriminator when is not obvious from the schema (i.e when returning a subtype). Unless I misunderstand it, adhering to the spec would require us to return @type everywhere which would significantly increase payload size (and thus would prob be a non-starter).

Let me know if you have any thoughts or reasons to not go this route.

@ePaul
Copy link
Contributor

ePaul commented Jun 3, 2016

Related: #707.

@dmejia dmejia changed the title Discriminator field required Discriminator field (unnecessarily) required Jun 15, 2016
@dmejia
Copy link
Author

dmejia commented Jun 15, 2016

Any comments on this?

@ralfhandl
Copy link
Contributor

I like this proposal, OData also omits the @odata.type field for instances of the base type.

@webron
Copy link
Member

webron commented Jul 21, 2016

Tackling PR: #741

@Relequestual
Copy link
Contributor

@webron #741 was closed in favour of merged #894.
Does that mean that this is now resolved? Appears not as of current branch:v3.0.2-dev

Is this being tracked on a new PR?

As an aside, having the ability to shortcut any/all/one Of sounds like a nice thing! Maybe we should look at putting this into JSON Schema itself!?

@handrews
Copy link
Member

handrews commented Jul 23, 2018

@Relequestual discriminator does not fit well with other aspects of JSON Schema- I looked into it while we considered if/then/else and again during the whole unevaluatedProperties saga. I'd have to do some digging to explain exactly why.

@Relequestual
Copy link
Contributor

No, that's fine. The same functionality can be provided using if/then/else.

@tmtron
Copy link

tmtron commented Sep 23, 2020

another use-case for an optional discriminator is API-evolution.

i.e. we may have an API that only supports one type (e.g. Dogs) and thus has no discriminator
But later we need to add support for Cats

Then we can change the API in a backwards compatible way

  • original Dog objects (without discriminator will still work)
  • new Dog object with discriminator Dog will now also work
  • new Cat objects must have a discriminator

@handrews
Copy link
Member

@OAI/tsc review request: Do we want to consider this for 3.2+? We are already tracking discriminator replacement in Moonwalk so if we don't want to do this in 3.x we should close it.

@lornajane
Copy link
Contributor

I'm in favour of closing. I think if we didn't do it in the first 8 years of this issue's lifetime, we should probably make 4.0 the host for changes in this area.

@handrews handrews added review and removed review labels May 22, 2024
@handrews
Copy link
Member

Two TSC votes to close, so I'm going to close this as that's our threshold for merging PRs.

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

No branches or pull requests

8 participants