Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Merge pull request #9216 from robertomiranda/where-with-empty-array

Activer Record:  Change behaviour with empty array in where clause
  • Loading branch information...
commit 85e1c4a058b32e3d1358ca5e24b469bdd658fc9c 2 parents bcf0e08 + 16f6f25
Guillermo Iguaran guilleiguaran authored
5 activerecord/CHANGELOG.md
View
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* Change behaviour with empty array in where clause,
+ the SQL generated when when were passed an empty array was insecure in some cases
+
+ Roberto Miranda
+
* Raise ArgumentError instead of generate invalid SQL when empty hash is used in where clause value
Roberto Miranda
2  activerecord/lib/active_record/associations/has_many_through_association.rb
View
@@ -136,7 +136,7 @@ def delete_records(records, method)
records = load_target if records == :all
scope = through_association.scope
- scope.where! construct_join_attributes(*records)
+ scope.where! construct_join_attributes(*records) unless records.empty?
Dalibor Nasevic
dalibor added a note

i find if records.present? more readable than unless records.empty?. What do you think?

This has been reverted now, so no problem. Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
case method
when :destroy
2  activerecord/lib/active_record/relation/predicate_builder.rb
View
@@ -17,6 +17,8 @@ def self.build_from_hash(klass, attributes, default_table)
queries.concat expand(association && association.klass, table, k, v)
end
end
+ elsif value.is_a?(Array) && value.empty?
+ raise ArgumentError, "Condition value in SQL clause can't be an empty array"
else
column = column.to_s
9 activerecord/test/cases/finder_test.rb
View
@@ -808,15 +808,6 @@ def test_find_with_nil_inside_set_passed_for_one_attribute
assert_equal [2, 1].sort, client_of.compact.sort
end
- def test_find_with_nil_inside_set_passed_for_attribute
- client_of = Company.all.merge!(
- :where => { :client_of => [nil] },
- :order => 'client_of DESC'
- ).map { |x| x.client_of }
-
- assert_equal [], client_of.compact
- end
-
def test_with_limiting_with_custom_select
posts = Post.references(:authors).merge(
:includes => :author, :select => ' posts.*, authors.id as "author_id"',
4 activerecord/test/cases/relation/where_test.rb
View
@@ -98,7 +98,9 @@ def test_where_with_table_name_and_empty_hash
end
def test_where_with_table_name_and_empty_array
- assert_equal 0, Post.where(:id => []).count
+ assert_raises(ArgumentError) do
+ Post.where(:id => [])
+ end
end
def test_where_with_empty_hash_and_no_foreign_key
5 activerecord/test/cases/relations_test.rb
View
@@ -515,8 +515,9 @@ def test_find_ids
end
def test_find_in_empty_array
- authors = Author.all.where(:id => [])
- assert authors.to_a.blank?
+ assert_raises(ArgumentError) do
+ Author.all.where(:id => [])
+ end
end
def test_where_with_ar_object
Dalibor Nasevic

i find if records.present? more readable than unless records.empty?. What do you think?

Carlos Antonio da Silva

This has been reverted now, so no problem. Thanks :)

Please sign in to comment.
Something went wrong with that request. Please try again.