DBMSSQL.php generates invalid SQL when applying a limit to a query sorted on a joined column

[chuck-jove]

I've provided a full writeup, with a proposed patch and test, here: https://groups.google.com/forum/?fromgroups=#!topic/propel-users/eUNGx5Hfjj4.

Briefly: The problem seems to be that aliases are not allowed in that ORDER BY clause: only actual column names are allowed. Here's a test which currently fails:

    function test_select_join_order_alias() {
        $adapter = new DBMSSQL();
        $sql = self::clean('
                SELECT Field, Related.Field AS [RelatedField]
                FROM Record
                LEFT JOIN Related
                    ON Record.RelatedID = Related.ID
                ORDER BY [RelatedField] ASC
        ');
        $adapter->applyLimit($sql,10,5);
        $this->assertEqual(
            self::clean($sql),
            self::clean('
                SELECT [Field], [RelatedField]
                FROM
                    (SELECT
                        ROW_NUMBER() OVER(ORDER BY Related.Field ASC) AS [RowNumber],
                        Field AS [Field],
                        Related.Field AS [RelatedField]
                    FROM Record
                    LEFT JOIN Related
                        ON Record.RelatedID = Related.ID)
                    AS derivedb
                WHERE RowNumber
                    BETWEEN 11 AND 15
            ')
        );
    }

    static function clean($string) {
        return preg_replace(
            array('/^\s+/','/\s+$/','/\s+/'),
            array('',      '',      ' '),
            $string
        );
    }

and here's a patch:

+++ PropelORM/runtime/lib/adapter/DBMSSQL.php        (working copy)
@@ -186,6 +186,7 @@

             $orders = explode(',', $order);

             for ($i = 0; $i < count($orders); $i ++) {
+                               $orders[$i] = trim($orders[$i]);
                 $orderArr[trim(preg_replace('/\s+(ASC|DESC)$/i', '', $orders[$i]))] = array(
                     'sort' => (stripos($orders[$i], ' DESC') !== false) ? 'DESC' : 'ASC',
                     'key' => $i
@@ -202,6 +203,27 @@

             //make sure the current column isn't * or an aggregate
             if ($selColArr[0] != '*' && ! strstr($selColArr[0], '(')) {
+
+                               /* START FIX */
+
+                               // Aliases can be used in ORDER BY clauses on a SELECT,
+                               // but aliases are not valid in the ORDER BY clause of ROW_NUMBER() OVER (...),
+                               // so if we notice that part of $order is actually an alias,
+                               // we replace it with the original Table.Column designation.
+
+                               if ($selColCount) {
+                                       // column with alias
+                                       foreach(array(' ASC',' DESC') as $sort) {
+                                               $index = array_search($selColArr[2].$sort,$orders);
+                                               if ($index !== FALSE) {
+                                                       // replace alias with "Table.Column ASC/DESC"
+                                                       $orders[$index] = $selColArr[0].$sort;
+                                               }
+                                       }
+                               }
+
+                               /* END FIX */
+
                 if (isset($orderArr[$selColArr[0]])) {
                     $orders[$orderArr[$selColArr[0]]['key']] = $selColArr[0] . ' ' . $orderArr[$selColArr[0]]['sort'];
                 }

I don't really understand all the surrounding code, so I'm not really confident that this wouldn't break something else.

[willdurand]

@staabm could you write a PR with both the provided patch and test?

[staabm]

Hmm @willdurand ? you are talking with @chuck-jove aren`t you?

[willdurand]

nope ^^

William Durand | http://www.williamdurand.fr

On Thu, Feb 14, 2013 at 11:50 PM, Markus Staab notifications@github.comwrote:

Hmm? you are talking with @chuck-jove https://github.com/chuck-jovearent you?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/596#issuecomment-13583399.

[staabm]

Ok, than I will have a look into it :-)