-
-
Notifications
You must be signed in to change notification settings - Fork 888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Corrected schema generation for array<string, T>
, object
, ?array
, ?object
and ?type
#3402
Corrected schema generation for array<string, T>
, object
, ?array
, ?object
and ?type
#3402
Conversation
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:
|
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)
899f1e3
to
a68808d
Compare
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 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/
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)
f4630a7
to
6b6c40d
Compare
Right, seems like the generated schema is now valid for OpenAPI, but Swagger will obviously be "special", since OpenAPI 2.0 doesn't seem to have the @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). |
Supposedly, we'll need different service configurations for when the 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 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 |
What does this mean? Is this syntax defined somewhere? EDIT: Uhh... Looks like it's a 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 |
39578be
to
03edb55
Compare
Mostly psalm/phpstan. |
No clue what's going on here: is it already broken in |
@Ocramius the failure you reported is probably because of a new bug in Symfony, @teohhanhui is investigating it. |
More misunderstanding of the spec: |
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)]; > ```
edaac89
to
9d17089
Compare
Thanks @Ocramius! 🎉 🚀 |
Houston, we have a problem. Swagger UI does not support 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 |
We can use the |
@Ocramius would it fit your use case if we only add |
@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 Example: /**
* @var int[]
* @ORM\Column(type="array")
*/
private $productIds = []; Going to generate an openAPI schema with string items that who looks like this:
Instead of having an array of integer items:
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)]; > ```
Type::BUILTIN_TYPE_OBJECT
changes thoughThe 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"}}