Fix: relationship join cache with query clones

.github/workflows/ci.yaml

@@ -23,15 +23,10 @@ jobs:
             fail-fast: true
             matrix:
                 php: [ 8.2, 8.3, 8.4 ]
-                laravel: [ 10.*, 11.* ]
+                laravel: [ 11.* ]
                 include:
                     -   laravel: 11.*
                         testbench: 9.*-dev
-                    -   laravel: 10.*
-                        testbench: 8.*
-                exclude:
-                    -   laravel: 10.*
-                        php: 8.4
 
         name: MySQL 8 - PHP ${{ matrix.php }} - Laravel ${{ matrix.laravel }}
 
@@ -82,15 +77,10 @@ jobs:
             fail-fast: true
             matrix:
                 php: [ 8.2, 8.3, 8.4 ]
-                laravel: [ 10.*, 11.* ]
+                laravel: [ 11.* ]
                 include:
                     -   laravel: 11.*
                         testbench: 9.*-dev
-                    -   laravel: 10.*
-                        testbench: 8.*
-                exclude:
-                    -   laravel: 10.*
-                        php: 8.4
 
         name: PostgreSQL 15 - PHP ${{ matrix.php }} - Laravel ${{ matrix.laravel }}
 
@@ -136,15 +126,10 @@ jobs:
             fail-fast: true
             matrix:
                 php: [ 8.2, 8.3, 8.4 ]
-                laravel: [ 10.*, 11.* ]
+                laravel: [ 11.* ]
                 include:
                     -   laravel: 11.*
                         testbench: 9.*-dev
-                    -   laravel: 10.*
-                        testbench: 8.*
-                exclude:
-                    -   laravel: 10.*
-                        php: 8.4
 
         name: SQLite - PHP ${{ matrix.php }} - Laravel ${{ matrix.laravel }}
 

composer.json

