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

Can't add a where clause to a relationship table inside whereHas in a Model that has relationship with itself. #54794

Closed
crazynds opened this issue Feb 25, 2025 · 16 comments

Comments

@crazynds
Copy link

crazynds commented Feb 25, 2025

Laravel Version

11.43.2

PHP Version

8.3.6

Database Driver & Version

Mysql

Description

I had the following problem:

  • I had a Model called PunchRegistry like follows:
<?php
class PunchRegistry extends Model
{
    public $timestamps = false;

    protected $guarded = [];

    public function pair()
    {
        return $this->belongsTo(PunchRegistry::class, 'punch_registry_id');
    }
}
  • I needed to find all registries that have a pair to check the hours diference between then, so I created the following query:
<?php
$registries = PunchRegistry::whereHas('pair', function (Builder $query) {
                $driver = DB::getDriverName(); // Get Database
                $query->where('punch_registries.type', 1);
                if ($driver === 'mysql') {
                    $query->whereRaw('TIMESTAMPDIFF(HOUR, punch_registries.record_at, laravel_reserved_0.record_at) >=  8');
                } elseif ($driver === 'pgsql') {
                    $query->whereRaw('EXTRACT(HOUR FROM (laravel_reserved_0.record_at - punch_registries.record_at)) >=  8' );
                }
            })
            ->with(['pair'])
            ->orderBy('record_at', 'asc')
            ->lazy();
  • As you can see, I'm using the laravel_reserved_0 table to refer to the pair.

That works normally in dev mode, but when I launched it in the test enviroment with laravel octane, I got error 500 in my logs because the table 'laravel_reserved_0' is not defined. Reading the source you can find the following lines in \Illuminate\Database\Eloquent\Relations\Relation class:

/**
* The count of self joins.
*
* @var int
*/
protected static $selfJoinCount = 0;

/**
* Get a relationship join table hash.
*
* @param bool $incrementJoinCount
* @return string
*/
public function getRelationCountHash($incrementJoinCount = true)
{
return 'laravel_reserved_'.($incrementJoinCount ? static::$selfJoinCount++ : static::$selfJoinCount);
}

As we can see, each new call to this request increases the table number. For the development environment, this can be ignored, since the value of the static variable is always reset to zero. However in octane, according to this docs, static variables are not reset every request, so Relation::$selfJoinCount keeps increasing every request.

I can suggest three possible fixes for this problem, and send a PR if it is needed:

  • Create an extra parameter to whereHas to add the alias name to that table. (Hard to code I think, but the best solution)
  • Add a way to make Relation::$selfJoinCount unique for each complete query (Principal query and sub queries).
  • Add a way to reset Relation::$selfJoinCount manually.

Steps To Reproduce

1- Create a self reference model;
2- Create a query with whereHas that has a condition that need to use laravel_reserved_0 table.
3- Start octane.
4- Make multiple requests to the same route, the first will work as intented and the next ones will throw a error.

@macropay-solutions
Copy link

macropay-solutions commented Feb 25, 2025

Have a look at #51825 (comment) this proposed solution for a similar static issue. In that case the static is set back to the original value after relation is retrieved.

But in your case you don't have control when the query will be executed or if it will ever be executed, not to mention when it will be executed multiple times for count and fetch for example.

We suggest you do a macro in Relation to get the value of

     protected static $selfJoinCount = 0;

via that get and use that in your raw query.

Of course this does not solve the octane issue, but this is just a "corner case" for laravel.

The interesting part would be to see the error that happens when that static becomes too big.

We use JIT + OPCACHE instead of octane to avoid issues.

@rodrigopedra
Copy link
Contributor

As you can see, I'm using the laravel_reserved_0 table to refer to the pair.

Well, as the table alias says, it is a reserved value, and not to be used on userland.

You can still manually join the related table, or use a subquery, to guarantee you would always get the same related table alias.

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

@macropay-solutions
Copy link

macropay-solutions commented Feb 26, 2025

@crazynds

Create an extra parameter to whereHas to add the alias name to that table. (Hard to code I think, but the best solution)

Changing function definition is a breaking change and it has low chances of being merged.

Aliasing the relation table is more difficult:

//relation definition


    public function pair(): BelongsTo
    {
        return new BelongsTo(
            (($m = $this->newRelatedInstance(self::class))->setTable($m->getTable() . ' as alias'))->newQuery(),
            $this,
            'punch_registry_id',
            'alias.id',
            'pair'
        );
    }


$registries = PunchRegistry::whereHas('pair', function (Builder $query) {
                $driver = DB::getDriverName(); // Get Database
                $query->where('punch_registries.type', 1);
                if ($driver === 'mysql') {
                    $query->whereRaw('TIMESTAMPDIFF(HOUR, punch_registries.record_at, alias.record_at) >=  8');
                } elseif ($driver === 'pgsql') {
                    $query->whereRaw('EXTRACT(HOUR FROM (alias.record_at - punch_registries.record_at)) >=  8' );
                }
            })
            ->with(['pair'])
            ->orderBy('record_at', 'asc')
            ->lazy();

this will create:

exists (select * from `punch_registries` as `alias` where `punch_registries`.`punch_registry_id` = `alias`.`id` and `punch_registries` as `alias.alias.id` is null)) 

But as you can see you have to fix this first:

`punch_registries` as `alias.alias.id` is null

and then you have another possible problem: you have to test if any condition applied on the relation without the alias works.

Bottom line: This is not feasible.

@macropay-solutions
Copy link

macropay-solutions commented Feb 26, 2025

@crazynds

We have a solution that can be tested in our demo page and that works with this query string:

hasRelations[parent][from]=1&relationsFilters[parent][id][in][0]=2&parent_id[in][0]=2

resulting in this query without the laravel_reserved_0 being hard coded:

and exists (select * from `operations` as `laravel_reserved_0` where `laravel_reserved_0`.`id` = `operations`.`parent_id` and `laravel_reserved_0`.`id` = '2')

So, it is possible without framework changes.

We can't share that code/logic as it is proprietary but we think you can manage with our first proposed solution (bear in mind you have to subtract 1 from that increment when you use it) or go with @rodrigopedra 's suggestion of joins.

@rodrigopedra
Copy link
Contributor

rodrigopedra commented Feb 26, 2025

or go with @rodrigopedra's suggestion of joins.

Or subquery:

$registries = PunchRegistry::query()
    ->with(['pair'])
    ->whereExists(function ($query) {
        $query->selectRaw(1)
            // guessing table name
            ->from('punch_registries', 'pairs')
            ->whereColumn('pairs.punch_registry_id', 'punch_registries.id')
            ->where('punch_registries.type', 1);

        $driver = $query->getConnection()->getDriverName();

        if ($driver === 'mysql') {
            $query->whereRaw('TIMESTAMPDIFF(HOUR, punch_registries.record_at, pairs.record_at) >=  8');
        } elseif ($driver === 'pgsql') {
            $query->whereRaw('EXTRACT(HOUR FROM (pairs.record_at - punch_registries.record_at)) >=  8');
        }
    })
    ->orderBy('record_at')
    // as Model@lazy() works with limit/offset,
    // ensure order when two rows has the same record_at
    ->orderBy('id')
    ->lazy();

@macropay-solutions
Copy link

macropay-solutions commented Feb 26, 2025

@rodrigopedra the situation has same table used twice. pairs table does not exist.

>from('punch_registries as pairs')

@macropay-solutions
Copy link

macropay-solutions commented Feb 26, 2025

@rodrigopedra we had issues with subqueries like this because they reference dynamically a column from parent query. This leads to improper usage of indexes and slow execution times in big data tables.

Which is the same situation when you use ->relation()->where(... anyway but just saying. It is a down side from eloquent which we fixed partially by using whereIn but NOT for exists (reason being that if you filter by a column from a relation, only those rows will be returned that have a relation with that condition).

@rodrigopedra
Copy link
Contributor

@macropay-solutions thanks for the heads up! I missed that detail.

I updated my answer to add an alias into the subquery's from clause:

->from('punch_registries', 'pairs')

@rodrigopedra
Copy link
Contributor

@macropay-solutions if they have a large table, I'd start storing the time difference in a column on the child records, so the subquery doesn't need to cross-reference the parent record.

It is a bit of a hassle to keep it synchronized, but querying should be much improved.

Right now, even with a Builder@whereIn(), due to the time difference needed to be calculated from its master record, cross-referencing the parent record appears to be inevitable.

@crazynds
Copy link
Author

crazynds commented Feb 26, 2025

@rodrigopedra @macropay-solutions Hi, thanks for all of your sugestions. I found a good enought solution for this problem based in these lines:

/**
* Create a new relation instance.
*
* @param \Illuminate\Database\Eloquent\Builder<TRelatedModel> $query
* @param TDeclaringModel $parent
* @return void
*/
public function __construct(Builder $query, Model $parent)
{
$this->query = $query;
$this->parent = $parent;
$this->related = $query->getModel();
$this->addConstraints();
}

public function getRelationExistenceQueryForSelfJoin(Builder $query, Builder $parentQuery, $columns = ['*'])
{
$query->select($columns);
$query->from($this->related->getTable().' as '.$hash = $this->getRelationCountHash());
$this->related->setTable($hash);

That way the model inside the query has a update table name, so you can use that as follow:

$registries = PunchRegistry::whereHas('pair', function (Builder $query) {
                $tableName = $query->getModel()->getTable(); // Get the current tableName of that relation
                $driver = DB::getDriverName();
                $query->where('punch_registries.type', 1);
                if ($driver === 'mysql') {
                    $query->whereRaw('TIMESTAMPDIFF(HOUR, punch_registries.record_at, '.$tableName.'.record_at) >= '.$hoursToIncident);
                } elseif ($driver === 'pgsql') {
                    $query->whereRaw('EXTRACT(HOUR FROM ('.$tableName.'.record_at - punch_registries.record_at)) >= '.$hoursToIncident);
                }
            })
            ->with(['pair'])
            ->orderBy('record_at', 'asc')
            ->lazy();

I don't know if it should be a feature or only by chance. To my thought this is an elegant enough way to solve this problem. Do you think that should be a easier/intuitive way to do that in laravel?

@rodrigopedra
Copy link
Contributor

Within the Builder@whereHas() callback, you can use this method to qualify your columns:

$query->qualifyColumn('record_at');

Regarding providing an easier way to do that, I think it is an edge case and, as there is a workaround, I don't think it should be added to the framework.

But that is my personal opinion, you can try sending a PR. If you explain the benefits thoroughly, I guess it has some chance to be accepted.

@crazynds
Copy link
Author

crazynds commented Feb 26, 2025

@rodrigopedra I also agree with your opinion. I will close this for now.

@macropay-solutions
Copy link

macropay-solutions commented Feb 26, 2025

@crazynds

Do you think that should be a easier/intuitive way to do that in laravel?

Your way, for this specific case, is the most elegant.

Congrats ;)

By the way: Belongs to has:

    public function getRelationExistenceQueryForSelfRelation(Builder $query, Builder $parentQuery, $columns = ['*'])
    {
        $query->select($columns)->from(
            $query->getModel()->getTable().' as '.$hash = $this->getRelationCountHash()
        );

        $query->getModel()->setTable($hash); // same line but in different file.

        return $query->whereColumn(
            $hash.'.'.$this->ownerKey, '=', $this->getQualifiedForeignKeyName()
        );
    }

and these are the relations that have it

Image

@crazynds
Copy link
Author

@macropay-solutions Yeah, I notice that every type of relationship has a function for SelfRelation that set the model table.

Thanks again for the help!

Another thing outside the context of this issue—I saw on the company's profile that you work with software development using Laravel, but I noticed that there are currently no open developer positions on your website. I am currently looking for a job, and if you are interested, I can send my resume at the company email.

I appreciate your attention!

@macropay-solutions
Copy link

@crazynds One of our colleagues will contact you on skype.

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

4 participants