Validation issue when using allOf with $ref

[osteel]

Hi,

Validating responses whose schemas is a reference with some overwritten values doesn't seem to work properly. What's seemingly happening is that the reference is correctly read but the overwritten values are ignored.

Here is a test class so you can see for yourself:

<?php

declare(strict_types=1);

namespace League\OpenAPIValidation\Tests\FromCommunity;

use GuzzleHttp\Psr7\Response;
use League\OpenAPIValidation\PSR7\OperationAddress;
use League\OpenAPIValidation\PSR7\ValidatorBuilder;
use PHPUnit\Framework\TestCase;

use function GuzzleHttp\Psr7\stream_for;

final class IssueWithAllOfTest extends TestCase
{
    private function assertResponseMatchesSpec(string $spec): void
    {
        $validator = (new ValidatorBuilder())->fromYaml($spec)->getResponseValidator();
        $operation = new OperationAddress('/dummy', 'get');
        $response  = (new Response())
            ->withHeader('Content-Type', 'application/json')
            ->withBody(stream_for(json_encode(['test' => null])));

        $validator->validate($operation, $response);
        $this->addToAssertionCount(1);
    }

    public function testNoRefOk(): void
    {
        $spec = <<<SPEC
openapi: 3.0.2
info:
  title: Dummy API
  version: 0.0.1
paths:
  /dummy:
    get:
      summary: dummy
      operationId: dummy
      responses:
        200:
          description: dummy
          content:
            application/json:
              schema:
                type: object
                properties:
                  test:
                    type: string
                    description: a dummy object
                    nullable: true
                    example: foobar
SPEC;

        $this->assertResponseMatchesSpec($spec);
    }

    public function testRefOnlyOk(): void
    {
        $spec = <<<SPEC
openapi: 3.0.2
info:
  title: Dummy API
  version: 0.0.1
paths:
  /dummy:
    get:
      summary: dummy
      operationId: dummy
      responses:
        200:
          description: dummy
          content:
            application/json:
              schema:
                \$ref: '#/components/schemas/dummy'
components:
  schemas:
    dummy:
      type: object
      properties:
        test:
          type: string
          description: a dummy object
          nullable: true
          example: foobar
SPEC;

        $this->assertResponseMatchesSpec($spec);
    }

    public function testAllOfWithRefNok(): void
    {
        $spec = <<<SPEC
openapi: 3.0.2
info:
  title: Dummy API
  version: 0.0.1
paths:
  /dummy:
    get:
      summary: dummy
      operationId: dummy
      responses:
        200:
          description: dummy
          content:
            application/json:
              schema:
                type: object
                properties:
                  test:
                    allOf:
                      - \$ref: '#/components/schemas/dummy'
                      - description: different description
                        nullable: true
components:
  schemas:
    dummy:
      type: string
      description: a dummy object
      example: foobar
SPEC;

        $this->assertResponseMatchesSpec($spec);
    }

    public function testAllOfWithoutRefNok(): void
    {
        $spec = <<<SPEC
openapi: 3.0.2
info:
  title: Dummy API
  version: 0.0.1
paths:
  /dummy:
    get:
      summary: dummy
      operationId: dummy
      responses:
        200:
          description: dummy
          content:
            application/json:
              schema:
                type: object
                properties:
                  test:
                    allOf:
                      - type: string
                        description: a dummy object
                        example: foobar
                      - description: different description
                        nullable: true
SPEC;

        $this->assertResponseMatchesSpec($spec);
    }

    public function testAllOfWithoutRefBothNullableNok(): void
    {
        $spec = <<<SPEC
openapi: 3.0.2
info:
  title: Dummy API
  version: 0.0.1
paths:
  /dummy:
    get:
      summary: dummy
      operationId: dummy
      responses:
        200:
          description: dummy
          content:
            application/json:
              schema:
                type: object
                properties:
                  test:
                    allOf:
                      - description: different description
                        nullable: true
                      - type: string
                        description: a dummy object
                        example: foobar
                        nullable: true
SPEC;

        $this->assertResponseMatchesSpec($spec);
    }
}

It seems kinda related to this issue.

I'm also not 100% sure if this is coming from this package or cebe/php-openapi.

I'm using version 0.15.2 at the moment.

Cheers,

Yannick

[scaytrase]

Hi, all "OK" tests do not utilze AllOf. Can you provide same exact test but with spec instead of $ref. I think the problem is not with $ref

[osteel]

@scaytrase sorry, not sure I follow. I provided three different tests which are all actually the same in terms of schema, but achieved differently:

• testNoRefOk: the schema is 100% in the path definition – the test passes;
• testRefOnlyOk: the schema is 100% in a reference – the test passes;
• testAllOfWithRefNok: the schema is partly defined in the path definition, partly in a reference and is recomposed using allOf – the test fails.

All schemas are equivalent, but when using allOf the properties which are defined in the path definition seem to be ignored.

Is that clearer?

[scaytrase]

The current tests are clear, but they are not allow me to diagnose since you're changing two things at once - adding override and adding $ref.

I'm asking you to replace two first tests with the third one, but having inline schema instead of $ref. This way we can see whether $ref is breaking allOf or allOf is not working at all

[osteel]

@scaytrase I've updated my first message with the fix and a fourth test checking allOf with inline definitions only.

You seem to be right – that test fails too

[scaytrase]

Yea, my guess was right. I think nullable is set to false by default, so you can't just override it with allOf sibling. Since in this case you're getting like

allOf:
 - nullable: false
 - nullable: true

Which results in definition impossible to match. I think the root cause is here (example, not the behaviour)

https://github.com/cebe/php-openapi/blob/9796b0e0859c5284bd48b9357ee679fa6f422114/tests/spec/SchemaTest.php#L36-L37

I don't really thing it comes against OAS3 as OAS3 clearly says that "if you need to allow null - set nullable: true". That also means nullable is by default false.

[canvural]

type: object
properties:
  test:
    allOf:
      - type: string
        description: a dummy object
        example: foobar
      - description: different description
        nullable: true

Is this even a valid schema? Doesn't the second schema of allOf also needs a type?

[scaytrase]

We've discussed somewhere, that actually type is not required keyword here. This explicitly means that data can be any type

[osteel]

Could there be discrepancies in the way documentation tools such as Swagger UI and other tools like cebe/phpopenapi interpret OpenAPI definitions?

Because with this example:

type: object
properties:
  test:
    allOf:
      - type: string
        description: a dummy object
        example: foobar
      - description: different description
        nullable: true

Swagger UI correctly (in my opinion) understands that the resulting schema has nullable set to true, and the second description overrides the first one.

Even from the documentation, it is my understanding that at least the properties which are in the second schema but not in the first one should be in the resulting schema (in this case, nullable).

[scaytrase]

I found no clear statements about default nullability. Currently cebe/php-openapi implements as explicity false but maybe it should be like undefined treated as false by default.

@cebe WDYT ?

[osteel]

I've given this another go and updated my initial post with a 5th test where both sides have nullable set to true. The test still fails (Keyword validation failed: Value cannot be null).

Now I admit I may be trying to use allOf in a way that it's not completely intended for, but it seems that there might be an issue here, still.

@cebe would be great to have your input on this

[zerodahero]

I'm also looking at this same situation. It's unclear to me what the intended outcome is from the docs since this situation doesn't seem to be explicitly called out in any way.

A few things I found which may be helpful to the conversation?

• JSON schema (of which I believe OAS is a superset) expresses allOf scenarios as requiring a match with all of the given blocks (docs). The OAS docs on null don't seem to call out any kind of "default" value for nullable if it's not set.
• So if the implied "default" for nullable is false, we would be validating against allOf: [nullable: true, nullable: false] which according to the JSON schema docs is acceptable (that it conflicts). (We would expect it to be both nullable and non-nullable, in which case only non-nullable successfully validates)
• An article from Stoplight (they sell api spec related products) seems to show the kind of behavior @osteel described above would have the intended behavior of setting nullable: true as the end result of validation.
• The Swagger online editor seems to confirm the intended behavior as a merge as well, with the last definition taking precedence.

[scaytrase]

I think this topic should be raised to the OAS 3 core itself. We will support any outcome whatether they decide.

[zerodahero]

Looks like there's an existing issue related to this on core. The original issue was on enum and nullable, but looks like the conversation shifted to this situation here starting with this comment.

The result of that conversation is a clarification proposal for OAS 3.0.3

In that proposal (link), our situation here would be considered incorrect OAS:

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.

Also noted is that the default value for nullable would be considered false.

From my read of this, it seems to answer this question (i.e. change the way I've written my spec), and at least my understanding of it would indicate there's no changes necessary here (or on the underlying php-openapi)?

[osteel]

Not what I would have hoped for, but fair enough! Thanks for digging that up @zerodahero.

I still think the last test case I provided indicates that something is wrong (nullable set to true on both sides of the allOf and yet the validator complaining that the value cannot be null).

That being said, it's now clear that we should not attempt to overwrite schema elements using allOf, which kinda settles that.

Closing for now as I don't think there's much that can be done about it

[AlbinoDrought]

I'm in a similar situation where nullable: true seems to be ignored, even in less-ambiguous contexts like this:

    public function testAllOfSingle(): void
    {
        $spec = <<<SPEC
openapi: 3.0.2
info:
  title: Dummy API
  version: 0.0.1
paths:
  /dummy:
    get:
      summary: dummy
      operationId: dummy
      responses:
        200:
          description: dummy
          content:
            application/json:
              schema:
                \$ref: '#/components/schemas/dummy'
components:
  schemas:
    dummy:
      type: object
      properties:
        test:
          allOf:
            - type: string
              description: a dummy object
              nullable: true
              example: foobar
SPEC;

        $this->assertResponseMatchesSpec($spec);
    }

This seems like it will become less of a problem once OpenAPI 3.1.0 becomes widely used - it replaces nullable: true with type: null. As an alternative for now, this gets accepted: (add nullable: true to the base schema, outside of anyOf/allOf/oneOf)

    public function testAllOfSingleRootNullable(): void
    {
        $spec = <<<SPEC
openapi: 3.0.2
info:
  title: Dummy API
  version: 0.0.1
paths:
  /dummy:
    get:
      summary: dummy
      operationId: dummy
      responses:
        200:
          description: dummy
          content:
            application/json:
              schema:
                \$ref: '#/components/schemas/dummy'
components:
  schemas:
    dummy:
      type: object
      properties:
        test:
          nullable: true
          allOf:
            - type: string
              description: a dummy object
              example: foobar
SPEC;

        $this->assertResponseMatchesSpec($spec);
    }