Moodle
  1. Moodle
  2. MDL-29496

Course Search breaks if summary is NULL

    Details

    • Database:
      PostgreSQL
    • Testing Instructions:
      Hide

      1) Create a new course using either the LDAP, or database auth mechanisms - these use create_course() without the second (optional) argument - alternatively, update an existing course to set the summary to NULL.
      2) Try and search for that course.
      3) Run the DB functional tests under all DBs. Verify now failure related to test_coalesce() happens.

      Show
      1) Create a new course using either the LDAP, or database auth mechanisms - these use create_course() without the second (optional) argument - alternatively, update an existing course to set the summary to NULL. 2) Try and search for that course. 3) Run the DB functional tests under all DBs. Verify now failure related to test_coalesce() happens.
    • Workaround:
      Hide

      Set a summary for the course

      Show
      Set a summary for the course
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-29496-master
    • Rank:
      18979

      Description

      If courses have been created by an enrolment plugin which doesn't provide any summary, then the summary field is set to null.

      Under Postgres (and probably other DBs) when the fullname and summary are concatenated, the presence of the NULL summary field means that the resulting concatenation is also null.

      E.g.:
      If summary = 'foo', and fullname = 'bar', then summary || ' ' || fullname = 'foo bar'
      If summary IS NULL, and fullname = 'bar', then summary || ' ' || fulname = NULL

      As a result, the search fails to match on valid fullname matches.

      I'm unsure as to how compatible COALESCE is, but under Postgres, COALESCEing the summary, and an empty string ensures that we can search if the summary is NULL.
      For example:

        $concat = $DB->sql_concat('c.summary', "' '", 'c.fullname', "' '", 'c.idnumber', "' '", 'c.shortname');
      

      becomes:

        $concat = $DB->sql_concat("COALESCE(c.summary, '')", "' '", 'c.fullname', "' '", 'c.idnumber', "' '", 'c.shortname');
      

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that, certainly this might resolve a number of mystery issues.

          Perhaps it's safer to check the value in code before passing it to the query.

          Show
          Michael de Raadt added a comment - Thanks for reporting that, certainly this might resolve a number of mystery issues. Perhaps it's safer to check the value in code before passing it to the query.
          Hide
          Andrew Nicols added a comment -

          Not sure how you could check the value in code since it's the null value in the table that's the problem, and not a null search.

          A quick git grep reveals that COALESCE is used in numerous placed in Moodle, so it should be safe to COALESCE, c.summary and an empty string together in this case (at least, there is precedent for it).

          Show
          Andrew Nicols added a comment - Not sure how you could check the value in code since it's the null value in the table that's the problem, and not a null search. A quick git grep reveals that COALESCE is used in numerous placed in Moodle, so it should be safe to COALESCE, c.summary and an empty string together in this case (at least, there is precedent for it).
          Hide
          Jason Fowler added a comment -

          Applied patch from the description

          Show
          Jason Fowler added a comment - Applied patch from the description
          Hide
          Ankit Agarwal added a comment -

          Hi Jason,
          It might be a good idea to use $DB->sql_empty() instead of empty string ('') as oracle treats empty strings as NULL.

          COALESCE(c.summary, $DB->sql_empty())
          

          Rest looks good.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Jason, It might be a good idea to use $DB->sql_empty() instead of empty string ('') as oracle treats empty strings as NULL. COALESCE(c.summary, $DB->sql_empty()) Rest looks good. Thanks
          Hide
          Jason Fowler added a comment -

          Good point, will do.

          Show
          Jason Fowler added a comment - Good point, will do.
          Hide
          Andrew Nicols added a comment -

          I wonder how this will work with the actual search then... The original problem is that a string concatenated onto a NULL will result in a NULL. Can anyone with access to an Oracle DB test this?

          That's to say, if c.summary is NULL, and $DB->sql_empty() returns NULL, how will a text search on 'foo' || NULL work with Oracle?

          Show
          Andrew Nicols added a comment - I wonder how this will work with the actual search then... The original problem is that a string concatenated onto a NULL will result in a NULL. Can anyone with access to an Oracle DB test this? That's to say, if c.summary is NULL, and $DB->sql_empty() returns NULL, how will a text search on 'foo' || NULL work with Oracle?
          Hide
          Ankit Agarwal added a comment -

          Hi Andrew,
          As far as my understanding goes, $DB->sql_empty() doesn't return NULL. It returns empty character supported by every database. (This character wont be considered as NULL by any DB).
          Reference:- http://docs.moodle.org/dev/DML_functions

          +1 to testing this on Oracle tough.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Andrew, As far as my understanding goes, $DB->sql_empty() doesn't return NULL. It returns empty character supported by every database. (This character wont be considered as NULL by any DB). Reference:- http://docs.moodle.org/dev/DML_functions +1 to testing this on Oracle tough. Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          LOL, really in this case, the $DB->sql_empty() is not necessary as far as Oracle is the ONLY ONE NOT ANSI DB when concatenating, so it 'foo' || null returns 'foo'.

          You can confirm it by running the DB unit tests under Oracle. The only test failing under a properly configured server is exactly that: concat not behaving in ANSI mode.

          So it does not need the COALESCE() (although support it, afaik) nor the sql_empty().

          I just would introduce one new unit test @ lib/dml/simpletest/testdml.php, surely in the test_get_records_sql_complicated() method, to verify that we won't run under problems with COALESCE at all (and will add the execution of unit tests to testing instructions).

          Halting this till fixed/completed. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited LOL, really in this case, the $DB->sql_empty() is not necessary as far as Oracle is the ONLY ONE NOT ANSI DB when concatenating, so it 'foo' || null returns 'foo'. You can confirm it by running the DB unit tests under Oracle. The only test failing under a properly configured server is exactly that: concat not behaving in ANSI mode. So it does not need the COALESCE() (although support it, afaik) nor the sql_empty(). I just would introduce one new unit test @ lib/dml/simpletest/testdml.php, surely in the test_get_records_sql_complicated() method, to verify that we won't run under problems with COALESCE at all (and will add the execution of unit tests to testing instructions). Halting this till fixed/completed. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Jason,

          I've written on new unit test to verify cross-db operations of COALESCE(). Please, consider cherry-picking it to your branches:

          https://github.com/stronk7/moodle/compare/master...MDL-29496

          Ciao

          PS: I've tested it under mysqli, pgsql, mssql and oci. Passing under all them ok. Only sqlsrv needs testing (I've not it here).

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Jason, I've written on new unit test to verify cross-db operations of COALESCE(). Please, consider cherry-picking it to your branches: https://github.com/stronk7/moodle/compare/master...MDL-29496 Ciao PS: I've tested it under mysqli, pgsql, mssql and oci. Passing under all them ok. Only sqlsrv needs testing (I've not it here).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks for adding the coalesce() tests, Jason.

          Oki, so there we go, integrating...

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks for adding the coalesce() tests, Jason. Oki, so there we go, integrating...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Sam Hemelryk added a comment -

          Passing this now thanks guys.

          Show
          Sam Hemelryk added a comment - Passing this now thanks guys.
          Hide
          Jason Fowler added a comment -

          My first issue passed through to a release!

          Show
          Jason Fowler added a comment - My first issue passed through to a release!
          Hide
          Ankit Agarwal added a comment -

          Congrats

          Show
          Ankit Agarwal added a comment - Congrats
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for all the hard work. This is now part of Moodle, your favorite LMS.

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for all the hard work. This is now part of Moodle, your favorite LMS. Closing as fixed, ciao
          Hide
          Ryan Smith added a comment - - edited

          I just updated our test server with this week's Moodle release that contains this tracker fix. The lib/datalib.php file change (from this fix, I presume) is causing the course search to fail with an error reading from the database. We are using MySQL 5.5.16.

          The error log contains (when you search for something):

          [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: Default exception handler: Error reading from database Debug: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '), ' ', c.fullname, ' ', c.idnumber, ' ', c.shortname)) LIKE LOWER('%trest%') CO' at line 4, referer: http://mymoodleserver/moodle/course/index.php
          [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: SELECT c.* , ctx.id AS ctxid, ctx.path AS ctxpath, ctx.depth AS ctxdepth, ctx.contextlevel AS ctxlevel, ctx.instanceid AS ctxinstance, referer: http://mymoodleserver/moodle/course/index.php
          [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: FROM mdl_course c, referer: http://mymoodleserver/moodle/course/index.php
          [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: LEFT JOIN mdl_context ctx ON (ctx.instanceid = c.id AND ctx.contextlevel = 50), referer: http://mymoodleserver/moodle/course/index.php
          [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: WHERE LOWER(CONCAT(COALESCE(c.summary, ), ' ', c.fullname, ' ', c.idnumber, ' ', c.shortname)) LIKE LOWER COLLATE utf8_bin ESCAPE '\\\\' AND c.id <> 1, referer: http://mymoodleserver/moodle/course/index.php
          [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: ORDER BY fullname ASC, referer: http://mymoodleserver/moodle/course/index.php
          [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: [array (, referer: http://mymoodleserver/moodle/course/index.php
          [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: 0 => '%trest%',, referer: http://mymoodleserver/moodle/course/index.php
          [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: )], referer: http://mymoodleserver/moodle/course/index.php
          [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: * line 394 of \\lib\\dml
          moodle_database.php: dml_read_exception thrown, referer: http://mymoodleserver/moodle/course/index.php
          [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: * line 768 of \\lib\\dml
          mysqli_native_moodle_database.php: call to moodle_database->query_end(), referer: http://mymoodleserver/moodle/course/index.php
          [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: * line 778 of \\lib
          datalib.php: call to mysqli_native_moodle_database->get_recordset_sql(), referer: http://mymoodleserver/moodle/course/index.php
          [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: * line 171 of \\course
          search.php: call to get_courses_search(), referer: http://mymoodleserver/moodle/course/index.php

          When I restore lib/datalib.php from the previous week's version, the course search works again.

          Show
          Ryan Smith added a comment - - edited I just updated our test server with this week's Moodle release that contains this tracker fix. The lib/datalib.php file change (from this fix, I presume) is causing the course search to fail with an error reading from the database. We are using MySQL 5.5.16. The error log contains (when you search for something): [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: Default exception handler: Error reading from database Debug: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '), ' ', c.fullname, ' ', c.idnumber, ' ', c.shortname)) LIKE LOWER('%trest%') CO' at line 4, referer: http://mymoodleserver/moodle/course/index.php [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: SELECT c.* , ctx.id AS ctxid, ctx.path AS ctxpath, ctx.depth AS ctxdepth, ctx.contextlevel AS ctxlevel, ctx.instanceid AS ctxinstance, referer: http://mymoodleserver/moodle/course/index.php [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: FROM mdl_course c, referer: http://mymoodleserver/moodle/course/index.php [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: LEFT JOIN mdl_context ctx ON (ctx.instanceid = c.id AND ctx.contextlevel = 50), referer: http://mymoodleserver/moodle/course/index.php [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: WHERE LOWER(CONCAT(COALESCE(c.summary, ), ' ', c.fullname, ' ', c.idnumber, ' ', c.shortname)) LIKE LOWER COLLATE utf8_bin ESCAPE '\\\\' AND c.id <> 1, referer: http://mymoodleserver/moodle/course/index.php [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: ORDER BY fullname ASC, referer: http://mymoodleserver/moodle/course/index.php [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: [array (, referer: http://mymoodleserver/moodle/course/index.php [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: 0 => '%trest%',, referer: http://mymoodleserver/moodle/course/index.php [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: )], referer: http://mymoodleserver/moodle/course/index.php [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: * line 394 of \\lib\\dml moodle_database.php: dml_read_exception thrown, referer: http://mymoodleserver/moodle/course/index.php [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: * line 768 of \\lib\\dml mysqli_native_moodle_database.php: call to moodle_database->query_end(), referer: http://mymoodleserver/moodle/course/index.php [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: * line 778 of \\lib datalib.php: call to mysqli_native_moodle_database->get_recordset_sql(), referer: http://mymoodleserver/moodle/course/index.php [Wed Oct 19 16:40:21 2011] [warn] [client x.x.x.x] mod_fcgid: stderr: * line 171 of \\course search.php: call to get_courses_search(), referer: http://mymoodleserver/moodle/course/index.php When I restore lib/datalib.php from the previous week's version, the course search works again.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          crap, the sql_empty() use should be enclosed by quotes, how did that escaped to our process, grrr.

          Can you try plz...

          It's simply about to add those 2 missing quotes:

          -    $concat = $DB->sql_concat("COALESCE(c.summary, ". $DB->sql_empty() .")", "' '", 'c.fullname', "' '", 'c.idnumber', "' '", 'c.sh
          +    $concat = $DB->sql_concat("COALESCE(c.summary, '". $DB->sql_empty() ."')", "' '", 'c.fullname', "' '", 'c.idnumber', "' '", 'c.
          

          If so, I'll re-roll the weeklies including this fix.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited crap, the sql_empty() use should be enclosed by quotes, how did that escaped to our process, grrr. Can you try plz... master: https://github.com/stronk7/moodle/compare/master...MDL-29496b 21_STABLE: - master: https://github.com/stronk7/moodle/compare/MOODLE_21_STABLE...MDL-29496b_21 20_STABLE: - master: https://github.com/stronk7/moodle/compare/MOODLE_20_STABLE...MDL-29496b_20 It's simply about to add those 2 missing quotes: - $concat = $DB->sql_concat( "COALESCE(c.summary, " . $DB->sql_empty() . ")" , "' '" , 'c.fullname', "' '" , 'c.idnumber', "' '" , 'c.sh + $concat = $DB->sql_concat( "COALESCE(c.summary, '" . $DB->sql_empty() . "')" , "' '" , 'c.fullname', "' '" , 'c.idnumber', "' '" , 'c. If so, I'll re-roll the weeklies including this fix.
          Hide
          Sam Hemelryk added a comment -

          Thanks Ryan + Eloy,

          Not sure how I missed that during testing! It has me completely baffled.
          I've now confirmed the error and tested your patch Eloy which fixes the problem perfectly thanks.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Ryan + Eloy, Not sure how I missed that during testing! It has me completely baffled. I've now confirmed the error and tested your patch Eloy which fixes the problem perfectly thanks. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sending changes to all git/cvs servers... will be available soon...

          Show
          Eloy Lafuente (stronk7) added a comment - Sending changes to all git/cvs servers... will be available soon...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Done, git and cvs servers have been updated with this emergency fix.

          Thanks Ryan & Sam... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Done, git and cvs servers have been updated with this emergency fix. Thanks Ryan & Sam... ciao
          Hide
          Ryan Smith added a comment -

          I just updated to the latest version. The bug is fixed now. Thanks for fixing it quickly!

          Show
          Ryan Smith added a comment - I just updated to the latest version. The bug is fixed now. Thanks for fixing it quickly!
          Hide
          Iñigo Zendegi added a comment -

          Hello,

          I´ve updated our Moodle to 2.1.2 (Build 20111019) and this change makes Moodle with Oracle database fail because summary column datatype is clob and (at least in Oracle) coalesce parameters must be the same datatype.

          I´ve seen there are (at least) three posible solutions to this issue in Oracle:

          1) No coalesce (as it was in 2.1.2) works fine (concatening NULL with not NULL values doesn´t make NULL the returned value):

          $concat = $DB->sql_concat("c.summary", "' '", 'c.fullname', "' '", 'c.idnumber', "' '", 'c.shortname');

          2) Make all coalesce parameters of the same datatype:

          $concat = $DB->sql_concat("COALESCE(c.summary, to_clob('". $DB->sql_empty() ."'))", "' '", 'c.fullname', "' '", 'c.idnumber', "' '", 'c.shortname');

          3) Use NVL instead of coalesce:

          $concat = $DB->sql_concat("NVL(c.summary, '". $DB->sql_empty() ."')", "' '", 'c.fullname', "' '", 'c.idnumber', "' '", 'c.shortname');

          I see that the first solution is not valid for Postgres, I don´t know if the other two will do it or another solution must be found...

          Show
          Iñigo Zendegi added a comment - Hello, I´ve updated our Moodle to 2.1.2 (Build 20111019) and this change makes Moodle with Oracle database fail because summary column datatype is clob and (at least in Oracle) coalesce parameters must be the same datatype. I´ve seen there are (at least) three posible solutions to this issue in Oracle: 1) No coalesce (as it was in 2.1.2) works fine (concatening NULL with not NULL values doesn´t make NULL the returned value): $concat = $DB->sql_concat("c.summary", "' '", 'c.fullname', "' '", 'c.idnumber', "' '", 'c.shortname'); 2) Make all coalesce parameters of the same datatype: $concat = $DB->sql_concat("COALESCE(c.summary, to_clob('". $DB->sql_empty() ."'))", "' '", 'c.fullname', "' '", 'c.idnumber', "' '", 'c.shortname'); 3) Use NVL instead of coalesce: $concat = $DB->sql_concat("NVL(c.summary, '". $DB->sql_empty() ."')", "' '", 'c.fullname', "' '", 'c.idnumber', "' '", 'c.shortname'); I see that the first solution is not valid for Postgres, I don´t know if the other two will do it or another solution must be found...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is going to become the most revisited issue ever. Neither of 1), 2), 3) above are cross-db, so I'm about to use conditional - $DB->dbfamily() - and create separate SQL for Oracle.

          Alternatively we could use sql_compare_text() to cast the clob to varchar2, but that will leave out some chars if the clob is > 4000 bytes , aka won't search against the complete contents.

          grrr, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This is going to become the most revisited issue ever. Neither of 1), 2), 3) above are cross-db, so I'm about to use conditional - $DB->dbfamily() - and create separate SQL for Oracle. Alternatively we could use sql_compare_text() to cast the clob to varchar2, but that will leave out some chars if the clob is > 4000 bytes , aka won't search against the complete contents. grrr, ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Linking this with MDL-29912 and continuing there...

          Show
          Eloy Lafuente (stronk7) added a comment - Linking this with MDL-29912 and continuing there...

            People

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

              Dates

              • Created:
                Updated:
                Resolved: