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

ActiveRecord: use association's `unscope` when preloading #21550

Merged
merged 1 commit into from Sep 24, 2015

Conversation

Projects
None yet
7 participants
@jbourassa
Contributor

jbourassa commented Sep 8, 2015

Preloading an association that is using unscope is dropping the unscope part.

As they say, 10 LoC is worth a thousand words:

class Developer < ActiveRecord::Base
  default_scope -> { where(awesome: true) }
end

class Project < ActiveRecord::Base
  has_and_belongs_to_many :developers, -> { unscope(where: :awesome) },
end

mediocre_dev = Developer.create(awesome: false)
project = Project.create
project.developers << mediocre_dev

project.developers.reload.size
# => 1
Project.where(id: project.id).includes(:developers).first.developers.size
# => 0 β€” but should be 1

This PR fixes that behaviour. This is my first PR on AR (πŸŽ‰) so I'm not all that familiar with the internals, let me know if anything seems wrong.

@jbourassa jbourassa changed the title from Include association's `unscope` when preloading to ActiveRecord: use association's `unscope` when preloading Sep 9, 2015

@markets

This comment has been minimized.

Show comment
Hide comment
@markets

markets Sep 9, 2015

Hi @jbourassa, I think this topic is also addressed here: #11036, #11082, #16531, #18983.

Seems like statements with unscoped (or unscope) + preloading are broken in AR4+.

markets commented Sep 9, 2015

Hi @jbourassa, I think this topic is also addressed here: #11036, #11082, #16531, #18983.

Seems like statements with unscoped (or unscope) + preloading are broken in AR4+.

@jbourassa

This comment has been minimized.

Show comment
Hide comment
@jbourassa

jbourassa Sep 9, 2015

Contributor

@markets Thanks for the heads up. I think they're all still valid issues albeit somewhat outdated. Part of #16531 (this) has already been implemented, and this PR builds on top of that change.

Contributor

jbourassa commented Sep 9, 2015

@markets Thanks for the heads up. I think they're all still valid issues albeit somewhat outdated. Part of #16531 (this) has already been implemented, and this PR builds on top of that change.

Show outdated Hide outdated ...ord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
:class_name => "LazyBlockDeveloperCalledDavid",
:join_table => "developers_projects",
:foreign_key => "project_id",
:association_foreign_key => "developer_id"

This comment has been minimized.

@eileencodes

eileencodes Sep 9, 2015

Member

Thanks for the PR! πŸ˜„ New code added to Rails should use the new hash syntax. foreign_key: "project_id"

@eileencodes

eileencodes Sep 9, 2015

Member

Thanks for the PR! πŸ˜„ New code added to Rails should use the new hash syntax. foreign_key: "project_id"

This comment has been minimized.

@jbourassa

jbourassa Sep 9, 2015

Contributor

Thank you. I updated the PR accordingly.

@jbourassa

jbourassa Sep 9, 2015

Contributor

Thank you. I updated the PR accordingly.

@markets

This comment has been minimized.

Show comment
Hide comment
@markets

markets Sep 9, 2015

Yes, they are still valid in latest 4.2-stable (not tried in master). I, particularly, want to fetch objects via associations like this:

has_manny :comments, -> { unscoped } # fetch comments by-passing the default_scope

But I didn't found a way to achieve that. So, finally I wrote a gem (unscoped_associations), it works for has_many, has_one, belongs_to; but it causes some issues (N+1) on preloading an unscoped association. I also see similar issues in other gems extending/patching AR (zendesk/predictive_load#6, salsify/goldiloader#13), so would be really nice to have this merged it in πŸ‘ and have a clear way to fetch objects via associations by-passing the default_scope.

markets commented Sep 9, 2015

Yes, they are still valid in latest 4.2-stable (not tried in master). I, particularly, want to fetch objects via associations like this:

has_manny :comments, -> { unscoped } # fetch comments by-passing the default_scope

But I didn't found a way to achieve that. So, finally I wrote a gem (unscoped_associations), it works for has_many, has_one, belongs_to; but it causes some issues (N+1) on preloading an unscoped association. I also see similar issues in other gems extending/patching AR (zendesk/predictive_load#6, salsify/goldiloader#13), so would be really nice to have this merged it in πŸ‘ and have a clear way to fetch objects via associations by-passing the default_scope.

@jbourassa

This comment has been minimized.

Show comment
Hide comment
@jbourassa

jbourassa Sep 22, 2015

Contributor

@eileencodes Sorry to ping you but I was expecting to hear from @rails-bot, but it's not happening. Any idea what needs to be done to get this in? Thanks!

Contributor

jbourassa commented Sep 22, 2015

@eileencodes Sorry to ping you but I was expecting to hear from @rails-bot, but it's not happening. Any idea what needs to be done to get this in? Thanks!

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Sep 24, 2015

Member

Thanks. I added a changelog entry for you as part of the merge commit, please make sure to add one when fixing bugs in the future.

Member

sgrif commented Sep 24, 2015

Thanks. I added a changelog entry for you as part of the merge commit, please make sure to add one when fixing bugs in the future.

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Sep 24, 2015

Member

Also if you notice that after you update a PR that Travis hasn't picked it up, you can help us out by clicking close and re-open which should force it.

Member

sgrif commented Sep 24, 2015

Also if you notice that after you update a PR that Travis hasn't picked it up, you can help us out by clicking close and re-open which should force it.

@sgrif sgrif merged commit d25321b into rails:master Sep 24, 2015

sgrif added a commit that referenced this pull request Sep 24, 2015

Merge pull request #21550 from didacte/unscope-associations
ActiveRecord: use association's `unscope` when preloading

sgrif added a commit that referenced this pull request Sep 24, 2015

Merge pull request #21550 from didacte/unscope-associations
ActiveRecord: use association's `unscope` when preloading
@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Sep 24, 2015

Member

Backported to 4.2 in 338894c

Member

sgrif commented Sep 24, 2015

Backported to 4.2 in 338894c

@jbourassa

This comment has been minimized.

Show comment
Hide comment
@jbourassa

jbourassa Sep 24, 2015

Contributor

πŸŽ‰ awesome, thank you very much @sgrif.

I'll make sure to edit the changelog next time.

Contributor

jbourassa commented Sep 24, 2015

πŸŽ‰ awesome, thank you very much @sgrif.

I'll make sure to edit the changelog next time.

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Sep 24, 2015

Member

Thanks for the great patch!

Member

sgrif commented Sep 24, 2015

Thanks for the great patch!

@GuyPaddock

This comment has been minimized.

Show comment
Hide comment
@GuyPaddock

GuyPaddock Oct 6, 2015

I'm using this patch but this still doesn't work:

belongs_to :membership, -> { unscoped }

We have a soft-deleted loyalty membership that is normally hidden by the default scope (deleted_at has to be nil). The only workaround at the moment is for us to explicitly specify:

belongs_to :membership, -> { unscope(where: :deleted_at) }

But this couples the referencing object to the column in the membership model, when we would prefer to do:

belongs_to :membership, -> { with_deleted }

GuyPaddock commented Oct 6, 2015

I'm using this patch but this still doesn't work:

belongs_to :membership, -> { unscoped }

We have a soft-deleted loyalty membership that is normally hidden by the default scope (deleted_at has to be nil). The only workaround at the moment is for us to explicitly specify:

belongs_to :membership, -> { unscope(where: :deleted_at) }

But this couples the referencing object to the column in the membership model, when we would prefer to do:

belongs_to :membership, -> { with_deleted }
@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Oct 6, 2015

Member

@GuyPaddock Please create a script we can use to reproduce the issue using this template, and open a new issue.

Member

sgrif commented Oct 6, 2015

@GuyPaddock Please create a script we can use to reproduce the issue using this template, and open a new issue.

@philipgiuliani

This comment has been minimized.

Show comment
Hide comment
@philipgiuliani

philipgiuliani Nov 3, 2015

@GuyPaddock have you found a workaround for unscoping the asosciation? I updated to rails 4.2.5.rc1 but i still wasn't able to do this.

philipgiuliani commented Nov 3, 2015

@GuyPaddock have you found a workaround for unscoping the asosciation? I updated to rails 4.2.5.rc1 but i still wasn't able to do this.

@kashifnaseer

This comment has been minimized.

Show comment
Hide comment
@kashifnaseer

kashifnaseer Mar 27, 2016

its still an issue in Rails 4.2.4. Any suggestion, how i can get this working in Rails 4.2.4 ?
As it is mentioned above that it has been backported to 4.2, should it not work with 4.2.4 ?

kashifnaseer commented Mar 27, 2016

its still an issue in Rails 4.2.4. Any suggestion, how i can get this working in Rails 4.2.4 ?
As it is mentioned above that it has been backported to 4.2, should it not work with 4.2.4 ?

@jbourassa

This comment has been minimized.

Show comment
Hide comment
@jbourassa

jbourassa Mar 28, 2016

Contributor

@kashifnaseer this has been released in 4.2.5, so it will not work in 4.2.4.

This may not work with belongs_to associations, as reported by @GuyPaddock β€” I'm not sure which code path is being used for belongs_tos.

Contributor

jbourassa commented Mar 28, 2016

@kashifnaseer this has been released in 4.2.5, so it will not work in 4.2.4.

This may not work with belongs_to associations, as reported by @GuyPaddock β€” I'm not sure which code path is being used for belongs_tos.

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