Skip to content

⚡ Optimize config version pruning to avoid N+1 queries #65

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Snider wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

⚡ Optimize config version pruning to avoid N+1 queries #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60eeca2
Optimize ConfigVersioning::pruneOldVersions to avoid N+1 queries
Snider Feb 2, 2026
d04bfd1
Fix CI SARIF/JSON generation and upgrade CodeQL action
Snider Feb 2, 2026
7b2eb6a
Sanitize Psalm SARIF output to fix invalid location coordinates
Snider Feb 2, 2026
90d54c2
Fix robust SARIF sanitization for Psalm output
Snider Feb 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions .github/workflows/qa.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ jobs:
id: stan
run: |
set +e
vendor/bin/phpstan analyse --error-format=json --no-progress > phpstan.json 2>&1
vendor/bin/phpstan analyse --error-format=json --no-progress > phpstan.json
EXIT_CODE=$?

# Also run for GitHub format
Expand Down Expand Up @@ -181,11 +181,16 @@ jobs:
id: psalm
run: |
set +e
vendor/bin/psalm --output-format=json --show-info=false > psalm.json 2>&1
vendor/bin/psalm --output-format=json --show-info=false > psalm.json
EXIT_CODE=$?

# Generate SARIF for GitHub Security
vendor/bin/psalm --output-format=sarif --show-info=false > psalm.sarif 2>&1 || true
vendor/bin/psalm --output-format=sarif --show-info=false > psalm.sarif || true

# Sanitize SARIF: Ensure all location coordinates are >= 1
if [ -f psalm.sarif ]; then
jq 'walk(if type == "object" then with_entries(if (.key | test("^(start|end)(Line|Column)$")) and (.value < 1) then .value = 1 else . end) else . end)' psalm.sarif > psalm.sarif.tmp && mv psalm.sarif.tmp psalm.sarif
fi

ERRORS=$(jq 'length' psalm.json 2>/dev/null || echo "0")
echo "errors=${ERRORS}" >> $GITHUB_OUTPUT
Expand All @@ -200,7 +205,7 @@ jobs:

- name: Upload Psalm SARIF
if: always()
uses: github/codeql-action/upload-sarif@v3
uses: github/codeql-action/upload-sarif@v4
with:
sarif_file: psalm.sarif
category: psalm
Expand Down Expand Up @@ -242,7 +247,7 @@ jobs:
id: fmt
run: |
set +e
OUTPUT=$(vendor/bin/pint --test --format=json 2>&1)
OUTPUT=$(vendor/bin/pint --test --format=json)
EXIT_CODE=$?
echo "$OUTPUT" > pint.json

Expand Down Expand Up @@ -292,7 +297,7 @@ jobs:
id: audit
run: |
set +e
composer audit --format=json > audit.json 2>&1
composer audit --format=json > audit.json
EXIT_CODE=$?

VULNS=$(jq '.advisories | to_entries | map(.value | length) | add // 0' audit.json 2>/dev/null || echo "0")
Expand Down
26 changes: 19 additions & 7 deletions src/Core/Config/ConfigVersioning.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,16 +295,28 @@ public function deleteVersion(int $versionId): void
*/
protected function pruneOldVersions(int $profileId): void
{
$versions = ConfigVersion::where('profile_id', $profileId)
// Find the oldest version we want to keep (the Nth newest)
$cutoff = ConfigVersion::where('profile_id', $profileId)
->orderByDesc('created_at')
->get();
->orderByDesc('id')
->skip($this->maxVersions - 1)
->take(1)
->first();
Comment on lines +298 to +304
Copy link

@coderabbitai coderabbitai bot Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Guard against maxVersions < 1 before computing the cutoff.

If configuration sets maxVersions to 0 or a negative value, skip($this->maxVersions - 1) becomes a negative offset and can yield invalid SQL. Clamping locally keeps pruning robust even with misconfiguration.

💡 Proposed fix 
-        $cutoff = ConfigVersion::where('profile_id', $profileId)
+        $maxVersions = max(1, $this->maxVersions);
+        $cutoff = ConfigVersion::where('profile_id', $profileId)
             ->orderByDesc('created_at')
             ->orderByDesc('id')
-            ->skip($this->maxVersions - 1)
+            ->skip($maxVersions - 1)
             ->take(1)
             ->first();
🤖 Prompt for AI Agents 
In `@src/Core/Config/ConfigVersioning.php` around lines 298 - 304, The code
computes a cutoff using skip($this->maxVersions - 1) which can become negative
if $this->maxVersions is 0 or less; clamp or validate $this->maxVersions before
building the query (e.g. $max = max(1, $this->maxVersions)) and then use
->skip($max - 1)->take(1)->first() when querying ConfigVersion so negative
offsets are never used; update any variable names (e.g. $cutoff calculation) to
use the clamped $max.


if ($versions->count() > $this->maxVersions) {
$toDelete = $versions->slice($this->maxVersions);
foreach ($toDelete as $version) {
$version->delete();
}
if ($cutoff === null) {
return;
}

// Delete versions older than the cutoff
ConfigVersion::where('profile_id', $profileId)
->where(function ($query) use ($cutoff) {
$query->where('created_at', '<', $cutoff->created_at)
->orWhere(function ($q) use ($cutoff) {
$q->where('created_at', $cutoff->created_at)
->where('id', '<', $cutoff->id);
});
Comment on lines +312 to +317
Copy link

@coderabbitai coderabbitai bot Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, check the actual file and the specific lines mentioned
echo "=== Checking file existence and content ===" 
if [ -f "src/Core/Config/ConfigVersioning.php" ]; then
    echo "File exists. Checking lines 310-320:"
    sed -n '310,320p' "src/Core/Config/ConfigVersioning.php"
else
    echo "File not found"
fi

Repository: host-uk/core-php

Length of output: 627

🏁 Script executed:

#!/bin/bash
# Search for existing patterns of where() with typed closures in src/Core
echo "=== Searching for query builder closure patterns ===" 
rg -n "where\(function" src/Core --type php -A 2 -B 1 | head -100

Repository: host-uk/core-php

Length of output: 5981

🏁 Script executed:

#!/bin/bash
# Check if Illuminate\Database\Query\Builder is already imported in the file
echo "=== Checking imports in ConfigVersioning.php ===" 
head -40 src/Core/Config/ConfigVersioning.php | grep -E "^use|^namespace"

Repository: host-uk/core-php

Length of output: 242

Add type hints to the nested query closures.

Both closures accept untyped parameters and have no return type, violating the coding guideline that "all parameters and return types must have type hints in PHP". Type-hint the builder parameter as Builder and declare a void return type on both closures.

Proposed fix 
+use Illuminate\Database\Query\Builder;
-            ->where(function ($query) use ($cutoff) {
+            ->where(function (Builder $query) use ($cutoff): void {
                 $query->where('created_at', '<', $cutoff->created_at)
-                    ->orWhere(function ($q) use ($cutoff) {
-                        $q->where('created_at', $cutoff->created_at)
+                    ->orWhere(function (Builder $q) use ($cutoff): void {
+                        $q->where('created_at', $cutoff->created_at)
                             ->where('id', '<', $cutoff->id);
                     });
             })
             ->delete();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
->where(function ($query) use ($cutoff) {
$query->where('created_at', '<', $cutoff->created_at)
->orWhere(function ($q) use ($cutoff) {
$q->where('created_at', $cutoff->created_at)
->where('id', '<', $cutoff->id);
});
->where(function (Builder $query) use ($cutoff): void {
$query->where('created_at', '<', $cutoff->created_at)
->orWhere(function (Builder $q) use ($cutoff): void {
$q->where('created_at', $cutoff->created_at)
->where('id', '<', $cutoff->id);
});
🤖 Prompt for AI Agents 
In `@src/Core/Config/ConfigVersioning.php` around lines 312 - 317, Type-hint the
nested query closures so their parameter is declared as Builder and they declare
a void return type: change the first closure's signature from function ($query)
use ($cutoff) to function (Builder $query) use ($cutoff): void and the inner
closure from function ($q) use ($cutoff) to function (Builder $q) use ($cutoff):
void; also ensure the appropriate Builder import (e.g.
Illuminate\Database\Query\Builder or the project's Builder alias) is added at
the top of ConfigVersioning.php.

})
->delete();
}

/**
Expand Down
77 changes: 77 additions & 0 deletions src/Core/Config/Tests/Feature/ConfigVersioningPerformanceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
<?php

namespace Core\Config\Tests\Feature;
Comment on lines +1 to +3
Copy link

@coderabbitai coderabbitai bot Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if file exists and read its content
if [ -f "src/Core/Config/Tests/Feature/ConfigVersioningPerformanceTest.php" ]; then
  echo "=== File exists. Reading content: ==="
  head -40 "src/Core/Config/Tests/Feature/ConfigVersioningPerformanceTest.php"
else
  echo "File not found"
fi

Repository: host-uk/core-php

Length of output: 1280

🏁 Script executed:

#!/bin/bash
# Check other files in the same directory for strict_types pattern
echo "=== Checking strict_types in same directory: ==="
find src/Core/Config/Tests/Feature -name "*.php" -type f | head -5 | while read f; do
  echo "--- $f ---"
  head -5 "$f"
done

Repository: host-uk/core-php

Length of output: 354

🏁 Script executed:

#!/bin/bash
# Find the test_pruning_performance method definition
sed -n '25,30p' src/Core/Config/Tests/Feature/ConfigVersioningPerformanceTest.php

Repository: host-uk/core-php

Length of output: 314

Add strict types declaration and type hint test method.

This file is missing declare(strict_types=1); at the top and the test_pruning_performance() method lacks a return type hint. Both are required per coding guidelines.

💡 Proposed fixes 
 <?php
+
+declare(strict_types=1);
 
 namespace Core\Config\Tests\Feature;
-    public function test_pruning_performance()
+    public function test_pruning_performance(): void
     {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<?php
namespace Core\Config\Tests\Feature;
<?php
declare(strict_types=1);
namespace Core\Config\Tests\Feature;
🤖 Prompt for AI Agents 
In `@src/Core/Config/Tests/Feature/ConfigVersioningPerformanceTest.php` around
lines 1 - 3, Add a strict types declaration at the top of the file and a return
type for the test method: insert declare(strict_types=1); immediately after the
opening <?php tag in the
Core\Config\Tests\Feature\ConfigVersioningPerformanceTest file, and change the
test method signature for test_pruning_performance() to include an explicit
return type (e.g., : void) so the method is type-hinted according to coding
guidelines.


use Core\Config\ConfigExporter;
use Core\Config\ConfigService;
use Core\Config\ConfigVersioning;
use Core\Config\Models\ConfigProfile;
use Core\Config\Models\ConfigVersion;
use Core\Tests\TestCase;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Illuminate\Support\Facades\Config;
use Mockery;

class ConfigVersioningPerformanceTest extends TestCase
{
use RefreshDatabase;

protected function setUp(): void
{
parent::setUp();
$this->loadMigrationsFrom(__DIR__.'/../../Migrations');
}

public function test_pruning_performance()
{
Comment on lines +25 to +26
Copy link

@coderabbitai coderabbitai bot Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n src/Core/Config/Tests/Feature/ConfigVersioningPerformanceTest.php | head -40

Repository: host-uk/core-php

Length of output: 1522

🏁 Script executed:

rg -n "function test_pruning_performance" src/Core/Config/Tests/Feature/ConfigVersioningPerformanceTest.php -A 5

Repository: host-uk/core-php

Length of output: 332

🏁 Script executed:

rg -n "public function test_" src/Core/Config/Tests/Feature/ConfigVersioningPerformanceTest.php

Repository: host-uk/core-php

Length of output: 109

🏁 Script executed:

rg -n "public function " src/Core/Config/Tests/Feature/ConfigVersioningPerformanceTest.php | head -20

Repository: host-uk/core-php

Length of output: 109

🏁 Script executed:

rg -n "public function " src/Core/Config/Tests/Feature/ConfigVersioningPerformanceTest.php -A 1

Repository: host-uk/core-php

Length of output: 118

🏁 Script executed:

# Check other test files to see if test methods have return types
rg -n "public function test_" src/Core/ -A 1 | head -50

Repository: host-uk/core-php

Length of output: 3293

Add a return type to the test method.

Test methods must declare return types to comply with the coding guideline requiring type hints on all parameters and return types in PHP files.

💡 Proposed fix 
-    public function test_pruning_performance()
+    public function test_pruning_performance(): void
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public function test_pruning_performance()
{
public function test_pruning_performance(): void
{
🤖 Prompt for AI Agents 
In `@src/Core/Config/Tests/Feature/ConfigVersioningPerformanceTest.php` around
lines 25 - 26, The test method test_pruning_performance lacks a return type;
update its signature (public function test_pruning_performance()) to declare a
void return type (public function test_pruning_performance(): void) so it
complies with the coding guideline requiring return type hints for PHP test
methods.

// 1. Setup dependencies
$mockExporter = Mockery::mock(ConfigExporter::class);
$mockExporter->shouldReceive('exportJson')
->andReturn(json_encode(['values' => []]));

$configService = app(ConfigService::class);

$versioning = new ConfigVersioning($configService, $mockExporter);

// set max versions to a small number to force pruning
Config::set('core.config.max_versions', 50);
// Re-instantiate to pick up config or use setter if available
$versioning->setMaxVersions(50);

// 2. Create a profile
$profile = ConfigProfile::ensureSystem();

// 3. Seed many versions (e.g., 500)
// We insert them directly to avoid triggering pruning during setup
$now = now();
$versions = [];
for ($i = 0; $i < 500; $i++) {
$versions[] = [
'profile_id' => $profile->id,
'workspace_id' => null,
'label' => "Version $i",
'snapshot' => '{}',
'author' => 'tester',
'created_at' => $now->subMinutes(500 - $i), // Oldest first
];
}
ConfigVersion::insert($versions);

$initialCount = ConfigVersion::count();
$this->assertEquals(500, $initialCount);

// 4. Measure pruning time
$start = microtime(true);

// createVersion triggers pruneOldVersions
$versioning->createVersion(null, 'New Version');

$duration = microtime(true) - $start;

// 5. Verify
$finalCount = ConfigVersion::count();
$this->assertEquals(50, $finalCount, 'Should have pruned down to 50 versions');

echo "\nPerformance: Pruning took ".number_format($duration * 1000, 2)."ms\n";
}
}
Loading