Boolean validation fails on false values #109

Closed
ajselvig opened this Issue Dec 19, 2014 · 9 comments

Comments

Projects
None yet
2 participants
@ajselvig
Contributor

ajselvig commented Dec 19, 2014

I think the title pretty much explains it. I can't believe I haven't seen this one before...

Complete code to reproduce below.

require 'nobrainer'

NoBrainer.configure do |config|
  config.rethinkdb_url          = 'rethinkdb://localhost/nobrainer_playground'
  config.warn_on_active_record  = true
  config.auto_create_databases  = true
  config.auto_create_tables     = true
end

class Foo
  include NoBrainer::Document

  field :bar, type: Boolean, required: true
end

f = Foo.new bar: false
f.save

Produces:

/Users/andy/.rvm/gems/ruby-2.1.0/gems/nobrainer-0.20.0/lib/no_brainer/document/persistance.rb:103:in `save': #<Foo id: "1N53vUk1EtRRbQ"> is invalid: Bar can't be blank (NoBrainer::Error::DocumentInvalid)
    from bool_validation.rb:17:in `<main>'
@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Dec 19, 2014

Contributor

Oh dear, never mind. This looks like a bug with ActiveModel! I just assumed it was NoBrainer because, well, way more people use ActiveModel.

class Foo2
  include ActiveModel::Validations

  attr_accessor :bar
  validates :bar, presence: true
end

f2 = Foo2.new
f2.bar = false
puts "f2 valid: #{f2.valid?}"
f2.errors.each do |attr, error|
  puts "Error on #{attr}: #{error}"
end

Prints:

f2 valid: false
Error on bar: can't be blank
Contributor

ajselvig commented Dec 19, 2014

Oh dear, never mind. This looks like a bug with ActiveModel! I just assumed it was NoBrainer because, well, way more people use ActiveModel.

class Foo2
  include ActiveModel::Validations

  attr_accessor :bar
  validates :bar, presence: true
end

f2 = Foo2.new
f2.bar = false
puts "f2 valid: #{f2.valid?}"
f2.errors.each do |attr, error|
  puts "Error on #{attr}: #{error}"
end

Prints:

f2 valid: false
Error on bar: can't be blank

@ajselvig ajselvig closed this Dec 19, 2014

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Dec 19, 2014

Contributor

For anyone curious, they're aware of this behavior and apparently okay with it:

rails/rails#6953

Seems very unintuitive to me. Anyway, sorry for the spam.

Contributor

ajselvig commented Dec 19, 2014

For anyone curious, they're aware of this behavior and apparently okay with it:

rails/rails#6953

Seems very unintuitive to me. Anyway, sorry for the spam.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Dec 20, 2014

Owner

If you had no type specified (or just Object), should false pass the validation of a required value?

Owner

nviennot commented Dec 20, 2014

If you had no type specified (or just Object), should false pass the validation of a required value?

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Dec 20, 2014

Contributor

Should it? I believe so. To me, a not present value is either nil or "never been set according to whatever internal attribute system is being used". This would apply regardless of whether there's a type associated with the field. This is obviously this is a matter of opinion, though, and the Rails guys don't share mine :)

Contributor

ajselvig commented Dec 20, 2014

Should it? I believe so. To me, a not present value is either nil or "never been set according to whatever internal attribute system is being used". This would apply regardless of whether there's a type associated with the field. This is obviously this is a matter of opinion, though, and the Rails guys don't share mine :)

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Dec 20, 2014

Owner

What about the empty string? or an empty array?

Owner

nviennot commented Dec 20, 2014

What about the empty string? or an empty array?

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Dec 20, 2014

Contributor

I would use the same argument there. Isn't that what length validations are for?

http://guides.rubyonrails.org/active_record_validations.html#length

Contributor

ajselvig commented Dec 20, 2014

I would use the same argument there. Isn't that what length validations are for?

http://guides.rubyonrails.org/active_record_validations.html#length

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Dec 20, 2014

Contributor

The ActiveModel docs are clear about empty strings being not present, and I guess this is a reasonable behavior if you know about it. Not sure off the top of my head how it handles arrays.

They do also explicitly tell you to use in: [true, false] for booleans, so at least they provide a path forward. I'm not sure it makes sense to change how NoBrainer behaves for either case.

Contributor

ajselvig commented Dec 20, 2014

The ActiveModel docs are clear about empty strings being not present, and I guess this is a reasonable behavior if you know about it. Not sure off the top of my head how it handles arrays.

They do also explicitly tell you to use in: [true, false] for booleans, so at least they provide a path forward. I'm not sure it makes sense to change how NoBrainer behaves for either case.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Dec 20, 2014

Owner

Ok so let's add a defined validator, and have required use the defined validator.
I'm not so happy about the "defined" validator name. Maybe "existence" would be better? I don't want to reuse presence.

This validator would raise errors when the value is nil or undefined.

Any chance you could take a stab at it?

Owner

nviennot commented Dec 20, 2014

Ok so let's add a defined validator, and have required use the defined validator.
I'm not so happy about the "defined" validator name. Maybe "existence" would be better? I don't want to reuse presence.

This validator would raise errors when the value is nil or undefined.

Any chance you could take a stab at it?

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig Dec 20, 2014

Contributor

Yeah, I can do that. Can't think of a better name than "defined" right now.

On Fri, Dec 19, 2014 at 6:31 PM, Nicolas Viennot notifications@github.com
wrote:

Ok so let's add a defined validator, and have required use the defined
validator.
I'm not so happy about the "defined" validator name. Maybe "existence"
would be better? I don't want to reuse presence.

This validator would raise errors when the value is nil or undefined.

Any chance you could take a stab at it?


Reply to this email directly or view it on GitHub
#109 (comment).

Contributor

ajselvig commented Dec 20, 2014

Yeah, I can do that. Can't think of a better name than "defined" right now.

On Fri, Dec 19, 2014 at 6:31 PM, Nicolas Viennot notifications@github.com
wrote:

Ok so let's add a defined validator, and have required use the defined
validator.
I'm not so happy about the "defined" validator name. Maybe "existence"
would be better? I don't want to reuse presence.

This validator would raise errors when the value is nil or undefined.

Any chance you could take a stab at it?


Reply to this email directly or view it on GitHub
#109 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment