diff options
Diffstat (limited to 'activerecord')
| -rw-r--r-- | activerecord/CHANGELOG.md | 14 | ||||
| -rw-r--r-- | activerecord/lib/active_record/errors.rb | 5 | ||||
| -rw-r--r-- | activerecord/lib/active_record/relation/query_methods.rb | 18 | ||||
| -rw-r--r-- | activerecord/test/cases/relations_test.rb | 34 |
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 |