Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.4, 2.2.1, 2.3.4, 2.4.1, 2.5
    • Fix Version/s: 2.5
    • Component/s: Backup
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide
      • Open a course
      • Turn editing on
      • Add a new lesson activity
      • Create a new 'Matching' question
      • Add some answers for each field
      • back up the module
        • Confirm that no errors were shown
      • Edit mod/lesson/backup/moodle2/backup_lesson_stepslib.php
      • find the call to set_source_table for lesson_answers ordering by id ASC.
      • change the 'id ASC' to 'id DESC'
      • back up the module
        • Confirm that no errors were shown
      • unzip both of the activity backups and diff the two activities/lesson_xx/lesson.xml files
        • confirm that the order for the answers is reversed on the 'DESC' version
      • restore the ASC backup
        • confirm that the question appears as expected with all answers ordered correctly
      Show
      Open a course Turn editing on Add a new lesson activity Create a new 'Matching' question Add some answers for each field back up the module Confirm that no errors were shown Edit mod/lesson/backup/moodle2/backup_lesson_stepslib.php find the call to set_source_table for lesson_answers ordering by id ASC. change the 'id ASC' to 'id DESC' back up the module Confirm that no errors were shown unzip both of the activity backups and diff the two activities/lesson_xx/lesson.xml files confirm that the order for the answers is reversed on the 'DESC' version restore the ASC backup confirm that the question appears as expected with all answers ordered correctly
    • Workaround:
      Hide

      Find and fix every occurrence of this issue individually

      Show
      Find and fix every occurrence of this issue individually
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
    • Rank:
      37907

      Description

      There are several places where the order of data in a backup is important, but is not necessarily enforced. These cases are difficult to track down because the natural return order of the content tends to match the insert order, and thus the id for a majority of the time.

      The attached patch enforces a sort order on the 'id' field which should satisfy a majority of cases.
      Where this is insufficient, it's possible to supply the sortby field to the set_source_table() function call.

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          This does cherry-pick cleanly to:

          • MOODLE_21_STABLE
          • MOODLE_22_STABLE

          but I can understand that it's probably a master-only change.

          Show
          Andrew Nicols added a comment - This does cherry-pick cleanly to: MOODLE_21_STABLE MOODLE_22_STABLE but I can understand that it's probably a master-only change.
          Hide
          Rajesh Taneja added a comment -

          I am not sure of this, hence putting it back for someone else to peer-review this.

          FYI: Might want to fix code-alignment.

          Show
          Rajesh Taneja added a comment - I am not sure of this, hence putting it back for someone else to peer-review this. FYI: Might want to fix code-alignment.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          My personal vote is -1 to this.

          Ideally, order shouldn't matter in any place and restore post_xxx methods should be able to set everything properly.

          If the previous paragraph makes things really harder, and the ordered backup is the preferred alternative I think it's better to do it explicitly (aka, changing those table sources to sql sources with visible "ORDER BY" clause).

          Also, forcing ordering to big recordsets by default can have a huge impact in performance, so better use it only if really necessary (previous paragraph).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - My personal vote is -1 to this. Ideally, order shouldn't matter in any place and restore post_xxx methods should be able to set everything properly. If the previous paragraph makes things really harder, and the ordered backup is the preferred alternative I think it's better to do it explicitly (aka, changing those table sources to sql sources with visible "ORDER BY" clause). Also, forcing ordering to big recordsets by default can have a huge impact in performance, so better use it only if really necessary (previous paragraph). Ciao
          Hide
          Andrew Nicols added a comment -

          Eloy,

          I wonder whether there's any scope in applying a part of this fix so that it's possible to call set_source_table() with an order by parameter, but have the default sort value of null.

          I realise it would be better if order didn't matter, but unfortunately we have many cases where it does. This would enable clearer backup code for those situations where it is required, without adding the performance hit most of the time.

          Any thoughts?

          Andrew

          Show
          Andrew Nicols added a comment - Eloy, I wonder whether there's any scope in applying a part of this fix so that it's possible to call set_source_table() with an order by parameter, but have the default sort value of null. I realise it would be better if order didn't matter, but unfortunately we have many cases where it does. This would enable clearer backup code for those situations where it is required, without adding the performance hit most of the time. Any thoughts? Andrew
          Hide
          Tim Hunt added a comment -

          +1 to Andrew's suggestion. Would similify some questions code.

          Show
          Tim Hunt added a comment - +1 to Andrew's suggestion. Would similify some questions code.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          ... without adding the performance hit most of the time.

          +        $this->tablesortby = 'id ASC';
          

          Show
          Eloy Lafuente (stronk7) added a comment - ... without adding the performance hit most of the time. + $ this ->tablesortby = 'id ASC';
          Hide
          Andrew Nicols added a comment -

          I haven't changed the patch yet, I was just wondering whether you'd object to adding the potential functionality without the default. E.g. a default of:

                  $this->tablesortby = null;
          
          Show
          Andrew Nicols added a comment - I haven't changed the patch yet, I was just wondering whether you'd object to adding the potential functionality without the default. E.g. a default of: $ this ->tablesortby = null ;
          Hide
          Andrew Nicols added a comment -

          I've updated the commit to not set a table sort order by default.

          The testing instructions I've provided are based on MDL-31386. Tim may have some better examples.

          Show
          Andrew Nicols added a comment - I've updated the commit to not set a table sort order by default. The testing instructions I've provided are based on MDL-31386 . Tim may have some better examples.
          Hide
          Tim Hunt added a comment -

          The changes in backuplib look good to me, but to motivate these changes, I think you should:

          1. grep the code for set_source_sql. (My search finds 69.)

          2. Identify all the ones that are only there to enable order-by to be specified.

          3. Fix them to take advantage of your new API.

          I think that should be done as a new commit on the same branch, and submitted for integration at the same time.

          Show
          Tim Hunt added a comment - The changes in backuplib look good to me, but to motivate these changes, I think you should: 1. grep the code for set_source_sql. (My search finds 69.) 2. Identify all the ones that are only there to enable order-by to be specified. 3. Fix them to take advantage of your new API. I think that should be done as a new commit on the same branch, and submitted for integration at the same time.
          Hide
          Andrew Nicols added a comment -

          I found a total of 19 cases in core which were using set_source_sql with an order by clause.
          Many of the users of set_source_sql were for the purpose of joins or OR tests.

          I've pushed an update to the branch. The changed locations are:

           backup/moodle2/backup_qtype_plugin.class.php                           |   14 ++------------
           backup/moodle2/backup_stepslib.php                                     |   19 ++++---------------
           mod/choice/backup/moodle2/backup_choice_stepslib.php                   |    7 +------
           mod/forum/backup/moodle2/backup_forum_stepslib.php                     |    5 +----
           mod/lesson/backup/moodle2/backup_lesson_stepslib.php                   |   13 ++-----------
           mod/scorm/backup/moodle2/backup_scorm_stepslib.php                     |   73 ++++++++++---------------------------------------------------------------
           question/type/match/backup/moodle2/backup_qtype_match_plugin.class.php |    7 +------
           7 files changed, 21 insertions(+), 117 deletions(-)
          
          Show
          Andrew Nicols added a comment - I found a total of 19 cases in core which were using set_source_sql with an order by clause. Many of the users of set_source_sql were for the purpose of joins or OR tests. I've pushed an update to the branch. The changed locations are: backup/moodle2/backup_qtype_plugin.class.php | 14 ++------------ backup/moodle2/backup_stepslib.php | 19 ++++--------------- mod/choice/backup/moodle2/backup_choice_stepslib.php | 7 +------ mod/forum/backup/moodle2/backup_forum_stepslib.php | 5 +---- mod/lesson/backup/moodle2/backup_lesson_stepslib.php | 13 ++----------- mod/scorm/backup/moodle2/backup_scorm_stepslib.php | 73 ++++++++++--------------------------------------------------------------- question/type/match/backup/moodle2/backup_qtype_match_plugin.class.php | 7 +------ 7 files changed, 21 insertions(+), 117 deletions(-)
          Hide
          Tim Hunt added a comment -

          Yay! Nice diff stat! +1 from me, though I think it would be better if Eloy peer reviewed this.

          Show
          Tim Hunt added a comment - Yay! Nice diff stat! +1 from me, though I think it would be better if Eloy peer reviewed this.
          Hide
          Andrew Nicols added a comment -

          Obviously, we could probably do with some updated test instructions for the affected areas:

          • qtype (qtype_plugin)
          • backup_questions_activity_structure_step::add_question_usages (backup/moodle2/backup_stepslib)
          • mod_choice (mod/choice/backup/moodle2/backup_choice_stepslib.php)
          • mod_forum (mod/forum/backup/moodle2/backup_forum_stepslib.php)
          • mod_lesson with multiple matching questions (mod/lesson/backup/moodle2/backup_lesson_stepslib.php)
          • mod_scorm with data (mod/scorm/backup/moodle2/backup_scorm_stepslib.php)
          Show
          Andrew Nicols added a comment - Obviously, we could probably do with some updated test instructions for the affected areas: qtype (qtype_plugin) backup_questions_activity_structure_step::add_question_usages (backup/moodle2/backup_stepslib) mod_choice (mod/choice/backup/moodle2/backup_choice_stepslib.php) mod_forum (mod/forum/backup/moodle2/backup_forum_stepslib.php) mod_lesson with multiple matching questions (mod/lesson/backup/moodle2/backup_lesson_stepslib.php) mod_scorm with data (mod/scorm/backup/moodle2/backup_scorm_stepslib.php)
          Hide
          Dan Poltawski added a comment -

          Hi Andrew,

          I like the patch. The git commit MDL- number is not this one, I think it would be better if it was this bug id. It's currently conflicting with master so please ensure its rebased and resolved.

          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [-] Language
          [Y] Databases
          [Y] Testing
          [Y] Security
          [-] Documentation
          [N] Git
          [Y] Sanity check

          I definitely would like to see Eloy comment on this, hopefully he'll integrate it

          Show
          Dan Poltawski added a comment - Hi Andrew, I like the patch. The git commit MDL- number is not this one, I think it would be better if it was this bug id. It's currently conflicting with master so please ensure its rebased and resolved. [Y] Syntax [-] Output [Y] Whitespace [-] Language [Y] Databases [Y] Testing [Y] Security [-] Documentation [N] Git [Y] Sanity check I definitely would like to see Eloy comment on this, hopefully he'll integrate it
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1 as far are ordering is now optional. And it, indeed, makes the backup code here and there much clearer.

          (note I was not against the idea, but about the fact that the previous 2 candidates were "forcing" ordering - by "id" if I'm not wrong - always. And that was nono from a performance POV.)

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - +1 as far are ordering is now optional. And it, indeed, makes the backup code here and there much clearer. (note I was not against the idea, but about the fact that the previous 2 candidates were "forcing" ordering - by "id" if I'm not wrong - always. And that was nono from a performance POV.) Thanks and ciao
          Hide
          Andrew Nicols added a comment -

          Thanking you kindly Eloy.

          Show
          Andrew Nicols added a comment - Thanking you kindly Eloy.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (master only), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          note I've amended the title to represent better the final solution implemented here and added the api/dev docs required labels.

          Show
          Eloy Lafuente (stronk7) added a comment - note I've amended the title to represent better the final solution implemented here and added the api/dev docs required labels.
          Hide
          Eloy Lafuente (stronk7) added a comment -
          Show
          Eloy Lafuente (stronk7) added a comment - backup/upgrade.txt now describes the change: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=bb8f31dbf3b938e9964c7a5023da445da26ec7ae Ciao
          Hide
          Adrian Greeve added a comment -

          Tested on the master integration branch.
          No errors found and sorting was correct.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on the master integration branch. No errors found and sorting was correct. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

          Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

          Thanks, closing as fixed!

          Show
          Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: