Model.unscoped block not removing default scope for joins #13775

Closed
aprescott opened this Issue Jan 20, 2014 · 38 comments

Comments

Projects
None yet
Contributor

aprescott commented Jan 20, 2014

Summary:

Suppose Model has a default scope.

Model.unscoped { Foo.joins(:model) }

acts as if Model.unscoped { ... } were not specified. That is, it constructs Foo.joins(:model) as if it weren't inside Model.unscoped, so that Model.default_scope is mistakenly continuing to apply to the joins, even though unscoped should have removed it.


Consider a User with a default_scope that specifies status = 1:

>> User.all.to_sql
#=> "SELECT `users`.* FROM `users`  WHERE `users`.`status` = 1"

If an Item belongs to User, trying to join the two with Item.joins(:user) seems to always respect User.default_scope, no matter how you try to remove the default scope. So the joins SQL

>> Item.joins(:user).to_sql # includes User.default_scope, CORRECTLY.
#=> "SELECT `items`.* FROM `items` INNER JOIN `users` ON `users`.`id` = `items`.`user_id` AND `users`.`status` = 1

correctly includes

`users`.`status` = 1

as it should, but if you try to remove it with User.unscoped { ... }, the status = 1 default scope on User is still applying, which I believe is a bug:

User.unscoped do
  Item.joins(:user) # User is incorrectly still scoped to status == 1 here
end

As a total aside, it would be nice to have something like joins_unscoped(:user) as an alternative.

Owner

rafaelfranca commented Jan 20, 2014

Which version are you using? Have you tried the master branch?

Contributor

aprescott commented Jan 20, 2014

Rails 4.0.2. I haven't tried on master but I can get around to checking.

Contributor

aprescott commented Jan 21, 2014

@themilkman that looks like the same problem, yes.

pallan commented Jan 21, 2014

If it helps I whipped up a example failing test using the bug report template (https://github.com/rails/rails/blob/master/guides/bug_report_templates/active_record_master.rb) against master https://gist.github.com/pallan/8542994

Member

vipulnsward commented Feb 9, 2014

This seems more like a feature request to me.

Owner

rafaelfranca commented Feb 9, 2014

Agree.

Contributor

aprescott commented Feb 9, 2014

Why was this closed? unscoped not being respected seems like a bug to me, even if it might read like a feature request (e.g., "provide" in the title). In either case, why close it?

Member

vipulnsward commented Feb 9, 2014

@aprescott lets discuss this on Rails-core Mailing List, I see a new mechanism needs to be implemented to handle unscoped joins, and depends of what kind of a response it receives from everyone.

Maybe a new method on relation,

Model.join_unscoped(:table_name)

to achieve the end results. It would be better to get a feedback from everyone before working on something like this.

Contributor

aprescott commented Feb 9, 2014

Model.unscoped being ignored is a bug to me, but if the answer to dealing with it is to flesh out a desired extra feature request for join_unscoped then that's a little disappointing. I'll try and find time to put an email together so as to not simply point to GitHub — where the discussion already sits.

Owner

rafaelfranca commented Feb 9, 2014

Wait. Model.unscoped should not be ignored. This is indeed a bug. Could you upgrade the description/title to look like more like a bug report and add more highlight to the fact of unscoped being ignored?

@rafaelfranca rafaelfranca reopened this Feb 9, 2014

Contributor

aprescott commented Feb 9, 2014

@rafaelfranca I've updated the title and description. Let me know if it's still not clear.

Owner

rafaelfranca commented Feb 9, 2014

It is very clear now. Thanks 👍

@rafaelfranca rafaelfranca self-assigned this Feb 9, 2014

Contributor

miguelgraz commented Feb 11, 2014

Thanks for the script @pallan!

Contributor

miguelgraz commented Feb 12, 2014

If you guys don't mind I'm trying to make a fix for this, will push for you to review as soon as possible.

Contributor

miguelgraz commented Feb 12, 2014

So considering the example given on the summary of the issue (where Item belongs to User, which has a default_scope), something like Item.joins(:user) should and is returning:

>> Item.joins(:user).to_sql
#=> "SELECT `items`.* FROM `items` INNER JOIN `users` ON `users`.`id` = `items`.`user_id` AND `users`.`status` = 1

correctly, right? Also something like Item.unscoped.joins(:user).to_sql should behave the same, since it is the Item which is being unscoped, right?

And the bug stands for

User.unscoped do
  Item.joins(:user) # User is still scoped to status == 1 here
end

which shouldn't add AND 'users'.'status' = 1 to the SQL generated.

Am I missing something or wrong somewhere here?

Owner

rafaelfranca commented Feb 12, 2014

@miguelgraz Yes. Exactly this

@rafaelfranca rafaelfranca removed their assignment Jul 15, 2014

@miguelgraz Were you able to make progress on this?

Contributor

miguelgraz commented Jul 15, 2014

@claptimes5 Unfortunately no, I've tried some different approaches some months ago but none of them worked as I wanted to and lately I didn't have the time to give it enough attention, sorry for that.

Contributor

miguelgraz commented Jul 17, 2014

If anyone wonder about this issue me and @claptimes5 are already working on it, we managed to write a test and, if we're doing everything right, the bug still seems to exist on master.

This is our base spec to work on, if you guys want to take a look and/or point something wrong: miguelgraz/rails@8cb2132

+1, looking forward to hear about any solutions for this

@aprescott @miguelgraz @rafaelfranca I just created the following #16531 that will allow one to say...

class User
  default_scope { where(status: 1) }
end

class Item
  belongs_to :user
  belongs_to :with_deleted_user, -> { unscope(where: :status) }, class_name: 'User', foreign_key: :user_Id
end

Item.joins(:with_deleted_user).to_sql
#=> "SELECT `items`.* FROM `items` INNER JOIN `users` ON `users`.`id` = `items`.`user_id`

OR

Item.joins(:user).to_sql
#=> "SELECT `items`.* FROM `items` INNER JOIN `users` ON `users`.`id` = `items`.`user_id` AND `users`.`status` = 1

Although, ActiveRecord::Base.unscoped { query_that_should_be_unscoped } still doesn't work (most likely due to the same reasons I mentioned in the PRs commit), I hope this helps until then.

Contributor

Bounga commented Oct 1, 2014

Here is an example failing test that reproduce exactly what @aprescott described: https://gist.github.com/e65c61fdebf76d050760

I tried to find a solution for a while but can't get any way to tell joins not to use the default scope when wrapped in Model.unscoped

I'm having similar problems with :includes. Neither passing -> { unscoped } to the association definition nor wrapping in Model.unscoped do .. end is respected.

Contributor

aprescott commented Mar 16, 2015

After some confusion from #18109, I have amended the description to better explain that User.unscoped { Item.joins(:user) } is ignoring User.unscoped as if it weren't present. My previous wording could have also suggested that Item.joins(:user) should always act as if it were called inside a User.unscoped { ... } block so that all joins calls ignored default scopes, which is not the aim of this issue and is something I'd consider broken.

@rails-bot rails-bot added the stale label Jun 26, 2015

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-2-stable, 4-1-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

I've been working on a new feature to potentially resolve this problem, as well as the other issues highlighted in #10643.

I accomplished it by introducing a new "inline scope," and also standardizing the order in which scopes are prioritized: 1) default, 2) association scope, 3) inline scope.

Say you have a has_many :comments association that applies a scope that hides soft-deletes via a :deleted_at attribute. You can undo the association scope with an inline scope like this:

    Post.includes(:comments => ->{ unscope(where: :deleted_at) })

Nested joins are possible within inline scopes. For the sake of clarity, these two lines are equivalent:

    Comment.joins(:author => :address)
    Comment.joins(:author => -> { joins(:address) })

Code here, still needs to be prepped for a PR but wanted to share first:

sidot3291/rails@1735537

Member

sgrif commented Jul 18, 2015

I'm not a fan of the lambda form. This shouldn't require new behavior or functionality. The issue is the lazy computation of the join path that we follow. Due to time constraints, this probably won't happen until Rails 5.1, but the goal is ultimately to require joins to be explicit (e.g. you must do User.joins(posts: :comments), not User.joins(:posts, :comments), which will allow the tree to be built eagerly instead fixing this issue.

Hi @sgrif

I'm not sure I understand your point about the tree being built eagerly. I think joins are already required to be explicit? In rails 4.1, User.joins(:posts, :comments) fails unless there is a :comments association on User. If a :comments association exists on both User and Post, the two examples here still produce different SQL.

Ultimately, what I want to see provided is granular control over scopes. I think default_scope has rightly received a lot of negative attention because it's so hard to alter once it's there. Rails 4.1 fixed some of those issues by allowing named scopes to override default scopes, but as this thread and #10643 highlight, we don't have an effective way to alter the scope when pulling in models by association.

I think we'll run into two issues with the strategy proposed in this thread, if it's the only solution we build:

  1. The solution applies scope to the Model, not to the association. And so, we are forced to assume that a scope is relevant to an association simply because the class matches. This may not be the desired effect when a user brings in multiple associations with the same class.
  2. Nesting quickly gets dirty. It's unclear to me how you could apply different scopes to :a, :b, :c in each of these cases: .joins(:a => {:b=>:c}), .joins(:a => [:b, :c]), .joins([:a, :b, :c])
Contributor

aprescott commented Jul 18, 2015

Even with the lambda functionality you described, I wouldn't consider that as fixing this issue. An unscoped acting as if it isn't there at all is still broken, lambda or no lambda.

Hmm, what is setting the expectation that the association should be unscoped in your example?

User.unscoped do
  Item.joins(:user) # User is incorrectly still scoped to status == 1 here
end

The association definition is:

class Item
  belongs_to :user, class_name: "User" #default class_name
end

So are you arguing that within the User.unscoped block, all references to User should return User.unscoped instead? If not, what is mechanism for the association working off of User.unscoped instead of just User?

Contributor

aprescott commented Jul 18, 2015

The definition of unscoped's block form tells you one thing, and User.unscoped { Item.joins(:user) } doesn't adhere to it, because it doesn't remove the default scope. This is already considered a legitimate bug (#13775 (comment) + others), and I think all the preceding discussion in this PR and other linked PRs expand on it a fair bit, so I won't rehash all that.

Turns out this was a bug for all scoping blocks, not just unscoped: http://apidock.com/rails/ActiveRecord/Relation/scoping

Just issued a PR: #20937

I still think we need more granular control over the scope of joins, and would be interested in some more conversation around the inline lambda idea. Does anyone have alternative ideas for altering scope at the association level?

@aprescott aprescott changed the title from Model.unscoped ignored for joins to Model.unscoped ignored for joins with default scope Jul 19, 2015

@aprescott aprescott changed the title from Model.unscoped ignored for joins with default scope to Model.unscoped block not removing default scope for joins Jul 19, 2015

@allenwq allenwq referenced this issue in Coursemology/coursemology2 Jul 20, 2015

Closed

`joins` are scoped under default_scope #436

andyrue commented Oct 20, 2015

Is there a suggested work around for this issue, or do I need to remove default_scope from my associated tables?

This is probably the worst bug in active record. It basically makes default scope completely unusable if you have any sort of complexity in your business logic. I wish default_scope came with a warning about this issue.

The only workaround I've found is to do the join in raw sql.

@arthurnn arthurnn self-assigned this Nov 12, 2015

Against my better judgement, I've been using a monkey patch that builds in my lambda-scope functionality for rails 4.1

https://gist.github.com/sidot3291/77b999ffeaa5d1d290b2

YMMV - solved my problems.

Running into this one, too. We've got some workarounds in places, but it's all pretty kludgey. Would be great to get it fixed in rails proper.

Contributor

tawan commented Jan 30, 2016

Hi, CodeTriage assigned this issue to me.

I read all comments carefully and came to the conclusion that PR #18109 solves exactly the problem that the author of the issue reported.

I try to summarise where the current behaviour deviates from the expected behaviour promised by the API:
Partial documentation of ActiveRecord::Base.unscoped

    # Returns a scope for the model without the previously set scopes.
    #   class Post < ActiveRecord::Base
    #     def self.default_scope
    #       where(published: true)
    #     end
    #   end
    # ...
    # This method also accepts a block. All queries inside the block will
    # not use the previously set scopes.
    #
    #   Post.unscoped {
    #     Post.limit(10) # Fires "SELECT * FROM posts LIMIT 10"
    #   }

It says that all queries inside the block will not use the previously set scopes. Here I agree with the author of the issue and I would expect that this also includes queries composed of joins with the AR model in question. All previously set scopes, including a default scope, should have no effect at all within the given block.

This method is realised by invoking ActiveRecord::Relation#scoping on a new plain vanilla relation object of the AR model (which does not include a default scope).

Documentation of ActiveRecord::Relation#scoping

# Scope all queries to the current scope.
#
#   Comment.where(post_id: 1).scoping do
#     Comment.first
#   end
#   # => SELECT "comments".* FROM "comments" WHERE "comments"."post_id" = 1 ORDER BY "comments"."id" ASC LIMIT 1
#
# Please check unscoped if you want to remove all previous scopes (including
# the default_scope) during the execution of a block.

Here the documentation says again that all queries within the given block are scoped to the current scope (in our case it's a clear new scope). The method achieves that by registering itself as the current scope for the AR model in question. It does this via ActiveRecord::Scoping::ScopeRegistry.set_value_for.

The API is currently not honoured when the join constraints are constructed without checking the ScopeRegistry for a potential :current_scope of the AR model.

klass.send(:build_default_scope, relation)

PR #18109 fixes this as expected, IMO.

I hope this summary helps in understanding the issue and the solution.

@sgrif sgrif closed this in #18109 Feb 11, 2016

sgrif added a commit that referenced this issue Feb 11, 2016

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