Mass assignment attributes change when backwards association uses subclass that sets self._accessible_attributes[:default] #7442

Closed
garysweaver opened this Issue Aug 24, 2012 · 3 comments

Comments

Projects
None yet
3 participants
Contributor

garysweaver commented Aug 24, 2012

In Rails 3.2.6 (need to see if is problem in latest), we have models similar to:

class Library < ActiveRecord::Base
  attr_accessible :name, :address, :phone_number
  has_many :books
end

class LibraryRef < Library
  self.table_name = :libraries

  # redefine accessible_attributes (messy way to set- it should have a better way to redefine)
  self._accessible_attributes[:default] = [:name]
end

class Book < ActiveRecord::Base
  attr_accessible :title, :isbn, :checked_out
  belongs_to :library, primary_key: :library_id, class_name: 'LibraryRef'
end

And when referring to Library before querying on Book, :name, :address, :phone_number were mass assignable.

After querying on Book which returned LibraryRef, if we query on Library again, it acts as if it were a LibraryRef because its mass assigned attributes have been redefined.

This is confusing because the whole point to class_attribute (like self._accessible_attributes in this case) was that it supported subclassing, I thought.

I've not had time to check this out outside of our app, so it might not be an issue, and it may have been fixed or in another ticket already- I looked but didn't see it.

Let me know if this looks familiar or you can easily reproduce. Otherwise, I'll try to confirm it outside of our application as soon as I have time.

In the meantime, this is the workaround:

module LibraryShared
  extend ActiveSupport::Concern

  included do
    attr_accessible :name, :address, :phone_number
    has_many :books
  end

  module ClassMethods
  end
end

class Library < ActiveRecord::Base
  include LibraryShared
end

class LibraryRef < Library
  include LibraryShared

  self.table_name = :libraries

  # redefine accessible_attributes (messy way to set- it should have a better way to redefine)
  self._accessible_attributes[:default] = [:name]
end

class Book < ActiveRecord::Base
  attr_accessible :title, :isbn, :checked_out
  belongs_to :library, primary_key: :library_id, class_name: 'LibraryRef'
end

Thanks!

Member

steveklabnik commented Sep 21, 2012

This kinda hurts my head. Messing with internals (ie, self._accessible_attributes) is not generally good practice. I'm not sure that this is the kind of thing we actually want to support.

@jonleighton @tenderlove thoughts?

I believe you should just call attr_accessible instead of using _accessible_attributes, as @steveklabnik said.

In any case, class_attribute does work for subclasses, but the thing is that it needs to be duped every time it's set. In your case, you're updating the hash reference:

self._accessible_attributes[:default] = [:name]

Instead you should be setting a duped version:

self._accessible_attributes = self.accessible_attributes.merge(:default => :name)

This would not override the parent class definition. But again, it'd be way better to not mess up with _accessible_attributes if possible :).

I'm giving this a close, please let us know if you find anything else. Thanks!

(Btw, I'm not even sure that merge thing I sent will work, since the implementation is not that simple, but you can take a look at the real implementation of attr_accessible).

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