Ensure Nodes::In and Nodes::NotIn know how to handle arrays with nil values #245

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@vanderhoorn

Currently Nodes::In and Nodes::NotIn do not take into account if an array has a nil value, so it generates queries like "users"."id" IN (NULL) and "users"."id" NOT IN (1, 2, NULL). This patch ensures the queries become "users"."id" IS NULL and ("users"."id" NOT IN (1, 2) AND "users"."id" IS NOT NULL)

Note that IN empty array [] still generates 1=0 and NOT IN empty array [] still generates 1=1.

I think this also fixes issue #227.

Method Argument Before this patch After this patch
In [] 1=0 1=0
In [nil] "users"."id" IN (NULL) "users"."id" IS NULL
In [1, 2, nil] "users"."id" IN (1, 2, NULL) ("users"."id" IN (1, 2) OR "users"."id" IS NULL)
NotIn [] 1=1 1=1
NotIn [nil] "users"."id" NOT IN (NULL) "users"."id" IS NOT NULL
NotIn [1, 2, nil] "users"."id" NOT IN (1, 2, NULL) ("users"."id" NOT IN (1, 2) AND "users"."id" IS NOT NULL)
@eric-chahin

This comment has been minimized.

Show comment Hide comment
@eric-chahin

eric-chahin Feb 9, 2014

👍

👍

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Feb 9, 2014

Owner

I'm not sure. I expect exactly the behavior of before this patch.

@tenderlove WDYT?

Owner

rafaelfranca commented Feb 9, 2014

I'm not sure. I expect exactly the behavior of before this patch.

@tenderlove WDYT?

@tenderlove

This comment has been minimized.

Show comment Hide comment
@tenderlove

tenderlove Feb 9, 2014

Owner

I think before this patch the behavior was undefined. I'm kind of torn. First, is "garbage in, garbage out". If you provide a nil value in your set like node = @attr.in [1, 2, nil], it doesn't make sense. You should do node = @attr.in([1, 2]).or(@attr.eq(nil)). OTOH, I think we support [nil] somewhere?

@ernie do you have any ideas?

If we are going to support this syntax, it should be at the ast level. It should generate new AST nodes (like an OR node and an EQUAL node) rather than hardcoding special case strings.

Owner

tenderlove commented Feb 9, 2014

I think before this patch the behavior was undefined. I'm kind of torn. First, is "garbage in, garbage out". If you provide a nil value in your set like node = @attr.in [1, 2, nil], it doesn't make sense. You should do node = @attr.in([1, 2]).or(@attr.eq(nil)). OTOH, I think we support [nil] somewhere?

@ernie do you have any ideas?

If we are going to support this syntax, it should be at the ast level. It should generate new AST nodes (like an OR node and an EQUAL node) rather than hardcoding special case strings.

@vanderhoorn

This comment has been minimized.

Show comment Hide comment
@vanderhoorn

vanderhoorn Feb 9, 2014

[1, 2, nil] doesn't seem like garbage to me. It's a perfectly reasonable array for a lot of things.

One more argument to add:
eq() also checks if a value is nil and if so, it uses a different syntax than if the value is not nil. If you want consistency than either in() should support nil values like eq() does, or .eq(nil) should result in = NULL and a separate method .is(null) should return IS NULL.

It should generate new AST nodes (like an OR node and an EQUAL node) rather than hardcoding special case strings.

Agreed.

[1, 2, nil] doesn't seem like garbage to me. It's a perfectly reasonable array for a lot of things.

One more argument to add:
eq() also checks if a value is nil and if so, it uses a different syntax than if the value is not nil. If you want consistency than either in() should support nil values like eq() does, or .eq(nil) should result in = NULL and a separate method .is(null) should return IS NULL.

It should generate new AST nodes (like an OR node and an EQUAL node) rather than hardcoding special case strings.

Agreed.

@bachand bachand referenced this pull request Feb 21, 2014

Closed

not_in with empty array #227

@tamird

This comment has been minimized.

Show comment Hide comment
@tamird

tamird Sep 16, 2014

needs a rebase

tamird commented Sep 16, 2014

needs a rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment