Open lcharette opened 2 years ago
Ping @alexweissman since it's originally your code 😬
Might be a solution : https://stackoverflow.com/a/55889981/445757
I was going to post that suggestion earlier, but had to run to my son soccer game. Using a CTE would likely be the solution, it's a lifesaver when working with Oracle as well.
Though you may be able to get away without using a CTE. I don't have MSSQL installed on my home PC, but if I understand the query correctly, this may work:
SELECT
permissions.*,
rolecounts.role_count
FROM [permissions]
LEFT JOIN (SELECT
role.id,
count(*) role_count,
ROW_NUMBER() OVER (PARTITION BY role.ID ORDER BY role.ID) rn
FROM [roles] role
INNER JOIN [permission_roles] pr ON pr.role_id = role.id
GROUP BY role.id) rolecounts ON rolecounts.ID = permissions.ID
Hum, role_count
is not right.
The result should be:
role_count | id | slug |
---|---|---|
3 | 2 | uri_spit_acid |
1 | 3 | uri_slash |
1 | 1 | uri_harvest |
0 | 4 | uri_royal_jelly |
FYI, you can run SQL Server from Docker : https://hub.docker.com/_/microsoft-mssql-server
Will try it later, but I just noticed I left off one of your join conditions, try:
SELECT
permissions.*,
rolecounts.role_count
FROM [permissions]
LEFT JOIN (SELECT
role.id,
count(*) role_count,
ROW_NUMBER() OVER (PARTITION BY role.ID ORDER BY role.ID) rn
FROM [roles] role
INNER JOIN [permission_roles] pr ON pr.role_id = role.id
INNER JOIN [role_users] ru ON ru.role_id = pr.role_id
GROUP BY role.id) rolecounts ON rolecounts.ID = permissions.ID
Nope.
I did found something interesting.
This code :
$user->permissions()
->withCount('roles')
->orderBy('roles_count', 'desc')
->toSql();
Return this SQL:
SELECT
[permissions].*,
(
SELECT
count(*)
FROM
[roles]
INNER JOIN [permission_roles] ON [roles].[id] = [permission_roles].[role_id]
WHERE
[permissions].[id] = [permission_roles].[permission_id]
) AS [roles_count]
FROM
[permissions]
INNER JOIN [permission_roles] ON [permissions].[id] = [permission_roles].[permission_id]
INNER JOIN [role_users] ON [role_users].[role_id] = [permission_roles].[role_id]
WHERE
[user_id] = 1
ORDER BY
[roles_count] DESC
Which returns this when run on the db : | id | slug | role_count |
---|---|---|---|
2 | uri_spit_acid | 3 | |
2 | uri_spit_acid | 3 | |
3 | uri_slash | 1 | |
1 | uri_harvest | 1 |
But when I do this :
$user->permissions()
->withCount('roles')
->orderBy('roles_count', 'desc')
->get()
->toArray();
Only 3 results are returned in the array, not 4... Laravel probably account for the duplicate somehow...
The error only happens when doing pagination on ->take(1)
, since we do our custom stuff in UserFrosting\Sprinkle\Core\Database\Relations\Concerns\Unique
...
$paginatedPermissions = $user->permissions()
->withCount('roles')
->orderBy('roles_count', 'desc')
->take(1)
->offset(0)
->get()
->toArray();
When ->get()
is called, the query is mutated by this code :
// $relatedPivotKeyName = permissions.id
$subQuery->addSelect($relatedPivotKeyName)
->groupBy($relatedPivotKeyName);
Producing this (failling) SQL :
SELECT
[permissions].*,
(
SELECT
count(*)
FROM
[roles]
INNER JOIN [permission_roles] ON [roles].[id] = [permission_roles].[role_id]
WHERE
[permissions].[id] = [permission_roles].[permission_id]
) AS [roles_count],
[permissions].[id] -- Added above
FROM
[permissions]
INNER JOIN [permission_roles] ON [permissions].[id] = [permission_roles].[permission_id]
INNER JOIN [role_users] ON [role_users].[role_id] = [permission_roles].[role_id]
WHERE
[user_id] = 1
GROUP BY
[permissions].[id] -- Added above
ORDER BY
[roles_count] DESC
Remove the [permissions].*,
, and the expected result is :
role_count | id |
---|---|
3 | 2 |
1 | 3 |
1 | 1 |
You beat me to the punch. I just noticed I had the other join on the wrong query. But I think I know where I went wrong. If I run this in MySQL, I get an output similar to what you have above with the duplicate rows:
SELECT
permissions.*,
rolecounts.role_count
FROM userfrosting.permissions
LEFT JOIN (SELECT
role.id,
count(*) role_count
#ROW_NUMBER() OVER (PARTITION BY role.ID ORDER BY role.ID) rn
FROM roles role
INNER JOIN permission_roles pr ON pr.role_id = role.id
GROUP BY role.id) rolecounts ON rolecounts.ID = permissions.ID # and rn = 1
INNER JOIN role_users ru ON ru.role_id = rolecounts.id
Of course there is no row_number in MySQL, but if you do that and add AND rn = 1
on the subquery join, you should get just the one row and not the multiple
Query 1 ERROR: Msg: 102, Line 10, State: 1, Level: 15
Incorrect syntax near '#ROW_NUMBER'.
Problem is, once the correct SQL is found, it will need to be converted to the correct Eloquent code.
Only 3 results are returned in the array, not 4... Laravel probably account for the duplicate somehow...
👀
Problem is this removes duplicates after the query, but when you limit the query, it needs to be done BEFORE, otherwise LIMIT 2
will return the same row, which then will be parsed through this and return only one row...
I wonder if in this case, the limit ~could~ should be done after the query is run ? Might have performance issue with large query, but since this is all related to belongsToManyThrough
... @alexweissman any thought on performance issue?
EDIT: I guess the performance hit on doing the limit on the whole results is similar than doing two queries? I guess this code is only used in Sprunje used to display the user permissions, if it's used at all ?
Ha, the # is a comment in MySQL, remove them for MSSQL
Ha, the # is a comment in MySQL, remove them for MSSQL
SELECT
permissions.*,
rolecounts.role_count
FROM
permissions
LEFT JOIN (
SELECT
role.id,
count(*) role_count,
ROW_NUMBER() OVER (PARTITION BY role.ID ORDER BY role.ID) rn
FROM
roles ROLE
INNER JOIN permission_roles pr ON pr.role_id = role.id
GROUP BY
role.id) rolecounts ON rolecounts.ID = permissions.ID and rn = 1
INNER JOIN role_users ru ON ru.role_id = rolecounts.id
Not quite, but closer
I've added SQL Server to the test workflow, and I get stuck on this issue. So I'm sharing in case someone as an idea and because writing things down sometimes helps me figure things out.
Actual problem is this: https://stackoverflow.com/a/13999903/445757
It doesn't affect MySQL because MySQL is smart enough to understand how to handle that (somehow). And it's a "fake" issue in our case, because they are really the same slug (same id).
The failed test is this one : https://github.com/userfrosting/sprinkle-core/blob/631fc7796847d7c3e77c9bdc4e08da8cea934c72/app/tests/Integration/Database/DatabaseTests.php#L743
But it's not the query that cause the error. The error is caused by this query : https://github.com/userfrosting/sprinkle-core/blob/631fc7796847d7c3e77c9bdc4e08da8cea934c72/app/src/Database/Relations/Concerns/Unique.php#L299
Here's the "beautified" SQL:
Problematic part is this :
[permissions].*,
. Remove the wildcard, remove theslug
from columns.Failed Solution n° 1
We could replace all existing columns (except the required "id" for the group by): https://github.com/userfrosting/sprinkle-core/blob/631fc7796847d7c3e77c9bdc4e08da8cea934c72/app/src/Database/Relations/Concerns/Unique.php#L282
...but this removes
roles_count
and mess up the sort.There's no way I know to remove
*
without removing theroles_count
(or other user defined select).Failed Solution n° 2
The issue is with GROUP BY, so we remove the group by right?
Here's the result without Group By and limit :
So it does work with Limit 1, but Limit 2 won't work (
testBelongsToManyThroughPaginated
test will fail in this case highlighting this).Group could be done on the Collection, but this means Limit and Offset also needs to be done In the Collection. Would make the whole thing less efficient.
Failed Solution n° 3
DISTINCT
instead ofGROUP BY
doesn't help too.It does fix
testBelongsToManyThroughPaginatedWithOrderByAggregateColumn
, but it doesn't fixtestBelongsToManyThroughPaginated
:Which returns ID
[2, 2]
instead of[2, 3]
Original SQL :
Other solutions
We could "Cheat" the test and force select only 'id' :
But
slug
is not part of the results anymore (and bug is still there).So I'm bit stuck now. SQL Server doesn't support this
Linked Branch : https://github.com/userfrosting/sprinkle-core/tree/5.0-mssql Other references: