diff options
author | Sean Griffin <sean@thoughtbot.com> | 2015-01-24 19:51:26 -0700 |
---|---|---|
committer | Sean Griffin <sean@thoughtbot.com> | 2015-01-24 19:51:26 -0700 |
commit | c80487eb1ae7f35cc780745b4f951a79e1a3482b (patch) | |
tree | 17be8fca763795483be08cb2aa15344859462838 /activerecord | |
parent | 4c0a9922d6bc608e330740b4384c418fe69fc587 (diff) | |
download | rails-c80487eb1ae7f35cc780745b4f951a79e1a3482b.tar.gz rails-c80487eb1ae7f35cc780745b4f951a79e1a3482b.tar.bz2 rails-c80487eb1ae7f35cc780745b4f951a79e1a3482b.zip |
Don't rely on relation mutability when building through associations
Specifically, the issue is relying on `where_unscoping` mutating the
where values. It does not, however, mutate the bind values, which could
cause an error under certain circumstances. This was not exposed by the
tests, since the only place which would have been affected is unscoping
a boolean, which doesn't go through prepared statements. I had a hard
time getting better test coverage to demonstrate the issue.
This in turn, caused `merge` to go through proper logic, and try to
clear out the binds associated with the unscoped relation, which then
exposed a source of `nil` for the columns, as binds weren't expanding
`{ "posts.id" => 1 }` to `{ "posts" => { "id" => 1 } }`. This has been
fixed.
The bulk of `create_binds` needed to be moved to a separate method,
since the dot notation should not be expanded recursively.
I'm pretty sure this removes a subtle quirk that a ton of code in
`Relation::Merger` is working around, and I suspect that code can be
greatly simplified. However, unraveling that rats nest is no small task.
Diffstat (limited to 'activerecord')
-rw-r--r-- | activerecord/lib/active_record/associations/through_association.rb | 2 | ||||
-rw-r--r-- | activerecord/lib/active_record/relation/predicate_builder.rb | 42 |
2 files changed, 25 insertions, 19 deletions
diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index 09828dbd9b..3ce9cffdbc 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/activerecord/lib/active_record/associations/through_association.rb @@ -18,7 +18,7 @@ module ActiveRecord reflection_scope = reflection.scope if reflection_scope && reflection_scope.arity.zero? - relation.merge!(reflection_scope) + relation = relation.merge(reflection_scope) end scope.merge!( diff --git a/activerecord/lib/active_record/relation/predicate_builder.rb b/activerecord/lib/active_record/relation/predicate_builder.rb index 2860a30f99..4c49d3eb81 100644 --- a/activerecord/lib/active_record/relation/predicate_builder.rb +++ b/activerecord/lib/active_record/relation/predicate_builder.rb @@ -29,24 +29,8 @@ module ActiveRecord end def create_binds(attributes) - result = attributes.dup - binds = [] - - attributes.each do |column_name, value| - case value - when String, Integer, ActiveRecord::StatementCache::Substitute - result[column_name] = Arel::Nodes::BindParam.new - binds.push([table.column(column_name), value]) - when Hash - attrs, bvs = associated_predicate_builder(column_name).create_binds(value) - result[column_name] = attrs - binds += bvs - when Relation - binds += value.arel.bind_values + value.bind_values - end - end - - [result, binds] + attributes = convert_dot_notation_to_hash(attributes.stringify_keys) + create_binds_for_hash(attributes) end def expand(column, value) @@ -108,6 +92,28 @@ module ActiveRecord end end + + def create_binds_for_hash(attributes) + result = attributes.dup + binds = [] + + attributes.each do |column_name, value| + case value + when String, Integer, ActiveRecord::StatementCache::Substitute + result[column_name] = Arel::Nodes::BindParam.new + binds.push([table.column(column_name), value]) + when Hash + attrs, bvs = associated_predicate_builder(column_name).create_binds_for_hash(value) + result[column_name] = attrs + binds += bvs + when Relation + binds += value.arel.bind_values + value.bind_values + end + end + + [result, binds] + end + private def associated_predicate_builder(association_name) |