fix: invalid nullable enums with OAS 3.1 version

[bnowak]

Fix from #2178 solved proper describing of nullable enums for OAS v 3.0, but it seems that broke it for OAS v 3.1.

As for version 3.0 it's correct and looks like:

'nullableType' => [
    'nullable' => true,
    'allOf' => [
        ['$ref' => '#/components/schemas/ArticleType81'], // enum ref
    ],
],

for version 3.1 it seems to be wrong. It's impossible to match these both types at the same time:

'nullableType' => [
    'allOf' => [
        ['$ref' => '#/components/schemas/ArticleType81'], // enum ref
        ['type' => 'null'],
    ]
],

I think the correct version for 3.1 should be:

'nullableType' => [
    'oneOf' => [
        ['$ref' => '#/components/schemas/ArticleType81'], // enum ref
        ['type' => 'null'],
    ]
],

In our project we're validating API data responses against API model schema using https://github.com/opis/json-schema and it reports above issue.

For now it's a dirty/draft PR to demonstrate the issue and start the discussion. It includes possible solution but I'm not sure 100% it's the correct way:

• I'm not sure about the responsibilities which lib (nelmio or https://github.com/zircote/swagger-php) should handle OAS versions. Currently I cannot see it's happening in nelmio side and there's handling of OAS versions in https://github.com/zircote/swagger-php/blob/4.8.4/src/Annotations/AbstractAnnotation.php#L390, however there could be lack of intention context how it should be serialized at the end.

Seems to be also connected with zircote/swagger-php#1528 and zircote/swagger-php#1531, is that right @DjordyKoert?

I'm happy to finalize this topic, but please point me in the right direction. Any feedback is welcome :)

[DjordyKoert]

This should be fixed inside swagger-php and yes you are correct zircote/swagger-php#1528 & zircote/swagger-php#1531 are related to this issue.

All OpenApi version conversions are done inside swagger-php's https://github.com/zircote/swagger-php/blob/4.8.4/src/Annotations/AbstractAnnotation.php#L390.

[DjordyKoert]

I don't think potentially adding a bunch of if-statements everywhere to fix the spec for specific openapi versions is the right way to go inside this codebase (nelmio) either. A processor(s) on swagger-php's end to convert generated specs for different versions would be a good way to handle these things.

[bnowak]

@DjordyKoert
I agree completely. However, AFIK at the moment there're no version-specific processors in place, so following the ifs-pattern it's the only way to fix it for now.

I'd treat it as a workaround solution, which should be replaced by version dependent processors in the future.

I'll continue the topic on swagger-php's end and we'll see how it envolves.
Thanks for reply :)

[bnowak]

Hello again @DjordyKoert

I played a bit with swagger-php and it looks it's a bigger more complex topic than I thought :) Will try to summarize it:

Originally I tried to handle current behavior of nelmio (which wraps enums with allOf) and for nullables I wanted to wrap it again with oneOf (described here) as IMO it doesn't make sense to have combined specific types in allOf alongside with null on the same level. I tried to recreate this case with enums, but they're handled with oneOf by default and it seems correctly for me:

enum Foo: string
{
    case one = 'One';
    case two = 'Two';
}

class Bar
{
    public ?Foo $foo;
}

// for OAS 3.0 be default it generates spec:
[
    'foo' => [
        'oneOf' => [
            ['$ref' => '#/components/schemas/Foo'], // this not necessarily need to be wrapped with `oneOf`, but still is correct
        ],
        'nullable' => true,
    ],
]

// for OAS 3.1 be default it generates spec:
[
    'foo' => [
        'oneOf' => [
            ['$ref' => '#/components/schemas/Foo'],
            ['type' => 'null'],
        ],
    ],
]

When I wanted to add additional property annotation to force using allOf (to demonstrate behavior of Nelmio) - it was added beside of oneOf, which obviously won't be working. Also, I tried to find another (more native) use case of allOf in test, but it looks like it's used only for inheritance (combining models with all from their parents).

