#317 ✓resolved
Dylan Markow

Saving Boolean fields with false values fails validations

Reported by Dylan Markow | May 27th, 2008 @ 08:09 PM

Given the class:

class Course
  include DataMapper::Resource
  include DataMapper::Timestamp
  property :id, Integer, :key => true, :serial => true
  property :completed, Boolean, :default => false
  property :title, String, :length => 255
end

I would expect that this code would return true:

c = Course.new
c.valid?

However, I get this:

c = Course.new
# <Course completed=false id=nil title=nil>
c.valid?
# false
c.errors
# <DataMapper::Validate::ValidationErrors:0x169f720 @errors={:completed=>["Completed must not be blank"]}>

If I change my model to have ":default => true" the validation passes fine.

Running 0.9.0.1 of all the dm-core and dm-more stuff with a Postgres DB.

Comments and changes to this ticket

  • Dylan Markow

    Dylan Markow May 27th, 2008 @ 08:19 PM

    On a side note, turning auto_validation off for this property fixes the problem.

    After looking at auto_validate.rb and , Boolean fields appear to automatically undergo a "validates_present" check. The validates_present check appears to just call the #blank? method for the property unless I'm mistaken. However, even though "false" is a valid value for a Boolean property, "false.blank?" evaluates to "true" thus rejecting the validation. Looks like the "offending" code is in the blank.rb file for dm-core:

    class FalseClass
      # False is always blank.
      def blank?
        true
      end
    end # class FalseClass
    
  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) May 27th, 2008 @ 08:49 PM

    • State changed from “new” to “open”

    I think the reason for this bug is that DataMapper::Validate::RequiredFieldValidator, which is being triggered behind the scenes, is using value.blank? to check to see if the value is present or not. The unfortunate thing is that FalseClass (false) has blank? set to return false.. meaning that a false value isn't being interpreted as being present.

    The fix is to explicitly use value.nil? in the code rather than value.blank?.

    Dylan, if you want to submit a patch with specs to fix this I can apply it, otherwise I will add this to my TODO list and should be able to get to it within the next week or so.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) May 27th, 2008 @ 08:50 PM

    I should also mention that DataMapper::Validate::AbsentFieldValidator suffers from the same bug.

  • Dylan Markow

    Dylan Markow May 27th, 2008 @ 11:42 PM

    Dan, I poked around in the code but couldn't figure out how to get this going. I'll study the internals some more and try to get a patch together if I can. I'm assuming that we only want to use value.nil? on Boolean properties and still use .blank? on the others. Otherwise, an empty string will pass the validation because it's not nil. Not sure how to determine the type of the property from the validation code, though.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) May 28th, 2008 @ 12:46 AM

    • Milestone cleared.

    Dylan, yeah I think you're right: We'd only want to use nil? on boolean properties, and blank? for everything else.

    It should be possible to introspect the property from the resources' model, but if you can't figure it out a simple approach would be to use logic like "if value.blank? && !value.kind_of?(FalseClass)" and if it returned true, then the field is blank, otherwise it's not.

  • Dylan Markow

    Dylan Markow May 29th, 2008 @ 09:31 PM

    Okay, managed to figure out how to introspect the property (using target.class.properties[field_name].type to know when we're dealing with Boolean properties.

    Attaching a patch w- specs to fix this for both AbsentFieldValidator and RequiredFieldValidator

    (Sorry in advance if I didn't do this patch properly, I'm a git-patch-virgin)

  • Dylan Markow
  • Ben Erickson

    Ben Erickson May 31st, 2008 @ 04:31 PM

    That patch didn't seem to work for me.

    target_property_type == DataMapper::Types::Boolean always came back as false. So I slightly altered Dylan's changes to have a more specific conditional. Altered patch is attached.

  • Ben Erickson

    Ben Erickson May 31st, 2008 @ 04:33 PM

    Side note, looks like I left a little unused code in there. Sorry about that.

  • Sam Smoot
  • Max Aller

    Max Aller June 6th, 2008 @ 12:57 PM

    Ben, I tried applying your patch (by hand) and now ALL Boolean fields fail validation, whether they're actually true or not. Haven't had a chance to try TrueClass/FalseClass. Additionally, other fields are breaking as well (String fields, for instance). Something still isn't quite right.

  • Max Aller

    Max Aller June 6th, 2008 @ 01:14 PM

    Disregard previous message, I made a mistake in applying the patch.

  • Max Aller

    Max Aller June 6th, 2008 @ 01:20 PM

    Having said that, I've tested Ben's patch and it seems to do the trick.

  • Ben Erickson

    Ben Erickson June 6th, 2008 @ 04:55 PM

    On Fri, Jun 6, 2008 at 1:20 PM, Lighthouse

    wrote:

  • Ben Erickson

    Ben Erickson June 6th, 2008 @ 05:00 PM

    That's strange. I ran the specs and all seemed well. I'll try to investigate

    a bit more this weekend.

    On Fri, Jun 6, 2008 at 12:57 PM, Lighthouse

    wrote:

  • Max Aller

    Max Aller June 6th, 2008 @ 05:43 PM

    Yeah, uh, you can ignore my previous comments. I tried using git apply to apply the patch and that didn't work on my local repo, and I don't know how to git merge, so I did manual copy/paste from your patch, since there wasn't much code to change. I didn't see that the "required" validator had "unless" instead of "if" though! --> retarded. So, yeah, it seems to be working just fine for me. I haven't tried this yet, but should this patch work not only for types FalseClass and TrueClass, but also Boolean? Sorry for the confusion.

  • Ben Erickson

    Ben Erickson June 6th, 2008 @ 06:19 PM

    Ya I messed up on the email reply. Sent it before I read your other messages. Glad it's working.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) June 9th, 2008 @ 08:13 PM

    • State changed from “open” to “resolved”

    This problem should be fixed by the following commit:

    http://github.com/sam/dm-more/co...

    I'm marking this as resolved, but if this doesn't solve your issue please add a comment and we'll re-open the ticket.

  • Max Aller

    Max Aller June 11th, 2008 @ 01:14 AM

    Using merbful_authentication, when I try to create a user (register) I get the following error:

    undefined method `primitive' for nil:NilClass

    in required_field_validator.rb:43

    Last few lines of stack trace: http://pastie.org/212822

  • Max Aller

    Max Aller June 15th, 2008 @ 03:00 PM

    Apparently it was fixed recently, as I'm no longer getting the error.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

People watching this ticket

Pages