[ci skip] updating active_record/associations to demonstrate where conflict with eager loading. #14139

Merged
merged 1 commit into from Apr 28, 2014

Conversation

Projects
None yet
5 participants
Contributor

anilmaurya commented Feb 21, 2014

Related to #12270

Adding example of a LEFT JOIN with comment.visible=true as an ON condition instead.

Member

arthurnn commented Apr 5, 2014

Thanks for the PR. However I am not 100% sure if this is necessary..
@senny @rafaelfranca thoughts

@senny senny commented on an outdated diff Apr 28, 2014

activerecord/lib/active_record/associations.rb
@@ -767,6 +767,13 @@ def association_instance_set(name, association)
# like this can have unintended consequences.
# In the above example posts with no approved comments are not returned at all, because
# the conditions apply to the SQL statement as a whole and not just to the association.
+ #
+ # The where clause will filter away rows where the left join doesn't succeed.
@senny

senny Apr 28, 2014

Member

the last sentence from the paragraph above already states that this might have unintended effects. Let's get rid of that first line and start directly with:

# If you want to load all posts (including posts with no approved comments) then write
Contributor

anilmaurya commented Apr 28, 2014

@senny updated PR.

@senny senny added a commit that referenced this pull request Apr 28, 2014

@senny senny Merge pull request #14139 from anilmaurya/master
[ci skip] updating active_record/associations to demonstrate where conflict with eager loading.
b7b319e

@senny senny merged commit b7b319e into rails:master Apr 28, 2014

Member

senny commented Apr 28, 2014

thanks 💛

@senny senny added a commit that referenced this pull request Apr 29, 2014

@senny senny Merge pull request #14139 from anilmaurya/master
[ci skip] updating active_record/associations to demonstrate where conflict with eager loading.
4841ad3

@carlosantoniodasilva carlosantoniodasilva commented on the diff May 2, 2014

activerecord/lib/active_record/associations.rb
@@ -767,6 +767,12 @@ def association_instance_set(name, association)
# like this can have unintended consequences.
# In the above example posts with no approved comments are not returned at all, because
# the conditions apply to the SQL statement as a whole and not just to the association.
+ #
+ # If you want to load all posts (including posts with no approved comments) then write
+ # your own LEFT OUTER JOIN query using ON
+ #
+ # Post.joins('LEFT OUTER JOIN comments ON comments.post_id = posts.id AND comments.approved = true')
@carlosantoniodasilva

carlosantoniodasilva May 2, 2014

Owner

Should we use comments.approved = true in the join clause? I mean, true might not work on some dbs and all that stuff, maybe we should add to the where instead?

@senny

senny May 3, 2014

Member

you are right that true might not work. However this example needs to have the condition in the join clause. Using where will give you another result. That is what this paragraph is all about. You see that the previous example actually uses where.

I agree that true is not optimal. I'm going to change it to '1', which should work for MySQL tinyint(1) and PostgreSQL boolean fields.

@senny senny added a commit that referenced this pull request May 3, 2014

@senny senny docs, restructure newly added part to `includes`. [ci skip]
This is a follow up to #14139.

/cc @carlosantoniodasilva
5757fe4
Contributor

anilmaurya commented May 3, 2014

thanks @senny for the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment