Allow a ref to be nullable

[momala454]

Fixes #1338

It creates a oneOf with a single ref, and with nullable true

[momala454]

the test fails, because it looks like by default "category" on Pet is nullable, but it contains this

/**
     * @OA\Property(
     *     title="Category",
     * )
     *
     * @var Category
     */
    private $category;

by default, properties are nullable ?

With the PR, it generates

   Pet:
      title: 'Pet model'
      description: 'Pet model'
      required:
        - name
        - photoUrls
      properties:
        id:
          title: ID
          description: ID
          type: integer
          format: int64
        category:
          nullable: true
          oneOf:
            -
              $ref: '#/components/schemas/Category'
        name:
          title: 'Pet name'
          description: 'Pet name'
          type: integer
          format: int64
        photoUrls:
          title: 'Photo urls'
          description: 'Photo urls'
          type: array
          items:
            type: string
            default: images/image-1.png
          xml:
            name: photoUrl
            wrapped: true
        tags:
          title: 'Pet tags'
          description: 'Pet tags'
          type: array
          items:
            $ref: '#/components/schemas/Tag'
          xml:
            name: tag
            wrapped: true
      type: object
      xml:
        name: Pet

[momala454]

requires #1397 to pass tests

[momala454]

i have no idea what are those errors on the tests, they seems unrelevant

[DerManoMann]

Yeah, there is another pr with the same...
I'll see if I can fix this on master.

[DerManoMann]

I update master so a new rebase/merge should fix the remaining build issues.

[momala454]

I rebased and the build is successful.
Can this PR be merged if you're ok with it ?

[DerManoMann]

Looking good I think. I don't think there is any way of oneOf already being set...

It would be cool if you could add a scratch test (file +spec) that tests this incl. handling of summary and description

[momala454]

so should I also support summary and description the same as I support nullable or should I make a test that test my nullable thing and also test if summary and description are added on version 3.1.0 ?

[DerManoMann]

Both, kinda. A test for both 3.0 and 3.1 would be great.
3.1 allows those other two properties next to $ref, so I would expect those to be also in the one of.

[momala454]

So for < 3.1 I must add the possibility to add description and summary beside nullable too ? Not sure if I understood correctly

[DerManoMann]

I just merged #1405 which adds a scratch test for refs in combination with nullable (ignored) and description (ignored in 3.0.0).
II would expect the yaml to change for all 4 cases with the description in the 3.1.0 case to be still present next to $ref inside the oneOf.

refplus endpoint in NullRef31.yaml:

              schema:
                $ref: '#/components/schemas/repository'
                description: 'The repository'

[momala454]

is it possible that something have a $ref AND a type ?
When i add {nullable: true, oneOf: [...]}, this code try to add type, but it does not exist :

if ($this->_context->version == OpenApi::VERSION_3_1_0) {
            if (isset($data->nullable)) {
                if (true === $data->nullable) {
                    $data->type = (array) $data->type;
                    $data->type[] = 'null';
                }
                unset($data->nullable);
            }
        }

Shouldn't I add something like

if ($this->_context->version == OpenApi::VERSION_3_1_0) {
            if (isset($data->nullable) && isset($data->type)) {
                if (true === $data->nullable) {
                    $data->type = (array) $data->type;
                    $data->type[] = 'null';
                }
                unset($data->nullable);
            }
        }

or maybe unset($data['nullable']); when I enter into my code that add oneOf

or

if ($this->_context->version == OpenApi::VERSION_3_1_0) {
            if (isset($data->nullable)) {
                if (true === $data->nullable) {
                    if (isset($data->type)) {
                        $data->type = [$data->type];
                    } else {
                        $data->type = [];
                    }
                    $data->type[] = 'null';
                }
                unset($data->nullable);
            }
        }

[DerManoMann]

Good point. I think unsetting nullable is actually the correct thing to do.

In addition there are two more things that should change:

1. The line $defaultValues = get_class_vars(get_class($this)); is already in the method a little further up (around line 360). Moving it out of the if clause should allow to remove the one you added.
2. I think the generated yaml is not correct in terms of the nullable property.
   I think the yaml should look something like ([BUG][codegen] Nullable references, nullable primitive types and complex types OpenAPITools/openapi-generator#5180 (comment)):

OptionalAddress:
      oneOf:
      - type: 'null'
      - $ref: "#/definitions/Address"

[momala454]

1. yes
2. this is only for 3.1 ? or for 3.0 it should be

OptionalAddress:
oneOf:
- nullable: true
- $ref: "#/definitions/Address"

[DerManoMann]

I do not believe having nullable inside oneOf is legit. To me this is mostly about 3.1 support.
The openapi-generator issue linked above contains some interesting variations and I would argue that adding just 3.1 support is enough until we have figured out a stable way to handle it in 3.0.

[momala454]

i need it on 3.0 as I can't upgrade until swagger ui support it :/

[momala454]

i updated the code for both 3.0 and 3.1.
The code style check is not successful but it doesn't say exactly what is the problem on the worker

[DerManoMann]

You can fix code style issues by running composer cs
I'll have a look in the next few days.

[DerManoMann]

Couple questions to clean up, otherwise looking good. I wonder if we could add a non-ref element to the scratch test and make it nullable - to see what happens when $ref is not set.

[DerManoMann]

Good stuff otherwise - looking forward to merging as this seems to be a major pain point....

[momala454]

thanks for your review. I updated the code

[DerManoMann]

Nice work @momala454 , thanks a lot.

[momala454]

thank you.
However I just remembered your message about a new test #1391 (review)

[DerManoMann]

Yeah, but I checked - I do not think that can happen. At least not how I thought the code is working.

[momala454]

Thank you.
Can you please also take a look at my issue #1396
it's flooding me every time I generate my swagger file