ZF-4996: Zend_Db_Adapter_Pdo_Mssql->limit() Does Not Account For Multiple ORDER BY Columns

Description

Zend_Db_Adapter_Pdo_Mssql->limit() does not account for multiple ORDER BY columns.

The way the adapter reverses the sub-select to get the limit, by reversing the sort order, it reverses only the last column in the ORDER BY list. It should be reversing all the columns. By "reverse" I mean, ASC becomes DESC, and DESC becomes ASC.

Then, in the outermost SELECT, it does not revert back to the original ORDER BY list. Again, only the last column in the list is reverted.

Below is an example (formatted for better reading):


SELECT * 
FROM
(
    SELECT TOP 10 * 
    FROM 
    (
        SELECT TOP 30 * 
        FROM "people"
        ORDER BY "lname" ASC, "fname" ASC
    )
    ORDER BY "lname" DESC, "fname" DESC
)
ORDER BY "lname" ASC, "fname" ASC

SELECT * 
FROM
(
    SELECT TOP 10 * 
    FROM 
    (
        SELECT TOP 30 * 
        FROM "people"
        ORDER BY "lname" ASC, "fname" ASC
    )
    ORDER BY "lname" , "fname" DESC
)
ORDER BY "lname" , "fname" asc

Comments

The method limit() has not changed (functionally) since at least 1.0.0. While this bug doesn't affect common direct usage. In my scenario, this renders the desperately needed Zend_Paginator component effectively useless in my application.

Let's take for example a common application, an "eCommerce" site. This site would show various items "for sale" and limit the amount of items shown on a single page. Well, let's say we want to re-sort those items in a different order; let's say price. Price is definitely not a unique key, and duplicate values are certain to come up. This is where the second "ORDER BY" is important. It ensures that regardless of how many duplicate primary ordered columns, there is a secondary order enforcing boundaries in limits. When this secondary order isn't enforced, it allows records to be returned in duplicate at various offsets.

This bug not only doesn't support multiple ORDER BY columns, but renders them completely backwards by flipping only the least significant column (the last one).

I could come up with all sorts of other scenarios where multiple ORDER BY columns, in league with limit() functionality, would be a paramount requirement for operation.

Ok, I have a fix, but it comes with a catch. This code will generate the expected SQL above, but it does not support omitted ASC/DESC. It must be defined otherwise it is ignored.

There might be a better "placeholder" instead of zASC or zDESC, but it works regardless. It reverses both ORDER BY columns; not just the least significant one. I'm sure with a bit more crafty regex, omitted ASC/DESC can be supported. However, internally, the framework tends to explicitly provide it regardless. So it works well for my application.


    /**
     * Adds an adapter-specific LIMIT clause to the SELECT statement.
     *
     * @link http://lists.bestpractical.com/pipermail/rt-devel/…
     *
     * @param string $sql
     * @param integer $count
     * @param integer $offset OPTIONAL
     * @throws Zend_Db_Adapter_Exception
     * @return string
     */
     public function limit($sql, $count, $offset = 0)
     {
        $count = intval($count);
        if ($count <= 0) {
            /** @see Zend_Db_Adapter_Exception */
            require_once 'Zend/Db/Adapter/Exception.php';
            throw new Zend_Db_Adapter_Exception("LIMIT argument count=$count is not valid");
        }

        $offset = intval($offset);
        if ($offset < 0) {
            /** @see Zend_Db_Adapter_Exception */
            require_once 'Zend/Db/Adapter/Exception.php';
            throw new Zend_Db_Adapter_Exception("LIMIT argument offset=$offset is not valid");
        }

        $orderby = stristr($sql, 'ORDER BY');
        if ($orderby !== false) {
            $order = str_ireplace('ORDER BY', '', $orderby);
            // A single pass will negate itself, so placeholders must be put in first
            $orderFlipped = preg_replace(array("/\bASC\b/i", "/\bDESC\b/i"), array('zDESC', 'zASC'), $order);
            $orderFlipped = preg_replace(array("/\bzASC\b/", "/\bzDESC\b/"), array('ASC', 'DESC'), $orderFlipped);
        }

        $sql = preg_replace('/^SELECT\s/i', 'SELECT TOP ' . ($count+$offset) . ' ', $sql);

        $sql = 'SELECT * FROM (SELECT TOP ' . $count . ' * FROM (' . $sql . ') AS inner_tbl';
        if ($orderby !== false) {
            $sql .= ' ORDER BY ' . $orderFlipped;
        }
        $sql .= ') AS outer_tbl';
        if ($orderby !== false) {
            $sql .= ' ORDER BY ' . $order;
        }

        return $sql;
    }

It duplicates ZF-4099 .

A fix has been provided in r17706, please test.

I'm noticing just a $ in the matching regex on lines 331 and 336. We should allow for some whitespace to the right of the order direction since that is legal SQL. It is acceptable to leave the whitespace out in the replacement though.

Suggested for line 331: $inv = preg_replace('/\s+desc\s*$/i', ' ASC', $orderPart, 1, $pregReplaceCount);

You are right, but since it is the replacement, i think removing whitespace is the better thing to do.

Fixed in r17728