-
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
Can't add a where clause to a relationship table inside whereHas in a Model that has relationship with itself. #54794
Comments
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. |
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. |
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! |
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. |
We have a solution that can be tested in our demo page and that works with this query string:
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. |
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(); |
@rodrigopedra the situation has same table used twice. pairs table does not exist.
|
@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). |
@macropay-solutions thanks for the heads up! I missed that detail. I updated my answer to add an alias into the subquery's ->from('punch_registries', 'pairs') |
@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 |
@rodrigopedra @macropay-solutions Hi, thanks for all of your sugestions. I found a good enought solution for this problem based in these lines: framework/src/Illuminate/Database/Eloquent/Relations/Relation.php Lines 86 to 100 in 50828e2
framework/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php Lines 1425 to 1432 in 50828e2
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? |
Within the $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. |
@rodrigopedra I also agree with your opinion. I will close this for now. |
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 |
@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! |
@crazynds One of our colleagues will contact you on skype. |
Laravel Version
11.43.2
PHP Version
8.3.6
Database Driver & Version
Mysql
Description
I had the following problem:
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:framework/src/Illuminate/Database/Eloquent/Relations/Relation.php
Lines 79 to 85 in 50828e2
framework/src/Illuminate/Database/Eloquent/Relations/Relation.php
Lines 273 to 284 in 50828e2
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:
Relation::$selfJoinCount
unique for each complete query (Principal query and sub queries).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.
The text was updated successfully, but these errors were encountered: