activerecord-hackery / squeel

Active Record, improved. Live again :)
http://erniemiller.org/2013/11/17/anyone-interested-in-activerecord-hackery/
MIT License
2.4k stars 213 forks source link

Using complex IN + OR scope inside an association results in invalid SQL on join condition #411

Closed azhi closed 8 years ago

azhi commented 8 years ago

Squeel 1.2.3 + AR 4.1.14 seems to generate invalid SQL when joining with an association that includes both simple condition and complex IN + OR condition.

E.g. a Parent model with has_many :complex_scoped_models, ->{ where(flag: true).where(field: ['test1', 'test2', nil]) }, class_name: 'Model' will generate following SQL when doing Parent.all.joins(:complex_scoped_models).first:

SELECT  "parents".* FROM "parents" INNER JOIN "models" ON "models"."parent_id" = "parents"."id" AND "models"."flag" = 't', (("models"."field" IN ('test1', 'test2') OR "models"."field" IS NULL))  ORDER BY "parents"."id" ASC LIMIT 1

Note the comma after "models"."flag" = 't', when there should be another AND. Same scope on the model itself (without an association) seems to be working.

I've put up a test bench for the issue, you can see full code and test at https://github.com/azhi/squeel_complex_scope_association_test.

PS: it looks similar to an error pointed by @vellotis in https://github.com/activerecord-hackery/squeel/issues/361#issuecomment-141457924.

jlhonora commented 8 years ago

Seeing this as well in a similar scenario but with AR 4.2.5.

mckinnsb commented 8 years ago

Hey @azhi,

Your test fails without Squeel; "parent" ends up being nil with that query. Is that intended?

It seems like it is omitting the AND from the query.

azhi commented 8 years ago

Hey @mckinnsb

Your test fails without Squeel; "parent" ends up being nil with that query. Is that intended?

Oops, my bad, fixed the test in the repo. Now it passes without Squeel.

It seems like it is omitting the AND from the query

Yeah, looks like it. More precisely, it uses a comma instead of AND.

vellotis commented 8 years ago

@azhi I had the exact comma vs AND problem @ #361.

azhi commented 8 years ago

@vellotis yeah, seems like it's the same issue, PR #362 fixes my use case as well.

I guess i'll close this issue as a duplicate, thanks everyone.