Moodle
  1. Moodle
  2. MDL-32215

course_sections table can contain duplicate sections

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      A)

      • Upgrade should be run on all DBS

      B)
      This test script should be carried out after the upgrade.

      1. Create a new course using all default settings (just enter the names).

      2. Add a label with some text in the third week.

      3. Drag the section around a bunch using the AJAX four-direction arrow thingy. After each move, reload the page to check that it really has moved. Move it to a few different weeks then to the last week and finally leave it at the top week (section 1).

      Note: You need to reload after each move to see if it worked, because the AJAX code ignores errors without reporting to user.

      Note: You may notice slightly odd behaviour with week numbers appearing/disappearing. This is probably a bug in the AJAX code and is not related to this change.

      4. Go to your user profile and turn AJAX off.

      5. Go back to the course page. Move the section with the label down a couple, then up a couple, using the up/down arrows. Verify there are no errors.

      7. Run unit tests (`phpunit courselib_testcase course/tests/courselib_test.php')

      Show
      A) Upgrade should be run on all DBS B) This test script should be carried out after the upgrade. 1. Create a new course using all default settings (just enter the names). 2. Add a label with some text in the third week. 3. Drag the section around a bunch using the AJAX four-direction arrow thingy. After each move, reload the page to check that it really has moved. Move it to a few different weeks then to the last week and finally leave it at the top week (section 1). Note: You need to reload after each move to see if it worked, because the AJAX code ignores errors without reporting to user. Note: You may notice slightly odd behaviour with week numbers appearing/disappearing. This is probably a bug in the AJAX code and is not related to this change. 4. Go to your user profile and turn AJAX off. 5. Go back to the course page. Move the section with the label down a couple, then up a couple, using the up/down arrows. Verify there are no errors. 7. Run unit tests (`phpunit courselib_testcase course/tests/courselib_test.php')
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-32215-master
    • Rank:
      38990

      Description

      It is possible for the course_sections table to contain multiple entries with the same 'course' (course id) and section (section number).

      This does not work correctly in the code because (a) what does that mean, and (b) get_all_sections uses section number as key for the array anyway.

      There is already an index on these two fields but it is not set to unique. I propose:

      1) In database update, find all duplicates. For each duplicate, delete all rows except one (leaving the 'original' row with lowest id).

      2) In same update, change index so that it is set to unique.

      3) Adding a unique index may cause problem in code which moves sections (e.g. if it swaps two sections A=2 and B=3 by first setting A to 3 then B to 2, this will conflict - to solve the problem, need to e.g. set A to 10000003 then B to 2 then A to 3). Find and fix this code.

      NOTE: In our VLE 2 system we do not have any such duplicates. In our 1.9 system we do, but only 30 rows. So this is probably a rare problem, however the change will make any future code modifications in this area (such as MDL-24419) safer.

        Issue Links

          Activity

          Sam Marshall created issue -
          Sam Marshall made changes -
          Field Original Value New Value
          Assignee moodle.com [ moodle.com ] Sam Marshall [ quen ]
          Hide
          Sam Marshall added a comment -

          Linking to MDL-24419, which includes a new section data cache. Should something go wrong with that cache, this change could make it easier to detect problems.

          Show
          Sam Marshall added a comment - Linking to MDL-24419 , which includes a new section data cache. Should something go wrong with that cache, this change could make it easier to detect problems.
          Sam Marshall made changes -
          Link This issue has a non-specific relationship to MDL-24419 [ MDL-24419 ]
          Hide
          Michael de Raadt added a comment -

          Thanks for suggesting this, Sam.

          I'd be interested to know where such duplicates could come from.

          Show
          Michael de Raadt added a comment - Thanks for suggesting this, Sam. I'd be interested to know where such duplicates could come from.
          Michael de Raadt made changes -
          Fix Version/s DEV backlog [ 10464 ]
          Labels triaged
          Sam Marshall made changes -
          Testing Instructions This test script should be carried out after the upgrade. It does not test changed code but is more of the 'test it isn't broken' variety.

          1. Create a new course using all default settings (just enter the names).

          2. Add a label with some text in the third week.

          3. Drag the section around a bunch using the AJAX four-direction arrow thingy. Check there are no errors (note: there are unlikely to be, because the AJAX code helpfully ignores them without reporting to user, but just in case). Eventually, leave the week as the first week.

          4. Reload the page and verify that the week actually is in the first week now.

          5. Go to your user profile and turn AJAX off.

          6. Go back to the course page. Move the section with the label down a couple, then up a couple, using the up/down arrows. Verify there are no errors.
          Sam Marshall made changes -
          Pull Master Diff URL https://github.com/sammarshallou/moodle/compare/master...MDL-32215-master
          Pull Master Branch MDL-32215-master
          Testing Instructions This test script should be carried out after the upgrade. It does not test changed code but is more of the 'test it isn't broken' variety.

          1. Create a new course using all default settings (just enter the names).

          2. Add a label with some text in the third week.

          3. Drag the section around a bunch using the AJAX four-direction arrow thingy. Check there are no errors (note: there are unlikely to be, because the AJAX code helpfully ignores them without reporting to user, but just in case). Eventually, leave the week as the first week.

          4. Reload the page and verify that the week actually is in the first week now.

          5. Go to your user profile and turn AJAX off.

          6. Go back to the course page. Move the section with the label down a couple, then up a couple, using the up/down arrows. Verify there are no errors.
          This test script should be carried out after the upgrade. It does not test changed code but is more of the 'test it isn't broken' variety.

          1. Create a new course using all default settings (just enter the names).

          2. Add a label with some text in the third week.

          3. Drag the section around a bunch using the AJAX four-direction arrow thingy. After each move, reload the page to check that it really has moved. Move it to a few different weeks then to the last week and finally leave it at the top week (section 1).

          Note: You need to reload after each move to see if it worked, because the AJAX code ignores errors without reporting to user.

          Note: You may notice slightly odd behaviour with week numbers appearing/disappearing. This is probably a bug in the AJAX code and is not related to this change.

          4. Go to your user profile and turn AJAX off.

          5. Go back to the course page. Move the section with the label down a couple, then up a couple, using the up/down arrows. Verify there are no errors.

          7. Run unit tests for course/simpletest/testcourselib.php
          Pull from Repository git://github.com/sammarshallou/moodle.git
          Hide
          Sam Marshall added a comment -

          Michael: I am unaware of any current code that would add duplicates and as noted there aren't any in our live Moodle 2 system (that started life as 2.0, it wasn't upgraded from 1.9), however there is also a comment in the code (I removed as part of this change) suggesting that there may be duplicates, and there are a few duplicates in our 1.9 system.

          In other words I think it's likely that at some point in the past there were bugs which could add duplicates, but hopefully there are none now. Still, you never know if there might be some in future, so it's safer if the database prevents it (given there is already an index so it might as well).

          In the current code it's also possible that duplicates might be added by odd race conditions if two people try to move sections at precisely the same time - especially as the code prior to this change did not use transactions even if available.

          Show
          Sam Marshall added a comment - Michael: I am unaware of any current code that would add duplicates and as noted there aren't any in our live Moodle 2 system (that started life as 2.0, it wasn't upgraded from 1.9), however there is also a comment in the code (I removed as part of this change) suggesting that there may be duplicates, and there are a few duplicates in our 1.9 system. In other words I think it's likely that at some point in the past there were bugs which could add duplicates, but hopefully there are none now. Still, you never know if there might be some in future, so it's safer if the database prevents it (given there is already an index so it might as well). In the current code it's also possible that duplicates might be added by odd race conditions if two people try to move sections at precisely the same time - especially as the code prior to this change did not use transactions even if available.
          Hide
          Sam Marshall added a comment -

          Requesting peer review.

          This is not a very important change as it stands, but it would be useful in conjunction with MDL-24419 to ensure that duplicate-section bugs are not introduced due to the section caching feature.

          A few notes:

          Runtime performance: When you move a section via AJAX, this change will increase the number of database updates if your update spans a large area because it updates each affected row twice. However, it reduces the number if your update spans a small area. For example, on a course with 20 sections, moving a section from 15 to 16 will require 4 set_field calls now vs 20 before; moving a section from 1 to 20 will require 40 vs 20 before. On average this probably makes it faster. The use of transactions also makes it faster by a large factor when using Postgres.

          Update performance: Drop and recreate of index might take a few moments, but the course_sections table is not large (e.g. if you have say 5,000 courses, typically it's probably going to be in the region of 100,000 rows) so it won't take very long to recreate the index.

          Risk of change: I think it's unlikely that most sites have many duplicate sections, and if they do, results are currently undefined (based on database order as to which record gets used), so this doesn't make matters worse.

          If anyone wants to check whether their own site contains duplicate sections (might be interesting), this kind of query works to list them all:

          select cs.* from 
          (select course,section from mdl_course_sections group by course, section having count(1) > 1) f
          inner join mdl_course_sections cs on cs.course=f.course and cs.section = f.section
          order by id
          
          Show
          Sam Marshall added a comment - Requesting peer review. This is not a very important change as it stands, but it would be useful in conjunction with MDL-24419 to ensure that duplicate-section bugs are not introduced due to the section caching feature. A few notes: Runtime performance: When you move a section via AJAX, this change will increase the number of database updates if your update spans a large area because it updates each affected row twice. However, it reduces the number if your update spans a small area. For example, on a course with 20 sections, moving a section from 15 to 16 will require 4 set_field calls now vs 20 before; moving a section from 1 to 20 will require 40 vs 20 before. On average this probably makes it faster. The use of transactions also makes it faster by a large factor when using Postgres. Update performance: Drop and recreate of index might take a few moments, but the course_sections table is not large (e.g. if you have say 5,000 courses, typically it's probably going to be in the region of 100,000 rows) so it won't take very long to recreate the index. Risk of change: I think it's unlikely that most sites have many duplicate sections, and if they do, results are currently undefined (based on database order as to which record gets used), so this doesn't make matters worse. If anyone wants to check whether their own site contains duplicate sections (might be interesting), this kind of query works to list them all: select cs.* from (select course,section from mdl_course_sections group by course, section having count(1) > 1) f inner join mdl_course_sections cs on cs.course=f.course and cs.section = f.section order by id
          Sam Marshall made changes -
          Status Open [ 1 ] Waiting for peer review [ 10012 ]
          Hide
          Sam Marshall added a comment -

          Forgot to say: While implementing this I searched for all cases of set_field on course_sections table setting the section field, and all uses of update_record on course_sections. So I hopefully got them all. (There are basically 2 different functions I had to change, one is used for AJAX and the other for non-AJAX section moving. Don't ask why it has different backend for two types of move...)

          Show
          Sam Marshall added a comment - Forgot to say: While implementing this I searched for all cases of set_field on course_sections table setting the section field, and all uses of update_record on course_sections. So I hopefully got them all. (There are basically 2 different functions I had to change, one is used for AJAX and the other for non-AJAX section moving. Don't ask why it has different backend for two types of move...)
          Sam Marshall made changes -
          Testing Instructions This test script should be carried out after the upgrade. It does not test changed code but is more of the 'test it isn't broken' variety.

          1. Create a new course using all default settings (just enter the names).

          2. Add a label with some text in the third week.

          3. Drag the section around a bunch using the AJAX four-direction arrow thingy. After each move, reload the page to check that it really has moved. Move it to a few different weeks then to the last week and finally leave it at the top week (section 1).

          Note: You need to reload after each move to see if it worked, because the AJAX code ignores errors without reporting to user.

          Note: You may notice slightly odd behaviour with week numbers appearing/disappearing. This is probably a bug in the AJAX code and is not related to this change.

          4. Go to your user profile and turn AJAX off.

          5. Go back to the course page. Move the section with the label down a couple, then up a couple, using the up/down arrows. Verify there are no errors.

          7. Run unit tests for course/simpletest/testcourselib.php
          This test script should be carried out after the upgrade.

          1. Create a new course using all default settings (just enter the names).

          2. Add a label with some text in the third week.

          3. Drag the section around a bunch using the AJAX four-direction arrow thingy. After each move, reload the page to check that it really has moved. Move it to a few different weeks then to the last week and finally leave it at the top week (section 1).

          Note: You need to reload after each move to see if it worked, because the AJAX code ignores errors without reporting to user.

          Note: You may notice slightly odd behaviour with week numbers appearing/disappearing. This is probably a bug in the AJAX code and is not related to this change.

          4. Go to your user profile and turn AJAX off.

          5. Go back to the course page. Move the section with the label down a couple, then up a couple, using the up/down arrows. Verify there are no errors.

          7. Run unit tests for course/simpletest/testcourselib.php
          Michael de Raadt made changes -
          Link This issue is duplicated by MDL-26706 [ MDL-26706 ]
          Michael de Raadt made changes -
          Link This issue is duplicated by MDL-26706 [ MDL-26706 ]
          Michael de Raadt made changes -
          Link This issue duplicates MDL-26706 [ MDL-26706 ]
          Michael de Raadt made changes -
          Labels triaged partner triaged
          Michael de Raadt made changes -
          Link This issue is duplicated by MDL-32331 [ MDL-32331 ]
          Michael de Raadt made changes -
          Priority Minor [ 4 ] Critical [ 2 ]
          David Mudrak made changes -
          Original Estimate 0 minutes [ 0 ]
          Remaining Estimate 0 minutes [ 0 ]
          Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
          Peer reviewer mudrd8mz
          Hide
          Michael de Raadt added a comment -

          Not that this makes any difference at this stage, bit I'm re-triaging this as a bug as there are obviously cases of this happening.

          Show
          Michael de Raadt added a comment - Not that this makes any difference at this stage, bit I'm re-triaging this as a bug as there are obviously cases of this happening.
          Michael de Raadt made changes -
          Issue Type Improvement [ 4 ] Bug [ 1 ]
          Fix Version/s STABLE backlog [ 10463 ]
          Fix Version/s DEV backlog [ 10464 ]
          Peer reviewer mudrd8mz
          David Mudrak made changes -
          Peer reviewer mudrd8mz
          Hide
          David Mudrak added a comment -

          Thanks for the patch sam. For the record, the only thing that came to my mind is using that 100000+ offset hack. It is theoretically unsafe, though practically it should work well unless someone uses similar hack for their own purposes (like hiding some course modules there). Alternative approach of using the negative int was mentioned in the chat (however, it could be used by others' hacks, too).

          +1 | I like it | Share it | Tweet it

          Show
          David Mudrak added a comment - Thanks for the patch sam. For the record, the only thing that came to my mind is using that 100000+ offset hack. It is theoretically unsafe, though practically it should work well unless someone uses similar hack for their own purposes (like hiding some course modules there). Alternative approach of using the negative int was mentioned in the chat (however, it could be used by others' hacks, too). +1 | I like it | Share it | Tweet it
          David Mudrak made changes -
          Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
          Hide
          Sam Marshall added a comment -

          Thanks for review David!

          I agree the negative number is neater now that I know that's possible, and have changed the code. I redid the manual test script and it still works.

          Also checked the phpunit testcase. (Not sure why the simpleunit test is still present.) As it turns out, the phpunit testcase did not need to be changed. This is because it only checks the end results (which are same as before); the simpleunit test was based on mock objects so it needed to know about the intermediate state. (I think the phpunit test is better...)

          Submitting for integration now.

          Show
          Sam Marshall added a comment - Thanks for review David! I agree the negative number is neater now that I know that's possible, and have changed the code. I redid the manual test script and it still works. Also checked the phpunit testcase. (Not sure why the simpleunit test is still present.) As it turns out, the phpunit testcase did not need to be changed. This is because it only checks the end results (which are same as before); the simpleunit test was based on mock objects so it needed to know about the intermediate state. (I think the phpunit test is better...) Submitting for integration now.
          Sam Marshall made changes -
          Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
          Testing Instructions This test script should be carried out after the upgrade.

          1. Create a new course using all default settings (just enter the names).

          2. Add a label with some text in the third week.

          3. Drag the section around a bunch using the AJAX four-direction arrow thingy. After each move, reload the page to check that it really has moved. Move it to a few different weeks then to the last week and finally leave it at the top week (section 1).

          Note: You need to reload after each move to see if it worked, because the AJAX code ignores errors without reporting to user.

          Note: You may notice slightly odd behaviour with week numbers appearing/disappearing. This is probably a bug in the AJAX code and is not related to this change.

          4. Go to your user profile and turn AJAX off.

          5. Go back to the course page. Move the section with the label down a couple, then up a couple, using the up/down arrows. Verify there are no errors.

          7. Run unit tests for course/simpletest/testcourselib.php
          This test script should be carried out after the upgrade.

          1. Create a new course using all default settings (just enter the names).

          2. Add a label with some text in the third week.

          3. Drag the section around a bunch using the AJAX four-direction arrow thingy. After each move, reload the page to check that it really has moved. Move it to a few different weeks then to the last week and finally leave it at the top week (section 1).

          Note: You need to reload after each move to see if it worked, because the AJAX code ignores errors without reporting to user.

          Note: You may notice slightly odd behaviour with week numbers appearing/disappearing. This is probably a bug in the AJAX code and is not related to this change.

          4. Go to your user profile and turn AJAX off.

          5. Go back to the course page. Move the section with the label down a couple, then up a couple, using the up/down arrows. Verify there are no errors.

          7. Run unit tests (course/simpletest/testcourselib.php and 'phpunit courselib_testcase course/tests/courselib_test.php')
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Dan Poltawski made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator poltawski
          Dan Poltawski made changes -
          Testing Instructions This test script should be carried out after the upgrade.

          1. Create a new course using all default settings (just enter the names).

          2. Add a label with some text in the third week.

          3. Drag the section around a bunch using the AJAX four-direction arrow thingy. After each move, reload the page to check that it really has moved. Move it to a few different weeks then to the last week and finally leave it at the top week (section 1).

          Note: You need to reload after each move to see if it worked, because the AJAX code ignores errors without reporting to user.

          Note: You may notice slightly odd behaviour with week numbers appearing/disappearing. This is probably a bug in the AJAX code and is not related to this change.

          4. Go to your user profile and turn AJAX off.

          5. Go back to the course page. Move the section with the label down a couple, then up a couple, using the up/down arrows. Verify there are no errors.

          7. Run unit tests (course/simpletest/testcourselib.php and 'phpunit courselib_testcase course/tests/courselib_test.php')
          A)
          * Upgrade should be run on all DBS

          B)
          This test script should be carried out after the upgrade.

          1. Create a new course using all default settings (just enter the names).

          2. Add a label with some text in the third week.

          3. Drag the section around a bunch using the AJAX four-direction arrow thingy. After each move, reload the page to check that it really has moved. Move it to a few different weeks then to the last week and finally leave it at the top week (section 1).

          Note: You need to reload after each move to see if it worked, because the AJAX code ignores errors without reporting to user.

          Note: You may notice slightly odd behaviour with week numbers appearing/disappearing. This is probably a bug in the AJAX code and is not related to this change.

          4. Go to your user profile and turn AJAX off.

          5. Go back to the course page. Move the section with the label down a couple, then up a couple, using the up/down arrows. Verify there are no errors.

          7. Run unit tests (`phpunit courselib_testcase course/tests/courselib_test.php')
          Hide
          Dan Poltawski added a comment -

          Ironically I was playing with course formats this weekend and introduced duplicate sections. Moodle broke pretty badly so I think this is a safe thing to add..

          Show
          Dan Poltawski added a comment - Ironically I was playing with course formats this weekend and introduced duplicate sections. Moodle broke pretty badly so I think this is a safe thing to add..
          Hide
          Dan Poltawski added a comment -

          Hi Sam,

          I've integrated this now. Note I discovered some weird line ending which I fixed up in the merge commit, I also removed the simpletest change as that file has now been removed.

          Show
          Dan Poltawski added a comment - Hi Sam, I've integrated this now. Note I discovered some weird line ending which I fixed up in the merge commit, I also removed the simpletest change as that file has now been removed.
          Dan Poltawski made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          Fix Version/s 2.3 [ 10657 ]
          Fix Version/s STABLE backlog [ 10463 ]
          Michael de Raadt made changes -
          Tester salvetore
          Hide
          Michael de Raadt added a comment -

          I've started testing this and I thought I should note my progress.

          I've tested this in master with PostgreSQL, MySQL and MS SQL without any problems appearing.

          I'm waiting for my Oracle instance to complete its punishment of my machine while it runs PHPUnit tests. I will complete this test when it is done.

          Show
          Michael de Raadt added a comment - I've started testing this and I thought I should note my progress. I've tested this in master with PostgreSQL, MySQL and MS SQL without any problems appearing. I'm waiting for my Oracle instance to complete its punishment of my machine while it runs PHPUnit tests. I will complete this test when it is done.
          Michael de Raadt made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Hide
          Michael de Raadt added a comment -

          Test result: Success

          Tested under all DBs, with and without AJAX.

          PHPUnit tests passed.

          Show
          Michael de Raadt added a comment - Test result: Success Tested under all DBs, with and without AJAX. PHPUnit tests passed.
          Michael de Raadt made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been near becoming rejected, because it's not the best code you are able to produce.

          But, luckily, at the end, it has landed and has been spread to all repos out there.

          Many thanks and, don't forget it, keep improving your skills, you can!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been near becoming rejected, because it's not the best code you are able to produce. But, luckily, at the end, it has landed and has been spread to all repos out there. Many thanks and, don't forget it, keep improving your skills, you can! Closing, ciao
          Eloy Lafuente (stronk7) made changes -
          Status Tested [ 10006 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Currently in integration Yes [ 10041 ]
          Integration date 27/Apr/12
          Hide
          Dan Poltawski added a comment -

          We caused a regression with this by calling rebuild_course_cache() in the upgrade. (Then you changed the behaviour of that function later on )

          Linking MDL-33915 for the report.

          Show
          Dan Poltawski added a comment - We caused a regression with this by calling rebuild_course_cache() in the upgrade. (Then you changed the behaviour of that function later on ) Linking MDL-33915 for the report.
          Dan Poltawski made changes -
          Link This issue caused a regression MDL-33915 [ MDL-33915 ]

            People

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

              Dates

              • Created:
                Updated:
                Resolved: