Issue #3444257 by bbrala, mxr576, quietone, acbramley, larowlan, casey, enyug,...

Issue #3444257 by bbrala, mxr576, quietone, acbramley, larowlan, casey, enyug, mstrelan, nick_schuch: ResourceObjectNormalizer::getNormalization can result in max-age drift when different sets of fields are requested

core/modules/jsonapi/src/EventSubscriber/ResourceObjectNormalizationCacher.php

@@ -5,6 +5,7 @@
 use Drupal\Core\Cache\CacheableMetadata;
 use Drupal\Core\Cache\VariationCacheInterface;
 use Drupal\jsonapi\JsonApiResource\ResourceObject;
+use Drupal\jsonapi\Normalizer\Value\CacheableNormalization;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 use Symfony\Component\HttpFoundation\RequestStack;
 use Symfony\Component\HttpKernel\Event\TerminateEvent;
@@ -102,7 +103,39 @@ public function get(ResourceObject $object) {
     }
 
     $cached = $this->variationCache->get($this->generateCacheKeys($object), new CacheableMetadata());
-    return $cached ? $cached->data : FALSE;
+    if (!$cached) {
+      return FALSE;
+    }
+
+    // When a cache hit occurs, we first calculate the remaining time before the
+    // cached record expires, ensuring that we do not reset the expiration with
+    // one that might have been generated on an earlier timestamp. This is done
+    // by subtracting the current timestamp from the cached record's expiration
+    // timestamp. If the max-age is set, we adjust it by merging the calculated
+    // remaining time with the original max-age of the cached item, ensuring
+    // that the expiration remains accurate based on the current cache state
+    // and timestamp.
+    $normalizer_values = $cached->data;
+    assert(is_array($normalizer_values));
+    if ($cached->expire >= 0) {
+      $max_age = max($cached->expire - $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME'), 0);
+      $cacheability = new CacheableMetadata();
+      $cacheability->setCacheMaxAge($max_age);
+
+      $subsets = [
+        ResourceObjectNormalizationCacher::RESOURCE_CACHE_SUBSET_BASE,
+        ResourceObjectNormalizationCacher::RESOURCE_CACHE_SUBSET_FIELDS,
+      ];
+      foreach ($subsets as $subset) {
+        foreach ($normalizer_values[$subset] as $name => $normalization) {
+          assert($normalization instanceof CacheableNormalization);
+          if ($normalization->getCacheMaxAge() > 0) {
+            $normalizer_values[$subset][$name] = $normalization->withCacheableDependency($cacheability);
+          }
+        }
+      }
+    }
+    return $normalizer_values;
   }
 
   /**

core/modules/jsonapi/tests/src/Kernel/EventSubscriber/ResourceObjectNormalizerCacherTest.php

@@ -6,6 +6,7 @@
 
 use Drupal\Core\Cache\Cache;
 use Drupal\Core\Cache\CacheableMetadata;
+use Drupal\entity_test\Entity\EntityTestComputedField;
 use Drupal\jsonapi\EventSubscriber\ResourceObjectNormalizationCacher;
 use Drupal\jsonapi\JsonApiResource\ResourceObject;
 use Drupal\jsonapi\Normalizer\Value\CacheableNormalization;
@@ -28,9 +29,11 @@ class ResourceObjectNormalizerCacherTest extends KernelTestBase {
    * {@inheritdoc}
    */
   protected static $modules = [
+    'entity_test',
     'file',
     'system',
     'serialization',
+    'text',
     'jsonapi',
     'user',
   ];
@@ -61,6 +64,7 @@ class ResourceObjectNormalizerCacherTest extends KernelTestBase {
    */
   protected function setUp(): void {
     parent::setUp();
+
     // Add the entity schemas.
     $this->installEntitySchema('user');
     // Add the additional table schemas.
@@ -108,4 +112,65 @@ public function testLinkNormalizationCacheability(): void {
     $this->assertFalse((bool) $this->cacher->get($resource_object));
   }
 
+  /**
+   * Tests that normalization max-age is correct.
+   *
+   * When max-age for a cached record is set the expiry is set accordingly. But
+   * if the cached normalization is partially used in a later normalization the
+   * max-age should be adjusted to a new timestamp.
+   *
+   * If we don't do this the expires of the cache record will be reset based on
+   * the original max age. This leads to a drift in the expiry time of the
+   * record.
+   *
+   * If a field tells the cache it should expire in exactly 1 hour, then if the
+   * cached data is used 10 minutes later in another resource, that cache should
+   * expire in 50 minutes and not reset to 60 minutes.
+   */
+  public function testMaxAgeCorrection(): void {
+    $this->installEntitySchema('entity_test_computed_field');
+
+    // Use EntityTestComputedField since ComputedTestCacheableStringItemList has a max age of 800
+    $baseMaxAge = 800;
+    $entity = EntityTestComputedField::create([]);
+    $entity->save();
+    $resource_type = $this->resourceTypeRepository->get($entity->getEntityTypeId(), $entity->bundle());
+    $resource_object = ResourceObject::createFromEntity($resource_type, $entity);
+
+    $resource_normalization = $this->serializer
+      ->normalize($resource_object, 'api_json', ['account' => NULL]);
+    $this->assertEquals($baseMaxAge, $resource_normalization->getCacheMaxAge());
+
+    // Save the normalization to cache, this is done at TerminateEvent.
+    $http_kernel = $this->prophesize(HttpKernelInterface::class);
+    $request = $this->prophesize(Request::class);
+    $response = $this->prophesize(Response::class);
+    $event = new TerminateEvent($http_kernel->reveal(), $request->reveal(), $response->reveal());
+    $this->cacher->onTerminate($event);
+
+    // Change request time to 500 seconds later
+    $current_request = \Drupal::requestStack()->getCurrentRequest();
+    $current_request->server->set('REQUEST_TIME', $current_request->server->get('REQUEST_TIME') + 500);
+    $resource_normalization = $this->serializer
+      ->normalize($resource_object, 'api_json', ['account' => NULL]);
+    $this->assertEquals($baseMaxAge - 500, $resource_normalization->getCacheMaxAge(), 'Max age should be 300 since 500 seconds has passed');
+
+    // Change request time to 800 seconds later, this is the last second the
+    // cache backend would return cached data. The max-age at that time should
+    // be 0 which is the same as the expire time of the cache entry.
+    $current_request->server->set('REQUEST_TIME', $current_request->server->get('REQUEST_TIME') + 800);
+    $resource_normalization = $this->serializer
+      ->normalize($resource_object, 'api_json', ['account' => NULL]);
+    $this->assertEquals(0, $resource_normalization->getCacheMaxAge(), 'Max age should be 0 since max-age has passed');
+
+    // Change request time to 801 seconds later. This validates that max-age
+    // never becomes negative. This should never happen as the cache entry
+    // is expired at this time and the cache backend would not return data.
+    $current_request->server->set('REQUEST_TIME', $current_request->server->get('REQUEST_TIME') + 801);
+    $resource_normalization = $this->serializer
+      ->normalize($resource_object, 'api_json', ['account' => NULL]);
+    $this->assertEquals(0, $resource_normalization->getCacheMaxAge(), 'Max age should be 0 since max-age has passed a second ago');
+
+  }
+
 }