add a specific "documentation_formats" that may differ from the API "… #1891

jdeniau · 2018-04-25T12:31:28Z

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	See descrption
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1854
License	MIT
Doc PR	api-platform/docs#475

This PR allow to disable the "json" format from the api and only allow jsonld for example, without breaking the swagger documentation.

About the deprecation though: I removed "json" and "html" from the formats, to keep only "jsonld", and moved "json" and "html" to the documentation formats only.
Technically, it's a compatibility break, but as often can be seen as a bugfix (at least for the html format).

If you like, I can add the json format back, but as I see it, api-platform default is really nicely coupled with json-jd, so I assumed that the json format it was for documentation only.

dunglas · 2018-04-25T12:51:37Z

src/EventListener/AddFormatListener.php

    private $mimeTypes;

-    public function __construct(Negotiator $negotiator, array $formats)
+    public function __construct(Negotiator $negotiator, array $formats, array $documentationFormats)


It must have a default value (= []), or it's BC break.

dunglas · 2018-04-25T12:52:15Z

src/EventListener/AddFormatListener.php

+
+    private function getRequestFormats(Request $request): array
+    {
+        return $this->isDocumentation($request)


Can you inline this please?

dunglas · 2018-04-25T12:52:33Z

src/EventListener/AddFormatListener.php

+        ;
+    }
+
+    private function isDocumentation(Request $request): bool


Can you remove this function? (It's in the hot path, and function calls are costly).

dunglas · 2018-04-25T12:56:30Z

src/EventListener/AddFormatListener.php

+
+    private function isDocumentation(Request $request): bool
+    {
+        return 'api_platform.action.documentation' === $request->attributes->get('_controller');


Alternative proposal:

set a _api_documentation attribute here https://github.com/api-platform/core/blob/master/src/Bridge/Symfony/Bundle/Resources/config/routing/docs.xml#L10, and test for the presence of this flag. It would be more flexible and give an extension point.

@dunglas What you think about a _api_endpoint_type set to documentation instead ? It may avoid anyone to add another variable later.

I usually prefer boolean flags (almost all others are boolean). What other type can it be?

may something related to graphql for example 🤷‍♂️

dunglas · 2018-04-26T09:40:10Z

src/Bridge/Symfony/Bundle/EventListener/SwaggerUiListener.php

@@ -34,4 +34,10 @@ public function onKernelRequest(GetResponseEvent $event)

        $request->attributes->set('_controller', 'api_platform.swagger.action.ui');
    }
+
+    public function addEndpointTypeToRequest(GetResponseEvent $event)


Is it necessary? The flag will always be set for every request even if it's not caught by Swagger UI isn't it?

Oh, I need to set the same check as onKernelRequest function.

I need to separate the two listener because the AddFormatListener come between priority 0 and 7

jdeniau · 2018-04-26T10:25:22Z

@dunglas done, I just changed _api_documentation by _api_endpoint_type set to documentation.

For the circleci test, I think it's a problem with circleci configuration

jdeniau · 2018-04-26T11:15:18Z

@dunglas I'm kind of stuck here with a circular exception on the SwaggerUiListener, maybe you have an idea :

The SwaggerUiListener depends on the AddFormatListener for the text/html request format to be set.

The AddFormatListener need to know if we are on a documentation page or not, and thus needs to have the documentation attribute set.

The only solution I see here is to dissociate the AddFormatListener and something that throw the exception but it seems really overkill for such a small feature, don't yout think ?

dunglas · 2018-04-30T09:51:45Z

I would handle Swagger UI in two different ways:

For the /docs URL: handle it using documentation_formats as done in this PR
For all others URL, consider that HTML is not a "documentation" format (because it is not really, it allows to serialize the resource itself as HTML). So there are no need to mark all those routes as "documentation" (they are not, they are resources routes). I would just let the hijacking mechanism working as it is currently, except that I suggest to disable the hijacking if Swagger is not present in documentation_formats. To clarify: disable the HTML format if Swagger is not enabled in the DI extension.

WDYT?

dunglas · 2018-05-14T09:41:05Z

Any news on this one?

jdeniau · 2018-05-14T10:46:13Z

Sorry, I was on holidays ;) I will check your return soon.

…

On Mon, May 14, 2018 at 11:41 AM, Kévin Dunglas ***@***.***> wrote: Any news on this one? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1891 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABVWxfTyDVE8A9n3F3IG7DOTLLU53DxPks5tyVEzgaJpZM4TjVZH> .

dunglas · 2018-05-24T10:30:21Z

Any news :)

jdeniau · 2018-05-24T12:08:05Z

@dunglas should be OK.

I removed the weird listener that I previously added.

As it is very internal functionality, I am not 100% sure of what I have done though ;)

dunglas · 2018-05-24T12:16:38Z

src/Bridge/Symfony/Bundle/Resources/config/swagger-ui.xml

@@ -7,7 +7,7 @@
    <services>

        <service id="api_platform.swagger.listener.ui" class="ApiPlatform\Core\Bridge\Symfony\Bundle\EventListener\SwaggerUiListener">
-            <tag name="kernel.event_listener" event="kernel.request" method="onKernelRequest" />


Why this change? 0 is the default priority.

jdeniau · 2018-05-24T12:29:22Z

Still an issue due to changes I made in the available formats

jdeniau · 2018-05-25T14:17:04Z

@dunglas everything is good

…formats". Fixes api-platform#1854

jdeniau · 2018-06-04T07:45:18Z

My turn to ping you @dunglas ;)

soyuka · 2018-06-04T08:19:49Z

tests/EventListener/AddFormatListenerTest.php

@@ -32,7 +32,7 @@ public function testNoResourceClass()
        $eventProphecy->getRequest()->willReturn($request)->shouldBeCalled();
        $event = $eventProphecy->reveal();

-        $listener = new AddFormatListener(new Negotiator(), ['notexist' => 'application/vnd.notexist']);
+        $listener = new AddFormatListener(new Negotiator(), ['notexist' => 'application/vnd.notexist'], []);


As the new array argument has a default value we don't really need to change this right?

dunglas

Can you also install this PR in the demo and check that URLs such as /books or /books/1 still display Swagger UI with the correct content inside when loaded trough a web browser please?

dunglas · 2018-06-04T08:37:05Z

features/main/content_negotiation.feature

@@ -110,8 +110,8 @@ Feature: Content Negotiation support
    Then the response status code should be 406
    And the header "Content-Type" should be equal to "application/problem+json; charset=utf-8"

-  Scenario: If the request format is HTML, the error should be in HTML


Can you keep this test (and duplicate it for XML if appropriate)? It checks that the documentation for any URL feature works (for instance when you open http://localhost/books/1, Swagger UI shows up and automatically runs the AJAX request).

dunglas · 2018-06-04T08:38:52Z

src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php

@@ -222,6 +222,9 @@ public function getConfigTreeBuilder()

        $this->addFormatSection($rootNode, 'formats', [
            'jsonld' => ['mime_types' => ['application/ld+json']],
+        ]);
+        $this->addFormatSection($rootNode, 'documentation_formats', [
+            'jsonld' => ['mime_types' => ['application/ld+json']], // Hydra documentation, admin and PWA generator
            'json' => ['mime_types' => ['application/json']], // Swagger support
            'html' => ['mime_types' => ['text/html']], // Swagger UI support


HTML must be defined also in addFormatSection section to have the "documentation for any URL" feature working.

dunglas · 2018-06-04T08:41:55Z

src/EventListener/AddFormatListener.php

+     */
+    private function getRequestAcceptedFormats(Request $request): array
+    {
+        if (!$request->attributes->has('_api_endpoint_type')) {


~~you can call get directly and store the result in a variable to prevent an extra function call. (get will return null if the attribute is not defined)~~

Just drop this function, see below.

dunglas · 2018-06-04T08:43:20Z

src/EventListener/AddFormatListener.php

@@ -51,13 +53,15 @@ public function onKernelRequest(GetResponseEvent $event)
            return;
        }

-        $this->populateMimeTypes();
-        $this->addRequestFormats($request, $this->formats);
+        $requestAcceptedFormats = $this->getRequestAcceptedFormats($request);


Actually, this private function can be replaced by 'documentation' === $request->attributes->get('_api_endpoint_type') ? $this->documentationFormats : $this->formats. It removes 2 function calls in the hot path.

dunglas · 2018-06-04T08:44:45Z

src/EventListener/AddFormatListener.php

@@ -84,11 +88,11 @@ public function onKernelRequest(GetResponseEvent $event)
                return;
            }

-            throw $this->getNotAcceptableHttpException($mimeType);
+            throw $this->getNotAcceptableHttpException($mimeType ?? $requestFormat);


$requestAcceptedFormats right? This should be covered by a test (because the error isn't caught now)

jdeniau · 2018-06-16T12:47:52Z

I'm kinda lost here because of #1798 : it added a format resolver but does not include error format.

Does documentation format should be in FormatsProvider ? What do you thing @antograssiot ?

antograssiot · 2018-06-16T13:36:28Z

Indeed there is no ErrorFormatsProvider (yet ?).
I can add it if there is a need, it should be pretty easy, and I guess that if you feel that document format needs in some cases to be specified at resource/operation level, you could use a DocumentFormatsProvider or something similar but it seems less likely to be used IMO.

Let's see maintainers opinion.

dunglas · 2018-08-26T15:53:18Z

@jdeniau, do you need any help to finish this PR?

jdeniau · 2018-08-26T18:44:24Z

Actually I think I do yes!
I am kind of lost here, because I don't know api-platform internals as you guys do.
Maybe I should look back at #1798 and try harder but any help would be appreciated!

flug · 2019-11-14T14:56:35Z

@jdeniau Up is this pull request still valid?

jdeniau · 2019-11-14T19:40:30Z

I did not manage to fix this and have no motivation on that now. I am closing this one.

jdeniau mentioned this pull request Apr 25, 2018

add default documentation formats api-platform/docs#475

Closed

jdeniau force-pushed the jd-feat-documentationFormats branch from f1ad101 to b0accf4 Compare April 25, 2018 12:36

dunglas requested changes Apr 25, 2018

View reviewed changes

jdeniau force-pushed the jd-feat-documentationFormats branch 3 times, most recently from 9bfc712 to e5a9af8 Compare April 26, 2018 07:32

dunglas reviewed Apr 26, 2018

View reviewed changes

jdeniau force-pushed the jd-feat-documentationFormats branch 2 times, most recently from 5e06a1e to d4ebcc9 Compare April 26, 2018 10:04

jdeniau force-pushed the jd-feat-documentationFormats branch from d4ebcc9 to d3d2660 Compare April 26, 2018 10:28

Simperfit added the enhancement label Apr 26, 2018

jdeniau force-pushed the jd-feat-documentationFormats branch from d3d2660 to 3645e4f Compare May 18, 2018 08:57

jdeniau force-pushed the jd-feat-documentationFormats branch from ebfab4c to 70e809b Compare May 24, 2018 12:05

dunglas reviewed May 24, 2018

View reviewed changes

dunglas approved these changes May 24, 2018

View reviewed changes

jdeniau force-pushed the jd-feat-documentationFormats branch from 70e809b to cdf3c2b Compare May 24, 2018 12:28

jdeniau force-pushed the jd-feat-documentationFormats branch 3 times, most recently from 7ac99f8 to 7b07a71 Compare May 25, 2018 14:16

jdeniau added 2 commits June 4, 2018 09:43

add a specific "documentation_formats" that may differ from the API "…

d799cb8

…formats". Fixes api-platform#1854

Set documentation endpoint type to all endpoints

2f4b017

Remove useless methods and fix tests

c106cb2

jdeniau force-pushed the jd-feat-documentationFormats branch from 7b07a71 to c106cb2 Compare June 4, 2018 07:43

soyuka approved these changes Jun 4, 2018

View reviewed changes

dunglas requested changes Jun 4, 2018

View reviewed changes

dunglas added this to the 2.5.0 milestone Jul 8, 2019

jdeniau closed this Nov 14, 2019

jdeniau deleted the jd-feat-documentationFormats branch September 27, 2023 06:41

Uh oh!

add a specific "documentation_formats" that may differ from the API "… #1891

add a specific "documentation_formats" that may differ from the API "… #1891

Uh oh!

Conversation

jdeniau commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdeniau Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdeniau commented Apr 26, 2018

Uh oh!

jdeniau commented Apr 26, 2018

Uh oh!

dunglas commented Apr 30, 2018

Uh oh!

dunglas commented May 14, 2018

Uh oh!

jdeniau commented May 14, 2018 via email

Uh oh!

dunglas commented May 24, 2018

Uh oh!

jdeniau commented May 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdeniau commented May 24, 2018

Uh oh!

jdeniau commented May 25, 2018

Uh oh!

jdeniau commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdeniau commented Jun 16, 2018

Uh oh!

antograssiot commented Jun 16, 2018

Uh oh!

dunglas commented Aug 26, 2018

Uh oh!

jdeniau commented Aug 26, 2018

jdeniau commented Apr 25, 2018 •

edited

Loading

jdeniau Apr 26, 2018 •

edited

Loading

jdeniau commented Jun 4, 2018 •

edited

Loading

dunglas left a comment •

edited

Loading

dunglas Jun 4, 2018 •

edited

Loading

dunglas Jun 4, 2018 •

edited

Loading

dunglas Jun 4, 2018 •

edited

Loading