Rails 3 validation of presence of Time - strange behavior #6045

Closed
Loremaster opened this Issue Apr 29, 2012 · 6 comments

Projects

None yet

4 participants

Contributor

I want to check that Time can not be empty, so i use for that presence helper:

#  id              :integer         not null, primary key
#  school_class_id :integer
#  meeting_time    :time
class Meeting < ActiveRecord::Base                                                    
  validates :meeting_time,
              :presence => { :message => "can't be empty!" }                                                            
end

Then, i check what will happen if time will contain string with few spaces only:

#  id              :integer         not null, primary key
#  school_class_id :integer
#  meeting_time    :time

require 'spec_helper'

describe Meeting do
  before(:each) do
    @class = FactoryGirl.create( :school_class )
    @attr_meeting = {
      :meeting_theme => 'Text is here',
      :meeting_date => "#{Date.today}",
      :meeting_time => "#{Time.now}",
      :meeting_room => '1234'
    }
  end

  describe "Validations" do
    describe "Rejection" do
      it "should reject blank time" do
        wrong_attr = @attr_meeting.merge( :meeting_time => "  " )
        @class.meetings.build( wrong_attr ).should_not be_valid
      end
    end
  end
end

And this fails. As for me this is very strange behavior. When i use same method to validate strings it doesn't allow strings with spaces.

Contributor
flexoid commented Apr 29, 2012

I fully agree. I think that creating valid Time object from blank string (with spaces) and nil from empty string is quite illogical.

Member

I'm not 100% sure that I'd consider this a bug, but I at least verified that it's not a factory girl problem.

The reason this happens is that PresenceValidator uses errors.add_on_blank which delegates to blank?.

Here's what's interesting:

1.9.3-p125 :001 > m = Meeting.new
 => #<Meeting id: nil, meeting_time: nil, created_at: nil, updated_at: nil> 
1.9.3-p125 :002 > m.meeting_time = " "
 => " " 
1.9.3-p125 :003 > m.valid?
 => true 
1.9.3-p125 :004 > " ".blank?
 => true 
1.9.3-p125 :005 > m.meeting_time.blank?
 => false 
1.9.3-p125 :006 > m.meeting_time
 => 2000-01-01 00:00:00 UTC 

So actually, it's getting turned into a Time somewhere. How odd...

Member

Furthermore, it's not the validations code that's causing the problem.

1.9.3-p125 :007 > m = Meeting.new
 => #<Meeting id: nil, meeting_time: nil, created_at: nil, updated_at: nil> 
1.9.3-p125 :008 > m.meeting_time = " "
 => " " 
1.9.3-p125 :009 > m.meeting_time
 => 2000-01-01 00:00:00 UTC 
Contributor
flexoid commented Apr 29, 2012

I think, it should be return nil if string.blank? instead of return nil if string.empty?. I can handle it, if you agree.

# activerecord/lib/active_record/connection_adapters/column.rb:177

def string_to_dummy_time(string)
  return string unless string.is_a?(String)
  return nil if string.empty?

  string_to_time "2000-01-01 #{string}"
end
# activerecord/lib/active_record/connection_adapters/column.rb:170

def string_to_time(string)
  return string unless string.is_a?(String)
  return nil if string.empty?

  fast_string_to_time(string) || fallback_string_to_time(string)
end
# activerecord/lib/active_record/connection_adapters/column.rb:89

# Casts value (which is a String) to an appropriate instance.
def type_cast(value)
  return nil if value.nil?
  return coder.load(value) if encoded?

  klass = self.class

  case type
  when :string, :text        then value
  when :integer              then value.to_i rescue value ? 1 : 0
  when :float                then value.to_f
  when :decimal              then klass.value_to_decimal(value)
  when :datetime, :timestamp then klass.string_to_time(value)
  when :time                 then klass.string_to_dummy_time(value)
  when :date                 then klass.value_to_date(value)
  when :binary               then klass.binary_to_string(value)
  when :boolean              then klass.value_to_boolean(value)
  else value
  end
end
Member

Personally, I think that's right, yeah. @tenderlove @jonleighton what do you think?

@flexoid flexoid added a commit to flexoid/rails that referenced this issue May 5, 2012
@flexoid flexoid Prevent creating valid time-like objects from blank string from db
Issue #6045
7f160b0
Member
vijaydev commented May 5, 2012

Closing since the PR is merged.

@vijaydev vijaydev closed this May 5, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment