From 16527358a2c534e429b033991b6af778db8129d0 Mon Sep 17 00:00:00 2001 From: Craig Potter Date: Sat, 18 Jul 2026 09:06:21 +0100 Subject: [PATCH] Fix five recording correctness bugs - Store request/response timestamps on the PendingRequest instead of the shared connector config, so concurrent requests through one connector no longer corrupt each other's durations - Use updateOrCreate for queued REQUEST jobs so a retry after a committed insert cannot create a duplicate row (matches the documented idempotency behaviour) - Match excluded request headers case-insensitively so `authorization` cannot slip past the `Authorization` exclusion, and find the response Content-Type header whatever its casing - Default keep_for_days to 30 when the config key is missing or null, instead of 0 which made model:prune delete every recording - Guard recordFatal() against a missing X-Barstool-UUID header instead of throwing a TypeError when recording an unrecorded request's fatal --- src/Barstool.php | 25 +++++-- src/BarstoolServiceProvider.php | 4 +- src/Jobs/RecordBarstoolJob.php | 10 +-- src/Models/Barstool.php | 2 +- tests/BarstoolTest.php | 113 ++++++++++++++++++++++++++++++++ 5 files changed, 143 insertions(+), 11 deletions(-) diff --git a/src/Barstool.php b/src/Barstool.php index 66e9de3..a4f95d2 100755 --- a/src/Barstool.php +++ b/src/Barstool.php @@ -239,7 +239,11 @@ private static function recordResponse(Response $data): void public static function calculateDuration(Response|PendingRequest $data): int { - $config = $data->getConnector()->config(); + // Timing lives on the PendingRequest, not the connector - connector config is + // shared between concurrent requests and the timestamps would overwrite each other. + $config = $data instanceof Response + ? $data->getPendingRequest()->config() + : $data->config(); $requestTime = (int) $config->get('barstool-request-time'); $responseTime = (int) $config->get('barstool-response-time', microtime(true) * 1000); @@ -250,7 +254,11 @@ public static function calculateDuration(Response|PendingRequest $data): int private static function recordFatal(FatalRequestException $data): void { $pendingRequest = $data->getPendingRequest(); + $uuid = $pendingRequest->headers()->get('X-Barstool-UUID'); + if (! is_string($uuid) || $uuid === '') { + return; + } $payload = [ 'duration' => self::calculateDuration($pendingRequest), @@ -324,8 +332,12 @@ public static function getRequestHeaders(PendingRequest $request): ?array return $headers->reject(fn ($value, $key) => $key !== 'X-Barstool-UUID')->toArray(); } + // Header names are matched case-insensitively so `authorization` cannot + // slip past an `Authorization` exclusion. + $excludedHeaders = array_map(mb_strtolower(...), $excludedHeaders); + return $headers->map(function ($value, $key) use ($excludedHeaders) { - if (in_array($key, $excludedHeaders)) { + if (in_array(mb_strtolower($key), $excludedHeaders)) { $value = 'REDACTED'; } @@ -359,9 +371,14 @@ public static function getResponseBody(Response $response): string return ''; } - $contentTypeHeaderKey = $response->headers()->get('Content-Type') ? 'Content-Type' : 'content-type'; + $contentType = collect($response->headers()->all()) + ->first(fn ($value, $key) => mb_strtolower($key) === 'content-type'); + + if (is_array($contentType)) { + $contentType = $contentType[0] ?? ''; + } - if (! Str::startsWith(mb_strtolower((string) $response->headers()->get($contentTypeHeaderKey)), self::supportedContentTypes())) { + if (! Str::startsWith(mb_strtolower((string) $contentType), self::supportedContentTypes())) { return ''; } diff --git a/src/BarstoolServiceProvider.php b/src/BarstoolServiceProvider.php index c80e4f1..310a02d 100644 --- a/src/BarstoolServiceProvider.php +++ b/src/BarstoolServiceProvider.php @@ -39,7 +39,7 @@ public function packageRegistered(): void return; } - $request->getConnector()->config()->add( + $request->config()->add( 'barstool-request-time', microtime(true) * 1000 ); @@ -51,7 +51,7 @@ public function packageRegistered(): void return; } - $response->getConnector()->config()->add( + $response->getPendingRequest()->config()->add( 'barstool-response-time', microtime(true) * 1000 ); diff --git a/src/Jobs/RecordBarstoolJob.php b/src/Jobs/RecordBarstoolJob.php index 5d09188..81205ed 100644 --- a/src/Jobs/RecordBarstoolJob.php +++ b/src/Jobs/RecordBarstoolJob.php @@ -38,11 +38,13 @@ public function uniqueId(): string public function handle(): void { if ($this->type === RecordingType::REQUEST) { + // updateOrCreate keeps retries idempotent - a job that fails after its + // insert committed must not create a duplicate row on the next attempt. Barstool::query() - ->create([ - 'uuid' => $this->uuid, - ...$this->data, - ]); + ->updateOrCreate( + ['uuid' => $this->uuid], + $this->data, + ); return; } diff --git a/src/Models/Barstool.php b/src/Models/Barstool.php index a0eb3b3..082a6be 100644 --- a/src/Models/Barstool.php +++ b/src/Models/Barstool.php @@ -70,7 +70,7 @@ public function prunable(): EloquentBuilder ->where( 'created_at', '<=', - now()->subDays(config('barstool.keep_for_days', 0)) + now()->subDays(config('barstool.keep_for_days') ?? 30) ); } } diff --git a/tests/BarstoolTest.php b/tests/BarstoolTest.php index 2e85294..29abd73 100644 --- a/tests/BarstoolTest.php +++ b/tests/BarstoolTest.php @@ -1005,3 +1005,116 @@ assertDatabaseCount('barstools', 2); }); + +it('tracks duration per request even when a connector is shared', function () { + config()->set('barstool.enabled', true); + + $connector = new RandomConnector; + + $first = $connector->createPendingRequest(new RequestWithConnector); + $second = $connector->createPendingRequest(new RequestWithConnector); + + // Two in-flight requests on the same connector with overlapping lifetimes + $first->config()->add('barstool-request-time', 1000); + $second->config()->add('barstool-request-time', 1200); + $first->config()->add('barstool-response-time', 1500); + $second->config()->add('barstool-response-time', 1300); + + expect(BarstoolRecorder::calculateDuration($first))->toBe(500); + expect(BarstoolRecorder::calculateDuration($second))->toBe(100); +}); + +it('does not pollute the connector config with timing keys', function () { + config()->set('barstool.enabled', true); + + MockClient::global([ + SoloUserRequest::class => MockResponse::make(body: ['data' => 'ok'], status: 200), + ]); + + (new SoloUserRequest)->send(); + + $barstool = Barstool::sole(); + expect($barstool->duration)->not->toBeNull(); +}); + +it('does not create duplicate rows when a queued request job is retried', function () { + $uuid = (string) Str::uuid(); + + $job = new RecordBarstoolJob(RecordingType::REQUEST, [ + 'connector_class' => NullConnector::class, + 'request_class' => SoloUserRequest::class, + 'method' => 'GET', + 'url' => 'https://tests.saloon.dev/api/user', + 'successful' => false, + ], $uuid); + + $job->handle(); + $job->handle(); + + assertDatabaseCount('barstools', 1); +}); + +it('redacts excluded headers regardless of casing', function () { + config()->set('barstool.enabled', true); + config()->set('barstool.excluded_request_headers', ['Authorization']); + + MockClient::global([ + SoloUserRequest::class => MockResponse::make(body: ['data' => 'ok'], status: 200), + ]); + + $request = new SoloUserRequest; + $request->headers()->add('authorization', 'Bearer super-secret'); + $request->headers()->add('AUTHORIZATION-ISH', 'not-excluded'); + $request->send(); + + $barstool = Barstool::sole(); + + expect($barstool->request_headers['authorization'])->toBe('REDACTED'); + expect($barstool->request_headers['AUTHORIZATION-ISH'])->toBe('not-excluded'); +}); + +it('stores response bodies whatever the content-type header casing', function () { + config()->set('barstool.enabled', true); + + MockClient::global([ + SoloUserRequest::class => MockResponse::make( + body: ['data' => 'ok'], + status: 200, + headers: ['CONTENT-TYPE' => 'application/json'], + ), + ]); + + (new SoloUserRequest)->send(); + + $barstool = Barstool::sole(); + expect($barstool->response_body)->toBe(json_encode(['data' => 'ok'])); +}); + +it('keeps recordings for 30 days when the keep_for_days config key is missing', function () { + config()->offsetUnset('barstool.keep_for_days'); + expect(config('barstool.keep_for_days'))->toBeNull(); + + $this->travel(-10)->days(); + Barstool::factory()->count(2)->create(); + + $this->travel(-25)->days(); + Barstool::factory()->count(3)->create(); + + $this->travelBack(); + Artisan::call('model:prune', ['--model' => [Barstool::class]]); + + // Without the safe default, everything would have been pruned + assertDatabaseCount('barstools', 2); +}); + +it('ignores a fatal exception for a request that was never recorded', function () { + // Recording disabled while the request is built, so no UUID header is added + config()->set('barstool.enabled', false); + $pendingRequest = (new SoloUserRequest)->createPendingRequest(); + + config()->set('barstool.enabled', true); + + BarstoolRecorder::record(new FatalRequestException(new Exception('boom'), $pendingRequest)); + + assertDatabaseCount('barstools', 0); +});