Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Scope is applied in place where it should not (with STI) #13466

Closed
astudnev opened this Issue Dec 24, 2013 · 17 comments

Comments

Projects
None yet
10 participants

After upgrading from 4.0.0 to 4.0.2 some of the scopes behave incorrectly IMHO

For example, if i have class hierarchy:

class User < ActiveRecord::Base
  belongs_to :role
end

class Term < ActiveRecord::Base
end

class Role < Term
end

class ProductCategory < Term

   belongs_to :realm, class_name: 'Role'

   scope :view_as_realm, -> {
        where realm: user.role

    }

end

Problem is that the code in the scope

user.role

is executed incorrectly,
when executing user.realm , Rails adds the scope to the SQL Query, like

SELECT `terms`.* FROM `terms` WHERE `terms`.`type` IN ('ProductCategory') AND `terms`.`type` IN ('Role') AND `terms`.`id` = 193 ORDER BY `terms`.`id` ASC LIMIT 1

which makes no sense (as it added WHERE terms.type IN ('ProductCategory') to querying absolutely other model !)

to overcome the problem i had to modify the code like:

unscoped {
   user.role    
 }

and then it works as expected

This appears in 4.0.2, 4.0.0 works correctly with this

Member

robin850 commented Dec 25, 2013

Hello,

Thanks for reporting! Could you please provide an executable gist to showcase this problem ? I think you didn't provide enough information since the ProductCategory model can't access any user instance ; I tried to reproduce through a gist but got the following error when executing the scope:

undefined local variable or method `user' for ProductCategory(id: integer):Class
# Activate the gem you are reporting the issue against.
gem 'activerecord', '4.0.2'
require 'active_record'
require 'minitest/autorun'
require 'logger'

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do

  create_table :terms do |t|
    t.belongs_to :realm
    t.string :type   # STI
  end

  create_table :users do |t|
    t.belongs_to :role
  end

end

class User < ActiveRecord::Base
  belongs_to :role
end

class Term < ActiveRecord::Base
end

class Role < Term
end

class ProductCategory < Term

  belongs_to :realm, class_name: 'Role'

  scope :view_as_realm, -> (user) {
    where realm_id: user.role.id
  }

end

class BugTest < Minitest::Unit::TestCase

  def test_association_stuff

    role = Role.create!
    user = User.create! role: role

    pc = ProductCategory.create! realm: role

    assert_equal role, User.first.role
    assert_equal 1, ProductCategory.view_as_realm(User.first).count

  end


end
Member

prathamesh-sonpatki commented Jan 8, 2014

@astudnev This fails for me on Rails 4.0 also

@tenderlove tenderlove referenced this issue Feb 8, 2014

@jonleighton jonleighton Fix scope chaining + STI
See #9869 and #9929.

The problem arises from the following example:

    class Project < ActiveRecord::Base
      scope :completed, -> { where completed: true }
    end

    class MajorProject < Project
    end

When calling:

    MajorProject.where(tasks_count: 10).completed

This expands to:

    MajorProject.where(tasks_count: 10).scoping {
      MajorProject.completed
    }

However the lambda for the `completed` scope is defined on Project. This
means that when it is called, `self` is Project rather than
MajorProject. So it expands to:

    MajorProject.where(tasks_count: 10).scoping {
      Project.where(completed: true)
    }

Since the scoping was applied on MajorProject, and not Project, this
fails to apply the tasks_count condition.

The solution is to make scoping apply across STI classes. I am slightly
concerned about the possible side-effects of this, but no tests fail and
it seems ok. I guess we'll see.
8606a7f
Owner

tenderlove commented Feb 8, 2014

This was broken in 8606a7f

Contributor

jefflai2 commented Feb 8, 2014

working on this

Owner

matthewd commented Feb 10, 2014

@tenderlove is #14003 (comment) relevant to our discussion of expected behaviour here?

@jefflai2 jefflai2 referenced this issue in friends-of-rails/rails Feb 10, 2014

Closed

Scope Evaluation Rework #4

Owner

tenderlove commented Feb 10, 2014

@matthewd ya, it looks like #14003 is a duplicate of this bug. Check this comment though. @rafaelfranca understands this is a bug. :-)

Contributor

dnagir commented Feb 10, 2014

If #14003 is a duplicate then it should not be reproducible in Rails 4.0.0 as this issue isn't.
But the #14003 happens in all currently published Rails 4 versions. So it may not necessarily be an exact duplicate.

Owner

pixeltrix commented Feb 10, 2014

@dnagir this happens in 4.0.0 as well - the attached test case fails in 4.0.0 but passes in 3.2.16

Contributor

dnagir commented Feb 10, 2014

@pixeltrix cool then. Just wanted to bring the attention based on the description of this issue

After upgrading from 4.0.0 to 4.0.2 some of the scopes behave incorrectly

but I missed the other comment #13466 (comment)

@jefflai2 jefflai2 pushed a commit to jefflai2/rails that referenced this issue Feb 10, 2014

Jefferson Lai Added test for issue #13466 7b52410

@jefflai2 jefflai2 pushed a commit to jefflai2/rails that referenced this issue Feb 25, 2014

Jefferson Lai Fixes Issue #13466.
Changed the call to a scope block to be evaluated with instance_eval.
The result is that ScopeRegistry can use the actual class instead of
base_class when caching scopes.
669e1da

@jefflai2 jefflai2 pushed a commit to jefflai2/rails that referenced this issue Feb 25, 2014

Jefferson Lai Fixes Issue #13466.
Changed the call to a scope block to be evaluated with instance_eval.
The result is that ScopeRegistry can use the actual class instead of base_class when
caching scopes so queries made by classes with a common ancestor won't leak scopes.
7fab9da

@jefflai2 jefflai2 pushed a commit to jefflai2/rails that referenced this issue Feb 25, 2014

Jefferson Lai Fixes Issue #13466.
Changed the call to a scope block to be evaluated with instance_eval.
The result is that ScopeRegistry can use the actual class instead of base_class when
caching scopes so queries made by classes with a common ancestor won't leak scopes.
6ce9b64

@jefflai2 jefflai2 pushed a commit to jefflai2/rails that referenced this issue Mar 26, 2014

Jefferson Lai Fixes Issue #13466.
Changed the call to a scope block to be evaluated with instance_eval.
The result is that ScopeRegistry can use the actual class instead of base_class when
caching scopes so queries made by classes with a common ancestor won't leak scopes.
4fd3005

@jefflai2 jefflai2 pushed a commit to jefflai2/rails that referenced this issue Mar 27, 2014

Jefferson Lai Fixes Issue #13466.
Changed the call to a scope block to be evaluated with instance_eval.
The result is that ScopeRegistry can use the actual class instead of base_class when
caching scopes so queries made by classes with a common ancestor won't leak scopes.
01a096e

@jefflai2 jefflai2 pushed a commit to jefflai2/rails that referenced this issue Mar 28, 2014

Jefferson Lai Fixes Issue #13466.
Changed the call to a scope block to be evaluated with instance_eval.
The result is that ScopeRegistry can use the actual class instead of base_class when
caching scopes so queries made by classes with a common ancestor won't leak scopes.
2068876

@jefflai2 jefflai2 added a commit to jefflai2/rails that referenced this issue Apr 24, 2014

@jefflai2 jefflai2 Fixes Issue #13466.
Changed the call to a scope block to be evaluated with instance_eval.
The result is that ScopeRegistry can use the actual class instead of base_class when
caching scopes so queries made by classes with a common ancestor won't leak scopes.
9c3afdc

@rafaelfranca rafaelfranca added this to the 4.0.6 milestone May 21, 2014

Owner

rafaelfranca commented May 21, 2014

Closed by #14544

@rafaelfranca rafaelfranca added a commit that referenced this issue May 21, 2014

@rafaelfranca rafaelfranca Merge pull request #14544 from jefflai2/named_scope_sti
Fixes Issue #13466.

Conflicts:
	activerecord/CHANGELOG.md
9a1abed

@rafaelfranca rafaelfranca added a commit that referenced this issue May 21, 2014

@rafaelfranca rafaelfranca Merge pull request #14544 from jefflai2/named_scope_sti
Fixes Issue #13466.

Conflicts:
	activerecord/CHANGELOG.md

Conflicts:
	activerecord/CHANGELOG.md
eed1e25

@rafaelfranca rafaelfranca added a commit that referenced this issue May 21, 2014

@rafaelfranca rafaelfranca Merge pull request #14544 from jefflai2/named_scope_sti
Fixes Issue #13466.

Conflicts:
	activerecord/CHANGELOG.md

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/scoping/named.rb
807ee0a
Owner

rafaelfranca commented May 21, 2014

#14544 break integration with activerecord-deprecated_finders so I'm reverting to not delay the release.

@rafaelfranca rafaelfranca reopened this May 21, 2014

@rafaelfranca rafaelfranca modified the milestones: 4.0.7, 4.0.6 May 21, 2014

Contributor

jefflai2 commented May 24, 2014

Any details about what was broken? Also, since this merge is in the history already, if I have a fix, should I make a new branch + PR?

Owner

rafaelfranca commented May 24, 2014

@jefflai2 you can see the failures at the 4-0-stable branch. See this one for example https://travis-ci.org/rails/rails/builds/25684252

@jefflai2 jefflai2 added a commit to jefflai2/rails that referenced this issue May 26, 2014

@rafaelfranca @jefflai2 rafaelfranca + jefflai2 Merge pull request #14544 from jefflai2/named_scope_sti
Fixes Issue #13466.

Conflicts:
	activerecord/CHANGELOG.md

Conflicts:
	activerecord/CHANGELOG.md
2d160f0

@jefflai2 jefflai2 added a commit to jefflai2/rails that referenced this issue May 26, 2014

@jefflai2 jefflai2 Updated #14544 to work with `activerecord-deprecated_finders`
Simply used older style of lambda definitions within a test.
Fixes Issue #13466. See #14544 for complete description.
d033f8f

@jefflai2 jefflai2 added a commit to jefflai2/rails that referenced this issue May 27, 2014

@jefflai2 jefflai2 Updated #14544 to work with `activerecord-deprecated_finders`
Simply used older style of lambda definitions within a test.
Fixes Issue #13466. See #14544 for complete description.

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/scoping/named.rb
4752c14

@jefflai2 jefflai2 added a commit to jefflai2/rails that referenced this issue Jun 1, 2014

@jefflai2 jefflai2 Updated #14544 to work with `activerecord-deprecated_finders`
Simply used older style of lambda definitions within a test.
Fixes Issue #13466. See #14544 for complete description.

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/scoping/named.rb
f103ab3

@jefflai2 jefflai2 added a commit to jefflai2/rails that referenced this issue Jun 10, 2014

@jefflai2 jefflai2 Updated #14544 to work with `activerecord-deprecated_finders`
Used older style of lambda definitions within a test. Also explicitly check for
the case when `activerecord-deprecated_finders` is active in the context of scopes
and handles this case specially. This handling relies on and is meant to be used
in unison with rails/activerecord-deprecated_finders#23.

Fixes Issue #13466. See #14544 for complete description.
e62d470

@jefflai2 jefflai2 added a commit to jefflai2/rails that referenced this issue Jun 10, 2014

@jefflai2 jefflai2 Updated #14544 to work with `activerecord-deprecated_finders`
Used older style of lambda definitions within a test. Also explicitly check for
the case when `activerecord-deprecated_finders` is active in the context of scopes
and handles this case specially. This handling relies on and is meant to be used
in unison with rails/activerecord-deprecated_finders#23.

Fixes Issue #13466. See #14544 for complete description.
b5aafd1

@rafaelfranca rafaelfranca modified the milestones: 4.0.10, 4.1.7 Aug 21, 2014

@rafaelfranca rafaelfranca modified the milestones: 4.1.9, 4.0.13 Nov 19, 2014

@jefflai2 any update on this?

Contributor

jefflai2 commented Nov 27, 2014

I'm having trouble recreating my environment and unfortunately don't have the bandwidth to update the changes. Someone else may want to approach this issue again starting with a fresh pull from stable.

@rafaelfranca rafaelfranca modified the milestones: 4.0.13, 4.1.9 Jan 2, 2015

Owner

rafaelfranca commented Feb 20, 2015

This is fixed at master branch.

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