Skip to content

Don't ignore joins from association scopes in preloader #12538

Closed
wants to merge 1 commit into from

5 participants

@mikecmpbll

Pull request for #12492

It'll need a test adding; I was hoping someone else might be able to help me with that. This fixes my example in the original issue and the test suite doesn't break!

@pftg
pftg commented Oct 15, 2013

May you add some tests for this PR?

@pftg pftg commented on the diff Oct 15, 2013
activerecord/lib/active_record/relation/query_methods.rb
@@ -400,6 +400,9 @@ def joins(*args)
end
def joins!(*args) # :nodoc:
+ args.reject!(&:blank?)
+ args.flatten!
@pftg
pftg added a note Oct 15, 2013

Is this change needed for this PR. I think this should be extracted to separated PR. What do you think?

@mikecmpbll
mikecmpbll added a note Oct 15, 2013

Well, when there was no join conditions (preload_values[:joins] || values[:joins] == nil) it was causing an error, 250 or so tests were failing. I noticed that .includes! has this code in it, and that was why it wasn't failing with no includes conditions, didn't see why the two weren't the same.

I could've put this check in associations/preloader/association.rb#build_scope instead with something like:

joins = preload_values[:joins] || values[:joins]
scope.joins! joins if joins

(or something cleaner, perhaps modifying joins_values directly as it does with where_values etc) but changing joins! to match includes! felt a nicer solution to me and didn't break any existing tests.

@pftg
pftg added a note Oct 15, 2013

Thanks for clarification, agree to leave this as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca
Ruby on Rails member

@pftg

It'll need a test adding; I was hoping someone else might be able to help me with that. This fixes my example in the original issue and the test suite doesn't break!

😄

@pftg
pftg commented Oct 15, 2013

@mikecmpbll I'd like to help with tests, will work on it (thanks @rafaelfranca, my bad )

@mikecmpbll

@pftg @rafaelfranca I've not worked with the rails/activerecord test suite yet so wasn't sure where abouts it would belong and whether to test it from a higher-level perspective as per my gist in the issue, or to test the AR methods themselves etc. I'm sure these are things I could infer from existing tests though so I'll take a look this afternoon.

Thanks guys.

@nixme nixme commented on the diff Feb 15, 2014
...b/active_record/associations/preloader/association.rb
@@ -118,6 +118,7 @@ def build_scope
scope.select! preload_values[:select] || values[:select] || table[Arel.star]
scope.includes! preload_values[:includes] || values[:includes]
+ scope.joins! preload_values[:joins] || values[:joins]
@nixme
nixme added a note Feb 15, 2014

I think you need to either splat * the argument or change joins! in query_methods.rb to not use +=

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mikecmpbll mikecmpbll closed this Mar 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.