Merge pull request #15 from carsdotcom/cache-seed-not-configurable

fix: cache key seed must increment regardless of config of host project

app/AbstractRequest.php

@@ -40,6 +40,7 @@ abstract class AbstractRequest
     protected bool $shouldReadCache = true;
     protected bool $shouldWriteCache = true;
 
+
     /** @var array Tags to be used when inserting to cache */
     protected array $cacheTags = [];
 
@@ -323,6 +324,8 @@ protected function shouldWriteResponseToCache(): bool
         return $this->shouldWriteCache && !$this->responseIsFromCache;
     }
 
+    public const CACHE_KEY_SEED = 'v2024.8.6';
+
     protected function writeResponseToCache(): void
     {
         if (!$this->shouldWriteResponseToCache()) {
@@ -331,6 +334,8 @@ protected function writeResponseToCache(): void
 
         // Streams can't be cached by Laravel,
         // so we flatten the body to strings then rehydrate the Response class manually
+        // Note, when the format of the cached value changes, you have to update CACHE_KEY_SEED
+        // So that previous cache entries with incompatible cached data are not read by responseFromCache
         Cache::tags($this->cacheTags)->put(
             $this->cacheKey(),
             [
@@ -354,6 +359,8 @@ protected function responseFromCache(): ?Response
             return null;
         }
 
+        // Note, when the format of $fromCache changes, you have to update CACHE_KEY_SEED
+        // So that previous cache entries with incompatible cached data are not read by responseFromCache
         $fromCache = Cache::tags($this->cacheTags)->get($this->cacheKey());
         if ($fromCache) {
             $this->sentLogs = $fromCache['logs'];
@@ -363,7 +370,9 @@ protected function responseFromCache(): ?Response
     }
 
     /**
-     * Return cache key string based on this class, URL, and request body
+     * Return cache key string based on this class, URL, request body
+     * and the CACHE_KEY_SEED. The seed changes when the format of the encoded data makes an incompatible change
+     * (e.g. when we moved from storing the string response body to storing the entire response object, and again when we added the log files)
      * @return string
      */
     public function cacheKey(): string
@@ -374,7 +383,7 @@ public function cacheKey(): string
                 self::class,
                 $this->getURL(),
                 $this->encodeBody(),
-                config('api-request.cache_key_seed', 'v2024.8.6'),
+                self::CACHE_KEY_SEED
             ]),
         );
     }

app/Testing/RequestClassAssertions.php

@@ -0,0 +1,43 @@
+<?php
+/**
+ * Shared assertions about test cases.
+ */
+declare(strict_types=1);
+
+namespace Carsdotcom\ApiRequest\Testing;
+
+use Carsdotcom\ApiRequest\AbstractRequest;
+use Illuminate\Support\Facades\Cache;
+
+trait RequestClassAssertions
+{
+    /**
+     * Create a fake cache entry for a given request.
+     */
+    protected static function mockRequestCachedResponse(
+        AbstractRequest $request,
+        string $body,
+        int $status = 200,
+        array $headers = [],
+        array $logs = [],
+    ): void {
+        $tags = getProperty($request, 'cacheTags');
+        Cache::tags($tags)->put($request->cacheKey(), [
+            'logs' => $logs,
+            'response' => [$status, $headers, $body],
+        ]);
+    }
+
+    /**
+     * Fetch the cached response to a request and assert that it contains a substring
+     */
+    protected static function assertRequestCacheBodyContains(
+        string $substring,
+        AbstractRequest $request,
+        string $message = '',
+    ): void {
+        $cached = callMethod($request, 'responseFromCache');
+        self::assertNotNull($cached, 'Cache should not be empty for request.');
+        self::assertStringContainsString($substring, (string) $cached->getBody(), $message);
+    }
+}

config/api-request.php

@@ -6,27 +6,17 @@
     | Storage disk name
     |--------------------------------------------------------------------------
     |
-    | This should be the name of a disk where Logs can be stored.
+    | This should be the name of a Laravel filesystem disk where Logs can be stored.
     |
     */
     'logs_storage_disk_name' => 'api-logs',
 
-    /*
-    |--------------------------------------------------------------------------
-    | Cache key seed
-    |--------------------------------------------------------------------------
-    |
-    | In the rare event that you change *what* we cache, increment this.
-    |
-    */
-    'cache_key_seed' => 'v2022.4.12.0',
-
     /*
     |--------------------------------------------------------------------------
     | Tapper: Data Storage Disk Name
     |--------------------------------------------------------------------------
     |
-    | This should be the name of a disk where data for tapper requests is stored.
+    | This should be the name of a Laravel filesystem disk where data for Tapper network responses are stored.
     |
     */
     'tapper_data_storage_disk_name' => 'tapper-data',

tests/BaseTestCase.php

@@ -28,30 +28,4 @@ protected function defineEnvironment($app)
             'prefix'   => '',
         ]);
     }
-
-    /**
-     * Create a fake cache entry for a given request.
-     */
-    protected static function mockRequestCachedResponse(
-        AbstractRequest $request,
-        string $body,
-        int $status = 200,
-        array $headers = [],
-    ) {
-        $tags = getProperty($request, 'cacheTags');
-        Cache::tags($tags)->put($request->cacheKey(), ['logs' => [], 'response' => [$status, $headers, $body]]);
-    }
-
-    /**
-     * Fetch the cached response to a request and assert that it contains a substring
-     */
-    protected static function requestCacheBodyContains(
-        string $substring,
-        AbstractRequest $request,
-        string $message = '',
-    ): void {
-        $cached = callMethod($request, 'responseFromCache');
-        self::assertNotNull($cached, 'Cache should not be empty for request.');
-        self::assertStringContainsString($substring, (string) $cached->getBody(), $message);
-    }
 }

tests/Feature/AbstractRequestTest.php

@@ -2,20 +2,21 @@
 
 namespace Tests\Feature;
 
+use Carbon\Carbon;
 use Carbon\CarbonInterval;
-use Carsdotcom\ApiRequest\Exceptions\UpstreamException;
+use Carsdotcom\ApiRequest\AbstractRequest;
 use Carsdotcom\ApiRequest\Exceptions\ToDoException;
+use Carsdotcom\ApiRequest\Exceptions\UpstreamException;
 use Carsdotcom\ApiRequest\Testing\GuzzleTapper;
+use Carsdotcom\ApiRequest\Testing\MocksGuzzleInstance;
+use Carsdotcom\ApiRequest\Testing\RequestClassAssertions;
 use Carsdotcom\ApiRequest\Traits\EncodeRequestJSON;
 use Carsdotcom\ApiRequest\Traits\ParseResponseJSON;
 use Carsdotcom\ApiRequest\Traits\ParseResponseJSONOrThrow;
-use Carbon\Carbon;
 use GuzzleHttp\Client;
 use GuzzleHttp\Exception\ClientException;
 use GuzzleHttp\Exception\ConnectException;
 use GuzzleHttp\Exception\ServerException;
-use GuzzleHttp\Handler\MockHandler;
-use GuzzleHttp\HandlerStack;
 use GuzzleHttp\Promise\Promise;
 use GuzzleHttp\Promise\PromiseInterface;
 use GuzzleHttp\Promise\RejectedPromise;
@@ -27,13 +28,13 @@
 use Illuminate\Support\Facades\Storage;
 use Tests\BaseTestCase;
 use Tests\MockClasses\ConcreteRequest;
-use Carsdotcom\ApiRequest\Testing\MocksGuzzleInstance;
 use TiMacDonald\Log\LogEntry;
 use TiMacDonald\Log\LogFake;
 
 class AbstractRequestTest extends BaseTestCase
 {
     use MocksGuzzleInstance;
+    use RequestClassAssertions;
 
     protected function setUp(): void
     {
@@ -72,6 +73,7 @@ public function testCacheMissUsesGuzzle(): void
         self::assertSame(['awesome' => 'sauce'], $result);
         // Result was cached
         self::assertTrue($requestClass->canBeFulfilledByCache());
+        self::assertRequestCacheBodyContains('sauce', $requestClass);
     }
 
     public function testCacheHitAfterFirstRequest(): void
@@ -208,7 +210,7 @@ public function testPurgeCache(): void
         self::assertTrue($firstRequest->canBeFulfilledByCache());
     }
 
-    public function testCacheDisabledInChildClass(): void
+    public function testCacheDisabledWrite(): void
     {
         $tapper = $this->mockGuzzleWithTapper();
         $tapper->addMatchBody('POST', '/awesome/', '{"awesome":"sauce"}');
@@ -223,7 +225,39 @@ public function testCacheDisabledInChildClass(): void
         $request->sync();
 
         self::assertSame(1, $tapper->getCountLike('POST', '/awesome/'));
+        // Still not in cache
+        self::assertFalse($request->canBeFulfilledByCache());
+
+        // But if something *else* caches it, we'll read from it
+        self::mockRequestCachedResponse($request, '{"awesome":"possum"}');
+        $response = $request->sync();
+        self::assertSame('{"awesome":"possum"}', $response, "Should receive cache value, not Tapper value");
+        self::assertSame(1, $tapper->getCountLike('POST', '/awesome/'));
+    }
+
+    public function testCacheDisabledRead(): void
+    {
+        $tapper = $this->mockGuzzleWithTapper();
+        $tapper->addMatchBody('POST', '/awesome/', '{"awesome":"sauce"}');
+
+        $request = new ConcreteRequest();
+        $request->setReadCache(false);
+
+        // Not in cache, never been called
         self::assertFalse($request->canBeFulfilledByCache());
+        self::assertSame(0, $tapper->getCountLike('POST', '/awesome/'));
+
+        $response = $request->sync();
+        self::assertSame('{"awesome":"sauce"}', $response, "Should receive Tapper value, ignoring cache");
+        self::assertSame(1, $tapper->getCountLike('POST', '/awesome/'));
+        self::assertTrue($request->canBeFulfilledByCache());
+
+        // But if the value in cache is manipulated
+        self::mockRequestCachedResponse($request, '{"awesome":"possum"}');
+        $response = $request->sync();
+        self::assertSame('{"awesome":"sauce"}', $response, "Should receive Tapper value, ignoring cache");
+        // Makes a second request even though a hit was in cache
+        self::assertSame(2, $tapper->getCountLike('POST', '/awesome/'));
     }
 
     public function testCacheMissRequestFails(): void
@@ -270,7 +304,7 @@ public function testPostProcessMutatesResult(): void
         // API returned JSON 42, postProcess doubles it
         self::assertSame(84, $result);
         // API result (not processed outcome!) was cached
-        self::requestCacheBodyContains('42', $request);
+        self::assertRequestCacheBodyContains('42', $request);
     }
 
     public function testPostProcessCanReject(): void
@@ -733,4 +767,34 @@ protected function getGuzzleClient(): Client
             self::assertSame("Server error: `POST https://awesome-api.com/url` resulted in a `500 Internal Server Error` response:\nThis method is not implemented\n", $exception->getMessage());
         }
     }
+
+    /**
+     * This test should start failing if you change the format of writeResponseToCache
+     * This is a reminder to increment CACHE_KEY_SEED before you fix this test,
+     */
+    public function testCacheStructureCanary(): void
+    {
+        $firstLogTime = '2018-01-01T00:00:00.000000+00:00';
+        Carbon::setTestNow($firstLogTime);
+
+        $this->mockGuzzleWithTapper()->addMatchBody('POST', '/awesome/', '{"awesome":"sauce"}');
+        $request = $this->mockRequestWithLog();
+        $request->setWriteCache(true)->sync();
+        self::assertTrue($request->canBeFulfilledByCache());
+
+        $cached = Cache::tags([])->get($request->cacheKey());
+        self::assertIsArray($cached);
+        self::assertSame(['logs', 'response'], array_keys($cached)); // No new keys, no removed keys
+        self::assertSame([
+            0 => 200,
+            1 => [],
+            2 => '{"awesome":"sauce"}',
+            3 => '1.1',
+            4 => 'OK',
+        ], $cached['response']);
+        self::assertSame([ "2018-01-01T00:00:00.000000+00:00" ], $cached['logs']);
+
+        // If you modified this test in any way, you need to change the CACHE_KEY_SEED
+        self::assertSame('v2024.8.6', AbstractRequest::CACHE_KEY_SEED);
+    }
 }