Fix: relationship join cache with query clones

[ralphjsmit]

Hello there, thank you for the nice library!

I am using it in e.g. the context of Filament and quite often I have a flaky test that fails on a "column X.Y does not exist". After much digging, the reason for this appears to be the "is relation already joined" cache, which depends on the $model inside the query. However, if a query is cloned, this $model is not refreshed, leaving you with a situation where the clone shares the cache with the original query. This can be quite dangerous, because if one of the queries is modified or e.g. executed, this impacts the cache for the other query, leaving the other query with potentially a incorrect cache for "is relation already joined".

I added tests with more examples, but a good example where you would have this query:

$query = User::query();

which could then be cloned:

$clone = $query->clone();

...whereas after that on both queries separately a join is applied:

$query = $query->joinRelationship('posts');

// This join is not applied, because the join cache *thinks* it is already applied but it actually is only applied on the first query.
$queryClone = $queryClone->joinRelationship('posts');

Two situations

There are generally two clone situations:

• Query is cloned before any interaction with the library (=> so we need to detect any duplicate models on first join).
• Query is cloned after any interaction with the library (=> so we can use the new Add Builder On Clone callback support laravel/framework#54477 to reset the cache)

I have extracted both checks into an JoinsHelper::ensureModelIsUniqueToQuery() method, which is called on every interaction with the library, which has the responsibility of preventing both situations. If the first situation is happening, it will correct immediately (by having a cache of which Spl object IDs of which model belongs to which query, and one model can only belong to max. 1 query). For the second situation, a respective callback is added.

The added TestCase JoinRelationshipAfterCloneTest also had examples of both situations. You can confirm these snippets fail on the current master.

Thanks, and let me know if there's any questions about this!

[luisdalmolin]

Thanks @ralphjsmit. Seems like this change is breaking a few tests, would you mind taking a look at those?

[ralphjsmit]

Thanks @luisdalmolin! Yes I am taking a look. It seems that only the tests are failing on Laravel 10, for which it is logical that the added test fails, as it depends on the newly added onClone() method in Laravel 11.

[luisdalmolin]

That's interesting. I'll take a look at this then. We can consider dropping Laravel 10 support, since Laravel 12 is near.

[ralphjsmit]

I just took a look, but without the onClone() method other tests fail because of the other logic, so if you would be OK with dropping Laravel 10 that would be awesome.

[ralphjsmit]

I only need to stop now for today, I will try tomorrow if all tests pass when Laravel 10 is dropped and at least ^11.42 is required. Or feel free to try and push up yourself!

[ralphjsmit]

@luisdalmolin Can you trigger the tests again?

• I removed Laravel 10 support. Bumped PHP from ^8.1 to ^8.2 in line with Laravel 11 requirement.
• I also refactored all array properties in the JoinsHelper to use a WeakMap instead. This way we have to work less with spl_object_id() and it allows PHP to drop the item from the array any time a Query or Model object can be garbage-flushed. I think this might also better for applications using Octane. I used the testsuite to benchmark this as it had a quite consistent run-time and memory. The runtime is a tiny bit slower but memory usage is reduced by 2 MB:

# Before:
Time: 00:01.858, Memory: 44.50 MB
# After:
Time: 00:01.864, Memory: 42.50 MB

[ralphjsmit]

All good now 👍

[luisdalmolin]

Looks good, I only pushed a small change to the tests (executing the queries as well) but I'm merging this. Since this is dropping support for Laravel 10, I'll have to tag a new major version.

[luisdalmolin]

Well, the problem with a new major version is that Filament would have to update that as well. And it seems like Filament supports Laravel 10 still, so this would likely only be added on the new version?

[ralphjsmit]

Thanks Luis! Looks good.

I am not sure actually if this would need a major version, because, if we would release this a minor version, only the people with Laravel 11 would be able to upgrade to this version and for them it would work, and for the people on Laravel 10 this new version would not even be suggested, because Composer already knows itself that it isn't compatible with Laravel 10, so these people will remain on what is the current latest version. So personally I don't really see why it should be a breaking change, because Composer always ensures that this version in only installable on L11 systems...

Does that make sense?

[luisdalmolin]

Yeah good point, I think we can go with this approach, I like this better.

[luisdalmolin]

This is tagged as 4.2.0 - please let me know if this works well for you. Thanks for this!

[ralphjsmit]

Hey @luisdalmolin, just tried and looking good for me, thanks!