Add and "order by 1" in sqlsrv limit for ver>11.0 #3128

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

No description provided.

Contributor

narfbg commented Jul 1, 2014

Why?

executing the query (generated from ion_auth)
SELECT "username", "username", "email", "id", "password", "active", "last_login"
FROM "AU_Users"
WHERE "username" = 'admin'
OFFSET 0 ROWS FETCH NEXT 1 ROWS ONLY

sql returns
Msg 102, Level 15, State 1, Line 4
Incorrect syntax near 'OFFSET'.
Msg 153, Level 15, State 2, Line 4
Invalid usage of the option NEXT in the FETCH statement.

adding an "order by" will fix the issue

the "if" is to check if there is already an orderby

Contributor

narfbg commented Jul 1, 2014

You should submit a PR to Ion_Auth so that it doesn't use limit().

true but if anyone try $this->db->limit(1) without order by will fail (is this accepted?)

Contributor

narfbg commented Jul 1, 2014

It's not a question of how you say it, I just don't want to have fixes for somebody else's "broken" code.

Contributor

narfbg commented Oct 28, 2014

@benedmunds Can you tell if limit() usage is really necessary in Ion_Auth? I'll merge this for obvious reasons, but still ...

Contributor

benedmunds commented Oct 28, 2014

Fill me in here. Is the issue using limit() without an order_by()? Or using limit() at all?

Sorry, I havent used SQL Server in years.

Contributor

benedmunds commented Oct 28, 2014

@ChristosGeorgiou at replying you in so you see this question

Contributor

narfbg commented Oct 28, 2014

@benedmunds Quoting the SQL Server 2012 manual:

OFFSET-FETCH can be used only with the ORDER BY clause.

So, yes ... SQL Server is a piece of crap. But my question is rather why do you need to have limit(1) calls in quite a lot of Ion_Auth's queries. Aren't lookups done via the primary keys?

Contributor

pfote commented Oct 28, 2014

@benedmunds instead of using limit you should make username unique

@benedmunds I thing limit(1) shouldn't be used at all in ion_auth. I would use the row() function and primary keys as narfbg notice.

@narfbg Regardless the decision of benedmunds for ion_auth part, IMHO drivers sould be unified and be used across different engines. For me at least that's the point of using active record

Contributor

benedmunds commented Oct 30, 2014

@narfbg @ChristosGeorgiou

I would prefer maintaining the limit() since a row() call isnt enforced. A single user could be queried and then something other than a row() called on it.

I'm fine with adding an order_by() as well though so I'll do that.

Contributor

benedmunds commented Oct 30, 2014

@ChristosGeorgiou this has been pushed to the 2 branch of Ion Auth. If you have any issues please open an issue over on the Ion Auth repo. Thanks!

Contributor

narfbg commented Oct 30, 2014

@benedmunds Yeah, but I wasn't thinking about row() ...

What I meant was, if you have one of username, user_id, group_id, etc. fields, obviously that field should be a primary key or at least a unique one, so doing lookups by it should always result in a single row anyway.
I'm not really familiar with Ion Auth, so I don't know if it works that way, but I assume that's how you do it. No?

Contributor

benedmunds commented Oct 30, 2014

@narfbg not always, a lot of the queries are by identity (email/username/etc) but that is user defined so I can't guarantee it'll even be a foreign key.

Contributor

narfbg commented Oct 31, 2014

@benedmunds Well, of course it's user defined, but if there can be matching "identity" fields in the same table, surely limit() doesn't solve the problem of the right row being fetched ... IMO, at least some assumptions about the DB architecture's sanity should be made. :)

Anyway, you're already aware of that issue, which was what I wanted to happen before merging this PR. Your code, so it's up to you how to deal with it.

@ChristosGeorgiou I'll leave some comments on your patch, it needs to match our styleguide.

@narfbg narfbg commented on the diff Oct 31, 2014

system/database/drivers/sqlsrv/sqlsrv_driver.php
@@ -483,6 +483,10 @@ protected function _limit($sql)
// As of SQL Server 2012 (11.0.*) OFFSET is supported
if (version_compare($this->version(), '11', '>='))
{
+ if (empty($this->qb_orderby))
+ {
+ $sql.=' ORDER BY 1 ';
@narfbg

narfbg Oct 31, 2014

Contributor

Spaces around the assignment operator.

@narfbg narfbg commented on the diff Oct 31, 2014

system/database/drivers/sqlsrv/sqlsrv_driver.php
@@ -483,6 +483,10 @@ protected function _limit($sql)
// As of SQL Server 2012 (11.0.*) OFFSET is supported
if (version_compare($this->version(), '11', '>='))
{
+ if (empty($this->qb_orderby))
+ {
+ $sql.=' ORDER BY 1 ';
+ }
@narfbg

narfbg Oct 31, 2014

Contributor

Put a new line after this one.

Contributor

narfbg commented Oct 31, 2014

Also, the same patch must be applied to system/database/drivers/pdo/subdrivers/pdo_sqlsrv_driver.php.

narfbg closed this in ed3fc51 Nov 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment