Skip to content

Commit

Permalink
Revert change on ActiveRecord::Relation#order method that prepends new
Browse files Browse the repository at this point in the history
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.

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/relation/query_methods.rb
	guides/source/upgrading_ruby_on_rails.md
  • Loading branch information
rafaelfranca committed Jul 30, 2013
1 parent d1ecd40 commit 609dae1
Show file tree
Hide file tree
Showing 8 changed files with 35 additions and 20 deletions.
15 changes: 15 additions & 0 deletions activerecord/CHANGELOG.md
@@ -1,5 +1,20 @@
## unreleased ##

* 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.

* When using optimistic locking, `update` was not passing the column to `quote_value`
to allow the connection adapter to properly determine how to quote the value. This was
affecting certain databases that use specific colmn types.
Expand Down
4 changes: 2 additions & 2 deletions activerecord/lib/active_record/relation/query_methods.rb
Expand Up @@ -289,7 +289,7 @@ def order!(*args) # :nodoc:
arg.is_a?(Symbol) ? "#{quoted_table_name}.#{arg} ASC" : arg
}

self.order_values = args + self.order_values
self.order_values += args
self
end

Expand All @@ -301,7 +301,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)
Expand Down
Expand Up @@ -570,9 +570,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
Expand Down
Expand Up @@ -308,9 +308,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
Expand Down
14 changes: 7 additions & 7 deletions activerecord/test/cases/relations_test.rb
Expand Up @@ -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
Expand Down Expand Up @@ -1174,20 +1174,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
Expand Down
8 changes: 4 additions & 4 deletions activerecord/test/cases/scoping/default_scoping_test.rb
Expand Up @@ -79,7 +79,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
Expand All @@ -91,7 +91,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
Expand Down Expand Up @@ -251,8 +251,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

Expand Down
4 changes: 2 additions & 2 deletions guides/source/active_record_querying.md
Expand Up @@ -543,11 +543,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
Expand Down
2 changes: 1 addition & 1 deletion guides/source/upgrading_ruby_on_rails.md
Expand Up @@ -53,7 +53,7 @@ Rails 4.0 no longer supports loading plugins from `vendor/plugins`. You must rep

* The `delete` method in collection associations can now receive `Fixnum` or `String` arguments as record ids, besides records, pretty much like the `destroy` method does. Previously it raised `ActiveRecord::AssociationTypeMismatch` for such arguments. From Rails 4.0 on `delete` automatically tries to find the records matching the given ids before deleting them.

* 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.
* 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 `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`.

Expand Down

9 comments on commit 609dae1

@mensfeld
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca: can you tell more about the "we will implement a new API to prepend the order clauses in
Rails 4.1"? I have a Rails 4.0 app that uses the new ordering and now it will be hard to migrate from 4.0.0 to 4.0.1. You made it easier to go from 3 to 4 but from 4 to 4.0.1 now sucks...

Maybe it would be better to allow us to change how it behaves on an app level (config)?

config.active_record.order = :prepend
config.active_record.order = :append

Monkey patch for that (put it into your initializers):

module ActiveRecord
  class Relation

    def order!(*args)
      args.flatten!
      validate_order_args args

      references = args.reject { |arg| Arel::Node === arg }
      references.map! { |arg| arg =~ /^([a-zA-Z]\w*)\.(\w+)/ && $1 }.compact!
      references!(references) if references.any?

      # if a symbol is given we prepend the quoted table name
      args = args.map { |arg|
        arg.is_a?(Symbol) ? "#{quoted_table_name}.#{arg} ASC" : arg
      }
      self.order_values = args + self.order_values
      self
    end

  end
end

@rafaelfranca
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already know we would made harder to upgrade from 4.0.0 to 4.0.1, but the number of affected applications would be less than people migrating from 3. The new API will be only added on 4.1 and I don't think is worth to add configuration to this since reorder can give you the old behavior.

@mensfeld
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca : thx, can you say, what are you planning to do in 4.1?

@rafaelfranca
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We didn't discussed this yet, but maybe will be something like prepend_order. Maybe instead of monkey patching order you can add this new method. Fell free to ping me if you are having problem with this change and could not make it work with reorder or with mokey path/new method. I'd like to help.

@mensfeld
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca: monkey patch works exactly as expected. Probably will stay with it until the 4.1. Really liked the new idea, since I could easily create default orders that would be then occasionally overwritten with a custom order. Ofc prepend_order would work the same but it would require a some changes in the app code and it's bit more risky. BTW either way I think that this change should be somehow highlighted - not all people read commits and/or changelogs, especially for minor "theoretically" bug fixing releases.

@rafaelfranca
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback. We highlighted the change on the blog post and release emails http://weblog.rubyonrails.org/2013/10/17/Rails-4-0-1-rc1-has-been-released/

@mensfeld
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca: I've got a feeling that after 4.0.1 release - you guys will have a real shitstorm about that :D - I will open a 🍺 and enjoy this one 💃

alt text

@rafaelfranca
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hahahaha. Yes, that is expected. But it is better to regret soon than buy a mistake for long time.

@mensfeld
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca : I will definitely enjoy this one :D can't wait till Tuesday. Thanks for help ;) keep up the good job but don't do things like that 2 often :D I don't like backporting stuff to Rails 3 or doing this type of monkey patches ;)

Please sign in to comment.