[Rails] [SOLVED] time validation (the feb. 31st problem)

[Ara.T.Howard]

On Tue, 15 Mar 2005, Justin Forder wrote:

> It isn't correct input to a Date constructor.

but is IS correct input to a Time constructor - at that is what will be used
in most cases.  this is really important.

> These are good requirements.

check.

> Let me digress for a moment:
>
> Going back to "Four Days on Rails", there's a bit on page 16 (that's the
> number printed on the page; Acrobat Reader says "page 20 of 37") that says:

i haven't read that - but i have read the source ;-)

> - when a user types input for a numeric field, Rails will always convert it
> to a number - if all else fails, a zero. If you want to check that the user
> has actually typed in a number, then you need to validate the input
> _before_type_cast, which lets you access the ‘raw’ input.

fine.  sketch out a method of

   * providing a _before_type_cast patch for multi-parameters

   * validating them for times

i agree this needs to be done - it's just that the easiest, and most correct,
way i could think of is what i did.  if you want to validate time/data input i
think you'll end up writing something very much like what is already written
in the builtin time/date classes - and then someone will need to maintain it
;-)

> Now I haven't put in the fancy "New To Do" page yet, so I still have fields
> to type into for "Done", "Priority", and "Private".  If I type "fred" into
> the Priority field and submit the form, I get an error saying "Priority must
> be between 1 (high) and 5 (low)", the Priority field has a red box around
> it, and the value displayed is still "fred".
>
> The validation specified for the Priority attribute was just:
>
> validates_inclusion_of :priority, :in => 1..5, :message => "must be between 1 
> (high) and 5 (low)"
>
> - there was no check specified on :priority_before_type_cast.

ah.  yes.  but this is VERY, VERY differnet because

   * here ctor did NOT munge the original input values, it kept them because
     THEY WERE NOT A LIST

   * the test of original input values can be expressed in < 100 lines of code

> So what has happened here is that Rails treated the integer equivalent of
> "fred" as zero, and this failed the range inclusion check - but when the
> screen was redisplayed for correction, it still showed "fred".
>
> Surprisingly,
> irb(main):001:0> "fred".to_i
> => 0
>
> (I didn't expect this - this is unlike Date.new.)

again, this is different for several reasons:

   * frist, we could have exploded early using

     irb(main):001:0> Integer "fred"
     ArgumentError: invalid value for Integer: "fred"
       from (irb):1:in `Integer'
       from (irb):1

   * but we did not, instead we warped the value into '0'

   * now, later we can see if this is inside a simple range

how is this different from letting times be normalized, but noting that they
were (my patch) so that something can be later?

   * the time is accepted, it does not explode early - this bit is the same

   * we warp time into 'normalized' values (default system behaviour!), again
     this is the same

       "fred"      -> 0

       2000, 2, 31 -> 2000, 3, 2

     but a key point is that for "fred" the original string is kept and for
     times the input list is lost.  my patch remembers it.

   * for validation it breaks down - we cannot use a simple range test of any
   sort?  why?  because any test of year, month, day, etc. to be within a
   certain range will succeed : here there is no way to know that input values
   were outside of ranges UNLESS they were remembered (this is what my patch
   does).  furthermore it has precalculated whether they were outside of
   allowable ranges already - you only need ask.

> If we want dates to behave like the example above, when we find that the raw
> data won't make a valid date we can make any old date - the easiest (and the
> equivalent of 0 for integers) is just the result of Date.new. So long as we
> bounce the user back to the input screen, that date isn't going to be saved
> to the database.

yes - that is what the patch does?  and this isn't quite right - the value
won't be saved if, and only if, validation is requested.

> If we were to replicate the good and bad points of the integer behaviour, we
> could silently convert invalid dates to Date.new and require users that
> didn't want invalid dates to go into the database to validate that the
> converted date value was within a sensible range (or at least that the
> converted date value was not equal to Date.new):
>
>   validates_each :due_date do |record, attr|
>     record.errors.add attr, 'invalid date' if attr == Date.new
>   end

this is not right - validation occurs before data hits the database!  if
validation fails the data is not saved!  this is true both for you
integer/range example AND my patch for times.

> (Untested!)
>
> This relies on the fact that the result of Date.new is not a very useful
> date. (This assumption would be invalid if the application used this value
> as a "sentinel value".) Apart from this, I think it satisfies your criteria:
>
> * does not throw an exception
>
> * remembers original input parameters
>
> * makes it possible to determine if the inputs where not used the way the
> web developer intended (even if the actual use was 'legal')

but this already exactly what's happening?  the only thing you missed is that
all this happend before we hit disk?

> Hmmm... to generalise this, a brutal (but consistent) approach to the
> general multi-parameter assignment issue would be to require any class being
> constructed via multi-parameter assignment to also be constructable with no
> arguments. If construction with the given arguments raised an exception,
> construct with no arguments instead. Leave it to the caller to either
> validate the raw data (e.g. in the form I suggested yesterday) or the result
> (being able to discriminate the result of no-args construction from other
> values).

no argument constructors for safety is a very c++ way of thinging no?  again,
this is all totally moot unless you can provide a simply algorithm for users
to 'validate' time values.  in general i agree with you but for time objects i
do not.

> Is this making any sense? I don't like the silent conversion of invalid
> input, but if that's what Ruby does...

it is making sense - but i think you need to try my patch.  i really think it
already does what you want.  again, Ruby does NOT DO any silent convertion,
the underlying c call does however.  this is the crux of the problem - if you
do not allow Ruby to do the same you violate POLS, if you do you suprise web
developers.  the happy medium is to flag when 'developer pols' has been
violated, but not raise an exception, then the web developers themself can
decide what to to.  this is exactly what my patch does and, in general, what
validation sets out to do: validate AFTER form values have been mapped to
objects but BEFORE data hits disk.  my patch is totally consistent with this.

> One other point - in version 0.10.0, at least, validation failure on
> xxx_before_type_cast isn't putting the red box around the offending field. I
> should test on 0.10.1 before flagging this as a bug, but given the problems
> being reported with 0.10.1 I'm inclined to wait for 0.10.2.
>
> Can you point me to the place in the current code where a Date is
> constructed from the "multiple parameters"? I haven't dug that far in yet.

line 1298 of activerecord-1.8.0/lib/active_record/base.rb, looks like

   def extract_callstack_for_multiparameter_attributes(pairs)

if you haven't looked here yet the discussion is largely moot - i think you'll
see the difficulties if you try to create a patch after reading this code.

in any event i'm happy someone is thinking about this too - so please don't
take my objections as motivation to quit!  i'd just like to see the 'right
thing' done ;-)

kind regards.

-a
-- 
===============================================================================
| EMAIL   :: Ara [dot] T [dot] Howard [at] noaa [dot] gov
| PHONE   :: 303.497.6469
| When you do something, you should burn yourself completely, like a good
| bonfire, leaving no trace of yourself.  --Shunryu Suzuki
===============================================================================