fix(supplier): Plan 3 Task 4 code-review fixes (4 Important defense-in-depth)
Закрывает 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.
This commit is contained in:
@@ -14,12 +14,16 @@ use Illuminate\Queue\SerializesModels;
|
|||||||
* Plan 3 Task 4 stub. Полная реализация — Task 5 (PlaywrightBridge integration).
|
* Plan 3 Task 4 stub. Полная реализация — Task 5 (PlaywrightBridge integration).
|
||||||
* До Task 5 dispatch_sync(RefreshSupplierSessionJob) — noop (handle() пустой).
|
* До Task 5 dispatch_sync(RefreshSupplierSessionJob) — noop (handle() пустой).
|
||||||
*/
|
*/
|
||||||
final class RefreshSupplierSessionJob implements ShouldQueue
|
class RefreshSupplierSessionJob implements ShouldQueue
|
||||||
{
|
{
|
||||||
use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;
|
use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;
|
||||||
|
|
||||||
public function handle(): void
|
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);'
|
||||||
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -14,6 +14,12 @@ use App\Models\SupplierProject;
|
|||||||
*/
|
*/
|
||||||
final readonly class SupplierProjectDto
|
final readonly class SupplierProjectDto
|
||||||
{
|
{
|
||||||
|
/** @var array<int, int> */
|
||||||
|
public array $workdays;
|
||||||
|
|
||||||
|
/** @var array<int, int|string> */
|
||||||
|
public array $regions;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @param array<int, int> $workdays [1,2,3,4,5,6,7] (Пн..Вс)
|
* @param array<int, int> $workdays [1,2,3,4,5,6,7] (Пн..Вс)
|
||||||
* @param array<int, int|string> $regions массив кодов регионов РФ
|
* @param array<int, int|string> $regions массив кодов регионов РФ
|
||||||
@@ -23,11 +29,21 @@ final readonly class SupplierProjectDto
|
|||||||
public string $signalType, // site / call / sms
|
public string $signalType, // site / call / sms
|
||||||
public string $uniqueKey, // domain / phone / sender+keyword / sender
|
public string $uniqueKey, // domain / phone / sender+keyword / sender
|
||||||
public int $limit, // daily quota
|
public int $limit, // daily quota
|
||||||
public array $workdays,
|
array $workdays,
|
||||||
public array $regions,
|
array $regions,
|
||||||
public bool $regionsReverse, // false = include (default), true = exclude
|
public bool $regionsReverse, // false = include (default), true = exclude
|
||||||
public string $status, // active / paused
|
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
|
public static function fromModel(SupplierProject $sp): self
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -72,7 +72,12 @@ final class SupplierPortalClient
|
|||||||
$session = $this->loadSession();
|
$session = $this->loadSession();
|
||||||
$portalUrl = (string) config('services.supplier.portal_url');
|
$portalUrl = (string) config('services.supplier.portal_url');
|
||||||
$url = rtrim($portalUrl, '/').$path;
|
$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
|
$request = $this->http
|
||||||
->withCookies(['PHPSESSID' => $session['phpsessid']], $host)
|
->withCookies(['PHPSESSID' => $session['phpsessid']], $host)
|
||||||
@@ -100,7 +105,15 @@ final class SupplierPortalClient
|
|||||||
responseBody: $response->body(),
|
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);
|
return $this->request($method, $path, $body, isRetry: true);
|
||||||
}
|
}
|
||||||
@@ -133,12 +146,19 @@ final class SupplierPortalClient
|
|||||||
$session = Cache::store('redis')->get('supplier:session');
|
$session = Cache::store('redis')->get('supplier:session');
|
||||||
|
|
||||||
if ($session === null || ! isset($session['phpsessid'], $session['csrf'])) {
|
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 */
|
/** @var array{phpsessid?: string, csrf?: string, refreshed_at?: string}|null $session */
|
||||||
$session = Cache::store('redis')->get('supplier:session');
|
$session = Cache::store('redis')->get('supplier:session');
|
||||||
|
|
||||||
if ($session === null || ! isset($session['phpsessid'], $session['csrf'])) {
|
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');
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,31 @@
|
|||||||
|
<?php
|
||||||
|
|
||||||
|
declare(strict_types=1);
|
||||||
|
|
||||||
|
namespace Tests\Unit\Supplier\Stubs;
|
||||||
|
|
||||||
|
use App\Jobs\Supplier\RefreshSupplierSessionJob;
|
||||||
|
use RuntimeException;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Test stub: simulates Task 5 Playwright failure by throwing a deterministic
|
||||||
|
* RuntimeException from handle(). Used in SupplierPortalClientTest to verify
|
||||||
|
* SupplierPortalClient correctly translates raw exceptions from the refresh
|
||||||
|
* job into typed SupplierAuthException (preserves error taxonomy).
|
||||||
|
*
|
||||||
|
* Cannot be an anonymous class because Laravel's dispatchSync serializes the
|
||||||
|
* job (SerializesModels trait), and anonymous classes cannot be serialized.
|
||||||
|
*/
|
||||||
|
class ThrowingRefreshSupplierSessionJob extends RefreshSupplierSessionJob
|
||||||
|
{
|
||||||
|
public function __construct(
|
||||||
|
public readonly string $simulatedMessage,
|
||||||
|
) {
|
||||||
|
//
|
||||||
|
}
|
||||||
|
|
||||||
|
public function handle(): void
|
||||||
|
{
|
||||||
|
throw new RuntimeException($this->simulatedMessage);
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -13,6 +13,7 @@ use Illuminate\Support\Facades\Bus;
|
|||||||
use Illuminate\Support\Facades\Cache;
|
use Illuminate\Support\Facades\Cache;
|
||||||
use Illuminate\Support\Facades\Http;
|
use Illuminate\Support\Facades\Http;
|
||||||
use Tests\TestCase;
|
use Tests\TestCase;
|
||||||
|
use Tests\Unit\Supplier\Stubs\ThrowingRefreshSupplierSessionJob;
|
||||||
|
|
||||||
uses(TestCase::class);
|
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')
|
&& str_ends_with($r->url(), '/admin/rt-project-delete')
|
||||||
&& ((int) ($r['id'] ?? 0)) === 12345);
|
&& ((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');
|
||||||
|
});
|
||||||
|
|||||||
@@ -0,0 +1,59 @@
|
|||||||
|
<?php
|
||||||
|
|
||||||
|
declare(strict_types=1);
|
||||||
|
|
||||||
|
use App\Services\Supplier\Dto\SupplierProjectDto;
|
||||||
|
use Tests\TestCase;
|
||||||
|
|
||||||
|
uses(TestCase::class);
|
||||||
|
|
||||||
|
test('SupplierProjectDto::equals returns true for same fields regardless of workdays/regions input order', function (): void {
|
||||||
|
$dto1 = new SupplierProjectDto(
|
||||||
|
platform: 'B1',
|
||||||
|
signalType: 'site',
|
||||||
|
uniqueKey: 'a.com',
|
||||||
|
limit: 5,
|
||||||
|
workdays: [1, 2, 3],
|
||||||
|
regions: [50, 77],
|
||||||
|
regionsReverse: false,
|
||||||
|
status: 'active',
|
||||||
|
);
|
||||||
|
$dto2 = new SupplierProjectDto(
|
||||||
|
platform: 'B1',
|
||||||
|
signalType: 'site',
|
||||||
|
uniqueKey: 'a.com',
|
||||||
|
limit: 5,
|
||||||
|
workdays: [3, 1, 2], // same set, different order
|
||||||
|
regions: [77, 50], // same set, different order
|
||||||
|
regionsReverse: false,
|
||||||
|
status: 'active',
|
||||||
|
);
|
||||||
|
|
||||||
|
expect($dto1->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();
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user