From 00c41938ba62edd2dcaca995eb7cc9403b975118 Mon Sep 17 00:00:00 2001 From: Jeremy Wadhams Date: Fri, 28 Feb 2025 11:19:08 -0600 Subject: [PATCH 1/2] fix: issue-19 sentLogs should store (and cache) the filename and the log folder --- app/AbstractRequest.php | 7 ++--- tests/Feature/AbstractRequestTest.php | 45 ++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/app/AbstractRequest.php b/app/AbstractRequest.php index d641a6c..fc2de5d 100644 --- a/app/AbstractRequest.php +++ b/app/AbstractRequest.php @@ -434,7 +434,7 @@ public function log($outcome): void } $logged = $this->getLogFileHelper()::put($this->getLogFolder(), [$this->toGuzzle(), $outcome, $this->requestStats]); if ($logged) { - $this->sentLogs[] = $logged; + $this->sentLogs[] = Str::finish($this->getLogFolder(), '/') . $logged; } } @@ -454,10 +454,7 @@ public function getLastLogContents(): string */ public function getLastLogFile(): string { - if (!$this->sentLogs) { - throw new DomainException('No log files have been saved by this instance.'); - } - return Str::finish($this->getLogFolder(), '/') . Arr::last($this->sentLogs); + return Arr::last($this->sentLogs) ?? throw new DomainException('No log files have been saved by this instance.'); } /** diff --git a/tests/Feature/AbstractRequestTest.php b/tests/Feature/AbstractRequestTest.php index 2825123..1f16ea4 100644 --- a/tests/Feature/AbstractRequestTest.php +++ b/tests/Feature/AbstractRequestTest.php @@ -141,6 +141,49 @@ public function testCacheHitHasAccessToOriginalLog(): void self::assertSame(["one/two/{$firstLogTime}"], Storage::disk('api-logs')->files('one/two')); } + public function testCachedLogFileContainsFolder(): void + { + Storage::fake('api-logs'); + $tapper = $this->mockGuzzleWithTapper(); + $tapper->addMatchBody('POST', '/awesome/', '{"awesome":"sauce"}'); + + $firstRequestTime = '2018-01-01T00:00:00.000000+00:00'; + Carbon::setTestNow($firstRequestTime); + $firstRequest = $this->mockRequestWithLogMutatingFolder('first'); + self::assertSame('root/first', $firstRequest->getLogFolder()); + $firstRequest->sync(); + self::assertTrue($firstRequest->canBeFulfilledByCache()); + self::assertSame("root/first/{$firstRequestTime}", $firstRequest->getLastLogFile()); + + $secondRequestTime = '2018-01-01T00:00:00.000000+00:00'; + Carbon::setTestNow($secondRequestTime); + $secondRequest = $this->mockRequestWithLogMutatingFolder('second'); + self::assertSame('root/second', $secondRequest->getLogFolder()); + $secondRequest->sync(); + self::assertTrue(getProperty($secondRequest, 'responseIsFromCache')); + self::assertSame("root/first/{$firstRequestTime}", $secondRequest->getLastLogFile(), "The log should be from the first request's folder and time"); + } + + /** + * In this request class, the log folder uses a value that is not present in the cache key + */ + protected function mockRequestWithLogMutatingFolder(string $subfolder): ConcreteRequest + { + return new class($subfolder) extends ConcreteRequest { + use ParseResponseJSON; + public function __construct(public string $subfolder) + { + } + + protected bool $shouldLog = true; + + public function getLogFolder(): string + { + return 'root/' . $this->subfolder; + } + }; + } + public function testDontCacheErrorStatus(): void { $firstRequest = $this->mockRequestWithPostProcessor(); @@ -792,7 +835,7 @@ public function testCacheStructureCanary(): void 3 => '1.1', 4 => 'OK', ], $cached['response']); - self::assertSame([ "2018-01-01T00:00:00.000000+00:00" ], $cached['logs']); + self::assertSame([ "one/two/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); From 28d2539ec7713ab5fba23cfe7eee95f5b7c584eb Mon Sep 17 00:00:00 2001 From: Jeremy Wadhams Date: Fri, 28 Feb 2025 13:17:52 -0600 Subject: [PATCH 2/2] fix: Shim in the old behavior, you monster --- app/AbstractRequest.php | 6 ++++- tests/Feature/AbstractRequestTest.php | 35 +++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/app/AbstractRequest.php b/app/AbstractRequest.php index fc2de5d..cb83e41 100644 --- a/app/AbstractRequest.php +++ b/app/AbstractRequest.php @@ -365,7 +365,11 @@ protected function responseFromCache(): ?Response // 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']; + $this->sentLogs = array_map( + fn ($filename) => Str::contains($filename, '/') + ? $filename + : Str::finish($this->getLogFolder(), '/') . $filename, + $fromCache['logs']); return new Response(...$fromCache['response']); } return null; diff --git a/tests/Feature/AbstractRequestTest.php b/tests/Feature/AbstractRequestTest.php index 1f16ea4..b2e63e3 100644 --- a/tests/Feature/AbstractRequestTest.php +++ b/tests/Feature/AbstractRequestTest.php @@ -840,4 +840,39 @@ public function testCacheStructureCanary(): void // 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); } + + /** + * https://github.com/carsdotcom/php-request-class/issues/19 + * Shim for cached results <=1.4.0 that have a filename but no folder + * This won't work as well as modern folder-inclusive storage, + * but it's consistent with the old behavior, so better than nothing + */ + public function testCacheLogFilesNotFolders(): void + { + $firstLogTime = '2018-01-01T00:00:00.000000+00:00'; + Carbon::setTestNow($firstLogTime); + + $this->mockGuzzleWithTapper()->addMatchBody('POST', '/awesome/', '{"awesome":"sauce"}'); + $request = $this->mockRequestWithLog(); + Cache::tags([])->put($request->cacheKey(), [ + 'logs' => [ "2018-01-01T00:00:00.000000+00:00" ], + 'response' => [ + 0 => 200, + 1 => [], + 2 => '{"awesome":"sauce"}', + 3 => '1.1', + 4 => 'OK', + ] + ]); + + $request->setReadCache(true)->sync(); + self::assertTrue($request->isFromCache()); + + self::assertSame( + [ "one/two/2018-01-01T00:00:00.000000+00:00" ], + getProperty($request, 'sentLogs'), + "Shim is compartmentalized to cache rehydration, private prop is set in the standard way" + ); + self::assertSame("one/two/2018-01-01T00:00:00.000000+00:00" , $request->getLastLogFile()); + } }