Permalink
Browse files

Revert change on ActiveRecord::Relation#order method that prepends new

order on the old ones

The previous behavior added a major backward incompatibility since it
impossible to have a upgrade path without major changes on the
application code.

We are taking the most conservative path to be consistent with the idea
of having a smoother upgrade on Rails 4.

We are reverting the behavior for what was in Rails 3.x and,
if needed, we will implement a new API to prepend the order clauses in
Rails 4.1.
  • Loading branch information...
1 parent fbaae89 commit 92c5a2244ec3e58fd5e70e7dd2d7882b80183c80 @rafaelfranca rafaelfranca committed Jul 30, 2013
View
@@ -1,3 +1,18 @@
+* Revert `ActiveRecord::Relation#order` change that make new order
+ prepend the old one.
+
+ Before:
+
+ User.order("name asc").order("created_at desc")
+ # SELECT * FROM users ORDER BY created_at desc, name asc
+
+ After:
+
+ User.order("name asc").order("created_at desc")
+ # SELECT * FROM users ORDER BY name asc, created_at desc
+
+ This also affects order defined in `default_scope` or any kind of associations.
+
* Don't allow `quote_value` to be called without a column.
Some adapters require column information to do their job properly.
@@ -299,7 +299,7 @@ def order!(*args) # :nodoc:
arg
}
- self.order_values = args.concat self.order_values
+ self.order_values += args
self
end
@@ -311,7 +311,7 @@ def order!(*args) # :nodoc:
#
# User.order('email DESC').reorder('id ASC').order('name ASC')
#
- # generates a query with 'ORDER BY name ASC, id ASC'.
+ # generates a query with 'ORDER BY id ASC, name ASC'.
def reorder(*args)
check_if_method_has_arguments!("reorder", args)
spawn.reorder!(*args)
@@ -506,9 +506,9 @@ def test_dynamic_find_should_respect_association_order
assert_equal high_id_jamis, projects(:active_record).developers.find_by_name('Jamis')
end
- def test_find_should_prepend_to_association_order
+ def test_find_should_append_to_association_order
ordered_developers = projects(:active_record).developers.order('projects.id')
- assert_equal ['projects.id', 'developers.name desc, developers.id desc'], ordered_developers.order_values
+ assert_equal ['developers.name desc, developers.id desc', 'projects.id'], ordered_developers.order_values
end
def test_dynamic_find_all_should_respect_readonly_access
@@ -253,9 +253,9 @@ def test_find_many_with_merged_options
assert_equal 2, companies(:first_firm).limited_clients.limit(nil).to_a.size
end
- def test_find_should_prepend_to_association_order
+ def test_find_should_append_to_association_order
ordered_clients = companies(:first_firm).clients_sorted_desc.order('companies.id')
- assert_equal ['companies.id', 'id DESC'], ordered_clients.order_values
+ assert_equal ['id DESC', 'companies.id'], ordered_clients.order_values
end
def test_dynamic_find_should_respect_association_order
@@ -180,7 +180,7 @@ def test_finding_last_with_arel_order
end
def test_finding_with_order_concatenated
- topics = Topic.order('title').order('author_name')
+ topics = Topic.order('author_name').order('title')
assert_equal 4, topics.to_a.size
assert_equal topics(:fourth).title, topics.first.title
end
@@ -1183,20 +1183,20 @@ def test_order_by_relation_attribute
end
def test_default_scope_order_with_scope_order
- assert_equal 'honda', CoolCar.order_using_new_style.limit(1).first.name
- assert_equal 'honda', FastCar.order_using_new_style.limit(1).first.name
+ assert_equal 'zyke', CoolCar.order_using_new_style.limit(1).first.name
+ assert_equal 'zyke', FastCar.order_using_new_style.limit(1).first.name
end
def test_order_using_scoping
car1 = CoolCar.order('id DESC').scoping do
- CoolCar.all.merge!(:order => 'id asc').first
+ CoolCar.all.merge!(order: 'id asc').first
end
- assert_equal 'honda', car1.name
+ assert_equal 'zyke', car1.name
car2 = FastCar.order('id DESC').scoping do
- FastCar.all.merge!(:order => 'id asc').first
+ FastCar.all.merge!(order: 'id asc').first
end
- assert_equal 'honda', car2.name
+ assert_equal 'zyke', car2.name
end
def test_unscoped_block_style
@@ -82,7 +82,7 @@ def test_default_scope_with_multiple_calls
end
def test_scope_overwrites_default
- expected = Developer.all.merge!(:order => ' name DESC, salary DESC').to_a.collect { |dev| dev.name }
+ expected = Developer.all.merge!(order: 'salary DESC, name DESC').to_a.collect { |dev| dev.name }
received = DeveloperOrderedBySalary.by_name.to_a.collect { |dev| dev.name }
assert_equal expected, received
end
@@ -94,7 +94,7 @@ def test_reorder_overrides_default_scope_order
end
def test_order_after_reorder_combines_orders
- expected = Developer.order('id DESC, name DESC').collect { |dev| [dev.name, dev.id] }
+ expected = Developer.order('name DESC, id DESC').collect { |dev| [dev.name, dev.id] }
received = Developer.order('name ASC').reorder('name DESC').order('id DESC').collect { |dev| [dev.name, dev.id] }
assert_equal expected, received
end
@@ -253,8 +253,8 @@ def test_unscope_errors_with_non_symbol_or_hash_arguments
end
def test_order_in_default_scope_should_not_prevail
- expected = Developer.all.merge!(:order => 'salary').to_a.collect { |dev| dev.salary }
- received = DeveloperOrderedBySalary.all.merge!(:order => 'salary').to_a.collect { |dev| dev.salary }
+ expected = Developer.all.merge!(order: 'salary desc').to_a.collect { |dev| dev.salary }
+ received = DeveloperOrderedBySalary.all.merge!(order: 'salary').to_a.collect { |dev| dev.salary }
assert_equal expected, received
end
@@ -549,11 +549,11 @@ Client.order("orders_count ASC, created_at DESC")
Client.order("orders_count ASC", "created_at DESC")
```
-If you want to call `order` multiple times e.g. in different context, new order will prepend previous one
+If you want to call `order` multiple times e.g. in different context, new order will append previous one
```ruby
Client.order("orders_count ASC").order("created_at DESC")
-# SELECT * FROM clients ORDER BY created_at DESC, orders_count ASC
+# SELECT * FROM clients ORDER BY orders_count ASC, created_at DESC
```
Selecting Specific Fields
@@ -153,8 +153,6 @@ Rails 4.0 no longer supports loading plugins from `vendor/plugins`. You must rep
* In Rails 4.0 when a column or a table is renamed the related indexes are also renamed. If you have migrations which rename the indexes, they are no longer needed.
-* Rails 4.0 has changed how orders get stacked in `ActiveRecord::Relation`. In previous versions of Rails, the new order was applied after the previously defined order. But this is no longer true. Check [Active Record Query guide](active_record_querying.html#ordering) for more information.
-
* Rails 4.0 has changed `serialized_attributes` and `attr_readonly` to class methods only. You shouldn't use instance methods since it's now deprecated. You should change them to use class methods, e.g. `self.serialized_attributes` to `self.class.serialized_attributes`.
* Rails 4.0 has removed `attr_accessible` and `attr_protected` feature in favor of Strong Parameters. You can use the [Protected Attributes gem](https://github.com/rails/protected_attributes) for a smooth upgrade path.

4 comments on commit 92c5a22

Contributor

schuetzm replied Jul 30, 2013

Is this really a good idea? This already made it into a release, so now users upgrading to Rails 4.1 will have the problem you want those who upgrade from Rails 3.2 not to have :-( For example, see issue #11117, whose reporter said they changed their code to match the new behaviour. They would have to revert again on next update. Were there any other complaints except that report?
Besides, shouldn't the original behaviour be considered a bug? I think there was even a bug report about it, but I can't find it right now. It would have to be reopened then...

Owner

rafaelfranca replied Jul 31, 2013

Yes, it is the best we can do. The old behavior is not a bug since users can use reorder to get what they want.

We have another report on discourse http://meta.discourse.org/t/when-should-we-upgrade-to-rails-4/4046/38

Keeping this behavior without giving any way to be backward compatible will make very harder to upgrade to Rails 4. We know it may be a problem to those who already change their code but we prefer to keep the upgrade path smother.

To be clear this as be released on 4.0.1 too.

Contributor

schuetzm replied Jul 31, 2013

Would you accept a PR that makes the behaviour configurable (i.e. with an application wide setting, not as a parameter to order())? The problem is that while appending may be ok if you call it twice in a row, in combination with scopes (and especially default_scope) it is completely useless. tenderlove suggested to clobber any previous ordering, but then you won't be able order by primary and secondary field. Prepending at least enables that.

Owner

rafaelfranca replied Jul 31, 2013

No thank you. We already discussed this a lot and we will only revert the old behavior.

In the future we may add an API to prepending but now you have to use reorder.

Please sign in to comment.