added ActiveRecord::Relation#left_outer_joins #12071

Merged
merged 1 commit into from Oct 30, 2015

Projects

None yet
@Crunch09

Hi,

this adds the ability to use a left outer join query without writing pure SQL as an argument to #joins and also query for a specific selection of columns which doesn't work with #includes.

The ability to do this is already implemented in ActiveRecord::Associations::JoinDependency#build but it was only used with Arel::InnerJoin as join_type.
I am not quite sure about the tests, i just used the InnerJoin tests as a starting point. Any help would be appreciated.

Example:

  User.outer_joins(:posts)
  => SELECT "users".* FROM "users" LEFT OUTER JOIN "posts" ON "posts"."user_id" = "users"."id"
@njakobsen

+1 @Crunch09 Your patch is so much more pro than my poor man's outer_join where I gsub the inner joins to outer joins. https://gist.github.com/njakobsen/6395146

@robin850
Ruby on Rails member

Thanks for your contribution @Crunch09! Actually, it would be awesome if we could update a little bit the guides with this new feature.

@Crunch09

yes, will write something tomorrow

@frodsan frodsan and 1 other commented on an outdated diff Aug 31, 2013
activerecord/lib/active_record/relation/query_methods.rb
@@ -948,6 +983,11 @@ def build_joins(manager, joins)
end
end
+ build_join_query(manager, buckets, Arel::InnerJoin)
+
@Crunch09
Crunch09 Sep 1, 2013

thank you, fixed that!

@Vetal4eg

+1

@csouls

+1

@Crunch09

@robin850 Added few sentences in the AR-querying guide about it and also rebased against current master.

@robin850
Ruby on Rails member

@Crunch09 : Awesome, thank you! ❤️

@rafaelfranca : Could you please have a look ? It looks awesome to me!

@Crunch09

@rafaelfranca @robin850 I just rebased again against current master, anything more i can do?

@toreriklinnerud

+1 Outer joins is one of the few remaining things that force us to fall back to SQL in our codebase

@tenderlove tenderlove commented on an outdated diff Apr 23, 2014
activerecord/lib/active_record/relation/query_methods.rb
@@ -937,6 +971,10 @@ def build_joins(manager, joins)
end
end
+ build_join_query(manager, buckets, Arel::InnerJoin)
+ end
+
+ def build_join_query(manager, buckets, join_type=nil)
@tenderlove
tenderlove Apr 23, 2014

Please do not default join_type to nil. I don't think nil is actually an acceptable value. Change the things that call build_join_query to actually pass in the things it needs.

@tenderlove tenderlove and 1 other commented on an outdated diff Apr 23, 2014
...est/cases/associations/outer_join_association_test.rb
+ assert_no_match(/JOIN/i, sql)
+ end
+
+ def test_construct_finder_sql_ignores_empty_joins_array
+ sql = Author.outer_joins([]).to_sql
+ assert_no_match(/JOIN/i, sql)
+ end
+
+ def test_join_conditions_added_to_join_clause
+ sql = Author.outer_joins(:essays).to_sql
+ assert_match(/writer_type.*?=.*?Author/i, sql)
+ assert_no_match(/WHERE/i, sql)
+ end
+
+ def test_join_conditions_allow_nil_associations
+ authors = Author.includes(:essays).where(:essays => {:id => nil})
@tenderlove
tenderlove Apr 23, 2014

What does this have to do with outer joins?

@Crunch09
Crunch09 Apr 24, 2014

nothing, will remove that and try to improve the tests overall. thank you!

@tenderlove
Ruby on Rails member

Can you please change these tests not to use to_sql? We have a capture_sql command that you can use to make assertions against the sql run against the database.

@tenderlove
Ruby on Rails member

I'm positive on this, but the copy/paste make me uneasy. :(

@matthewd matthewd commented on an outdated diff Apr 23, 2014
guides/source/active_record_querying.md
@@ -937,11 +938,17 @@ This will result in the following SQL:
SELECT clients.* FROM clients LEFT OUTER JOIN addresses ON addresses.client_id = clients.id
```
-### Using Array/Hash of Named Associations
+Instead of writing raw SQL in this case you could also use the `outer_joins` method
+which would produce the same result.
@matthewd
matthewd Apr 23, 2014

If we're going to introduce an outer_joins, I think this is a bit of an "also-ran" position to mention it... we should probably be offering it before we suggest raw SQL.

In fact, maybe the SQL should change, to show a still-legitimate use case for a raw JOIN, as it previously did, instead of an inferior spelling of built-in functionality.

@matthewd
Ruby on Rails member

I'm vaguely wary of outer_joins == LEFT OUTER JOIN. With includes, that's abstracted-away implementation detail. But here...

Maybe consider left_joins?

@tenderlove
Ruby on Rails member

I'm vaguely wary of outer_joins == LEFT OUTER JOIN. With includes, that's abstracted-away implementation detail. But here...

Maybe consider left_joins?

Good point. If it's actually going to do an LOJ, the method should probably be left_joins or left_outer_joins.

@Crunch09

Thank you for your feedback! I'll update this PR over the weekend and report back then

@al2o3cr

One thing I'd love to see but don't have a clear API for - the ability to specify additional bits in the ON clause. Perhaps something like:

User.outer_joins(:posts) { where(featured: true) }

which would produce the SQL:

SELECT ... FROM users
LEFT OUTER JOIN posts ON posts.user_id = users.id AND posts.featured = 't'

assuming there's a boolean featured column on posts.

Note that the above is not equivalent to:

User.outer_joins(:posts).where(featured: true)

since that query doesn't include users who don't have any featured posts.

Without this feature, it would still be possible to create the desired SQL with a scoped association:

# on User
has_many :featured_posts, -> { where(featured: true) }, class_name: 'Post'

User.outer_joins(:featured_posts)

but that method doesn't exactly scale to arbitrary sets of conditions.

@matthewd
Ruby on Rails member

@al2o3cr +1. We should probably address that separately, afterwards, though... whatever we end up with seems like it should apply to includes as well. This would address one of the biggest causes of scoped-association proliferation that I see. (:through targets being the other.)

@njakobsen

Any word on when this will be merged? It's been a long time in coming, and every project I make ends up needing left outer joins.

@Crunch09

it's my fault. There were lots of changes in the affected code and i need to solve the merge conflicts and also apply the feedback. I started with that and i hope i can finish it this weekend, sorry about the long wait.

@Crunch09

I updated the code now and renamed the method to left_outer_joins. I also chose a (hopefully) better example for the guides, as @matthewd suggested.

cc: @tenderlove

@k0kubun

+1

@k0kubun

left_outer_joins is easy to understand, but I think it is a long name.

I propose alias :left_joins :left_outer_joins (otherwise I'll send PR after this patch is merged). Because OUTER keyword can be abbreviated in an actual query, a real behavior is reflected in it.

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Jan 2, 2015
@Crunch09

@k0kubun Good point! I would be fine either way.
@tenderlove I don't know if you remember, but we talked briefly at Arrrcamp about this 😄 I rebased it against current master, would be great if you could take another look if you have the time 😉

@dmitry

@al2o3cr have you already created separate issue for supporting the User.outer_joins(:posts) { where(featured: true) }?

@njakobsen

Doh. I needed a left outer join again today.

@sgrif sgrif locked and limited conversation to collaborators Mar 10, 2015
@Crunch09 Crunch09 added ActiveRecord::Relation#left_outer_joins
Example:
  User.left_outer_joins(:posts)
  => SELECT "users".* FROM "users" LEFT OUTER JOIN "posts" ON "posts"."user_id" = "users"."id"
3f46ef1
@matthewd matthewd unlocked this conversation Sep 10, 2015
@sgrif sgrif was assigned by rails-bot Oct 20, 2015
@sgrif sgrif merged commit 3f46ef1 into rails:master Oct 30, 2015

1 check passed

Details continuous-integration/travis-ci/pr The Travis CI build passed
@sgrif sgrif added a commit that referenced this pull request Oct 30, 2015
@sgrif sgrif Fix test failures caused by #12071
This assumes only one query was ever executed, but it appears to
sometimes be loading schema information. We can just look at the array
of queries, rather than the "first" one that was run
3cd4957
@egilburg

🎉

@thibaudgg

👏 👏

@Crunch09

Thank you all ❤️ So happy this got merged!

@Crunch09 Crunch09 deleted the Crunch09:outer_joins branch Oct 31, 2015
@njakobsen

👏👏

@rafaelfranca rafaelfranca modified the milestone: 5.0.0 [temp], 5.0.0 Dec 30, 2015
@prathamesh-sonpatki prathamesh-sonpatki changed the title from added ActiveRecord::Relation#outer_joins to added ActiveRecord::Relation#left_outer_joins Mar 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment