ORDER BY xx NULLS first/last will break reverse_sql_order #7423

Closed
wants to merge 1 commit into
from

Projects

None yet
Contributor
sishen commented Aug 22, 2012

Oracle and PostgreSQL supports "NULLS first/last" in order by statement. It
will break the reverse_sql_order by concating " DESC" to the order by statement.

For example,

    User.order("name ASC NULLS FIRST").last
Contributor
sishen commented Aug 22, 2012

@rafaelfranca Sorry I did a mistake to push to the same branch and then the prior pull request got auto updated. Please review. Thanks.

Contributor
sishen commented Aug 22, 2012

reference to @al2o3cr's comment on #7386: Should reversing the order reverse the position of the nulls as well, so that reversing 'name ASC NULLS LAST' gets you 'name DESC NULLS FIRST'?

Contributor
yahonda commented Aug 22, 2012

In rails, id is usually a primary key for database. Your test case tests with Oracle database whose primary key constraint consists of not null and unique. Therefore your test case do not always satisfy your code because id should not be null.

Contributor
sishen commented Aug 22, 2012

@yahonda Thanks for the suggestion! I also followed @al2o3cr's suggestion to reverse the position of the nulls.

I think we need a changelog entry for that, and please squash your commits into one. I'm also going to ask for some thoughts from @tenderlove . Thanks!

Contributor
sishen commented Sep 10, 2012

Thanks @carlosantoniodasilva! Please review.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Sep 20, 2012
activerecord/CHANGELOG.md
@@ -1,5 +1,14 @@
## Rails 4.0.0 (unreleased) ##
+* Oracle and PostgreSQL supports "NULLS first/last" in order by statement.
+ `reverse_sql_order` now supports the syntax and will reverse the order too. Fixes #7423.
+
+ Example:
carlosantoniodasilva
carlosantoniodasilva Sep 20, 2012 Owner

You can remove this example.

Contributor
sishen commented Sep 21, 2012

Updated and example removed. Thanks @carlosantoniodasilva.

@frodsan frodsan commented on an outdated diff Oct 26, 2012
activerecord/CHANGELOG.md
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* Oracle and PostgreSQL supports "NULLS first/last" in order by statement.
+ `reverse_sql_order` now supports the syntax and will reverse the order too. Fixes #7423.
frodsan
frodsan Oct 26, 2012 Contributor

✂️ whitespace

@frodsan frodsan commented on an outdated diff Oct 26, 2012
activerecord/test/cases/base_test.rb
@@ -1459,6 +1459,18 @@ def test_last
assert_equal Developer.all.merge!(:order => 'id desc').first, Developer.last
end
+ if current_adapter?(:PostgreSQLAdapter, :OracleAdapter)
+ def test_last_with_null_order_first
+ first = Topic.all.merge!(:order => 'author_email_address ASC').first
frodsan
frodsan Oct 26, 2012 Contributor

Please, use 1.9 hash syntax. Thanks.

Contributor
frodsan commented Oct 26, 2012

@carlosantoniodasilva any news on this? :)

Member
schneems commented Dec 3, 2012

Looks like @sishen has added some commits, can we get some of the original commenters to review the most recent changes?

Contributor
yahonda commented Dec 3, 2012

My review has been implemented to sishen@4e0e6c2. It looks good. except CHANGELOG.md needs rebase, I think.

Contributor
sishen commented Dec 4, 2012

@schneems Yes. I follow @carlosantoniodasilva's suggestion to squash the commits to one so keep merging simple.
@yahonda You mean rebase to current origin/master and update the CHANGELOG.md?

Thanks all.

Member

You mean rebase to current origin/master and update the CHANGELOG.md?

Yes.

Contributor
sishen commented Jan 7, 2013

Any updates? When it's approved and ready to go, I'll rebase to current origin/master and update the CHANGELOG.md. Thanks!

Contributor
sishen commented Apr 20, 2013

I just rebased to current origin/master and made the changes. Let me know what else should be done, thanks.

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Apr 20, 2013
activerecord/test/cases/base_test.rb
@@ -1133,6 +1133,18 @@ def test_last
assert_equal Developer.all.merge!(:order => 'id desc').first, Developer.last
end
+ if current_adapter?(:PostgreSQLAdapter, :OracleAdapter)
+ def test_last_with_null_order_first
+ first = Topic.all.merge!(order: 'author_email_address ASC').first
+ assert_equal first, Topic.order('author_email_address DESC NULLS FIRST').last
+ end
+
+ def test_last_with_null_order_last
+ first = Topic.all.merge!(order: 'author_email_address DESC').first
carlosantoniodasilva
carlosantoniodasilva Apr 20, 2013 Owner

There's an extra space before the two first = assignments.

sishen
sishen Apr 20, 2013 Contributor

@carlosantoniodasilva Is that ok if I amend the fix into this commit instead of a new commit? Then the discussion will be outdated and not visible.

carlosantoniodasilva
carlosantoniodasilva Apr 20, 2013 Owner

Please do, we usually ask people to squash related commits together :)

Contributor
sishen commented Apr 20, 2013

@carlosantoniodasilva Thanks and done.

Great! :)

I think it'd be interesting to add an example on the reverse_order method docs about it, just a simple one similar to the existing there, like this:

# It also reverts +nulls first/last+ when the database supports it:
#
#   .... then you add an example here :)

The same example could be used in the changelog entry then. Wdyt?

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Apr 20, 2013
activerecord/CHANGELOG.md
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* Oracle and PostgreSQL supports "NULLS first/last" in order by statement.
+ `reverse_sql_order` now supports the syntax and will reverse the order too. Fixes #7423.
carlosantoniodasilva
carlosantoniodasilva Apr 20, 2013 Owner

It may be better to not cite this method since it's private. Perhaps something more high level:

`reverse_order` will now revert `nulls first/last` when they're part of the order by statement:

    Example code with output here (like the docs one).

This is currently supported by Oracle and PostgreSQL. Fixes #7423.

Makes sense?

Contributor
sishen commented Apr 20, 2013

@carlosantoniodasilva Agree. That's better. Is the following changes fine to you?

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 475bed3..0cb68d6 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,7 +1,12 @@
 ## Rails 4.0.0 (unreleased) ##

 *   Oracle and PostgreSQL supports "NULLS first/last" in order by statement.
-    `reverse_sql_order` now supports the syntax and will reverse the order too. Fixes #7423.
+    And `reverse_order` will now revert `nulls first/last` when
+    they're part of the order by statement.  For example,
+
+        User.order("name ASC NULLS LAST").last # generated SQL has 'ORDER BY name DESC NULLS FIRST'
+
+    Fixes #7423.

     *Dingding Ye*

diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index cf37df7..62e2cd1 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -782,6 +782,10 @@ module ActiveRecord
     # Reverse the existing order clause on the relation.
     #
     #   User.order('name ASC').reverse_order # generated SQL has 'ORDER BY name DESC'
+    #
+    # It also reverts +nulls first/last+ when the database supports it:
+    #
+    #   User.order("name ASC NULLS LAST").last # generated SQL has 'ORDER BY name DESC NULLS FIRST
     def reverse_order
       spawn.reverse_order!
     end

@sishen yeah it seems ok, but I think the example in reverse_order should probably call this method instead of last. Other than that, seems good :)

Contributor
sishen commented Apr 20, 2013

@carlosantoniodasilva Made the change. Thanks for the advice!

chinshr commented May 13, 2013

@sishen this is still open, is your commit tracked in a branch?

Contributor
sishen commented May 14, 2013

@chinshr Yes. Let me know what I should do to make it merged. Thanks.

chinshr commented May 14, 2013

Sending a pull request might be the right thing?
On May 13, 2013 7:04 PM, "Dingding Ye" notifications@github.com wrote:

@chinshr https://github.com/chinshr Yes. Let me know what I should do
to make it merged. Thanks.


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/7423#issuecomment-17852759
.

Contributor
sishen commented May 14, 2013

@chinshr Rebase to latest again and send a new PR?

chinshr commented May 14, 2013

Yes, try.
On May 13, 2013 7:17 PM, "Dingding Ye" notifications@github.com wrote:

@chinshr https://github.com/chinshr Rebase to latest again and send a
new PR?


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/7423#issuecomment-17853084
.

@sishen sishen ORDER BY xx NULLS first/last break reverse_sql_order
Oracle and PostgreSQL supports "NULLS first/last" in order by statement.
And `reverse_order` will now revert `nulls first/last` when they're part
of the order by statement. For example,

    # "name ASC NULLS LAST" will be reversed to "name DESC NULLS FIRST"
    User.order("name ASC NULLS LAST").reverse_order
192af0f
Contributor
sishen commented May 14, 2013

@chinshr Done. Let me know what else I should do. Thanks!

chinshr commented May 14, 2013

@spastorino Is there any chance this will make it into a release?

Owner

I think @carlosantoniodasilva is on it

Owner

Sorry, won't this break backwards compatibility with people who do reverse but aren't expecting NULLS LAST to be added to the query?

chinshr commented Jul 16, 2013

Not really, but it depends on what the app specific definition of reverse order is.

However, the current implementation breaks my app code by executing wrong SQL.

Sent from my iPhone

On Jul 15, 2013, at 4:39 PM, Aaron Patterson notifications@github.com wrote:

Sorry, won't this break backwards compatibility with people who do reverse but aren't expecting NULLS LAST to be added to the query?


Reply to this email directly or view it on GitHub.

Contributor
sishen commented Jul 16, 2013

@tenderlove The problem is that if user doesn't expecting "NULLS LAST" to be reversed, it always returns the same result if calling .first() or .last(). Is this the expected?

@laurocaetano laurocaetano commented on the diff May 8, 2014
activerecord/CHANGELOG.md
@@ -1,3 +1,13 @@
+* Oracle and PostgreSQL supports "NULLS FIRST/LAST" in order by statement.
+ And `reverse_order` will now revert `NULLS FIRST/LAST` when
+ they're part of the order by statement. For example,
+
+ User.order("name ASC NULLS LAST").reverse_order # generated SQL has 'ORDER BY name DESC NULLS FIRST'
+
+ Fixes #7423.
laurocaetano
laurocaetano May 8, 2014 Contributor

It will fix #11571.

@laurocaetano laurocaetano commented on the diff May 8, 2014
activerecord/lib/active_record/relation/query_methods.rb
@@ -978,7 +982,9 @@ def reverse_sql_order(order_query)
when String
o.to_s.split(',').collect do |s|
s.strip!
- s.gsub!(/\sasc\Z/i, ' DESC') || s.gsub!(/\sdesc\Z/i, ' ASC') || s.concat(' DESC')
+ s.gsub!(/\sasc(?=\s+|\Z)/i, ' DESC') || s.gsub!(/\sdesc(?=\s+|\Z)/i, ' ASC') || s.concat(' DESC')
+ s.gsub!(/\snulls\s+first\Z/i, ' NULLS LAST') || s.gsub!(/\snulls\s+last\Z/i, ' NULLS FIRST')
laurocaetano
laurocaetano May 8, 2014 Contributor

I would just change s.gsub!(/\snulls\s+first\Z/i, ' NULLS LAST') || s.gsub!(/\snulls\s+last\Z/i, ' NULLS FIRST')
to s.gsub!(/\snulls\s+first\s*\Z/i, ' NULLS LAST') || s.gsub!(/\snulls\s+last\s*\Z/i, ' NULLS FIRST') so it can handle spaces after the last or first.

@laurocaetano laurocaetano commented on the diff May 8, 2014
activerecord/test/cases/base_test.rb
@@ -1133,6 +1133,18 @@ def test_last
assert_equal Developer.all.merge!(:order => 'id desc').first, Developer.last
end
+ if current_adapter?(:PostgreSQLAdapter, :OracleAdapter)
+ def test_last_with_null_order_first
+ first = Topic.all.merge!(order: 'author_email_address ASC').first
+ assert_equal first, Topic.order('author_email_address DESC NULLS FIRST').last
+ end
+
+ def test_last_with_null_order_last
+ first = Topic.all.merge!(order: 'author_email_address DESC').first
+ assert_equal first, Topic.order('author_email_address ASC NULLS LAST').last
+ end
+ end
+
laurocaetano
laurocaetano May 8, 2014 Contributor

Looks good. I'd just add some tests to ensure reverse_order will work as you described in the PR.

This was referenced Jun 30, 2014
@sgrif sgrif was assigned by rails-bot Oct 20, 2015
Member
sgrif commented Oct 28, 2015

This feature was rejected in #18754

@sgrif sgrif closed this Oct 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment