#reset_column_information (from active_record/base) does not work properly with inheritance #22057

Closed
common-nighthawk opened this Issue Oct 24, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@common-nighthawk

reset_column_information only works on the Model that calls the method, and not on all models that inherit from it. This seems like bad behavior, because if you are going out of your way to call #reset_column_information--then you want all models that changed to be up to date.

I created a small app that displays the problem here: https://github.com/common-nighthawk/rails-reset_column_information-bug

In summary, say I have two models--

class Animal < ActiveRecord::Base
end

class Bird < Animal
end

Both have 2 columns: id and type.

Then I run this migration--

add_column :animals, :greeting, :string
Animal.reset_column_information
Animal.find_by(type: nil).update_attributes(greeting: 'im an animal')
Animal.find_by(type: 'Bird').update_attributes(greeting: 'im a bird')

The Animal will have greeting 'im an animal' and the Bird will have greeting 'nil'.

I can get around this by calling #reset_column_information on all Models explicitly. But this feel clunky, because all models that inherit from the parent have changing columns--so I don't see why the method call should not update all.

@atul-shimpi

This comment has been minimized.

Show comment
Hide comment
@atul-shimpi

atul-shimpi Nov 5, 2015

Contributor

@rafaelfranca
The Root Cause is Reset column information is not getting called for subclasses.
Below is the fix for above issue.

File - activerecord-4.2.3/lib/active_record/attributes.rb

def reset_column_information 
  super
  clear_caches_calculated_from_columns
  # Change 1
  descendants.map(&:clear_caches_calculated_from_columns)
end

# Change 2 -(private to public)
public
def clear_caches_calculated_from_columns
   # no change in body
end

I have tested this with attached code and is working fine.
Let me know your comments regarding this approach.

Contributor

atul-shimpi commented Nov 5, 2015

@rafaelfranca
The Root Cause is Reset column information is not getting called for subclasses.
Below is the fix for above issue.

File - activerecord-4.2.3/lib/active_record/attributes.rb

def reset_column_information 
  super
  clear_caches_calculated_from_columns
  # Change 1
  descendants.map(&:clear_caches_calculated_from_columns)
end

# Change 2 -(private to public)
public
def clear_caches_calculated_from_columns
   # no change in body
end

I have tested this with attached code and is working fine.
Let me know your comments regarding this approach.

@common-nighthawk

This comment has been minimized.

Show comment
Hide comment
@common-nighthawk

common-nighthawk Nov 5, 2015

I don't know if this approach is better or worse, but I discovered this fix--

calling #reset_column_information recursively on the subclasses, with:
direct_descendants.each { |klass| klass.reset_column_information }

resulting in--

def reset_column_information
        connection.clear_cache!
        undefine_attribute_methods
        connection.schema_cache.clear_table_cache!(table_name)
        @arel_engine        = nil
        @column_names       = nil
        @column_types       = nil
        @content_columns    = nil
        @default_attributes = nil
        @inheritance_column = nil unless defined?(@explicit_inheritance_column) && @explicit_inheritance_column
        @relation           = nil
        direct_descendants.each { |klass| klass.reset_column_information }
      end

advantages would be: not needing to expose a method to the public api (keep private)
disadvantage might be: not getting at the root problem

would love thoughts from someone who knows better :) . happy to submit a PR.

I don't know if this approach is better or worse, but I discovered this fix--

calling #reset_column_information recursively on the subclasses, with:
direct_descendants.each { |klass| klass.reset_column_information }

resulting in--

def reset_column_information
        connection.clear_cache!
        undefine_attribute_methods
        connection.schema_cache.clear_table_cache!(table_name)
        @arel_engine        = nil
        @column_names       = nil
        @column_types       = nil
        @content_columns    = nil
        @default_attributes = nil
        @inheritance_column = nil unless defined?(@explicit_inheritance_column) && @explicit_inheritance_column
        @relation           = nil
        direct_descendants.each { |klass| klass.reset_column_information }
      end

advantages would be: not needing to expose a method to the public api (keep private)
disadvantage might be: not getting at the root problem

would love thoughts from someone who knows better :) . happy to submit a PR.

@atul-shimpi

This comment has been minimized.

Show comment
Hide comment
@atul-shimpi

atul-shimpi Nov 5, 2015

Contributor

This approach even clicked me.
However, in this approach below code will get called for each subclass.

connection.clear_cache!        
connection.schema_cache.clear_table_cache!(table_name)

This should get called only once for whole hierarchy since in STI, table is shared by all the classes in hierarchy. Hence clearing table cache should be one time activity.

Below code should get called for all the subclasses

def clear_caches_calculated_from_columns
  @attributes_builder = nil
  @column_names = nil
  @column_types = nil
  @columns = nil
  @columns_hash = nil
  @content_columns = nil
  @default_attributes = nil
  @persistable_attribute_names = nil
end
Contributor

atul-shimpi commented Nov 5, 2015

This approach even clicked me.
However, in this approach below code will get called for each subclass.

connection.clear_cache!        
connection.schema_cache.clear_table_cache!(table_name)

This should get called only once for whole hierarchy since in STI, table is shared by all the classes in hierarchy. Hence clearing table cache should be one time activity.

Below code should get called for all the subclasses

def clear_caches_calculated_from_columns
  @attributes_builder = nil
  @column_names = nil
  @column_types = nil
  @columns = nil
  @columns_hash = nil
  @content_columns = nil
  @default_attributes = nil
  @persistable_attribute_names = nil
end
@atul-shimpi

This comment has been minimized.

Show comment
Hide comment
@atul-shimpi

atul-shimpi Nov 7, 2015

Contributor

@senny
Can someone approve or suggest better approach for above?
We can go for PR then.

Contributor

atul-shimpi commented Nov 7, 2015

@senny
Can someone approve or suggest better approach for above?
We can go for PR then.

@sgrif sgrif closed this in 9deb6ab Nov 7, 2015

@common-nighthawk

This comment has been minimized.

Show comment
Hide comment
@common-nighthawk

common-nighthawk Nov 8, 2015

@sgrif: thanks for fixing this. always cool when opening an issue on a big open source project actually results in a quick patch.

i didn't realize #reset_column_information had recently been split to call the private method #reload_schema_from_cache.

change makes sense to me. thanks again. also--fan of the bike shed.

@sgrif: thanks for fixing this. always cool when opening an issue on a big open source project actually results in a quick patch.

i didn't realize #reset_column_information had recently been split to call the private method #reload_schema_from_cache.

change makes sense to me. thanks again. also--fan of the bike shed.

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