Moodle
  1. Moodle
  2. MDL-35547

Inifinite loop if question category points to itself as a parent

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.5, 2.3, 2.4
    • Fix Version/s: 2.2.6, 2.3.3
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      1. Create several question categories and sub-categories in a course (the more the better).
      2. Confirm all categories are displayed OK in a drop-down on question/edit.php?courseid=N page.
      3. Edit database by hand to create a loop: in table mdl_question_categories find a top-level (with parent=0) category from your course and change parent to the id of one of the children.
      4. Confirm that an exception is thrown, instead of infinite loop and timeout (previous behaviour)
      5 Repeat 3 & 4 but change mdl_question_categories.parent to ID of one of the grand-children and finally to the id of the top-level category itself.

      Show
      1. Create several question categories and sub-categories in a course (the more the better). 2. Confirm all categories are displayed OK in a drop-down on question/edit.php?courseid=N page. 3. Edit database by hand to create a loop: in table mdl_question_categories find a top-level (with parent=0) category from your course and change parent to the id of one of the children. 4. Confirm that an exception is thrown, instead of infinite loop and timeout (previous behaviour) 5 Repeat 3 & 4 but change mdl_question_categories.parent to ID of one of the grand-children and finally to the id of the top-level category itself.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:

      Description

      After an upgrade, we have got some question categories to point to themselves as a parent. That is mdl_question_categories.id == mdl_question_categories.parent.
      This is causing question_categorylist() function to go into infinite loop, until all the resources available (memory) is used up.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Tomasz Muras added a comment -

            I'd suggest we add a run-time check to prevent the infinite loop and throw an error. That's much better than potentially significantly degrade the performance. It will also make it easier to fix the problem and improve the user experience (error will be given instead of blank page or timeout).

            Show
            Tomasz Muras added a comment - I'd suggest we add a run-time check to prevent the infinite loop and throw an error. That's much better than potentially significantly degrade the performance. It will also make it easier to fix the problem and improve the user experience (error will be given instead of blank page or timeout).
            Hide
            Tim Hunt added a comment -

            Correct, we should make to code robust in the face of garbage data like this in the DB.

            Show
            Tim Hunt added a comment - Correct, we should make to code robust in the face of garbage data like this in the DB.
            Hide
            Tim Hunt added a comment -

            However, I am not sure your code goes far enough. Surely we can and should also detect longer loops like X is the parent category of Y, is the parent category of X.

            Show
            Tim Hunt added a comment - However, I am not sure your code goes far enough. Surely we can and should also detect longer loops like X is the parent category of Y, is the parent category of X.
            Hide
            Tomasz Muras added a comment -

            Yeah, good point - we should be detecting loops no matter how long they are. This will this call a bit slower but it's probably worth doing it. I'll have a look later on.

            Show
            Tomasz Muras added a comment - Yeah, good point - we should be detecting loops no matter how long they are. This will this call a bit slower but it's probably worth doing it. I'll have a look later on.
            Hide
            Tomasz Muras added a comment -

            OK, that should do - see the diff. As an added bonus I've removed recursion.

            Show
            Tomasz Muras added a comment - OK, that should do - see the diff. As an added bonus I've removed recursion.
            Hide
            Tim Hunt added a comment -

            This is definitely going in the right direction.

            You could make the code even more efficient: Use get_in_or_equal to process the whole of $templist each trip through the while loop.

            Also, using get_records_sql_menu, or get_recordset_sql, would probably improve the efficiency too.

            Picky points:

            1. Code checker says that

            // Comments should look like this.
            //not like this
            

            2. $templist is a bad variable name. It does not really say what this variable does.

            Show
            Tim Hunt added a comment - This is definitely going in the right direction. You could make the code even more efficient: Use get_in_or_equal to process the whole of $templist each trip through the while loop. Also, using get_records_sql_menu, or get_recordset_sql, would probably improve the efficiency too. Picky points: 1. Code checker says that // Comments should look like this. //not like this 2. $templist is a bad variable name. It does not really say what this variable does.
            Hide
            Tomasz Muras added a comment -

            I like the idea of limiting SQL queries, all done. It'll probably be easier to look at the full code than just a diff, here: https://github.com/enovation/moodle/blob/9f95b264e0bdfd618ce52336a1e23ec1f094eec9/lib/questionlib.php#L1179

            Show
            Tomasz Muras added a comment - I like the idea of limiting SQL queries, all done. It'll probably be easier to look at the full code than just a diff, here: https://github.com/enovation/moodle/blob/9f95b264e0bdfd618ce52336a1e23ec1f094eec9/lib/questionlib.php#L1179
            Hide
            Tim Hunt added a comment -

            Very nice! If you can just write some testing instructions, we can get this submitted for integration.

            Show
            Tim Hunt added a comment - Very nice! If you can just write some testing instructions, we can get this submitted for integration.
            Hide
            Tim Hunt added a comment -

            Great!

            As a bug fix, please can you cherry-pick to stable branches after you have rebased, after this week's weeklies.

            Show
            Tim Hunt added a comment - Great! As a bug fix, please can you cherry-pick to stable branches after you have rebased, after this week's weeklies.
            Hide
            Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Tomasz Muras added a comment -

            Rebased and cherry picked for 2.2 and 2.3 branches. Please review & integrate.

            Show
            Tomasz Muras added a comment - Rebased and cherry picked for 2.2 and 2.3 branches. Please review & integrate.
            Hide
            Sam Hemelryk added a comment -

            Thanks Tomasz and Tim, this has now been integrated.

            Show
            Sam Hemelryk added a comment - Thanks Tomasz and Tim, this has now been integrated.
            Hide
            Jason Fowler added a comment -

            Sorry Tomasz,

            in all branches: I haven't received the exception. Instead the categories just disappear completely.

            Show
            Jason Fowler added a comment - Sorry Tomasz, in all branches: I haven't received the exception. Instead the categories just disappear completely.
            Hide
            Dan Poltawski added a comment -

            Jason: just to confirm you were testing with DEBUG_DEVELOPER?

            Show
            Dan Poltawski added a comment - Jason: just to confirm you were testing with DEBUG_DEVELOPER?
            Hide
            Jason Fowler added a comment -

            $CFG->debug = 38911;
            $CFG->xmlstrictheaders = false;
            $CFG->debugdisplay = true;
            $CFG->perfdebug = 15;
            define('MDL_PERFDB' , true);
            $CFG->upgradeshowsql = true;
            $CFG->debugpageinfo = true;

            That's my config script section for debugging, as per instructions from Sam and Aparup ... is that sufficient?

            Show
            Jason Fowler added a comment - $CFG->debug = 38911; $CFG->xmlstrictheaders = false; $CFG->debugdisplay = true; $CFG->perfdebug = 15; define('MDL_PERFDB' , true); $CFG->upgradeshowsql = true; $CFG->debugpageinfo = true; That's my config script section for debugging, as per instructions from Sam and Aparup ... is that sufficient?
            Hide
            Tim Hunt added a comment -

            Right, I think that the problem is that this is very hard to test. I think the code is OK if you can work out how to test it.

            As Jason observed, once you have a cycle of categories, they do not show up in the dropdown, menu, so you cannot choose them. So, to reproduce the original infinite, look, which now throws an exception, do this:

            Suppose you start with three categories like

            • id = 192 Default for Test course (parent = 0)
              • id = 200 Subcategory (parent = 192)
                • id = 201 Sub-sub-category (parent = 200)

            Either:

            1. Add a random question picking from "Default for Test course" and all subcategories to the quiz, or
            2. Switch your question bank view so you are looking at category 192. (Make sure there is a bit like &category=192%2C109 in the URL.)

            Then, directly in the database, try editing as follows:

            1. Set the parent category of 192 to be 192, and reload the page. You should see the exception.
            2. Set the parent category of 192 to be 201, and reload the page. You should see the exception.
            3. Finally, you probably want to set the parent category of 192 back to 0.

            Of course, your category ids will probably be different, but you get the idea.

            Actually, I have just done all that, so why don't I assign myself as tester, and mark this as test passed?

            Show
            Tim Hunt added a comment - Right, I think that the problem is that this is very hard to test. I think the code is OK if you can work out how to test it. As Jason observed, once you have a cycle of categories, they do not show up in the dropdown, menu, so you cannot choose them. So, to reproduce the original infinite, look, which now throws an exception, do this: Suppose you start with three categories like id = 192 Default for Test course (parent = 0) id = 200 Subcategory (parent = 192) id = 201 Sub-sub-category (parent = 200) Either: Add a random question picking from "Default for Test course" and all subcategories to the quiz, or Switch your question bank view so you are looking at category 192. (Make sure there is a bit like &category=192%2C109 in the URL.) Then, directly in the database, try editing as follows: Set the parent category of 192 to be 192, and reload the page. You should see the exception. Set the parent category of 192 to be 201, and reload the page. You should see the exception. Finally, you probably want to set the parent category of 192 back to 0. Of course, your category ids will probably be different, but you get the idea. Actually, I have just done all that, so why don't I assign myself as tester, and mark this as test passed?
            Hide
            Tim Hunt added a comment -

            Oh, I do not have permission to change the status of this issue, even if I make myself tester. Also, since I peer reviewed this, I probably should not be tester.

            Show
            Tim Hunt added a comment - Oh, I do not have permission to change the status of this issue, even if I make myself tester. Also, since I peer reviewed this, I probably should not be tester.
            Hide
            Sam Hemelryk added a comment -

            Back to the testing phase it goes

            Show
            Sam Hemelryk added a comment - Back to the testing phase it goes
            Hide
            Tim Barker added a comment -

            This is assigned to Tim, David has offered to test it so I'll re-assign it to him.

            Show
            Tim Barker added a comment - This is assigned to Tim, David has offered to test it so I'll re-assign it to him.
            Hide
            David Monllaó added a comment -

            Tested in master and 22 following Tim comments (results explication with the same used IDs)

            I've been able to see the exception changing 192 parent to be 201, 192 and also 200 with a categories hierarchy like the one used by Tim:
            Default for COURSENAME (192 in Tim example)

            _cat1
            _cat11

            Then I added more categories to the structure to finish with something like this:
            Default for COURSENAME (192 in Tim example)

            _cat1
            _cat11
            _cat12
            _cat2

            With this scenario when changing the 192 parent to 201 I can see the exception, but not when changing 192 parent to 192 nor 200, the categories doesn't appear and the dropdown menu focuses on 'Default for COURSECATEGORYNAME' (with URL like &category=192%2C50) but there is no exception.

            No infinite loop in any case

            Show
            David Monllaó added a comment - Tested in master and 22 following Tim comments (results explication with the same used IDs) I've been able to see the exception changing 192 parent to be 201, 192 and also 200 with a categories hierarchy like the one used by Tim: Default for COURSENAME (192 in Tim example) _cat1 _cat11 Then I added more categories to the structure to finish with something like this: Default for COURSENAME (192 in Tim example) _cat1 _cat11 _cat12 _cat2 With this scenario when changing the 192 parent to 201 I can see the exception, but not when changing 192 parent to 192 nor 200, the categories doesn't appear and the dropdown menu focuses on 'Default for COURSECATEGORYNAME' (with URL like &category=192%2C50) but there is no exception. No infinite loop in any case
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Closing as fixed, many thanks for your awesome collaboration.

            Show
            Eloy Lafuente (stronk7) added a comment - Closing as fixed, many thanks for your awesome collaboration.

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: