Corrected schema generation for `array<string, T>`, `object`, `?array`, `?object` and `?type` by Ocramius · Pull Request #3402 · api-platform/core · GitHub

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no - please check Type::BUILTIN_TYPE_OBJECT changes though
Deprecations?	no
Tickets
License	MIT
Doc PR

The previous version of the TypeFactory generated following WRONG definitions:

null|T[] as {"type": array, "items": {"type": "T"}}
?T[] as {"type": array, "items": {"type": "T"}}
array<string, T> as {"type": array, "items": {"type": "T"}}
object without explicit schema definition as {"type": "string"}
?T as {"type": T}

The new definitions instead do fix this by mapping:

array<string, T> as {"type": "object", "additionalProperties": {"type": "T"}}
array<string, ?T> as {"type": object, "additionalProperties": {"type": ["T", "null"]}}
null|array<string, T> as {"type": ["object", "null"], "additionalProperties": {"type": "T"}}
array<int, T> as {"type": "array", "items": {"type": "T"}} (not very precise, but list support is not yet in symfony)
object without explicit schema definition as {"type": "object"}
?T[] as {"type": "array", "items": {"type": ["T", "null"]}}
null|T[] as {"type": ["array", "null"], "items": {"type": "T"}}

These annotations were too specific, since the returned type would never match anyway. Specifically, psalm cannot yet declare `ArrayType1&ArrayType2`, so an union type of two arrays is not currently something we can declare explicitly. Ref: api-platform#3402 (comment)

Hmm, can't run the failing test cases locally:

8) ApiPlatform\Core\Tests\Bridge\Symfony\Bundle\Test\ApiTestCaseTest::testFindIriBy
ReflectionException: Class "MongoDB\Driver\Monitoring\CommandSubscriber" not found while loading "Doctrine\Bundle\MongoDBBundle\APM\PSRCommandLogger".

/home/ocramius/Documents/api-platform/core/vendor/symfony/config/Resource/ClassExistenceResource.php:74
/home/ocramius/Documents/api-platform/core/vendor/symfony/dependency-injection/ContainerBuilder.php:353
/home/ocramius/Documents/api-platform/core/vendor/symfony/dependency-injection/Compiler/PriorityTaggedServiceTrait.php:64
/home/ocramius/Documents/api-platform/core/vendor/symfony/dependency-injection/Compiler/ResolveTaggedIteratorArgumentPass.php:34
/home/ocramius/Documents/api-platform/core/vendor/symfony/dependency-injection/Compiler/AbstractRecursivePass.php:82
/home/ocramius/Documents/api-platform/core/vendor/symfony/dependency-injection/Compiler/ResolveTaggedIteratorArgumentPass.php:31
/home/ocramius/Documents/api-platform/core/vendor/symfony/dependency-injection/Compiler/AbstractRecursivePass.php:91
/home/ocramius/Documents/api-platform/core/vendor/symfony/dependency-injection/Compiler/ResolveTaggedIteratorArgumentPass.php:31
/home/ocramius/Documents/api-platform/core/vendor/symfony/dependency-injection/Compiler/AbstractRecursivePass.php:82
/home/ocramius/Documents/api-platform/core/vendor/symfony/dependency-injection/Compiler/ResolveTaggedIteratorArgumentPass.php:31
/home/ocramius/Documents/api-platform/core/vendor/symfony/dependency-injection/Compiler/AbstractRecursivePass.php:46
/home/ocramius/Documents/api-platform/core/vendor/symfony/dependency-injection/Compiler/Compiler.php:94
/home/ocramius/Documents/api-platform/core/vendor/symfony/dependency-injection/ContainerBuilder.php:762
/home/ocramius/Documents/api-platform/core/vendor/symfony/http-kernel/Kernel.php:626
/home/ocramius/Documents/api-platform/core/vendor/symfony/http-kernel/Kernel.php:136
/home/ocramius/Documents/api-platform/core/vendor/symfony/framework-bundle/Test/KernelTestCase.php:80
/home/ocramius/Documents/api-platform/core/tests/Bridge/Symfony/Bundle/Test/ApiTestCaseTest.php:163

These annotations were too specific, since the returned type would never match anyway. Specifically, psalm cannot yet declare `ArrayType1&ArrayType2`, so an union type of two arrays is not currently something we can declare explicitly. Ref: api-platform#3402 (comment)

Similarly to the above, cannot really dump the generated OpenAPI schema to check what validation doesn't work (fails schema validation at https://travis-ci.org/api-platform/core/jobs/652720111)

Did some local testing against a real world project: seems to work correctly (thus far), but the type definitions aren't necessarily very elaborate (especially $ref nullability missing).

If anyone can give me a copy of the generated schema in the failing builds, lemme know: will fix up if it's broken by this patch, or will fix the patch if the generated schema is indeed pants-over-head broken 👍

Ok, seems like a misconception (coming from swagger-world). The official docs state: https://swagger.io/docs/specification/data-models/data-types/

type takes a single value. type as a list is not valid in OpenAPI (even though it is valid in JSON Schema):

I'll have to fix that :-P (I was working with OpenAPI 3.1 stuff)

As per OpenAPI documentation at https://swagger.io/docs/specification/data-models/data-types/ the OpenAPI v3 documentation does not allow defining union types via `type: ['string', 'integer']`, but requires explicit usage of `oneOf` and nested type declarations or references instead. Ref: api-platform#3402 (comment)

These annotations were too specific, since the returned type would never match anyway. Specifically, psalm cannot yet declare `ArrayType1&ArrayType2`, so an union type of two arrays is not currently something we can declare explicitly. Ref: api-platform#3402 (comment)

As per OpenAPI documentation at https://swagger.io/docs/specification/data-models/data-types/ the OpenAPI v3 documentation does not allow defining union types via `type: ['string', 'integer']`, but requires explicit usage of `oneOf` and nested type declarations or references instead. Ref: api-platform#3402 (comment)

Right, seems like the generated schema is now valid for OpenAPI, but Swagger will obviously be "special", since oneOf doesn't seem to properly work.

OpenAPI 2.0 doesn't seem to have the 'null' type at all: what to do?

@dunglas can we chop the head off 2.x? Would need to re-target the patch.

Unfortunately many tools in the OpenAPI ecosystem aren’t supporting v3 yet. We plan to remove support for v2 in API Platform (and we hope most tools will support OpenAPI v3 at this point).

Unfortunately many tools in the OpenAPI ecosystem aren’t supporting v3 yet. We plan to remove support for v2 in API Platform (and we hope most tools will support OpenAPI v3 at this point).

Supposedly, we'll need different service configurations for when the v2 and v3 schemas are replaced then, to allow TypeFactory to be swapped out?

We'd then inject both services into the command, and decide which to run depending on the flag: would that work as an approach? Problem with it is that I don't have a stack where to test that properly.

Yes that sounds good to me. Regarding the stack, I’ll check tomorrow morning (sorry I only have my phone with me right now).

Looking at ApiPlatform\Core\JsonSchema\TypeFactory::getType(), without introducing a BC break or too much complexity right now, I could add an 'openapi-v3.0' key (optional) that switches the entire behavior on/off conditionally.

Ugly as heck, but then it would be easy to switch over to the new behavior once v2 is gone.

This API is only used in 2 locations, so the addition shouldn't be too problematic.

What is unclear to me is why the validator still fails in CI (without 2.0 flags)

?T[]

What does this mean? Is this syntax defined somewhere?

EDIT: Uhh... Looks like it's a phpstan/phpdoc-parser thing.

I guess the only hint is here: https://github.com/phpstan/phpdoc-parser/blob/928179efc5368145a8b03cb20d58cb3f3136afae/tests/PHPStan/Parser/TypeParserTest.php#L793-L800 - but it seems to indicate array<T>|null as opposed to array<T|null>.

?T[]

What does this mean? Is this syntax defined somewhere?

Mostly psalm/phpstan. T stands for a type variable. This is generally unbeknownst to most of the PHP community, but very common in contexts where better type systems exist (Scala, Java, Kotlin, Haskell, C++, etc.)

004 Scenario: Get collection by dummyBoolean false             # features/doctrine/boolean_filter.feature:129
      When I send a "GET" request to "/dummies?dummyBoolean=0" # features/doctrine/boolean_filter.feature:130
        Type error: Too few arguments to function ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy::hasRole(), 0 passed in /tmp/api-platform/core/vendor/symfony/property-access/PropertyAccessor.php on line 382 and exactly 1 expected (Behat\Testwork\Call\Exception\FatalThrowableError)

No clue what's going on here: is it already broken in 2.5? Seems like unrelated to schema definitions, unless schema definitions are also re-used internally (at runtime)

