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

Fix eager loading association using default_scope for finder methods. #19586

Merged

Conversation

mechanicles
Copy link
Contributor

  • Eager loading was not working for the default_scope (class method)
    for 'find' & 'find_by' methods.
  • Fixed these by adding a new check 'respond_to?(:default_scope)'.

@mechanicles mechanicles force-pushed the fix-eager-loading-for-find-methods branch from 978b802 to 03609fd Compare March 30, 2015 07:48
@mechanicles
Copy link
Contributor Author

@rafaelfranca Sorry I don't know to whom to address this PR currently. So I pinged you.

Actually yesterday I sent this PR. But today morning, I saw that Travis CI got failed. I checked that and my PR got failed mostly for jruby and rbx related builds. Today I rebased with master and did force push to my branch. But now Travis doesn't show any status. Let me know if I missed something.

@rafaelfranca
Copy link
Member

No problem, Is this PR fixing any issue?

@mechanicles
Copy link
Contributor Author

@rafaelfranca I just checked that is there any similar issues to this issue. But didn't get any. Actually I found this issue while testing this file activerecord/test/cases/associations/eager_test.rb and found that eager loading is not working for some finder methods if we use default_scope as class method.

@@ -142,6 +142,7 @@ def find(*ids) # :nodoc:
return super if block_given? ||
primary_key.nil? ||
scope_attributes? ||
respond_to?(:default_scope) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not we only return if it is a eager loading?

@mechanicles
Copy link
Contributor Author

@rafaelfranca I'm not getting your point. Could you elaborate more on it?.

Also one more thing, I have added respond_to?(:default_scope) in two places in this PR. We already have a method scope_attributes? in scoping/default.rb. I think, it's good place to move respond_to?(:default_scope) over there.

something like this,

 # Are there attributes associated with this scope?
def scope_attributes? # :nodoc:
  super || default_scopes.any? || respond_to?(:default_scope)
end

Let me know your thoughts

@mechanicles
Copy link
Contributor Author

@rafaelfranca Also it is not just about eager loading. Actually problem is, if we define default_scope as class method, then it doesn't work for finder methods find and find_by.

class Post
  def self.default_scope
    where(published: true)
  end
end

Suppose posts table has columns like category and published.

If we try to search a post like,

Post.find_by(category: 'rails')

Then here 'default_scope' doesn't work. It doesn't not load published: true in the query.

@rafaelfranca
Copy link
Member

Right, now I get the problem.

Yeah, I think we should move the respond_to? to that method.

@mechanicles mechanicles force-pushed the fix-eager-loading-for-find-methods branch from 03609fd to 100d33d Compare March 31, 2015 17:23
- Eager loading was not working for the default_scope (class method)
  for 'find' & 'find_by' methods.
- Fixed these by adding a new check 'respond_to?(:default_scope)'.
@mechanicles mechanicles force-pushed the fix-eager-loading-for-find-methods branch from 100d33d to 4596e16 Compare March 31, 2015 17:25
@mechanicles
Copy link
Contributor Author

@rafaelfranca I have update code and moved respond_to?(:default_scope) to scope_attributes? method.

rafaelfranca added a commit that referenced this pull request Mar 31, 2015
…-methods

Fix eager loading association using default_scope for finder methods.
@rafaelfranca rafaelfranca merged commit baa176e into rails:master Mar 31, 2015
@mechanicles mechanicles deleted the fix-eager-loading-for-find-methods branch March 31, 2015 18:08
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

Successfully merging this pull request may close these issues.

None yet

2 participants