Refactor the responder to ease usage of content negotiation #590

dunglas · 2016-07-02T14:36:29Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	todo

Thanks to this PR using the content negotiation feature of API Platform is much easier than before:

Formats supported by the Symfony Serializer can be used without writing 1 line of code (XML, JSON and soon CSV)
To support a new format, you just need to register a new encoder for this format (no responder needed anymore)

With this PR, enabling XML support for an existing API Platform app is just a matter of adding the following config:

# app/config/config.yml

api_platform:
    # ...
    formats:
        jsonld: ['application/ld+json']
        xml:    ['application/xml', 'text/xml']

The key is the format passed to the $format parameter of the Symfony Serializer. The value is an array of mime types corresponding to this format. Request containing an Accept header matching one of the configured mime type will trigger content negotiation. The API will reply with the mime type requested (if supported). If the requested type is not supported (or if the Accept header isn't set), the API will reply in the format configured to be the first.

ping @Simperfit @clementtalleu @gorghoa.

TODO:

Add some unit tests
~~Switch back JsonLd and Hydra actions to the ADR pattern~~

Simperfit · 2016-07-02T18:14:16Z

src/EventListener/SerializerViewListener.php

+        $resourceClass = $request->attributes->get('_resource_class');
+        $collectionOperationName = $request->attributes->get('_collection_operation_name');
+        $itemOperationName = $request->attributes->get('_item_operation_name');
+


What about the norm/denorm groups ?

We are using a custom ResponderViewListener with a part on the norm groups (like I updated the docs):

I may be wrong but what about something like this (denorm will be maybe needed too) ?

$itemContext = $resourceMetadata->getItemOperationAttribute($itemOperationName, 'normalization_context', ['groups' => []], false); $collectionContext = $resourceMetadata->getCollectionOperationAttribute($collectionOperationName, 'normalization_context', ['groups' => []], false); if (!$itemContext['groups'] && !$collectionContext['groups']) { $context = $resourceMetadata->getAttribute('normalization_context', []); } else { $context = array_merge($itemContext, $collectionContext); }

I'm 👎 for this change. The whole context is set on the resource or on a specific operation. The more specific one is used. Changing dynamically the content of the context will lead to weird edge cases.

What do I have to do then to have my collections groups taken in account with this change ?

Unregistring my Responder and using this I don't get my collections & item groups taken in account on my route.

Can you post your @ApiResource config?

If you set a context for a specific operation, you must set all groups in it. Groups from the resource will not be inherited (and this is intended). Are we talking about the same thing?

/** * @ApiResource(iri="http://schema.org/Person", attributes={ * "filters"={"person.search", "person.order", "person.subscribers", "person.customSearch", "person.subscription"}, * "normalization_context"={"groups"={"person_default_output", "postal_address_output", "organ_affiliation_default_output", "organ_default_output", "name_default_output", "time_default_output", "person_responsability_default_output", "person_receipt_output", "person_status_default_output", "person_mandate_default_output", "mandate_default_output", "territory_default_output", "bank_default_output"}}, * "denormalization_context"={"groups"={"person_default_input", "postal_address_input", "organ_affiliation_default_input", "organ_default_input", "name_default_input", "time_default_input", "person_responsability_default_input", "person_status_default_input", "person_mandate_default_input", "mandate_default_input", "territory_default_input", "bank_default_input"}} * }, itemOperations={ * "get"={"method"="GET"}, * "put"={"method"="PUT"}, * "export_subscribers"={ * "path"="people_export_subscribers", * "method"="GET", * }, * "export_emails"={ * "path"="people_export_emails", * "method"="GET", * } * }, * collectionOperations={ * "get"={"method"="GET"}, * "post"={"method"="POST"}, * "export_members_email"={ * "path"="/people/export_members_email", * "method"="GET", * "pagination_enabled"=false, * "normalization_context"={"groups"={"export_mails_output"}}, * } * } * ) */

In this case, in my export_members_email i've selected 3 fields. Using this route, with the CsvEncoder in the symfony's PR you made (thanks for that btw). I expect to get this three fields, instead of that I get everything

But in this case, they are inherited or I guess the normalization_context in the collectionOperations is not taken in account

Ok I just got why. I'll update the PR.

Can you try the last commit fix the issue @Simperfit?

It does work properly for me thank you !

Simperfit · 2016-07-04T07:22:59Z

supported_formats -> formats is a BC Break, not a big one, but still.

dunglas · 2016-07-04T07:52:49Z

@Simperfit yes, it is the last chance to introduce BC break. When 2.0 will be tagged it will be too late :-)

Simperfit · 2016-07-04T08:48:43Z

features/overriding_operation.feature

@@ -1,14 +1,14 @@
-Feature: Create-Retrieve-Update-Delete with custom attribute
+Feature: Create-Retrieve-Update-Delete with a Overrode Operation context


Maybe Override

an overridden operation

dunglas · 2016-07-04T15:45:59Z

ping @api-platform/core-team

I want to merge it for the release of the first beta (probably tomorrow).

theofidry · 2016-07-04T15:54:44Z

src/Api/RequestAttributesExtractor.php

     * @throws RuntimeException
     *
-     * @return array
+     * @return array [$resourceClass, $collectionOperation, $itemOperation, $format]


we should add an example with the @example tag to be more clear on what those values can be

IIUC @example is used to import external files as examples: https://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.example.pkg.html

According to http://stackoverflow.com/questions/15414103/best-way-to-document-array-options-in-phpdoc there is not better way that what I've done here but I'm to change that if someone has something better :)

A plain old example in the phpdoc to give an idea of what value you may have is good enough (besides what you added) IMO, @example or not

I've never been a fan of list. It's not explicit at all.

Hence the VO I wanted to introduce at some point :/ But what's important is that the user doesn't have to do a step by step debugging or a dump to understand what values are returned... the doc should be informative enough to avoid that

I'll change that to an associative array with explicit keys for now.

I just pushed a new commit with this change an a refactoring of request attributes usage.

teohhanhui · 2016-07-05T04:34:11Z

src/JsonLd/Serializer/JsonLdEncoder.php

 * @author Kévin Dunglas <dunglas@gmail.com>
 */
-final class JsonLdEncoder extends JsonEncoder
+final class JsonLdEncoder implements EncoderInterface, DecoderInterface


Why can't we use JsonEncoder directly for JSON-LD? There is no special handling whatsoever required.

Because we need to override supports* method and we tweak the options passed to JSON_ENCODE for RFC compliance.

Because we need to override supports* method

I don't quite understand that. Why can't we just set $format to json? Is there any benefit of using a different jsonld format?

we tweak the options passed to JSON_ENCODE for RFC compliance

I thought that's what $context['json_encode_options'] is for?

We need to use jsonld as format to trigger JSON-LD normalizers when we call $serializer->serialize(). An encore must support the jsonld format.

dunglas · 2016-07-06T06:43:17Z

@api-platform/core-team OK to merge it right now? I'll open another PR following this one trying to move the item data provider call, the deserializer listener and the validator listener to the kernel.controller event as smartly suggested by @teohhanhui. It will be easier if this PR is merged.

Simperfit · 2016-07-06T06:49:32Z

This is working for us and its LGTM. So 👍

…ener Refactor the responder to ease usage of content negotiation

dunglas added the enhancement label Jul 2, 2016

dunglas force-pushed the serializer_listener branch from 417b4a1 to d413c8e Compare July 2, 2016 14:37

Simperfit reviewed Jul 2, 2016
View reviewed changes

dunglas force-pushed the serializer_listener branch 4 times, most recently from 06db102 to 8084ee8 Compare July 3, 2016 08:55

Simperfit reviewed Jul 4, 2016
View reviewed changes

dunglas mentioned this pull request Jul 4, 2016

feat: swagger v2 #591

Merged

3 tasks

dunglas force-pushed the serializer_listener branch 3 times, most recently from 200eaeb to e645c28 Compare July 4, 2016 15:43

dunglas added 7 commits July 4, 2016 17:44

Refactor the responder to ease usage of content negotiation

7d431f5

Add tests for Responder and Serializer view listeners

5a096ab

Use composition for the JsonLdEncoder

9b58ec5

Rename the supported_formats config key to formats

0695a9c

Extract context generation

22cf383

Improve the overrode operation feature

854c305

Fix test CS

d03c629

dunglas force-pushed the serializer_listener branch from e645c28 to ead8c9d Compare July 4, 2016 15:44

theofidry reviewed Jul 4, 2016
View reviewed changes

dunglas added 2 commits July 4, 2016 21:41

Test that overriden operations are supported by content negotiation

9d67c78

Refactor the Serializer context building process

2bc1754

teohhanhui reviewed Jul 5, 2016
View reviewed changes

dunglas force-pushed the serializer_listener branch from ead8c9d to f095a3b Compare July 6, 2016 06:39

dunglas force-pushed the serializer_listener branch from f095a3b to 00e7221 Compare July 6, 2016 06:44

Refactor RequestAttributesExtractor to use explicit keys

373af5b

dunglas force-pushed the serializer_listener branch from 00e7221 to 373af5b Compare July 6, 2016 06:45

dunglas merged commit 2d972c0 into master Jul 6, 2016

dunglas deleted the serializer_listener branch July 6, 2016 16:42

magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017

Merge pull request api-platform#590 from api-platform/serializer_list…

dba8cad

…ener Refactor the responder to ease usage of content negotiation

		@@ -1,14 +1,14 @@
		Feature: Create-Retrieve-Update-Delete with custom attribute
		Feature: Create-Retrieve-Update-Delete with a Overrode Operation context

Uh oh!

Refactor the responder to ease usage of content negotiation #590

Refactor the responder to ease usage of content negotiation #590

Uh oh!

Conversation

dunglas commented Jul 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Simperfit Jul 2, 2016 • edited by dunglas 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!

Choose a reason for hiding this comment

Uh oh!

dunglas Jul 3, 2016 • 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!

Simperfit Jul 3, 2016 • edited by theofidry 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!

Choose a reason for hiding this comment

Uh oh!

Simperfit commented Jul 4, 2016

Uh oh!

dunglas commented Jul 4, 2016 • 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!

theofidry Jul 4, 2016 • 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 commented Jul 4, 2016 • 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!

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!

dunglas commented Jul 6, 2016

Uh oh!

Simperfit commented Jul 6, 2016 • 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!

dunglas commented Jul 2, 2016 •

edited

Loading

Simperfit Jul 2, 2016 •

edited by dunglas

Loading

dunglas Jul 3, 2016 •

edited

Loading

Simperfit Jul 3, 2016 •

edited by theofidry

Loading

dunglas commented Jul 4, 2016 •

edited

Loading

theofidry Jul 4, 2016 •

edited

Loading

dunglas commented Jul 4, 2016 •

edited

Loading

Simperfit commented Jul 6, 2016 •

edited

Loading