@Ocramius the failure you reported is probably because of a new bug in Symfony, @teohhanhui is investigating it.

More misunderstanding of the spec: ["type" => "null"] is not valid OpenAPI v3. Instead, "nullable": true is to be used, but special handling is needed for $ref composition

@teohhanhui is investigating it

I better start investigating...

…`, `?object` and `?type` The previous version of the `TypeFactory` generated following **WRONG** definitions: * `null|T[]` as `{"type": array, "items": {"type": "T"}}` * `?T[]` as `{"type": array, "items": {"type": "T"}}` * `array<string, T> as `{"type": array, "items": {"type": "T"}}` * `object` without explicit schema definition as `{"type": "string"}` * `?T` as `{"type": T}` The new definitions instead do fix this by mapping: * `array<string, T>` as `{"type": "object", "additionalProperties": {"type": "T"}}` * `array<string, ?T> as `{"type": object, "additionalProperties": {"type": ["T", "null"]}}` * `null|array<string, T>` as `{"type": ["object", "null"], "additionalProperties": {"type": "T"}}` * `array<int, T>` as `{"type": "array", "items": {"type": "T"}}` (not very precise, but list support is not yet in symfony) * `object` without explicit schema definition as `{"type": "object"}` * `?T[]` as `{"type": "array", "items": {"type": ["T", "null"]}}` * `null|T[]` as `{"type": ["array", "null"], "items": {"type": "T"}}`

These annotations were too specific, since the returned type would never match anyway. Specifically, psalm cannot yet declare `ArrayType1&ArrayType2`, so an union type of two arrays is not currently something we can declare explicitly. Ref: api-platform#3402 (comment)

As per OpenAPI documentation at https://swagger.io/docs/specification/data-models/data-types/ the OpenAPI v3 documentation does not allow defining union types via `type: ['string', 'integer']`, but requires explicit usage of `oneOf` and nested type declarations or references instead. Ref: api-platform#3402 (comment)

…I v2 OpenAPI V2 has no way to generate accurate nullable types, so we need to disable nullable `oneOf` syntax in JSON-Schema by providing some context to the `TypeFactory` when not operating under OpenAPI v3 or newer considerations. In future, Swagger/OpenAPIV2 will (finally) at some point disappear, so we will be able to get rid of these conditionals once that happens.

…nAPI 3.0 OpenAPI 3.1 is not yet released, but fixes nullability in the way we had fixed it before (via `{"oneOf": [{"type": "null"}, ...]}`) in OAI/OpenAPI-Specification#1977. Until OpenAPI 3.1 is released, things like ``{"type": ["integer", "null"]}` are not valid definitions (because `"null"` is not yet a recognized type). We'll stick to OpenAPI 3.0 for now, using: * `{"nullable": true, ...}` for simple types * `{"nullable": true, "anyOf": [{"$ref": ...}]}` for type references

…": "string"}` To avoid BC breaks, we defer this fix to api-platform#3403 > **API Platform version(s) affected**: 2.5.x > > **Description** > > In our tests for `TypeFactory`: > > ``` > > yield [['type' => 'string'], new Type(Type::BUILTIN_TYPE_OBJECT)]; > yield [['type' => 'string'], new Type(Type::BUILTIN_TYPE_OBJECT, true)]; > > // ... > > yield [['type' => 'string'], new Type(Type::BUILTIN_TYPE_OBJECT, false, Dummy::class)]; > yield [['type' => 'string'], new Type(Type::BUILTIN_TYPE_OBJECT, true, Dummy::class)]; > ``` > > While reviewing api-platform#3402, @dunglas found a potential BC break with objects that may be used in URIs as `string`s (therefore not `objects`): > > * [api-platform#3402 (comment)](api-platform#3402 (comment)) > > * [api-platform#3402 (comment)](api-platform#3402 (comment)) > > > **How to reproduce** > > The test should instead convert `object` to `object`: > > ``` > > yield [['type' => 'object'], new Type(Type::BUILTIN_TYPE_OBJECT)]; > yield [['type' => 'object'], new Type(Type::BUILTIN_TYPE_OBJECT, true)]; > > // ... > > yield [['type' => 'object'], new Type(Type::BUILTIN_TYPE_OBJECT, false, Dummy::class)]; > yield [['type' => 'object'], new Type(Type::BUILTIN_TYPE_OBJECT, true, Dummy::class)]; > ```

