Moodle
  1. Moodle
  2. MDL-32178

Duplicate shortname can crash database enrolment sync

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      38925

      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.

        Activity

        Hide
        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
        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
        Petr Škoda added a comment -

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

        Show
        Petr Škoda added a comment - Hello, the sync script stops when it finds invalid data, I would not personally call it a "crash".
        Hide
        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
        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
        Petr Škoda added a comment -

        idnumber duplicates result in the same error message

        Show
        Petr Škoda added a comment - idnumber duplicates result in the same error message
        Hide
        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
        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
        Petr Škoda added a comment -

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

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

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

        Show
        Michael Aherne added a comment - Thanks for the feedback Petr! I've updated the patch to check the database before calling create_course().
        Hide
        Petr Škoda 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
        Petr Škoda 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
        Sam Hemelryk added a comment -

        Thanks Petr - this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Petr - this has been integrated now
        Hide
        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
        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
        Petr Škoda 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
        Petr Škoda 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
        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
        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
        Michael de Raadt added a comment -

        Also tested in 2.2.

        Show
        Michael de Raadt added a comment - Also tested in 2.2.
        Hide
        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
        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: