Skip to content
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

Open
allandantasdev opened this issue Jun 18, 2024 · 53 comments

Comments

@allandantasdev
Copy link

allandantasdev commented Jun 18, 2024

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:

 // First we will "back up" the existing where conditions on the query so we can
 // add our eager constraints.
 $relation = $this->getRelation($name);
 $relation->addEagerConstraints($models);
 
 // Then we will merge the wheres that were on the
 // query back to it in order that any where conditions might be specified.
 $constraints($relation);

Which calls the Illuminate\Database\Eloquent\Builder@getRelation method:

 // We want to run a relationship query without any constraints 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) {
         throw RelationNotFoundException::make($this->getModel(), $name);
     }
 });

Which gets to the root cause of the problem in Illuminate\Database\Eloquent\Relations\Relation@noConstraints:

$previous = static::$constraints;

 static::$constraints = false;

 // When resetting the relation where clause, we want to shift the first element
 // off of the bindings, leaving only the constraints that the developers put
 // as "extra" on the relationships, and not original relation constraints.
 try {
 return $callback();
 } finally {
 static::$constraints = $previous;
 }

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

  1. Create the following schema
        // 0001_01_01_000000_create_users_table.php
        Schema::create('categories', function (Blueprint $table) {
            $table->bigIncrements('id');
            $table->unsignedBigInteger('user_id');
            $table->foreign('user_id')->references('id')->on('users');
            $table->text('name');
            $table->timestamps();
        });
        Schema::create('examples', function (Blueprint $table) {
            $table->bigIncrements('id');
            $table->unsignedBigInteger('category_id');
            $table->foreign('category_id')->references('id')->on('categories');
            $table->text('name');
            $table->boolean('restricted');
            $table->timestamps();
        });
  1. Create the following seeder:
        $user = User::factory()->create();

        $categories = [
            Category::query()->create(['user_id' => $user->id, 'name' => 'Category 1']),
            Category::query()->create(['user_id' => $user->id, 'name' => 'Category 2']),
            Category::query()->create(['user_id' => $user->id, 'name' => 'Category 3']),
        ];

        Example::insert([
            ['category_id' => $categories[0]->id, 'name' => 'Example 1', 'restricted' => false],
            ['category_id' => $categories[1]->id, 'name' => 'Example 2', 'restricted' => true],
            ['category_id' => $categories[2]->id, 'name' => 'Example 3', 'restricted' => false],
            ['category_id' => $categories[2]->id, 'name' => 'Example 4', 'restricted' => false],
            ['category_id' => $categories[2]->id, 'name' => 'Example 5', 'restricted' => true],
        ]);

        User::factory()->create(); // another user just for demonstration
  1. Create a scope in the Example model:
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,
                    fn($query) => $query->orWhereIn('category_id', $user->categories->pluck('id'))
                )
        );
    }
}
  1. Add the following relations to the models:
class User extends Authenticatable
{
    // ...
    public function categories(): HasMany
    {
        return $this->hasMany(Category::class);
    }
}

class Category extends Model
{
    // ...
    public function examples(): HasMany
    {
        return $this->hasMany(Example::class)
            ->hasAccess(Auth::user());
     }
}
  1. Authenticate:
    Auth::login(User::find(1));
    // Auth::login(User::find(2));
  1. Fetch all categories with their respective examples
        dump('Authenticated user: '.Auth::user()->id);
        Category::get()->each(
            fn(Category $category) => dump(sprintf('- %s: %d examples', $category->name, $category->examples->count()))
        );
        
// Authenticated user: 1
// - Category 1: 1 examples"
// - Category 2: 1 examples"
// - Category 3: 3 examples"
// ----------------------------
// Authenticated user: 2"
// - Category 1: 1 examples"
// - Category 2: 0 examples"
// - Category 3: 2 examples"
  1. Execute the code again but eager loading the examples relation:
        dump('Authenticated user: '.Auth::user()->id);
        Category::with('examples')->get()->each(
            fn(Category $category) => dump(sprintf('- %s: %d examples', $category->name, $category->examples->count()))
        );
        
// Authenticated user: 1
// - Category 1: 1 examples"
// - Category 2: 1 examples"
// - Category 3: 3 examples"
// ----------------------------
// Authenticated user: 2"
// - Category 1: 1 examples"
// - Category 2: 1 examples"
// - Category 3: 3 examples"
  • Expected behavior:
    The fetched relations should be consistent regardless of whether they are lazy or eager loaded.

  • Actual behavior:

    • Without eager loading: The user of id 2 has access to 3 examples (correct)
    • With eager loading: The user of id 2 has access to 5 examples (wrong)
@crynobone
Copy link
Member

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.

laravel new bug-report --github="--public"

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!

@allandantasdev
Copy link
Author

allandantasdev commented Jun 18, 2024

@allandantasdev
Copy link
Author

Additional info:

  • This is the query generated by Auth::user()->categories->pluck('id') at Example@scopeHasAccess:24 while lazy loading
    image

  • And is the same query generated after trying to eagerloading Category::with('examples'):
    image

Copy link

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!

@Tofandel
Copy link
Contributor

Tofandel commented Jun 19, 2024

I found a quick and dirty solution, in the constructor of Relation (beware that it breaks some other cases)

   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

@crynobone
Copy link
Member

I believe #52461 have fixed this issue, please open a new issue if you still face the problem

@Tofandel
Copy link
Contributor

Tofandel commented Oct 2, 2024

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

@crynobone crynobone reopened this Oct 2, 2024
@ghost
Copy link

ghost commented Oct 15, 2024

        // 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
        );
    }

@macropay-solutions
Copy link

macropay-solutions commented Oct 16, 2024

Sorry for the previous replies. Now we understood the real issue.
This is called

$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.

@macropay-solutions
Copy link

macropay-solutions commented Oct 16, 2024

@Tofandel @allandantasdev

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?

@macropay-solutions
Copy link

macropay-solutions commented Oct 16, 2024

UPDATE: #51825 (comment)

@Tofandel
Copy link
Contributor

@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

@macropay-solutions
Copy link

macropay-solutions commented Oct 16, 2024

@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.

@ghost
Copy link

ghost commented Oct 16, 2024

@Tofandel can you please share those special cases?

@Tofandel
Copy link
Contributor

Tofandel commented Oct 16, 2024

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());
    }

@Tofandel
Copy link
Contributor

Tofandel commented Oct 16, 2024

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 where is the only method that needs this

@ghost
Copy link

ghost commented Oct 17, 2024

@Tofandel @macropay-solutions There is another case that would be uncovered:

public function relationName(): HasMany|HasManyThrough
{
    if ($this->exists && $this->children()->exist()) { // construct of Relation is called on eager loading relations in an existing model which I remember had issues also
        return $this->hasManyThrough...; // construct of Relation is called
    }

    return $this->hasMany...; // construct of Relation is called
}

@macropay-solutions
Copy link

@marius-mcp
Good catch. Then the whole Relation::noConstraints logic from \Illuminate\Database\Eloquent\Builder::getRelation is not fitting in...

@macropay-solutions
Copy link

macropay-solutions commented Oct 17, 2024

PROPOSED SOLUTION

@Tofandel

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;
        }
    }

image

\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;
    }

image

\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.

@ghost
Copy link

ghost commented Oct 18, 2024

@macropay-solutions

Another solution that sadly can't be implemented via macros for older versions:

The good thing is that if the relations are defined with

if ($this->exists && {{other relations calls}}) {

or no other relations are called after the relation is instantiated, then this bug is not affecting older versions.

For the ->load( function call on the existing model, the fix is to not use it until this gets in the core.

So there is a way of avoiding it.

I tested the change. Works also for me.

@jkpeyi
Copy link

jkpeyi commented Oct 18, 2024

@allandantasdev Hello

Are you sure this code is correct ?

{
    // ...
    public function examples(): HasMany
    {
        return $this->hasMany(Example::class);  // this ends by a semicolon , so how the ->hasAccess() will work ?
            ->hasAccess(Auth::user());  
     }
}```

@ghost
Copy link

ghost commented Oct 18, 2024

@jkpeyi that is just a typo.

@macropay-solutions
Copy link

FYI
We will not be issuing a merge request or pull request with this solution.
We shared the solution to help the community but only if the community wants to be helped.

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.

@macropay-solutions
Copy link

@taylorotwell do you see any uncovered situations by this solution #51825 (comment) ?

macropay-solutions pushed a commit to macropay-solutions/laravel-crud-wizard-free that referenced this issue Nov 21, 2024

Verified

This commit was signed with the committer’s verified signature.
TuDo1403 tu-do.ron
@macropay-solutions
Copy link

macropay-solutions commented Nov 29, 2024

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
We started testing intensively and came up with a 6th version that now seems to work.

@staudenmeir your lib https://github.com/staudenmeir/eloquent-has-many-deep can also be adapted for this retroactively.

UPDATE
This will not cover the cases when the relation is instantiated in the model without calling the functions from the HasRelationship trait!
Those cases will still have this issue.
To cover also those cases, for example for HasManyThrough use in model anonymous class:

        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
                );
            }
        };

macropay-solutions pushed a commit to macropay-solutions/laravel-crud-wizard-free that referenced this issue Dec 3, 2024
macropay-solutions pushed a commit to macropay-solutions/laravel-crud-wizard-free that referenced this issue Dec 3, 2024
macropay-solutions pushed a commit to macropay-solutions/laravel-crud-wizard-free that referenced this issue Dec 3, 2024
@macropay-solutions
Copy link

macropay-solutions commented Dec 6, 2024

This php issue appears for the above retrospective solution only when executing a relation inside an append function.

Symfony\Component\ErrorHandler\Error\FatalError: Illuminate\Database\Eloquent\Relations\Relation and MacropaySolutions\LaravelCrudWizard\Eloquent\CustomRelations\RelationCleverTrait define the same property ($constraints) in the composition of Illuminate\Database\Eloquent\Relations\HasOne@anonymous. However, the definition differs and is considered incompatible. Class was composed in file /var/www/html/core-api/vendor/macropay-solutions/laravel-crud-wizard/src/Eloquent/CustomRelations/HasCleverRelationships.php on line 33

Interesting that this does not happen on eager loading... nor on filtering.

Update
It executes the query and loads the required relations with no issue using the anonymous class and when it comes to \Illuminate\Database\Eloquent\Concerns\HasAttributes::attributesToArray function that calls the accessor that will execute a relation, then the above error appears...

Update
If the resource is retrieved without relations so without eager loading, but just with the appends/accessors the error does not appear....

Solution #53783 (comment)

Update
We managed to fix this issue.

Update
The global property was the only way to make it work retroactively (that we could find as working solution).

@allandantasdev
Copy link
Author

allandantasdev commented Jan 15, 2025

@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.

@macropay-solutions
Copy link

@allandantasdev Ok. There is no rush.

@macropay-solutions
Copy link

macropay-solutions commented Jan 31, 2025

@allandantasdev
Today we released and tested on our demo pages the proposed solution for this bug, from our auto-filter api library, also by aggregating on the relation's columns when listing the resource via API (by using withSum, withAvg, withMin, withMax). It did not break anything.

Details #49577 (comment)

@Sophist-UK
Copy link

Sophist-UK commented Feb 18, 2025

I am not experiencing this problem, but my opinion on this issue is as follows:

  1. When eager and lazy loading the same relationship gives different results, then this is absolutely is a bug. When such a bug can result in personal data for one user being exposed to another user it is a security related bug.

  2. Whilst Laravel is open-source, it is a tightly controlled open-source project where A) Most enhancements are provided by @taylorotwell's Laravel team and B) Most PRs submitted by the community are rejected by @taylorotwell (though I am not objecting to having most PRs rejected - just stating a fact here.

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.)

@macropay-solutions
Copy link

macropay-solutions commented Feb 19, 2025

@Sophist-UK

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.
Our free lib addresses laravel/lumen >=8 already, but it does not cover the cases when the relation is instantiated directly and not through the HasAttributes trait as explained here: #51825 (comment)

@Sophist-UK
Copy link

Ah - ok so...

  1. If it is macroable, then L9/10/11 should be addressed with a 1st party package.
  2. L12 should either have it as a fix, possibly with an enabling switch, possibly with a deprecation notice saying that the switch will default to enabled in a future Laravel major release.

@Tofandel
Copy link
Contributor

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)

@macropay-solutions
Copy link

macropay-solutions commented Feb 19, 2025

@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
Nested test n=2

withRelations[0]=productsValueScopeIssue51825Nested&limit=10&page=1

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)"

@Tofandel
Copy link
Contributor

Tofandel commented Feb 19, 2025

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'));
    }

@macropay-solutions
Copy link

@Tofandel

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'));
    }

@Tofandel
Copy link
Contributor

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 where 0 = 1 ?

@macropay-solutions
Copy link

macropay-solutions commented Feb 19, 2025

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

@Tofandel
Copy link
Contributor

Tofandel commented Feb 19, 2025

Maybe you could try another relation from $user like $user->unsetRelation('categories') and then $user->categories instead of $this just to see if the query has the proper constraints

@macropay-solutions
Copy link

macropay-solutions commented Feb 19, 2025

Our first example is just like that.

Operation::query()->where('id', 2)->first()

We don't have users on the demo.

@Tofandel
Copy link
Contributor

Tofandel commented Feb 19, 2025

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 Operation::first()->aRelationOfOperation and not even use it in a where, as long as it's in the relation definition that is being lazy loaded, the relation loading will still happen, and I just want to see the query resulting there in the 2nd level of nesting

@macropay-solutions
Copy link

->client->operations is not like that?

                fn($query) => $query->orWhereIn('id', Operation::query()->where('id', 2)->first()->client->operations->pluck('id')->toArray())

@Tofandel
Copy link
Contributor

Tofandel commented Feb 19, 2025

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

@macropay-solutions
Copy link

macropay-solutions commented Feb 19, 2025

@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)"

@macropay-solutions
Copy link

macropay-solutions commented Feb 19, 2025

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.
Reason being that in the scope of this issue, php is single threaded without OCTANE.

Can someone confirm that each worker from OCTANE has its own bootstrapped app?

@Tofandel
Copy link
Contributor

Tofandel commented Feb 19, 2025

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 Relation::noConstraint() while doing a database query for example but I doubt there is any kind of fiber handling or release during the entire execution of a request within laravel or there would be a lot more of those issues, because it does make quite a good use of those static properties and laravel was not built with async in mind (Since fibers only came out in php 8)

Edit:
After reading the source of Octane, this is almost a forked process scenario. Basically a server is created that spawns a pool of worker threads, each thread is then bootstrapped with the laravel server ready to resolve a route from a request and is like a separate server and waiting from the main process to send it a request. Each worker can only handle 1 request at a time, the server stays loaded in memory ready for the next request when it's done (and this is where static issues usually occur because it could reuse something static defined in a previous request). So yes no data leak from this scenario in Octane. (But still the original issue)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants