aboutsummaryrefslogtreecommitdiffstats
path: root/activerecord
diff options
context:
space:
mode:
Diffstat (limited to 'activerecord')
-rw-r--r--activerecord/CHANGELOG.md14
-rw-r--r--activerecord/lib/active_record/errors.rb5
-rw-r--r--activerecord/lib/active_record/relation/query_methods.rb18
-rw-r--r--activerecord/test/cases/relations_test.rb34
4 files changed, 69 insertions, 2 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index b59c9b4635..50a0b291b3 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,17 @@
+* `ActiveRecord::Relation#reverse_order` throws `ActiveRecord::IrreversibleOrderError`
+ when the order can not be reversed using current trivial algorithm.
+ Also raises the same error when `#reverse_order` is called on
+ relation without any order and table has no primary key:
+
+ Topic.order("concat(author_name, title)").reverse_order
+ # Before: SELECT `topics`.* FROM `topics` ORDER BY concat(author_name DESC, title) DESC
+ # After: raises ActiveRecord::IrreversibleOrderError
+ Edge.all.reverse_order
+ # Before: SELECT `edges`.* FROM `edges` ORDER BY `edges`.`` DESC
+ # After: raises ActiveRecord::IrreversibleOrderError
+
+ *Bogdan Gusiev*
+
* Improve schema_migrations insertion performance by inserting all versions
in one INSERT SQL.
diff --git a/activerecord/lib/active_record/errors.rb b/activerecord/lib/active_record/errors.rb
index e5906b6756..87f32c042c 100644
--- a/activerecord/lib/active_record/errors.rb
+++ b/activerecord/lib/active_record/errors.rb
@@ -275,4 +275,9 @@ module ActiveRecord
# The mysql2 and postgresql adapters support setting the transaction isolation level.
class TransactionIsolationError < ActiveRecordError
end
+
+ # IrreversibleOrderError is raised when a relation's order is too complex for
+ # +reverse_order+ to automatically reverse.
+ class IrreversibleOrderError < ActiveRecordError
+ end
end
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 716b1e8505..8ef9f9f627 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -1104,14 +1104,21 @@ module ActiveRecord
end
def reverse_sql_order(order_query)
- order_query = ["#{quoted_table_name}.#{quoted_primary_key} ASC"] if order_query.empty?
+ if order_query.empty?
+ return [table[primary_key].desc] if primary_key
+ raise IrreversibleOrderError,
+ "Relation has no current order and table has no primary key to be used as default order"
+ end
order_query.flat_map do |o|
case o
when Arel::Nodes::Ordering
o.reverse
when String
- o.to_s.split(',').map! do |s|
+ if does_not_support_reverse?(o)
+ raise IrreversibleOrderError, "Order #{o.inspect} can not be reversed automatically"
+ end
+ o.split(',').map! do |s|
s.strip!
s.gsub!(/\sasc\Z/i, ' DESC') || s.gsub!(/\sdesc\Z/i, ' ASC') || s.concat(' DESC')
end
@@ -1121,6 +1128,13 @@ module ActiveRecord
end
end
+ def does_not_support_reverse?(order)
+ #uses sql function with multiple arguments
+ order =~ /\([^()]*,[^()]*\)/ ||
+ # uses "nulls first" like construction
+ order =~ /nulls (first|last)\Z/i
+ end
+
def build_order(arel)
orders = order_values.uniq
orders.reject!(&:blank?)
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 0638edacbd..090b885dd5 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -19,6 +19,7 @@ require 'models/aircraft'
require "models/possession"
require "models/reader"
require "models/categorization"
+require "models/edge"
class RelationTest < ActiveRecord::TestCase
fixtures :authors, :topics, :entrants, :developers, :companies, :developers_projects, :accounts, :categories, :categorizations, :posts, :comments,
@@ -223,6 +224,39 @@ class RelationTest < ActiveRecord::TestCase
assert_equal topics(:fifth).title, topics.first.title
end
+ def test_reverse_order_with_function
+ topics = Topic.order("length(title)").reverse_order
+ assert_equal topics(:second).title, topics.first.title
+ end
+
+ def test_reverse_order_with_function_other_predicates
+ topics = Topic.order("author_name, length(title), id").reverse_order
+ assert_equal topics(:second).title, topics.first.title
+ topics = Topic.order("length(author_name), id, length(title)").reverse_order
+ assert_equal topics(:fifth).title, topics.first.title
+ end
+
+ def test_reverse_order_with_multiargument_function
+ assert_raises(ActiveRecord::IrreversibleOrderError) do
+ Topic.order("concat(author_name, title)").reverse_order
+ end
+ end
+
+ def test_reverse_order_with_nulls_first_or_last
+ assert_raises(ActiveRecord::IrreversibleOrderError) do
+ Topic.order("title NULLS FIRST").reverse_order
+ end
+ assert_raises(ActiveRecord::IrreversibleOrderError) do
+ Topic.order("title nulls last").reverse_order
+ end
+ end
+
+ def test_default_reverse_order_on_table_without_primary_key
+ assert_raises(ActiveRecord::IrreversibleOrderError) do
+ Edge.all.reverse_order
+ end
+ end
+
def test_order_with_hash_and_symbol_generates_the_same_sql
assert_equal Topic.order(:id).to_sql, Topic.order(:id => :asc).to_sql
end