All of above turned me to thinking whether wrapping enums with allOf (or even oneOf because it's handled the same way on swagger-php side) in Nelmio is in general correct way. I checked that if we'll delete condition for wrapping enums with allOf - all test related to enum fails ofc because other expectation, but all the rest is fine. So it's enum-specific topic. It would work the same even without wrapping enums with oneOf.

now short question (I promise 😅) - why do we need to wrap enums by allOf at all and could it be removed?

[DjordyKoert]

Just had a small look at the openapi spec and I just now noticed that our nullable are just incorrect currently as can be seen here at "Nullable enums" https://swagger.io/docs/specification/data-models/enums/.

1. Maybe it would make sense to generate a 2nd schema Article81Nullable (example name) with the proper nullable enum spec.

2. Or instead of generating a schema, instead replace the usage of $ref with all fields from the schema which would normally be generated. This would duplicate a lot of data for every place where that enum would be used, BUT it would make it easier to generate proper nullable enums.

[bnowak]

Hello @DjordyKoert

The spec you sent is dedicated only for OAS 3.0 version, and there's nullable field which doesn't exist in OAS 3.1 (which is compatible with the newest json schema draft where we can define many types as alternative).

That's true, that null has to be included in enum list (in OAS 3.0) to field be validated correctly, however it's version-specific. As Nelmio doesn't consider for which version of OAS is generating spec, I think it should be some general/universal definition which swagger-php would be able to handle correctly for both of them.

I think the spec with oneOf and null type would work both versions (also defining null in enum list wouldn't be needed as oneOf is on top of both types):

[
    'foo' => [
        'oneOf' => [
            ['$ref' => '#/components/schemas/Foo'],
            ['type' => 'null'],
        ],
    ],
]

So basically it's version for OAS 3.1, but it's backward compatibility with OAS 3.0

Addressing your suggestions:

1. what such 2nd schema would bring additionally, as in current Article81 there is nullable enum type already public readonly ?ArticleType81 $nullableType ?
2. I think using $ref here is proper approach as it can be reused on different levels/parts of API spec (similar as classes) without duplications.

I wouldn't treat nullable enums properties, in the same way as null would be one of allowed value of enum itself as it's invalid definition in PHP (at least for now):

IMO it's rather on type level that it could be enum OR null exclusively.

I would be happy to implement solution as proposed (with oneOf), but would like to be with you "on the same page" firstly :)
Also, I will repeat my question about allOf - do you know why we're wrapping enums with it? What was the reason?

[DjordyKoert]

I will repeat my question about allOf - do you know why we're wrapping enums with it? What was the reason?

The main reason for using this is because the swagger editor does not show a proper input field when using oneOf #2112 (comment)

There is also an open issue on swagger-ui swagger-api/swagger-ui#7912

A fix suitable fix for both openapi versions (3.0 & 3.1) would be by doing one of the things I mentioned earlier #2226 (comment) or by keeping as it is (even if oneOf is the more accurate field 😢).

[bnowak]

ok I've got it now, thanks. I've added one question to swagger-ui issue for more context.

However, returning back to the issue:

A fix suitable fix for both openapi versions (3.0 & 3.1) would be by doing one of the things I mentioned earlier #2226 (comment) or by keeping as it is (even if oneOf is the more accurate field 😢).

Keeping as it is (with allOf) doesn't solve anything, as still the issue in 3.1 is about the impossibility to match both types at the same time:
https://www.jsonschemavalidator.net/s/RlfcQG1A (with oneOf version it works as expected https://www.jsonschemavalidator.net/s/cJ39zB0m)
also, I don't understand how your suggestions would solve this for both versions. Could you elaborate it a bit more, or give some examples please?

[DjordyKoert]

aslo, I don't understand how your #2226 (comment) would solve this for both versions. Could you elaborate it a bit more, or give some examples please?

I stand corrected, I have done some testing with the swagger editor (https://editor.swagger.io/ & https://editor-next.swagger.io/) and my potential 'fixes' and none of those are properly visible OR they simply send incorrect data (when using Try it out). I think this is an issue which should be fixed in https://github.com/swagger-api/swagger-ui.

openapi: 3.0.0
paths:
  /1:
    get:
      parameters:
      - name: order
        in: query
        description: oneOf does not create a dropdown.
        schema:
          oneOf:
          - $ref: '#/components/schemas/SortOrder'
  /2:
    get:
      parameters:
      - name: order
        in: query
        description: allOf DOES create dropdown.
        schema:
          allOf:
          - $ref: '#/components/schemas/SortOrder'
  /3:
    get:
      parameters:
      - name: order
        in: query
        description: allOf with nullable does not show null as an option
        schema:
            nullable: true
            allOf:
            - $ref: '#/components/schemas/SortOrder'
  /4:
    get:
      parameters:
      - name: order
        in: query
        description: allOf with nullable does not allow sending a null value
        required: true
        schema:
            nullable: true
            $ref: '#/components/schemas/SortOrder'
  /5:
    get:
      parameters:
      - name: order
        in: query
        description: NullableSortOrder sends ?order=null which does not get handles by Symfony's MapQueryParameter properly (results in an exception being thrown)
        required: true
        schema:
            $ref: '#/components/schemas/NullableSortOrder'
components:
  schemas:
    SortOrder:
      type: string
      enum:
      - ascending
      - descending
    NullableSortOrder:
      type: string
      nullable: true
      enum:
      - ascending
      - descending
      - null

My conclusion:

Let's always use oneOf and remove the usage of allOf for BOTH OpenApi versions and accept that the swagger-ui will NOT show a dropdown until swagger-ui fixes it themself.

[bnowak]

cool, so I'll adjust PR accordingly :)

Thanks for involving into the topic and being so responsively 👍

[bnowak]

@DjordyKoert done, ready for review

[DjordyKoert]

Thank you @bnowak!