From e242e7d7fc09892600e7e5f646e6af6d4b8ac723 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 18:15:36 +0300 Subject: [PATCH] fix(projects): Plan 5 Task 2 code-review fixes (2 Important + 2 Minor) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I-1/M-1: introduce resolvedSupplierProjects() private helper on Project model; rewrite aggregateSyncStatus(), aggregateLastSyncedAt(), getSupplierLinks() to read from eager-loaded supplierB1/B2/B3 relations instead of SupplierProject::find() — eliminates up to 120 SELECTs/page. I-2: aggregateLastSyncedAt() now uses sortBy(timestamp) instead of Collection::min() on Carbon objects (string-comparison was unreliable). M-2: add explanatory comment on intval+array_filter silent-drop behaviour in the ?ids batch-fetch path. M-3: new test — ?ids batch silently excludes foreign-tenant project IDs. M-4: new test — show returns 200 for archived project (read preserved). PHPStan baseline updated: 2 new test functions raise actingAs() count 7→9. Tests: 9/9 passed (33 assertions). Larastan: 0 errors. Co-Authored-By: Claude Sonnet 4.6 --- .../Controllers/Api/ProjectController.php | 11 +++- app/app/Models/Project.php | 61 ++++++++++++------- app/phpstan-baseline.neon | 2 +- .../Plan5/Projects/ProjectsListShowTest.php | 42 +++++++++++++ 4 files changed, 91 insertions(+), 25 deletions(-) diff --git a/app/app/Http/Controllers/Api/ProjectController.php b/app/app/Http/Controllers/Api/ProjectController.php index 2575207c..defa0af3 100644 --- a/app/app/Http/Controllers/Api/ProjectController.php +++ b/app/app/Http/Controllers/Api/ProjectController.php @@ -24,10 +24,15 @@ class ProjectController extends Controller /** GET /api/projects */ public function index(Request $request): JsonResponse { - $query = Project::query()->where('tenant_id', $request->user()->tenant_id); + $query = Project::query() + ->with(['supplierB1', 'supplierB2', 'supplierB3']) // eager-load to avoid N+1 in aggregation helpers + ->where('tenant_id', $request->user()->tenant_id); // Batch-fetch по ids — возвращает без пагинации (для dropdown'ов и т.п.) if ($ids = $request->query('ids')) { + // '?ids=' batch fetch. Non-numeric and zero values silently dropped via intval+filter + // (intval('abc')=0 → array_filter drops 0). Acceptable for a read-only dropdown: + // invalid input produces empty result, not 422. $idArray = array_filter(array_map('intval', explode(',', (string) $ids))); $items = $query->whereIn('id', $idArray)->get(); @@ -76,7 +81,9 @@ class ProjectController extends Controller /** GET /api/projects/{id} */ public function show(Request $request, int $id): JsonResponse { - $project = Project::where('tenant_id', $request->user()->tenant_id)->findOrFail($id); + $project = Project::with(['supplierB1', 'supplierB2', 'supplierB3']) // eager-load to avoid N+1 + ->where('tenant_id', $request->user()->tenant_id) + ->findOrFail($id); return response()->json(['data' => new ProjectResource($project)]); } diff --git a/app/app/Models/Project.php b/app/app/Models/Project.php index 7f1e1a15..a3f62469 100644 --- a/app/app/Models/Project.php +++ b/app/app/Models/Project.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Models; +use Carbon\CarbonInterface; use Database\Factories\ProjectFactory; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Factories\HasFactory; @@ -159,18 +160,34 @@ class Project extends Model return $query->whereNotNull('archived_at'); } + /** + * Все связанные SupplierProject из eager-loaded BelongsTo отношений. + * + * Используется внутри aggregateSyncStatus(), aggregateLastSyncedAt(), + * getSupplierLinks() — устраняет N+1 (каждый из трёх методов вызывал + * SupplierProject::find() независимо; теперь читает из уже загруженных + * $this->supplierB1 / supplierB2 / supplierB3). + * + * Требует eager-load: Project::with(['supplierB1', 'supplierB2', 'supplierB3']). + * + * @return Collection + */ + private function resolvedSupplierProjects(): Collection + { + return collect([$this->supplierB1, $this->supplierB2, $this->supplierB3])->filter()->values(); + } + /** * Агрегированный статус синхронизации по всем связанным SupplierProject. * * Логика: если нет ни одного — pending; если есть failed — failed; * если есть pending — pending; иначе — ok. + * + * Читает из eager-loaded отношений (см. resolvedSupplierProjects()). */ public function aggregateSyncStatus(): string { - /** @var Collection $statuses */ - $statuses = collect(['supplier_b1_project_id', 'supplier_b2_project_id', 'supplier_b3_project_id']) - ->map(fn (string $col) => $this->{$col} ? SupplierProject::find($this->{$col})?->sync_status : null) - ->filter(); + $statuses = $this->resolvedSupplierProjects()->pluck('sync_status'); if ($statuses->isEmpty()) { return 'pending'; @@ -187,13 +204,19 @@ class Project extends Model /** * Минимальная дата последней синхронизации по всем связанным SupplierProject. + * + * Использует sortBy по timestamp вместо Collection::min() на Carbon-объектах + * (min() сравнивает строковое представление, что ненадёжно для Carbon). + * + * Читает из eager-loaded отношений (см. resolvedSupplierProjects()). */ public function aggregateLastSyncedAt(): ?string { - $ts = collect(['supplier_b1_project_id', 'supplier_b2_project_id', 'supplier_b3_project_id']) - ->map(fn (string $col) => $this->{$col} ? SupplierProject::find($this->{$col})?->last_synced_at : null) + $ts = $this->resolvedSupplierProjects() + ->pluck('last_synced_at') ->filter() - ->min(); + ->sortBy(fn (CarbonInterface $c) => $c->timestamp) + ->first(); return $ts?->toIso8601String(); } @@ -201,26 +224,20 @@ class Project extends Model /** * Массив ссылок на связанные SupplierProject (для show endpoint). * + * Читает из eager-loaded отношений (см. resolvedSupplierProjects()). + * * @return array */ public function getSupplierLinks(): array { - return collect(['b1', 'b2', 'b3']) - ->map(function (string $p) { - $id = $this->{"supplier_{$p}_project_id"}; - if ($id === null) { - return null; - } - $sp = SupplierProject::find($id); - - return [ - 'platform' => $p, - 'supplier_project_id' => $id, - 'sync_status' => $sp?->sync_status, - 'last_synced_at' => $sp?->last_synced_at?->toIso8601String(), - ]; - }) + return collect(['b1' => $this->supplierB1, 'b2' => $this->supplierB2, 'b3' => $this->supplierB3]) ->filter() + ->map(fn (SupplierProject $sp, string $platform) => [ + 'platform' => $platform, + 'supplier_project_id' => $sp->id, + 'sync_status' => $sp->sync_status, + 'last_synced_at' => $sp->last_synced_at?->toIso8601String(), + ]) ->values() ->all(); } diff --git a/app/phpstan-baseline.neon b/app/phpstan-baseline.neon index 42823c0b..b258b924 100644 --- a/app/phpstan-baseline.neon +++ b/app/phpstan-baseline.neon @@ -861,7 +861,7 @@ parameters: - message: '#^Call to an undefined method Pest\\PendingCalls\\TestCall\:\:actingAs\(\)\.$#' identifier: method.notFound - count: 7 + count: 9 path: tests/Feature/Plan5/Projects/ProjectsListShowTest.php - diff --git a/app/tests/Feature/Plan5/Projects/ProjectsListShowTest.php b/app/tests/Feature/Plan5/Projects/ProjectsListShowTest.php index 9074ccec..e2302cda 100644 --- a/app/tests/Feature/Plan5/Projects/ProjectsListShowTest.php +++ b/app/tests/Feature/Plan5/Projects/ProjectsListShowTest.php @@ -87,3 +87,45 @@ it('show returns project with supplier_links array', function () { $response->assertOk(); $response->assertJsonStructure(['data' => ['id', 'name', 'supplier_links']]); }); + +it('?ids batch filters out projects from foreign tenants silently', function () { + $tenantA = Tenant::factory()->create(); + $tenantB = Tenant::factory()->create(); + $userA = User::factory()->create(['tenant_id' => $tenantA->id]); + $ownProject = Project::factory()->create([ + 'tenant_id' => $tenantA->id, + 'signal_type' => 'site', + 'signal_identifier' => 'own.ru', + ]); + $foreignProject = Project::factory()->create([ + 'tenant_id' => $tenantB->id, + 'signal_type' => 'site', + 'signal_identifier' => 'foreign.ru', + ]); + + $response = $this->actingAs($userA)->getJson( + "/api/projects?ids={$ownProject->id},{$foreignProject->id}" + ); + + $response->assertOk(); + $data = $response->json('data'); + expect(count($data))->toBe(1); + expect($data[0]['id'])->toBe($ownProject->id); +}); + +it('show returns 200 for archived project (read access preserved)', function () { + $tenant = Tenant::factory()->create(); + $user = User::factory()->create(['tenant_id' => $tenant->id]); + $project = Project::factory()->create([ + 'tenant_id' => $tenant->id, + 'archived_at' => now(), + 'signal_type' => 'site', + 'signal_identifier' => 'archived.ru', + ]); + + $response = $this->actingAs($user)->getJson("/api/projects/{$project->id}"); + + $response->assertOk(); + expect($response->json('data.id'))->toBe($project->id); + expect($response->json('data.archived_at'))->not->toBeNull(); +});