@@ -18,14 +18,14 @@
         }
     ],
     "require": {
-        "php": "^8.1",
-        "illuminate/support": "^10.0|^11.0|^12.0",
-        "illuminate/database": "^10.0|^11.0|^12.0"
+        "php": "^8.2",
+        "illuminate/support": "^11.42|^12.0",
+        "illuminate/database": "^11.42|^12.0"
     },
     "require-dev": {
         "friendsofphp/php-cs-fixer": "dev-master",
         "laravel/legacy-factories": "^1.0@dev",
-        "orchestra/testbench": "^8.0|^9.0",
+        "orchestra/testbench": "^9.0",
         "phpunit/phpunit": "^10.0"
     },
     "autoload": {

src/JoinsHelper.php

@@ -2,29 +2,51 @@
 
 namespace Kirschbaum\PowerJoins;
 
+use Closure;
+use Illuminate\Database\Eloquent\Builder;
 use Illuminate\Database\Eloquent\Relations\BelongsToMany;
 use Illuminate\Database\Eloquent\Relations\HasManyThrough;
 use Illuminate\Database\Eloquent\Relations\Relation;
+use WeakMap;
 
 class JoinsHelper
 {
-    public static array $instances = [];
+    public static WeakMap $instances;
+
+    /**
+     * Cache to determine which query model belongs to which query.
+     * This is used to determine if a query is a clone of another
+     * query and therefore if we should refresh the model in it.
+     *
+     * The keys are the model objects, and the value is the spl
+     * object ID of the associated Eloquent builder instance.
+     */
+    public static WeakMap $modelQueryDictionary;
+
+    /**
+     * An array of `beforeQuery` callbacks that
+     * are registered by the library.
+     */
+    public static WeakMap $beforeQueryCallbacks;
 
     protected function __construct()
     {
+        static::$instances ??= new WeakMap();
+        static::$modelQueryDictionary ??= new WeakMap();
+        static::$beforeQueryCallbacks ??= new WeakMap();
+
+        $this->joinRelationshipCache = new WeakMap();
     }
 
-    public static function make(): static
+    public static function make($model): static
     {
-        $objects = array_map(fn ($object) => spl_object_id($object), func_get_args());
-
-        return static::$instances[implode('-', $objects)] ??= new self();
+        return static::$instances[$model] ??= new self();
     }
 
     /**
      * Cache to not join the same relationship twice.
      */
-    private array $joinRelationshipCache = [];
+    private WeakMap $joinRelationshipCache;
 
     /**
      * Join method map.
@@ -35,6 +57,80 @@ public static function make(): static
         'rightJoin' => 'rightPowerJoin',
     ];
 
+    /**
+     * Ensure that any query model can only belong to
+     * maximum one query, e.g. because of cloning.
+     */
+    public static function ensureModelIsUniqueToQuery($query): void
+    {
+        $originalModel = $query->getModel();
+
+        $querySplObjectId = spl_object_id($query);
+
+        if (
+            isset(static::$modelQueryDictionary[$originalModel])
+            && static::$modelQueryDictionary[$originalModel] !== $querySplObjectId
+        ) {
+            // If the model is already associated with another query, we need to clone the model.
+            // This can happen if a certain query, *before having interacted with the library
+            // `joinRelationship()` method*, was cloned by previous code.
+            $query->setModel($model = new ($query->getModel()));
+
+            // Link the Spl Object ID of the query to the new model...
+            static::$modelQueryDictionary[$model] = $querySplObjectId;
+
+            // If there is a `JoinsHelper` with a cache associated with the old model,
+            // we will copy the cache over to the new fresh model clone added to it.
+            $originalJoinsHelper = JoinsHelper::make($originalModel);
+            $joinsHelper = JoinsHelper::make($model);
+
+            foreach ($originalJoinsHelper->joinRelationshipCache[$originalModel] ?? [] as $relation => $value) {
+                $joinsHelper->markRelationshipAsAlreadyJoined($model, $relation);
+            }
+        } else {
+            static::$modelQueryDictionary[$originalModel] = $querySplObjectId;
+        }
+
+        $query->onClone(static function (Builder $query) {
+            $originalModel = $query->getModel();
+            $originalJoinsHelper = JoinsHelper::make($originalModel);
+
+            // Ensure the model of the cloned query is unique to the query.
+            $query->setModel($model = new $originalModel());
+
+            // Update any `beforeQueryCallbacks` to link to the new `$this` as Eloquent Query,
+            // otherwise the reference to the current Eloquent query goes wrong. These query
+            // callbacks are stored on the `QueryBuilder` instance and therefore do not get
+            // an instance of Eloquent Builder passed, but an instance of `QueryBuilder`.
+            foreach ($query->getQuery()->beforeQueryCallbacks as $key => $beforeQueryCallback) {
+                /** @var Closure $beforeQueryCallback */
+                if (isset(static::$beforeQueryCallbacks[$beforeQueryCallback])) {
+                    static::$beforeQueryCallbacks[$query->getQuery()->beforeQueryCallbacks[$key] = $beforeQueryCallback->bindTo($query)] = true;
+                }
+            }
+
+            $joinsHelper = JoinsHelper::make($model);
+
+            foreach ($originalJoinsHelper->joinRelationshipCache[$originalModel] ?? [] as $relation => $value) {
+                $joinsHelper->markRelationshipAsAlreadyJoined($model, $relation);
+            }
+        });
+    }
+
+    public static function clearCacheBeforeQuery($query): void
+    {
+        $beforeQueryCallback = function () {
+            /* @var Builder $this */
+            JoinsHelper::make($this->getModel())->clear();
+        };
+
+        $query->getQuery()->beforeQuery(
+            $beforeQueryCallback = $beforeQueryCallback->bindTo($query)
+        );
+
+        static::$beforeQueryCallbacks[$beforeQueryCallback] = true;
+    }
+
     /**
      * Format the join callback.
      */
@@ -96,19 +192,21 @@ public function getAliasName(bool $useAlias, Relation $relation, string $relatio
      */
     public function relationshipAlreadyJoined($model, string $relation): bool
     {
-        return isset($this->joinRelationshipCache[spl_object_id($model)][$relation]);
+        return isset($this->joinRelationshipCache[$model][$relation]);
     }
 
     /**
      * Marks the relationship as already joined.
      */
     public function markRelationshipAsAlreadyJoined($model, string $relation): void
     {
-        $this->joinRelationshipCache[spl_object_id($model)][$relation] = true;
+        $this->joinRelationshipCache[$model] ??= [];
+
+        $this->joinRelationshipCache[$model][$relation] = true;
     }
 
     public function clear(): void
     {
-        $this->joinRelationshipCache = [];
+        $this->joinRelationshipCache = new WeakMap();
     }
 }

src/Mixins/JoinRelationship.php

@@ -104,9 +104,8 @@ public function joinRelationship(): Closure
             $joinHelper = JoinsHelper::make($this->getModel());
             $callback = $joinHelper->formatJoinCallback($callback);
 
-            $this->getQuery()->beforeQuery(function () use ($joinHelper) {
-                $joinHelper->clear();
-            });
+            JoinsHelper::ensureModelIsUniqueToQuery($this);
+            JoinsHelper::clearCacheBeforeQuery($this);
 
             if (is_null($this->getSelect())) {
                 $this->select(sprintf('%s.*', $this->getModel()->getTable()));

tests/JoinRelationshipAfterCloneTest.php

@@ -0,0 +1,91 @@
+<?php
+
+namespace Kirschbaum\PowerJoins\Tests;
+
+use Kirschbaum\PowerJoins\Tests\Models\User;
+
+class JoinRelationshipAfterCloneTest extends TestCase
+{
+    /** @test */
+    public function test_join_with_clone_before_first_join()
+    {
+        // If you have a query and clone it, then apply a `joinRelationship()` to *each*
+        // of the queries/clones, and only after that executing both queries, then the
+        // `JoinsHelper` will think that the join is already applied to both of the
+        // queries, whereas they are actually are separate queries. This happening
+        // within Filament Tables, when a join is applied to the query and there
+        // could be several clones happening based on query scope and counters.
+        $query = User::query();
+        $queryClone = $query->clone();
+
+        $query = $query->joinRelationship('posts');
+
+        $queryClone = $queryClone->joinRelationship('posts');
+
+        $this->assertSame(
+            $querySql = $query->toSql(),
+            $queryCloneSql = $queryClone->toSql()
+        );
+
+        $this->assertQueryContains('inner join "posts" on "posts"."user_id" = "users"."id"', $querySql);
+        $this->assertQueryContains('inner join "posts" on "posts"."user_id" = "users"."id"', $queryCloneSql);
+
+        // executing the queries to make sure there are no exceptions
+        $this->assertCount(0, $query->get());
+        $this->assertCount(0, $queryClone->get());
+    }
+
+    /** @test */
+    public function test_join_with_clone_after_first_join()
+    {
+        $query = User::query();
+
+        $query = $query->joinRelationship('posts');
+
+        $queryClone = $query->clone();
+        $queryClone = $queryClone->joinRelationship('posts');
+
+        $this->assertSame(
+            $querySql = $query->toSql(),
+            $queryCloneSql = $queryClone->toSql()
+        );
+
+        $this->assertQueryContains('inner join "posts" on "posts"."user_id" = "users"."id"', $querySql);
+        $this->assertQueryContains('inner join "posts" on "posts"."user_id" = "users"."id"', $queryCloneSql);
+
+        // executing the queries to make sure there are no exceptions
+        $this->assertCount(0, $query->get());
+        $this->assertCount(0, $queryClone->get());
+    }
+
+    /** @test */
+    public function test_join_with_clone_after_first_join_before_query_callbacks_maintain_this()
+    {
+        $beforeQueryCallbackBoundThis = [];
+
+        $query = User::query();
+
+        $query = $query->joinRelationship('posts');
+
+        $query->beforeQuery(function () use (&$beforeQueryCallbackBoundThis) {
+            $beforeQueryCallbackBoundThis[] = $this;
+        });
+
+        $queryClone = $query->clone();
+        $queryClone = $queryClone->joinRelationship('posts');
+
+        $this->assertSame(
+            $querySql = $query->toSql(),
+            $queryCloneSql = $queryClone->toSql()
+        );
+
+        $this->assertQueryContains('inner join "posts" on "posts"."user_id" = "users"."id"', $querySql);
+        $this->assertQueryContains('inner join "posts" on "posts"."user_id" = "users"."id"', $queryCloneSql);
+
+        $this->assertCount(2, $beforeQueryCallbackBoundThis);
+
+        foreach ($beforeQueryCallbackBoundThis as $beforeQueryCallbackBound) {
+            $this->assertSame($this, $beforeQueryCallbackBound);
+        }
+    }
+}

tests/JoinRelationshipTest.php

@@ -281,7 +281,11 @@ public function test_it_doesnt_join_the_same_relationship_twice()
             ->toSql();
 
         // making sure it doesn't throw any errors
-        User::query()->select('users.*')->joinRelationship('posts')->joinRelationship('posts')->get();
+        User::query()
+            ->select('users.*')
+            ->joinRelationship('posts')
+            ->joinRelationship('posts')
+            ->get();
 
         $this->assertQueryContains(
             'inner join "posts" on "posts"."user_id" = "users"."id"',
@@ -895,7 +899,12 @@ public function test_join_with_clone_does_not_duplicate()
         $query = Post::query();
 
         $query->leftJoinRelationship('user');
-        $clonedSql = $query->clone()->leftJoinRelationship('user')->toSql();
+
+        $clonedSql = $query
+            ->clone()
+            ->leftJoinRelationship('user')
+            ->toSql();
+
         $sql = $query->toSql();
 
         $this->assertEquals($clonedSql, $sql);