Moodle
  1. Moodle
  2. MDL-26229

Invalid input syntax caused by repeated tag elements on restore (quiz, lesson)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.0.1
    • Fix Version/s: 2.0.2
    • Component/s: Backup, Quiz
    • Labels:
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      16284

      Description

      It looks like this error only occurs when you try to restore a course with quizzes and a quiz has no attempts. An invalid input syntax is returned with the full error looking like:

      ERROR: invalid input syntax for integer: "
      "
      INSERT INTO mdl_quiz
      (name,intro,introformat,timeopen,timeclose,optionflags,penaltyscheme,attempts,attemptonlast,grademethod,decimalpoints,questiondecimalpoints,review,questionsperpage,shufflequestions,shuffleanswers,questions,sumgrades,grade,timecreated,timemodified,timelimit,password,subnet,popup,delay1,delay2,showuserpicture,showblocks,course)
      VALUES($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16,$17,$18,$19,$20,$21,$22,$23,$24,$25,$26,$27,$28,$29,$30)
      RETURNING id
      [array (
      'name' => 'arrays',
      'intro' => '',
      'introformat' => '1',
      'timeopen' => 1292338151,
      'timeclose' => '0',
      'optionflags' => '0',
      'penaltyscheme' => '0',
      'attempts' => '
      ',
      'attemptonlast' => '0',
      'grademethod' => '3',
      'decimalpoints' => '2',
      'questiondecimalpoints' => '2',
      'review' => '71760879',
      'questionsperpage' => '0',
      'shufflequestions' => '0',
      'shuffleanswers' => '0',
      'questions' => '',
      'sumgrades' => '0.00000',
      'grade' => '0.00000',
      'timecreated' => 1295837491,
      'timemodified' => 1295837491,
      'timelimit' => '3600',
      'password' => '',
      'subnet' => '',
      'popup' => '1',
      'delay1' => '0',
      'delay2' => '0',
      'showuserpicture' => '0',
      'showblocks' => '0',
      'course' => 48,
      )]

      You can see where the 'attempts' elements is, it seems to contain a new line escape character which breaks the input rule of it only allowing an integer.

        Issue Links

          Activity

          Hide
          Jason Ilicic added a comment -

          Added an if statement in the restore library which checks if the 'attempts' element is numeric... if not, then it sets it to false because it's not needed.

          Patch can be found at: https://github.com/jasonilicic/moodle/commit/c9cf46bfa84afe86970695f56cdfc7e2aaafeef5

          Show
          Jason Ilicic added a comment - Added an if statement in the restore library which checks if the 'attempts' element is numeric... if not, then it sets it to false because it's not needed. Patch can be found at: https://github.com/jasonilicic/moodle/commit/c9cf46bfa84afe86970695f56cdfc7e2aaafeef5
          Hide
          Helen Foster added a comment -

          Jason, thanks for your report and patch. Increasing priority to blocker as the problem results in an error.

          Show
          Helen Foster added a comment - Jason, thanks for your report and patch. Increasing priority to blocker as the problem results in an error.
          Hide
          Ashley Holman added a comment -

          The problem occurs because the quiz XML has two <attempts> elements. One set to an integer (the quiz.attempts settings), and the other one is a nested tag with child <attempt> tags.

          Depending on the order which they appear in the XML, the restore parser might grab the nested tag which will return an empty string if there are no attempts in the backup file.

          Perhaps need to rename these tags to use different names.

          Show
          Ashley Holman added a comment - The problem occurs because the quiz XML has two <attempts> elements. One set to an integer (the quiz.attempts settings), and the other one is a nested tag with child <attempt> tags. Depending on the order which they appear in the XML, the restore parser might grab the nested tag which will return an empty string if there are no attempts in the backup file. Perhaps need to rename these tags to use different names.
          Hide
          Ashley Holman added a comment -

          note the linked patch is a workaround not a fix since it will force the attempts setting to be 0 when it could have been some other integer.

          Show
          Ashley Holman added a comment - note the linked patch is a workaround not a fix since it will force the attempts setting to be 0 when it could have been some other integer.
          Hide
          Martin Dougiamas added a comment -

          Tim and Eloy, can you advise on some good names?

          Show
          Martin Dougiamas added a comment - Tim and Eloy, can you advise on some good names?
          Hide
          Adam Olley added a comment -

          Example xml from a quiz.xml generated during a course backup:

          <?xml version="1.0" encoding="UTF-8"?>
          <activity id="959" moduleid="43766" modulename="quiz" contextid="44032">
          <quiz id="959">
          <name>coursename</name>
          <intro></intro>
          <introformat>1</introformat>
          <timeopen>1266212040</timeopen>
          <timeclose>1267941600</timeclose>
          <optionflags>0</optionflags>
          <penaltyscheme>0</penaltyscheme>
          <attempts>1</attempts>
          <attemptonlast>0</attemptonlast>
          <grademethod>4</grademethod>
          <decimalpoints>2</decimalpoints>
          <questiondecimalpoints>2</questiondecimalpoints>
          <review>71698464</review>
          <questionsperpage>0</questionsperpage>
          <shufflequestions>0</shufflequestions>
          <shuffleanswers>0</shuffleanswers>
          <questions></questions>
          <sumgrades>0.00000</sumgrades>
          <grade>70.00000</grade>
          <timecreated>1287216250</timecreated>
          <timemodified>1287216250</timemodified>
          <timelimit>3600</timelimit>
          <password></password>
          <subnet></subnet>
          <popup>1</popup>
          <delay1>0</delay1>
          <delay2>0</delay2>
          <showuserpicture>0</showuserpicture>
          <showblocks>0</showblocks>
          <question_instances>
          </question_instances>
          <feedbacks>
          </feedbacks>
          <overrides>
          </overrides>
          <grades>
          </grades>
          <attempts>
          </attempts>
          </quiz>
          </activity>

          The second set of attempts tags are used for for any attempts associated with the quiz, when not backing up userinfo, or if there are none, it's just empty (well, newline).

          Show
          Adam Olley added a comment - Example xml from a quiz.xml generated during a course backup: <?xml version="1.0" encoding="UTF-8"?> <activity id="959" moduleid="43766" modulename="quiz" contextid="44032"> <quiz id="959"> <name>coursename</name> <intro></intro> <introformat>1</introformat> <timeopen>1266212040</timeopen> <timeclose>1267941600</timeclose> <optionflags>0</optionflags> <penaltyscheme>0</penaltyscheme> <attempts>1</attempts> <attemptonlast>0</attemptonlast> <grademethod>4</grademethod> <decimalpoints>2</decimalpoints> <questiondecimalpoints>2</questiondecimalpoints> <review>71698464</review> <questionsperpage>0</questionsperpage> <shufflequestions>0</shufflequestions> <shuffleanswers>0</shuffleanswers> <questions></questions> <sumgrades>0.00000</sumgrades> <grade>70.00000</grade> <timecreated>1287216250</timecreated> <timemodified>1287216250</timemodified> <timelimit>3600</timelimit> <password></password> <subnet></subnet> <popup>1</popup> <delay1>0</delay1> <delay2>0</delay2> <showuserpicture>0</showuserpicture> <showblocks>0</showblocks> <question_instances> </question_instances> <feedbacks> </feedbacks> <overrides> </overrides> <grades> </grades> <attempts> </attempts> </quiz> </activity> The second set of attempts tags are used for for any attempts associated with the quiz, when not backing up userinfo, or if there are none, it's just empty (well, newline).
          Hide
          Ashley Holman added a comment -

          I suppose changing the XML tags will break backwards compatability. So might need to add some extra logic to the XML parsing to differentiate between the two <attempts> tags based on whether it's an integer or contains nested elements / newline.

          Show
          Ashley Holman added a comment - I suppose changing the XML tags will break backwards compatability. So might need to add some extra logic to the XML parsing to differentiate between the two <attempts> tags based on whether it's an integer or contains nested elements / newline.
          Hide
          Tim Hunt added a comment -

          What happens when backing up and restoring in 1.9? Ah, in 1.9, the first <ATTEMPTS> element, the one that corresponds to the quiz.attempts column, was put into an <ATTEMPTS_NUMBER> tag.

          So, this is definitely new breakage in 2.0. I have no yet wrapped my head around the 2.0 backup system, so I'm really looking for advice from Eloy here. Please.

          Show
          Tim Hunt added a comment - What happens when backing up and restoring in 1.9? Ah, in 1.9, the first <ATTEMPTS> element, the one that corresponds to the quiz.attempts column, was put into an <ATTEMPTS_NUMBER> tag. So, this is definitely new breakage in 2.0. I have no yet wrapped my head around the 2.0 backup system, so I'm really looking for advice from Eloy here. Please.
          Hide
          Adam Olley added a comment -

          The problem seems to be with the quiz backup and restore stepslibs.

          When creating a backup with or without userinfo, it creates the second set of 'attempts' tags that relate to the quiz_attempts table.
          When restoring a backup without userinfo, the restore stepslib doesn't add the 'attempts' path to set of paths the parser needs to know about, so when the xml parser gets to the second set of tags, it doesn't see it as part of the parentpaths and decides it must be part of the current "/activity/quiz" path.

          I've made a patch to the backup_quiz_stepslib that alleviates this issue when you're making a backup without userinfo by it not adding the extra 'attempts' tags at all since you're not backing that info up.

          Patch follows:

          diff --git a/mod/quiz/backup/moodle2/backup_quiz_stepslib.php b/mod/quiz/backup/moodle2/backup_quiz_stepslib.php
          index 7a05c5a..8d6b931 100644
          — a/mod/quiz/backup/moodle2/backup_quiz_stepslib.php
          +++ b/mod/quiz/backup/moodle2/backup_quiz_stepslib.php
          @@ -68,7 +68,9 @@ class backup_quiz_activity_structure_step extends backup_questions_activity_stru
          $grade = new backup_nested_element('grade', array('id'), array(
          'userid', 'gradeval', 'timemodified'));

          • $attempts = new backup_nested_element('attempts');
            + if ($userinfo) { + $attempts = new backup_nested_element('attempts'); + }

          $attempt = new backup_nested_element('attempt', array('id'), array(
          'uniqueid', 'userid', 'attemptnum', 'sumgrades',
          @@ -94,8 +96,10 @@ class backup_quiz_activity_structure_step extends backup_questions_activity_stru
          $quiz->add_child($grades);
          $grades->add_child($grade);

          • $quiz->add_child($attempts);
          • $attempts->add_child($attempt);
            + if ($userinfo) { + $quiz->add_child($attempts); + $attempts->add_child($attempt); + }
          Show
          Adam Olley added a comment - The problem seems to be with the quiz backup and restore stepslibs. When creating a backup with or without userinfo, it creates the second set of 'attempts' tags that relate to the quiz_attempts table. When restoring a backup without userinfo, the restore stepslib doesn't add the 'attempts' path to set of paths the parser needs to know about, so when the xml parser gets to the second set of tags, it doesn't see it as part of the parentpaths and decides it must be part of the current "/activity/quiz" path. I've made a patch to the backup_quiz_stepslib that alleviates this issue when you're making a backup without userinfo by it not adding the extra 'attempts' tags at all since you're not backing that info up. Patch follows: diff --git a/mod/quiz/backup/moodle2/backup_quiz_stepslib.php b/mod/quiz/backup/moodle2/backup_quiz_stepslib.php index 7a05c5a..8d6b931 100644 — a/mod/quiz/backup/moodle2/backup_quiz_stepslib.php +++ b/mod/quiz/backup/moodle2/backup_quiz_stepslib.php @@ -68,7 +68,9 @@ class backup_quiz_activity_structure_step extends backup_questions_activity_stru $grade = new backup_nested_element('grade', array('id'), array( 'userid', 'gradeval', 'timemodified')); $attempts = new backup_nested_element('attempts'); + if ($userinfo) { + $attempts = new backup_nested_element('attempts'); + } $attempt = new backup_nested_element('attempt', array('id'), array( 'uniqueid', 'userid', 'attemptnum', 'sumgrades', @@ -94,8 +96,10 @@ class backup_quiz_activity_structure_step extends backup_questions_activity_stru $quiz->add_child($grades); $grades->add_child($grade); $quiz->add_child($attempts); $attempts->add_child($attempt); + if ($userinfo) { + $quiz->add_child($attempts); + $attempts->add_child($attempt); + }
          Hide
          Adam Olley added a comment -

          Naturally comments messes up copy/pasted code.

          Attached the patch as a file to the ticket.

          Show
          Adam Olley added a comment - Naturally comments messes up copy/pasted code. Attached the patch as a file to the ticket.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi,

          my fault! I never should have named both elements the same way and forgot to alias the first one to attempts_num or whatever (like in 1.9). I remember myself doing tons of tests but surely I did them restoring with/without user info, but always with quizzes having attempts, so the problem wasn't reproducible.

          The main problem here is that the "second" attempts tag overwrites the 1st one and it's not fair to assume it will be 0/false, a.k.a. "unlimited attempts".

          So I will be:

          1) changing backup & restore from now to start using the "attempts_num" tag (as was in 1.9).
          2) try to go deeper in the XML parser to avoid the substitution commented above, so old (current) backups will restore ok. I'm not sure if this is going to be possible or no (perhaps some nasty harcoded trick could do the job, let's see).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, my fault! I never should have named both elements the same way and forgot to alias the first one to attempts_num or whatever (like in 1.9). I remember myself doing tons of tests but surely I did them restoring with/without user info, but always with quizzes having attempts, so the problem wasn't reproducible. The main problem here is that the "second" attempts tag overwrites the 1st one and it's not fair to assume it will be 0/false, a.k.a. "unlimited attempts". So I will be: 1) changing backup & restore from now to start using the "attempts_num" tag (as was in 1.9). 2) try to go deeper in the XML parser to avoid the substitution commented above, so old (current) backups will restore ok. I'm not sure if this is going to be possible or no (perhaps some nasty harcoded trick could do the job, let's see). Ciao
          Hide
          Tim Hunt added a comment -

          Sounds like the right solution Eloy.

          Show
          Tim Hunt added a comment - Sounds like the right solution Eloy.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Can anybody try this patch:

          https://github.com/stronk7/moodle/commit/9b97f1857773a4a5adf80ae3b6ad15593e41e68e

          and report if it causes the correct (the first) "attempts" tag to be applied properly in some of the offending backups?

          In the meantime, I'll be working on 1) above, renaming the tag and also adding some checks in backup to detect repetitions like this @ development stages.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Can anybody try this patch: https://github.com/stronk7/moodle/commit/9b97f1857773a4a5adf80ae3b6ad15593e41e68e and report if it causes the correct (the first) "attempts" tag to be applied properly in some of the offending backups? In the meantime, I'll be working on 1) above, renaming the tag and also adding some checks in backup to detect repetitions like this @ development stages. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Aha, detection now seems to work, also detected another wrongly-reused tag in lesson:

          /activity/lesson/highscores

          Improving a bit more the detection... going to change this issue title to cover both lesson and quiz.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Aha, detection now seems to work, also detected another wrongly-reused tag in lesson: /activity/lesson/highscores Improving a bit more the detection... going to change this issue title to cover both lesson and quiz. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Done,

          now quiz and lesson both use non-conflicting tags (lesson->showhighscores and quiz->attempts_number) on backup. And restore is able to handle both the old and the new tags.

          Also, when defining backup structures, those conflicts are detected @ development time, so this problem shouldn't happen anymore.

          Tested both with old and new course backups.

          Complete changes: https://github.com/stronk7/moodle/compare/master...MDL-26229_restore_attempts_wip
          Sent to integration as: PULL-268

          Thanks for the report and research, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Done, now quiz and lesson both use non-conflicting tags (lesson->showhighscores and quiz->attempts_number) on backup. And restore is able to handle both the old and the new tags. Also, when defining backup structures, those conflicts are detected @ development time, so this problem shouldn't happen anymore. Tested both with old and new course backups. Complete changes: https://github.com/stronk7/moodle/compare/master...MDL-26229_restore_attempts_wip Sent to integration as: PULL-268 Thanks for the report and research, ciao
          Hide
          Tim Hunt added a comment -

          Thanks Eloy. The patch looks good to me.

          Show
          Tim Hunt added a comment - Thanks Eloy. The patch looks good to me.
          Hide
          Helen Foster added a comment -

          This issue is fixed in the latest 2.0.1+ weekly. Thanks everyone!

          Show
          Helen Foster added a comment - This issue is fixed in the latest 2.0.1+ weekly. Thanks everyone!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: