Add support for PATCH, refactor formats detection #2895

dunglas · 2019-07-01T20:06:27Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#759 (partially)
License	MIT
Doc PR	api-platform/docs#878

Add proper support for partial modifications using the PATCH HTTP method. The first supported format will be JSON Merge Patch (RFC 7386), while JSON:API Patch will keep working. Supporting JSON Patch (RFC 6902) isn't a a goal as this format is not very popular in the wild, but could be done by leveraging the new infrastructure I'm building.

This PR also deprecates FormatsProviderInterface and OperationMethodResolverInterface and all there implementation in favor of populating the appropriate metadata. This approach as several benefits:

simplification of the code base
better performance because we move some run time logic to compile time
better flexibility

TODO:

Fix remaining deprecations
Set Serializer's skip_null_values to true when appropriate (always when PATCH is enabled?)
Use patch formats in docs
Find a way to block partial PUT (I've no clue of how to that for now, but actually it could be some documentation telling that there is no guarantees than doing that will work).

src/Metadata/Resource/Factory/FormatsResourceMetadataFactory.php

dkarlovi · 2019-07-02T14:52:42Z

Supporting JSON Patch (RFC 6902) isn't a a goal as this format is not very popular in the wild

As noted before,

It solves the partial collection management problem well and IMO it should be kept as a future option.

So having it not be "popular" doesn't mean it might not become so if well supported here.

Not saying you should implement it in this PR, of course.

dunglas · 2019-07-02T15:12:10Z

So having it not be "popular" doesn't mean it might not become so if well supported here.

I'll be glad to merge support for it if someone contribute it (it's why the new system is designed to support several patch formats)! To clarify: it's not on my todo list (yet)!

src/EventListener/DeserializeListener.php

bendavies · 2019-07-08T12:11:40Z

Find a way to block partial PUT

how would you prevent a BC break here?
partial PUT is how we currently do PATCH of course. PUT with denormalization groups.

src/Serializer/SerializerContextBuilder.php

jewome62 · 2019-07-08T16:10:03Z

Do you think implement the Header Accept-Patch ?
https://tools.ietf.org/html/rfc5789#section-3.1

That's just the new patchFormat but this is only on OPTIONS request.
So that's managed by nelmio/cors-bundle ?

dunglas · 2019-07-08T17:26:57Z

yes it should be implemented in API Platform, but in a follow up PR (and maybe not in 2.5).

dunglas · 2019-07-09T13:32:57Z

how would you prevent a BC break here? partial PUT is how we currently do PATCH of course. PUT with denormalization groups.

It would be opt-in in 2.x, and opt-out in 3.x. But currently I struggle to find a god way to implement this: https://twitter.com/dunglas/status/1148578034787782656

teohhanhui · 2019-07-15T15:35:12Z

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

        $errorFormats = $this->getFormats($config['error_formats']);

-        $this->registerCommonConfiguration($container, $config, $loader, $formats, $errorFormats);
+        // Backward Compatibility layer
+        if (isset($formats['jsonapi']) && !isset($patchFormats['jsonapi'])) {


I really think we should deprecate specifying the format keys, and just use the mime type directly.

EDIT: Hmm... I see it's more complicated than that.

teohhanhui · 2019-07-15T15:36:44Z

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

@@ -163,6 +163,7 @@ public function getConfigTreeBuilder()
            'json' => ['mime_types' => ['application/json']], // Swagger support
            'html' => ['mime_types' => ['text/html']], // Swagger UI support
        ]);
+        $this->addFormatSection($rootNode, 'patch_formats', []);


This doesn't seem like a good default?

teohhanhui · 2019-07-15T15:43:15Z

src/EventListener/AddFormatListener.php

     */
-    public function __construct(Negotiator $negotiator, /* FormatsProviderInterface */ $formatsProvider)
+    public function __construct(Negotiator $negotiator, $resourceMetadataFactory, array $formats = [])


I think we need to do something like this in so many places. Otherwise we'll end up with lots of holes in our arguments in 3.0 😆

teohhanhui · 2019-07-15T15:44:26Z

src/EventListener/AddFormatListener.php

@@ -63,33 +65,54 @@ public function __construct(Negotiator $negotiator, /* FormatsProviderInterface
    public function onKernelRequest(GetResponseEvent $event): void
    {
        $request = $event->getRequest();
-        if (!($request->attributes->has('_api_resource_class') || $request->attributes->getBoolean('_api_respond', false) || $request->attributes->getBoolean('_graphql', false))) {
+        if (
+                !($request->attributes->has('_api_resource_class')


This is indented too much.

teohhanhui · 2019-07-15T15:48:46Z

src/EventListener/AddFormatListener.php

@@ -130,37 +153,29 @@ private function addRequestFormats(Request $request, array $formats): void
    }

    /**
-     * Populates the $mimeTypes property.
+     * Retries the flattened list of MIME types.


Retries -> Retrieves

teohhanhui · 2019-07-15T15:52:00Z

src/EventListener/DeserializeListener.php

     */
-    public function __construct(SerializerInterface $serializer, SerializerContextBuilderInterface $serializerContextBuilder, /* FormatsProviderInterface */$formatsProvider, ResourceMetadataFactoryInterface $resourceMetadataFactory = null)
+    public function __construct(SerializerInterface $serializer, SerializerContextBuilderInterface $serializerContextBuilder, $resourceMetadataFactory, ResourceMetadataFactoryInterface $legacyResourceMetadataFactory = null)


We should remove the typehint for $legacyResourceMetadataFactory, right? And check that in the constructor body?

dunglas requested review from teohhanhui and antograssiot July 1, 2019 20:06

dunglas added this to the 2.5.0 milestone Jul 1, 2019

antograssiot reviewed Jul 2, 2019

View reviewed changes

src/Metadata/Resource/Factory/FormatsResourceMetadataFactory.php Show resolved Hide resolved

dunglas force-pushed the json-merge-patch branch 2 times, most recently from 5e84a32 to f63b3b3 Compare July 5, 2019 14:47

bendavies reviewed Jul 6, 2019

View reviewed changes

src/EventListener/DeserializeListener.php Show resolved Hide resolved

dunglas force-pushed the json-merge-patch branch from 03bd7d9 to 5a61462 Compare July 8, 2019 07:50

bendavies reviewed Jul 8, 2019

View reviewed changes

src/Serializer/SerializerContextBuilder.php Show resolved Hide resolved

dunglas marked this pull request as ready for review July 9, 2019 13:30

dunglas and others added 15 commits July 9, 2019 15:31

Add support for PATCH, refactor formats detection

2b80cab

Migrate AddFormatListener

183d77d

Fix more deprecations

efb0474

Fix some more deprecations, and tests

9243895

Fix more deprecations

3455cf6

Fix all remaining deprecations

4fd4d73

Fix a Behat test

9f184e8

Fix PHPStan

522ac97

More deprecations

0387023

fix build

2fb54bd

Fix MongoDB test

614bcb7

Input / output formats

fb16244

Set skip_null_values to true by default if PATCH is enabled

833995c

Use in/out formats parameters

8a3df58

fix OperationResourceMetadataFactoryTest

43e7daf

dunglas added 5 commits July 9, 2019 15:31

Fix more tests

853d75a

Fix Behat tests

186b6dd

All green!

b38bba5

Add support for input_formats in DocumentationNormalizer

78c9a83

Last commit? :)

c760ad1

dunglas force-pushed the json-merge-patch branch from 9f8ec42 to c760ad1 Compare July 9, 2019 13:31

dunglas added 2 commits July 9, 2019 17:57

Remove dead code

4dc9da0

Add a unit test for FormatsResourceMetadataFactory

164600d

dunglas merged commit a62a077 into api-platform:master Jul 10, 2019

dunglas deleted the json-merge-patch branch July 10, 2019 16:41

teohhanhui reviewed Jul 15, 2019

View reviewed changes

teohhanhui mentioned this pull request Jul 18, 2019

How to add new entry in a ManyToMany? #1628

Closed

dunglas mentioned this pull request Sep 24, 2019

Add docs for PATCH, and various improvements api-platform/docs#878

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for PATCH, refactor formats detection #2895

Add support for PATCH, refactor formats detection #2895

dunglas commented Jul 1, 2019 •

edited

Loading

dkarlovi commented Jul 2, 2019

dunglas commented Jul 2, 2019

bendavies commented Jul 8, 2019 •

edited

Loading

jewome62 commented Jul 8, 2019

dunglas commented Jul 8, 2019

dunglas commented Jul 9, 2019

teohhanhui Jul 15, 2019 •

edited

Loading

teohhanhui Jul 15, 2019

teohhanhui Jul 15, 2019

teohhanhui Jul 15, 2019

teohhanhui Jul 15, 2019

teohhanhui Jul 15, 2019

Add support for PATCH, refactor formats detection #2895

Add support for PATCH, refactor formats detection #2895

Conversation

dunglas commented Jul 1, 2019 • edited Loading

dkarlovi commented Jul 2, 2019

dunglas commented Jul 2, 2019

bendavies commented Jul 8, 2019 • edited Loading

jewome62 commented Jul 8, 2019

dunglas commented Jul 8, 2019

dunglas commented Jul 9, 2019

teohhanhui Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

teohhanhui Jul 15, 2019

Choose a reason for hiding this comment

teohhanhui Jul 15, 2019

Choose a reason for hiding this comment

teohhanhui Jul 15, 2019

Choose a reason for hiding this comment

teohhanhui Jul 15, 2019

Choose a reason for hiding this comment

teohhanhui Jul 15, 2019

Choose a reason for hiding this comment

dunglas commented Jul 1, 2019 •

edited

Loading

bendavies commented Jul 8, 2019 •

edited

Loading

teohhanhui Jul 15, 2019 •

edited

Loading