Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fix the generated SQL when In predicate is supplied an empty Array

  • Loading branch information...
commit 11dc44ac339199680704b52402fd53f2e3c0ee07 1 parent e2dad56
@lifo lifo authored
View
6 lib/arel/engines/sql/core_extensions/array.rb
@@ -2,7 +2,11 @@ module Arel
module Sql
module ArrayExtensions
def to_sql(formatter = nil)
- "(" + collect { |e| e.to_sql(formatter) }.join(', ') + ")"
+ if any?
+ "(" + collect { |e| e.to_sql(formatter) }.join(', ') + ")"
+ else
+ "(NULL)"
+ end
end
def inclusion_predicate_sql
View
19 spec/arel/engines/sql/unit/predicates/in_spec.rb
@@ -45,6 +45,25 @@ module Predicates
end
end
end
+
+ describe 'when the array is empty' do
+ before do
+ @array = []
+ end
+
+ it 'manufactures sql with a comma separated list' do
+ sql = In.new(@attribute, @array).to_sql
+
+ adapter_is :mysql do
+ sql.should be_like(%Q{`users`.`id` IN (NULL)})
+ end
+
+ adapter_is_not :mysql do
+ sql.should be_like(%Q{"users"."id" IN (NULL)})
+ end
+ end
+ end
+
end
describe 'when relating to a range' do

7 comments on commit 11dc44a

@dkubb

Is this correct? Typically when you have an empty set, nothing should be able to match it. Just like how [].include?(object) will always return false regardless of what object is in ruby. When it's an empty set, perhaps the SQL should be something that always returns false, like "1 = 0" or something similar.

Also Enumerable#any? will return false if the Array values are false or nil, eg: [ nil, false ].any? # => false

@andhapp

Using "1=0"(i.e. false) or (NULL) in the sql would yield the same results isn't it or am I being very shortsighted?

I do agree with the second argument.

@dkubb

@andhapp: Oh my mistake, you're right. I forgot that IN(NULL) is also false. I'd gotten into the habit of using 1=0 to represent false, and 1=1 to represent true when I am building up SQL queries programmatically, which feels more intuitive to me.

The second point I made could probably be resolved by using any? { |e| !e.nil? }.

@andhapp

@dkubb: Fix for second point seems alright to me.

@lifo
Collaborator

Ah, thanks for catching this dkubb. I think using .present? would work too ?

@dkubb

@lifo: I'm not sure. My first thought was that Array#present? would work, but then I started to think about a few different scenarios and I'm unsure what the best approach should be. Here's a few scenarios, and what (I think) the SQL should be:


[]                # => "col IN (NULL)" (although as stated earlier, I generally prefer 1=0 to represent "nothing matches".
[ 'value' ]       # => "col IN ('value')"
[ false ]         # => "col IN (0)"  (or whatever represents false in the DB)
[ nil ]           # => "col IS NULL"
[ nil, 'value' ]  # => "col IS NULL OR col IN ('value')"

I'm especially unsure about the last one, because it's one of those weird SQL generation edge cases. I'm guessing that the nil could be at the end, or in the middle of the Array, so it would need to be identified, and then the OR condition appended. Not sure if you want to handle this case or not.

@andhapp

@dkubb: IMHO, if we get rid of the nil's then case 4 and case 1 are identical and case 5 will become:

[ 'value' ]  # => "col IN ('value')"  (and this would be okay because "col IS NULL " with an OR would not change the results. 
Please sign in to comment.
Something went wrong with that request. Please try again.