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

Support unscoping default_scope in eager_loaded associations #16531

Conversation

cwjenkins
Copy link

For eager_loaded joins move the default scope to the front of the scope chain to be merged first.
For preloading check values for unscope and add to the scope being built unscope_values.
On preload, no need to change merge sequence given default scope is merged first.

@cwjenkins
Copy link
Author

@rafaelfranca or @jonleighton is there anything else I should add?

@cwjenkins cwjenkins force-pushed the allow-unscoping-default-scope-on-associations branch 3 times, most recently from fd4fab3 to db08e83 Compare August 25, 2014 14:19
@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Sep 10, 2014
For eager_loaded joins move the default scope to the front of the scope chain to be merged first.
For preloading check values for unscope and add to the scope being built unscope_values.
On preload, no need to change merge sequence given default scope is merged first.
@bquorning
Copy link
Contributor

What happens if Post has the same default_scope, i.e. where(deleted_at: nil)? Would the unscoping on the association know only to remove the condition from the joined table, or would it remove the condition also from the posts table?

grosser added a commit to zendesk/predictive_load that referenced this pull request Sep 2, 2015
unscoped { includes } is broken in rails 4+
so we try to avoid this bug by disabling eager loading -> triggering less includes

maybe coming fix: rails/rails#16531
grosser added a commit to zendesk/predictive_load that referenced this pull request Sep 2, 2015
unscoped { includes } is broken in rails 4+
so we try to avoid this bug by disabling eager loading -> triggering less includes

maybe coming fix: rails/rails#16531
grosser added a commit to zendesk/predictive_load that referenced this pull request Sep 2, 2015
unscoped { includes } is broken in rails 4+
so we try to avoid this bug by disabling eager loading -> triggering less includes

maybe coming fix: rails/rails#16531
grosser added a commit to zendesk/predictive_load that referenced this pull request Sep 2, 2015
unscoped { includes } is broken in rails 4+
so we try to avoid this bug by disabling eager loading -> triggering less includes

maybe coming fix: rails/rails#16531
grosser added a commit to zendesk/predictive_load that referenced this pull request Sep 2, 2015
unscoped { includes } is broken in rails 4+
so we try to avoid this bug by disabling eager loading -> triggering less includes

maybe coming fix: rails/rails#16531
@akaspick
Copy link
Contributor

Any chance of this being backported to Rails 4.2 as well?

@rafaelfranca rafaelfranca removed this from the 5.0.0 [temp] milestone Apr 5, 2016
@NullVoxPopuli
Copy link

Is this fixed in rails 5?

@k0kubun
Copy link
Contributor

k0kubun commented Apr 2, 2017

This will fix #20679 for joins/eager_load and also covers preload/includes(without references).

While we can unscope default_scope in joins/eager_load by #18109, somehow the counterpart for preload/includes is rejected in #25187. So this PR would be the only way to unscope default_scipe in preload/includes.

@cwjenkins Could you rebase this against master?

kamipo added a commit to kamipo/rails that referenced this pull request Jun 28, 2017
Unscoping `default_scope` in associations has already supported (rails#17360
for preloading, c9cf8b8 for eager loading).

Fixes rails#20679.
Closes rails#16531.
kamipo added a commit that referenced this pull request Sep 4, 2017
Unscoping `default_scope` in associations has already supported (#17360
for preloading, c9cf8b8 for eager loading).

Fixes #20679.
Closes #16531.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants