nullable oneOf/anyOf value rejected

[AlbinoDrought]

<?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 League\OpenAPIValidation\Tests\PSR7\BaseValidatorTest;

final class IssueWithNullableMergeTest extends BaseValidatorTest
{
    public function testNullableMergeOneOf(): void
    {
        $yaml = /** @lang yaml */
               <<<'YAML'
openapi: 3.0.0
paths:
  /api/nullable-merge:
    get:
      description: 'Test'
      responses:
        '200':
          description: 'ok'
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Thing"
components:
  schemas:
    FooResult:
      type: object
      properties:
        id:
          type: integer
        foo:
          type: string
    BarResult:
      type: object
      nullable: true
      properties:
        id:
          type: integer
        bar:
          type: string
    Thing:
      type: object
      properties:
        result:
          oneOf:
            - $ref: "#/components/schemas/FooResult"
            - $ref: "#/components/schemas/BarResult"
YAML;

        $validator = (new ValidatorBuilder())->fromYaml($yaml)->getResponseValidator();
        $operation = new OperationAddress('/api/nullable-merge', 'get');

        $responseContent = /** @lang JSON */
            '
    {
      "result": null
    }
';

        $response = new Response(200, ['Content-Type' => 'application/json'], $responseContent);

        $validator->validate($operation, $response);

        $this->addToAssertionCount(1);
    }
}

FooResult is not nullable. BarResult is nullable. Thing.result can be one of FooResult or BarResult, so my expectation is that null should be an acceptable value. Validation fails with:

There was 1 error:

1) League\OpenAPIValidation\Tests\FromCommunity\IssueWithNullableMergeTest::testNullableMergeOneOf
League\OpenAPIValidation\PSR7\Exception\Validation\InvalidBody: Body does not match schema for content-type "application/json" for Response [get /api/nullable-merge 200]

/openapi-psr7-validator/src/PSR7/Exception/Validation/AddressValidationFailed.php:28
/openapi-psr7-validator/src/PSR7/Exception/Validation/InvalidBody.php:19
/openapi-psr7-validator/src/PSR7/Validators/BodyValidator/UnipartValidator.php:60
/openapi-psr7-validator/src/PSR7/Validators/BodyValidator/BodyValidator.php:73
/openapi-psr7-validator/src/PSR7/Validators/ValidatorChain.php:25
/openapi-psr7-validator/src/PSR7/ResponseValidator.php:42
/openapi-psr7-validator/tests/FromCommunity/IssueWithNullableMergeTest.php:68

Caused by
League\OpenAPIValidation\Schema\Exception\KeywordMismatch: Keyword validation failed: Value cannot be null

/openapi-psr7-validator/src/Schema/Exception/KeywordMismatch.php:22
/openapi-psr7-validator/src/Schema/Keywords/Nullable.php:21
/openapi-psr7-validator/src/Schema/SchemaValidator.php:60
/openapi-psr7-validator/src/Schema/Keywords/Properties.php:85
/openapi-psr7-validator/src/Schema/SchemaValidator.php:139
/openapi-psr7-validator/src/PSR7/Validators/BodyValidator/UnipartValidator.php:58
/openapi-psr7-validator/src/PSR7/Validators/BodyValidator/BodyValidator.php:73
/openapi-psr7-validator/src/PSR7/Validators/ValidatorChain.php:25
/openapi-psr7-validator/src/PSR7/ResponseValidator.php:42
/openapi-psr7-validator/tests/FromCommunity/IssueWithNullableMergeTest.php:68

This same issue happens if oneOf: in the above snippet is replaced with anyOf:.

Swagger UI shows the nullable: true constraint as expected:

[AlbinoDrought]

Here's a 3.1.0 example (which I think is not supported by this library yet) that has the same result:

<?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 League\OpenAPIValidation\Tests\PSR7\BaseValidatorTest;

final class IssueWithNullableMergeTest extends BaseValidatorTest
{
    public function testNullableMergeOneOf(): void
    {
        $yaml = /** @lang yaml */
               <<<'YAML'
openapi: 3.1.0
paths:
  /api/nullable-merge:
    get:
      description: 'Test'
      responses:
        '200':
          description: 'ok'
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Thing"
components:
  schemas:
    FooResult:
      type: object
      properties:
        id:
          type: integer
        foo:
          type: string
    Thing:
      type: object
      properties:
        result:
          oneOf:
            - $ref: "#/components/schemas/FooResult"
            - type: null
YAML;

        $validator = (new ValidatorBuilder())->fromYaml($yaml)->getResponseValidator();
        $operation = new OperationAddress('/api/nullable-merge', 'get');

        $responseContent = /** @lang JSON */
            '
    {
      "result": null
    }
';

        $response = new Response(200, ['Content-Type' => 'application/json'], $responseContent);

        $validator->validate($operation, $response);

        $this->addToAssertionCount(1);
    }
}

[AlbinoDrought]

This succeeds but I'm not sure if it's allowed according to spec: (nullable: true moved above oneOf:)

<?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 League\OpenAPIValidation\Tests\PSR7\BaseValidatorTest;

final class IssueWithNullableMergeTest extends BaseValidatorTest
{
    public function testNullableMergeOneOf(): void
    {
        $yaml = /** @lang yaml */
               <<<'YAML'
openapi: 3.0.0
paths:
  /api/nullable-merge:
    get:
      description: 'Test'
      responses:
        '200':
          description: 'ok'
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Thing"
components:
  schemas:
    FooResult:
      type: object
      properties:
        id:
          type: integer
        foo:
          type: string
    BarResult:
      type: object
      properties:
        id:
          type: integer
        bar:
          type: string
    Thing:
      type: object
      properties:
        result:
          nullable: true
          oneOf:
            - $ref: "#/components/schemas/FooResult"
            - $ref: "#/components/schemas/BarResult"
YAML;

        $validator = (new ValidatorBuilder())->fromYaml($yaml)->getResponseValidator();
        $operation = new OperationAddress('/api/nullable-merge', 'get');

        $responseContent = /** @lang JSON */
            '
    {
      "result": null
    }
';

        $response = new Response(200, ['Content-Type' => 'application/json'], $responseContent);

        $validator->validate($operation, $response);

        $this->addToAssertionCount(1);
    }
}

[scaytrase]

3.1 is not supported yet. I will check the given issue about nullables

[scaytrase]

As far as I understand the problem is that at the moment validator validates result field as a property which is no nullable by itself (by default nullable interpretation).

In OAS 3.0.3 and 3.1 they had clarifyied this behaviour it still says it's proposal but it's actually in the spec and in the release notes.

A true value adds "null" to the allowed type specified by the type keyword, only if type is explicitly defined within the same Schema Object. Other Schema Object constraints retain their defined behavior, and therefore may disallow the use of null as a value. A false value leaves the specified or default type unmodified. The default value is false.

I think we can adopt these changes and consider everything nullable by default (this follows the common principle of OAS which is every definition can only make constraints more strict, but not loose)

@cebe WDYT? Can we have nullable being true by default(or at least null to interpret null differently in different cases)?

[scaytrase]

Currently this is fixable with this kind of patch. But serializing schema is not really plays well with performance, so we need somehow check if property nullable is set on schema (maybe at least make SpecBaseObject::hasProperty public? or make nullable has default null value as for required ?)

Index: src/Schema/SchemaValidator.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/Schema/SchemaValidator.php b/src/Schema/SchemaValidator.php
--- a/src/Schema/SchemaValidator.php	(revision a87c402b6c9af07e4638ab542031c606483f75bd)
+++ b/src/Schema/SchemaValidator.php	(date 1635374751548)
@@ -56,8 +56,19 @@
         $breadCrumb = $breadCrumb ?? new BreadCrumb();
 
         try {
+
+            $nullable = true;
+            $rawSchema = $schema->getSerializableData();
+            if (!isset($rawSchema->nullable)) {
+                if ($schema->type !== null) {
+                    $nullable = false;
+                }
+            } else {
+                $nullable = $rawSchema->nullable;
+            }
+
             // These keywords are not part of the JSON Schema at all (new to OAS)
-            (new Nullable($schema))->validate($data, $schema->nullable);
+            (new Nullable($schema))->validate($data, $nullable);
 
             // We don't want to validate any more if the value is a valid Null
             if ($data === null) {

[cebe]

@cebe WDYT? Can we have nullable being true by default(or at least null to interpret null differently in different cases)?

according to what you cited from the spec it should be false by default, not true.

        $rawSchema = $schema->getSerializableData();
        if (!isset($rawSchema->nullable)) {

can't you just do if (!isset($schema->nullable)) { ?

[cebe]

nullable is false if not specified. here is the test case for that:

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

[cebe]

Looks like the default value of nullable should depend on whether type is set.

A true value adds "null" to the allowed type specified by the type keyword, only if type is explicitly defined within the same Schema Object.

[cebe]

So this can be adjusted in php-openapi, but you may also check whether a type is explicitly defined in the current schema, if there is no type, ignore the nullable value.

[scaytrase]

Looks like the default value of nullable should depend on whether type is set.

When the type IS set - it's the easy one, I agree.

But when it's NOT set - I see three cases:

1. No restrictions. This allows everything AND null

components:
  schemas:
    FooResult: {}

2. Nullable=false. This allows everyything BUT null

components:
  schemas:
    FooResult:
      nullable: false

3. Nullable=true. This allows everything AND null (and spec says this cases is non-sense since it's equal to case1)

components:
  schemas:
    FooResult:
      nullable: true

If nullable defaults to false in spec object - we cannot distinguish cases 1 and 2, since both of them will give me nullable: false on accesssing spec object property. That's why we need to distinguish true, false and unset cases for nullable.

For other cases - yes, we can do more smart checks. checking type first if it's present.

[cebe]

If the type is not set you must not check nullable at all.

A true value adds "null" to the allowed type specified by the type keyword, only if type is explicitly defined within the same Schema Object. Other Schema Object constraints retain their defined behavior, and therefore may disallow the use of null as a value. A false value leaves the specified or default type unmodified. The default value is false.

So in all cases there is no type sepecified, i.e. no type check is performed, therefore also no check for null.

I think this is part of the reason why nullable is removed in openapi 3.1 and instead null is part of the type array.

[cebe]

@scaytrase would be great to have your opinion on this change: cebe/php-openapi#132