Repository: hotmeteor/spectator (php)

Issue:
Nullable seems to fail?
I suddenly get an error on code that has worked before.

response->dump() returns the following 👍 

`email_verified_at: string? <== The data (null) must match the type: string`

This seems wrong to me.. the `string?` type clearly states the nullable attribute is set.

The property is defined as 

`@OA\Property(property="email_verified_at",description="Email verified at",example="2021-01-27 11:23:45", format="date-time", type="string", nullable=true )`

Any clue how to figure out what is going on?

Validation on nested object with `allOf` does not work correctly
I have the following case:

```
get:
  tags:
    - test
  summary: test
  description: test
  responses:
    '200':
      description: Success
      content:
        application/json:
          schema:
            type: object
            properties:
              data:
                type: array
                items:
                  allOf:
                    - type: object
                      properties:
                        id:
                          type: integer
                        NestedObject:
                          nullable: true
                          $ref: "NestedObject.yaml"
                    - $ref: "AnotherObject.yaml"
                    - $ref: "AnotherObject.yaml"
                          
```

Validation error:
```
 NestedObject: object++ <== The data (null) must match the type: object
```

it seems the `allOf` ignores the `nullable` tag and required all properties to not be null. 
The same code without `allOf`(manually merging objects) seems to work fine.

According to swagger's documentation if all objects in `allOf` are valid then the merged object is valid as well, but here this does not seem to be true.

Pull request:
Fix nullable and readonly/writeonly parsing
`AbstractValidator::prepareData`has 2 objectives :
- transform the `nullable:true` openapi 3.0 format to the openapi 3.1 format adding `"null"` to the types
- remove readonly/writeonly properties depending on the context or preparation (request or response)

Original code had some issues in the implementation : 
- nullable transformation was only done on root object (and it's properties) but not on a root array for example. Same thing with attribute filtering
- the recursion was done 2 times, not exactly the same way
- nullable objects or array where not correctly prepared
- ...

I hope that this new implementation resolve all of these issues. Every field is prepared and the treatments are run recursively to inner fields. We now correctly apply 3.0->3.1 nullable migration and readonly/writeonly filtering.

Instead of looking at the type of the property, I chose to look at the attributes.
This way, for an object declared like
```yml
title: User
type: object
nullable: true
properties:
  email: 
    type: string
    nullable: true
```
will be correctly translated to Openapi 3.1 style
```yml
title: User
type:
  - object
  - 'null'
properties:
  email: 
    type: 
      - string
      - 'null'
```
which I think was not the case.

I tried to implement tests for each issue it closes but not sure I covered all the cases.

Fixes #68
Fixes #99
Fixes #132
Fixes #145

Patch:
diff --git a/src/Validation/AbstractValidator.php b/src/Validation/AbstractValidator.php
index 60d64c5..a980fea 100644
--- a/src/Validation/AbstractValidator.php
+++ b/src/Validation/AbstractValidator.php
@@ -16,195 +16,134 @@ abstract class AbstractValidator
      * @param  string|null  $mode  Access mode 'read' or 'write'
      * @return mixed
      */
-    protected function prepareData(Schema $schema, ?string $mode = null)
+    protected function prepareData(Schema $schema, ?string $mode = null): object
     {
-        $data = $schema->getSerializableData();
+        return $this->prepareProperty($schema->getSerializableData(), $mode);
+    }
 
-        if (! isset($data->properties)) {
+    private function prepareProperty(object $data, ?string $mode): object
+    {
+        // Does this object contain an unresolved "$ref"? This occurs when `cebe\openapi\Reader`
+        // encounters a cyclical reference. Skip it.
+        if (data_get($data, '$ref')) {
             return $data;
         }
 
-        $clone = clone $data;
+        $data = $this->migrateNullableTo31Style($data);
 
-        $clone = $this->filterProperties($clone, $mode);
+        if ($this->shouldHaveProperties($data)) {
+            $data->properties ??= new \stdClass();
+        }
 
-        $v30 = Str::startsWith($this->version, '3.0');
+        match (true) {
+            isset($data->properties) => (function () use ($data, $mode) {
+                $data = $this->filterProperties($data, $mode);
+                foreach ($data->properties as $key => $property) {
+                    $data->properties->$key = $this->prepareProperty($property, $mode);
+                }
+            })(),
+            isset($data->items) => $data->items = $this->prepareProperty($data->items, $mode),
+            isset($data->anyOf) => $data->anyOf = array_map(
+                fn ($item) => $this->prepareProperty($item, $mode),
+                $data->anyOf,
+            ),
+            isset($data->allOf) => $data->allOf = array_map(
+                fn ($item) => $this->prepareProperty($item, $mode),
+                $data->allOf,
+            ),
+            isset($data->oneOf) => $data->oneOf = array_map(
+                fn ($item) => $this->prepareProperty($item, $mode),
+                $data->oneOf,
+            ),
+            default => null
+        };
 
-        if ($v30) {
-            $clone->properties = $this->wrapAttributesToArray($clone->properties);
+        return $data;
+    }
+
+    private function shouldHaveProperties(object $data): bool
+    {
+        if (! isset($data->type)) {
+            return false;
         }
 
-        return $clone;
+        if (is_string($data->type)) {
+            return $data->type === 'object';
+        }
+
+        if (is_array($data->type)) {
+            return in_array('object', $data->type);
+        }
+
+        return false;
     }
 
     /**
      * Filters out readonly|writeonly properties.
      *
-     * @param  string|null  $type  Access mode 'read' or 'write'
-     * @return mixed
+     * @param  string|null  $mode  Access mode 'read' or 'write'
      */
-    protected function filterProperties(object $data, ?string $mode = null): object
+    private function filterProperties(object $data, ?string $mode): object
     {
-        if (data_get($data, '$ref')) {
-            return $data;
-        }
+        $filterBy = match ($mode) {
+            'read' => 'writeOnly',
+            'write' => 'readOnly',
+            default => null,
+        };
 
-        switch ($mode) {
-            case 'read':
-                $filter_by = 'writeOnly';
-                break;
-            case 'write':
-                $filter_by = 'readOnly';
-                break;
-            default:
-                return $data;
-        }
-
-        if (isset($data->type) && $data->type === 'object') {
-            $data->properties ??= new \stdClass();
-        }
-
-        if (! isset($data->properties)) {
+        if ($filterBy === null) {
             return $data;
         }
 
-        /**
-         * Create a new array of properties that need to be filtered out.
-         */
-        $filter_properties = array_keys(
+        // Create a new array of properties that need to be filtered out.
+        $filterProperties = array_keys(
             array_filter(
                 (array) $data->properties,
-                function ($property) use ($filter_by) {
-                    return isset($property->$filter_by) && $property->$filter_by === true;
+                function ($property) use ($filterBy) {
+                    return isset($property->$filterBy) && $property->$filterBy === true;
                 },
             )
         );
 
-        /**
-         * Filter out properties from schema's properties.
-         */
-        foreach ($filter_properties as $property) {
+        //Filter out properties from schema's properties.
+        foreach ($filterProperties as $property) {
             unset($data->properties->$property);
         }
 
-        /**
-         * Filter out properties from schema's required properties array.
-         * (Skip if nothing to filter out).
-         */
+        // Filter out properties from schema's required properties array.
+        // (Skip if nothing to filter out).
         if (isset($data->required)) {
             $data->required = array_filter(
                 $data->required,
-                function ($property) use ($filter_properties) {
-                    return ! in_array($property, $filter_properties);
-                },
+                fn ($property) => ! in_array($property, $filterProperties),
             );
         }
 
-        foreach ($data->properties as $key => $property) {
-            if (isset($property->type)) {
-                $type = $property->type;
-            } elseif (isset($property->anyOf)) {
-                $type = 'anyOf';
-            } elseif (isset($property->allOf)) {
-                $type = 'allOf';
-            } elseif (isset($property->oneOf)) {
-                $type = 'oneOf';
-            } else {
-                $type = null;
-            }
-
-            switch ($type) {
-                case 'object':
-                    $data->properties->$key = $this->filterProperties($property, $mode);
-                    break;
-
-                case 'array':
-                    $property->items = $this->filterProperties($property->items, $mode);
-                    break;
-
-                case 'anyOf':
-                case 'allOf':
-                case 'oneOf':
-                    $property->$type = array_map(
-                        function ($item) use ($mode) {
-                            return $this->filterProperties($item, $mode);
-                        },
-                        $property->$type,
-                    );
-                    break;
-
-                default:
-                    // Unknown type, skip
-                    break;
-            }
-        }
-
         return $data;
     }
 
     /**
-     * Return an associative array mapping "objects" to "properties" for the purposes of spec testing.
-     * When this function finishes, you should have a structure with the following format:.
-     *
-     * [
-     *     "Pet" => "{ resolved properties of a pet }"
-     *     "Order" => "{ resolved properties of an order }"
-     *     ...
-     * ]
-     *
-     * @return mixed
+     * Migrate Openapi 3.0 nullable declaration to Openapi 3.1 style.
      */
-    protected function wrapAttributesToArray($properties)
+    private function migrateNullableTo31Style(object $data): object
     {
-        foreach ($properties as $key => $attributes) {
-            // Does this object contain an unresolved "$ref"? This occurs when `cebe\openapi\Reader`
-            // encounters a cyclical reference. Skip it.
-            if (data_get($attributes, '$ref')) {
-                break;
-            }
-
-            // Does this object define "nullable"? If so, unset "nullable" and include "null"
-            // in array of possible types (e.g. "type" => [..., "null"]).
-            if (isset($attributes->nullable)) {
-                if (isset($attributes->anyOf)) {
-                    $attributes->anyOf[] = (object) ['type' => 'null'];
-                } else {
-                    $type = Arr::wrap($attributes->type ?? null);
-                    $type[] = 'null';
-                    $attributes->type = array_unique($type);
-                }
-                unset($attributes->nullable);
-            }
-
-            // anyOf -> recurse ...
-            if (isset($attributes->anyOf)) {
-                $attributes->anyOf = $this->wrapAttributesToArray($attributes->anyOf);
-            }
-
-            // Before we check recursive cases, make sure this object defines a "type".
-            if (! isset($attributes->type)) {
-                break;
-            }
-
-            // This object has a sub-object, recurse...
-            if ($attributes->type === 'object' && isset($attributes->properties)) {
-                $attributes->properties = $this->wrapAttributesToArray($attributes->properties);
-            }
+        if (! Str::startsWith($this->version, '3.0')) {
+            return $data;
+        }
 
-            // This object is an array of sub-objects, recurse...
-            if (
-                isset($attributes->items, $attributes->items->properties) && $attributes->type === 'array' && $attributes->items->type === 'object'
-            ) {
-                $attributes->items->properties = $this->wrapAttributesToArray($attributes->items->properties);
-            }
-            // This object is an array of anyOf, recurse...
-            if (
-                isset($attributes->items, $attributes->items->anyOf) && $attributes->type === 'array'
-            ) {
-                $attributes->items->anyOf = $this->wrapAttributesToArray($attributes->items->anyOf);
+        // Does this object define "nullable"? If so, unset "nullable" and include "null"
+        // in array of possible types (e.g. "type" => [..., "null"]).
+        if (isset($data->nullable)) {
+            if (isset($data->anyOf)) {
+                $data->anyOf[] = (object) ['type' => 'null'];
+            } else {
+                $type = Arr::wrap($data->type ?? null);
+                $type[] = 'null';
+                $data->type = array_unique($type);
             }
+            unset($data->nullable);
         }
 
-        return $properties;
+        return $data;
     }
 }


Test patch:
diff --git a/tests/Fixtures/AllOf.v1.yml b/tests/Fixtures/AllOf.v1.yml
index 195d18f..d96e460 100644
--- a/tests/Fixtures/AllOf.v1.yml
+++ b/tests/Fixtures/AllOf.v1.yml
@@ -27,6 +27,29 @@ paths:
                   - $ref: '#/components/schemas/Dog'
                 discriminator:
                   propertyName: pet_type
+  /all-of-with-nullable:
+    get:
+      responses:
+        '200':
+          description: ok
+          content:
+            application/json:
+              schema:
+                type: object
+                properties:
+                  data:
+                    type: array
+                    items:
+                      allOf:
+                        - type: object
+                          properties:
+                            id:
+                              type: integer
+                            owner:
+                              type: string
+                              nullable: true
+                        - $ref: '#/components/schemas/Dog'
+
 components:
   schemas:
     Pet:
diff --git a/tests/Fixtures/Dictionary.v1.yml b/tests/Fixtures/Dictionary.v1.yml
index 1c8fcf5..d3c8621 100644
--- a/tests/Fixtures/Dictionary.v1.yml
+++ b/tests/Fixtures/Dictionary.v1.yml
@@ -21,5 +21,22 @@ paths:
                     type: object
                     additionalProperties:
                       type: integer
+  /nullable-dictionary-of-integers:
+    get:
+      summary: Get nullable dictionary of integers
+      tags: [ ]
+      responses:
+        '200':
+          description: OK
+          content:
+            application/json:
+              schema:
+                type: object
+                properties:
+                  data:
+                    type: object
+                    nullable: true
+                    additionalProperties:
+                      type: integer
 components:
   schemas: {}
diff --git a/tests/Fixtures/Nullable.3.0.json b/tests/Fixtures/Nullable.3.0.json
index b671608..c08d66d 100644
--- a/tests/Fixtures/Nullable.3.0.json
+++ b/tests/Fixtures/Nullable.3.0.json
@@ -92,6 +92,44 @@
       }
     },
     "/users": {
+      "get": {
+        "summary": "Get all users",
+        "tags": [],
+        "responses": {
+          "200": {
+            "description": "OK",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "type": "array",
+                  "items": {
+                    "type": "object",
+                    "properties": {
+                      "name": {
+                        "type": "string",
+                        "example": "Adam Campbell"
+                      },
+                      "email": {
+                        "type": "string",
+                        "example": "test@tets.com"
+                      },
+                      "nickname": {
+                        "type": "string",
+                        "example": "hotmeteor",
+                        "nullable": true
+                      }
+                    },
+                    "required": [
+                      "name",
+                      "email"
+                    ]
+                  }
+                }
+              }
+            }
+          }
+        }
+      },
       "post": {
         "summary": "Create user",
         "tags": [],
@@ -134,6 +172,34 @@
         },
         "operationId": "create-user"
       }
+    },
+    "/nullable-array-of-nullable-string": {
+      "get": {
+        "summary": "Get nullable array of nullable string",
+        "tags": [],
+        "responses": {
+          "200": {
+            "description": "OK",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "type": "object",
+                  "properties": {
+                    "data": {
+                      "type": "array",
+                      "nullable": true,
+                      "items": {
+                        "type": "string",
+                        "nullable": true
+                      }
+                    }
+                  }
+                }
+              }
+            }
+          }
+        }
+      }
     }
   },
   "components": {
diff --git a/tests/Fixtures/Nullable.3.1.json b/tests/Fixtures/Nullable.3.1.json
index ef2fe77..b4eeece 100644
--- a/tests/Fixtures/Nullable.3.1.json
+++ b/tests/Fixtures/Nullable.3.1.json
@@ -41,21 +41,30 @@
                       "example": "Bloggs"
                     },
                     "email": {
-                      "type": ["string", "null"],
+                      "type": [
+                        "string",
+                        "null"
+                      ],
                       "example": "test@test.com"
                     },
                     "settings": {
                       "type": "object",
                       "properties": {
                         "last_updated_at": {
-                          "type": ["string", "null"],
+                          "type": [
+                            "string",
+                            "null"
+                          ],
                           "example": "2020-10-10"
                         },
                         "notifications": {
                           "type": "object",
                           "properties": {
                             "email": {
-                              "type": ["string", "null"],
+                              "type": [
+                                "string",
+                                "null"
+                              ],
                               "example": "daily"
                             }
                           }
@@ -72,7 +81,10 @@
                             "example": "My first post"
                           },
                           "body": {
-                            "type": ["string", "null"],
+                            "type": [
+                              "string",
+                              "null"
+                            ],
                             "example": "Welcome to my blog!"
                           }
                         }
@@ -115,7 +127,10 @@
                     "example": "test@test.com"
                   },
                   "nickname": {
-                    "type": ["string", "null"],
+                    "type": [
+                      "string",
+                      "null"
+                    ],
                     "example": "hotmeteor"
                   }
                 },
@@ -129,6 +144,38 @@
         },
         "operationId": "create-user"
       }
+    },
+    "/nullable-array-of-nullable-string": {
+      "get": {
+        "summary": "Get nullable array of nullable string",
+        "tags": [],
+        "responses": {
+          "200": {
+            "description": "OK",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "type": "object",
+                  "properties": {
+                    "data": {
+                      "type": [
+                        "array",
+                        "null"
+                      ],
+                      "items": {
+                        "type": [
+                          "string",
+                          "null"
+                        ]
+                      }
+                    }
+                  }
+                }
+              }
+            }
+          }
+        }
+      }
     }
   },
   "components": {
diff --git a/tests/ResponseValidatorTest.php b/tests/ResponseValidatorTest.php
index f7b8e43..eca6c70 100644
--- a/tests/ResponseValidatorTest.php
+++ b/tests/ResponseValidatorTest.php
@@ -369,6 +369,102 @@ class ResponseValidatorTest extends TestCase
         ];
     }
 
+    public function test_array_of_objects_with_nullable(): void
+    {
+        Spectator::using('Nullable.3.0.json');
+
+        Route::get('/users', static function () {
+            return [
+                ['name' => 'John Doe', 'email' => 'john.doe@test.com'],
+                ['name' => 'Jane Doe', 'email' => 'jane.doe@test.com', 'nickname' => null],
+                ['name' => 'Adam Campbell', 'email' => 'test@test.com', 'nickname' => 'hotmeteor'],
+            ];
+        })->middleware(Middleware::class);
+
+        $this->getJson('/users')
+            ->assertValidRequest()
+            ->assertValidResponse();
+    }
+
+    /**
+     * @dataProvider nullableArrayOfNullableStringsProvider
+     */
+    public function test_nullable_array_of_nullable_strings($version, $payload, $isValid): void
+    {
+        Spectator::using("Nullable.$version.json");
+
+        Route::get('/nullable-array-of-nullable-string', static function () use ($payload) {
+            return ['data' => $payload];
+        })->middleware(Middleware::class);
+
+        $this->getJson('/nullable-array-of-nullable-string')
+            ->assertValidRequest()
+            ->assertValidResponse();
+
+        if ($isValid) {
+            $this->getJson('/nullable-array-of-nullable-string')
+                ->assertValidRequest()
+                ->assertValidResponse();
+        } else {
+            $this->getJson('/nullable-array-of-nullable-string')
+                ->assertValidRequest()
+                ->assertInvalidResponse();
+        }
+    }
+
+    public static function nullableArrayOfNullableStringsProvider()
+    {
+        $validResponse = true;
+        $invalidResponse = false;
+
+        $v30 = '3.0';
+        $v31 = '3.1';
+
+        return [
+            '3.0, null' => [
+                $v30,
+                null,
+                $validResponse,
+            ],
+            '3.0, array of strings' => [
+                $v30,
+                ['foo', 'bar'],
+                $validResponse,
+            ],
+            '3.0, array with null' => [
+                $v30,
+                ['foo', null],
+                $validResponse,
+            ],
+            '3.0, array with int' => [
+                $v30,
+                ['foo', null],
+                $invalidResponse,
+            ],
+            '3.1, null' => [
+                $v31,
+                null,
+                $validResponse,
+            ],
+            '3.1, array of strings' => [
+                $v31,
+                ['foo', 'bar'],
+                $validResponse,
+            ],
+            '3.1, array with null' => [
+                $v31,
+                ['foo', null],
+                $validResponse,
+            ],
+            '3.1, array with int' => [
+                $v31,
+                ['foo', null],
+                $invalidResponse,
+            ],
+
+        ];
+    }
+
     /**
      * @dataProvider oneOfSchemaProvider
      */
@@ -568,6 +664,80 @@ class ResponseValidatorTest extends TestCase
         ];
     }
 
+    /**
+     * @dataProvider allOfWithNullableProvider
+     */
+    public function test_handles_allOf_with_nullable($payload, $isValid): void
+    {
+        Spectator::using('AllOf.v1.yml');
+
+        Route::get('/all-of-with-nullable', static function () use ($payload) {
+            return ['data' => [$payload]];
+        })->middleware(Middleware::class);
+
+        $request = [
+            'pet_type' => 'Cat',
+            'age' => 3,
+            'hunts' => true,
+        ];
+
+        $handled_request = $this->getJson('/all-of-with-nullable', $request);
+
+        if ($isValid) {
+            $handled_request->assertValidResponse();
+        } else {
+            $handled_request->assertInvalidResponse();
+        }
+    }
+
+    public static function allOfWithNullableProvider(): array
+    {
+        $valid = true;
+        $invalid = false;
+
+        return [
+            'valid, Dog with owner' => [
+                [
+                    'id' => 1,
+                    'owner' => 'John Doe',
+                    'pet_type' => 'Dog',
+                    'bark' => true,
+                    'breed' => 'Husky',
+                ],
+                $valid,
+            ],
+            'valid, Dog without owner' => [
+                [
+                    'id' => 1,
+                    'owner' => null,
+                    'pet_type' => 'Dog',
+                    'bark' => true,
+                    'breed' => 'Husky',
+                ],
+                $valid,
+            ],
+            'invalid, owner missing' => [
+                [
+                    'id' => 1,
+                    'pet_type' => 'Dog',
+                    'bark' => true,
+                    'breed' => 'Husky',
+                ],
+                $invalid,
+            ],
+            'invalid, invalid owner missing' => [
+                [
+                    'id' => 1,
+                    'pet_type' => 'Dog',
+                    'owner' => false,
+                    'bark' => true,
+                    'breed' => 'Husky',
+                ],
+                $invalid,
+            ],
+        ];
+    }
+
     public function test_handles_invalid_spec(): void
     {
         Spectator::using('Malformed.v1.yaml');
@@ -872,4 +1042,50 @@ class ResponseValidatorTest extends TestCase
             ],
         ];
     }
+
+    /**
+     * @dataProvider nullableObjectAsDictionaryProvider
+     */
+    public function test_nullable_object_as_dictionary(mixed $payload, bool $isValid): void
+    {
+        Spectator::using('Dictionary.v1.yml');
+
+        Route::get('/nullable-dictionary-of-integers', static function () use ($payload) {
+            return ['data' => $payload];
+        })->middleware(Middleware::class);
+
+        if ($isValid) {
+            $this->getJson('/nullable-dictionary-of-integers')
+                ->assertValidResponse();
+        } else {
+            $this->getJson('/nullable-dictionary-of-integers')
+                ->assertInvalidResponse();
+        }
+    }
+
+    public static function nullableObjectAsDictionaryProvider(): array
+    {
+        return [
+            'valid' => [
+                ['foo' => 1, 'bar' => -1],
+                true,
+            ],
+            'valid as null' => [
+                null,
+                true,
+            ],
+            'invalid as string' => [
+                'foo',
+                false,
+            ],
+            'invalid as array' => [
+                [1, 2],
+                false,
+            ],
+            'invalid as dictionary of string' => [
+                ['foo' => 'foo', 'bar' => 'bar'],
+                false,
+            ],
+        ];
+    }
 }