From a8a23cb269d2aa6dc2d302e5c7993c338787a6ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=94=D0=BC=D0=B8=D1=82=D1=80=D0=B8=D0=B9?= Date: Mon, 11 May 2026 02:02:50 +0300 Subject: [PATCH] fix(supplier): Plan 3 Task 4 code-review fixes (4 Important defense-in-depth) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Закрывает 4 Important issues из code-review Task 4 (a2c5374): - #1 SupplierPortalClient: parse_url host validation → SupplierClientException вместо silent cookie skip + false-positive SupplierAuthException - #2 dispatch_sync(RefreshSupplierSessionJob) обёрнут try/catch (request retry + loadSession) → raw exceptions translated в SupplierAuthException для consistency с error taxonomy перед Task 5 real Playwright impl - #3 RefreshSupplierSessionJob stub handle() теперь throws LogicException с понятным сообщением (вместо silent no-op → confusing 'cache still empty' error). После Task 5 — LogicException заменяется real Playwright code. Снят final-модификатор класса (test override через container bind + Laravel dispatchSync serialization не работает с anonymous classes). - #5 SupplierProjectDto::equals → canonical order для workdays/regions через sort в constructor (defense vs PG jsonb non-deterministic order). Без этого Task 6 SyncJob false-positive обнаруживал бы diff где его нет → unnecessary updateProject HTTP calls. +3 tests в SupplierPortalClientTest (malformed url, 2 retry-translation paths) +2 tests в новом SupplierProjectDtoTest (order-independent equals + non-equal) +1 stub-класс ThrowingRefreshSupplierSessionJob (anonymous classes несовместимы с SerializesModels trait в dispatchSync). Pest: 38/38 supplier-suite, 574/574 full suite (576 total, 2 skipped, +5 new tests vs Task 4 baseline). PHPStan 0 errors. Pint clean. --- .../Supplier/RefreshSupplierSessionJob.php | 8 ++- .../Supplier/Dto/SupplierProjectDto.php | 22 ++++++- .../Supplier/SupplierPortalClient.php | 28 +++++++-- .../ThrowingRefreshSupplierSessionJob.php | 31 ++++++++++ .../Supplier/SupplierPortalClientTest.php | 56 ++++++++++++++++++ .../Unit/Supplier/SupplierProjectDtoTest.php | 59 +++++++++++++++++++ 6 files changed, 195 insertions(+), 9 deletions(-) create mode 100644 app/tests/Unit/Supplier/Stubs/ThrowingRefreshSupplierSessionJob.php create mode 100644 app/tests/Unit/Supplier/SupplierProjectDtoTest.php diff --git a/app/app/Jobs/Supplier/RefreshSupplierSessionJob.php b/app/app/Jobs/Supplier/RefreshSupplierSessionJob.php index cdf1fe59..dabb0c0e 100644 --- a/app/app/Jobs/Supplier/RefreshSupplierSessionJob.php +++ b/app/app/Jobs/Supplier/RefreshSupplierSessionJob.php @@ -14,12 +14,16 @@ use Illuminate\Queue\SerializesModels; * Plan 3 Task 4 stub. Полная реализация — Task 5 (PlaywrightBridge integration). * До Task 5 dispatch_sync(RefreshSupplierSessionJob) — noop (handle() пустой). */ -final class RefreshSupplierSessionJob implements ShouldQueue +class RefreshSupplierSessionJob implements ShouldQueue { use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; public function handle(): void { - // Task 5 implementation + throw new \LogicException( + 'RefreshSupplierSessionJob stub: real implementation lands in Plan 3 Task 5. ' + .'Until then, manually seed supplier:session cache for local dev:'."\n" + .' Cache::store(\'redis\')->put(\'supplier:session\', [\'phpsessid\' => \'…\', \'csrf\' => \'…\'], 6 * 3600);' + ); } } diff --git a/app/app/Services/Supplier/Dto/SupplierProjectDto.php b/app/app/Services/Supplier/Dto/SupplierProjectDto.php index c7d74b95..4fe2ed3e 100644 --- a/app/app/Services/Supplier/Dto/SupplierProjectDto.php +++ b/app/app/Services/Supplier/Dto/SupplierProjectDto.php @@ -14,6 +14,12 @@ use App\Models\SupplierProject; */ final readonly class SupplierProjectDto { + /** @var array */ + public array $workdays; + + /** @var array */ + public array $regions; + /** * @param array $workdays [1,2,3,4,5,6,7] (Пн..Вс) * @param array $regions массив кодов регионов РФ @@ -23,11 +29,21 @@ final readonly class SupplierProjectDto public string $signalType, // site / call / sms public string $uniqueKey, // domain / phone / sender+keyword / sender public int $limit, // daily quota - public array $workdays, - public array $regions, + array $workdays, + array $regions, public bool $regionsReverse, // false = include (default), true = exclude public string $status, // active / paused - ) {} + ) { + // Canonical order for deterministic equals() vs PG jsonb non-deterministic order. + // sort() reorders in-place AND re-indexes keys 0..N-1 (PHP guarantees list-semantics). + $sortedWorkdays = $workdays; + sort($sortedWorkdays, SORT_NUMERIC); + $this->workdays = $sortedWorkdays; + + $sortedRegions = $regions; + sort($sortedRegions, SORT_NUMERIC); + $this->regions = $sortedRegions; + } public static function fromModel(SupplierProject $sp): self { diff --git a/app/app/Services/Supplier/SupplierPortalClient.php b/app/app/Services/Supplier/SupplierPortalClient.php index 11ee29be..59a80724 100644 --- a/app/app/Services/Supplier/SupplierPortalClient.php +++ b/app/app/Services/Supplier/SupplierPortalClient.php @@ -72,7 +72,12 @@ final class SupplierPortalClient $session = $this->loadSession(); $portalUrl = (string) config('services.supplier.portal_url'); $url = rtrim($portalUrl, '/').$path; - $host = (string) parse_url($url, PHP_URL_HOST); + $host = parse_url($url, PHP_URL_HOST); + if (! is_string($host) || $host === '') { + throw new SupplierClientException( + "Misconfigured services.supplier.portal_url: cannot parse host from '{$url}'", + ); + } $request = $this->http ->withCookies(['PHPSESSID' => $session['phpsessid']], $host) @@ -100,7 +105,15 @@ final class SupplierPortalClient responseBody: $response->body(), ); } - dispatch_sync(new RefreshSupplierSessionJob); + try { + dispatch_sync(app(RefreshSupplierSessionJob::class)); + } catch (\Throwable $e) { + throw new SupplierAuthException( + "Session refresh failed during retry on {$path}: {$e->getMessage()}", + httpStatus: $response->status(), + previous: $e, + ); + } return $this->request($method, $path, $body, isRetry: true); } @@ -133,12 +146,19 @@ final class SupplierPortalClient $session = Cache::store('redis')->get('supplier:session'); if ($session === null || ! isset($session['phpsessid'], $session['csrf'])) { - dispatch_sync(new RefreshSupplierSessionJob); + try { + dispatch_sync(app(RefreshSupplierSessionJob::class)); + } catch (\Throwable $e) { + throw new SupplierAuthException( + "Session refresh failed during loadSession: {$e->getMessage()}", + previous: $e, + ); + } /** @var array{phpsessid?: string, csrf?: string, refreshed_at?: string}|null $session */ $session = Cache::store('redis')->get('supplier:session'); if ($session === null || ! isset($session['phpsessid'], $session['csrf'])) { - throw new SupplierAuthException('Session refresh failed: cache still empty'); + throw new SupplierAuthException('Session refresh failed during loadSession: cache still empty'); } } diff --git a/app/tests/Unit/Supplier/Stubs/ThrowingRefreshSupplierSessionJob.php b/app/tests/Unit/Supplier/Stubs/ThrowingRefreshSupplierSessionJob.php new file mode 100644 index 00000000..69d477ce --- /dev/null +++ b/app/tests/Unit/Supplier/Stubs/ThrowingRefreshSupplierSessionJob.php @@ -0,0 +1,31 @@ +simulatedMessage); + } +} diff --git a/app/tests/Unit/Supplier/SupplierPortalClientTest.php b/app/tests/Unit/Supplier/SupplierPortalClientTest.php index 4c1ed191..2991b161 100644 --- a/app/tests/Unit/Supplier/SupplierPortalClientTest.php +++ b/app/tests/Unit/Supplier/SupplierPortalClientTest.php @@ -13,6 +13,7 @@ use Illuminate\Support\Facades\Bus; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Http; use Tests\TestCase; +use Tests\Unit\Supplier\Stubs\ThrowingRefreshSupplierSessionJob; uses(TestCase::class); @@ -145,3 +146,58 @@ test('deleteProject POSTs to /admin/rt-project-delete with id only', function () && str_ends_with($r->url(), '/admin/rt-project-delete') && ((int) ($r['id'] ?? 0)) === 12345); }); + +test('malformed portal_url throws SupplierClientException, not auth path', function (): void { + config(['services.supplier.portal_url' => '/no-scheme-no-host']); + Http::fake(); // не должен быть вызван + + expect(fn () => app(SupplierPortalClient::class)->listProjects()) + ->toThrow(SupplierClientException::class); +}); + +test('RefreshSupplierSessionJob throws during retry path translated to SupplierAuthException', function (): void { + // Initial session valid → first request goes through → 401 → triggers refresh sync + Http::fake(['crm.bp-gr.ru/*' => Http::response('Unauthorized', 401)]); + + // Override container binding — simulates Task 5 Playwright failure + app()->bind( + RefreshSupplierSessionJob::class, + fn () => new ThrowingRefreshSupplierSessionJob('Simulated Playwright crash during retry'), + ); + + $caught = null; + try { + app(SupplierPortalClient::class)->listProjects(); + } catch (SupplierAuthException $e) { + $caught = $e; + } + + expect($caught)->toBeInstanceOf(SupplierAuthException::class); + // Message MUST mention "refresh failed" — proves translation, not sticky-401 path + expect($caught->getMessage())->toContain('Session refresh failed') + ->and($caught->getPrevious())->toBeInstanceOf(RuntimeException::class) + ->and($caught->getPrevious()?->getMessage())->toBe('Simulated Playwright crash during retry'); +}); + +test('RefreshSupplierSessionJob throws during initial loadSession translated to SupplierAuthException', function (): void { + // Force loadSession path: clear cache so request() enters loadSession() refresh branch + Cache::store('redis')->forget('supplier:session'); + + app()->bind( + RefreshSupplierSessionJob::class, + fn () => new ThrowingRefreshSupplierSessionJob('Simulated Playwright crash during loadSession'), + ); + + $caught = null; + try { + app(SupplierPortalClient::class)->listProjects(); + } catch (SupplierAuthException $e) { + $caught = $e; + } + + expect($caught)->toBeInstanceOf(SupplierAuthException::class); + // Message MUST mention "loadSession" — proves translation from raw RuntimeException + expect($caught->getMessage())->toContain('loadSession') + ->and($caught->getPrevious())->toBeInstanceOf(RuntimeException::class) + ->and($caught->getPrevious()?->getMessage())->toBe('Simulated Playwright crash during loadSession'); +}); diff --git a/app/tests/Unit/Supplier/SupplierProjectDtoTest.php b/app/tests/Unit/Supplier/SupplierProjectDtoTest.php new file mode 100644 index 00000000..2fef7117 --- /dev/null +++ b/app/tests/Unit/Supplier/SupplierProjectDtoTest.php @@ -0,0 +1,59 @@ +equals($dto2))->toBeTrue() + ->and($dto2->equals($dto1))->toBeTrue(); // symmetry +}); + +test('SupplierProjectDto::equals returns false when limit differs', function (): void { + $dto1 = new SupplierProjectDto( + platform: 'B1', + signalType: 'site', + uniqueKey: 'a.com', + limit: 5, + workdays: [1, 2, 3], + regions: [], + regionsReverse: false, + status: 'active', + ); + $dto2 = new SupplierProjectDto( + platform: 'B1', + signalType: 'site', + uniqueKey: 'a.com', + limit: 6, // differs + workdays: [1, 2, 3], + regions: [], + regionsReverse: false, + status: 'active', + ); + + expect($dto1->equals($dto2))->toBeFalse(); +});