Skip to content

Commit 926581a

Browse files
jwadhamsjwadhams
authored
Merge pull request #15 from carsdotcom/cache-seed-not-configurable
fix: cache key seed must increment regardless of config of host project
2 parents 066cd42 + 32e48e8 commit 926581a

5 files changed

Lines changed: 127 additions & 47 deletions

File tree

‎app/AbstractRequest.php‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ abstract class AbstractRequest
4040
protected bool $shouldReadCache = true;
4141
protected bool $shouldWriteCache = true;
4242

43+
4344
/** @var array Tags to be used when inserting to cache */
4445
protected array $cacheTags = [];
4546

@@ -323,6 +324,8 @@ protected function shouldWriteResponseToCache(): bool
323324
return $this->shouldWriteCache && !$this->responseIsFromCache;
324325
}
325326

327+
public const CACHE_KEY_SEED = 'v2024.8.6';
328+
326329
protected function writeResponseToCache(): void
327330
{
328331
if (!$this->shouldWriteResponseToCache()) {
@@ -331,6 +334,8 @@ protected function writeResponseToCache(): void
331334

332335
// Streams can't be cached by Laravel,
333336
// so we flatten the body to strings then rehydrate the Response class manually
337+
// Note, when the format of the cached value changes, you have to update CACHE_KEY_SEED
338+
// So that previous cache entries with incompatible cached data are not read by responseFromCache
334339
Cache::tags($this->cacheTags)->put(
335340
$this->cacheKey(),
336341
[
@@ -354,6 +359,8 @@ protected function responseFromCache(): ?Response
354359
return null;
355360
}
356361

362+
// Note, when the format of $fromCache changes, you have to update CACHE_KEY_SEED
363+
// So that previous cache entries with incompatible cached data are not read by responseFromCache
357364
$fromCache = Cache::tags($this->cacheTags)->get($this->cacheKey());
358365
if ($fromCache) {
359366
$this->sentLogs = $fromCache['logs'];
@@ -363,7 +370,9 @@ protected function responseFromCache(): ?Response
363370
}
364371

365372
/**
366-
* Return cache key string based on this class, URL, and request body
373+
* Return cache key string based on this class, URL, request body
374+
* and the CACHE_KEY_SEED. The seed changes when the format of the encoded data makes an incompatible change
375+
* (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)
367376
* @return string
368377
*/
369378
public function cacheKey(): string
@@ -374,7 +383,7 @@ public function cacheKey(): string
374383
self::class,
375384
$this->getURL(),
376385
$this->encodeBody(),
377-
config('api-request.cache_key_seed', 'v2024.8.6'),
386+
self::CACHE_KEY_SEED
378387
]),
379388
);
380389
}

‎app/Testing/RequestClassAssertions.php‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
<?php
2+
/**
3+
* Shared assertions about test cases.
4+
*/
5+
declare(strict_types=1);
6+
7+
namespace Carsdotcom\ApiRequest\Testing;
8+
9+
use Carsdotcom\ApiRequest\AbstractRequest;
10+
use Illuminate\Support\Facades\Cache;
11+
12+
trait RequestClassAssertions
13+
{
14+
/**
15+
* Create a fake cache entry for a given request.
16+
*/
17+
protected static function mockRequestCachedResponse(
18+
AbstractRequest $request,
19+
string $body,
20+
int $status = 200,
21+
array $headers = [],
22+
array $logs = [],
23+
): void {
24+
$tags = getProperty($request, 'cacheTags');
25+
Cache::tags($tags)->put($request->cacheKey(), [
26+
'logs' => $logs,
27+
'response' => [$status, $headers, $body],
28+
]);
29+
}
30+
31+
/**
32+
* Fetch the cached response to a request and assert that it contains a substring
33+
*/
34+
protected static function assertRequestCacheBodyContains(
35+
string $substring,
36+
AbstractRequest $request,
37+
string $message = '',
38+
): void {
39+
$cached = callMethod($request, 'responseFromCache');
40+
self::assertNotNull($cached, 'Cache should not be empty for request.');
41+
self::assertStringContainsString($substring, (string) $cached->getBody(), $message);
42+
}
43+
}

‎config/api-request.php‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,27 +6,17 @@
66
| Storage disk name
77
|--------------------------------------------------------------------------
88
|
9-
| This should be the name of a disk where Logs can be stored.
9+
| This should be the name of a Laravel filesystem disk where Logs can be stored.
1010
|
1111
*/
1212
'logs_storage_disk_name' => 'api-logs',
1313

14-
/*
15-
|--------------------------------------------------------------------------
16-
| Cache key seed
17-
|--------------------------------------------------------------------------
18-
|
19-
| In the rare event that you change *what* we cache, increment this.
20-
|
21-
*/
22-
'cache_key_seed' => 'v2022.4.12.0',
23-
2414
/*
2515
|--------------------------------------------------------------------------
2616
| Tapper: Data Storage Disk Name
2717
|--------------------------------------------------------------------------
2818
|
29-
| This should be the name of a disk where data for tapper requests is stored.
19+
| This should be the name of a Laravel filesystem disk where data for Tapper network responses are stored.
3020
|
3121
*/
3222
'tapper_data_storage_disk_name' => 'tapper-data',

‎tests/BaseTestCase.php‎

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -28,30 +28,4 @@ protected function defineEnvironment($app)
2828
'prefix' => '',
2929
]);
3030
}
31-
32-
/**
33-
* Create a fake cache entry for a given request.
34-
*/
35-
protected static function mockRequestCachedResponse(
36-
AbstractRequest $request,
37-
string $body,
38-
int $status = 200,
39-
array $headers = [],
40-
) {
41-
$tags = getProperty($request, 'cacheTags');
42-
Cache::tags($tags)->put($request->cacheKey(), ['logs' => [], 'response' => [$status, $headers, $body]]);
43-
}
44-
45-
/**
46-
* Fetch the cached response to a request and assert that it contains a substring
47-
*/
48-
protected static function requestCacheBodyContains(
49-
string $substring,
50-
AbstractRequest $request,
51-
string $message = '',
52-
): void {
53-
$cached = callMethod($request, 'responseFromCache');
54-
self::assertNotNull($cached, 'Cache should not be empty for request.');
55-
self::assertStringContainsString($substring, (string) $cached->getBody(), $message);
56-
}
5731
}

‎tests/Feature/AbstractRequestTest.php‎

Lines changed: 71 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,21 @@
22

33
namespace Tests\Feature;
44

5+
use Carbon\Carbon;
56
use Carbon\CarbonInterval;
6-
use Carsdotcom\ApiRequest\Exceptions\UpstreamException;
7+
use Carsdotcom\ApiRequest\AbstractRequest;
78
use Carsdotcom\ApiRequest\Exceptions\ToDoException;
9+
use Carsdotcom\ApiRequest\Exceptions\UpstreamException;
810
use Carsdotcom\ApiRequest\Testing\GuzzleTapper;
11+
use Carsdotcom\ApiRequest\Testing\MocksGuzzleInstance;
12+
use Carsdotcom\ApiRequest\Testing\RequestClassAssertions;
913
use Carsdotcom\ApiRequest\Traits\EncodeRequestJSON;
1014
use Carsdotcom\ApiRequest\Traits\ParseResponseJSON;
1115
use Carsdotcom\ApiRequest\Traits\ParseResponseJSONOrThrow;
12-
use Carbon\Carbon;
1316
use GuzzleHttp\Client;
1417
use GuzzleHttp\Exception\ClientException;
1518
use GuzzleHttp\Exception\ConnectException;
1619
use GuzzleHttp\Exception\ServerException;
17-
use GuzzleHttp\Handler\MockHandler;
18-
use GuzzleHttp\HandlerStack;
1920
use GuzzleHttp\Promise\Promise;
2021
use GuzzleHttp\Promise\PromiseInterface;
2122
use GuzzleHttp\Promise\RejectedPromise;
@@ -27,13 +28,13 @@
2728
use Illuminate\Support\Facades\Storage;
2829
use Tests\BaseTestCase;
2930
use Tests\MockClasses\ConcreteRequest;
30-
use Carsdotcom\ApiRequest\Testing\MocksGuzzleInstance;
3131
use TiMacDonald\Log\LogEntry;
3232
use TiMacDonald\Log\LogFake;
3333

3434
class AbstractRequestTest extends BaseTestCase
3535
{
3636
use MocksGuzzleInstance;
37+
use RequestClassAssertions;
3738

3839
protected function setUp(): void
3940
{
@@ -72,6 +73,7 @@ public function testCacheMissUsesGuzzle(): void
7273
self::assertSame(['awesome' => 'sauce'], $result);
7374
// Result was cached
7475
self::assertTrue($requestClass->canBeFulfilledByCache());
76+
self::assertRequestCacheBodyContains('sauce', $requestClass);
7577
}
7678

7779
public function testCacheHitAfterFirstRequest(): void
@@ -208,7 +210,7 @@ public function testPurgeCache(): void
208210
self::assertTrue($firstRequest->canBeFulfilledByCache());
209211
}
210212

