solnic / virtus

[DISCONTINUED ] Attributes on Steroids for Plain Old Ruby Objects
MIT License
3.77k stars 228 forks source link

Custom coercers don't respect `strict` setting #293

Open ravicious opened 9 years ago

ravicious commented 9 years ago

Custom coercers don't raise any errors if they fail to coerce a value to a given primitive.

class FunkyInteger < Virtus::Attribute
  primitive Integer

  def coerce(value)
    'not an integer'
  end
end

class CoolObject
  include Virtus.model(strict: true)

  attribute :funky_integers, Array['FunkyInteger']
end

my_cool_object = CoolObject.new funky_integers: ['23', '12']
#=> #<CoolObject:0xbc4842b8 @funky_integers=['not an integer', 'not an integer']>

It doesn't matter if strict is set on the whole model or just a specific attribute. Might be related to issue #284.

Tested under Virtus 1.0.1 and 1.0.3.

solnic commented 9 years ago

I guess this should be treated as a "gotcha" as this logic is implemented in the coerce method that you override :(

I will address this in virtus 2.0

justfalter commented 9 years ago

FWIW, this only seems to be an issue when the array member type is defined lazily (in quotes). Under 1.0.3, if I change the following line: attribute :funky_integers, Array['FunkyInteger'] to attribute :funky_integers, Array[FunkyInteger] the code raises the expected coercion error.

Add Virtus.finalize does not fix the issue, either.

solnic commented 9 years ago

OK I'll try to fix it in 1.x.y

solnic commented 9 years ago

OH and thanks for the spec <3

ravicious commented 9 years ago

@justfalter Under 1.0.1 and 1.0.3, it does not raise the error for me:

class PositiveFloatWithOneDecimalPoint < Virtus::Attribute
  primitive Float

  def coerce(value)
    if value.respond_to?(:to_f)
      value = value.to_f
      raise ArgumentError, "Value has to be greater than zero" unless value > 0
      value.round(1)
    end
  end
end

class Parcel
  include Virtus.model(strict: true)

  attribute :width, PositiveFloatWithOneDecimalPoint
  attribute :length, PositiveFloatWithOneDecimalPoint
  attribute :height, PositiveFloatWithOneDecimalPoint
  attribute :weight, PositiveFloatWithOneDecimalPoint
end

Parcel.new(weight: 2, height: 2, width: 2, length: [])
#=> #<Parcel:0xbd6ac788 @weight=2.0, @height=2.0, @width=2.0, @length=nil>
justfalter commented 9 years ago

@ravicious Your coerce function returns nil unless the object responds to '#to_f'. This isn't the problem, but I feel like I need to point that out for the sake of clarity.

The issue is that the attribute's primitive is showing up as BasicObject:

Parcel.attribute_set[:width].primitive
=> BasicObject

If you trace the execution of the code, you can see that virtus first calls your coerce method and then checks to see if the value was properly coerced by calling the attribute's '#value_coerced?(value)' method. All that does is check to see if the value is of the attribute's primitive type. The problem is that Virtus is using BasicObject as the primitive, rather than Float. Since all objects descend from BasicObject, virtus always thinks that it has done its job properly.

I'll quote a comment comment @elskwid made here:

the Virtus::Attribute.primitive interface can get finicky (unstable) when you use a primitive for a built-in primitive, something like Date, String, etc. The lookup inside Virtus gets confused and you end up with the BasicObject that you noted.

He goes on to explain that one can use a custom coercer can be used to work around this issue. This might help when you're coercing to a primitive, but it does not work for other objects as evidenced by #284.

solnic commented 9 years ago

Here's how to do it properly to work with virtus 1.0.x: https://gist.github.com/solnic/9b9deedd7891e2c7ac83

ravicious commented 7 years ago

Sorry for resurrecting an old issue.

FYI, if you decide to use the above solution, please read my comment under the gist to be aware of its caveats. You may not need to call the primitive method within the custom attribute class body.