Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it

Confirmed.

This is what I wrote on IRC, shortly after I discovered this issue:

 <WimLeers> klausi: neclimdul Crell Is there anybody who has *ever* successfully PATCHed a Comment entity? \Drupal\comment\CommentAccessControlHandler::checkFieldAccess() *forbids* 'update' access to 'comment_type' (the bundle field), yet \Drupal\serialization\Normalizer\EntityNormalizer::denormalize() *requires* the bundle field to be specified. These are opposite
18:01:19 <WimLeers> requirements, and so it's AFAICT impossible to update a Comment entity via REST :(

Had a first stab at updating the issue summary. Will try to push forward @tyler.frankenstein's proposed solution.

Option 0: the patch in #2

The patch doesn't work (tests fail), and only fixes the problem in the particular case of the author because the access restrictions are simply removed. That's not a proper solution.

Option 1: naïve

I would say we do something like this:

diff --git a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
index 4b4d027..856af6e 100644
--- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -144,7 +144,9 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
     }
 
     // Overwrite the received properties.
+    $entity_keys = array_values($entity->getEntityType()->getKeys());
     $langcode_key = $entity->getEntityType()->getKey('langcode');
+
     foreach ($entity->_restSubmittedFields as $field_name) {
       $field = $entity->get($field_name);
       // It is not possible to set the language to NULL as it is automatically
@@ -153,7 +155,7 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
         continue;
       }
 
-      if (!$original_entity->get($field_name)->access('edit')) {
+      if (in_array($field_name, $entity_keys) || !$original_entity->get($field_name)->access('edit')) {
         throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
       }
       $original_entity->set($field_name, $field->getValue());

… but that means we also won't get errors anymore when trying to change the UUID, i.e. no more errors like:

{"error":"Access denied on updating field 'uuid'."}

… even though you actually are changing the UUID field.

Option 2: generalize

But, really, we do something similar for the langcode field already. So why not simply generalize it? That would look like this:

diff --git a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
index 4b4d027..a13f469 100644
--- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -144,12 +144,12 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
     }
 
     // Overwrite the received properties.
-    $langcode_key = $entity->getEntityType()->getKey('langcode');
+    $entity_keys = array_values($entity->getEntityType()->getKeys());
+
     foreach ($entity->_restSubmittedFields as $field_name) {
       $field = $entity->get($field_name);
-      // It is not possible to set the language to NULL as it is automatically
-      // re-initialized. As it must not be empty, skip it if it is.
-      if ($field_name == $langcode_key && $field->isEmpty()) {
+      // None of an entity's keys can be changed when updating an entity.
+      if (in_array($field_name, $entity_keys)) {
         continue;
       }
 

But, the same problem: no complains when trying to change the UUID field. This time around, we are not actually changing the UUID field anymore though: any entity field that is also an entity key is simply ignored. This is also in compliance with the general rule Be liberal in what you accept: https://en.wikipedia.org/wiki/Robustness_principle.

Option 3: special case the bundle field just like we special case the langcode field

 core/modules/rest/src/Plugin/rest/resource/EntityResource.php | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
index 4b4d027..6e2ca6a 100644
--- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -145,6 +145,8 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
 
     // Overwrite the received properties.
     $langcode_key = $entity->getEntityType()->getKey('langcode');
+    $bundle_key = $entity->getEntityType()->getKey('langcode');
+
     foreach ($entity->_restSubmittedFields as $field_name) {
       $field = $entity->get($field_name);
       // It is not possible to set the language to NULL as it is automatically
@@ -152,6 +154,9 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
       if ($field_name == $langcode_key && $field->isEmpty()) {
         continue;
       }
+      if ($field_name == $bundle_key) {
+        continue;
+      }
 
       if (!$original_entity->get($field_name)->access('edit')) {
         throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");

Option 4: ?

I don't think there's a significantly different option? We can't modify \Drupal\serialization\Normalizer\EntityNormalizer::denormalize() to not need the bundle in any case.

Option 2 actually doesn't make sense, because e.g. entity label can definitely be updated for most entities, yet option 2 would no longer allow for that.

Given all that, I think something like option 3 makes most sense: it fulfills the requirements for the EntityNormalizer to work correctly, and gets in the way as little as possible! (It only gets in the way in that you won't be getting a {"error":"Access denied on updating field 'comment_type'."} error anymore, but that was a directly conflicting requirement anyway.)

So #8 allows you to update a comment (or any entity with a bundle field that you're not allowed to change). But I can confirm the problem that the original poster mentioned:

This allows the comment to be successfully updated via REST (including sending along the content_type value), but it throws a 500 error:

LogicException: The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\rest\ResourceResponse. in Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (line 159 of /var/www/discasaurus.com/drupal/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php).

This fixes that.

IS updated with the proposed solution.

Not every entity type has bundles.

#2405091: Cannot create user entities - {"error":"Access denied on creating field pass"} solved similar problems for creating entities. It also vastly expanded \Drupal\rest\Tests\CreateTest(), just like we should expand \Drupal\rest\Tests\UpdateTest here.

Re: Option 1
This was the approach I expected. But this code:

      if (in_array($field_name, $entity_keys) || !$original_entity->get($field_name)->access('edit')) {
         throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
       }

I assumed this would look like:

      if ((in_array($field_name, $entity_keys) && $original_entity->get($field_name) == $entity->get($field_name))
        || !$original_entity->get($field_name)->access('edit')) {
        
        throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
      }

This would still give us that error because if the values aren't the same we fall back on the access check.

That indeed makes sense. Let's do that.

There, did that. And since the special handling for the langcode field is just another special case of the more general entity key field special handling we're introducing here, we can (and should) refactor that while doing this, to keep the code understandable.

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
--- a/core/modules/rest/src/Tests/ResourceTest.php
+++ b/core/modules/rest/src/Tests/ResourceTest.php

Oops.

This snuck in from another issue. And it also caused the sole failure.

Manually edited patch.

Works for me. Preemptive RTBC assuming this would should actually come back green.

else if (isset($entity_keys['langcode']) && $field_name === $entity_keys['langcode'] && $field->isEmpty()) { + continue;
This will never evaluate TRUE because you now call array values on the keys

Also, why the rdf changes? I think they might break unsaved entity preview, there is a similar issue for users and terms #2314509: RDF module prevents viewing of unsaved users - might need a try/catch?

Why RDF changes: see IS, explained there. Does not break anything, we merely prevent URL generation from bubbling metadata.

#23: nice catch! Will fix that.

Finally wrote the test coverage I said we'd need in #8. We don't ever want to find out this PITA again!

In doing so, I also noticed that this problem only occurs for the JSON normalization/serialization, not for HAL+JSON! The fact that pretty much all our test coverage for REST is only testing HAL+JSON therefore does not help inspire confidence… Even worse: the $entity->set($field_name, NULL)-style removal of values that are read-only that the existing UpdateTest does only work for HAL+JSON! For JSON, we have to resort to array_diff_key(), because it's not actually possible to unset cid, for example. So, great, we'd standardize on array_diff_key(), right? Nope! Wrong again! Doesn't work because then we'll still end up with children of _links in HAL+JSON referring to fields we're not allowed to update. These differences are because HAL uses its own normalizer.
I've now got less faith in REST test coverage than I already had.

So, I present to you, a piece of very solid test coverage, that exposes this problem in the first place, but that also shows the crazy normalizer-dependent omissions you have to do when building PATCH requests. See \Drupal\rest\Tests\UpdateTest::assertPatchEntity().

No interdiff, because this is pure test coverage to prove that HEAD is broken. In the next reroll, I will add #21.

Also, please note the difference in read-only fields when comparing HAL+JSON and JSON:

HAL+JSON (read-only fields depend on hal module's normalizer)

+++ b/core/modules/rest/src/Tests/UpdateTest.php
@@ -223,7 +238,135 @@ public function testUpdateUser() {
+    $this->pass('Test case 1: PATCH comment using HAL+JSON.');
+    $comment->setSubject('Initial subject')->save();
+    $read_only_fields = [
+      'uuid',
+      'name',
+      'created',
+      'changed',
+      'status',
+      'thread',
+      'entity_type',
+      'field_name',
+      'entity_id',
+      'uid',
+    ];

JSON (read-only fields depend on serialization module's normalizer)

+++ b/core/modules/rest/src/Tests/UpdateTest.php
@@ -223,7 +238,135 @@ public function testUpdateUser() {
+    $this->pass('Test case 1: PATCH comment using JSON.');
+    $comment->setSubject('Initial subject')->save();
+    $read_only_fields = [
+      'cid', // Extra compared to HAL+JSON.
+      'uuid',
+      'pid', // Extra compared to HAL+JSON.
+      'entity_id',
+      'uid',
+      'name',
+      'homepage', // Extra compared to HAL+JSON.
+      'created',
+      'changed',
+      'status',
+      'thread',
+      'entity_type',
+      'field_name',
+    ];

So, here's the fix merged in again (i.e. #21 === interdiff.txt).

Note that because the test coverage is so comprehensive, this will fail!

The test coverage starts with the given entity, tries to PATCH it, but gets a 403 because it's trying to modify read-only fields. Then it removes the first item in the list of read-only fields, tries again. Now it gets a 403 for the next read-only field. And so on. So, we comprehensively test which fields you're allowed to modify. Should this list change, then that will be a significant difference for REST API users, so it's important that we do exercise that.

Now, the patch (#21) actually makes it so that any entity key can be sent without complaints. So we won't get an error anymore for the UUID and CID fields either.

This patch will be green.

It no longer removes entity key fields, because those are now safe to send (see the explanation in #27).

Just a quick driveby review.

1. +++ b/core/modules/rdf/rdf.module
   @@ -233,8 +233,9 @@ function rdf_comment_storage_load($comments) {
        $comment->rdf_data['date'] = rdf_rdfa_attributes($created_mapping, $comment->get('created')->first()->toArray());
   +    /** @var \Drupal\Core\Entity\EntityInterface $entity */
   

   Ideally we should typehint $comment, because then we don't have to typehint $entity, as its type follows automatically.

2. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -223,7 +238,132 @@ public function testUpdateUser() {
   +    // Create & save an entity to comment on, plus a comment.
   +    $node = Node::create([
   +      'type' => 'resttest',
   +      'title' => 'some node',
   +    ]);
   

   If possible it always seems better to test with the entity_test entity type and not a special snowflake like node.

1. Well-spotted! I didn't even realize that. :) Done.
2. This is what \Drupal\rest\Tests\RESTTestBase::setUp() and \Drupal\rest\Tests\CreateTest already do. But I think you're right of course. Fixed.

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -145,13 +145,21 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
+      // Entity key fields need special treatment.
+      if (in_array($field_name, $entity_keys)) {

Let's explain why. The code explains that they need special treatments already.

This is what \Drupal\rest\Tests\RESTTestBase::setUp() and \Drupal\rest\Tests\CreateTest already do. But I think you're right of course. Fixed.

No worries. I just try to push for not using node all the time, just because it seems to be more convenient.

Let's explain why. The code explains that they need special treatments already.

Done.

No worries. I just try to push for not using node all the time, just because it seems to be more convenient.

+1. Thanks for pushing for that :)

Triaging with valthebald.

Confirmed #34 applies and resolves the issue.

rtbc +1 - thanks all

1. +++ b/core/modules/rdf/rdf.module
   @@ -233,9 +233,10 @@ function rdf_comment_storage_load($comments) {
   -    $comment->rdf_data['entity_uri'] = $entity->url();
   +    $comment->rdf_data['entity_uri'] = $entity->toUrl()->toString(TRUE)->getGeneratedUrl();
   

   Why's this necessary? And what's the difference? Could use a comment to explain if it really is.

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -151,13 +151,24 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +    $entity_keys = array_values($entity->getEntityType()->getKeys());
   

   Why is the array_values() necessary?

3. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -151,13 +151,24 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +        // Unchanged Values for entity keys same don't need access checking.
   

   Should this be:

   'Unchanged values for entity keys don't need access checking'?

1) IMHO we need to fix #2626298: REST module must cache only responses to GET requests and then we don't need this change. In this hook (rdf_comment_storage_load()) we only need to obtain the url... and well, a patch request should not be cached anyway. I've tested manually applying that patch and the Exception disappears.

2) There's an empty value (see the attached image) and we only avoid to check some values. To be honest I'm not sure 100% if makes sense this option or maybe simply verify langcode + bundle_key.

3) Done.

1) IMHO we need to fix #2626298: REST module must cache only GET requests. and then we don't need this change. In this hook (rdf_comment_storage_load()) we only need to obtain the url... and well, a patch request should not be cached anyway. I've tested manually applying that patch and the Exception disappears.

Conceptually it would be still nice to do that. Giving good examples to people is the right thing to do. On the other hand your are right about that. The other patch fixes most proabably the problem.

For me this is ready for it.

+++ b/core/modules/rdf/rdf.module
@@ -233,9 +233,10 @@ function rdf_comment_storage_load($comments) {
-    $comment->rdf_data['entity_uri'] = $entity->url();
+    $comment->rdf_data['entity_uri'] = $entity->toUrl()->toString(TRUE)->getGeneratedUrl();

So given @catch's comment and the responses don't we need an @todo here?

What about adding something like
// We are in a storage load hook, which is outside of a render context, so we should avoid to bubble metadata up, which <code>$entity->url() would.
?

(Updating issue credit for the major triage sprint. Thanks @gabesullice for helping triage this!)

Was triaging the issue during DrupalCon NOLA Friday sprint with @gabesullice

(Added credit.)

Let's just fix that. I also changed the "assert" function to use a normal function. There is no clear abstraction happening in this function but rather a set of operations and some assertions in there.

Discussed with @alexpott, @catch, @xjm, @webchick, and @Cottser, and we agreed this is Major priority, so tagging. As per the recently updated bullet points in https://www.drupal.org/core/issue-priority#major-bug, this is a "feature unusable with no workaround".

I think all feedback has been addressed.

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -146,13 +146,24 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
+    $entity_keys = array_values($entity->getEntityType()->getKeys());
...
+      if (in_array($field_name, $entity_keys)) {

I still don't get the need for array_values() here. Since using it does not remove the empty value. If we want to do that we should use array_filter. Not sure it is worth it. Plus I'd change the in_array to use the strict comparison option just to be type safe.

+++ b/core/modules/rest/src/Tests/RESTTestBase.php
@@ -261,10 +261,15 @@ protected function enableService($resource_type, $method = 'GET', $format = NULL
+      if (is_array($format)) {
+        $settings[$resource_type][$method]['supported_formats'] = $format;
+      }

Do we need to protect against empty arrays here? And fallback to the default format if so?

#50: oops, sorry, I thought that was addressed. I looked at the various patches here. Turns out I proposed this in #7. AFAICT I did it originally just to stress we only want the entity keys for a particular entity type. But keeping the keys would be totally harmless:

$keys = ['id' => 'nid' , …];
array_values($keys) === ['nid', …];

So, +1 to remove array_values().

#51: the reason is that we want to keep BC for enableService() for custom/contrib tests. It used to only allow a single format to be passed in, and otherwise default to "the default format" (for a particular test). This patch is making it possible to specify multiple formats.
We don't need to protect against empty arrays: if somebody specifies that, then it's intentional (and probably wrong).
IOW: we don't need to babybsit crappy/broken tests, but we need to make sure we retain BC.

Fixed #50

Nice, better code!

I think this fix is BC compatible and therefore should be committed to 8.1.x as well since this is a major bug fix. Committed 829fe8a and pushed to 8.1.x and 8.2.x. Thanks!

diff --git a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
index d6fdcd1..63857e4 100644
--- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -161,7 +161,7 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
         }
         // It is not possible to set the language to NULL as it is automatically
         // re-initialized. As it must not be empty, skip it if it is.
-        else if (isset($entity_keys['langcode']) && $field_name === $entity_keys['langcode'] && $field->isEmpty()) {
+        elseif (isset($entity_keys['langcode']) && $field_name === $entity_keys['langcode'] && $field->isEmpty()) {
           continue;
         }
       }

Coding standards fix on commit.

YAY! Took almost 4 months, but so happy to finally see this in. This was a huge, show-stopping REST bug, IMO bordering on critical.

Thanks for also committing it to Drupal 8.1!

For the brittleness we saw here wrt formats and test coverage, as described in #25 in particular, I opened #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.

This sounds like it allows developers to use comments as first-class citizens, so worthy of release note mention.

Draft change record created.
Edit: I was following the guide and I'm unsure whether draft is correct when the patch is already merged.

Drupal 8 supports OAuth 2.0 as na authentication mechanism?

I just noticed the CR was never published. Just did that: https://www.drupal.org/node/2778563.

Proposed resolution

method: PATCH
URL: drupal8/comment/1
Content-Type: application/json

{ 
"comment_type":[{"target_id":"comment"}],
"uid":[{"target_id":7}], 
  "subject":[{"value":"Editado Comentario"}],
  "comment_body":[
    {"value":"<p>Comentario</p>","format":"basic_html"}
  ]
}

note: the user must have the administrator role