Moodle
  1. Moodle
  2. MDL-39572

sql_order_by_text() definition wrong for MSSQL/sqlsrv

    Details

      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.

        Gliffy Diagrams

        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: