JSON:API returns a CacheableResponseInterface instance for non-cacheable methods; causes unnecessary exceptions.

Thank you for the excellent bug report @logickal! @logickal++

Obviously, it would seem that the real issue might indeed be Views (why render the whole view like this for field validation?)

I agree that this is probably the root cause of the bug.

...or even in the caching system as a whole (why hit these cache problems when doing a PATCH?) (emphasis added)

I completely agree that this is WTF bug. There's no good reason to unnecessarily fail on an uncacheable response. I see two ways to handle it:

1. Create a child class of Drupa\jsonapi\ResourceResponse called CacheableResourceResponse and only use it for actually cacheable responses
2. Find where the 500 error is thrown elsewhere in core and only throw the response-ruining exception for cacheable HTTP methods.

I'll let @Wim Leers weigh in. He might have a third way.

Obviously, it would seem that the real issue might indeed be Views (why render the whole view like this for field validation?)

I want to agree this is problematic. But I don't think it is. For \Drupal\views\Plugin\EntityReferenceSelection\ViewsSelection::validateReferenceableEntities() to be able to validate, it needs to know which entities are listed by the view, so it needs to run the view.

1. Create a child class of Drupa\jsonapi\ResourceResponse called CacheableResourceResponse and only use it for actually cacheable responses

+1 :)

FYI: REST already does what you suggested and what I just +1'd:

• \Drupal\rest\ResourceResponse
• \Drupal\rest\ModifiedResourceResponse

This was implemented in #2626298: REST module must cache only responses to GET requests. We should essentially do the same for JSON:API.

Thanks for working on this, @logickal! 🙏

It looks like that patch includes lots of unrelated changes 😅

Thanks for this! This is 90% there. I have a few small nits, a couple remarks and one last medium-sized thing to add.

I think this patch covers a broader scope than the original issue summary and so I think we can clean it up a bit. Also, we should have a regression test to prove the bug we're fixing.

My first thought (IOW, it's off the cuff and if you have a better way, do it!) is to make a test module that implements hook_entity_presave() and hook_entity_predelete() and within those functions, we do something that'll bubble cacheability (like render a Drupal\Core\Url). Then, the regression test can assert 2xx level responses for HEAD|GET|POST|PATCH|DELETE. The tests should fail for everything except HEAD/GET before this patch is applied and should pass afterward.

🙏 I so appreciate your help @Logickal!

1. +++ b/core/modules/jsonapi/src/CacheableResourceResponse.php
   @@ -0,0 +1,28 @@
   + * Extends Resource Response with Cacheability.
   

   Nit: no need to capitalize anything after Extends in this sentence.

2. +++ b/core/modules/jsonapi/src/CacheableResourceResponse.php
   @@ -0,0 +1,28 @@
   + * to the REST module's Modified Resource Response.
   

   Same. This should be modified resource response or ModifiedResourceResponse without any spaces.

3. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -195,7 +196,7 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Ent
   -   * @return \Drupal\jsonapi\ResourceResponse
   +   * @return \Drupal\jsonapi\CacheableResourceResponse
   
   @@ -290,7 +290,7 @@ public function createIndividual(ResourceType $resource_type, Request $request)
   -   * @return \Drupal\jsonapi\ResourceResponse
   +   * @return \Drupal\jsonapi\CacheableResourceResponse
   
   @@ -491,7 +491,7 @@ protected function executeQueryInRenderContext(QueryInterface $query, CacheableM
   -   * @return \Drupal\jsonapi\ResourceResponse
   +   * @return \Drupal\jsonapi\CacheableResourceResponse
   
   @@ -569,7 +573,7 @@ public function getRelationship(ResourceType $resource_type, FieldableEntityInte
   -   * @return \Drupal\jsonapi\ResourceResponse
   +   * @return \Drupal\jsonapi\CacheableResourceResponse
   
   @@ -638,7 +642,7 @@ public function addToRelationshipData(ResourceType $resource_type, FieldableEnti
   -   * @return \Drupal\jsonapi\ResourceResponse
   +   * @return \Drupal\jsonapi\CacheableResourceResponse
   
   @@ -719,7 +723,7 @@ protected function doPatchMultipleRelationship(EntityInterface $entity, array $r
   -   * @return \Drupal\jsonapi\ResourceResponse
   +   * @return \Drupal\jsonapi\CacheableResourceResponse
   
   @@ -970,7 +974,7 @@ protected static function relationshipResponseRequiresBody(array $received_resou
   -   * @return \Drupal\jsonapi\ResourceResponse
   +   * @return \Drupal\jsonapi\CacheableResourceResponse
   

   These can all remain ResourceResponse since CacheableResourceResponse is inherited from ResourceResponse.

   Also, many of these renames are on methods that can only ever return instances of ResourceResponse. I'm guessing this is just a leftover from some automatic refactoring in your editor :)

4. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -520,7 +520,9 @@ function (EntityInterface $entity) {
   -    $response->addCacheableDependency($entity);
   +    if ($response instanceof CacheableResourceResponse) {
   +      $response->addCacheableDependency($entity);
   +    }
   
   @@ -553,7 +555,9 @@ public function getRelationship(ResourceType $resource_type, FieldableEntityInte
   -    $response->addCacheableDependency($entity);
   +    if ($response instanceof CacheableResourceResponse) {
   +      $response->addCacheableDependency($entity);
   +    }
   

   👌 Good catch, even though these are in "get" methods on the class, they can be called from non-cacheable methods too.

5. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -980,14 +984,22 @@ protected function buildWrappedResponse($data, Request $request, IncludedData $i
   +    if ($request->isMethod('GET') || $request->isMethod('HEAD')) {
   

   We should use $request->isMethodCacheable() here.

6. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -980,14 +984,22 @@ protected function buildWrappedResponse($data, Request $request, IncludedData $i
   +      $response->addCacheableDependency($cacheability);
   +
   +    }
   ...
   +      $response = new ResourceResponse($document, $response_code, $headers);
   +
   +    }
   

   Nit: there are some extra newlines here.

7. +++ b/core/modules/jsonapi/src/EventSubscriber/DefaultExceptionSubscriber.php
   @@ -58,7 +58,7 @@ public function onException(GetResponseForExceptionEvent $event) {
   -    $response = new ResourceResponse(new JsonApiDocumentTopLevel(new ErrorCollection([$exception]), new NullIncludedData(), new LinkCollection([])), $exception->getStatusCode(), $exception->getHeaders());
   +    $response = new CacheableResourceResponse(new JsonApiDocumentTopLevel(new ErrorCollection([$exception]), new NullIncludedData(), new LinkCollection([])), $exception->getStatusCode(), $exception->getHeaders());
   

   We can conditionally return a CacheableResourceResponse here with:

   if ($exception->getRequest()->isMethodCacheable()) {...}

8. +++ b/core/modules/jsonapi/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -120,10 +121,12 @@ protected function renderResponseBody(Request $request, ResourceResponse $respon
   -      assert($jsonapi_doc_object instanceof CacheableNormalization);
   -      $response->addCacheableDependency($jsonapi_doc_object);
   -      // Finally, encode the normalized data (JSON:API's encoder rasterizes it
   -      // automatically).
   +      if ($response instanceof CacheableResourceResponse) {
   +        assert($jsonapi_doc_object instanceof CacheableNormalization);
   

   It's a little odd, but let's keep the assert outside of the conditional block. The assert is here not to validate that the normalization is safe to be passed to addCacheableDependency, but rather to enforce the contract that JSON:API serializer is only allowed to return CacheableNormalization objects.

9. +++ b/core/modules/jsonapi/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -120,10 +121,12 @@ protected function renderResponseBody(Request $request, ResourceResponse $respon
   +        // Finally, encode the normalized data (JSON:API's encoder rasterizes it
   +        // automatically).
   +      }
   

   Nit: let's leave this comment outside of the conditional.

+++ b/core/modules/jsonapi/src/EventSubscriber/DefaultExceptionSubscriber.php
@@ -58,7 +59,7 @@ public function onException(GetResponseForExceptionEvent $event) {
-    $response = new ResourceResponse(new JsonApiDocumentTopLevel(new ErrorCollection([$exception]), new NullIncludedData(), new LinkCollection([])), $exception->getStatusCode(), $exception->getHeaders());
+    $response = new CacheableResourceResponse(new JsonApiDocumentTopLevel(new ErrorCollection([$exception]), new NullIncludedData(), new LinkCollection([])), $exception->getStatusCode(), $exception->getHeaders());

#18.7: 🙈 my bad! I pointed you in the wrong direction. GetResponseForExceptionEvent has a getRequest method. My example should have been $event->getRequest() not $exception->getRequest(). Fixed.

Other than that, I think everything else is good to go once we have a test. Fantastic work @logickal!

Thanks for pushing this forward! 👏

+++ b/core/modules/jsonapi/src/CacheableResourceResponse.php
@@ -0,0 +1,28 @@
+class CacheableResourceResponse extends ResourceResponse implements CacheableResponseInterface {

1. \Drupal\jsonapi\ResourceResponse already does class ResourceResponse extends Response implements CacheableResponseInterface.
2. I would understand this if it were also changing ResourceResponse to no longer implement this interface.
3. Right now, it's confusing that both the non-cacheable resource response and the cacheable one both implement this interface.

D'oh. I'd swear dreditor wasn't showing this?! 😨Can't be. My bad! Please ignore all of #22 😅 I agree the key remaining thing is test coverage then!

+++ b/core/modules/jsonapi/src/EventSubscriber/ResourceResponseSubscriber.php
@@ -121,9 +122,11 @@ protected function renderResponseBody(Request $request, ResourceResponse $respon
+      if ($response instanceof CacheableResourceResponse) {

Nit: why hardcode this to the concrete class instead of checking the interface? \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber::flattenResponse() also doesn't do this.

@gabesullice I think this would also allow you to close #2794385: Node grants make it impossible to return a CacheableResponse from a controller? :)

WHOA, what a blast from the past! That should already have been closed by #2984964: JSON API + hook_node_grants() implementations: accessing /jsonapi/node/article as non-admin user results in a cacheability metadata leak. I went ahead and did that.

I just put this on a project and I think it's enough a WTF it would be great to get in sooner rather than later. Tagging for sprints at DrupalCon Amsterdam. Thanks everyone for your work on this! I was relieved to find it in the issue queue rather than scratching my head in bewilderment.

I modified this for a project running on 8.7.x and the patch in #21 just needed a slight update to work. I've attached what works for me on 8.7.8.

On it during contribution day at DrupalCon Amsterdam 2019. Trying to recreate the patch against the 8.9.x-dev version.

I will be helping with this issue during DrupalCon Amsterdam 2019 as mentor.

I refactored the patch against 8.9.x. Here it is.

The patch on #32 is working on 8.9.x, I've tested it.

retagging

The patch on #32 is working on 8.9.0-dev, I've tested it.

@nehiryeli Thank you very much for the work you have done.
I've seen that you did the steps to reproduce the issue, applied the patch and tested that it now works.

@cgoffin Thanks you very much for the rewriting of the patch. I see that it passes now :)

Rolled tests as per the outline in https://www.drupal.org/comment/13229447#comment-13229447 and noted above.

Coding standards in https://www.drupal.org/project/drupal/issues/3072076#comment-13431828, new patch.

Apologies, first Drupal core issue I've worked on so missed rolling a tests only patch. Included here with the complete to show the fail then succeed.

@JordanDukart Congrats and THANKS for your first core patch with a test!

I have a smal remark about the test. I'm not sure about the rest of the code, I don't know the JSON:API subsystem well enough.

1. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
   @@ -1240,4 +1240,67 @@ public function testAliasedFieldsWithVirtualRelationships() {
   +    $this->assertTrue($this->container->get('module_installer')->install([
   +      'jsonapi_test_non_cacheable_methods',
   +    ], TRUE), 'Installed modules.');
   

   I don't think we need to assert that this returns true. We don't need to test the module installer here.

2. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
   @@ -1240,4 +1240,67 @@ public function testAliasedFieldsWithVirtualRelationships() {
   +    $methods = [
   +      'HEAD',
   +      'GET',
   +      'PATCH',
   +      'DELETE',
   +    ];
   ...
   +    foreach ($methods as $method) {
   +      $response = $this->request($method, Url::fromUri('internal:/jsonapi/node/article/' . $node->uuid()), $non_update_delete_request_options);
   +      $this->assertSame($method === 'DELETE' ? 204 : 200, $response->getStatusCode());
   +    }
   

   This looks like it could benefit from using a @dataProvider instead of doing it like this.

   This adds too much logic to the test class I think.

As I mentioned before this my first core patch, and into testing with 8 at least. The asserting on the module install is all over the existing regression tests which I was cribbing from so that is easily removed.

Haven't explored @dataProvider too much. If I'm understanding correctly you are suggesting providing a separate test function with that annotation to feed in a method and its response code?

Yeah, but as I think about it more, it probably shouldn't do that, because this is a functional test and that would slow them down a lot, so all you'd need to change is the other remark.

Updated as per @borisson_'s comments in #49.

This looks great! Thank you @JordanDukart! Thanks for helping with the review @borisson_. I agree with your assessment about the data provider. I'm ambivalent about the $this->assertTrue() around the module installer. I think it's nice to have that there in case the module ever gets renamed or something. Then the test would fail with a really clear error message whereas it might be a little unclear otherwise.

I don't think that difference of opinion is worth holding this patch up though, so RTBC! Great work everyone!

1. +++ b/core/modules/jsonapi/src/CacheableResourceResponse.php
   @@ -0,0 +1,28 @@
   + * @see https://www.drupal.org/project/jsonapi/issues/3032787
   

   Not sure why we're pointing to the jsonapi project from core. I guess this is c&p from ResourceResponse - I think there should be a follow-up to address these links - they are all over jsonapi.

2. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -567,7 +569,9 @@ public function getRelationship(ResourceType $resource_type, FieldableEntityInte
   +    if ($response instanceof CacheableResourceResponse) {
   +      $response->addCacheableDependency($entity);
   +    }
   
   @@ -994,14 +998,20 @@ protected function buildWrappedResponse(TopLevelDataInterface $data, Request $re
   +    else {
   +      $response = new ResourceResponse($document, $response_code, $headers);
   +    }
   

   So what's interesting here is that ResourceResponse implements CacheableResponseInterface so this is pretty messy and we're not checking on the interface. Did we consider going the other way around? Ie creating an UncacheableResourceResponse? Or removing CacheableResponseInterface and the trait from ResourceResponse?

3. +++ b/core/modules/jsonapi/src/ResourceResponse.php
   @@ -2,8 +2,6 @@
   -use Drupal\Core\Cache\CacheableResponseInterface;
   -use Drupal\Core\Cache\CacheableResponseTrait;
    use Symfony\Component\HttpFoundation\Response;
    
    /**
   @@ -22,9 +20,7 @@
   
   @@ -22,9 +20,7 @@
     *
     * @see \Drupal\rest\ModifiedResourceResponse
     */
   -class ResourceResponse extends Response implements CacheableResponseInterface {
   -
   -  use CacheableResponseTrait;
   +class ResourceResponse extends Response {
   

   Ah I see we've done that. Nice. So the point above can be ignored but it'll be great to change conditions like f ($response instanceof CacheableResourceResponse) { to test against the interface and not the concrete class.

4. +++ b/core/modules/jsonapi/src/Controller/EntryPoint.php
   @@ -124,7 +124,7 @@ public function index() {
   -    $response = new ResourceResponse(new JsonApiDocumentTopLevel(new ResourceObjectData([]), new NullIncludedData(), $urls, $meta));
   +    $response = new CacheableResourceResponse(new JsonApiDocumentTopLevel(new ResourceObjectData([]), new NullIncludedData(), $urls, $meta));
        return $response->addCacheableDependency($cacheability);
   

   Yeah this route is only for GETs - $entry_point->setMethods(['GET']);. Looks good.

5. +++ b/core/modules/jsonapi/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -121,9 +122,11 @@ protected function renderResponseBody(Request $request, ResourceResponse $respon
          // Having just normalized the data, we can associate its cacheability with
          // the response object.
          assert($jsonapi_doc_object instanceof CacheableNormalization);
   -      $response->addCacheableDependency($jsonapi_doc_object);
          // Finally, encode the normalized data (JSON:API's encoder rasterizes it
          // automatically).
   +      if ($response instanceof CacheableResourceResponse) {
   +        $response->addCacheableDependency($jsonapi_doc_object);
   +      }
   

   The comment above the new code doesn't apply to it. Why has it moved position?

   Also I think maybe we only need to do the assert about CacheableNormalization if $response is a CacheableResourceResponse.

Will roll a patch to address the above.

Patch and interdiff attached.

Patch in #57 was incomplete. Re-attaching / running tests.

#55:

1. Good call. Done: #3122113: Convert all PHPDoc links targeting JSON:API contrib issues to target Drupal core issues.
2.
3. Good call. It looks like @JordanDukart has already done that too 👌
4. 👍
5. #57 addresses this too. Thanks @JordanDukart!

This was kicked back to needs work for an unrelated test failure. I queued a retest and it's green again. Back to RTBC :)

If you are still pulling your hair out after #60, you might need this https://www.drupal.org/project/jsonapi_earlyrendering_workaround

It's great to see all the collaboration on this issue and the patience over time.

The latest patch no longer applies to 9.1.x, so it needs a reroll. It also could use that IS update so that reviewers can follow more easily.

Saving issue credits, including for the mentor and participants who worked on the issue during the sprint. Usually we like to see comments that are a bit more detailed about how you review the issue, but since your mentor vouched for your work I'm adding credit. :)

Will roll the 9.1 patch, not entirely sure how the IS should be re-written but will bring up in the API-First meeting.

Missed the deprecation in https://www.drupal.org/node/3072702, another patch.

Garbled patch in 73, rectifying.

Thanks for all the effort and shepherding of this issue @JordanDukart! This patch looks perfect to me. I've updated the IS by providing more context and description of these changes.

Heh - I just used a screenshot of this issue on the contributions page for DrupalCon Global 2020, as an example of a real, live open issue. It looks might it might even be closed before the event - good going everyone!

1. +++ b/core/modules/jsonapi/src/CacheableResourceResponse.php
   @@ -0,0 +1,28 @@
   + * where the response is expected to be cacheable.  This approach is similar
   + * to the REST module's ModifiedResourceResponse.
   

   I'm not sure referencing what Rest module does is relevant enough to warrant a comment in the code that may need to be removed in the future, fair enough here on the issue, but in the code?

2. +++ b/core/modules/jsonapi/src/CacheableResourceResponse.php
   @@ -0,0 +1,28 @@
   + * @see https://www.drupal.org/project/jsonapi/issues/3032787
   

   alexpott pointed this out earlier too - why are we linking to an issue in json api module?

3. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -994,14 +999,20 @@ protected function buildWrappedResponse(TopLevelDataInterface $data, Request $re
   +      $response->addCacheableDependency($cacheability);
   

   nit: we could return early here and avoid an else

4. +++ b/core/modules/jsonapi/src/ResourceResponse.php
   @@ -22,9 +20,7 @@
   +class ResourceResponse extends Response {
   

   this is a BC break - there is possibly downstream code that assumes the previous interface was implemented I think we'd need to do the opposite, i.e. keep the existing behaviour but add a uncacheable flavour

   I realise alex also flagged this above and was OK with it, but I'm not sure its the right approach for BC, will ping him to discuss

   None of the other feedback warrants a new patch, so just putting to needs review till I can discuss with him

Re #81.4 The class has the following documentation - (dreditor-- for lacking context).

 * @internal JSON:API maintains no PHP API. The API is the HTTP API. This class
 *   may change at any time and could break any dependencies on it.

It was added to core with this documentation

I think we should only make this change in minor but I think we can make the change this way otherwise our documentation is not true.

Thanks Alex, I'd missed that.

This is 9.1 only then

Can we get a release notes snippet and change notice here

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

+1 to the CR

Will handle #81.1-3.

RE: #81.1
Will remove this reference.

#81.2
@alexpott pointed this out above and it spawned https://www.drupal.org/project/drupal/issues/3122113. Will update the referenced links in this patch to point to Drupal.

#81.3
Will return early.

Interdiff + patch attached. Re-rolled some things as my previous patch in #74 no longer applied cleanly in 9.1.x see: https://www.drupal.org/project/drupal/issues/3133033 and https://www.drupal.org/project/drupal/issues/3138766.

Thanks for addressing #81 @JordanDukart! This looks good to me.

Unrelated test fail.

Another unrelated test fail.

Committed 83c181f and pushed to 9.1.x. Thanks!

Added the release note.