makandra / active_type

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

Default values are sharing the same object instances #186

Closed makmic closed 7 months ago

makmic commented 8 months ago

I was recently surprised by some code like this :

class Frog < ActiveType::Object
  attribute :flies, default: []
end

first_frog = Frog.new
second_frog = Frog.new
first_frog.flies << 'Some Fly'
second_frog.flies # => ['Some Fly']

It's quite clear that both Frog instances are sharing the same reference to an Array. active_type offers a solution to the problem ("use default: proc { [] }") but I'd suggest to fix this specific use case in the library. Setting default: [] seems to work fine at first and can break apps in very unexpected ways much later on.

Corresponding source code:

module ActiveType
  module VirtualAttributes
    module Serialization
    class VirtualColumn

      def default_value(object)
        default = @options[:default]
        default.respond_to?(:call) ? object.instance_eval(&default) : default # could be `default.dup` to solve this issue
      end

WDYT?

kratob commented 8 months ago

I'm a bit hesitant. Unnecessary duping does cost performance (although not a lot); and the behavior is in line with for example "has_defaults".

Would it help if we printed a warning if you pass an unfrozen object to default:?

makmic commented 8 months ago

Hi @kratob, being warned against this edge case would definitely be sufficient!

kratob commented 7 months ago

Went one step further and simply deprecated passing unfrozen objects, so you will get a warning. Released as 2.5.0.