Skip to content
This repository has been archived by the owner on Mar 30, 2022. It is now read-only.

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

Closed
azhi opened this issue Jan 4, 2016 · 5 comments

Comments

@azhi
Copy link

azhi commented Jan 4, 2016

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 #361 (comment).

@jlhonora
Copy link

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

@mckinnsb
Copy link

mckinnsb commented Mar 3, 2016

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
Copy link
Author

azhi commented Mar 3, 2016

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
Copy link

vellotis commented Mar 3, 2016

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

@azhi
Copy link
Author

azhi commented Mar 3, 2016

@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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants