add strict mode nullify

[gucki]

Having an attribute of ex. type Float and passing an empty string sets the atribute to that empty string. This is the default, but imo completely unexpeced. Now, when strict mode is enabled an exception is raised. I'd like to propose another strict mode :nullify which doesn't raise but sets the attribute to nil'.:strict_mode => trueshould be deprecated and replaced with:strict_mode => :raise`.

So to sum up: false -> invalid input is just passed along :raise -> raise on invalid input :nulliy -> set the attribute to nil on invalid input

This can be very handy fo other types like Boolean as well. Just consider implementing a Boolean using a selectbox with a prompt. (no selection -> nil, yes -> true, no -> false) instead of currently (no selection -> "", ...).

What do you think?

[solnic]

Actually I think that it'd be better to have a coercion mode that coerces empty string to nil. This would eliminate the need for complicating strict mode. I actually do want to see an exception when something that is supposed to be coercible is not and empty strings are just a common case specific to web-apps and it would be handy to nullify them automatically.

We could have something like:

Params = Virtus.model(nullify_blank: true)

class UserParams
  include Params
  attribute :birthday, Date
end

UserParams.new(birthday: "").birthday # => nil

[gucki]

Imo such exceptions make only sense in dev mode. In production I'd silently coerce them to nil and possibly log them for later inspection. Otherwise you might get flooded with exceptions caused by some script kiddies, easily overloading your app if you send exceptions by email (as a lot of people are doing).

Empty strings should always be coerced to nil, unless the attribute is of type string (just pass it along in this case). Do you have any usecase where this could be problematic?

From a security point of view, unintentionally returning the original value is bad. So the default should be raise in dev mode and nullify in all other envs.

Another additional idea: allow the user to set strict_mode to a block which gets called when coercion fails. The return value of the block is used as the attribute's new value. This way everbody is free to decide what should happen (raise, nullify, log and nullify, ...).

[lucasmazza]

The nullify_blank would be a nice touch to deal with user input with Virtus.

@solnic can I put up a PR with the implementation?

[solnic]

@lucasmazza yes this would be very useful

[lucasmazza]

alright, I'll look into this this week.

[gucki]

Sorry to come back to this now, but I still feel we need the strict mode of nullify:

class Test
  include Virtus.model(:nullify_blank => true)

  attribute :birthday, Date
end

Test.new(:birthday => "bla").birthday # => "bla"

This is completely counter intuitive and might open the door for security issues easily. An attribute should never be of any other type than defined or nil.

[gucki]

An example why the current options don't fit.

Without strict mode the user can pass anything through, which is really bad:

class Test
  include Virtus.model(:nullify_blank => true)

  attribute :birthday, Date
end

Test.new(:birthday => "bla").birthday # => "bla"

With strict mode enabled, it fails on invalid input, which is expected and great:

class Test
  include Virtus.model(:nullify_blank => true, :strict => true)

  attribute :birthday, Date
end

Test.new(:birthday => "bla").birthday # => "bla"
=> Virtus::CoercionError: Failed to coerce attribute `birthday' from "bla" into Date

However with strict mode enabled, it's not possible to have optional attributes. This is because "" or nil are also treated as a coercion errors, which is really bad:

class Test
  include Virtus.model(:nullify_blank => true, :strict => true)

  attribute :birthday, Date
end

Test.new(:birthday => "").birthday
=> Virtus::CoercionError: Failed to coerce attribute `birthday' from nil into Date

[solnic]

That's a great example why having a general purpose coercion/attribute handler is not a good idea. I've tried to keep virtus small but with so many concerns handled in one layer it is not possible and confusing from both development pov as well as usage pov.

For now I think keeping current behavior is fine because we need to have original input in order to make it work nicely with validations.

You can always adjust settings to your use cases if you want but the default is sensible, although in general I am not happy with it hah!

I'll be working on a better solution to this problem and probably virtus won't be part of it.

Coercions and validations are related but they are separate concerns. On Fri 24 Jul 2015 at 22:37 Corin Langosch notifications@github.com wrote:

An example why the current options don't fit.

Without strict mode the user can pass anything through, which is really bad:

class Test include Virtus.model(:nullify_blank => true)

attribute :birthday, Date end

Test.new(:birthday => "bla").birthday # => "bla"

With strict mode enabled, it fails on invalid input, which is expected and great:

class Test include Virtus.model(:nullify_blank => true, :strict => true)

attribute :birthday, Date end

Test.new(:birthday => "bla").birthday # => "bla" => Virtus::CoercionError: Failed to coerce attribute `birthday' from "bla" into Date

However with strict mode enabled, it's not possible to have optional attributes. This is because "" or nil are also treated as a coercion errors, which is really bad:

class Test include Virtus.model(:nullify_blank => true, :strict => true)

attribute :birthday, Date end

Test.new(:birthday => "").birthday => Virtus::CoercionError: Failed to coerce attribute `birthday' from nil into Date

— Reply to this email directly or view it on GitHub https://github.com/solnic/virtus/issues/310#issuecomment-124714232.