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

nullable + allOf does this work? #333

Open
bgoesswe opened this issue Sep 18, 2020 · 8 comments
Open

nullable + allOf does this work? #333

bgoesswe opened this issue Sep 18, 2020 · 8 comments
Labels
bug no solution yet There is no solution yet, but might be considered later: follow-up patch requires a patch-version (x.x.1 for example)
Milestone

Comments

@bgoesswe
Copy link
Member

This is not an urgent issue, but just to have this documented:

I recently came to an issue regarding the GET /process_graphs/{process_graph_id} endpoint and the used response schema *user_defined_process_meta` using the openEO validator:

user_defined_process_meta:
      title: User-defined Process Metadata
      description: A user-defined process, may only contain metadata and no process graph.
      type: object
      required:
        - id
      properties:
        summary:
          nullable: true
        description:
          nullable: true
        parameters:
          nullable: true
        returns:
          nullable: true
      allOf:
        - $ref: '#/components/schemas/process'

Since description and summary is not nullable in the '#/components/schemas/process schema, the validator does not take the nullable: true in the properties into account and assumes that it is not nullable. I am actually not sure what happens if the properties and the properties in an allOf reference are named the same, but I found a very long issue at the OpenAPI Github page regarding the nullable issue and also some similar issues from other projects.

Even after reading these issues, I am not completely sure if the validator or the API is wrong here.

@m-mohr
Copy link
Member

m-mohr commented Sep 21, 2020

Yeah, the hell of "nullable"...

I'm also not 100% sure. The reason is that nullable is just not clearly defined by the implements spec version 3.0.2, so everybody can argue differently. They have clarified in 3.0.3, but that was just recently and is not included in our API yet.

The way forward is likely to just add the types to the "standalone" nullables and change the OpenAPI version to 3.0.3. We can surely do this in a backward compatible manner and thus just have it in openEO API version 1.0.1.

Long-term the issue will go away anyway with OpenAPI version 3.1.0, but that still needs time for tooling to adopt.

To solve the issue quickly for you in the validator and to check what really works, could you please try whether replacing your code snippet above with one of the alternatives below works?

user_defined_process_meta:
      title: User-defined Process Metadata
      description: A user-defined process, may only contain metadata and no process graph.
      type: object
      required:
        - id
      properties:
        allOf:
        - $ref: '#/components/schemas/process'
        - summary:
            nullable: true
          description:
            nullable: true
          parameters:
            nullable: true
          returns:
            nullable: true

or (what OpenAPI suggests):

user_defined_process_meta:
      title: User-defined Process Metadata
      description: A user-defined process, may only contain metadata and no process graph.
      type: object
      required:
        - id
      properties:
        summary:
          type: string
          nullable: true
        description:
          type: string
          nullable: true
        parameters:
          type: array
          nullable: true
        returns:
          type: object
          nullable: true
      allOf:
        - $ref: '#/components/schemas/process'

@m-mohr m-mohr added feedback required patch requires a patch-version (x.x.1 for example) labels Sep 21, 2020
@m-mohr m-mohr added this to the v1.0.1 milestone Sep 21, 2020
@bgoesswe
Copy link
Member Author

Ok, I tried both attempts with the following results:

  1. There I tried around some combinations, but the validator has troubles reading/parsing it that way.
    image

  2. With the second approach, I also get an error. Apparently arrays can not be nullable. But even if I set it to a string, it still interprets it as not nullable.
    image

For D28 it is not really necessary that this is fixed, because we can simply set the not nullable values.
So there is no urgent reason for a fix, therefore it is probably best to wait for version 3.1.0 on this.

@m-mohr
Copy link
Member

m-mohr commented Sep 22, 2020

Okay, from https://github.com/OAI/OpenAPI-Specification/blob/master/proposals/003_Clarify-Nullable.md:

Can allOf be used to define a nullable subtype of a non-nullable base schema? (See #1368.)

No. Subtypes can add constraints, but not relax them.

and

Can allOf be used to define a non-nullable subtype of a nullable base schema?

Yes. The subtype can specify a type without nullable: true, or can specify not: {enum: [null]}.

So that means I need to invert the process schema. Currently it doesn't allow null and then I add it. That's not possible, I need to allow null first and then disallow it.

@m-mohr
Copy link
Member

m-mohr commented Nov 20, 2020

I have tried to implement it in compliance with OpenAPI 3.0.3, but redoc doesn't render it nicely. It actually now claims false things and that makes the rendered documentation really bad. As I assume most people consult the rendered version, it would confuse people and lead to broken implementations. Thus I'm not sure whether our change really helps or makes things worse, although the new version is formally better.

Could you try the new OpenAPI file, @bgoesswe? See the nullable branch (or the commit above).

m-mohr added a commit that referenced this issue Nov 25, 2020
@m-mohr
Copy link
Member

m-mohr commented Nov 25, 2020

I'll move this to later, I can't find a good solution supported by tooling yet. Also, I don't think our definitions are incorrect as I thought they would be. In most cases there's no type specified in the "base" schema thus I should be able to extend it as we do it now. Maybe OpenAPI 3.1. will solve this.

Related issues:

@m-mohr m-mohr modified the milestones: v1.0.1, future Nov 25, 2020
m-mohr added a commit that referenced this issue Nov 25, 2020
@m-mohr m-mohr added no solution yet There is no solution yet, but might be considered later: follow-up and removed feedback required labels Nov 25, 2020
@fenollp
Copy link

fenollp commented Feb 23, 2021

I don't think there's anything wrong (or unsatisfiable) with the first comment's schema.
The lib's schema validation is broken though. I am working on this :)

@m-mohr
Copy link
Member

m-mohr commented Feb 23, 2021

It seems that with OpenAPI 3.0.3 it is in fact wrong, but with previous versions it is somewhat okay (or at least undefined in the OpenAPI spec). Thanks for working on a bug fix, @fenollp ! Highly appreciated and also good work on the validation library.

@m-mohr
Copy link
Member

m-mohr commented Nov 3, 2021

I doubt that I'll fix this before migrating to OpenAPI 3.1 (see reasons above), but migrating to OpenAPI 3.1 will also not take place any time soon (due to tooling support). So don't expect any changes here. #299

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug no solution yet There is no solution yet, but might be considered later: follow-up patch requires a patch-version (x.x.1 for example)
Projects
None yet
Development

No branches or pull requests

3 participants