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

[9.x] Fix issue with aggregates (withSum, etc.) for pivot columns on self-referencing many-to-many relations #44286

Merged
merged 5 commits into from
Oct 4, 2022
Merged

Conversation

kayrunm
Copy link
Contributor

@kayrunm kayrunm commented Sep 23, 2022

An SQL error currently occurs when trying to run any withX aggregate functions on a column from a pivot table for a many-to-many relation that references the same table. The error that occurs is similar to the below:

Illuminate\Database\QueryException : SQLSTATE[HY000]: General error: 1 no such column: laravel_reserved_0.transaction_transaction.amount (SQL: select "transactions".*, (select sum("laravel_reserved_0"."transaction_transaction"."amount") from "transactions" as "laravel_reserved_0" inner join "transaction_transaction" on "laravel_reserved_0"."id" = "transaction_transaction"."allocated_to_id" where "transactions"."id" = "transaction_transaction"."allocated_from_id") as "total_allocated" from "transactions" limit 1)

The issue at hand is from lines 625–627 in QueriesRelationships:

$hashedColumn = $this->getQuery()->from === $relation->getQuery()->getQuery()->from
     ? "{$relation->getRelationCountHash(false)}.$column"
     : $column;

The condition in this ternary checks whether the current query references the same table as the relation's query, and if so, it prepends a "relationCountHash" which in the above error is "laravel_reserved_0". In a many-to-many relationship which references the same table, for example a "friends" relationship which links from a users table to the users table, this would return true and it would make referencing columns from a pivot table impossible in this situation.

My proposed fix for this is to move the above logic into a new method which checks for a period (.) in the $column, to see if the referenced column is on a different table to the original query. If this is the case, it just returns the $column, without the reserved table alias.

This shouldn't, to my knowledge, introduced any breaking changes with current versions as any columns passed in with a period are automatically escaped into "table_name"."column" syntax, rather than treated as a single column (e.g. "table_name.column".

@driesvints
Copy link
Member

driesvints commented Sep 26, 2022

Please add a thorough explanation to your PR and don't link to an issue (see the pr template).

@driesvints driesvints marked this pull request as draft September 26, 2022 07:04
@kayrunm kayrunm changed the title Fix issue #44285 with aggregates on self-referencing many-to-many relations Fix issue with aggregates (withSum, etc.) for pivot columns on self-referencing many-to-many relations Sep 26, 2022
@kayrunm kayrunm changed the title Fix issue with aggregates (withSum, etc.) for pivot columns on self-referencing many-to-many relations [9.x] Fix issue with aggregates (withSum, etc.) for pivot columns on self-referencing many-to-many relations Sep 26, 2022
@kayrunm kayrunm marked this pull request as ready for review September 26, 2022 08:47
@kayrunm
Copy link
Contributor Author

kayrunm commented Sep 26, 2022

Apologies @driesvints, sorted now.

For more information on the issue, you can view the issue at #44285.

@taylorotwell taylorotwell merged commit d8eb542 into laravel:9.x Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants