Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-32178

Duplicate shortname can crash database enrolment sync

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.2
    • Fix Version/s: 2.2.3
    • Component/s: Enrolments
    • Labels:
    • Testing Instructions:
      Hide

      1/ create external course table with duplicates in idnumber and shortname
      2/ set up enrol_database sync
      3/ sync course from cli in verbose mode
      4/ expect verbose message during the first execution only

      Show
      1/ create external course table with duplicates in idnumber and shortname 2/ set up enrol_database sync 3/ sync course from cli in verbose mode 4/ expect verbose message during the first execution only
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w19_MDL-32178_m23_dbenrolignore

      Description

      In database enrolment, if two new course records appear with the same shortname, the process will crash with the following error message:

      !!! Short name is already used for another course !!!
      !! Stack trace: * line 3840 of \course\lib.php: moodle_exception thrown
      * line 730 of \enrol\database\lib.php: call to create_course()
      * line 78 of \enrol\database\cli\sync.php: call to enrol_database_plugin->sync_courses()

      NB: This does not happen if one of the courses already existed before the script is run - in that case, the duplicate record is just ignored - it only happens when both records are new.

        Gliffy Diagrams

          Activity

          Hide
          maherne Michael Aherne added a comment - - edited

          I've added a pull request which just wraps the course_create in a try/catch block, although I wonder if it might be better to check new courses against the existing $coursecreate array as well as the database earlier in the script?

          Show
          maherne Michael Aherne added a comment - - edited I've added a pull request which just wraps the course_create in a try/catch block, although I wonder if it might be better to check new courses against the existing $coursecreate array as well as the database earlier in the script?
          Hide
          skodak Petr Skoda added a comment -

          Hello, the sync script stops when it finds invalid data, I would not personally call it a "crash".

          Show
          skodak Petr Skoda added a comment - Hello, the sync script stops when it finds invalid data, I would not personally call it a "crash".
          Hide
          maherne Michael Aherne added a comment -

          Hi Petr. Sorry - maybe calling it a "crash" was a bit excessive, but the script doesn't always stop when it finds invalid data, so this behaviour seems a little inconsistent. For example, if there are two courses in the external database with the same idnumber, and one of them has already been created in Moodle during a previous execution of the script, it just outputs an error message and the sync continues.

          I realise the real issue here is that people need to ensure their external database has accurate data in it, but it would be good to have consistent behaviour when the script encounters bad data.

          Show
          maherne Michael Aherne added a comment - Hi Petr. Sorry - maybe calling it a "crash" was a bit excessive, but the script doesn't always stop when it finds invalid data, so this behaviour seems a little inconsistent. For example, if there are two courses in the external database with the same idnumber, and one of them has already been created in Moodle during a previous execution of the script, it just outputs an error message and the sync continues. I realise the real issue here is that people need to ensure their external database has accurate data in it, but it would be good to have consistent behaviour when the script encounters bad data.
          Hide
          skodak Petr Skoda added a comment -

          idnumber duplicates result in the same error message

          Show
          skodak Petr Skoda added a comment - idnumber duplicates result in the same error message
          Hide
          maherne Michael Aherne added a comment -

          When I tested this, idnumber duplicates were caught before the call to create_course(), and a warning message output, provided they already existed in the database. This error only occurred if the duplicate idnumbers were both on completely new records. This is why I say it is inconsistent, as there are two different, but very similar cases:

          1. A course record with the same idnumber as an existing (database enrolled) Moodle course appears in the external database.
          2. Two course records with identical idnumbers which don't yet exist in Moodle appear in the external database.

          In the first case a warning is output and the sync process continues, while in the second, the sync stops completely and no enrolments are actually synchronised.

          As I say, I understand that this is an edge case and that the real answer is to fix the external database, so if you'd prefer to close it as a "won't fix" issue I'd be OK with that.

          Show
          maherne Michael Aherne added a comment - When I tested this, idnumber duplicates were caught before the call to create_course(), and a warning message output, provided they already existed in the database. This error only occurred if the duplicate idnumbers were both on completely new records. This is why I say it is inconsistent, as there are two different, but very similar cases: A course record with the same idnumber as an existing (database enrolled) Moodle course appears in the external database. Two course records with identical idnumbers which don't yet exist in Moodle appear in the external database. In the first case a warning is output and the sync process continues, while in the second, the sync stops completely and no enrolments are actually synchronised. As I say, I understand that this is an edge case and that the real answer is to fix the external database, so if you'd prefer to close it as a "won't fix" issue I'd be OK with that.
          Hide
          skodak Petr Skoda added a comment -

          I think we can add extra record_exists() calls right before the craete_course(), the performance cost is negligible.

          Show
          skodak Petr Skoda added a comment - I think we can add extra record_exists() calls right before the craete_course(), the performance cost is negligible.
          Hide
          maherne Michael Aherne added a comment -

          Thanks for the feedback Petr! I've updated the patch to check the database before calling create_course().

          Show
          maherne Michael Aherne added a comment - Thanks for the feedback Petr! I've updated the patch to check the database before calling create_course().
          Hide
          skodak Petr Skoda added a comment -

          Thanks for the report and patch, I did not use the try {} catch because it is really the responsibility of admin to make sure there is no invalid data in external tables.

          Show
          skodak Petr Skoda added a comment - Thanks for the report and patch, I did not use the try {} catch because it is really the responsibility of admin to make sure there is no invalid data in external tables.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Petr - this has been integrated now

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Petr - this has been integrated now
          Hide
          phalacee Jason Fowler added a comment -

          Petr, could you please eexpand on step 1, I have no idea what needs to go into the table to satisfy this test

          Show
          phalacee Jason Fowler added a comment - Petr, could you please eexpand on step 1, I have no idea what needs to go into the table to satisfy this test
          Hide
          skodak Petr Skoda added a comment -

          http://docs.moodle.org/22/en/External_database_enrolment contains information what enrol_database is and how to use it, please add more info there if necessary, thanks

          Show
          skodak Petr Skoda added a comment - http://docs.moodle.org/22/en/External_database_enrolment contains information what enrol_database is and how to use it, please add more info there if necessary, thanks
          Hide
          salvetore Michael de Raadt added a comment -

          Test result: Success

          Tested in master.

          Duplicate shortname in course sync produced...

          can not insert new course, duplicate shortname detected: Master Test (MySQL) 104

          ...and kept going.

          Duplicate idnumber in course sync produced...

          error: duplicate idnumber, can not create course: Master Test (MySQL) 107 [104]

          ...and kept going.

          Show
          salvetore Michael de Raadt added a comment - Test result: Success Tested in master. Duplicate shortname in course sync produced... can not insert new course, duplicate shortname detected: Master Test (MySQL) 104 ...and kept going. Duplicate idnumber in course sync produced... error: duplicate idnumber, can not create course: Master Test (MySQL) 107 [104] ...and kept going.
          Hide
          salvetore Michael de Raadt added a comment -

          Also tested in 2.2.

          Show
          salvetore Michael de Raadt added a comment - Also tested in 2.2.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome?

          Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U

          Closing, ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome? Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                14/May/12