Unable to update an embedded collection relation with a PATCH operation

[jonag]

API Platform version(s) affected: 3.1.8

Description

I'm trying to update an embedded relation with a PATCH operation. Basically this use case but with a PATCH instead of a PUT. However my relation is deleted and recreated instead of updated.

How to reproduce

1. Create a resource with a collection.
2. Try to update an item of this collection with a PATCH operation.

Possible Solution

I tried to debug the issue. I think the issue happens because when the request reach this line, $data['@id'] contains the id of my relation but $context[self::OBJECT_TO_POPULATE] is set and contains the entire collection instead of just the item.

If I update this line with

$childContext = $this->createChildContext($context, $attribute, $format);
$childContext[self::OBJECT_TO_POPULATE] = null;
$values[$index] = $this->denormalizeRelation($attribute, $propertyMetadata, $className, $obj, $format, $childContext);

then the request behave as expected, but I'm really not sure that this is the right fix.

Please tell me if I can do anything to help fix the issue (or if I'm doing anything wrong).

[soyuka]

Hi, merge-patch specification is like this see https://datatracker.ietf.org/doc/html/rfc7386 use put or help us implementing https://datatracker.ietf.org/doc/html/rfc6902 ?

[jonag]

Hello, thank you for your answer ! I've read the specification and while it states that the array will be replaced (which I'm totally fine with), it doesn't say that the members composing it should also be replaced (well actually this use case is not documented).

I don't know if my first message was detailed enough so here is exactly what I'm doing:

1. I have these two resources
2. I create a new Store with:

curl -X 'POST' \
  'http://localhost:8000/stores' \
  -H 'accept: application/ld+json' \
  -H 'Content-Type: application/ld+json' \
  -d '{
  "name": "My store",
  "pets": [
    {
      "name": "Bernie",
      "age": 2
    },
    {
      "name": "Felix",
      "age": 3
    }
  ]
}'

3. The store is created and the API responds with:

{
  "@context": "/contexts/Store",
  "@id": "/stores/1",
  "@type": "Store",
  "name": "My store",
  "pets": [
    {
      "@id": "/pets/1",
      "@type": "Pet",
      "name": "Bernie",
      "age": 2
    },
    {
      "@id": "/pets/2",
      "@type": "Pet",
      "name": "Felix",
      "age": 3
    }
  ]
}

4. I want to update the age of one of my pets with:

curl -X 'PATCH' \
  'http://localhost:8000/stores/1' \
  -H 'accept: application/ld+json' \
  -H 'Content-Type: application/ld+json' \
  -d '{
  "pets": [
    "/pets/1",
    {
      "@id": "/pets/2",
      "age": 4
    }
  ]
}'

5. The API throws an error 500 because it tries to create a new pet instead of updating the existing pet and so some of its properties are not set anymore:

{
  "@context": "/contexts/Error",
  "@type": "hydra:Error",
  "hydra:title": "An error occurred",
  "hydra:description": "An exception occurred while executing a query: SQLSTATE[23502]: Not null violation: 7 ERREUR:  une valeur NULL viole la contrainte NOT NULL de la colonne « name » dans la relation « pet »\nDETAIL:  La ligne en échec contient (3, 1, null, 3).",
  "trace": [...]
}

If I'm doing a PUT instead:

curl -X 'PUT' \
  'http://localhost:8000/stores/1' \
  -H 'accept: application/ld+json' \
  -H 'Content-Type: application/ld+json' \
  -d '{
  "pets": [
    "/pets/1",
    {
      "@id": "/pets/2",
      "age": 4
    }
  ]
}'

it works as expected:

{
  "@context": "/contexts/Store",
  "@id": "/stores/1",
  "@type": "Store",
  "name": "My store",
  "pets": [
    {
      "@id": "/pets/1",
      "@type": "Pet",
      "name": "Bernie",
      "age": 2
    },
    {
      "@id": "/pets/2",
      "@type": "Pet",
      "name": "Felix",
      "age": 4
    }
  ]
}

If the PATCH behaviour is intended (completely replacing the objects of the collection), why sending an existing IRI is allowed and the existing pet (/pet/1) reused? Shouldn't the API throws with a message saying that only nested documents are allowed?

I'm fine with using a PUT operation (actually this is what I'm doing until now on my projects) but since partial updates with PUT operations are now deprecated, I'm working on migrating to the proposed alternative (which is using a PATCH). But if not supporting this behaviour is intentional, the migration will be quite hard since asking the consummers of my API to switch from PUT to PATCH is reasonable, asking them to send the complete object (to stay with PUT) or updating each child separatly, will need a huge work from their part.

Anyway the choice is yours and I will adapt according to your decision on this subject.

[soyuka]

Keep using PUT ;). I don't think that we will deprecate the partial updates with put right away and even if we do it you'll be able to keep the behavior with a normalizer.

PATCH is doing partial updates and yes it reuses objects. But on collection it replaces the collection. There are other PATCH specifications that we could implement. For these use cases we also worked on #1645 but there are no perfect solutions.

[m-this]

Hello !

I'm facing the same issue using PATCH. I'm confused since partial updates with PUT seems already deprecated and the notice ask us to use PATCH to keep the same behaviour :

https://github.com/api-platform/core/blob/main/src/Metadata/Resource/Factory/DeprecationResourceMetadataCollectionFactory.php#L36

$this->triggerDeprecationOnce($operation, 'extraProperties["standard_put"]', 'In API Platform 4 PUT will always replace the data, use extraProperties["standard_put"] to "true" on every operation to avoid breaking PUT\'s behavior. Use PATCH to use the old behavior.');

It looks like it's not the case yet ?

[soyuka]

Replacing the data doesn't mean that we will change our behavior of reading the embedded values, not really sure what @dunglas has on its mind for that. What I can tell you for sure is that we want our users to use atomic resources and embrace Edge Side APIs : https://speakerdeck.com/dunglas/edge-side-apis.

Do your tests break with standard_put on ?

[jonag]

Do your tests break with standard_put on ?

If I set standard_put on the PUT operation of the resource Pet linked before and try to execute the cURl command "as-is", the request throws an error. Here is what it tries to do (I had to add a cascade persist on the relation Pet->Store, otherwise it crashed before, which is strange):

1. DELETE FROM pet WHERE store_id = 1;
2. INSERT INTO store (id, name) VALUES (2, NULL);

The second query fails because the column name is non-nullable. It seems that API Platform tries to create a completly new store with its pets, instead of replacing the existing one. I can upload a repository with my code if it can help better understand the issue.

[ilicmsreten]

@jonag had this problem #5093 and PR #5104, but is never merged!

@soyuka I rebased on 3.1.* #5577

[mrossard]

The second query fails because the column name is non-nullable. It seems that API Platform tries to create a completly new store with its pets, instead of replacing the existing one. I can upload a repository with my code if it can help better understand the issue.

Well that's exactly what a standard PUT is supposed to do : create or replace a resource entirely. Sending partial data for a standard PUT is incorrect and should trigger a validation error IMO..

[jonag]

Yes I know (I was only answering @soyuka question), and because non-standard PUT are now deprecated I'm trying to migrate to PATCH operations (the recommended solution) but it behaves almost the same way as a standard PUT with collections, hence the original issue.

@ilicmsreten indeed your issue is exactly the same as mine and your fix is quite similar as the one I found! I hope your PR can get merged...

[mrossard]

Yes I know (I was only answering @soyuka question), and because non-standard PUT are now deprecated I'm trying to migrate to PATCH operations (the recommended solution) but it behaves almost the same way as a standard PUT with collections, hence the original issue.

Indeed that's actually a BC break that can't be avoided IMO - PATCH or (standard) PUT on the resource shouldn't try to do a partial update on members of the resource, which is a problem if you're currently relying on non-standard PUT behaviour that does that.

[soyuka]

Can't you work on localhost:8000/stores/1/pets ?

[jonag]

I don't think it's possible to PATCH (nor PUT) multiple items through a subresource?

Even if it's possible, while I all-in with the concept of "edge-side APIs" to retrieve resources, I still think that not being able to update a resource with its children is a drawback (and the presentation by @dunglas don't really cover this point).

Le'ts take a more "real-life" example : a price condition (or a quotation price) has multiple lines. If we can't update the header and its lines in the same operation, here are some (real) use cases that will be hard to handle (without considering the need to rewrite all the code that uses the API):

• The header has a field (discount) whose validation depends of its lines : if we can't update both at the same time, the field can't be validated.
• If the header or one of its lines is updated, a message is dispatched and the whole price condition go through a validation process which can notify a validator if necessary : if we need to do multiple PATCH to update the price condition, the message will be dispatched multiple times, which will result on wasted CPU resource and multiple notification sent.
• If all updates are independent (and therefore not wrapped in a single transaction), a failure (or validation issue) with one of the updates will result on a price condition with an invalid state.

For all this reasons I don't understand what is the issue with @ilicmsreten's PR...

[mrossard]

@jonag Just my opinion, but in this case i'd expect a quotation resource containing multiple quotation lines (which have no need to be api resources - as far as i can see quotation lines only have meaning in the context of the quotation, exposing them directly doesn't seem logical to me).

You'd POST a quotation containing all the lines needed, and be able to PATCH it to change them, just like any other member of the resource. You'd probably need a custom processor, but that's to be expected with "complex" resources that need validation according to specific business rules.

As far as i can see, either the subresource has meaning without its "parent" and you want to use direct operations on them, or they only exist in the context of their parent, and you don't actually need/want to expose them as api resources. There could be some use cases I don't see yet, but in my experience when you try to do that you likely want to reevaluate what you expose in your API and see if there isn't a better way to achieve the desired functionnality.

[jonag]

If the lines are not API Resource then if I patch the quotation its lines are replaced with the one in the payload (the old one are deleted and new one are created) so we loose all the fields not sent in the payload. And what if the user calling the API doesn't have the permission to write all the fields? AFAIK this was until now the recommended way.

Regarding the solution to use a custom processor, since the ValidateListener is called before the WriteListener, it won't work because the quotation won't be correctly deserialized at the validation stage.

[mrossard]

If the lines are not API Resource then if I patch the quotation its lines are replaced with the one in the payload (the old one are deleted and new one are created) so we loose all the fields not sent in the payload.

As far as i understand it, that's what's expected from a standard PUT. A PATCH could (should?) be able to do a partial update on collections (not sure whether it's supposed to do that "deep" in the objects contained in the collection or if it's just changing "complete" objects in the collection though), but as @soyuka mentioned earlier it doesn't right now in api platform.

And what if the user calling the API doesn't have the permission to write all the fields?

Well in that case you need it to be a resource, but it definitely makes much more sense to use /quotations/{id}/lines/{lineid} to patch them than to try to allow that through /quotations/{id} !

Regarding the solution to use a custom processor, since the ValidateListener is called before the WriteListener, it won't work because the quotation won't be correctly deserialized at the validation stage.

You can customize the deserializer too, if needed...you got the point. ;-)