-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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 |
PROPOSED SOLUTION Another solution that sadly can't be implemented via macros for older versions: \Illuminate\Database\Eloquent\Relations\Relation UPDATED the Relation construct (and all its children's constructs to pass down the resourceModel). protected static ?string $noConstraintsForRelationName = null;
public function __construct(Builder $query, Model $parent, Model $resourceModel) // new resourceModel needed
{
$this->query = $query;
$this->parent = $parent;
$this->related = $query->getModel();
if (
'' !== (string)static::$noConstraintsForRelationName
|| '' !== (string)$resourceModel->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 !== $resourceModel->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;
} UPDATE protected function getRelationWithoutConstraints($relation): Relation
{
return Relation::noConstraints(function () use ($relation) {
$model = $this->getModel();
/** @var Model $model */
$model->nowEagerLoadingRelationNameWithNoConstraints = $relation;
return $model->{$relation}();
}, $relation);
}
protected function getBelongsToRelation(MorphTo $relation, $type): BelongsTo
{
/** here it would work as before ! */
$belongsTo = Relation::noConstraints(function () use ($relation, $type) {
return $this->model->belongsTo(
$type,
$relation->getForeignKeyName(),
$relation->getOwnerKeyName()
);
});
$belongsTo->getQuery()->mergeConstraintsFrom($relation->getQuery());
return $belongsTo;
} \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 "1469.42 ms, sql: select count(*) as aggregate from `operations`",
"1.37 ms, sql: select * from `operations` limit 10 offset 0",
-- Operation::query()->where('id', 2)->first()
"1.04 ms, sql: select * from `operations` where `id` = 2 limit 1",
-- ->children->pluck('id')
"1 ms, sql: select * from `operations` where `operations`.`parent_id` = 2 and `operations`.`parent_id` is not null",
-- eager load
"1.71 ms, sql: select `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 (`value` > 10 or `id` in (3)) and `operations_products_pivot`.`operation_id` in (1, 2, 3, 4, 5, 6, 7, 8, 9, 10)" @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. |
@taylorotwell do you see any uncovered situations by this solution #51825 (comment) ? |
No one (including both laravel community and users of laravel) seems to be interested in fixing this, not realizing that it is a possible data leak scenario... until that data is leaked and then there will be a big fuss about it... UPDATE @staudenmeir your lib https://github.com/staudenmeir/eloquent-has-many-deep can also be adapted for this retroactively. UPDATE return new class (
$query,
$farParent,
$throughParent,
$firstKey,
$secondKey,
$localKey,
$secondLocalKey,
$this // this is crucial
) extends HasManyThrough {
public function __construct(
Builder $query,
Model $farParent,
Model $throughParent,
string $firstKey,
string $secondKey,
string $localKey,
string $secondLocalKey,
BaseModel $resourceModel
) {
$this->setConstraintsStaticFlag($resourceModel);
return parent::__construct(
$query,
$farParent,
$throughParent,
$firstKey,
$secondKey,
$localKey,
$secondLocalKey
);
}
}; |
This php issue appears for the above retrospective solution only when executing a relation inside an append function.
Interesting that this does not happen on eager loading... nor on filtering. Update Update Solution #53783 (comment) Update Update |
@macropay-solutions, @bulletproof-coding and @Tofandel thank you for your attention to this matter. I will test the proposed solution on our code and give you feedback as soon as possible. |
@allandantasdev Ok. There is no rush. |
@allandantasdev Details #49577 (comment) |
I am not experiencing this problem, but my opinion on this issue is as follows:
Taken together, I believe that this is something that @taylorotwell should take extremely seriously before it is logged as a CVE, and should direct one of his team to fix as a matter of urgency, with fixes being applied to all currently supported versions of Laravel. (Considering L12 is impending, by the time it is written I would imagine this would be L10, L11 and L12.) |
The proposed fix is a breaking change so, only in 12.x could be included because it needs an extra param on the Relation's construct and on the function noConstraints. UPDATE. |
Ah - ok so...
|
For L10/11, I believe a breaking change can be avoided with some static magic and backtrace checking or maybe throwing there if it cannot be fixed would be an acceptable solution as this would now make it clear if the code is affected and this should still be a very low impact use case. (As it only happens if you load a relation in the definition of a relation) For a breaking change on this I'd rather the root of the issue be solved in a major (which is a code design issue of using the same static property throughout the whole call tree instead of a contextual property for eager loading relations, basically there is small refactor necessary on the Relation class and the way they are created maybe using factories instead of direct constructor call) because I believe the proposed fix would work for 1 nested call but not n nested call (but I could be wrong) |
@Tofandel It should work for n nested also. The issue is related to nested relation calls. We tested it. We put it out there on our demo page as proof. It can be tested by anyone wanting to test it via raw vendor change or via the lib. UPDATE
http://89.40.19.34/laravel-10-free/laravel-lumen-crud-wizard#operations
public function productsValueScopeIssue51825Nested(): 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())
)->when(
true,
fn($query) => $query->orWhereIn('id', Operation::query()->where('id', 2)->first()->client->operations->pluck('id')->toArray())
)
);
} "3.01 ms, sql: select count(*) as aggregate from `operations`",
"0.85 ms, sql: select * from `operations` limit 10 offset 0",
"0.48 ms, sql: select * from `operations` where `id` = 2 limit 1",
"0.44 ms, sql: select * from `operations` where `operations`.`parent_id` = 2 and `operations`.`parent_id` is not null",
"0.36 ms, sql: select * from `operations` where `id` = 2 limit 1",
"0.4 ms, sql: select * from `clients` where `clients`.`id` = 2 limit 1",
"0.41 ms, sql: select * from `operations` where `operations`.`client_id` = 2 and `operations`.`client_id` is not null",
"1.68 ms, sql: select `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 (`value` > 10 or `id` in (3) or `id` in (2, 3, 14063, 213871, 414260, 701972, 888608, 1183086, 1233610, 1251337, 1415201, 1450921, 1532867, 1831801, 1944821, 2202742, 2239391, 2551666, 2950315, 3008255)) and `operations_products_pivot`.`operation_id` in (1, 2, 3, 4, 5, 6, 7, 8, 9, 10)" |
That doesn't seem to be a real n=2 test, because the relations are instanciated in serie in the same definition, basically in the original report in the categories relation, you would load yet another relation in it public function categories(): HasMany
{
return $this->hasMany(Category::class)->whereIn('id', $user->loadAnotherRelation->pluck('category_id'));
} |
Because $this does not exist when eager loading your example gives this query: "0.78 ms, sql: select `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 0 = 1 and `operations_products_pivot`.`operation_id` in (11, 12, 13, 14, 15, 16, 17, 18, 19, 20)" public function productsValueScopeIssue51825NestedTwo(): HasManyThrough
{
return $this->hasManyThrough(
Product::class,
OperationProductPivot::class,
'operation_id',
'id',
'id',
'product_id'
)->whereIn('id', $this->children->pluck('id'));
} |
What's of interest would be the eager load query on $this->children which is the query that is nested 2 times Also why is there a |
Because children returns [] without a query because $this does not exist(is not hydrated) and pluck on that is [] and so, whereIn protected function whereIn(Builder $query, $where)
{
if (! empty($where['values'])) {
return $this->wrap($where['column']).' in ('.$this->parameterize($where['values']).')';
}
return '0 = 1';
} from \Illuminate\Database\Query\Grammars\Grammar::whereIn |
Maybe you could try another relation from |
Our first example is just like that.
We don't have users on the demo. |
Yes it is like that but only 1 level deep, which is why I am asking with 2 level deep of relation loading if it is properly constrained, it really doesn't matter what you load, you could even just do |
->client->operations is not like that? fn($query) => $query->orWhereIn('id', Operation::query()->where('id', 2)->first()->client->operations->pluck('id')->toArray()) |
I don't believe it is no, it's 2 completetely different things, because that's just 2 eager loading in series in the same nesting level of relation definition, there is no nesting of what actually causes the bug where you have a separate eager loading within a relation definition that is itself eager loaded To demonstrate 2 level depth you would need 2 relation definitions, in the first one you eager load the second relation and in the second definition you eager load another relation |
@Tofandel if you can write a relation like you want starting from this model https://github.com/macropay-solutions/laravel-crud-wizard-demo/blob/production/app/Models/Operation.php we can deploy that on the demo page and check. UPDATE public function productsValueScopeIssue51825NestedTwo(): HasManyThrough
{
return $this->products()->where(
fn ($query) => $query->where('value', '>', 10)->when(
true,
function($query) {
Client::query()->limit(2)->with('operations')->get();
}
)
);
} "DEMO_ONLY_sql_debugger": [
"3.5 ms, sql: select count(*) as aggregate from `operations`",
"0.5 ms, sql: select * from `operations` limit 10 offset 0",
"0.63 ms, sql: select * from `clients` limit 2",
"1.89 ms, sql: select * from `operations` where `operations`.`client_id` in (1, 2)",
"1.47 ms, sql: select `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 (`value` > 10) and `operations_products_pivot`.`operation_id` in (1, 2, 3, 4, 5, 6, 7, 8, 9, 10)" and merged public function productsValueScopeIssue51825Nested333(): HasManyThrough
{
return $this->products()->where(
fn ($query) => $query->where('value', '>', 10)->when(
true,
function($query) {
Client::query()->limit(2)->with('operations')->get();
$query->orWhereIn('id', Operation::query()->where('id', 2)->first()->children->pluck('id')->toArray())
}
)
);
} "3.13 ms, sql: select count(*) as aggregate from `operations`",
"0.51 ms, sql: select * from `operations` limit 10 offset 0",
"0.53 ms, sql: select * from `clients` limit 2",
"0.74 ms, sql: select * from `operations` where `operations`.`client_id` in (1, 2)",
"0.82 ms, sql: select * from `operations` where `id` = 2 limit 1",
"1.93 ms, sql: select * from `operations` where `operations`.`parent_id` = 2 and `operations`.`parent_id` is not null",
"1.6 ms, sql: select `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 (`value` > 10 or `id` in (3)) and `operations_products_pivot`.`operation_id` in (1, 2, 3, 4, 5, 6, 7, 8, 9, 10)" |
If the bootstrapped app will be used by multiple workers at the same time in OCTANE, with or without this fix, the code will have LOW probability of DATA LEAK because of the static::$constraints. Can someone confirm that each worker from OCTANE has its own bootstrapped app? |
That is a very good question and remark. Assuming technical limitations from PHP pretty much yes. There is no true multithreading possible there. Only a coroutine implementation or fork of the server handling the request can be made for each received request. Each forked process would maintain it's own state I have also been using websockets and even that when using fibers or reactphp is still single threaded and deterministic (octane uses swool or frankenphp), true php multithreading within a single server process is just not something you see unless you build a custom php with the parallel extension, which is simply not stable enough for real production use Based on this, the static here should not a problem, that is of course unless an implementation is able to release the current fiber within the Edit: |
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: