makandra / active_type

Make any Ruby object quack like ActiveRecord
MIT License
1.09k stars 74 forks source link

`cast` function in `ActiveRecord::Base` not copying attributes with `attr_accessor` #174

Closed MaximilianoGarciaRoe closed 9 months ago

MaximilianoGarciaRoe commented 1 year ago

When using the cast function with ActiveRecord::Base in conjunction with a model that has attr_accessor defined, the values of the attr_accessor attributes are not persisted in the new object

Example

Class Foo < ActiveRecord::Base
 attr_accessor :bar
End

a = Foo.create()
a.bar = 'foo'

b = ActiveCast(a, Foo)
b.bar
# >> nil

During my local testing, I added ActiveRecord::Baseand defined a method thus

module ActiveType
  module Util
    class ActiveRecord::Base
      def copy_attr_accessor(record) end
    end
  end
end

Class Foo < ActiveRecord::Base
 attr_accessor :bar
 def copy_attr_accessor(record)
  bar = record.bar
 end
End

and add to cast_record the following

casted.copy_attr_accessor(record)

This worked and does what I need, however,this solution I'm currently failing a test at './spec/isolated/require_does_not_trigger_ar_load_spec.rb:7'. Do you have any plans in the future for something similar? If so, I can submit a Pull Request to show what I've accomplished and we can work together to find a solution.

Greetings and thank you for your time

kratob commented 1 year ago

Hi @MaximilianoGarciaRoe,

So, as a "workaround", you could simply use attribute :bar instead of attr_accessor :bar and have the attribute managed by ActiveType and survive a cast, or am I missing some subtlety here?

I think it might be a bit awkward to also add support for attr-accessors, since the only way to detect them is by looking at the existence of instance variables. That might work, but seems a bit dangerous to me, since I'm not sure copying every instance variable is always correct.

If there is no strong reason to have attr_accessor instead of attribute, I think I'd prefer to mention this behavior as a caveat in the Readme, instead of handling it.

Current behavior is also consistent with ActiveRecord#becomes though that might not be a strong argument.

MaximilianoGarciaRoe commented 1 year ago

Thank you for the response, I understand the point about attr_reader. However, would it be possible to have an empty method that calls the cast function, which I can then override? Alternatively, could we use an event system or a proc to apply business logic to the ActiveType?"

Example in cast method

def cast(record)
  # logic method

  casted.business_logic(record)

  # continue method
end

and so

Class Foo < ActiveRecord::Base
  def business_logic(record)
    # business logic 
  end
End
# >> nil

Now I have implemented it with a mocky patch, but it feels very hacky. I hope my point was understood. Thank you for your time.

kratob commented 1 year ago

I think that is a decent proposal. If you want to make a pull request for that I'd probably accept it.

I would suggest calling it after_cast, but otherwise make it work like your business_logic example above.