limit_offset_sql is returning incorrect SQL.

[hairypalm]

Using fetch limits in mssql-django results in incorrect SQL.

In operations.py, DatabaseOperations is the method:

    def limit_offset_sql(self, low_mark, high_mark):
        """Return LIMIT/OFFSET SQL clause."""
        limit, offset = self._get_limit_offset_params(low_mark, high_mark)
        return '%s%s' % (
            (' OFFSET %d ROWS' % offset) if offset else '',
            (' FETCH FIRST %d ROWS ONLY' % limit) if limit else '',
        )

This will generate code like the following when there is no offset but a limit of 3:
SELECT * FROM [table] ORDER BY [id] FETCH FIRST 3 ROWS ONLY

Such SQL fails to run, with error of:
[S0002][153] Line 1: Invalid usage of the option FIRST in the FETCH statement.

The correct SQL should be:
SELECT * FROM [table] ORDER BY [id] OFFSET 0 ROWS FETCH FIRST 3 ROWS ONLY

I'm not an expert in T-SQL so I don't know the appropriate logic, but it seems that OFFSET offset ROWS should be included when a FETCH FIRST limit ROWS ONLY is in place regardless of whether the offset is 0 or not provided.

[mShan0]

Thanks for the report. Will take a look into this. What version of SQL Server are you using?

[hairypalm]

@mShan0 It is the latest docker container. Looks like server:2022

https://hub.docker.com/_/microsoft-mssql-server

[maikhanhbui]

Hi @hairypalm, could you post your models.py for the affected tables and some code samples that reproduce this issue?

[hairypalm]

Hi @khanhmaibui. The package I'm using is https://github.com/matthiask/django-tree-queries and I was trying to add support for MSSQL. The issue comes up just with a "get" (https://github.com/django/django/blob/c914d6cff176ae6bfab2f33a84bcfd45208f1894/django/db/models/query.py#L623) I think because django get function seems to sometimes use a limit of MAX_GET_RESULTS in its query:

def get(self, *args, **kwargs):
        """
        Perform the query and return a single object matching the given
        keyword arguments.
        """
        if self.query.combinator and (args or kwargs):
            raise NotSupportedError(
                "Calling QuerySet.get(...) with filters after %s() is not "
                "supported." % self.query.combinator
            )
        clone = self._chain() if self.query.combinator else self.filter(*args, **kwargs)
        if self.query.can_filter() and not self.query.distinct_fields:
            clone = clone.order_by()
        limit = None
        if (
            not clone.query.select_for_update
            or connections[clone.db].features.supports_select_for_update_with_limit
        ):
            limit = MAX_GET_RESULTS
            clone.query.set_limits(high=limit)
        num = len(clone)
        if num == 1:
            return clone._result_cache[0]
        if not num:
            raise self.model.DoesNotExist(
                "%s matching query does not exist." % self.model._meta.object_name
            )
        raise self.model.MultipleObjectsReturned(
            "get() returned more than one %s -- it returned %s!"
            % (
                self.model._meta.object_name,
                num if not limit or num < limit else "more than %s" % (limit - 1),
            )
        )

Not sure if MS SQL has changed, but I think one definitely needs OFFSET 0 to FETCH FIRST and that function limit_offset_sql will not generate the required OFFSET 0. I'm surprised that mssql-django is passing tests because I can't pass the tests in django-tree-queries without editing limit_offset_sql to force the OFFSET 0.

[maikhanhbui]

@hairypalm I have opened a PR to fix this issue. Can you try that to see if that works?

[hairypalm]

@khanhmaibui This fix is the same as mine, so yes it works in my code. Thank you!!