Issue Details (XML | Word | Printable)

Key: MDL-14817
Type: Bug Bug
Status: Open Open
Priority: Blocker Blocker
Assignee: Eloy Lafuente (stronk7)
Reporter: John Isner
Votes: 1
Watchers: 6
Operations

Add/Edit UI Mockup to this issue
If you were logged in you would be able to see more operations.
Moodle

Duplicate ra's in $USER->access

Created: 13/May/08 10:39 PM   Updated: 07/May/09 09:51 PM
Return to search
Component/s: Roles
Affects Version/s: 1.9
Fix Version/s: 2.0

File Attachments: 1. Text File accesslib.patch.txt (2 kB)

Image Attachments:

1. another duplicate ra.png
(8 kB)
Issue Links:
Duplicate
 
Relates
 

Participants: Eloy Lafuente (stronk7), John Isner and Petr Skoda
Security Level: None
QA Assignee: Petr Skoda
Difficulty: Difficult
Affected Branches: MOODLE_19_STABLE
Fixed Branches: MOODLE_20_STABLE


 Description  « Hide
I very often see duplicate ra's in $USER->access. For example (just the first few lines):

Array
(
::::[ra] => Array
::::::::(
::::::::::::[/1/154/184] => Array
::::::::::::::::(
:::::::::::::::::::::[0] => 5
:::::::::::::::::::::[1] => 5

Duplicate ra's double the work of calculating permissions without changing the result. Sometimes there are more than two duplicates. I recently helped a user debug a problem and there were SEVEN duplicates of two different roles! This is a major efficiency issue.

Martin Langhoff suggested the fix below, but it does not eliminate the problem.
-----
Change the last line of the SQL query in get_user_access_sitewide() and load_subcontext() (both functions in lib/accesslib.php

           ORDER BY ctx.depth ASC, ctx.path ASC, ra.roleid ASC

(Added ASC to make the order unambiguous. Different DBs default to
different defaults, so what works in mySQL might break subtly in Oracle.)


 All   Comments   Change History   Version Control      Sort Order: Ascending order - Click to sort in descending order
Eloy Lafuente (stronk7) added a comment - 14/May/08 05:52 AM
Uhm...

it seems that ordering by ctx.depth ASC, ctx.path ASC, ra.roleid ASC solves the problem in get_user_access_sitewide(), because there is code included to prevent duplicates (the $lastseen checker).

But AFAIK load_subcontext() doesn't include such code to prevent duplicates, so the ordering solution won't cause any effect.

So I've added that prevention of duplicates in load_subcontext() and seems to be working here. Patch attached, can you please test it?

Ciao


John Isner added a comment - 14/May/08 06:39 AM
Eloy,
I just tested the patch. Unfortunately I get the same ra as before. When you say "it is working here" how can you be sure that your example is one that would produce duplicates? I frequently see duplicates, but not always.

Eloy Lafuente (stronk7) added a comment - 14/May/08 06:51 AM
Well, because I went to my user profile page (with debug=developer enabled) and I saw the duplicates there (in some contexts).

Then I applied the patch, logged-out, logged-in and bump, the duplicates weren't there.

Ciao


John Isner added a comment - 14/May/08 07:09 AM
My fault, I forgot to logout. Tried it again and it works. You're a genius! Thanks.

Eloy Lafuente (stronk7) added a comment - 14/May/08 07:21 AM
Thanks 4 feedback, John!

John Isner added a comment - 31/May/08 02:03 AM
Eloy,
I hate to report bad news, but I just ran a scenario on my site (which has your patch) and got yet another duplicate ra. Screenshot attached.

This can be a big performance problem. I recently helped a user (who did not have your patch), and he had SIXTY duplicates in a single USER->access data structure.


John Isner added a comment - 31/May/08 02:05 AM
Screenshot showing duplicate ra after Eloy's patch was applied.

Eloy Lafuente (stronk7) added a comment - 20/Jun/08 08:20 AM
Hi John,

I've been reviewing all the places where data is added to that "ra" array, and apart from the two functions fixed by the first patch:

  • get_user_access_sitewide()
  • load_subcontext()

There are some more additions to the "ra" array in:

  • load_user_accessdata()
  • load_all_capabilities()
  • load_temp_role()

and these last three functions don't check if the "ra" exists previously, agree, but they seems to be used only for SYSTEM and FRONTPAGE and GUEST role assignments, and that doesn't seem the case of your screenshot.

Anyway.. perhaps we should, simply, change all those:

array_push($ra, roleid);

by:

$ra[$roleid] = $roleid;

with this, we get:

1) Speed, because assigning is faster than array_push()
2) Avoid duplicates, because, we are using the keys of the array.
3) The ORDER BY clause currently in code can be avoided, and that's more speed, for sure.

Please, comment. Ciao


Eloy Lafuente (stronk7) added a comment - 20/Jun/08 08:22 AM
Adding Petr and Martin Langhoff here to see if the last proposal can be good enough.

Ciao


John Isner added a comment - 20/Jun/08 08:41 AM
Eloy,
I am away attending back-to-back Moodle Moots and will test your changes one week from today!

Petr Skoda added a comment - 27/Jun/08 08:45 AM
my +1 for HEAD commit, and later backport if everything works file during the next 3 weeks

John Isner added a comment - 27/Jun/08 09:54 PM
Petr, I will test your changes today.

John Isner added a comment - 28/Jun/08 01:14 AM
On 20/Jun/08 08:20 AM Eloy wrote...."

Anyway.. perhaps we should, simply change all those:

array_push($ra, roleid);

by:

$ra[$roleid] = $roleid;

...." but I don't see any such lines in lib/accesslib.php!

If I grep for array_push I get

$ubu[/../www/play/lib]: grep -n 'array_push(' accesslib.php
170: array_push($searchcontexts, $context->id);
1294: array_push($accessdata['ra'][$ra->path], $ra->roleid);
1456: // array_push($accessdata['ra'][$ra->path], $ra->roleid);
1457: // array_push($localroles, $ra->roleid);
1463: array_push($accessdata['ra'][$ra->path], $ra->roleid);
1464: array_push($localroles, $ra->roleid);
1614: array_push($accessdata['ra'][$base], $CFG->defaultuserroleid);
1628: array_push($accessdata['ra'][$base], $CFG->defaultfrontpageroleid);
1694: array_push($accessdata['ra'][$base], $CFG->defaultuserroleid);
1710: array_push($accessdata['ra'][$base], CFG->defaultfrontpageroleid);
1807: array_push($accessdata['loaded'], $context->path);
1813: array_push($accessdata['ra'][$context->path], $roleid);
3283: array_push($storedcaps, $cachedcap->name);
3615: array_push($records, $record);

Which of these do I change, and how exactly do I change them?


Eloy Lafuente (stronk7) added a comment - 19/Aug/08 06:51 AM
I've committed the patch above (accesslib.patch.txt ) to 19_STABLE and HEAD.

As commented by John, it fixes "some" duplicates but not all. In order to get this fixed forever I'd suggest to change the 'ra' array to be roleid indexed (instead of current array_push() approach, as commented in http://tracker.moodle.org/browse/MDL-14817?focusedCommentId=50927#action_50927

But need confirmation about that from somebody knowing internals better than me (not sure if that indexing will afect anything else).

Ciao


Eloy Lafuente (stronk7) added a comment - 07/May/09 09:48 PM
With all the changes performed both in 1.9.x and 2.0.x branches related to accesslib... I think it's better not to change this right now, so I'm moving this to Moodle 2.0, whatever it will be the new incarnation of "ra's" and friends we'll ensure there that there aren't duplicates anymore.

Also raising it to Blocker (although it isn't really a blocker, to have it present there).

Ciao


Eloy Lafuente (stronk7) added a comment - 07/May/09 09:51 PM
Petr, I've added you here as QA, feel free to switch to assignee if you think this is one thing that will fit in your roles/enrolments/permission clean for 2.0

Ciao