Moodle
  1. Moodle
  2. MDL-30370 Meta: Oracle SQL issues
  3. MDL-30700

Error sorting assignment submissions by comment (in Oracle)

    Details

    • Database:
      Oracle
    • Testing Instructions:
      Hide

      Test pre-requisites - Oracle on Moodle 2.2, Moodle 2.3 and Moodle 2.5-dev

      1. Create an advanced uploading of files (Assignment 2.2) in a course with some students
      2. Click the "No attempts have been made on this assignment" link
      3. Grade the first student and add a feedback comment
      4. Grade the second student and add a feedback comment
      5. Click on the column header "Comment" to sort by the feedback comments
      6. Verify the column is sorted correctly by the feedback comments
      7. Click on the "Update" link for one of the submissions to update the feedback/grades
      8. Verify you see the grading page for that submission and not an error.
      Show
      Test pre-requisites - Oracle on Moodle 2.2, Moodle 2.3 and Moodle 2.5-dev Create an advanced uploading of files (Assignment 2.2) in a course with some students Click the "No attempts have been made on this assignment" link Grade the first student and add a feedback comment Grade the second student and add a feedback comment Click on the column header "Comment" to sort by the feedback comments Verify the column is sorted correctly by the feedback comments Click on the "Update" link for one of the submissions to update the feedback/grades Verify you see the grading page for that submission and not an error.
    • Workaround:
      Hide

      As far as clob datatype isn't allowed in an order by sentence, I suppose that those datatype columns should be casted in allowed datatypes somehow.

      Show
      As far as clob datatype isn't allowed in an order by sentence, I suppose that those datatype columns should be casted in allowed datatypes somehow.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30700-master
    • Rank:
      33529

      Description

      When sorting the assignment submissions by comment in Oracle it crashes with ORA-00932 inconsistent datatypes: expected - got clob

      * line 394 of \lib\dml\moodle_database.php: dml_read_exception thrown
      * line 268 of \lib\dml\oci_native_moodle_database.php: call to moodle_database->query_end()
      * line 1093 of \lib\dml\oci_native_moodle_database.php: call to oci_native_moodle_database->query_end()
      * line 1325 of \mod\assignment\lib.php: call to oci_native_moodle_database->get_records_sql()
      * line 649 of \mod\assignment\lib.php: call to assignment_base->display_submissions()
      * line 369 of \mod\assignment\type\upload\assignment.class.php: call to assignment_base->submissions()
      * line 57 of \mod\assignment\submissions.php: call to assignment_upload->submissions()
      

      This is because the comment column (mdl_assignment_submissions.submissioncomment) datatype is CLOB, and (at least in Oracle) that kind of columns aren't allowed in the order by sentence

        Issue Links

          Activity

          Hide
          Iñigo Zendegi added a comment -

          This issue is identical to MDL-30352, which was closed as a duplicate of MDL-29937, but I think that this is a differente issue as far as the symptoms aren't the same (on that case it just don't sort results as expected and in this case it crashes with a database error).

          Even if on both issues the problem is in the order by clause, here the problem is because the column datatype used in Oracle (clob) is not allowed in the order by sentence, and that makes the database throw an ORA-00932, and in MDL-29937 the problem is a not expected sort when the column has null values.

          Even so, I tried so apply the solution provided on the MDL-29937 (just in case), but it didn't work.

          Show
          Iñigo Zendegi added a comment - This issue is identical to MDL-30352 , which was closed as a duplicate of MDL-29937 , but I think that this is a differente issue as far as the symptoms aren't the same (on that case it just don't sort results as expected and in this case it crashes with a database error). Even if on both issues the problem is in the order by clause, here the problem is because the column datatype used in Oracle (clob) is not allowed in the order by sentence, and that makes the database throw an ORA-00932, and in MDL-29937 the problem is a not expected sort when the column has null values. Even so, I tried so apply the solution provided on the MDL-29937 (just in case), but it didn't work.
          Hide
          Michael de Raadt added a comment -

          Hi, Iñigo.

          Sorry, I didn't mean to ignore your comment on the closed issue, it had just fallen off my list.

          Clearly these problems are different, but it would make sense to work on the issues at the same time.

          I'll leave this open and make it a sub-task of the other issue.

          Show
          Michael de Raadt added a comment - Hi, Iñigo. Sorry, I didn't mean to ignore your comment on the closed issue, it had just fallen off my list. Clearly these problems are different, but it would make sense to work on the issues at the same time. I'll leave this open and make it a sub-task of the other issue.
          Hide
          Michael de Raadt added a comment -

          OK. This issue has now been duplicated.

          I had a bit of a look at the issue in the code.

          This appears to be due to the sorting on the flexible_table which is used as an input into the query that gathers the tables information.

          Adding the following would prevent sorting by comment...

          mod/assignment/lib.php, ~line 1370
          $table->no_sorting('submissioncomment');
          

          ...however I'm not sure why comparing a comment to another comment would be considered a problem in an order by.

          Logging out and logging in again should clear the session and revert the sort back to the default.

          Show
          Michael de Raadt added a comment - OK. This issue has now been duplicated. I had a bit of a look at the issue in the code. This appears to be due to the sorting on the flexible_table which is used as an input into the query that gathers the tables information. Adding the following would prevent sorting by comment... mod/assignment/lib.php, ~line 1370 $table->no_sorting('submissioncomment'); ...however I'm not sure why comparing a comment to another comment would be considered a problem in an order by. Logging out and logging in again should clear the session and revert the sort back to the default.
          Hide
          Iñigo Zendegi added a comment -

          Hi Michael,

          I tried adding that line but didn't work on Moodle 2.1.5, then I realized that's for Moodle 2.2.2.

          When I've added that on the right line for Moodle 2.1.5 (line 1304) it worked fine (now you cannot sort by comment).

          Anyway, the problem with comparing or sorting by comment is the datatype of this column (CLOB), as far as that kind of operations aren't allowed with this kind of columns.

          Thanks Michael!

          Show
          Iñigo Zendegi added a comment - Hi Michael, I tried adding that line but didn't work on Moodle 2.1.5, then I realized that's for Moodle 2.2.2. When I've added that on the right line for Moodle 2.1.5 (line 1304) it worked fine (now you cannot sort by comment). Anyway, the problem with comparing or sorting by comment is the datatype of this column (CLOB), as far as that kind of operations aren't allowed with this kind of columns. Thanks Michael!
          Hide
          Damyon Wiese added a comment -

          I added a function to flexible_table to allow specifying which columns require

          "DB->sql_order_by_text()"

          Tested on Oracle 11g

          Show
          Damyon Wiese added a comment - I added a function to flexible_table to allow specifying which columns require "DB->sql_order_by_text()" Tested on Oracle 11g
          Hide
          Damyon Wiese added a comment -

          I tested this change on master with Oracle and postgres (postgres worked before and after the change). I cleanly cherry-picked to 23 and 22 and verified that the function $DB->sql_order_by_text() exists in 23 and 22.

          Show
          Damyon Wiese added a comment - I tested this change on master with Oracle and postgres (postgres worked before and after the change). I cleanly cherry-picked to 23 and 22 and verified that the function $DB->sql_order_by_text() exists in 23 and 22.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The change looks spotty, so going to apply it, thanks!

          Side comment: It seems that we don't have any phpunit test covering flexible_table, and it really looks testable given its OOP nature. So I'd propose to create a new issue to cover it properly. For your consideration, Damyon/Michael.

          Show
          Eloy Lafuente (stronk7) added a comment - The change looks spotty, so going to apply it, thanks! Side comment: It seems that we don't have any phpunit test covering flexible_table, and it really looks testable given its OOP nature. So I'd propose to create a new issue to cover it properly. For your consideration, Damyon/Michael.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Also, I've added the api_change and dev_docs_required labels. This new function should be documented somewhere (upgrade.txt, release notes...).

          Show
          Eloy Lafuente (stronk7) added a comment - Also, I've added the api_change and dev_docs_required labels. This new function should be documented somewhere (upgrade.txt, release notes...).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (22, 23, 24 & master), thanks!

          PS: Doesn't the new assign module have any sorting by cLOB ? (just in case)

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (22, 23, 24 & master), thanks! PS: Doesn't the new assign module have any sorting by cLOB ? (just in case)
          Hide
          Damyon Wiese added a comment -

          Because the new assignment asks the plugins to generate their own table columns they all get set to no_sorting.

          Show
          Damyon Wiese added a comment - Because the new assignment asks the plugins to generate their own table columns they all get set to no_sorting.
          Hide
          David Monllaó added a comment -

          It fails, after setting the 2 students grade and feedback when I click 'Update' to update a student feedback I receive the attached error.

          Only tested in 22.

          Show
          David Monllaó added a comment - It fails, after setting the 2 students grade and feedback when I click 'Update' to update a student feedback I receive the attached error. Only tested in 22.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          David, which error?

          Show
          Eloy Lafuente (stronk7) added a comment - David, which error?
          Hide
          David Monllaó added a comment -

          Sorry Eloy, I forgot to attach the image, tomorrow morning (in about 10h) I'll post it

          Show
          David Monllaó added a comment - Sorry Eloy, I forgot to attach the image, tomorrow morning (in about 10h) I'll post it
          Hide
          Damyon Wiese added a comment -

          I just tried this and managed to get Davids error:

          ORA-00932: inconsistent datatypes: expected - got CLOB
          SELECT oracle_o.*
          FROM (SELECT oracle_i.*, rownum AS oracle_rownum
          FROM (SELECT u.id,u.picture,u.firstname,u.lastname,u.imagealt,u.email,u.lastaccess,
          s.id AS submissionid, s.grade, s.submissioncomment,
          s.timemodified, s.timemarked,
          CASE WHEN s.timemarked > 0 AND s.timemarked >= s.timemodified THEN 1
          ELSE 0 END AS status FROM ttuser u LEFT JOIN ttassignment_submissions s ON u.id = s.userid
          AND s.assignment = 2 WHERE u.id IN (161,257,508,107,649,175,184,422) ORDER BY submissioncomment DESC, lastname ASC ) oracle_i
          WHERE rownum <= :o_oracle_num_rows
          ) oracle_o
          WHERE oracle_rownum > :o_oracle_skip_rows
          [array (
          'o_oracle_num_rows' => 8,
          'o_oracle_skip_rows' => 6,
          )]

          I'll take a look.

          Show
          Damyon Wiese added a comment - I just tried this and managed to get Davids error: ORA-00932: inconsistent datatypes: expected - got CLOB SELECT oracle_o.* FROM (SELECT oracle_i.*, rownum AS oracle_rownum FROM (SELECT u.id,u.picture,u.firstname,u.lastname,u.imagealt,u.email,u.lastaccess, s.id AS submissionid, s.grade, s.submissioncomment, s.timemodified, s.timemarked, CASE WHEN s.timemarked > 0 AND s.timemarked >= s.timemodified THEN 1 ELSE 0 END AS status FROM ttuser u LEFT JOIN ttassignment_submissions s ON u.id = s.userid AND s.assignment = 2 WHERE u.id IN (161,257,508,107,649,175,184,422) ORDER BY submissioncomment DESC, lastname ASC ) oracle_i WHERE rownum <= :o_oracle_num_rows ) oracle_o WHERE oracle_rownum > :o_oracle_skip_rows [array ( 'o_oracle_num_rows' => 8, 'o_oracle_skip_rows' => 6, )] I'll take a look.
          Hide
          David Monllaó added a comment -

          Hi Damyon, yes, seems the same, I've (finally) attached the screenshot

          Show
          David Monllaó added a comment - Hi Damyon, yes, seems the same, I've (finally) attached the screenshot
          Hide
          Damyon Wiese added a comment -

          Added another patch to each of the branches to fix Davids issue.

          The fix is required because there is a static function get_sort_for_table which also needs to know which tables require text sorting. This patch adds the list of columns that need text sorting to the session (when the table is first setup) so that the parameters for the functions in tablelib do not require changing.

          Tested on 22 and master.

          Show
          Damyon Wiese added a comment - Added another patch to each of the branches to fix Davids issue. The fix is required because there is a static function get_sort_for_table which also needs to know which tables require text sorting. This patch adds the list of columns that need text sorting to the session (when the table is first setup) so that the parameters for the functions in tablelib do not require changing. Tested on 22 and master.
          Hide
          Damyon Wiese added a comment -

          Added to the test instructions to cover Davids fail case.

          Show
          Damyon Wiese added a comment - Added to the test instructions to cover Davids fail case.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          New commit added to all branches (22, 23, 24 & master). Thanks guys!

          Show
          Eloy Lafuente (stronk7) added a comment - New commit added to all branches (22, 23, 24 & master). Thanks guys!
          Hide
          David Monllaó added a comment -

          It passes. Tested in 22, 23 and master with the new testing instructions. Thanks to Aparup for let me use his VM

          Show
          David Monllaó added a comment - It passes. Tested in 22, 23 and master with the new testing instructions. Thanks to Aparup for let me use his VM
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao
          Hide
          Damyon Wiese added a comment -

          The API change here is:

          There is a new function "text_sorting($columnname)" for the class flexible_table which allows you to specify which columns are of type "text" so they can be sorted correctly in all databases (currently required for Oracle). This should be called during the table initialisation before the first call to "setup()".

          Show
          Damyon Wiese added a comment - The API change here is: There is a new function "text_sorting($columnname)" for the class flexible_table which allows you to specify which columns are of type "text" so they can be sorted correctly in all databases (currently required for Oracle). This should be called during the table initialisation before the first call to "setup()".

            People

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

              Dates

              • Created:
                Updated:
                Resolved: