Skip to content
Browse files

Do not generate NOT IN (NULL) when empty right

  • Loading branch information...
1 parent af07bd1 commit 62207faee92c820da9fbc9cc6fe785461c2d826b @spastorino spastorino committed
Showing with 5 additions and 5 deletions.
  1. +1 −1 lib/arel/visitors/to_sql.rb
  2. +4 −4 test/visitors/test_to_sql.rb
View
2 lib/arel/visitors/to_sql.rb
@@ -467,7 +467,7 @@ def visit_Arel_Nodes_InfixOperation o
alias :visit_Arel_Nodes_Division :visit_Arel_Nodes_InfixOperation
def visit_Array o
- o.empty? ? 'NULL' : o.map { |x| visit x }.join(', ')
+ o.map { |x| visit x }.join(', ')
end
def quote value, column = nil
View
8 test/visitors/test_to_sql.rb
@@ -167,10 +167,10 @@ def dispatch
}
end
- it "should turn empty right to NULL" do
+ it "should return IN () when empty right which is invalid SQL" do
node = @attr.in []
@visitor.accept(node).must_be_like %{
- "users"."id" IN (NULL)
+ "users"."id" IN ()
}
end
@@ -255,10 +255,10 @@ def quote value, column = nil
}
end
- it "should turn empty right to NULL" do
+ it "should return NOT IN () when empty right which is invalid SQL" do
node = @attr.not_in []
@visitor.accept(node).must_be_like %{
- "users"."id" NOT IN (NULL)
+ "users"."id" NOT IN ()
}
end

8 comments on commit 62207fa

@dkubb

@spastorino What about generating something that always evaluates to false, like 1 = 0 instead of generating invalid SQL? Or is the goal to move ARel away from trying to infer the user's intentions, and map to SQL 1:1 more closely, even if it results in invalid SQL?

@ernie
Ruby on Rails member

@spastorino I do this with Squeel, after @dkubb mentioned it. It's been like that for months and months and I haven't gotten complaints. Highly recommended.

@spastorino
Ruby on Rails member

I'm ok with that if you want to change it, just do it ;).
But @dkubb are you saying that a user intentions of node = @attr.in [] is to execute a query that never returns a value?.

I've committed this to prevent User.find_by_token(params[:token]) generating NOT IN (NULL) which if the value of ANSI_NULLS is OFF could be a sec issue.

/cc @tenderlove @NZKoz

@dkubb

@spastorino yes, that's what I'm saying.

Asking if a value is in an empty set should always be false. It also matches ruby's semantics for [].include?(anything) (it's always false regardless of the value).

I use a similar approach in DataMapper, although there is a bug in the DM logic that I'm about to release a fix for in the next day or so, which is why this is all fresh in my mind. The logic I'm moving towards is identical to what @ernie shows above.

I can't speak to the security implications for AR specifically since I'm not as familiar with it's internals as others here.

@spastorino
Ruby on Rails member

@dkubb cool, no worries. I'm ok with the idea, feel free to patch it.

@ernie
Ruby on Rails member

Yeah, the meaning of "in an empty set" is always false. The meaning of "not in an empty set" is always true.I'd be happy to write something to that effect. It'd involve a little bit of branching in visit_Arel_Nodes_In and visit_Arel_Nodes_NotIn. My only question would be whether this should get backported to the various -stable branches, or we should just let things there alone, and keep this for master.

@ernie
Ruby on Rails member

Pushed cbff1bc based on this discussion. I seem to remember having a talk with @tenderlove a long while back about this, and I think at the time he felt it would be better to render an IN/NOT IN regardless of the RHS of the node. If that's still the case, revert away. It was a 30 second thing. :)

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