3.1.1 discriminator improvements

[jdesrosiers]

Here are some updates for the discriminator keyword specification. Mostly, this update is to make it clear that discriminator shouldn't change the validation behavior of the oneOf/anyOf keywords. discriminator can only be used to improve efficiency or messaging of evaluating oneOf/anyOf I also fixed bugs and clarified some confusing parts.

• The language in the table introducing discriminator seems to describe the value of the discriminator keyword to be an "object name" (whatever that means) rather than an object. I tweaked the wording to avoid that contradiction.

• The discriminator keyword uses the concept of "schema names", but what that means is never defined. There is no concept of a schema having a name in JSON Schema. They only have URIs. I defined "schema name" and added some clarifications where the use of schema names got confusing.

• The discriminator keyword can't be a shortcut, only a hint. I tried to tweak any language that might suggest that discriminator overrides the behavior of anyOf/oneOf in any way.

• The last example was super confusing, which is mostly because it doesn't indicate which schema the payload examples are being applied to. So, I added the schema and an explanation of what is happening.

[MikeRalphson]

On a first reading, this looks to be a fantastic set of cleanups, improvements and fixes.

One thing that might still be outstanding is to clarify the schema name resolution process for the case of multiple (externally reffed) resources. If the "shortcut" reference was always scoped to the same resource, that would seem to make sense, but would have a subtle impact on bundling. Effectively we would be treating the schema name as if it was a schema $id. Tooling would have to provide some mechanism to avoid schema name clashes, or to rewrite the mapping.

[MikeRalphson]

Also, if the discriminatoris used in a pure JSON Schema resource, it is likely that there will be a $defs section, not a components/schemas section. Do we allow those shortcuts?

[jdesrosiers]

@MikeRalphson You're highlighting some of the major problems with the way discriminator is defined. Let's start with,

if the discriminator is used in a pure JSON Schema resource, it is likely that there will be a $defs section, not a components/schemas section. Do we allow those shortcuts?

The "schema name" concept doesn't exist in JSON Schema and isn't defined in OAS, so I had to make up a definition based on context and the existing examples. However, It doesn't look to me like the existing spec considers schemas in $defs to be "named" schemas. If it did there would have to be consideration for how to deal with naming conflicts that would then be possible.

Given the way things are defined, I think that "schema names" can only be declared in the /components/schemas section of an OAS document. Therefore, the "schema name" addressing feature of discriminator can't be used in a pure JSON Schema. It also means that the schema in /components/schemas can't have an $id or the #/components/schemas/{schemaName} URI that is constructed will not point to an OAS document.

Which brings us to,

If the "shortcut" reference was always scoped to the same resource, that would seem to make sense, but would have a subtle impact on bundling.

Bundling is only a concern if you are working with external schemas and since the "schema name" feature can't work with external schemas, that means bundlers never have to worry about schema names colliding because the schemas it's bundling can't have them.

I don't know how much of that should go in the spec, but it's probably worth adding that schemas defined in $defs are not considered named schemas.

[MikeRalphson]

I think I agree with the thrust of what you say there @jdesrosiers

Therefore, the "schema name" addressing feature of discriminator can't be used in a pure JSON Schema

Obviously a 'pure' JSON Schema resource could still have a components section masquerading as an unknown keyword, so let me re-phrase my question as: should we allow shorthand references to the components/schemas section in a resource which does not identify itself as an OAS document? Is this overcomplicating the parsing expected of the mapping keyword?
Should we deprecate the shorthand name form as soon as possible?

Bundling is only a concern if you are working with external schemas

I was using bundling in a generic sense to include the cases where one OAS document includes another, or includes a bare fragment of JSON/YAML, e.g.:

openapi: 3.1.1
info:
  title: Music API
  version: 1.0.0
paths:
  /bands:
    get:
      responses:
        '200':
          content:
            'application/json':
              schema:
                $ref: './subresource.yaml#/components/schemas/Punk'
components:
  schemas:
    Clash: { not: {} }

components:
  schemas:
     Punk:
      oneOf:
        - $ref: '#/components/schemas/Clash'
        - $ref: '#/components/schemas/Pistols'
        - $ref: '#/components/schemas/Damned'
      discriminator:
        propertyName: bandName
        mapping:
          clash: Clash
          pistols: Pistols
          damned: Damned
     Clash: ...
     Pistols: ...
     Damned: ...

in the above example, you can see where the clash (or Clash) occurs.

[MikeRalphson]

Very open to dealing with additional issues outside of this PR, no need for mapping considerations to block this.

[jdesrosiers]

should we allow shorthand references to the components/schemas section in a resource which does not identify itself as an OAS document?

I'm trying to be careful not to add or change anything in this patch update so I think keeping "schema names" as an OpenAPI-only concept is the safest choice. Since this concept was never defined in the spec, I think the best thing to do is to get feedback from vendors who have implemented discriminator and try to match what existing tooling does. That's assuming different tools even behave the same. Every JSON Schema validator I've seen that implements discriminator does it differently and none follow the OpenAPI spec exactly. Interestingly, none that I've seen use schema names.

However, I hadn't considered an OpenAPI document like in your example that is just a container for schemas that another OpenAPI document can reference. I definitely think "schema names" would apply in that case and bundling would become an issue. I would expect that schema names should be scoped to the OpenAPI document where they are defined. Because you can't embed an OpenAPI document in another like you can with pure JSON Schema, you would have to merge /components/schemas which could require rewriting/introducing the mapping sub-keyword to deal with naming conflicts. You would have to deal with naming conflicts for $refs as well.

Is this overcomplicating the parsing expected of the mapping keyword?

No, I think you are identifying important edge cases that need to be addressed. I don't think we will be able to fix much in this patch update, but it's definitely a conversation that needs to happen.

Should we deprecate the shorthand name form as soon as possible?

That would be my recommendation. At a minimum, mapping shouldn't allow schema names. There's no unambiguous way for implementations to distinguish between value that is intended to be a schema name and a value that is intended to be a URI. My preference would be to deprecate discriminator entirely in favor of a new JSON Schema keyword I've proposed, json-schema-org/json-schema-spec#1082. Instead of ...

components:
  schemas:
    Punk:
      oneOf:
        - $ref: '#/components/schemas/Clash'
        - $ref: '#/components/schemas/Pistols'
        - $ref: '#/components/schemas/Damned'
      discriminator:
        propertyName: bandName
        mapping:
          clash: Clash
          pistols: Pistols
          damned: Damned
    Clash: ...
    Pistols: ...
    Damned: ...

... it would be ...

components:
  schemas:
    Punk:
      propertyDependencies:
        bandName:
          clash:
            $ref: '#/components/schemas/Clash'
          pistols:
            $ref: '#/components/schemas/Pistols'
          dammed:
            $ref: '#/components/schemas/Damned'
    Clash: ...
    Pistols: ...
    Damned: ...

But that's a whole other conversation.

[darrelmiller]

Please review and merge @OAI/tsc

[jdesrosiers]

I think perhaps for 3.1.1 we can say that if the mapping string is a fragment-only uri-reference, then it's resolved against the containing document (e.g. for '#/components/schemas/Dog'), but otherwise (no leading #) it's treated as a "named schema", where we go to look for that schema under /components/schemas.

I don't have an opinion here, but I'd point out that this excludes the possibility of an external reference in discriminator. That may or may not be a good thing. I think a lot depends on what existing implementations support and what users are expecting.

However, according to @MikeRalphson and @webron, that is not how it has historically worked.

This PR is specifically not trying to change anything, just clarify and fill some gaps. I'm far from an expert how this has historically worked, so I might have inadvertently introduced a change. I'm happy to rewrite anything that needs fixing or better yet, hand this over to someone more actively involved with OpenAPI to make the corrections.

[handrews]

I don't have an opinion here, but I'd point out that this excludes the possibility of an external reference in discriminator. That may or may not be a good thing. I think a lot depends on what existing implementations support and what users are expecting.

Yeah, that's pretty murky. The final 2023 milestone for the OASComply project is starting a set of test cases around referencing ambiguities (including both literal $ref and things like implicit mappings in the Discriminator Object). The spec ambiguities mean that there are several possible outcomes that are at least arguably within the spec. So this "test suite" will be more of an "assessment suite" in the sense that it will specify several potentially valid outcomes. So we can use that to find out what tools actually support.

I think this PR is a solid effort at clarification, which will be a lot easier to resolve once we figure out the current tooling landscape and the TSC makes a ruling on how to handle the ambiguities. That could be done by leaving this open and revisiting it, or just filing an issue (or updating an existing one?) to revisit it after we've assessed the tools.

[earth2marsh]

Discussed today, this generally seems like a good idea, but since Moonwalk plans to revisit discriminator as part of its focus on signatures, it seems wiser to see how that plays out before we try to resolve the ambiguities here.

However, there are a number of good wording improvements (thank you), it seems useful to separate those as a separate PR? Then we can incrementally improve the spec in the short term. @jdesrosiers , if you are able to do that, that would be great, and if not, folks seemed positive on today's call that we could find help for that.

[jdesrosiers]

I could do a revision, but it's not clear to me which parts you'd like to keep and which to revert. If someone wants to submit a review pointing out the parts they'd like changed, that would be helpful. Or, I'm totally fine if someone want to just take the content and submit a new PR if that's easier.

[handrews]

@jdesrosiers I saw your question about what could and couldn't be kept - I've added some comments based on my understanding and the discussions we're having elsewhere related to how referencing and component names interact in general.

AFAICT I philosophically agree with what you're trying to do here, I'm just conveying what I've been told about what's complex and ambiguous right now.

Up to you whether you want to edit this or close it. I'm pretty sure all of these have issues related to them so they'd get revisited eventually.

[handrews]

This is the main part that's controversial. In Swagger 2.0, the references were strongly implied to be scoped to document "A", and many tools implement that behavior. There are other component name connections that have similar ambiguities and we should address them all at once. (FWIW, I strongly agree with what you've written here, but it would be a de-facto breaking change for some and we recently decided not to do "clarifications" that are de-facto breaking changes).

[handrews]

My understanding is that the "allOf" form specifically exists to avoid having to list the subschemas in a "oneOf" or "anyOf", so this should not be changed. I don't personally understand how this even works, and I'm not the only one. But it's related to the controversial thing- if all schema names resolve to the Components Object of what we now call the "entry document", you can just search that one list. I guess? Apparently we need to figure that out.

[jdesrosiers]

The problem I was trying to solve here is that the example payloads never say which schema is being used to evaluate those payloads. Based on my understanding of the allOf usage, none of those schemas could produce the results the documentation says it does, so I introduced something that could.

At this point I can only assume that the "Pet" schema is what these payloads are intended to be evaluated against and the behavior described is correct in that case. So, I've reverted my change other than to specify that "Pet" is the schema these payloads are evaluated against. Let me know if that's the right thing to do. I can't see how that makes sense, so I wouldn't be surprised if it's still not correct. Let me know.

[handrews]

This would need to be removed if the oneOf addition to the example is removed.

[jdesrosiers]

Thanks @handrews. That looks like what I need to do a revision. It'll be a couple more days still until I can get to it, but I wanted to acknowledge that I saw it and plan to get to it soon.

[handrews]

Thanks! LGTM

[mikekistler]

This is a nice improvement, but I suggested some alternate wording for one paragraph that I think is a tad better.

[mikekistler]

Looks good! 👍