Improve order filter generated API doc #2971

fredericbarthelet · 2019-08-07T14:08:43Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#2891
License	MIT
Doc PR	-

alanpoulain

LGTM. Is it used in the UI?

fredericbarthelet · 2019-08-07T16:24:27Z

Yep

src/Bridge/Doctrine/Common/Filter/OrderFilterTrait.php

teohhanhui · 2019-08-08T13:23:42Z

src/Bridge/Doctrine/Common/Filter/OrderFilterTrait.php

@@ -52,6 +52,10 @@ public function getDescription(string $resourceClass): array
                'property' => $propertyName,
                'type' => 'string',
                'required' => false,
+                'enum' => [
+                    strtolower(OrderFilterInterface::DIRECTION_ASC),


Actually the order filter parameter could also be used without a value, e.g. GET /books?order[datePublished]

It'd use the default order configured for that property. (I don't like it, but it's something that's supported.)

See Behat test scenario here:

core/features/doctrine/order_filter.feature

Line 244 in cfa8f34

Scenario: Get collection ordered by default configured order on a string property and on which order filter has been enabled in whitelist mode with default descending order

It can be documented like this but could be used without value too, doesn't it?

Enum usually means you must use one of the specified values in the list. Haha...

So, there is allowEmptyValue ~~in OpenAPI v3~~ (it's also in Swagger v2). We could probably add that for other filters too, where applicable. But perhaps in another PR.

So this PR could be merged as is.

@alanpoulain See the other review comment above.

dunglas · 2019-08-20T13:09:16Z

@teohhanhui can we merge this one?

teohhanhui · 2019-08-20T13:09:47Z

Test is missing for the Swagger DocumentationNormalizer.

fredericbarthelet · 2019-08-20T13:58:12Z

@dunglas @teohhanhui test added in DocumentationNormalizerV3Test

teohhanhui · 2019-08-20T17:58:28Z

Thanks @fredericbarthelet! 🎉

dunglas · 2019-08-20T18:01:22Z

src/Bridge/Doctrine/Common/Filter/OrderFilterTrait.php

@@ -52,6 +52,13 @@ public function getDescription(string $resourceClass): array
                'property' => $propertyName,
                'type' => 'string',


Actually, I think that we should deprecate type and required, in favor of schema (because JSON Schema already supports type and required). Using a JSON Schema to describe the filter is smart, and we should only support that. WDYT @teohhanhui?

I think that'll just make a lot of code more complicated...

dunglas · 2019-08-20T18:05:36Z

src/Swagger/Serializer/DocumentationNormalizer.php

@@ -690,6 +690,10 @@ private function getFiltersParameters(bool $v3, string $resourceClass, string $o
                $type = \in_array($data['type'], Type::$builtinTypes, true) ? $this->jsonSchemaTypeFactory->getType(new Type($data['type'], false, null, $data['is_collection'] ?? false)) : ['type' => 'string'];
                $v3 ? $parameter['schema'] = $type : $parameter += $type;

+                if ($v3 && isset($data['schema'])) {


Shouldn't we do this test before calling $this->jsonSchemaTypeFactory->getType, in order to not call it when it's useless (when data['schema'] exists)? Also, are the provided schema incompatible with Swagger v2?

We already do useless work like this in many other places in this class. (At least it was the case before. Not sure after your refactoring.)

I guess it's okay in this case as it keeps the code as simple as possible.

Also, are the provided schema incompatible with Swagger v2?

It has no support for schema in parameters.

We already do useless work like this in many other places in this class. (At least it was the case before. Not sure after your refactoring.)

I tried to improve it as much as possible :)

I guess it's okay in this case as it keeps the code as simple as possible.

It doesn't increase the complexity that much:

if ($v3 && isset($data['schema'])) { $parameter['schema'] = $data['schema']; } else { $type = \in_array($data['type'], Type::$builtinTypes, true) ? $this->jsonSchemaTypeFactory->getType(new Type($data['type'], false, null, $data['is_collection'] ?? false)) : ['type' => 'string']; $v3 ? $parameter['schema'] = $type : $parameter += $type; }

(it could probably be more simplified btw)

It has no support for schema in parameters.

It is supported: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#fixed-fields-7 (If in is "body":).

And because it is supported, the code can be simplified a lot:

$type = $data['schema'] ?? \in_array($data['type'], Type::$builtinTypes, true) ? $this->jsonSchemaTypeFactory->getType(new Type($data['type'], false, null, $data['is_collection'] ?? false)) : ['type' => 'string']; $v3 ? $parameter['schema'] = $type : $parameter += $type;

No, it's not supported. That's for the request body. We're talking about filters here.

fredericbarthelet force-pushed the improve-filter-generated-doc branch 2 times, most recently from ec560b4 to 9caa985 Compare August 7, 2019 14:58

alanpoulain approved these changes Aug 7, 2019

View reviewed changes

teohhanhui reviewed Aug 8, 2019

View reviewed changes

src/Bridge/Doctrine/Common/Filter/OrderFilterTrait.php Outdated Show resolved Hide resolved

teohhanhui reviewed Aug 8, 2019

View reviewed changes

fredericbarthelet force-pushed the improve-filter-generated-doc branch from 9caa985 to 087d53f Compare August 10, 2019 18:53

fredericbarthelet force-pushed the improve-filter-generated-doc branch 3 times, most recently from a140df4 to b2f3ef3 Compare August 20, 2019 13:04

fredericbarthelet force-pushed the improve-filter-generated-doc branch from b2f3ef3 to f05eb7c Compare August 20, 2019 13:45

Add enum specification in order filter API docs

8dbf90f

fredericbarthelet force-pushed the improve-filter-generated-doc branch from f05eb7c to 8dbf90f Compare August 20, 2019 16:48

fredericbarthelet mentioned this pull request Aug 20, 2019

Improve serialization groups filter doc #3007

Closed

teohhanhui approved these changes Aug 20, 2019

View reviewed changes

teohhanhui merged commit 1c19bf5 into api-platform:master Aug 20, 2019

dunglas reviewed Aug 20, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve order filter generated API doc #2971

Improve order filter generated API doc #2971

fredericbarthelet commented Aug 7, 2019

alanpoulain left a comment

fredericbarthelet commented Aug 7, 2019

teohhanhui Aug 8, 2019

alanpoulain Aug 8, 2019

teohhanhui Aug 8, 2019

teohhanhui Aug 8, 2019 •

edited

alanpoulain Aug 8, 2019

teohhanhui Aug 8, 2019 •

edited

dunglas commented Aug 20, 2019

teohhanhui commented Aug 20, 2019

fredericbarthelet commented Aug 20, 2019

teohhanhui commented Aug 20, 2019

dunglas Aug 20, 2019 •

edited

teohhanhui Aug 20, 2019

dunglas Aug 20, 2019

teohhanhui Aug 20, 2019 •

edited

dunglas Aug 20, 2019

dunglas Aug 20, 2019

teohhanhui Aug 20, 2019

		@@ -52,6 +52,13 @@ public function getDescription(string $resourceClass): array
		'property' => $propertyName,
		'type' => 'string',

Improve order filter generated API doc #2971

Improve order filter generated API doc #2971

Conversation

fredericbarthelet commented Aug 7, 2019

alanpoulain left a comment

Choose a reason for hiding this comment

fredericbarthelet commented Aug 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teohhanhui Aug 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teohhanhui Aug 8, 2019 • edited

Choose a reason for hiding this comment

dunglas commented Aug 20, 2019

teohhanhui commented Aug 20, 2019

fredericbarthelet commented Aug 20, 2019

teohhanhui commented Aug 20, 2019

dunglas Aug 20, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teohhanhui Aug 20, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teohhanhui Aug 8, 2019 •

edited

teohhanhui Aug 8, 2019 •

edited

dunglas Aug 20, 2019 •

edited

teohhanhui Aug 20, 2019 •

edited