virtus icon indicating copy to clipboard operation
virtus copied to clipboard

Distinguish empty arrays from an unset attribute

Open migu0 opened this issue 9 years ago • 13 comments

I have several forms that submit to a Virtus object (through a controller). Some forms contain an extras attribute, others don't.

I currently can't distinguish whether extras has been set to an empty array (i.e. the user deselects all checkboxes on the form) or whether extras has never been submitted. In either case, extras will be an empty array.

I need this distinction because if the user deselects all extras I need to update them. On the other hand, if extras were not part of the form (i.e. not in the params), I shouldn't update them.

    class UpdateForm
      include Virtus.model(nullify_blank: true)
      include ActiveModel::Model
      attribute :extras, Array
      attribute :booking_time, Time

      def save
        updatable_attributes = self.attributes.delete_if { |key, value| value.blank? }
        some_object.update(updatable_attributes)
      end
    end

How can I make Virtus to give me a nil on extras if I call it like this:

    UpdateForm.new(booking_time: Time.current)

Or is there a better pattern to do this?

migu0 avatar Aug 12 '15 01:08 migu0

You can do attribute :extras, Array, coerce: false so that nil won't be coerced to an empty array

solnic avatar Aug 12 '15 07:08 solnic

Thanks for your reply.

I tried that just now. So no extras were passed into the Virtus model, attribute :extras, Array, coerce: false is set like you said but if I call self.attributes I still get :extras=>[]

migu0 avatar Aug 12 '15 07:08 migu0

@michaelimstepf I'm having the same issue. Did you find a solution to this?

forest avatar Feb 12 '16 22:02 forest

@forest Sorry, pls ignore previous comment (deleted), I was referring to a different issue. Can you try this:

class UpdateForm
  include Virtus.model(nullify_blank: true)
  include ActiveModel::Model
  attribute :extras, Array
  attribute :booking_time, Time

  def initialize(opts={})
    super
    # http://stackoverflow.com/a/31959720/1076279      
    @extras = nil unless opts['extras'].present?
  end

  def save
    some_object.update(updatable_attributes)
  end
end

migu0 avatar Feb 13 '16 01:02 migu0

@michaelimstepf thanks for the reply. If I just needed this for one attribute on one class I'd go this route.

I opted for this https://gist.github.com/657fd558a9c05416cf81 temporary solution until #209 lands on master so I can create my own attribute extension.

It is odd to me that NullifyBlank only works on strings, but I wasn't sure if others would agree.

forest avatar Feb 13 '16 21:02 forest

Thanks for posting this

migu0 avatar Feb 14 '16 23:02 migu0

This (kind of) worked for me: attribute :extras, Array, coerce: false, default: nil

But ;-) If you look at the use case "I want to be able to distinguish nil from empty", I don't think any of the currently suggested solutions/workarounds are totally fitting:

  • With the solution above, you loose coercion functionality (i.e. you could set the attribute's value to types you might not want)
  • When overwriting the initialize method only works for initialization (obviously ;-), but you can't set the attribute to nil later on
  • Overwriting NullifyBlank like in https://gist.github.com/forest/657fd558a9c05416cf81 makes it impossible to set the attribute to an empty array (because it's blank it gets nullified)

I would expect that specifying required: false would also work on Array types in the way that I can assign both nil and an empty array to the attribute.

Since nil gets coerced to an empty Array by the coercer, maybe the code to adapt is Virtus::Attribute::Coercible#set, like this:

      def set(instance, value)
        coerced_value = value.nil? && !required? ? value : coerce(value)
        super(instance, coerced_value)
      end

(edit:) With this change, setting required: false implies that nil should never be coerced, regardless of the type, even if it could be.

I can make a PR and write some tests if that's an acceptable solution, what do you think?

mherold avatar Feb 29 '16 15:02 mherold

@mherold your analysis and solution sounds right. I knew changing NullifyBlank wasn't the right solution. :)

I'm happy to test with my use case when you push up the PR.

forest avatar Feb 29 '16 18:02 forest

Thanks @forest, I appreciate it!! PR is available.

mherold avatar Mar 01 '16 10:03 mherold

@mherold PR #354 covers my use case. I was able to remove my hack to NullifyBlank.

forest avatar Mar 03 '16 00:03 forest

@forest awesome, thanks for testing!

mherold avatar Mar 06 '16 18:03 mherold

@forest we have such hack in our app either. It would be great to have it merged.

ka8725 avatar Apr 29 '16 22:04 ka8725

Is it possible to fix this? is there any problem with @mherold PR #354 suggestion?

fguillen avatar Apr 06 '17 16:04 fguillen