Ref: api-platform#3402 (comment)

…a::getVersion()

Thanks @Ocramius! 🎉 🚀

Houston, we have a problem.

Swagger UI does not support oneOf / anyOf / allOf: swagger-api/swagger-ui#3803

This causes the property to be omitted from the example entirely.

On top of that, the desired nullability effect is not achieved: OAI/OpenAPI-Specification#1368 (comment)

Is this pertinent to v3 or v2? For v2, we disabled everything, so the swagger UI should still work.

See the linked issues. Unfortunately, it seems like our use of anyOf has not gained us anything (does not fulfill the goal of allowing null value for a relation), but has caused us to lose something (unintentionally breaking Swagger UI - though it's really their fault at this point).

We can use the NullValue trick from OAI/OpenAPI-Specification#1368 (comment)

@Ocramius would it fit your use case if we only add null type for OpenAPI >= 3.1? I think it might be the sweet spot.

@teohhanhui is there an issue tracking that? I would kinda isolate it to a smaller scope.

Hello there, is it normal, that the schema generated based on annotation @ORM\Column(type="array") could override the schema generated based on the PHPDoc ?

Example:

/**
 * @var int[]
 * @ORM\Column(type="array")
 */
private $productIds = [];

Going to generate an openAPI schema with string items that who looks like this:

"productIds": {
        "description": "...",
        "type": "array",
        "items": {
            "type": "string"
        }
    },

Instead of having an array of integer items:

"productIds": {
        "description": "...",
        "type": "array",
        "items": {
            "type": "integer"
        }
    },

Cheers!

These annotations were too specific, since the returned type would never match anyway. Specifically, psalm cannot yet declare `ArrayType1&ArrayType2`, so an union type of two arrays is not currently something we can declare explicitly. Ref: api-platform/core#3402 (comment)

As per OpenAPI documentation at https://swagger.io/docs/specification/data-models/data-types/ the OpenAPI v3 documentation does not allow defining union types via `type: ['string', 'integer']`, but requires explicit usage of `oneOf` and nested type declarations or references instead. Ref: api-platform/core#3402 (comment)

…": "string"}` To avoid BC breaks, we defer this fix to api-platform/core#3403 > **API Platform version(s) affected**: 2.5.x > > **Description** > > In our tests for `TypeFactory`: > > ``` > > yield [['type' => 'string'], new Type(Type::BUILTIN_TYPE_OBJECT)]; > yield [['type' => 'string'], new Type(Type::BUILTIN_TYPE_OBJECT, true)]; > > // ... > > yield [['type' => 'string'], new Type(Type::BUILTIN_TYPE_OBJECT, false, Dummy::class)]; > yield [['type' => 'string'], new Type(Type::BUILTIN_TYPE_OBJECT, true, Dummy::class)]; > ``` > > While reviewing #3402, @dunglas found a potential BC break with objects that may be used in URIs as `string`s (therefore not `objects`): > > * [#3402 (comment)](api-platform/core#3402 (comment)) > > * [#3402 (comment)](api-platform/core#3402 (comment)) > > > **How to reproduce** > > The test should instead convert `object` to `object`: > > ``` > > yield [['type' => 'object'], new Type(Type::BUILTIN_TYPE_OBJECT)]; > yield [['type' => 'object'], new Type(Type::BUILTIN_TYPE_OBJECT, true)]; > > // ... > > yield [['type' => 'object'], new Type(Type::BUILTIN_TYPE_OBJECT, false, Dummy::class)]; > yield [['type' => 'object'], new Type(Type::BUILTIN_TYPE_OBJECT, true, Dummy::class)]; > ```

Ref: api-platform/core#3402 (comment)

Ocramius commented Feb 19, 2020

View reviewed changes

Comment thread tests/JsonSchema/TypeFactoryTest.php Outdated

Comment thread tests/JsonSchema/TypeFactoryTest.php Outdated

alanpoulain reviewed Feb 19, 2020

View reviewed changes

Comment thread src/JsonSchema/TypeFactory.php Outdated

alanpoulain reviewed Feb 19, 2020

View reviewed changes

Comment thread src/JsonSchema/TypeFactory.php Outdated

zerkms reviewed Feb 19, 2020

View reviewed changes

Comment thread src/JsonSchema/TypeFactory.php

Ocramius force-pushed the fix/correct-json-schema-definitions-for-nullability-and-nested-structures branch from 899f1e3 to a68808d Compare February 19, 2020 22:34

Ocramius force-pushed the fix/correct-json-schema-definitions-for-nullability-and-nested-structures branch from f4630a7 to 6b6c40d Compare February 19, 2020 23:19

teohhanhui reviewed Feb 20, 2020

View reviewed changes

Comment thread src/JsonSchema/TypeFactory.php Outdated

Ocramius force-pushed the fix/correct-json-schema-definitions-for-nullability-and-nested-structures branch 2 times, most recently from 39578be to 03edb55 Compare February 20, 2020 11:11

dunglas reviewed Feb 20, 2020

View reviewed changes

Comment thread src/JsonSchema/TypeFactory.php Outdated

Ocramius and others added 9 commits February 27, 2020 15:38

Moved public const declarations after use of traits

48afa94

Ref: api-platform#3402 (comment)

Squashed coalesce operation into one line

25697ee

Ref: api-platform#3402 (comment)

Revert changes related to $serializerContext as we already have Schem…

9d17089

…a::getVersion()

teohhanhui force-pushed the fix/correct-json-schema-definitions-for-nullability-and-nested-structures branch from edaac89 to 9d17089 Compare February 27, 2020 15:32

teohhanhui merged commit 713b3ed into api-platform:2.5 Feb 27, 2020

Ocramius deleted the fix/correct-json-schema-definitions-for-nullability-and-nested-structures branch March 3, 2020 17:13

soyuka mentioned this pull request Mar 4, 2020

SchemaFactory fail for type @var array<string, string> api-platform/api-platform#1439

Closed

Korbeil mentioned this pull request Apr 24, 2020

Adjust null handling in exported JSON Schema / OpenAPI #3518

Closed

alanpoulain mentioned this pull request Nov 6, 2020

[JSON Schema] Manage correctly nullability #3817

Merged

alanpoulain mentioned this pull request Mar 9, 2021

[OpenAPI] Add nullable property description #3312

Closed

guilliamxavier mentioned this pull request Apr 9, 2021

JsonSchema doesn't handle nullable properties that are objects #4133

Closed

soyuka pushed a commit to api-platform/json-schema that referenced this pull request Mar 21, 2023

Moved public const declarations after use of traits

e0a2491

Ref: api-platform/core#3402 (comment)

soyuka pushed a commit to api-platform/json-schema that referenced this pull request Mar 21, 2023

Squashed coalesce operation into one line

6e4cd21

Ref: api-platform/core#3402 (comment)

Corrected schema generation for `array<string, T>`, `object`, `?array`, `?object` and `?type` by Ocramius · Pull Request #3402 · api-platform/core · GitHub

Uh oh!

Uh oh!

Corrected schema generation for array<string, T>, object, ?array, ?object and ?type - #3402

Conversation

Ocramius commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ocramius commented Feb 19, 2020

Uh oh!

Ocramius commented Feb 19, 2020

Uh oh!

Ocramius commented Feb 19, 2020

Uh oh!

Ocramius commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ocramius commented Feb 19, 2020

Uh oh!

dunglas commented Feb 19, 2020

Uh oh!

Ocramius commented Feb 19, 2020

Uh oh!

dunglas commented Feb 19, 2020

Uh oh!

Ocramius commented Feb 19, 2020

Uh oh!

Uh oh!

teohhanhui commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ocramius commented Feb 20, 2020

Uh oh!

Ocramius commented Feb 20, 2020

Uh oh!

Uh oh!

dunglas commented Feb 20, 2020

Uh oh!

Ocramius commented Feb 20, 2020

Uh oh!

teohhanhui commented Feb 20, 2020

Uh oh!

teohhanhui commented Feb 27, 2020

Uh oh!

teohhanhui commented Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ocramius commented Mar 3, 2020

Uh oh!

teohhanhui commented Mar 3, 2020

Uh oh!

teohhanhui commented Mar 3, 2020

Uh oh!

teohhanhui commented Mar 4, 2020

Uh oh!

Ocramius commented Apr 22, 2020

Uh oh!

ahmed-bhs commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Ocramius commented Feb 19, 2020 •
edited
Loading

Ocramius commented Feb 19, 2020 •
edited
Loading

teohhanhui commented Feb 20, 2020 •
edited
Loading

teohhanhui commented Mar 3, 2020 •
edited
Loading

ahmed-bhs commented Sep 20, 2021 •
edited
Loading