Moodle
  1. Moodle
  2. MDL-39572

sql_order_by_text() definition wrong for MSSQL/sqlsrv

    Details

    • Rank:
      50257

      Description

      The arguments used in the MSSQL version of sql_order_by_text() appear to be incorrect. In current master:

          public function sql_order_by_text($fieldname, $numchars=32) {
              return ' CONVERT(varchar, ' . $fieldname . ', ' . $numchars . ')';
          }
      

      But according to these docs: http://msdn.microsoft.com/en-us/library/ms187928.aspx

      The CONVERT function accepts length as part of the 1st argument and the 3rd argument is used for date and time styles.

      The result that this function is always converting text fields using the MSSQL default length of 30 characters.

      Steps to reproduce

      1. Install a new site on MSSQL.
      2. Add the test.php file (attached to this bug) to the root code folder.
      3. Visit [site-url]/test.php in a web browser.

      Expected behaviour

      The script would output:

      Without using sql_order_by_text():         123456789a123456789b123456789c123456789d
      With sql_order_by_text() default length:   123456789a123456789b123456789c12
      With sql_order_by_text() 35 char length:   123456789a123456789b123456789c12345
      

      Actual behaviour
      The script outputs:

      Without using sql_order_by_text():         123456789a123456789b123456789c123456789d
      With sql_order_by_text() default length:   123456789a123456789b123456789c
      With sql_order_by_text() 35 char length:   123456789a123456789b123456789c
      

      e.g. the default length is incorrect (30 instead of 32 characters) and changing the length has no effect.

      1. test.php
        0.5 kB
        Simon Coggins

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that, Simon.

          Feel free to push that to peer review.

          Show
          Michael de Raadt added a comment - Thanks for reporting that, Simon. Feel free to push that to peer review.
          Hide
          Dan Poltawski added a comment -

          Hi Simon,

          Thanks a lot - the fix looks good.

          But I think that we need to improve the unit test in lib/dml/tests/dml_test.php::test_sql_order_by_text() to test this functionality and ensure we have it working on all DB engines.

          So, could you improve that unit test to cover this case?

          thanks,
          Dan

          Show
          Dan Poltawski added a comment - Hi Simon, Thanks a lot - the fix looks good. But I think that we need to improve the unit test in lib/dml/tests/dml_test.php::test_sql_order_by_text() to test this functionality and ensure we have it working on all DB engines. So, could you improve that unit test to cover this case? thanks, Dan
          Hide
          Simon Coggins added a comment -

          Sure can, good idea!

          Simon

          Show
          Simon Coggins added a comment - Sure can, good idea! Simon
          Hide
          Simon Coggins added a comment -

          I have pushed a new patch to the master branch with a proposed test case. I need to get MSSQL tests working in my environment to test it.

          A couple of notes:

          1. I am testing sql_compare_text() instead of sql_order_by_text() but really they are the same thing (one function is just calling the other).
          2. The tests assert different statements on MSSQL/Oracle which is a bit strange but I think it's hard to avoid because the function only truncates TEXT fields on MSSQL/Oracle.

          Simon

          Show
          Simon Coggins added a comment - I have pushed a new patch to the master branch with a proposed test case. I need to get MSSQL tests working in my environment to test it. A couple of notes: I am testing sql_compare_text() instead of sql_order_by_text() but really they are the same thing (one function is just calling the other). The tests assert different statements on MSSQL/Oracle which is a bit strange but I think it's hard to avoid because the function only truncates TEXT fields on MSSQL/Oracle. Simon
          Hide
          Dan Poltawski added a comment -

          Hi Simon,

          I ran the test on mssql (and removed your fix part of the commit) but it doesn't fail. It would be good if the test could demonstrate the problem and that its fixed.

          thanks

          Show
          Dan Poltawski added a comment - Hi Simon, I ran the test on mssql (and removed your fix part of the commit) but it doesn't fail. It would be good if the test could demonstrate the problem and that its fixed. thanks
          Hide
          Simon Coggins added a comment -

          This is an interesting one - it appears the whole sql_compare_text() test method in dml_test.php wasn't being run by PHPUnit because the method name isn't prefixed with 'test' as described in point 3 here:

          http://phpunit.de/manual/3.7/en/writing-tests-for-phpunit.html

          So neither my new tests or any of the existing tests in that function were being executed.

          Further to that, the 2nd of the existing tests would fail on PostgreSQL/MySQL if it actually ran because it is expecting the description to be truncated, but that is only done for MSSQL/Oracle databases.

          I did some quick code grepping and couldn't find any other test files with this problem except itest_wrong_creation() in backup/util/structure/tests/baseoptiogroup_test.php.

          Maybe we need a test to ensure the tests are named correctly

          Would you be able to try again with the latest master patch (in which I've added the method prefix and a condition to fix the broken existing test).

          Simon

          Show
          Simon Coggins added a comment - This is an interesting one - it appears the whole sql_compare_text() test method in dml_test.php wasn't being run by PHPUnit because the method name isn't prefixed with 'test' as described in point 3 here: http://phpunit.de/manual/3.7/en/writing-tests-for-phpunit.html So neither my new tests or any of the existing tests in that function were being executed. Further to that, the 2nd of the existing tests would fail on PostgreSQL/MySQL if it actually ran because it is expecting the description to be truncated, but that is only done for MSSQL/Oracle databases. I did some quick code grepping and couldn't find any other test files with this problem except itest_wrong_creation() in backup/util/structure/tests/baseoptiogroup_test.php. Maybe we need a test to ensure the tests are named correctly Would you be able to try again with the latest master patch (in which I've added the method prefix and a condition to fix the broken existing test). Simon
          Hide
          Dan Poltawski added a comment -

          Ah, great spotting!

          We do actually have some tests like that on our continuous integration server for misnamed classes and missing from the xml file: https://github.com/moodlehq/moodle-local_ci/blob/master/run_phpunittests/run_phpunittests.sh#L102

          So if we can make that grep of yours reproduceable then we can add that. (My first throught was that it might be tricky because often people create utility methods)

          Show
          Dan Poltawski added a comment - Ah, great spotting! We do actually have some tests like that on our continuous integration server for misnamed classes and missing from the xml file: https://github.com/moodlehq/moodle-local_ci/blob/master/run_phpunittests/run_phpunittests.sh#L102 So if we can make that grep of yours reproduceable then we can add that. (My first throught was that it might be tricky because often people create utility methods)
          Hide
          Simon Coggins added a comment -

          Yes, like you said it's hard because there will be false positives which intentionally don't contain tests.

          Potentially you could look for methods in test files which don't start with test* but do contain calls to assertions like assertEquals().

          Show
          Simon Coggins added a comment - Yes, like you said it's hard because there will be false positives which intentionally don't contain tests. Potentially you could look for methods in test files which don't start with test* but do contain calls to assertions like assertEquals().
          Hide
          Dan Poltawski added a comment -

          Hi Simon,

          I get a fail without your patch now - but not with the new assertions? (and sorry, I know this is probably getting more indepth into the test than you wanted ) I'd like to see the tests demonstrating the problem before hand and then shwoing it fixed.

          Some more comments

          1. I prefer the test to be in a seperate commit (so that people who are curious like me can revert the fix to proove the failure, with the unit test in failure)
          2. I know it was previously assertEquals(), but we could convert the assertEquals(count() to assertCount (and also fix the ordering of the arguments which is currently wrong).
          3. Seems like you may as well encapsulate those two new query/assertion pairs inside the if, rather than running them needlessly on enginges where it doesn't make a difference
          Show
          Dan Poltawski added a comment - Hi Simon, I get a fail without your patch now - but not with the new assertions? (and sorry, I know this is probably getting more indepth into the test than you wanted ) I'd like to see the tests demonstrating the problem before hand and then shwoing it fixed. Some more comments I prefer the test to be in a seperate commit (so that people who are curious like me can revert the fix to proove the failure, with the unit test in failure) I know it was previously assertEquals(), but we could convert the assertEquals(count() to assertCount (and also fix the ordering of the arguments which is currently wrong). Seems like you may as well encapsulate those two new query/assertion pairs inside the if, rather than running them needlessly on enginges where it doesn't make a difference
          Hide
          Dan Poltawski added a comment -

          I've created MDLSITE-2398 about detecting misnamed methods.

          Show
          Dan Poltawski added a comment - I've created MDLSITE-2398 about detecting misnamed methods.
          Hide
          Simon Coggins added a comment -

          It's okay I like testing

          I have made the changes you suggested to the master branch. I've also got our MSSQL tests running and checked it with and without the fix and I think everything is right now.

          Let me know if you are happy and I'll update the other branches.

          Show
          Simon Coggins added a comment - It's okay I like testing I have made the changes you suggested to the master branch. I've also got our MSSQL tests running and checked it with and without the fix and I think everything is right now. Let me know if you are happy and I'll update the other branches.
          Hide
          Dan Poltawski added a comment -

          Thanks Simon, that looks good to me now. I'm sending this to integration and i'll also run the tests on oracle

          Show
          Dan Poltawski added a comment - Thanks Simon, that looks good to me now. I'm sending this to integration and i'll also run the tests on oracle
          Hide
          Simon Coggins added a comment -

          Great thanks. I've updated the other branches too.

          Simon

          Show
          Simon Coggins added a comment - Great thanks. I've updated the other branches too. Simon
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Dan Poltawski 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
          Dan Poltawski 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
          Simon Coggins added a comment -

          Patches rebased.

          Show
          Simon Coggins added a comment - Patches rebased.
          Hide
          Damyon Wiese added a comment -

          Thanks Simon,

          This change looks good - tested in postgres and mssql and no unit test failures found.

          Integrated to 24, 25 and master (23 does not receive non-security updates).

          Show
          Damyon Wiese added a comment - Thanks Simon, This change looks good - tested in postgres and mssql and no unit test failures found. Integrated to 24, 25 and master (23 does not receive non-security updates).
          Hide
          Michael de Raadt added a comment -

          I'm going through various DB-version combinations.

          I found a problem in master on all DBs. This is due to MDL-39876.

          In 2.4 I found a problem in MSSQL and SQLSVR, but I don't think it is related.

          1) core_phpunit_generator_testcase::test_create
          Failed asserting that 0 is identical to '0'.
          
          /home/michael/web/htdocs/24_integration/lib/phpunit/tests/generator_test.php:59
          /home/michael/web/htdocs/24_integration/lib/phpunit/classes/advanced_testcase.php:76
          
          To re-run:
           vendor/bin/phpunit core_phpunit_generator_testcase lib/phpunit/tests/generator_test.php
          

          In 2.5 on MSSQL and SQLSVR I found an issue with a query in forums. It's clearly not related to this issue, but I'm not sure why it didn't come up in master. I suspect this is the result of MDL-41191.

          1) mod_forum_lib_testcase::test_forum_get_courses_user_posted_in
          dml_read_exception: Error reading from database (The ntext data type cannot be selected as DISTINCT because it is not comparable.
          SELECT DISTINCT c.* , ctx.id AS ctxid, ctx.path AS ctxpath, ctx.depth AS ctxdepth, ctx.contextlevel AS ctxlevel, ctx.instanceid AS ctxinstance
                      FROM phpu_course c
                      JOIN phpu_forum_discussions fd ON fd.course = c.id
                              JOIN phpu_forum_posts fp ON fp.discussion = fd.id
                      LEFT JOIN phpu_context ctx ON (ctx.instanceid = c.id AND ctx.contextlevel = 50)
                      WHERE fp.userid = ?
          [array (
            0 => '5',
          )])
          
          /home/michael/web/htdocs/25_integration/lib/dml/moodle_database.php:423
          /home/michael/web/htdocs/25_integration/lib/dml/mssql_native_moodle_database.php:256
          /home/michael/web/htdocs/25_integration/lib/dml/mssql_native_moodle_database.php:722
          /home/michael/web/htdocs/25_integration/lib/dml/mssql_native_moodle_database.php:756
          /home/michael/web/htdocs/25_integration/mod/forum/lib.php:8177
          /home/michael/web/htdocs/25_integration/mod/forum/tests/lib_test.php:86
          /home/michael/web/htdocs/25_integration/lib/phpunit/classes/advanced_testcase.php:76
          
          To re-run:
           vendor/bin/phpunit mod_forum_lib_testcase mod/forum/tests/lib_test.php
          
          Show
          Michael de Raadt added a comment - I'm going through various DB-version combinations. I found a problem in master on all DBs. This is due to MDL-39876 . In 2.4 I found a problem in MSSQL and SQLSVR, but I don't think it is related. 1) core_phpunit_generator_testcase::test_create Failed asserting that 0 is identical to '0'. /home/michael/web/htdocs/24_integration/lib/phpunit/tests/generator_test.php:59 /home/michael/web/htdocs/24_integration/lib/phpunit/classes/advanced_testcase.php:76 To re-run: vendor/bin/phpunit core_phpunit_generator_testcase lib/phpunit/tests/generator_test.php In 2.5 on MSSQL and SQLSVR I found an issue with a query in forums. It's clearly not related to this issue, but I'm not sure why it didn't come up in master. I suspect this is the result of MDL-41191 . 1) mod_forum_lib_testcase::test_forum_get_courses_user_posted_in dml_read_exception: Error reading from database (The ntext data type cannot be selected as DISTINCT because it is not comparable. SELECT DISTINCT c.* , ctx.id AS ctxid, ctx.path AS ctxpath, ctx.depth AS ctxdepth, ctx.contextlevel AS ctxlevel, ctx.instanceid AS ctxinstance FROM phpu_course c JOIN phpu_forum_discussions fd ON fd.course = c.id JOIN phpu_forum_posts fp ON fp.discussion = fd.id LEFT JOIN phpu_context ctx ON (ctx.instanceid = c.id AND ctx.contextlevel = 50) WHERE fp.userid = ? [array ( 0 => '5', )]) /home/michael/web/htdocs/25_integration/lib/dml/moodle_database.php:423 /home/michael/web/htdocs/25_integration/lib/dml/mssql_native_moodle_database.php:256 /home/michael/web/htdocs/25_integration/lib/dml/mssql_native_moodle_database.php:722 /home/michael/web/htdocs/25_integration/lib/dml/mssql_native_moodle_database.php:756 /home/michael/web/htdocs/25_integration/mod/forum/lib.php:8177 /home/michael/web/htdocs/25_integration/mod/forum/tests/lib_test.php:86 /home/michael/web/htdocs/25_integration/lib/phpunit/classes/advanced_testcase.php:76 To re-run: vendor/bin/phpunit mod_forum_lib_testcase mod/forum/tests/lib_test.php
          Hide
          Michael de Raadt added a comment -

          Test result: Passed.

          Unit tests are in a bit of a state this week, but as far as I can tell, none of the problems appear to be caused by this change.

          Tested in MySQL, MSSQL, SQLSVR, PostgreSQL and Oracle with Moodle 2.4, 2.5 and master.

          Show
          Michael de Raadt added a comment - Test result: Passed. Unit tests are in a bit of a state this week, but as far as I can tell, none of the problems appear to be caused by this change. Tested in MySQL, MSSQL, SQLSVR, PostgreSQL and Oracle with Moodle 2.4, 2.5 and master.
          Hide
          Damyon Wiese added a comment -

          Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week.

          Hurray!

          Show
          Damyon Wiese added a comment - Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week. Hurray!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: