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

The mutually-exclusive url and identifier properties of License object are optional in specification and required in schema #2975

Closed
arno-di-loreto opened this issue Jul 21, 2022 · 11 comments · Fixed by #2991

Comments

@arno-di-loreto
Copy link
Contributor

In the definition of the License object of OpenAPI 3.1: the url and identifier properties are not marked as REQUIRED, so optional, and are defined as mutually exclusive.

The schema of the License object does not match with the specification making url or identifier required using the following construction at the root of of the object definition:

"oneOf": [
  {"required": ["identifier"] },
  {"required": ["url"]}
]

If we compare both behaviors, that means the following (note that name is required):

SpecificationSchema
  • We can have
    • name alone
    • name and url
    • name and identifier
  • But we can't have
    • name, url and identifier
  • We can have
    • name alone
    • name and url
    • name and identifier
  • But we can't have
    • name alone
    • name, url and identifier
  • The following document which is valid according to the specification is considered invalid when using the schema with the validation script or directly using it with ajv:

    openapi: 3.1.0
    info:
      title: Dummy Bookshop
      version: '1.0'
      license:
        name: Apache 2.0
    paths: {}

    Before proposing a pull request to fix that issue, I would like to confirm that my interpretation of the specification is correct and to know if there are any recommendations to follow regarding when modifying the schema. Besides sticking to the JSON Schema version already used, there are maybe some features to not use to ensure compatibility? (I already found 4 different ways to say "url and identifier are optional and mutually exclusive", I'll put them in the discussion if fixing the schema is the solution).

    @jdesrosiers
    Copy link
    Contributor

    If the schema is wrong, it's my fault. Reading the spec it doesn't appear that one of "url" or "identifier" is required, but I'd be surprised if that wasn't the intention. "name" is just a free-form label, right? The License Object wouldn't convey any real information without one of those included.

    @darrelmiller
    Copy link
    Member

    @jdesrosiers The challenge with making url or identifier required is that it would be breaking from v3.0 where url was not required and identifier didn't exist. https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.0.md#license-object

    We should probably make url or identifier required in v4.

    @webron
    Copy link
    Member

    webron commented Jul 21, 2022

    Yeah, in that sense it's no different than 3.0.

    @jdesrosiers
    Copy link
    Contributor

    Understood. Unless @arno-di-loreto already has something prepared, I'll update the schema.

    @arno-di-loreto
    Copy link
    Contributor Author

    Working on it.

    @arno-di-loreto
    Copy link
    Contributor Author

    @jdesrosiers Here are some proposals before sending a PR. I found 5 different ways of telling "url and identifier are mutually exclusive (but not required)". I put them all in a repo (including the original schema) with samples and a test suite demonstrating the original bug and the fixes.

    I think option 5 could be the one: not too verbose and clear intent. What do you think?

    Option 1 - Global anyOf defining 2 objects

    My first working attempt. The name is duplicated, but it works 😅. I am just keeping it for the record.

    Full source here

    Option 2 - A not required identifier and url

    The original oneOf is replaced by the not shown below. Not verbose but not evident for a JSON Schema 2020-12 beginner like me.

        not:
          required:
            - identifier
            - url
    

    Full source here

    Option 3 - A allOf if url then required identifier and reverse

    The original oneOf is replaced by the allOf shown below. More verbose but maybe more clear regarding the intent.

        allOf:
          - if:
              properties:
                identifier: {}
              required:
                - identifier
            then:
              not:
                required:
                  - url
          - if:
              properties:
                url: {}
              required:
                - url
            then:
              not:
                required:
                  - identifier
    

    Full source here

    Option 4 - Using dependentSchemas

    The original oneOf is replaced by the dependentSchemas shown below. A bit like option 4 but far less verbose.

    
        dependentSchemas:
          identifier:
            not:
              required:
                - url
          url:
            not:
              required:
                - identifier
    

    Full source here

    Option 5 - Taking advantage of inner $defs to be more explicit about the intent

    I took option 2 and put the not in an inner $defs. To use it, I had to move the extension $ref into a allOf.
    The idea comes from JSON Schema documentation which proposes to take advantage of $defs name to clarify the intent of a complex sub-schema.
    I like that structure because that way we have the "main" description (that goes above the allOf) and then some variations/precisions/traits (in the allOf).

    
        allOf:
          - $ref: '#/$defs/license/$defs/mutually_exclusive_url_and_identifier'
          - $ref: '#/$defs/specification-extensions'
        $defs:
          mutually_exclusive_url_and_identifier:
            not:
              required:
                - identifier
                - url
    

    Full source here

    @jdesrosiers
    Copy link
    Contributor

    I'm impressed that you found nearly every possible way to express this constraint. The only one I'd actively discourage is Option 1. It could be reworked to reduce the duplication, but in any case, the error messaging using this pattern is going to be confusing.

    There's only one option I can think of that you missed and that's the implication pattern.

    anyOf:
      - not:
          required:
            - identifier
      - not:
          required:
            - url

    Which can be written more simply using if/then (Option 3-ish)

    if:
      required:
        - identifier
    then:
      - not:
          required:
            - url

    And can be written even more concisely with dependentSchemas (Option 4-ish)

    dependentSchemas:
      identifier:
        not:
          required:
            - url

    I didn't include the inverse because it isn't necessary and produces unnecessary error results. If the license object has both "url" and "identifier" you'll get a message that says "url" isn't allowed with "identifier" and another message that says "identifier" isn't allowed with "url". One of those is sufficient.

    So, the choice comes down to required-not (Option 2/5) or dependentSchemas. I think both are good options. Both should have reasonably good error messages, although it's never great when not is involved.

    I'm a big fan of using $defs to name complex constraints (In fact I wrote that advice in UJS), but I personally think both of these options are concise enough and their behavior is clear enough that we probably don't need the $defs naming pattern in this case. However, I don't feel strongly about that if others think it's useful.

    @Wyze5280
    Copy link

    I'm not really sure what this all mean I'm look for someone to teach me and understand coding and how it works

    @arno-di-loreto
    Copy link
    Contributor Author

    🤦🏻‍♂️😅 Obviously "if A not B" also means "if B not A". I really like the dependentSchemas option, let's go for this one.

    dependentSchemas:
      identifier:
        not:
          required:
            - url
    

    @arno-di-loreto
    Copy link
    Contributor Author

    Few questions to do a PR :

    • I didn't find a "how to do a schema PR" in the documentation, did I miss it?
    • Do you want that I add all of my test files in the tests fail/pass folder?
    • I guess I have to modify the YAML schema file and then convert it to JSON probably using the yaml2json script?
    • Should I modify the date in the $id? $id: 'https://spec.openapis.org/oas/3.1/schema/2022-02-27'
    • I assume I should modify from the main branch?
    • I think I'll have to target a branch with my PR, if that's so, which one?

    @webron
    Copy link
    Member

    webron commented Jul 25, 2022

    There's a brief explanation at the schema directory (version based), for example https://github.com/OAI/OpenAPI-Specification/tree/main/schemas/v3.1.

    You only need to modify the YAML version, no need to update the date and the PR should be against the main branch.

    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

    Successfully merging a pull request may close this issue.

    5 participants