211-
public function testCacheDisabledInChildClass(): void
213+
public function testCacheDisabledWrite(): void
212214
{
213215
$tapper = $this->mockGuzzleWithTapper();
214216
$tapper->addMatchBody('POST', '/awesome/', '{"awesome":"sauce"}');
@@ -223,7 +225,39 @@ public function testCacheDisabledInChildClass(): void
223225
$request->sync();
224226

225227
self::assertSame(1, $tapper->getCountLike('POST', '/awesome/'));
228+
// Still not in cache
229+
self::assertFalse($request->canBeFulfilledByCache());
230+
231+
// But if something *else* caches it, we'll read from it
232+
self::mockRequestCachedResponse($request, '{"awesome":"possum"}');
233+
$response = $request->sync();
234+
self::assertSame('{"awesome":"possum"}', $response, "Should receive cache value, not Tapper value");
235+
self::assertSame(1, $tapper->getCountLike('POST', '/awesome/'));
236+
}
237+
238+
public function testCacheDisabledRead(): void
239+
{
240+
$tapper = $this->mockGuzzleWithTapper();
241+
$tapper->addMatchBody('POST', '/awesome/', '{"awesome":"sauce"}');
242+
243+
$request = new ConcreteRequest();
244+
$request->setReadCache(false);
245+
246+
// Not in cache, never been called
226247
self::assertFalse($request->canBeFulfilledByCache());
248+
self::assertSame(0, $tapper->getCountLike('POST', '/awesome/'));
249+
250+
$response = $request->sync();
251+
self::assertSame('{"awesome":"sauce"}', $response, "Should receive Tapper value, ignoring cache");
252+
self::assertSame(1, $tapper->getCountLike('POST', '/awesome/'));
253+
self::assertTrue($request->canBeFulfilledByCache());
254+
255+
// But if the value in cache is manipulated
256+
self::mockRequestCachedResponse($request, '{"awesome":"possum"}');
257+
$response = $request->sync();
258+
self::assertSame('{"awesome":"sauce"}', $response, "Should receive Tapper value, ignoring cache");
259+
// Makes a second request even though a hit was in cache
260+
self::assertSame(2, $tapper->getCountLike('POST', '/awesome/'));
227261
}
228262

229263
public function testCacheMissRequestFails(): void
@@ -270,7 +304,7 @@ public function testPostProcessMutatesResult(): void
270304
// API returned JSON 42, postProcess doubles it
271305
self::assertSame(84, $result);
272306
// API result (not processed outcome!) was cached
273-
self::requestCacheBodyContains('42', $request);
307+
self::assertRequestCacheBodyContains('42', $request);
274308
}
275309

276310
public function testPostProcessCanReject(): void
@@ -733,4 +767,34 @@ protected function getGuzzleClient(): Client
733767
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());
734768
}
735769
}
770+
771+
/**
772+
* This test should start failing if you change the format of writeResponseToCache
773+
* This is a reminder to increment CACHE_KEY_SEED before you fix this test,
774+
*/
775+
public function testCacheStructureCanary(): void
776+
{
777+
$firstLogTime = '2018-01-01T00:00:00.000000+00:00';
778+
Carbon::setTestNow($firstLogTime);
779+
780+
$this->mockGuzzleWithTapper()->addMatchBody('POST', '/awesome/', '{"awesome":"sauce"}');
781+
$request = $this->mockRequestWithLog();
782+
$request->setWriteCache(true)->sync();
783+
self::assertTrue($request->canBeFulfilledByCache());
784+
785+
$cached = Cache::tags([])->get($request->cacheKey());
786+
self::assertIsArray($cached);
787+
self::assertSame(['logs', 'response'], array_keys($cached)); // No new keys, no removed keys
788+
self::assertSame([
789+
0 => 200,
790+
1 => [],
791+
2 => '{"awesome":"sauce"}',
792+
3 => '1.1',
793+
4 => 'OK',
794+
], $cached['response']);
795+
self::assertSame([ "2018-01-01T00:00:00.000000+00:00" ], $cached['logs']);
796+
797+
// If you modified this test in any way, you need to change the CACHE_KEY_SEED
798+
self::assertSame('v2024.8.6', AbstractRequest::CACHE_KEY_SEED);
799+
}
736800
}

0 commit comments

Comments
 (0)