Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Join by adding conditions instead of replacing them #67

Open
pelletencate opened this issue May 12, 2017 · 10 comments
Open

Join by adding conditions instead of replacing them #67

pelletencate opened this issue May 12, 2017 · 10 comments

Comments

@pelletencate
Copy link

pelletencate commented May 12, 2017

Feature request: In an ORM I used to use in my former life, there was an alternative to .on for joins that was called .with. Basically, what it did was take the original join conditions, and add another condition to it using AND. I found this to be extremely helpful, as part of the reasons why you use an ORM is that you don't have to bother with join conditions, and most of the time when you want to override them, it's because you want to restrict them by adding something to the original conditions.

Basically, it works like this:

# Implicitly follows schema
A.joining { b }
# SELECT * FROM a INNER JOIN b ON b.a_id = a.id

# Explicitly, it'd look like this:
A.joining { b.on(b.a_id == id) }
# SELECT * FROM a INNER JOIN b ON b.a_id = a.id

# Now, adding a condition
A.joining { b.on((b.a_id == id) & (b.active == true)) }
# SELECT * FROM a INNER JOIN b ON b.a_id = a.id AND b.active == true

# My suggestion:
A.joining { b.with(b.active == true) }
# SELECT * FROM a INNER JOIN b ON b.a_id = a.id AND b.active == true

Especially if your models are named MerchantIdentityOperatorExtension instead of A, this can save you a lot of code that would be the same in > 90% of the usages of .on

@rzane
Copy link
Owner

rzane commented May 12, 2017

This is definitely an interesting idea. Though, I think it might be really tricky to implement.

The problem is that BabySqueel lets Active Record do the heavy lifting in the case of an implicit join. In the case of an explicit join, it's just Arel.

Combining the two would be very difficult and more likely to break as Active Record changes. However, I think your idea could be implemented more easily if it worked like this:

A.joining { b.with(b.active == true) }
# SELECT * FROM a INNER JOIN b ON b.a_id = a.id WHERE b.active = true

@octavpo
Copy link

octavpo commented May 23, 2017

This won't give the same result in an outer join.

@pelletencate
Copy link
Author

True, that won't work for outer joins.

Can't you convert a query to Arel after AR has done the heavy lifting, and then simply append an AND part to it?

@octavpo
Copy link

octavpo commented Sep 11, 2017

Any chance to have this implemented? It would be nice not to have to repeat the implicit join clause.

@rzane
Copy link
Owner

rzane commented Sep 11, 2017

I'd say pretty unlikely. BabySqueel isn't generating those join nodes: Active Record is (with a lot of help from polyamorous). I think even if we got this working, it would be brittle, and I'd like to avoid that.

That being said, if someone came up with a clever solution here, I'd be very interested in seeing it. It is a really cool feature request and I'd love to have it in baby_squeel.

@octavpo
Copy link

octavpo commented Sep 14, 2017

Wouldn't @pelletencate's suggestion work? So for cases where the joined table is an association, you'd call AR to generate an AR relation, then apply an .arel and add the on condition. For other cases just do arel directly as usual. I don't even think it needs a new construct, it's hard to believe anybody would want to join an association and not want the default condition added automatically.

@rzane
Copy link
Owner

rzane commented Sep 14, 2017

Yes, I agree, but I think this is one of those things that is easier said than done. More than happy to accept a PR.

@pelletencate
Copy link
Author

I'm not quite a wizard with Arel, but if I have some spare time within the next few weeks (and that's unfortunately a big if), I'll give it a try.

@pelletencate
Copy link
Author

pelletencate commented Nov 29, 2017

@rzane I haven't had time to look into this yet. But I do realize, we may be able to steal some code from ActiveRecord, they must do something similar in their implementation of .where; that is, if you call .where multiple times on a single relation, it somehow hangs all of those together with AND. The exact same logic should be used using the .with. I haven't looked in the source code yet, but I just realized that where it's most of the time a convenience, it can become a necessity if you are working with default scopes that need to be applied in place to joined models.

@octavpo
Copy link

octavpo commented Nov 29, 2017

Unless I'm missing something I don't see how that helps. For all query helpers AR just stores all user options in instance variables until the time it needs to generate the query, when it combines them appropriately. The issue here is that unlike in the case of where, we have these on conditions that AR doesn't know how to handle AFAIK. And on the other hand Arel doesn't know how to generate the implicit join conditions. So you still need to somehow send the 'naked' joined models to AR first to make it generate the implicit conditions, and then get the generated Arel and add the on conditions, as you were suggesting in a previous comment. Or the only alternative I can see would be to mimic the AR code in baby_squeel and make it generate the implicit Arel conditions, which I think goes beyond what @rzane wants to do in this package.

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

No branches or pull requests

3 participants