Active Record: if an AR class is duplicated, records can be returned with incorrect class

[alexeymuranov]

Here is a strange issue with duplicating an AR class. The actual situation is more or less the following: i extend my models to be more friendly, like to know which fields to show and how to sort their records. These extensions are different in different controllers. To not worry about possible violations of Open/closed principle, i've decided to duplicate the model classes instead of inheriting from them, something like:

class UsersController < ApplicationController
  User = ::User.dup
  class User
  end
end

instead of

class UsersController < ApplicationController
  class User < ::User
  end
end

Then i started getting an error, apparently caused by a wrong model class being used, which depended on the order in which i visited pages of my application!

Here is a script to reproduce. I do not know if this should be considered a bug, but to me this behavior is strange:

gem 'activerecord', '3.2.7'
require 'active_record'
# require 'logger'

# Print out what version we're running
puts "Active Record #{ActiveRecord::VERSION::STRING}"

# ActiveRecord::Base.logger = Logger.new(STDOUT)

# Connect to an in-memory sqlite3 database (more on this in a moment)
ActiveRecord::Base.establish_connection( :adapter  => 'sqlite3',
                                         :database => ':memory:' )

# Create the minimal database schema necessary to reproduce the bug
ActiveRecord::Schema.define do
  create_table "users", :force => true do |t|
  end
end

# Create the minimal set of models to reproduce the bug
class User < ActiveRecord::Base
end

# Create some test data
User.create # XXX: this line is important

module M
  User = ::User.dup
  class User
  end
end

# Create some test data
User.create

# Reproduce the actual bug!
p M::User.first.class # => User (expected M::User)

Commenting or uncommenting the line marked with XXX changes the class of the record returned. I think it has to always be M::User.

Update: Same with clone instead of dup.

[steveklabnik]

i extend my models to be more friendly, like to know which fields to show and how to sort their records. These extensions are different in different controllers.

I don't mean to toot my own horn too much, but this is exactly what presenters/view models were created for: Draper, for example.

[frodsan]

@steveklabnik Is it me, or the link is broken?

[steveklabnik]

I'm an idiot and can't write markdown, apparently. Fixed!

[alexeymuranov]

@steveklabnik, but the current behavior of AR, doesn't it look weird?

[steveklabnik]

Honestly, this is so far outside of a usual use-case that I have no idea if ActiveRecord's behavior is weird or not. I would personally not let this code pass a code review, so I have no idea what the proper semantics are in this case.

[MSch]

@steveklabnik kinda off topic: Draper is nice for presenting models, but do you know of a lib that complements it by supporting roundtripping the presented data in a form?

[steveklabnik]

Oh, and

User = ::User.dup
class User
end

and

class User < ::User
end

are not even equivalent anyway. In one, you're duping a class, then opening it up, and in the other, you're making a new class that inherits from another class.

[steveklabnik]

@MSch you are correct, it is very off topic, but that isn't part of the presenter pattern, so it's outside of Draper's scope. What you want is an ActiveModel compliant class that contains references to several ActiveRecord models. So the answer to your question is 'ActiveModel'.

Further questions about this can be emailed to me, let's not derail this Issue.

[alexeymuranov]

@steveklabnik, this is exactly why i wanted to use the first and not the second. But after thinking i while, the second might do for me. This is not important. I was reporting the issue because the behavior looked weird to me, because i thought that a duplication should not break any class, ActiveRecord or not. (Or it should break it in some predictable way.)

[pixeltrix]

The reason the dup fails is the class instance variables caching things like arel_table, arel_engine and relation aren't reset when the class is duplicated. Personally I think you'd be better off doing it differently but adding this method to your User class will make it work:

class User < ActiveRecord::Base
  def self.initialize_copy(other)
    @relation = nil
    @arel_table = nil
    @arel_engine = nil
  end
end

There may be other variables that need resetting, but this seems to work on first glance. @jonleighton do we want to support this properly by adding an initialize_copy method to Active Record models?

[alexeymuranov]

@pixeltrix, i have just tried this with my script above, it didn't help.

[reinh]

Rails probably does a number of things on class instantiation that are not being duplicated by dup. Even if it were possible to fix this, I'm not at all sure that it's worth fixing. This is an anti-use case as far as I'm concerned.

[alexeymuranov]

I think the problem might be with metaclass (with class methods). Maybe it was a bad idea to experiment with this. If you do not see anything unusual, this issue can be closed, i think.

[jonleighton]

Yeah, I don't think we should support this in Active Record. It seems quite an avant-garde technique and I don't want the maintenance burden.

[pixeltrix]

@alexeymuranov hmm, that worked in the rails console - initialize_copy doesn't appear to be called when defined in your script. It is the correct explanation though - this script works:

gem 'activerecord', '3.2.7'
require 'active_record'

# Print out what version we're running
puts "Active Record #{ActiveRecord::VERSION::STRING}"

# ActiveRecord::Base.logger = Logger.new(STDOUT)

# Connect to an in-memory sqlite3 database (more on this in a moment)
ActiveRecord::Base.establish_connection( :adapter  => 'sqlite3',
                                         :database => ':memory:' )

# Create the minimal database schema necessary to reproduce the bug
ActiveRecord::Schema.define do
  create_table "users", :force => true do |t|
  end
end

# Create the minimal set of models to reproduce the bug
class User < ActiveRecord::Base
  def self.dup
    copy = super
    copy.reset_class_variables
    copy
  end

  def self.reset_class_variables
    @arel_table = nil
    @arel_engine = nil
    @relation = nil
  end
end

# Create some test data
User.create # XXX: this line is important

module M
  User = ::User.dup
end

# Create some test data
User.create

# Reproduce the actual bug!
p M::User.first.class

[alexeymuranov]

@pixeltrix, thanks for the explanation, it works. I will also think about Draper.