JSON API module must not send cacheable responses to PATCH, POST and DELETE requests

I agree :) My issue summary was very very very brief, too brief, and hence probably confusing.

HEAD === body-less GET. A HEAD request is treated the same as a GET request by Drupal, including Page Cache.

Both a HEAD request or a GET request will result in the full GET response being stored in Page Cache. Symfony strips the response body at the last moment.

The REST module's integration test coverage even explicitly tests this in detail, because HEAD requests are important for API-first use cases:

    // 200 for well-formed HEAD request.
    $response = $this->request('HEAD', $url, $request_options);
    $this->assertResourceResponse(200, '', $response, $this->getExpectedCacheTags(), $this->getExpectedCacheContexts(), static::$auth ? FALSE : 'MISS', 'MISS');
    $head_headers = $response->getHeaders();

    // 200 for well-formed GET request. Page Cache hit because of HEAD request.
    // Same for Dynamic Page Cache hit.
    $response = $this->request('GET', $url, $request_options);
    $this->assertResourceResponse(200, FALSE, $response, $this->getExpectedCacheTags(), $this->getExpectedCacheContexts(), static::$auth ? FALSE : 'HIT', static::$auth ? 'HIT' : 'MISS');

…

    // Verify that the GET and HEAD responses are the same. The only difference
    // is that there's no body. For this reason the 'Transfer-Encoding' and
    // 'Vary' headers are also added to the list of headers to ignore, as they
    // may be added to GET requests, depending on web server configuration. They
    // are usually 'Transfer-Encoding: chunked' and 'Vary: Accept-Encoding'.
    $ignored_headers = ['Date', 'Content-Length', 'X-Drupal-Cache', 'X-Drupal-Dynamic-Cache', 'Transfer-Encoding', 'Vary'];
    $header_cleaner = function ($headers) use ($ignored_headers) {
      foreach ($headers as $header => $value) {
        if (strpos($header, 'X-Drupal-Assertion-') === 0 || in_array($header, $ignored_headers)) {
          unset($headers[$header]);
        }
      }
      return $headers;
    };
    $get_headers = $header_cleaner($get_headers);
    $head_headers = $header_cleaner($head_headers);
    $this->assertSame($get_headers, $head_headers);

The port of this test coverage to JSON API in #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only already includes those same assertions:

    // 200 for well-formed HEAD request.
    $response = $this->request('HEAD', $url, $request_options);
    $this->assertResourceResponse(200, '', $response, $this->getExpectedCacheTags(), $this->getExpectedCacheContexts(), FALSE, 'MISS');
    $head_headers = $response->getHeaders();

    // 200 for well-formed GET request. Page Cache hit because of HEAD request.
    // Same for Dynamic Page Cache hit.
    $response = $this->request('GET', $url, $request_options);
    $this->assertResourceResponse(200, FALSE, $response, $this->getExpectedCacheTags(), $this->getExpectedCacheContexts(), FALSE, 'HIT');

…

    // Verify that the GET and HEAD responses are the same. The only difference
    // is that there's no body. For this reason the 'Transfer-Encoding' and
    // 'Vary' headers are also added to the list of headers to ignore, as they
    // may be added to GET requests, depending on web server configuration. They
    // are usually 'Transfer-Encoding: chunked' and 'Vary: Accept-Encoding'.
    $ignored_headers = ['Date', 'Content-Length', 'X-Drupal-Cache', 'X-Drupal-Dynamic-Cache', 'Transfer-Encoding', 'Vary'];
    $header_cleaner = function ($headers) use ($ignored_headers) {
      foreach ($headers as $header => $value) {
        if (strpos($header, 'X-Drupal-Assertion-') === 0 || in_array($header, $ignored_headers)) {
          unset($headers[$header]);
        }
      }
      return $headers;
    };
    $get_headers = $header_cleaner($get_headers);
    $head_headers = $header_cleaner($head_headers);
    $this->assertSame($get_headers, $head_headers);

It's that test coverage that made me notice that JSON API did not yet support HEAD requests.

And it's related test coverage that made me discover that JSON API's responses to PATCH and POST requests are cacheable responses (which is why they contain cache tags and contexts even though they should not). That is what this issue is about.

My bad!

Actually, #2883086-17: [PP-1] Port RequestHandler + ResourceResponseSubscriber improvements from REST module to JSON API would also fix this! This is pretty much a subset of that issue.

#2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only and #2932031: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases landed, now we can do this with very strong assurances we don't break anything!

Fixing this results in the minimal set of changes that makes all cacheability-related assertions pass that #2930028 and #2932031 added, but needed to provide work-arounds for.

Green on both 8.4 and 8.6!

+++ b/tests/src/Functional/ResourceTestBase.php
@@ -306,8 +306,7 @@ abstract class ResourceTestBase extends BrowserTestBase {
-      // @todo Figure out why this is no longer among the response's cache contexts.
-      // 'user.permissions',
+      'user.permissions',

@@ -1187,8 +1186,7 @@ abstract class ResourceTestBase extends BrowserTestBase {
-    // @todo investigate this more (cache tags + contexts), cfr https://www.drupal.org/project/drupal/issues/2626298 + https://www.drupal.org/project/jsonapi/issues/2933939
-    $this->assertResourceResponse(200, FALSE, $response, Cache::mergeTags(['http_response', $this->entity->getCacheTags()[0]], []), $this->getExpectedCacheContexts());
+    $this->assertResourceResponse(200, FALSE, $response);

+++ b/tests/src/Functional/TermTest.php
@@ -327,8 +327,7 @@ class TermTest extends ResourceTestBase {
-    // @todo investigate this more (cache tags + contexts), cfr https://www.drupal.org/project/drupal/issues/2626298 + https://www.drupal.org/project/jsonapi/issues/2933939
-    $this->assertResourceResponse(200, FALSE, $response, ['http_response', 'taxonomy_term:1'], $this->getExpectedCacheContexts());
+    $this->assertResourceResponse(200, FALSE, $response);

These (and many more like those last two) are the work-arounds that the test coverage issues were forced to introduce in order to have passing tests, and which is what this issue is fixing. This issue is single-handedly fixing all cacheability issues!