Return complete entity after successful PATCH

Makes sense, though a HTTP/REST expert should verify this is okay.

The sister issue #2546216: Return entity object in REST response body after successful POST landed. Let's get this done for 8.1 too!

Your code example looks good. Perhaps on top of that also do what #2546216: Return entity object in REST response body after successful POST did: verify the UUID is in the response.

Thanks for pushing this forward! :)

Next time please include the interdiff: https://www.drupal.org/documentation/git/interdiff :)

+++ b/core/modules/rest/src/Tests/UpdateTest.php
@@ -56,12 +56,19 @@ public function testPatchUpdate() {
+    $response = $this->httpRequest($entity->urlInfo(), 'PATCH', $serialized, $this->defaultMimeType);
...
+    // Assert that the response test_text field value has been updated.
+    $response = Json::decode($response);
+    $this->assertEqual($entity->field_test_text->value, $response['field_test_text'][0]['value'], 'Field was successfully updated (response).');
+
+    // Additionally check the entity UUID.
+    $this->assertEqual($entity->uuid->value, $response['uuid'][0]['value'], 'Updated entity response has correct uuid after successful PATCH over Rest API.');

The quoted changes are actually form the interdiff.

I think the updated test coverage is sufficient now. RTBC as far as I'm concerned!

I just would like sign-off of this from someone with deeper REST knowledge though. So not just yet RTBC'ing.

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -171,8 +171,8 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
-      return new ResourceResponse(NULL, 204);
...
+      return new ResourceResponse($original_entity, 200);

Is it a "api" break to change the status code? I guess noone really can actually rely on this behaviour?

#13: it's an API break to the same extent that we were returning an empty body before and now not anymore. HTTP required us to use 204 for HEAD's code, and with this change, it requires us to use 200.

A quote from https://tools.ietf.org/html/rfc5789

Successful PATCH response to existing text file:

HTTP/1.1 204 No Content
Content-Location: /file.txt
ETag: "e0023aa4f"

The 204 response code is used because the response does not carry a
message body (which a response with the 200 code would have). Note
that other success codes could be used as well.

Given that, the idea of the patch is perfect.

Excellent! Now we just need to get the patch green then.

There is a potential BC break when client code actually checks for this exact return code. On the other hand I think the risk is negligible.

Agreed. And usually it means they can remove an extra round trip.

That being said, this needs a CR, just like #2546216: Return entity object in REST response body after successful POST needed one: https://www.drupal.org/node/2665276.

CR created: https://www.drupal.org/node/2733655

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -171,8 +171,8 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +      // Update responses return complete resource.
   

   Let's make this comment be in line with the comment that #2546216: Return entity object in REST response body after successful POST added to ::post().

2. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -56,12 +56,19 @@ public function testPatchUpdate() {
   +    // Assert that the response test_text field value has been updated.
   +    $response = Json::decode($response);
   +    $this->assertEqual($entity->field_test_text->value, $response['field_test_text'][0]['value'], 'Field was successfully updated (response).');
   +
   

   The left-hand side of this assertion is wrong, which is why this test is failing. I think it's clearer to make this just like the assertion added in #2546216: Return entity object in REST response body after successful POST:

       $request = Json::decode($serialized);
       $response = Json::decode($response);
       $this->assertEqual($request['field_test_text'][0]['value'], $response['field_test_text'][0]['value']);
   

3. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -56,12 +56,19 @@ public function testPatchUpdate() {
   +    // Additionally check the entity UUID.
   +    $this->assertEqual($entity->uuid->value, $response['uuid'][0]['value'], 'Updated entity response has correct uuid after successful PATCH over Rest API.');
   

   I know I asked for the UUID to be checked because that's what #2546216: Return entity object in REST response body after successful POST tested. But that actually does not make a lot of sense: when POSTing (like in that other issue), a UUID is generated. When PATCHing (like here), no UUID is generated, it remains the same. So, it makes more sense to just verify that the updated field was actually updated. You're already testing that above.

4. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -56,12 +56,19 @@ public function testPatchUpdate() {
   -    $this->assertEqual($entity->field_test_text->value, $patch_entity->field_test_text->value, 'Field was successfully updated.');
   +    $this->assertEqual($entity->field_test_text->value, $patch_entity->field_test_text->value, 'Field was successfully updated (load).');
   

   Unnecessary change.

Addressed all my feedback.

I know I asked for the UUID to be checked because that's what #2546216: Return entity object in REST response body after successful POST tested. But that actually does not make a lot of sense: when POSTing (like in that other issue), a UUID is generated. When PATCHing (like here), no UUID is generated, it remains the same. So, it makes more sense to just verify that the updated field was actually updated. You're already testing that above.

IMHO we should have at least one test which tests the entire response, rather than just a small subset of it.

Fair!

First, a rebase, because #2626298: REST module must cache only responses to GET requests landed and conflicted with this.

So, there was one new PATCH assertion that is still causing that one fail in #25. This fixes that.

And now this morning #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it landed, which again expanded test coverage for PATCHing, and hence causes yet a different failure now, because this patch did not yet update it. This fixes that.

And this time the failure is due to details, and I cannot reproduce it locally :( Anyway, the failure is because #29 already tries to verify the entire response, like #23 requested. Because since #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it, we can verify the entire response not just for patching an entity_test entity, but also for a comment entity.

As #2631774-25: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it already indicated, there is extreme brittleness in JSON vs HAL+JSON, and this is just further proof of that.

So, let's revert most of #29's interdiff, and do the absolute minimum: verify that it's a 200 response now instead of a 204, and don't verify the entire response.

I added #31 to #2737719-2: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.

So, at long last, finally, this patch addresses #23!

#23: Done.

However, in doing so, I encountered yet another bug in REST:

    // All REST routes are restricted to exactly one format, so instead of
    // parsing it out of the Accept headers again, we can simply retrieve the
    // format requirement. If there is no format associated, just pick JSON.
    $format = $route_match->getRouteObject()->getRequirement('_format') ?: 'json';

That comment is plain wrong. We only have _format set for the GET route, not for the PATCH/POST routes (see \Drupal\rest\Plugin\ResourceBase::routes()). Which means that you ALWAYS get a json response for PATCH/POST, even if you ask xml or, like in this case, hal_json!!!!!!!! And this is why verifying the entire response doesn't work, because we don't get HAL+JSON back, but just JSON!
This code was always wrong: it would also not have worked in the days we were using Accept headers!
So, to fix that, I added a getResponseFormat() helper method to determine the format to use for the response.

After having done that, I encountered a new problem: RequestHandler::handle() currently always serializes the data, even if there isn't any (i.e. $data === NULL). In HEAD, and due to the code quoted above, which always defaults to 'json', the response to a DELETE request (which has no data to send in the response) will always end up serializing that NULL to the 'json' format. Which works (it results in nothing), but then with my fix above in place, no response format is determined (since the route for DELETE doesn't list any acceptable format, due to \Drupal\rest\Plugin\ResourceBase::routes()'s logic), which means it tries to serialize NULL to the '' format, which does not exist. However… why are we even trying to serialize NULL?! That makes no sense at all. So, this interdiff makes \Drupal\rest\RequestHandler::renderResponse() a little less braindead.

And from there, I found even more problems with REST's routing. Solving them all would be vastly out of scope here, so I opened #2737751: Refactor REST routing to address its brittleness for that.

+++ b/core/modules/rest/src/RequestHandler.php
@@ -105,6 +102,53 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
+   *
+   * Respects the requested format if one is specified. However, it is common to
+   * forget to specify a request format in case of a POST or PATCH. Rather than
+   * simply throwing an error, we apply the robustness principle: when POSTing
+   * or PATCHing using a certain format, you probably expect a response in that
+   * same format.
+   *

I'm wondering whether we could put that into the routing layer itself, rather than implementing a one-off implementation for REST.

#35: I agree with that in principle. Which points to the real problem: the REST module does not really use the routing system, or doesn't use it properly, and does much of its routing on its own, in its controller. See #2737751: Refactor REST routing to address its brittleness for fixing that. That is a much bigger undertaking. I'd much rather land this now, because it's a clear improvement over the current implementation, and then do #2737751 next.

There were 2 bugs in my code, and one significant bug in the existing test coverage.

Two bugs in my code:

1. getResponseFormat() did not yet handle _format and _content_type_format correctly in case they did not exist
2. handle()'s changes failed to deal correctly with NULL (and it was needlessly repetitive too)

The significant bug in the existing test coverage: '_format' => 'json' was being specified as a route default instead of a route requirement. Why did this go unnoticed? That's right… because of #33: because it just always picked JSON as the response format as the fallback…

One more edge case: DELETE routes don't have any specified format. It just used to always fall back to 'json', which is why it "worked" (as explained in #33). Thanks to the increased strictness of getResponseFormat(), we now discovered this edge case.

1. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -105,6 +102,58 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +  protected function getResponseFormat(RouteMatchInterface $route_match, Request $request) {
   +    $route = $route_match->getRouteObject();
   

   I was worried about heuristics for cacheable GET requests, but I think the code returns always the same as Content-Type doesn't vary itself.

2. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -105,6 +102,58 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +    // If an acceptable format is requested, then use that. Otherwise, including
   +    // and particularly when the client forgot to specify a format, then use
   +    // heuristics to select the format that is most likely expected.
   +    if (in_array($requested_format, $acceptable_request_formats)) {
   +      return $requested_format;
   +    }
   +    // If a request body is present, then use the format corresponding to the
   +    // request body's Content-Type for the response, if it's an acceptable
   +    // format for the request.
   +    elseif (!empty($request->getContent()) && in_array($content_type_format, $acceptable_content_type_formats)) {
   +      return $content_type_format;
   +    }
   +    // Otherwise, use the first acceptable format.
   +    else {
   +      if (!empty($acceptable_content_type_formats)) {
   +        return $acceptable_content_type_formats[0];
   +      }
   +      elseif (!empty($acceptable_request_formats)) {
   +        return $acceptable_request_formats[0];
   +      }
   +      // Sometimes, there are no acceptable formats, e.g. DELETE routes.
   +      else {
   +        return NULL;
   +      }
   

   I'm wondering whether we need tests for all these "new behaviour", or whether we have a lot of implicit tests already.

I'm wondering whether we need tests for all these "new behaviour", or whether we have a lot of implicit tests already.

We have a lot of implicit tests already. The fact that we don't need to modify any request URLs or request headers in tests is proof of that.

Given that we have a lot of implicit test coverage seems enough for me.

To add to #42: #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method will be where I exercise all these edge cases in complete test coverage: I'll mimic a developer trying things out.

Note: this blocks #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources, see #61 there.

1. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -105,6 +102,58 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +  protected function getResponseFormat(RouteMatchInterface $route_match, Request $request) {
   +    $route = $route_match->getRouteObject();
   +    $acceptable_request_formats = $route->hasRequirement('_format') ? explode('|', $route->getRequirement('_format')) : [];
   +    $acceptable_content_type_formats = $route->hasRequirement('_content_type_format') ? explode('|', $route->getRequirement('_content_type_format')) : [];
   +
   +    $requested_format = $request->getRequestFormat();
   +    $content_type_format = $request->getContentType();
   +
   +    // If an acceptable format is requested, then use that. Otherwise, including
   +    // and particularly when the client forgot to specify a format, then use
   +    // heuristics to select the format that is most likely expected.
   +    if (in_array($requested_format, $acceptable_request_formats)) {
   +      return $requested_format;
   +    }
   +    // If a request body is present, then use the format corresponding to the
   +    // request body's Content-Type for the response, if it's an acceptable
   +    // format for the request.
   +    elseif (!empty($request->getContent()) && in_array($content_type_format, $acceptable_content_type_formats)) {
   +      return $content_type_format;
   +    }
   +    // Otherwise, use the first acceptable format.
   +    else {
   +      if (!empty($acceptable_content_type_formats)) {
   +        return $acceptable_content_type_formats[0];
   +      }
   +      elseif (!empty($acceptable_request_formats)) {
   +        return $acceptable_request_formats[0];
   +      }
   +      // Sometimes, there are no acceptable formats, e.g. DELETE routes.
   +      else {
   +        return NULL;
   +      }
   +    }
   +  }
   

   Do we have full test coverage of the fallback ability of this method - I can't spot it in the test changes.

2. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -105,6 +102,58 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +    $acceptable_request_formats = $route->hasRequirement('_format') ? explode('|', $route->getRequirement('_format')) : [];
   +    $acceptable_content_type_formats = $route->hasRequirement('_content_type_format') ? explode('|', $route->getRequirement('_content_type_format')) : [];
   

   This can be simplified to

   $acceptable_request_formats = explode('|', $route->getRequirement('_format'));
   $acceptable_content_type_formats = explode('|', $route->getRequirement('_content_type_format'));
   

   If the requirement does not exist it returns NULL and explode will then just return an empty array as desired.

3. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -105,6 +102,58 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +        return NULL;
   

   Why don't we return 'json' as before?

1. Only implicit test coverage, see #41.2 and #42.
2. This is also what I expected, and what the code first looked like. Sadly, that's not how PHP behaves. Try this:
   

   $value = NULL;
   var_dump(explode('|', $value));
   

   It will output:

   array(1) {
     [0] =>
     string(0) ""
   }
   

3. Because then we're back to broken behavior. For DELETE, there is no response format, and there is no data to serialize, hence having NULL as the format is fine.
   

   +++ b/core/modules/rest/src/RequestHandler.php
   @@ -129,8 +178,9 @@ public function csrfToken() {
   -   * @param string $format
   -   *   The response format.
   +   * @param string|null $format
   +   *   The response format, or NULL in case the response does not need a format,
   +   *   for example for the response to a DELETE request.
   

   Hence this, among other related changes. See #33 for details.

Yes, these patches look confusing and less than ideal. But that's because they're trying to bring sanity to utterly broken code. For most components, I'd agree with you, because most components are in better shape.

So I think we should have explicit test coverage of the fallback as this is a logic change.

First rerolling. This conflicted with #2308745: Remove rest.settings.yml, use rest_resource config entities.

Next: the explicit test coverage @alexpott is asking.

Forgot the actual patch. Oops.

Let's be honest :)

Eh yes, that's what I'm working on — from #50:

Next: the explicit test coverage @alexpott is asking.

I forgot to update the issue status here. Sorry.

Turns out that in order to have sane testing, i.e. without having to set up a lot of normalizers, making the container aware of those and so on and so on, we need #2419825: Make serialization_class optional. Because that allows us to just send request bodies that don't need to be deserialized into an instance of a class. Because that'd be testing something we don't need to test when testing \Drupal\rest\RequestHandler::getResponseFormat. It'd make that test far more complex than necessary. We just need to test the response format. And #2419825: Make serialization_class optional is what allows this patch to test that in a far easier manner.

So… attaching a patch that includes #2419825 to prove that the test coverage works. And also attaching a do-not-test patch without it, because we should first land #2419825 separately.

You wanted tests? I'm glad you insisted, because this means at least this one low-level area of rest now has comprehensive, solid test coverage! I was also able to improve the code slightly in the process :)

Wow, #2419825: Make serialization_class optional already landed!

Here's a rerolled patch, with only one change, to address the new failure. This one change is necessary because \Drupal\Tests\rest\Kernel\RequestHandlerTest::testBaseHandler() was using the same Route object for both GET and PATCH requests, which is not how the REST module actually works. GET specifies a _format requirement, PATCH specifies a _content_type_format requirement. By correcting that, the tests pass again, as expected.

Note that #2419825 has been committed by Alex Pott, but not pushed. So this patch will fail. Once it's pushed, we can re-test it here, and it should pass :)

#2419825: Make serialization_class optional was pushed now, re-testing.

1. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -68,8 +68,20 @@ public function testPatchUpdate() {
   +    unset($request['_links']);
   +    unset($response['_links']);
   +    unset($response['id']);
   +    unset($response['uuid']);
   +    unset($response['name']);
   +    $this->assertEqual($request, $response);
    
   

   +1 for that

2. +++ b/core/modules/rest/tests/src/Kernel/RequestHandlerTest.php
   @@ -132,6 +137,211 @@ public function providerTestSerialization() {
   +    assert('\Drupal\Component\Assertion\Inspector::assertAllStrings($methods) && count($methods) > 0');
   +    assert('\Drupal\Component\Assertion\Inspector::assertAllStrings($supported_formats) && count($methods) > 0');
   +    assert('is_string($request_format) || $request_format === FALSE');
   +    assert('\Drupal\Component\Assertion\Inspector::assertAllStrings($request_headers)');
   +    assert('is_string($request_body) || is_null($request_body)');
   +    assert('is_string($expected_response_content_type) || is_null($expected_response_content_type)');
   +    assert('is_string($expected_response_content)');
   

   Honestly, its quite weird to test our tests. This is not really adding test coverage IMHO and on top is quite confusing

#61.2: Happy to remove those if you want. But time and time again I've seen future additions to data providers introduce more allowed types (start with only string, but then also allow FALSE, NULL, arrays), and that has caused subtle problems. That's why I added explicit assertions there.

If that's your biggest concern, then yay.

@dawehner Can you indicate what you think is blocking RTBC?

As we talked on IRC let's remove the assert() statements in core to not confuse people.

A patch removing assert statements per #64

Hmm that is very strange test to fail. Test is fine locally. Re-testing

Ok, not sure why that test failed first time. Back to Needs Review

dawehner RTBC'd in #43, alexpott un-RTBC'd asking for explicit test coverage, which I added in #55. @dawehner was +1 in #61, had only one minor remark that @tedbow addressed in #65.

So, back to RTBC.

This has been ready for a month. Let's get it in.

Committed 2bb0468 and pushed to 8.2.x. Thanks!

Yay! This means one blocker less for #2664780-62: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources!