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

broken hash conditions when querying for an activerecord object on a joined table #338

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ashanbrown
Copy link

Not really a pull request, just a test. Seems like squeel breaks the following expressions:

    Comment.joins(:article).where(articles: {person_id: person})
    Comment.joins(:article).where(articles: {person: person})

These will both raise an exception telling us that 'articles' cannot be converted to a class.

The fix looks like it would be pretty involved. Essentially squeel is trying to replicate AR predicate builder, but it might be better offer using it, and somehow converting the resulting arel back into something it can work with because trying to keep the behavior up to date and matching might be a pain.

@ashanbrown
Copy link
Author

Looks like a dup of #313 which was closed, but this behavior definitely fails.

@ashanbrown ashanbrown changed the title broken hash conditions broken hash conditions when querying for an activerecord object on a joined table Sep 10, 2014
@bigxiang
Copy link
Contributor

@dontfidget Thank you very much. Does it mean AR can pass this test? Squeel has a long history when AR was not as good as current. So it provided another way to build predicates to Arel. From Rails 3.0 to 4.2, AR changed a lot and is changing a lot, so it's harder to keep the same behaviors with AR. I'd like to consider your suggestion to use AR's code as possible as we can. It needs lots of works. Any information you provide would be helpful :)

@ashanbrown
Copy link
Author

I assume AR can pass the test. It was in my app and it started failing when I added squeel. Unfortunately, I don't know any more about AR than taking a cursory glance and seeing that there was something called a predicate builder. I don't know how hard it would to use it. In general, I don't understand why squeel is involved at all for a where clause that isn't passed a block, but I guess it must because it is built on top of arel rather than AR. Anyhow, I just worked the problem by writing it as:

   Comment.joins(:article).where('articles.person_id' => person})

Incidentally, the reason this syntax is handy is that you can pass both a relation and a single AR record to it, which I've found convenient when defining scopes.

@bigxiang
Copy link
Contributor

hum... you can also use squeel like this:

Comment.joins { article }.where { article.person_id == person }

It's also convenient when defining scopes :)

@ashanbrown
Copy link
Author

That true, but I'm not in the habit of using squeel unless I really need it
to make things more readable, so my workaround has the property that it
equally readable and works whether I'm using squeel or not. So far, at
least on rails 4.1.x, calling squeel production ready is a bit of a
stretch, so I currently don't want to commit to it more than I have to.
The fact that it breaks my existing code is a bit terrifying, although by
using the master branch of squeel and fixing a few things here and there, I
think I'm going to run with it because I do much appreciate the syntax
improvements it offers over arel. I'm looking forward to using it more and
greatly appreciate all the work you've done so far to make it work at all
on rails 4.

Andrew

On Wed, Sep 10, 2014 at 10:14 PM, Xiang Li [email protected] wrote:

hum... you can also use squeel like this:

Comment.joins { article }.where { article.person_id == person }

It's also convenient when defining scopes :)


Reply to this email directly or view it on GitHub
#338 (comment)
.

@bigxiang
Copy link
Contributor

Yes, one of Squeel's goal is to make users couldn't detect the existence of Squeel when they are not using them. If you find more compatible problems, just tell me, I would schedule them one by one. Then I would try to find a way to refactor the code. Thank you.

@sevaorlov
Copy link

@bigxiang, hi, do you plan to move it forward?

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

Successfully merging this pull request may close these issues.

3 participants