Node methods containing multiple children make inplace changes #251

Closed
odedniv opened this Issue Jul 10, 2013 · 5 comments

Projects

None yet

3 participants

@odedniv
odedniv commented Jul 10, 2013

The problem is in the file: lib/squeel/nodes/nary.rb

Works as expected:

User.where do
  tolerant_query = (x == 1)
  strict_query = tolerant_query & (z == 3)
  tolerant_query | strict_query
end.to_sql
=> "SELECT `users`.* FROM `users` WHERE `users`.`x` = 1 OR (`users`.`x` = 1 AND `users.`z` = 3`)"

Works as unexpected:

User.where do
  tolerant_query = (x == 1) & (y == 2)
  strict_query = tolerant_query & (z == 3)
  tolerant_query | strict_query
end.to_sql
=> "SELECT `users`.* FROM `users` WHERE (`users`.`x` = 1 AND `users`.`y` = 2 AND `users`.`z` = 3) OR (`users`.`x` = 1 AND `users`.`y` = 2 AND `users`.`z` = 3)"

The operators makes an inplace change and returns self, so tolerant_query and strict_query are actually the same object.
The operator & should return a new node (clone doesn't do the trick).

@ernie
Member
ernie commented Jul 10, 2013

I don't disagree, but if you read the documentation for Squeel::Nodes::Nary, you'll see it behaves as documented.

@odedniv
odedniv commented Jul 10, 2013

Behaves as documented but not as expected, took a long time to figure out, my query is a little bit more complicated than that ;)
Will it be fixed? For now I moved tolerant_query into a sifter so it is generated twice, figuring it's a good solution for others having the same issue.

@ernie
Member
ernie commented Jul 10, 2013

I'll change the behavior it but it will go into 1.1 (master), and not be backported to 1.0.x.

@odedniv
odedniv commented Jul 10, 2013

Thanks plenty!

@odedniv odedniv closed this Jul 10, 2013
@odedniv odedniv reopened this Jul 10, 2013
@the8472
the8472 commented Jul 10, 2013

As a backwards-compatible fix I would suggest making them dup-able, similar to keypath, which also gets modified in place.
That way you can reuse them simply by duping.

@ernie ernie added a commit that closed this issue Jul 14, 2013
@ernie ernie Stop mutating And nodes with & and -
Fixes #251
616ec03
@ernie ernie closed this in 616ec03 Jul 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment