Problem/Motivation

All pages under the 'jsonapi/*' routes that contain nodes with default includes are not cached. This is because the default include is added dynamically in the 'Drupal\jsonapi_defaults\Controller\EntityResource' dynamic route controller service. The 'getIncludes' method executes after the cache is retrieved from the database, resulting in the cache cid being generated without default includes during the cache retrieval process.

On the other hand, 'getIncludes' executes before the cache is set to the database, and during this phase, the cache cid is generated using default includes added to the request. Consequently, we have a cache stored in the database using the default includes context. However, Drupal always attempts to retrieve the cache with a different cid, one that was generated without the default includes context.

As a result, the only way to obtain a cached response is by adding default includes explicitly as get parameters. This is because the cache stored in the database is under a cid generated using these includes added dynamically as default includes. Consequently, we consistently receive an uncached response for pages with default includes.

Steps to reproduce

  1. Add default includes to any content type
  2. Create a page of this content type
  3. Visit this page using the jsonapi/* route, for instance, jsonapi/node/article/324f078e-e3a0-4e3a-b902-0a76f072bdd2
  4. Check the cache header in the response
  5. Reload the page
  6. Check the cache header in the response

Actual result: After the first visit to the page, the cache status is marked as "MISS" and after reloading the page, the cache status remains "MISS."

Expected result: Following the initial visit to the page, the cache status should be "MISS," but after reloading the page, the cache status is expected to change to "HIT."

Proposed resolution

Override the core QueryArgsCacheContext (cache_context.url.query_args) service to disregard all default includes arguments. Consequently, for all requests to resources with default includes, the cache will be stored under the cid generated without using the default includes context. This modification enables us to obtain cached responses for all resources with default includes.

CommentFileSizeAuthor
#27 3402388-27.patch21.82 KB_tarik_
#26 3402388-31.patch21.64 KB_tarik_
#16 3402388-default-includes-caching-fix-15.patch22.48 KBihor_allin
#15 3402388-default-includes-caching-fix-14.patch22.34 KBihor_allin
#13 3402388-default-includes-caching-fix-13.patch19.83 KBihor_allin
#12 3402388-default-includes-caching-fix-12.patch19.79 KBihor_allin
#7 3402388-default-includes-caching-fix-7.patch19.8 KBihor_allin
#4 3402388-default-includes-caching-fix-3.23.patch18.41 KBihor_allin
#2 3402388-default-includes-caching-fix.patch18.38 KBihor_allin
Show 4 more files

Issue fork jsonapi_extras-3402388

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ihor_allin created an issue. See original summary.

ihor_allin’s picture

ihor_allin CreditAttribution: ihor_allin commented
StatusFileSize
new 3402388-default-includes-caching-fix.patch18.38 KB
bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented

Great work, that will save quite some request/cpu time.

Any change you could add a line to the tests for that module? I think using:

$this->assertSession()->responseHeaderContains() should be usefull for that to see if the cache has been hit if a second request is done.

ihor_allin’s picture

ihor_allin CreditAttribution: ihor_allin commented
StatusFileSize
new 3402388-default-includes-caching-fix-3.23.patch18.41 KB
ihor_allin’s picture

ihor_allin CreditAttribution: ihor_allin commented

It seems that the responses in these tests do not contain headers, just a raw JSON string.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented

You need to use:

    $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'MISS');

    // 1a. Check if second request is cached.
    $response = $this->drupalGet('/api/articles');
    $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'HIT');

Or something similar. When looking at the html output in files//simpletest/browser_output

array (
  'Date' => 'Sat, 18 Nov 2023 11:41:16 GMT',
  'Server' => 'Apache/2.4.56 (Debian)',
  'X-Powered-By' => 'PHP/8.1.25',
  'X-Drupal-Assertion-0' => 'a%3A3%3A%7Bi%3A0%3BO%3A25%3A%22Drupal%5CCore%5CRender%5CMarkup%22%3A1%3A%7Bs%3A9%3A%22%00%2A%00string%22%3Bs%3A196%3A%22Since%20symfony%2Fserializer%206.3%3A%20%22Drupal%5Cjsonapi%5CNormalizer%5CImpostorFrom%5Cjsonapi_extras%5CFieldItemNormalizerImpostor%22%20should%20implement%20%22NormalizerInterface%3A%3AgetSupportedTypes%28%3Fstring%20%24format%29%3A%20array%22.%22%3B%7Di%3A1%3Bs%3A24%3A%22User%20deprecated%20function%22%3Bi%3A2%3Ba%3A3%3A%7Bs%3A8%3A%22function%22%3Bs%3A21%3A%22trigger_deprecation%28%29%22%3Bs%3A4%3A%22file%22%3Bs%3A61%3A%22%2Fgcl-builds%2Fvendor%2Fsymfony%2Fdeprecation-contracts%2Ffunction.php%22%3Bs%3A4%3A%22line%22%3Bi%3A25%3B%7D%7D',
  'X-Drupal-Assertion-1' => 'a%3A3%3A%7Bi%3A0%3BO%3A25%3A%22Drupal%5CCore%5CRender%5CMarkup%22%3A1%3A%7Bs%3A9%3A%22%00%2A%00string%22%3Bs%3A205%3A%22Since%20symfony%2Fserializer%206.3%3A%20%22Drupal%5Cjsonapi%5CNormalizer%5CImpostorFrom%5Cjsonapi_extras%5CResourceIdentifierNormalizerImpostor%22%20should%20implement%20%22NormalizerInterface%3A%3AgetSupportedTypes%28%3Fstring%20%24format%29%3A%20array%22.%22%3B%7Di%3A1%3Bs%3A24%3A%22User%20deprecated%20function%22%3Bi%3A2%3Ba%3A3%3A%7Bs%3A8%3A%22function%22%3Bs%3A21%3A%22trigger_deprecation%28%29%22%3Bs%3A4%3A%22file%22%3Bs%3A61%3A%22%2Fgcl-builds%2Fvendor%2Fsymfony%2Fdeprecation-contracts%2Ffunction.php%22%3Bs%3A4%3A%22line%22%3Bi%3A25%3B%7D%7D',
  'X-Drupal-Assertion-2' => 'a%3A3%3A%7Bi%3A0%3BO%3A25%3A%22Drupal%5CCore%5CRender%5CMarkup%22%3A1%3A%7Bs%3A9%3A%22%00%2A%00string%22%3Bs%3A201%3A%22Since%20symfony%2Fserializer%206.3%3A%20%22Drupal%5Cjsonapi%5CNormalizer%5CImpostorFrom%5Cjsonapi_extras%5CResourceObjectNormalizerImpostor%22%20should%20implement%20%22NormalizerInterface%3A%3AgetSupportedTypes%28%3Fstring%20%24format%29%3A%20array%22.%22%3B%7Di%3A1%3Bs%3A24%3A%22User%20deprecated%20function%22%3Bi%3A2%3Ba%3A3%3A%7Bs%3A8%3A%22function%22%3Bs%3A21%3A%22trigger_deprecation%28%29%22%3Bs%3A4%3A%22file%22%3Bs%3A61%3A%22%2Fgcl-builds%2Fvendor%2Fsymfony%2Fdeprecation-contracts%2Ffunction.php%22%3Bs%3A4%3A%22line%22%3Bi%3A25%3B%7D%7D',
  'X-Drupal-Assertion-3' => 'a%3A3%3A%7Bi%3A0%3BO%3A25%3A%22Drupal%5CCore%5CRender%5CMarkup%22%3A1%3A%7Bs%3A9%3A%22%00%2A%00string%22%3Bs%3A202%3A%22Since%20symfony%2Fserializer%206.3%3A%20%22Drupal%5Cjsonapi%5CNormalizer%5CImpostorFrom%5Cjsonapi_extras%5CContentEntityDenormalizerImpostor%22%20should%20implement%20%22NormalizerInterface%3A%3AgetSupportedTypes%28%3Fstring%20%24format%29%3A%20array%22.%22%3B%7Di%3A1%3Bs%3A24%3A%22User%20deprecated%20function%22%3Bi%3A2%3Ba%3A3%3A%7Bs%3A8%3A%22function%22%3Bs%3A21%3A%22trigger_deprecation%28%29%22%3Bs%3A4%3A%22file%22%3Bs%3A61%3A%22%2Fgcl-builds%2Fvendor%2Fsymfony%2Fdeprecation-contracts%2Ffunction.php%22%3Bs%3A4%3A%22line%22%3Bi%3A25%3B%7D%7D',
  'X-Drupal-Assertion-4' => 'a%3A3%3A%7Bi%3A0%3BO%3A25%3A%22Drupal%5CCore%5CRender%5CMarkup%22%3A1%3A%7Bs%3A9%3A%22%00%2A%00string%22%3Bs%3A201%3A%22Since%20symfony%2Fserializer%206.3%3A%20%22Drupal%5Cjsonapi%5CNormalizer%5CImpostorFrom%5Cjsonapi_extras%5CConfigEntityDenormalizerImpostor%22%20should%20implement%20%22NormalizerInterface%3A%3AgetSupportedTypes%28%3Fstring%20%24format%29%3A%20array%22.%22%3B%7Di%3A1%3Bs%3A24%3A%22User%20deprecated%20function%22%3Bi%3A2%3Ba%3A3%3A%7Bs%3A8%3A%22function%22%3Bs%3A21%3A%22trigger_deprecation%28%29%22%3Bs%3A4%3A%22file%22%3Bs%3A61%3A%22%2Fgcl-builds%2Fvendor%2Fsymfony%2Fdeprecation-contracts%2Ffunction.php%22%3Bs%3A4%3A%22line%22%3Bi%3A25%3B%7D%7D',
  'Cache-Control' => 'must-revalidate, no-cache, private',
  'X-Drupal-Dynamic-Cache' => 'MISS',
  'Content-language' => 'en',
  'X-Content-Type-Options' => 'nosniff',
  'X-Frame-Options' => 'SAMEORIGIN',
  'X-Drupal-Cache-Tags' => 'config:filter.format.plain_text config:jsonapi_extras.settings config:jsonapi_resource_config_list config:user.role.anonymous http_response node:1 node:2 node_list taxonomy_term:2 taxonomy_term:3 taxonomy_term:4 taxonomy_term:5',
  'X-Drupal-Cache-Contexts' => 'languages:language_interface theme url.query_args:fields url.query_args:filter url.query_args:include url.query_args:page url.query_args:resourceVersion url.query_args:sort url.site user.permissions',
  'X-Drupal-Cache-Max-Age' => '-1 (Permanent)',
  'Expires' => 'Sun, 19 Nov 1978 05:00:00 GMT',
  'X-Generator' => 'Drupal 10 (https://www.drupal.org)',
  'X-Drupal-Cache' => 'MISS',
  'Transfer-Encoding' => 'chunked',
  'Content-Type' => 'application/vnd.api+json',
)

I see cache info. Locally i was not sure how to reproduce the error, i got a hit on the second call.

ihor_allin’s picture

ihor_allin CreditAttribution: ihor_allin commented
StatusFileSize
new 3402388-default-includes-caching-fix-7.patch19.8 KB

@bbrala, thank you for the clear explanation. Updating the patch with the corresponding tests. Passed successfully on my local env

bbrala opened merge request !39

bbrala opened merge request !40

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented

Hmm, gitlab is weird.

But when i run those tests locally, when i do only the test (which should fail right?) then the test is still green. So that test doesn't proove the faulty caching...

ihor_allin’s picture

ihor_allin CreditAttribution: ihor_allin commented

@bbrala do you have default includes added when you test it locally?

ihor_allin’s picture

ihor_allin CreditAttribution: ihor_allin commented
StatusFileSize
new 3402388-default-includes-caching-fix-12.patch19.79 KB

Fixed bug with variable name

ihor_allin’s picture

ihor_allin CreditAttribution: ihor_allin commented
StatusFileSize
new 3402388-default-includes-caching-fix-13.patch19.83 KB
ihor_allin’s picture

ihor_allin CreditAttribution: ihor_allin commented

For some reason, locally, I have also passed tests by adding a cache tag check: $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'HIT') after the second request to '/api/articles'. However, at the same time, when I check it by visiting the JSON API route with the resource with default includes, I always receive an uncached response.

ihor_allin’s picture

ihor_allin CreditAttribution: ihor_allin commented
StatusFileSize
new 3402388-default-includes-caching-fix-14.patch22.34 KB

Update patch with minor code styles fixes

ihor_allin’s picture

ihor_allin CreditAttribution: ihor_allin commented
StatusFileSize
new 3402388-default-includes-caching-fix-15.patch22.48 KB

Fixed 500 error for the resources without default includes

_tarik_’s picture

_tarik_ CreditAttribution: _tarik_ at DevBranch commented

Thanks ihor_allin. The patch in comment #16 works for me.

Ankush_03’s picture

Ankush_03
Location Gurgaon, India 🇮🇳
CreditAttribution: Ankush_03 commented

Hey @bbrala,

Any update on the above fix ??

Ankush_03’s picture

Ankush_03
Location Gurgaon, India 🇮🇳
CreditAttribution: Ankush_03 commented
Priority: Normal » Major
gaurav_manerkar’s picture

gaurav_manerkar CreditAttribution: gaurav_manerkar at EPAM Systems commented
Status: Active » Needs review

The last submitted patch, 2: 3402388-default-includes-caching-fix.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 7: 3402388-default-includes-caching-fix-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gaurav_manerkar’s picture

gaurav_manerkar CreditAttribution: gaurav_manerkar at EPAM Systems commented

Review https://git.drupalcode.org/project/jsonapi_extras/-/merge_requests/40

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
CreditAttribution: anybody as a volunteer and at DROWL.de commented
Related issues: +#3403298: Pages with includes are cached without (default) included referenced content
_tarik_’s picture

_tarik_ CreditAttribution: _tarik_ at DevBranch commented

I have found an issue with response caching with this patch.
The isDefaultIncludes method checks only if the request contains included parameters from the jsonapi_extras configuration. So, it can cause problems when the one cached response is used for different includes params.

Say, you have an endpoint for an article that uses 3 taxonomy fields (field_tag, field_type, field_category).
The jsonapi_extras is configured to include by default the field_type field. Then if you send the first request with ?include=field_category this response will be cached and applied to the response without the include param or for ?include=field_tag.

So, instead of checking if the request uses default includes we need to check if the request uses ONLY default includes.

_tarik_’s picture

_tarik_ CreditAttribution: _tarik_ at DevBranch commented
StatusFileSize
new 3402388-31.patch21.64 KB

The diff for PR to avoid adding numerous commits to the composer.json.

_tarik_’s picture

_tarik_ CreditAttribution: _tarik_ at DevBranch commented
StatusFileSize
new 3402388-27.patch21.82 KB

I realized that made a mistake in my previous commit. Here is the correct version of the patch

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented
Status: Needs review » Fixed

Ty for taking the time here to fix this up. Changes seem to work as promised <3

  • bbrala committed 373f0114 on 8.x-3.x 
    Issue #3402388 by ihor_allin, bbrala, _tarik_, gaurav_manerkar,...
bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.