Skip to content

Commit

Permalink
Make filter_binds filter out symbols that are equal to strings
Browse files Browse the repository at this point in the history
ActiveRecord::Relation::Merger's filter_binds method does not filter out bind
variables when one of the attribute nodes has a string name, but the other has
a symbol name, even when those names are actually equal.

This can result in there being more bind variables than placeholders in the
generated SQL.  This is particularly an issue for PostgreSQL, where this is
treated as an error.

This patch changes the filter_binds method to make it convert both attribute
names to strings before comparing.
  • Loading branch information
Nat Budin committed May 14, 2014
1 parent eacb426 commit 1d316ac
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 1 deletion.
9 changes: 9 additions & 0 deletions activerecord/CHANGELOG.md
@@ -1,3 +1,12 @@
* `ActiveRecord::Relation::Merger#filter_binds` now compares equivalent symbols and
strings in column names as equal.

This fixes a rare case in which more bind values are passed than there are
placeholders for them in the generated SQL statement, which can make PostgreSQL
throw a `StatementInvalid` exception.

*Nat Budin*

* `change_column_default` allows `[]` as argument to `change_column_default`.

Fixes #11586.
Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/relation/merger.rb
Expand Up @@ -156,7 +156,7 @@ def merge_single_values
def filter_binds(lhs_binds, removed_wheres)
return lhs_binds if removed_wheres.empty?

set = Set.new removed_wheres.map { |x| x.left.name }
set = Set.new removed_wheres.map { |x| x.left.name.to_s }
lhs_binds.dup.delete_if { |col,_| set.include? col.name }
end

Expand Down
5 changes: 5 additions & 0 deletions activerecord/test/cases/relation/merging_test.rb
Expand Up @@ -108,6 +108,11 @@ def test_merging_reorders_bind_params
merged = left.merge(right)
assert_equal post, merged.first
end

def test_merging_compares_symbols_and_strings_as_equal
post = PostThatLoadsCommentsInAnAfterSaveHook.create!(title: "First Post", body: "Blah blah blah.")
assert_equal "First comment!", post.comments.where(body: "First comment!").first_or_create.body
end
end

class MergingDifferentRelationsTest < ActiveRecord::TestCase
Expand Down
8 changes: 8 additions & 0 deletions activerecord/test/models/comment.rb
Expand Up @@ -40,3 +40,11 @@ class SubSpecialComment < SpecialComment

class VerySpecialComment < Comment
end

class CommentThatAutomaticallyAltersPostBody < Comment
belongs_to :post, class_name: "PostThatLoadsCommentsInAnAfterSaveHook", foreign_key: :post_id

after_save do |comment|
comment.post.update_attributes(body: "Automatically altered")
end
end
9 changes: 9 additions & 0 deletions activerecord/test/models/post.rb
Expand Up @@ -208,3 +208,12 @@ class SpecialPostWithDefaultScope < ActiveRecord::Base
self.table_name = 'posts'
default_scope { where(:id => [1, 5,6]) }
end

class PostThatLoadsCommentsInAnAfterSaveHook < ActiveRecord::Base
self.table_name = 'posts'
has_many :comments, class_name: "CommentThatAutomaticallyAltersPostBody", foreign_key: :post_id

after_save do |post|
post.comments.load
end
end

0 comments on commit 1d316ac

Please sign in to comment.