? Information Required ? Success

User tests: Successful: Unsuccessful:

avatar Deltachaos
Deltachaos
5 Jul 2019

Summary of Changes

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.

avatar Deltachaos Deltachaos - open - 5 Jul 2019
avatar Deltachaos Deltachaos - change - 5 Jul 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 5 Jul 2019
Category Libraries
avatar Deltachaos Deltachaos - change - 5 Jul 2019
Labels Added: ?
avatar Razzo1987
Razzo1987 - comment - 17 Oct 2020

I reuse this code to create a lot of user via Joomla 4 API.
Joomla4API-users.zip
It can be usefully

avatar ceford
ceford - comment - 17 Nov 2020

This PR needs a better description of the problem. What to look for before applying the patch and what to look for after.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25437.

avatar rdeutz
rdeutz - comment - 15 Mar 2021

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

avatar rdeutz rdeutz - change - 15 Mar 2021
Title
Fix high memory consumption by returning long list of duplicate entries
[4.1] Fix high memory consumption by returning long list of duplicate entries
avatar rdeutz rdeutz - edited - 15 Mar 2021
avatar richard67
richard67 - comment - 24 Oct 2021

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.

avatar richard67
richard67 - comment - 24 Oct 2021

Hmm, but thinking about it further, a "DISTINCT" select should do the same job here.

avatar richard67
richard67 - comment - 9 Apr 2022

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.

Case 1: Parameter $recursive is false and no user ID is given

In this case the result is the given guest user group ID, i.e. no SQL query is executed.

Case 2: Parameter $recursive is false and a user ID is given

In 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.

Case 3: Parameter $recursive is true and no user ID is given

In 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.

Case 4: Parameter $recursive is true and a user ID is given

In 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.

Conclusion

Either I missed something, or this PR is useless for the normal case of a consistent #__usergroups table.

avatar richard67
richard67 - comment - 9 Apr 2022

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

avatar richard67
richard67 - comment - 9 Apr 2022

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.

avatar laoneo laoneo - change - 11 Apr 2022
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: ?
avatar laoneo
laoneo - comment - 11 Apr 2022

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

avatar laoneo laoneo - close - 11 Apr 2022

Add a Comment

Login with GitHub to post a comment