Methods created by ActiveRecord::Delegation.delegate_to_scoped_klass incorrectly responding to respond_to? calls #8080

Closed
contentfree opened this Issue Oct 31, 2012 · 3 comments

Projects

None yet

3 participants

@contentfree

Given:

class Transaction < ActiveRecord::Base
  scope :limited, limit(10)
  scope :recent, order('created_at desc')
end

class Subscriber < ActiveRecord::Base
  scope :limited, limit(10)
end

The following code will produce a NoMethodError even though respond_to? returns true:

relation = Transaction.limited
relation.recent #=> This creates the scoped method on ActiveRecord::Delegation module

relation = Subscriber.limited
relation.respond_to?(:recent) #=> true
relation.recent #=> NoMethodError

This problem cropped up for me while interacting with different models and their scopes across resourceful controllers where some of the resources don't have the exact same set of scopes.

The problem is caused by ActiveRecord::Delegation.delegate_to_scoped_klass creating a method on the ActiveRecord::Delegation module that points to the ActiveRecord::Relation's current @klass instead of the scoped class. This is a problem because, obviously, subsequent usages of different models that don't have the same scopes may say they support a method that they really don't.

@carlosantoniodasilva
Ruby on Rails member

Thanks for reporting. What's your Rails version please?

@nikitug nikitug added a commit to nikitug/rails that referenced this issue Nov 21, 2012
@nikitug nikitug Fix #8080 (wrong #respond_to? value for AR::Delegation).
Fix the case when `Relation#respond_to?` always return `true` for the methods delegated to `@klass` without checking for this method in `@klass`.

Before patch
------------

Topic.all.by_lifo              #=> Creates the scoped method
Post.all.respond_to?(:by_lifo) #=> true
Post.all.by_lifo               #=> NoMethodError

After patch
-----------

Topic.all.by_lifo              #=> Creates the scoped method
Post.all.respond_to?(:by_lifo) #=> false
afb9740
@jonleighton jonleighton added a commit to jonleighton/rails that referenced this issue Nov 23, 2012
@jonleighton jonleighton Use separate Relation subclasses for each AR class
At present, ActiveRecord::Delegation compiles delegation methods on a
global basis. The compiled methods apply to all subsequent Relation
instances. This creates several problems:

1) After Post.all.recent has been called, User.all.respond_to?(:recent)
   will be true, even if User.all.recent will actually raise an error due
   to no User.recent method existing. (See #8080.)

2) Depending on the AR class, the delegation should do different things.
   For example, if a Post.zip method exists, then Post.all.zip should call
   it. But this will then result in User.zip being called by a subsequent
   User.all.zip, even if User.zip does not exist. (There are various
   variants of this problem.)

We are creating these compiled delegations in order to avoid method
missing and to avoid repeating logic on each invocation.

One way of handling these issues is to add additional checks in various
places to ensure we're doing the "right thing". However, this makes the
compiled methods signficantly slower. In which case, there's almost no
point in avoiding method_missing at all. (See #8127 for a proposed
solution which takes this approach.)

This is an alternative approach which involves creating a subclass of
ActiveRecord::Relation for each AR class represented. So, with this
patch, Post.all.class != User.all.class. This means that the delegations
are compiled for and only apply to a single AR class. A compiled method
for Post.all will not be invoked from User.all.

This solves the above issues without incurring significant performance
penalties. It's designed to be relatively seamless, however the downside
is a bit of complexity and potentially confusion for a user who thinks
that Post.all and User.all should be instances of the same class.

Benchmark
---------

require 'active_record'
require 'benchmark/ips'

class Post < ActiveRecord::Base
  establish_connection adapter: 'sqlite3', database: ':memory:'
  connection.create_table :posts

  def self.omg
    :omg
  end
end

relation = Post.all

Benchmark.ips do |r|
  r.report('delegation')   { relation.omg }
  r.report('constructing') { Post.all }
end

Before
------

Calculating -------------------------------------
          delegation      4392 i/100ms
        constructing      4780 i/100ms
-------------------------------------------------
          delegation   144235.9 (±27.7%) i/s -     663192 in   5.038075s
        constructing   182015.5 (±21.2%) i/s -     850840 in   5.005364s

After
-----

Calculating -------------------------------------
          delegation      6677 i/100ms
        constructing      6260 i/100ms
-------------------------------------------------
          delegation   166828.2 (±34.2%) i/s -     754501 in   5.001430s
        constructing   116575.5 (±18.6%) i/s -     563400 in   5.036690s

Comments
--------

Bear in mind that the standard deviations in the above are huge, so we
can't compare the numbers too directly. However, we can conclude that
Relation construction has become a little slower (as we'd expect), but
not by a huge huge amount, and we can still construct a large number of
Relations quite quickly.
d15fb68
@jonleighton jonleighton added a commit that referenced this issue Nov 23, 2012
@jonleighton jonleighton Use separate Relation subclasses for each AR class
At present, ActiveRecord::Delegation compiles delegation methods on a
global basis. The compiled methods apply to all subsequent Relation
instances. This creates several problems:

1) After Post.all.recent has been called, User.all.respond_to?(:recent)
   will be true, even if User.all.recent will actually raise an error due
   to no User.recent method existing. (See #8080.)

2) Depending on the AR class, the delegation should do different things.
   For example, if a Post.zip method exists, then Post.all.zip should call
   it. But this will then result in User.zip being called by a subsequent
   User.all.zip, even if User.zip does not exist, when in fact
   User.all.zip should call User.all.to_a.zip. (There are various
   variants of this problem.)

We are creating these compiled delegations in order to avoid method
missing and to avoid repeating logic on each invocation.

One way of handling these issues is to add additional checks in various
places to ensure we're doing the "right thing". However, this makes the
compiled methods signficantly slower. In which case, there's almost no
point in avoiding method_missing at all. (See #8127 for a proposed
solution which takes this approach.)

This is an alternative approach which involves creating a subclass of
ActiveRecord::Relation for each AR class represented. So, with this
patch, Post.all.class != User.all.class. This means that the delegations
are compiled for and only apply to a single AR class. A compiled method
for Post.all will not be invoked from User.all.

This solves the above issues without incurring significant performance
penalties. It's designed to be relatively seamless, however the downside
is a bit of complexity and potentially confusion for a user who thinks
that Post.all and User.all should be instances of the same class.

Benchmark
---------

require 'active_record'
require 'benchmark/ips'

class Post < ActiveRecord::Base
  establish_connection adapter: 'sqlite3', database: ':memory:'
  connection.create_table :posts

  def self.omg
    :omg
  end
end

relation = Post.all

Benchmark.ips do |r|
  r.report('delegation')   { relation.omg }
  r.report('constructing') { Post.all }
end

Before
------

Calculating -------------------------------------
          delegation      4392 i/100ms
        constructing      4780 i/100ms
-------------------------------------------------
          delegation   144235.9 (±27.7%) i/s -     663192 in   5.038075s
        constructing   182015.5 (±21.2%) i/s -     850840 in   5.005364s

After
-----

Calculating -------------------------------------
          delegation      6677 i/100ms
        constructing      6260 i/100ms
-------------------------------------------------
          delegation   166828.2 (±34.2%) i/s -     754501 in   5.001430s
        constructing   116575.5 (±18.6%) i/s -     563400 in   5.036690s

Comments
--------

Bear in mind that the standard deviations in the above are huge, so we
can't compare the numbers too directly. However, we can conclude that
Relation construction has become a little slower (as we'd expect), but
not by a huge huge amount, and we can still construct a large number of
Relations quite quickly.
bceb623
@jonleighton jonleighton added a commit that referenced this issue Nov 30, 2012
@jonleighton jonleighton Use separate Relation subclasses for each AR class
At present, ActiveRecord::Delegation compiles delegation methods on a
global basis. The compiled methods apply to all subsequent Relation
instances. This creates several problems:

1) After Post.all.recent has been called, User.all.respond_to?(:recent)
   will be true, even if User.all.recent will actually raise an error due
   to no User.recent method existing. (See #8080.)

2) Depending on the AR class, the delegation should do different things.
   For example, if a Post.zip method exists, then Post.all.zip should call
   it. But this will then result in User.zip being called by a subsequent
   User.all.zip, even if User.zip does not exist, when in fact
   User.all.zip should call User.all.to_a.zip. (There are various
   variants of this problem.)

We are creating these compiled delegations in order to avoid method
missing and to avoid repeating logic on each invocation.

One way of handling these issues is to add additional checks in various
places to ensure we're doing the "right thing". However, this makes the
compiled methods signficantly slower. In which case, there's almost no
point in avoiding method_missing at all. (See #8127 for a proposed
solution which takes this approach.)

This is an alternative approach which involves creating a subclass of
ActiveRecord::Relation for each AR class represented. So, with this
patch, Post.all.class != User.all.class. This means that the delegations
are compiled for and only apply to a single AR class. A compiled method
for Post.all will not be invoked from User.all.

This solves the above issues without incurring significant performance
penalties. It's designed to be relatively seamless, however the downside
is a bit of complexity and potentially confusion for a user who thinks
that Post.all and User.all should be instances of the same class.

Benchmark
---------

require 'active_record'
require 'benchmark/ips'

class Post < ActiveRecord::Base
  establish_connection adapter: 'sqlite3', database: ':memory:'
  connection.create_table :posts

  def self.omg
    :omg
  end
end

relation = Post.all

Benchmark.ips do |r|
  r.report('delegation')   { relation.omg }
  r.report('constructing') { Post.all }
end

Before
------

Calculating -------------------------------------
          delegation      4392 i/100ms
        constructing      4780 i/100ms
-------------------------------------------------
          delegation   144235.9 (±27.7%) i/s -     663192 in   5.038075s
        constructing   182015.5 (±21.2%) i/s -     850840 in   5.005364s

After
-----

Calculating -------------------------------------
          delegation      6677 i/100ms
        constructing      6260 i/100ms
-------------------------------------------------
          delegation   166828.2 (±34.2%) i/s -     754501 in   5.001430s
        constructing   116575.5 (±18.6%) i/s -     563400 in   5.036690s

Comments
--------

Bear in mind that the standard deviations in the above are huge, so we
can't compare the numbers too directly. However, we can conclude that
Relation construction has become a little slower (as we'd expect), but
not by a huge huge amount, and we can still construct a large number of
Relations quite quickly.
64c53d7
@jonleighton
Ruby on Rails member

FIxed by 64c53d7

@ttosch ttosch pushed a commit that referenced this issue Jan 19, 2015
@jonleighton jonleighton Use separate Relation subclasses for each AR class
At present, ActiveRecord::Delegation compiles delegation methods on a
global basis. The compiled methods apply to all subsequent Relation
instances. This creates several problems:

1) After Post.all.recent has been called, User.all.respond_to?(:recent)
   will be true, even if User.all.recent will actually raise an error due
   to no User.recent method existing. (See #8080.)

2) Depending on the AR class, the delegation should do different things.
   For example, if a Post.zip method exists, then Post.all.zip should call
   it. But this will then result in User.zip being called by a subsequent
   User.all.zip, even if User.zip does not exist, when in fact
   User.all.zip should call User.all.to_a.zip. (There are various
   variants of this problem.)

We are creating these compiled delegations in order to avoid method
missing and to avoid repeating logic on each invocation.

One way of handling these issues is to add additional checks in various
places to ensure we're doing the "right thing". However, this makes the
compiled methods signficantly slower. In which case, there's almost no
point in avoiding method_missing at all. (See #8127 for a proposed
solution which takes this approach.)

This is an alternative approach which involves creating a subclass of
ActiveRecord::Relation for each AR class represented. So, with this
patch, Post.all.class != User.all.class. This means that the delegations
are compiled for and only apply to a single AR class. A compiled method
for Post.all will not be invoked from User.all.

This solves the above issues without incurring significant performance
penalties. It's designed to be relatively seamless, however the downside
is a bit of complexity and potentially confusion for a user who thinks
that Post.all and User.all should be instances of the same class.

Benchmark
---------

require 'active_record'
require 'benchmark/ips'

class Post < ActiveRecord::Base
  establish_connection adapter: 'sqlite3', database: ':memory:'
  connection.create_table :posts

  def self.omg
    :omg
  end
end

relation = Post.all

Benchmark.ips do |r|
  r.report('delegation')   { relation.omg }
  r.report('constructing') { Post.all }
end

Before
------

Calculating -------------------------------------
          delegation      4392 i/100ms
        constructing      4780 i/100ms
-------------------------------------------------
          delegation   144235.9 (±27.7%) i/s -     663192 in   5.038075s
        constructing   182015.5 (±21.2%) i/s -     850840 in   5.005364s

After
-----

Calculating -------------------------------------
          delegation      6677 i/100ms
        constructing      6260 i/100ms
-------------------------------------------------
          delegation   166828.2 (±34.2%) i/s -     754501 in   5.001430s
        constructing   116575.5 (±18.6%) i/s -     563400 in   5.036690s

Comments
--------

Bear in mind that the standard deviations in the above are huge, so we
can't compare the numbers too directly. However, we can conclude that
Relation construction has become a little slower (as we'd expect), but
not by a huge huge amount, and we can still construct a large number of
Relations quite quickly.
d0ad00a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment