Remove where clause grouping from RelationExtensions#collapse_wheres

[farrspace]

When a column is referenced in more than one where clause, the group_by and subsequent re-ordering of columns can cause the wrong bound values to be associated with columns. The columns are reordered, but the bound values are attached in their original order.

It looks like the current collapse_wheres in Squeel is based on the one from Active Record 3

I am unsure why the columns were grouped originally, but this now results in incorrect behavior under certain circumstances. You can see that in Active Record 4's version of this method, where clauses are no longer grouped.

My change removes the grouping and reordering.

With the original collapse_wheres, you can see the spec I have added failing:

it 'does not reorder binaries' do
  relation = Cat.where(hair_type: "short").where(name: "Crookshanks").where(hair_type: "long")
  relation.to_sql.should eq "SELECT #{Q}cats#{Q}.* FROM #{Q}cats#{Q} WHERE #{Q}cats#{Q}.#{Q}hair_type#{Q} = 'short' AND #{Q}cats#{Q}.#{Q}name#{Q} = 'Crookshanks' AND #{Q}cats#{Q}.#{Q}hair_type#{Q} = 'long'"
end

Failures:

  1) Squeel::Adapters::ActiveRecord::RelationExtensions#collapse_wheres does not reorder binaries
     Failure/Error: relation.to_sql.should eq "SELECT #{Q}cats#{Q}.* FROM #{Q}cats#{Q} WHERE #{Q}cats#{Q}.#{Q}hair_type#{Q} = 'short' AND #{Q}cats#{Q}.#{Q}name#{Q} = 'Crookshanks' AND #{Q}cats#{Q}.#{Q}hair_type#{Q} = 'long'"

       expected "SELECT \"cats\".* FROM \"cats\" WHERE \"cats\".\"hair_type\" = 'short' AND \"cats\".\"name\" = 'Crookshanks' AND \"cats\".\"hair_type\" = 'long'"
            got "SELECT \"cats\".* FROM \"cats\" WHERE \"cats\".\"hair_type\" = 'short' AND \"cats\".\"hair_type\" = 'Crookshanks' AND \"cats\".\"name\" = 'long'"

       (compared using ==)
     # /Users/farr/.rbenv/versions/2.3.0/lib/ruby/gems/2.3.0/gems/rspec-expectations-2.6.0/lib/rspec/expectations/fail_with.rb:29:in `fail_with'
     # /Users/farr/.rbenv/versions/2.3.0/lib/ruby/gems/2.3.0/gems/rspec-expectations-2.6.0/lib/rspec/expectations/handler.rb:19:in `handle_matcher'
     # /Users/farr/.rbenv/versions/2.3.0/lib/ruby/gems/2.3.0/gems/rspec-expectations-2.6.0/lib/rspec/expectations/extensions/kernel.rb:27:in `should'
     # ./spec/squeel/adapters/active_record/relation_extensions_spec.rb:1007:in `block (3 levels) in <module:ActiveRecord>'

Finished in 1.43 seconds
870 examples, 1 failure, 7 pending

Failed examples:

rspec ./spec/squeel/adapters/active_record/relation_extensions_spec.rb:1005 # Squeel::Adapters::ActiveRecord::RelationExtensions#collapse_wheres does not reorder binaries
~/projects/squeel <master> &

Note that as the query was built, it is trying to use "long" as the cat's name instead of "Crookshanks".

[farrspace]

This might address issues #386 and #400

[joebui]

Hi, do you have any update on this pull request? I tested your change and it works on rails 4.2.x

[farrspace]

@joebui I don't have any specific updates. We used this change for a while but eventually removed Squeel from the project.