deep_object_to_populate: true causes failure to update existing subresources

[rimas-kudelis]

When executing a partial update, and subresources are linked by their IRIs in the update payload, API Platform will fail to update them and instead attempt to create new ones.

This happens when deep_object_to_populate is true in the deserialization context, which is the case for all PATCH requests by default.

---

The long story:

While working on Sylius/Sylius#13441, I discovered that you can't just exchange PUT requests for PATCH ones at the moment, because even if you take care to change request Content Type, a PUT request like this will work, but a PATCH request will fail:

PUT|PATCH /api/v2/admin/product-variants/product1

{
    "channelPricings": {
        "default": {
            "@id": "/api/v2/admin/channel-pricings/1",
            "price": 3000,
            "originalPrice": 4000
        }
    }
}

This key here is that a related (ToMany) resource is being updated. By tracing these otherwise identical PUT and PATCH request, I stumbled upon this piece of code which appears to trigger a bug:

core/src/Serializer/SerializerContextBuilder.php

Lines 83 to 85 in 119b05b

	if ($context['api_allow_update'] && 'PATCH' === $method) {
	$context['deep_object_to_populate'] = $context['deep_object_to_populate'] ?? true;
	}

The actual bug, I think is elsewhere, but this is the snowball which causes it later.

When deep_object_to_populate is true, Symfony Serializer adds the object to populate (the collection in this case) to the attribute deserialization context.

...which later on causes JSON-LD normalizer to not fill in this context key with the correct object (using the IRI), instead retaining the collection which is already there:

core/src/JsonLd/Serializer/ItemNormalizer.php

Lines 139 to 151 in 119b05b

	public function denormalize($data, $class, $format = null, array $context = [])
	{
	// Avoid issues with proxies if we populated the object
	if (isset($data['@id']) && !isset($context[self::OBJECT_TO_POPULATE])) {
	if (true !== ($context['api_allow_update'] ?? true)) {
	throw new NotNormalizableValueException('Update is not allowed for this operation.');
	}
	$context[self::OBJECT_TO_POPULATE] = $this->iriConverter->getItemFromIri($data['@id'], $context + ['fetch_data' => true]);
	}
	return parent::denormalize($data, $class, $format, $context);
	}

Since that collection cannot be used as an object to populate the resource afterwards and extractObjectToPopulate() returns null, a new object is being created instead:

core/src/Serializer/AbstractItemNormalizer.php

Lines 226 to 229 in 119b05b

	if (null === $objectToPopulate = $this->extractObjectToPopulate($class, $context, static::OBJECT_TO_POPULATE)) {
	$normalizedData = is_scalar($data) ? [$data] : $this->prepareForDenormalization($data);
	$class = $this->getClassDiscriminatorResolvedClass($normalizedData, $class);
	}

I think there are two issues at hand here:

1. special processing for PATCH requests which triggers the bug for PATCH requests only.
2. when building a context to serialize a single item within a collection, object_to_populate (if it is set in the parent context) is neither cleared, nor updated, which is the root cause of the bug.

[dunglas]

Thank you for the in depth investigation!

Do you think you could work on a PR to fix these bugs?

[rimas-kudelis]

Hmm, not sure. My brain almost exploded just writing all this. I have a hunch that Symfony's default denormalizer should probably be used for collections (instead of building an array in AbstractItemNormalizer), but I have no idea why it isn't like that at the moment.

[rimas-kudelis]

@dunglas I could certainly make a PR removing the special case for PATCH from SerializerContextBuilder. My gut says $context['deep_object_to_populate'] should either always be set for updates, or never be set. It should not depend on the method. What do you think?

[dunglas]

I tend to agree but I need to double check... we can at least check if tests still pass if we remove this option for PATCH.

[usu]

Nice debug job @rimas-kudelis. I ran into the same issue but never got as far as you to identify the real root cause.

Issue #4293 already described the same problem, but unfortunately was closed earlier last year. In that issue, @mbrodala posted a workaround that kind of worked for us. But of course it would be so much nicer to have a proper solution directly in the core.

[rimas-kudelis]

@dunglas I wonder what your team's position on this will be.

After reading #4293, I tend to disagree with @soyuka on whether patch currently works as expected, particularly when patching collection items is involved. I strongly believe that it doesn't. The JSON Merge Patch RFC has no knowledge of JSON-LD, so of course it could not say that if an embedded item is referenced by an IRI, you have to update the embedded item. But JSON-LD says very clearly that IRIs are identifiers, so NOT updating an unambigously identified collection item seems to me like a wrong choice here.

Also, although this wasn't discussed in #4293, updating items in "associated" collections (in contrast to "indexed" collections ) should also work, because, at the very least, from the client's point of view, these collections are objects and not arrays.

Indexed collections, however, should lose all of their other members when patched. That is, if I had a resource like this:

{
  "@context": "/api/contexts/Person",
  "@id": "/api/people/1",
  "@type": "Person",
  "firstName": "John",
  "lastName": "Smith",
  "addresses": [
    {
      "@id": "/api/addresses/1",
      "@type": "Address",
      "city": "New York",
      "street": "736 Manor Station St.",
      "country": "USA"
    },
    {
      "@id": "/api/addresses/2",
      "@type": "Address",
      "city": "Boston",
      "street": "7 Denver St.",
      "country": "USA"
    }
  ]
}

... and I attempted to update it like this:

PATCH /api/people/1

{
  "addresses": [
    {
      "@id": "/api/addresses/1",
      "street": "63 Oak St."
    }
  ]
}

... the first address should be updated and the second one should be removed from the collection, resulting in an object like this:

{
 "@context": "/api/contexts/Person",
 "@id": "/api/people/1",
 "@type": "Person",
 "firstName": "John",
 "lastName": "Smith",
 "addresses": [
   {
     "@id": "/api/addresses/1",
     "@type": "Address",
     "city": "New York",
     "street": "63 Oak St.",
     "country": "USA"
   }
 ]
}

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[soyuka]

we need to get to the bottom of this eventually

[erikkubica]

Guys, can you please take a look at my last comment on a related issue? #4293 (comment)