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:
    • Rank:
      44260

      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.

        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: