-
Notifications
You must be signed in to change notification settings - Fork 11k
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.
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
Eager Loading a relation that calls another relation returns incorrect results #51825
Comments
Hey there, thanks for reporting this issue. We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as one separate commit on the main/master branch and share the repository here? Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.
Do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue. Thanks! |
Here it is: allandantasdev/laravel-bug-report-51825@ce9741a |
Thank you for reporting this issue! As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub. If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team. Thank you! |
I found a quick and dirty solution, in the constructor of public function __construct(Builder $query, Model $parent)
{
$this->query = $query;
$this->parent = $parent;
$this->related = $query->getModel();
$this->addConstraints();
static::$constraints = true; // This
} I've been working on getting a proper fix, but there doesn't seem to be a straight path forward as changing one thing breaks another one, this will likely require some debug_backtrace to fix this without breaking some test cases |
I believe #52461 have fixed this issue, please open a new issue if you still face the problem |
Sorry, that PR doesn't address this issue at all, I was only talking about how using static causes this kind of issues You can reopen it |
// We want to run a relationship query without any constrains so that we will
// not have to remove these where clauses manually which gets really hacky
// and error prone. We don't want constraints because we add eager ones.
$relation = Relation::noConstraints(function () use ($name) {
try {
return $this->getModel()->newInstance()->$name();
} catch (BadMethodCallException $e) {
throw RelationNotFoundException::make($this->getModel(), $name);
}
}); @allandantasdev this makes sense because the conditions might imply columns from the model (other than the foreign key), that will not be in the query for the related model. the solution might be to add the constraints on the collection of related models based on each model protected function eagerLoadRelation(array $models, $name, Closure $constraints)
{
// First we will "back up" the existing where conditions on the query so we can
// add our eager constraints. Then we will merge the wheres that were on the
// query back to it in order that any where conditions might be specified.
$relation = $this->getRelation($name);
$relation->addEagerConstraints($models);
$constraints($relation);
///////////////////////////////////////////////// HERE
// Once we have the results, we just match those back up to their parent models
// using the relationship instance. Then we just return the finished arrays
// of models which have been eagerly hydrated and are readied for return.
return $relation->match(
$relation->initRelation($models, $name),
$relation->getEager(), $name
);
} |
Sorry for the previous replies. Now we understood the real issue. $user->categories->pluck('id') while the static::$constraints is false because of the eager load of the relation in which it is called, resulting in all categories being retrieved not only the categories from that user. |
If the Relation had this function(which is doable via macros as static function): /**
* Run a callback with constraints enabled on the relation.
*
* @param \Closure $callback
* @return mixed
*/
public static function yesConstraints(Closure $callback)
{
$previous = static::$constraints;
static::$constraints = true;
try {
return $callback();
} finally {
static::$constraints = $previous;
}
} then the scope or condition could be written like this: class Example extends Model
{
// ...
/**
* The authenticated user should only have access to not restricted Examples
* or to the examples he owns.
*/
public function scopeHasAccess(Builder $query, ?User $user = null): Builder
{
return $query->where(
fn ($query) => $query->where('restricted', false)
->when(
$user !== null,
function ($query) use ($user) {
$userCategories = $user->relationLoaded('categories') ?
$user->categories :
Relation::yesConstraints(function () use ($user) {
try {
return $user->categories();
} catch (BadMethodCallException $e) {
throw RelationNotFoundException::make($user, 'categories');
}
});
return $query->orWhereIn('category_id', $userCategories->pluck('id');); // also, this can be written with a sub select and the issue is avoided in that way
}
)
);
}
} Can this solution be embedded in laravel somehow so the user does not need to handle it in the scope or relation definition? |
UPDATE: #51825 (comment) |
@macropay-solutions I already tried this solution but it breaks some special cases The withConstraints approach might be the easieast workaround to get into the core |
@Tofandel Your code always set static:$constraints = true; #51825 (comment) Our suggestion sets it only once (with the previous value not with hard codded true) at the construct's end and not in that finally clause. But if you say it breaks special cases, we believe you. |
@Tofandel can you please share those special cases? |
It will fail on those kind of relations because the Relation constructor is called twice in there and so it restores constraints too early public function price_without_key_in_aggregates()
{
return $this->hasOne(HasOneOfManyTestPrice::class, 'user_id')->ofMany(['published_at' => 'MAX']);
}
public function price_with_shortcut()
{
return $this->hasOne(HasOneOfManyTestPrice::class, 'user_id')->latestOfMany(['published_at', 'id']);
}
public function teamMatesWithPendingRelation()
{
return $this->through($this->ownedTeams())
->has(fn (Team $team) => $team->members());
} |
Just run the vendor/bin/phpunit tests on the repo with your changes and see how it goes, likely it will be very unreliable, I doubt |
@Tofandel @macropay-solutions There is another case that would be uncovered:
|
@marius-mcp |
Another solution that sadly can't be implemented via macros for older versions: \Illuminate\Database\Eloquent\Relations\Relation protected static ?string $noConstraintsForRelationName = null;
public function __construct(Builder $query, Model $parent)
{
$this->query = $query;
$this->parent = $parent;
$this->related = $query->getModel();
/** @see self::noConstraints */
if (
'' !== (string)static::$noConstraintsForRelationName
&& '' !== (string)$parent->nowEagerLoadingRelationNameWithNoConstraints
) {
/** 1st execution is for ExampleModel $exampleModel on 'rel' relation
with nowEagerLoadingRelationNameWithNoConstraints = 'rel'
and with $noConstraintsForRelationName = 'rel'
*/
// 2nd execution is for UserModel $userModel on 'categories' relation
// with nowEagerLoadingRelationNameWithNoConstraints = null
// and with $noConstraintsForRelationName = 'rel'
// 1st execution is for ExampleModel $exampleModel on 'children' relation
// with nowEagerLoadingRelationNameWithNoConstraints = null
// and with $noConstraintsForRelationName = 'rel'
/** 2nd execution is for ExampleModel $exampleModel on 'rel' relation
with nowEagerLoadingRelationNameWithNoConstraints = 'rel'
and with $noConstraintsForRelationName = 'rel'
*/
// 3rd execution is for UserModel $userModel on 'categories' relation
// with nowEagerLoadingRelationNameWithNoConstraints = null
// and with $noConstraintsForRelationName = 'rel'
static::$constraints =
static::$noConstraintsForRelationName !== $parent->nowEagerLoadingRelationNameWithNoConstraints;
}
$this->addConstraints();
} /**
* Run a callback with constraints disabled on the relation based on relationName.
*/
public static function noConstraints(\Closure $callback, ?string $relationName = null): mixed
{
$previous = static::$constraints;
$previousNoConstraintsForRelationName = static::$noConstraintsForRelationName;
if ('' !== (string)$relationName) {
static::$noConstraintsForRelationName = $relationName;
} else {
static::$constraints = false;
}
try {
return $callback();
} finally {
static::$constraints = $previous;
static::$noConstraintsForRelationName = $previousNoConstraintsForRelationName;
}
} \Illuminate\Database\Eloquent\Builder /**
* Get the relation instance for the given relation name (for eager loading)
*
* @param string $name
* @return \Illuminate\Database\Eloquent\Relations\Relation
*/
public function getRelation($name)
{
// We want to run a relationship query without any constrains so that we will
// not have to remove these where clauses manually which gets really hacky
// and error prone. We don't want constraints because we add eager ones.
$relation = Relation::noConstraints(function () use ($name) {
try {
$model = $this->getModel()->newInstance();
$model->nowEagerLoadingRelationNameWithNoConstraints = $name;
return $model->$name();
} catch (BadMethodCallException) {
throw RelationNotFoundException::make($this->getModel(), $name);
}
}, $name);
$nested = $this->relationsNestedUnder($name);
// If there are nested relationships set on the query, we will put those onto
// the query instances so that they can be handled after this relationship
// is loaded. In this way they will all trickle down as they are loaded.
if (count($nested) > 0) {
$relation->getQuery()->with($nested);
}
return $relation;
} \Illuminate\Database\Eloquent\Concerns\HasRelationships public ?string $nowEagerLoadingRelationNameWithNoConstraints = null; The definition of the relation: public function productsValueScope(): HasManyThrough
{
return $this->products()->where(
fn ($query) => $query->where('value', '>', 10)->when(
true,
fn($query) => $query->orWhereIn('id', Operation::query()->where('id', 2)->first()->children->pluck('id')->toArray())
)
);
} RESULTS: GET page=1&withRelations[0]=productsValueScope&limit=2 -- Operation::query()->where('id', 2)->first()
"0.48 ms, sql: select * from `operations` where `id` = 2 limit 1",
-- ->children->pluck('id')
"0.55 ms, sql: select * from `operations` where `operations`.`parent_id` = 2 and `operations`.`parent_id` is not null",
-- count
"0.56 ms, sql: select count(*) as aggregate from `operations`",
-- fetch the list
"1 ms, sql: select * from `operations` order by `created_at` desc limit 2 offset 0",
-- eager load
"0.63 ms, sql: select * from `operations` where `id` = 2 limit 1",
"0.56 ms, sql: select * from `operations` where `operations`.`parent_id` = 2 and `operations`.`parent_id` is not null",
"1.15 ms, sql: select distinct `products`.*, `operations_products_pivot`.`operation_id` as `laravel_through_key` from `products` inner join `operations_products_pivot` on `operations_products_pivot`.`product_id` = `products`.`id` where `operations_products_pivot`.`operation_id` is null and (`value` > 10 or `id` in (3)) and `operations_products_pivot`.`operation_id` in (3748917, 3748918)" @crynobone we will not open a MR or PR so feel free to do it if you want to fix the issue. |
The good thing is that if the relations are defined with
or no other relations are called after the relation is instantiated, then this bug is not affecting older versions. For the So there is a way of avoiding it. I tested the change. Works also for me. |
@allandantasdev Hello Are you sure this code is correct ?
|
@jkpeyi that is just a typo. |
FYI It would be pity to see here "closing this issue because it is too old" or something similar just like we saw happening in the past with other bugs that were not solved. Please do not bury this under the carpet. |
Laravel Version
11.7.0
PHP Version
8.3.7
Database Driver & Version
PostgreSQL 15.7 and MySQL 8.0.37
Description
When eager loading a model relationships, the results differ from when they are lazy loaded.
This issue occurs whenever a Relation is called inside the definition of another Relation, but only when eager loading is used on the main one.
After investigation I realized that the cause lies in Illuminate\Database\Eloquent\Builder@eagerLoadRelation:
Which calls the Illuminate\Database\Eloquent\Builder@getRelation method:
Which gets to the root cause of the problem in Illuminate\Database\Eloquent\Relations\Relation@noConstraints:
The method Illuminate\Database\Eloquent\Relations\Relation@noConstraints is called during eager loading and uses a boolean attribute to manage the constraints. However, this flag is static and seems to be causing the where clauses of other relations to be omitted, leading to incorrect results.
Steps To Reproduce
Expected behavior:
The fetched relations should be consistent regardless of whether they are lazy or eager loaded.
Actual behavior:
The text was updated successfully, but these errors were encountered: