User tests: Successful: Unsuccessful:
Grouping will only return a small set of items. This will save a lot of memory if you have a Joomla installation with a lot of users in a specific group.
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
Labels |
Added:
?
|
This PR needs a better description of the problem. What to look for before applying the patch and what to look for after.
I am moving this to 4.1, we are in beta and it seems there is some work to do to make this ready to merge
Title |
|
From code review it looks ok to me. Adding a group by could be wrong if there were more columns selected than only that one which is used in the group by, but that's not the case, so it seems to be safe. But I have no idea about the performance effect ... not yet maybe, I have to check.
Hmm, but thinking about it further, a "DISTINCT" select should do the same job here.
Meanwhile I've found the time to dig a bit deeper into this PR and the query it wants to modify.
Sorry that it took such a long time. The PR somehow went out of my focus.
Here my results.
We have to distinguish 4 cases in that function.
$recursive
is false and no user ID is givenIn this case the result is the given guest user group ID, i.e. no SQL query is executed.
$recursive
is false and a user ID is givenIn this case the query is
SELECT `a`.`id`
FROM `#__user_usergroup_map` AS `map`
LEFT JOIN `#__usergroups` AS `a` ON `a`.`id` = `map`.`group_id`
WHERE `map`.`user_id` = :userId;
The JOIN is used to make sure the user group still exists, i.e. the entry in the #__user_usergroup_map
table is no orphan.
:userId
is a single integer value for one user.
The group ID a.id
can only be duplicate in the result if there are entries with duplicate combination of user_id
and group_id
in the #__user_usergroup_map
table.
But the primary key of that table is the combination of these 2 columns, so that can never have happened.
So this query cannot return any duplicate values for a.id
.
$recursive
is true and no user ID is givenIn this case we want to know all user groups from which the guest user group inherits permissions.
The query is;
SELECT `b`.`id`
FROM `#__usergroups` AS `a`
LEFT JOIN `#__usergroups` AS `b` ON `b`.`lft` <= `a`.`lft` AND `b`.`rgt` >= `a`.`rgt`
WHERE `a`.`id` = :guest;
:guest
is a single integer value for the guest user group ID.
Here the JOIN is used to get the parent groups from which permissions are inherited.
It cannot result in duplicate b.id
values as long as the nested #__usergroups
table is consistent, i.e. there are no wrong duplicates for lft
or rgt
values.
But as the #__usergroups
does not have a unique index on each of the lft
or rgt
columns but only on the combination, it might be possible that such duplicates exist.
So duplicate values for b.id
can only exist in the result if the lft
or rgt
values of the #__usergroups
are messed up.
$recursive
is true and a user ID is givenIn this case the query is
SELECT `b`.`id`
FROM `#__user_usergroup_map` AS `map`
LEFT JOIN `#__usergroups` AS `a` ON `a`.`id` = `map`.`group_id`
LEFT JOIN `#__usergroups` AS `b` ON `b`.`lft` <= `a`.`lft` AND `b`.`rgt` >= `a`.`rgt`
WHERE `map`.`user_id` = :userId;
As stated above in case 2, the #__user_usergroup_map
cannot contain any duplicates for the combination of user_id
and group_id
, so the first JOIN cannot cause duplicate results.
For the second JOIN the above said for case 3 applies: It can only result in duplicates if there are duplicates for lft
or rgt
values in the #__usergroups
table, and this is only the case if the nested table is messed.
So also in this case duplicate values for b.id
can only exist in the result if the lft
or rgt
values of the #__usergroups
are messed up.
Either I missed something, or this PR is useless for the normal case of a consistent #__usergroups
table.
@Deltachaos Could you check my previous comment and report back if I'm missing something and how you can reproduce that the query results in duplicates and that your PR fixes that?
Thinking about it deeper, even duplicates for lft
or rgt
values in the #__usergroups
table cannot cause duplicates for the cases 3 and 4. The JOIN condition b.lft <= a.lft AND b.rgt >= a.rgt
might return to many results when the nested table is messed, but they still will be unique since we have unique b.id
values because that's the primary key of the table.
So I think this PR is based on the wrong assumption that the query is executed to get the groups for multiple users with one query, but that's wrong. The query is always executed for one user only, or for one group if the user if not given.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-04-11 06:10:22 |
Closed_By | ⇒ | laoneo | |
Labels |
Added:
?
Information Required
?
Removed: ? |
@richard67 thanks for your deep investigation. @Deltachaos as Access is an important part of the CMS we need to carefully decide what to change in this layer. Can you please extend your description what it actually fixes and why this change is needed Also a detailed testing instruction would help so testers can reproduce the issue also on their end. In the meantime I'm closing this pr. When ready, please reopen again. Thanks for your help making Joomla better.
I reuse this code to create a lot of user via Joomla 4 API.
Joomla4API-users.zip
It can be usefully