DDC-2687: Paginator with ORDER BY not working in MSSQL #3426

Closed
doctrinebot opened this Issue Sep 18, 2013 · 5 comments

2 participants

@doctrinebot

Jira issue originally created by user flip101:

This bug report is similar to this one:
http://www.doctrine-project.org/jira/browse/[DDC-2622](http://www.doctrine-project.org/jira/browse/DDC-2622)

It's decided to make a new bug report to keep things cleaner (commits have already been done on the other report).

PHP code to test (A symfony 2.3 controller):

<?php
public function testAction() {
    $em = $this->getDoctrine()->getManager();
    $query = $em->createQuery("
        SELECT report, user
        FROM Report:Report report

        JOIN report.user user

        WHERE user.id = ?1
        ORDER BY report.created DESC
    ");
    $query->setMaxResults(10);
    $query->setParameter(1, 1);
    $paginator = new Paginator($query, $fetchJoinCollection = true);
    $result = $paginator->getIterator(); // Trigger querying, using paginator
    //$result = $query->getResult();

    return [
        'var' => $result
    ];
}
// returning the result to a template debug function

Schema:
One User to Many Reports

SQL ERROR:

An exception occurred while executing '
SELECT *
FROM (
    SELECT DISTINCT id0
        ,ROW_NUMBER() OVER (
            ORDER BY r0_.aangemaakt DESC
            ) AS doctrine_rownum
    FROM (
        SELECT r0_.id AS id0
            ,r0_.Naam AS Naam1
            ,r0_.Omschrijving AS Omschrijving2
            ,r0_.aangemaakt AS aangemaakt3
            ,r0_.gewijzigd AS gewijzigd4
            ,r0_.verwijderd AS verwijderd5
            ,g1_.username AS username6
            ,g1*.username_canonical AS username*canonical7
            ,g1_.email AS email8
            ,g1*.email_canonical AS email*canonical9
            ,g1_.enabled AS enabled10
            ,g1_.salt AS salt11
            ,g1_.password AS password12
            ,g1*.last_login AS last*login13
            ,g1_.locked AS locked14
            ,g1_.expired AS expired15
            ,g1*.expires_at AS expires*at16
            ,g1*.confirmation_token AS confirmation*token17
            ,g1*.password_requested_at AS password_requested*at18
            ,g1_.roles AS roles19
            ,g1*.credentials_expired AS credentials*expired20
            ,g1*.credentials_expire_at AS credentials_expire*at21
            ,g1_.id AS id22
        FROM Rapporten r0_ WITH (NOLOCK)
        INNER JOIN Gebruiker g1* ON r0_.GebruikerId = g1*.id
        WHERE (g1_.id = ?)
            AND (r0_.verwijderd IS NULL)
        ) dctrn_result
    ) AS doctrine_tbl
WHERE doctrine_rownum BETWEEN 1
        AND 10
' with params [1]:

SQLSTATE[42000]: [Microsoft][SQL Server Native Client 11.0][SQL Server]The multi-part identifier "r0_.aangemaakt" could not be bound. 

FIX:
Change

sqlORDER BY r0_.aangemaakt DESC
to
sqlORDER BY aangemaakt3 DESC

This fix should only be applied in case of the Paginator scenario, which looks like this:

    $paginator = new Paginator($query, $fetchJoinCollection = true);
    $result = $paginator->getIterator(); // Trigger querying, using paginator
    //$result = $query->getResult();

When getting a normal result, like this:

    //$paginator = new Paginator($query, $fetchJoinCollection = true);
    //$result = $paginator->getIterator(); // Trigger querying, using paginator
    $result = $query->getResult();

it works fine without any problems.

The "normal" result should keep working correctly!

The SQL produced by the non-paginator result:

FROM (
    SELECT r0_.id AS id0
        ,r0_.Naam AS Naam1
        ,r0_.Omschrijving AS Omschrijving2
        ,r0_.aangemaakt AS aangemaakt3
        ,r0_.gewijzigd AS gewijzigd4
        ,r0_.verwijderd AS verwijderd5
        ,g1_.username AS username6
        ,g1*.username_canonical AS username*canonical7
        ,g1_.email AS email8
        ,g1*.email_canonical AS email*canonical9
        ,g1_.enabled AS enabled10
        ,g1_.salt AS salt11
        ,g1_.password AS password12
        ,g1*.last_login AS last*login13
        ,g1_.locked AS locked14
        ,g1_.expired AS expired15
        ,g1*.expires_at AS expires*at16
        ,g1*.confirmation_token AS confirmation*token17
        ,g1*.password_requested_at AS password_requested*at18
        ,g1_.roles AS roles19
        ,g1*.credentials_expired AS credentials*expired20
        ,g1*.credentials_expire_at AS credentials_expire*at21
        ,g1_.id AS id22
        ,r0_.GebruikerId AS GebruikerId23
        ,g1*.group_id AS group*id24
        ,ROW_NUMBER() OVER (
            ORDER BY r0_.aangemaakt DESC
            ) AS doctrine_rownum
    FROM Rapporten r0_ WITH (NOLOCK)
    INNER JOIN Gebruiker g1* ON r0_.GebruikerId = g1*.id
    WHERE (g1_.id = ?)
        AND (r0_.verwijderd IS NULL)
    ) AS doctrine_tbl
WHERE doctrine_rownum BETWEEN 1
        AND 10

Notice sqlORDER BY r0_.aangemaakt DESC this is correct!

The difference is that the Paginator wraps the query in in a query with distinct, this is done here:
https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Tools/Pagination/LimitSubqueryOutputWalker.php#L160

So far i know two different solutions.

1.
Detect the wrapping query in SQLServerPlatform::doModifyLimitQuery with regex. See relevant code snippet below

if (preg*match('/SELECT DISTINCT .* FROM \(.*\) dctrn*result/', $query)) {
        $isWrapped = true;
} else {
        $isWrapped = false;
}

//Find alias for each colum used in ORDER BY
if ( ! empty($orderbyColumns)) {
        foreach ($orderbyColumns as $column) {

                if ($isWrapped) {
                        $pattern    = sprintf('/%s\.%s\s</ins>(AS\s<ins>)?([<sup>,\s\)]</ins>)/i', $column['table'], $column['column']);
                        $overColumn = preg_match($pattern, $query, $matches) ? $matches[2] : $column['column'];
                } else {
                        $pattern    = sprintf('/%s\.(%s)\s**(AS)?\s*([</sup>,\s\)]**)/i', $column['table'], $column['column']);
                        $overColumn = preg_match($pattern, $query, $matches)
                                ? ($column['hasTable'] ? $column['table']  . '.' : '') . $column['column'] 
                                : $column['column'];
                }

                if (isset($column['sort'])) {
                        $overColumn .= ' ' . $column['sort'];
                }

                $overColumns[] = $overColumn;
        }
}

This code would replace:
https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php#L856-L870
And takes two lines of codes from this commit:
https://github.com/doctrine/dbal/blob/5d7bcb6637646eb1daf6d90827233f5562cbda29/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php#L860-L861

Another solution is that the Paginator should be responsible for notifying the SQLServerPlatform::doModifyLimitQuery method if the query is wrapped or not. In that case, this line of code
https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Tools/Pagination/LimitSubqueryOutputWalker.php#L171
should be replaced by

$sql, $this->maxResults, $this->firstResult, true

And this line of code:
https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php#L818
should be replaced by

protected function doModifyLimitQuery($query, $limit, $offset = null, $isWrapped = false)

Of course parts of the code from the previous solution are still needed. But the $isWrapped detection is not done anymore by regex this way.

@doctrinebot

Comment created by flip101:

I ran the full doctrine orm testsuite on a SQL Server 2008 R2 database. A before and after the first suggested bug fix has been applied. Tests results showed no difference at all. This means:
1. The change does not break any other stuff
2. There is no test yet to cover this case

@doctrinebot

Comment created by flip101:

solved bug, can be closed. See: doctrine/dbal#383

@doctrinebot

Comment created by @ocramius:

Merged: doctrine/dbal@b4bcc18

@doctrinebot

Issue was closed with resolution "Fixed"

@doctrinebot

Comment created by @doctrinebot:

A related Github Pull-Request [GH-789] was closed:
#789

@beberlei beberlei was assigned by doctrinebot Dec 6, 2015
@doctrinebot doctrinebot added this to the 2.4 milestone Dec 6, 2015
@doctrinebot doctrinebot closed this Dec 6, 2015
@doctrinebot doctrinebot added the Bug label Dec 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment