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 unscoping bug in preload and includes #16367

Closed
wants to merge 1 commit into from

Conversation

k0kubun
Copy link
Contributor

@k0kubun k0kubun commented Aug 1, 2014

This PR fixes #11036.

Currently unscoped block does not work for includes and preload.
I changed ActiveRecord::Associations::Preloader to use all instead of default_scoped.
And ignore :preload and :includes scope to pass #5667.

@rafaelfranca
Copy link
Member

Tests are broken with this patch.

@egilburg
Copy link
Contributor

egilburg commented Aug 1, 2014

Seems it broke cases designed to cover #5667. The way I understand that issue, though, is that it used unscoped as a means to avoid infinite recursion bug, rather than as its own design decision.

@k0kubun
Copy link
Contributor Author

k0kubun commented Aug 2, 2014

Ignoring scoping closes a chance to unscope.
At first, I couldn't understand why test_preload_ignores_the_scoping exists.

The way I understand that issue, though, is that it used unscoped as a means to avoid infinite recursion bug, rather than as its own design decision.

I see. I understand test_preload_ignores_the_scoping is not a design but a way to fix test_scoping_with_a_circular_preload.

I'll find another way to fix circular preload without ignoring the scoping.

@k0kubun
Copy link
Contributor Author

k0kubun commented Aug 2, 2014

I fixed broken tests. 93be88e
I changed ActiveRecord::Associations::Preloader::Association#build_scope not to ignore scoping except preload and includes scope.

In this line, the klass's preload/includes values are already preloaded/included.
So I unscoped only preload/includes scoping. That fixes #5667 without ignoring all scoping.

@egilburg
Copy link
Contributor

egilburg commented Aug 2, 2014

I'd combine both changelogs into one.

Unscope preload and includes in unscoped block. Preload and includes scope is still ignored for #5667.

Fixes #11036.

*Takashi Kokubun*

@@ -346,7 +346,7 @@ def reorder!(*args) # :nodoc:

VALID_UNSCOPING_VALUES = Set.new([:where, :select, :group, :order, :lock,
:limit, :offset, :joins, :includes, :from,
:readonly, :having])
:readonly, :having, :preload])
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change deserve an independent spec? It's behavior that could be reused for other purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

preload changes preload_values.
:preload is in not SINGLE_VALUE_METHODS but MULTI_VALUE_METHODS.
Thus symbol_unscoping changes preload_values and it works well.

I think the feature of unscoping preload itself is reasonable because currently unscoping includes is supported.
I'll add some tests which assert it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the test ensuring that unscoping preload works. 6c19f24
Then I squashed all commits into 1145c48.

@k0kubun
Copy link
Contributor Author

k0kubun commented Aug 3, 2014

I'd combine both changelogs into one.

Thank you for a nice suggestion.
I squashed commits and combined changelogs. 69bb040

@k0kubun k0kubun changed the title Unscope preload and includes in unscoped block Fix unscoping bug in preload and includes Aug 4, 2014
Change preload not to ignore scoping.
Preload and includes scope is still ignored for rails#5667.

And preload is changed to be unscopable.
@k0kubun
Copy link
Contributor Author

k0kubun commented Nov 24, 2014

#11036 is resolved by #17360

@k0kubun k0kubun closed this Nov 24, 2014
@k0kubun k0kubun deleted the unscope_includes_and_preload branch November 24, 2014 12:56
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.

Using Includes and Unscoped
3 participants