diff --git a/app/Models/Asset.php b/app/Models/Asset.php index 23b24eaad9..e623930c59 100644 --- a/app/Models/Asset.php +++ b/app/Models/Asset.php @@ -1481,7 +1481,7 @@ class Asset extends Depreciable public function scopePending($query) { // Pluck IDs then whereIn — do NOT replace with whereHas. whereHas generates a correlated EXISTS per row and causes severe slowdowns in withCount contexts. - $ids = Statuslabel::where('deployable', 0)->where('pending', 1)->where('archived', 0)->whereNull('deleted_at')->pluck('id'); + $ids = Statuslabel::idsFor('pending'); return $query->whereIn('assets.status_id', $ids->isEmpty() ? [0] : $ids); } @@ -1534,7 +1534,7 @@ class Asset extends Depreciable public function scopeRTD($query) { // Pluck IDs then whereIn — do NOT replace with whereHas. whereHas generates a correlated EXISTS per row and causes severe slowdowns in withCount contexts. - $ids = Statuslabel::where('deployable', 1)->where('pending', 0)->where('archived', 0)->whereNull('deleted_at')->pluck('id'); + $ids = Statuslabel::idsFor('deployable'); return $query->whereNull('assets.assigned_to') ->whereIn('assets.status_id', $ids->isEmpty() ? [0] : $ids); @@ -1549,7 +1549,7 @@ class Asset extends Depreciable public function scopeUndeployable($query) { // Pluck IDs then whereIn — do NOT replace with whereHas. whereHas generates a correlated EXISTS per row and causes severe slowdowns in withCount contexts. - $ids = Statuslabel::where('deployable', 0)->where('pending', 0)->where('archived', 0)->whereNull('deleted_at')->pluck('id'); + $ids = Statuslabel::idsFor('undeployable'); return $query->whereIn('assets.status_id', $ids->isEmpty() ? [0] : $ids); } @@ -1563,7 +1563,7 @@ class Asset extends Depreciable public function scopeNotArchived($query) { // Pluck IDs then whereIn — do NOT replace with whereHas. whereHas generates a correlated EXISTS per row and causes severe slowdowns in withCount contexts. - $ids = Statuslabel::where('archived', 0)->whereNull('deleted_at')->pluck('id'); + $ids = Statuslabel::idsFor('not_archived'); return $query->whereIn('assets.status_id', $ids->isEmpty() ? [0] : $ids); } @@ -1730,9 +1730,7 @@ class Asset extends Depreciable { // Pluck IDs then whereIn — do NOT replace with whereHas. whereHas generates a correlated EXISTS per row and causes severe slowdowns in withCount contexts. if (Setting::getSettings()->show_archived_in_list != 1) { - $validStatusIds = Statuslabel::where('archived', 0) - ->whereNull('deleted_at') - ->pluck('id'); + $validStatusIds = Statuslabel::idsFor('not_archived'); return $query->whereIn('assets.status_id', $validStatusIds->isEmpty() ? [0] : $validStatusIds); } @@ -1749,7 +1747,7 @@ class Asset extends Depreciable public function scopeArchived($query) { // Pluck IDs then whereIn — do NOT replace with whereHas. whereHas generates a correlated EXISTS per row and causes severe slowdowns in withCount contexts. - $ids = Statuslabel::where('deployable', 0)->where('pending', 0)->where('archived', 1)->whereNull('deleted_at')->pluck('id'); + $ids = Statuslabel::idsFor('archived'); return $query->whereIn('assets.status_id', $ids->isEmpty() ? [0] : $ids); } diff --git a/app/Models/AssetModel.php b/app/Models/AssetModel.php index 2bd5792cae..31d1474e86 100755 --- a/app/Models/AssetModel.php +++ b/app/Models/AssetModel.php @@ -160,11 +160,17 @@ class AssetModel extends SnipeModel public function percentRemaining() { - if ($this->availableAssets()->count() == 0) { + // Cache the available count locally — the old code called + // availableAssets()->count() twice, which under the API transformer + // loop showed up as a duplicated `select count(*) … assets where + // model_id = ?` for every model in the response (visible in any + // /api/v1/models?category_id=… query log). + $available = $this->availableAssets()->count(); + if ($available === 0) { return 0; } - return $this->availableAssets()->count() / $this->assets()->count() * 100; + return $available / $this->assets()->count() * 100; } /** diff --git a/app/Models/Statuslabel.php b/app/Models/Statuslabel.php index c0a30711f4..d5fedc4dce 100755 --- a/app/Models/Statuslabel.php +++ b/app/Models/Statuslabel.php @@ -10,6 +10,7 @@ use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Relations\Relation; use Illuminate\Database\Eloquent\SoftDeletes; use Illuminate\Database\Query\Builder; +use Illuminate\Support\Collection; use Illuminate\Support\Facades\Gate; use Watson\Validating\ValidatingTrait; @@ -47,6 +48,51 @@ class Statuslabel extends SnipeModel use Searchable; + /** + * Per-request memo of the ID lists used by Asset's status-driven scopes + * (RTD / Pending / Undeployable / Archived / NotArchived). Each scope + * fired a separate `SELECT id FROM status_labels WHERE …` query on every + * call before this was introduced, which under withCount + transformer + * loops (see AssetModelsTransformer) added dozens of redundant queries + * per page or API hit. + * + * Cleared on save/delete via model events below, and reset between tests + * in InitializesSettings (alongside Setting::$_cache). + */ + protected static array $statusIdCache = []; + + /** + * Return the cached set of status_label IDs matching the requested + * "kind" — one of: deployable, pending, undeployable, archived, + * not_archived. Each call after the first in a single request reads + * from memory. + */ + public static function idsFor(string $type): Collection + { + return self::$statusIdCache[$type] ??= match ($type) { + 'deployable' => self::where('deployable', 1)->where('pending', 0)->where('archived', 0)->whereNull('deleted_at')->pluck('id'), + 'pending' => self::where('deployable', 0)->where('pending', 1)->where('archived', 0)->whereNull('deleted_at')->pluck('id'), + 'undeployable' => self::where('deployable', 0)->where('pending', 0)->where('archived', 0)->whereNull('deleted_at')->pluck('id'), + 'archived' => self::where('deployable', 0)->where('pending', 0)->where('archived', 1)->whereNull('deleted_at')->pluck('id'), + 'not_archived' => self::where('archived', 0)->whereNull('deleted_at')->pluck('id'), + default => throw new \InvalidArgumentException('Unknown status type: '.$type), + }; + } + + public static function clearIdCache(): void + { + self::$statusIdCache = []; + } + + protected static function booted(): void + { + // Any mutation to a status label invalidates the cached ID lists. + $invalidate = fn () => self::clearIdCache(); + static::saved($invalidate); + static::deleted($invalidate); + static::restored($invalidate); + } + /** * The attributes that should be included when searching the model. * diff --git a/resources/views/blade/box/index.blade.php b/resources/views/blade/box/index.blade.php index f0e1976920..8be64fb1e0 100644 --- a/resources/views/blade/box/index.blade.php +++ b/resources/views/blade/box/index.blade.php @@ -31,9 +31,10 @@ @endif - @if (($slot) && (!$slot->isEmpty())) - {{ $slot }} - @endif + {{-- Render slot unconditionally — ComponentSlot::isEmpty() + materializes the slot to inspect it, doubling every DB call + inside. An empty slot renders nothing visible. --}} + {{ $slot }} diff --git a/resources/views/blade/tabs/index.blade.php b/resources/views/blade/tabs/index.blade.php index 77591a7a0b..a40ec24cde 100644 --- a/resources/views/blade/tabs/index.blade.php +++ b/resources/views/blade/tabs/index.blade.php @@ -6,16 +6,17 @@ @endif - @if ((isset($content)) && (!$content->isEmpty())) + {{-- Render slots unconditionally — ComponentSlot::isEmpty() + materializes the slot to inspect it, doubling every DB call + inside (asset/model counts, presenter dataTableLayout, etc.). + `isset($content)` is fine because it's a named slot check that + doesn't render anything. --}} + @isset($content) {{ $content }} - @endif + @endisset - @if (($slot) && (!$slot->isEmpty())) - {{ $slot }} - @endif + {{ $slot }} diff --git a/resources/views/categories/view.blade.php b/resources/views/categories/view.blade.php index a4a4687d48..5e0c03d1c1 100644 --- a/resources/views/categories/view.blade.php +++ b/resources/views/categories/view.blade.php @@ -20,15 +20,22 @@ @if ($category->category_type=='asset') - + {{-- Use the relation method (->models()) — property access (->models) hydrates + the whole AssetModel collection just to take ->count(), and on a category + with hundreds of models it's a notable allocator on the shell render. --}} + @elseif ($category->category_type=='accessory') - + {{-- Method-style ->accessories()->count() so we issue a + SELECT count(*) instead of hydrating the full + collection just to read its size. Same rationale + as the ->models() call above. --}} + @elseif ($category->category_type=='license') - + @elseif ($category->category_type=='consumable') - + @elseif ($category->category_type=='component') - + @endif diff --git a/tests/Support/InitializesSettings.php b/tests/Support/InitializesSettings.php index ee55a86f9d..e60117ded8 100644 --- a/tests/Support/InitializesSettings.php +++ b/tests/Support/InitializesSettings.php @@ -3,6 +3,7 @@ namespace Tests\Support; use App\Models\Setting; +use App\Models\Statuslabel; trait InitializesSettings { @@ -13,5 +14,10 @@ trait InitializesSettings $this->settings = Settings::initialize(); $this->beforeApplicationDestroyed(fn () => Setting::$_cache = null); + // Same idea as Setting::$_cache — a per-request memo lives across + // tests if we don't explicitly reset it. Without this, the second + // test in a file would see status_label IDs from the *previous* + // test's transactional rows (which have since rolled back). + $this->beforeApplicationDestroyed(fn () => Statuslabel::clearIdCache()); } } diff --git a/tests/Unit/AssetModelTest.php b/tests/Unit/AssetModelTest.php index eb5a8341ba..a5e718b0e5 100644 --- a/tests/Unit/AssetModelTest.php +++ b/tests/Unit/AssetModelTest.php @@ -5,6 +5,7 @@ namespace Tests\Unit; use App\Models\Asset; use App\Models\AssetModel; use App\Models\Category; +use Illuminate\Support\Facades\DB; use Tests\TestCase; class AssetModelTest extends TestCase @@ -84,6 +85,35 @@ class AssetModelTest extends TestCase $this->assertEquals(40.0, $model->percentRemaining()); } + public function test_percent_remaining_only_calls_available_assets_count_once(): void + { + // Regression: the old implementation called $this->availableAssets() + // ->count() twice — once for the zero guard, once for the ratio. + // Under the API transformer loop in /api/v1/models that doubled the + // status-label pluck + count(*) queries for every row. Pin the + // single-call contract so a future cleanup doesn't reintroduce it. + $category = Category::factory()->create(['category_type' => 'asset']); + $model = AssetModel::factory()->create(['category_id' => $category->id]); + Asset::factory()->count(2)->create(['model_id' => $model->id]); + + $availableCountQueries = 0; + DB::listen(function ($query) use (&$availableCountQueries, $model) { + // The availableAssets() relation generates a count(*) on assets + // filtered by model_id AND assigned_to IS NULL. + if (str_contains($query->sql, 'count(*)') + && str_contains($query->sql, '"assets"') + && str_contains($query->sql, '"assigned_to" is null') + && in_array($model->id, $query->bindings, true) + ) { + $availableCountQueries++; + } + }); + + $model->percentRemaining(); + + $this->assertSame(1, $availableCountQueries, 'availableAssets()->count() must be called exactly once per percentRemaining()'); + } + public function test_percent_remaining_returns_one_hundred_when_all_assets_are_available() { $model = new class extends AssetModel diff --git a/tests/Unit/StatuslabelTest.php b/tests/Unit/StatuslabelTest.php index 55e5b135c8..39322bf48e 100644 --- a/tests/Unit/StatuslabelTest.php +++ b/tests/Unit/StatuslabelTest.php @@ -3,6 +3,7 @@ namespace Tests\Unit; use App\Models\Statuslabel; +use Illuminate\Support\Facades\DB; use Tests\TestCase; class StatuslabelTest extends TestCase @@ -42,4 +43,61 @@ class StatuslabelTest extends TestCase $statuslabel = Statuslabel::factory()->lost()->create(); $this->assertModelExists($statuslabel); } + + public function test_ids_for_returns_matching_status_labels_per_kind(): void + { + $rtd = Statuslabel::factory()->rtd()->create(); + $pending = Statuslabel::factory()->pending()->create(); + $archived = Statuslabel::factory()->archived()->create(); + + Statuslabel::clearIdCache(); + + $this->assertContains($rtd->id, Statuslabel::idsFor('deployable')->all()); + $this->assertNotContains($pending->id, Statuslabel::idsFor('deployable')->all()); + + $this->assertContains($pending->id, Statuslabel::idsFor('pending')->all()); + $this->assertNotContains($rtd->id, Statuslabel::idsFor('pending')->all()); + + $this->assertContains($archived->id, Statuslabel::idsFor('archived')->all()); + $this->assertContains($rtd->id, Statuslabel::idsFor('not_archived')->all(), 'RTD label is "not archived"'); + $this->assertNotContains($archived->id, Statuslabel::idsFor('not_archived')->all()); + } + + public function test_ids_for_memoizes_within_a_request(): void + { + // Each scope (RTD / Pending / Undeployable / Archived / NotArchived) + // used to fire a fresh `SELECT id FROM status_labels` every time it + // was called. Under the API transformer loops that became dozens of + // identical queries per request. The cache should collapse repeated + // calls to a single query. + Statuslabel::factory()->rtd()->create(); + Statuslabel::clearIdCache(); + + $queries = 0; + DB::listen(function ($q) use (&$queries) { + if (str_contains($q->sql, 'status_labels')) { + $queries++; + } + }); + + Statuslabel::idsFor('deployable'); + Statuslabel::idsFor('deployable'); + Statuslabel::idsFor('deployable'); + + $this->assertSame(1, $queries, 'idsFor should hit the DB once per kind per request'); + } + + public function test_ids_for_cache_invalidates_on_save(): void + { + $original = Statuslabel::factory()->rtd()->create(); + + Statuslabel::clearIdCache(); + $this->assertContains($original->id, Statuslabel::idsFor('deployable')->all()); + + // New deployable label after the cache was warmed — must show up + // because the saved event clears the cache. + $added = Statuslabel::factory()->rtd()->create(); + + $this->assertContains($added->id, Statuslabel::idsFor('deployable')->all()); + } }