Date Validator - remove the "required"ness

[Tim Haines]

Hi there,

I've updated the DateValidator locally so that null and empty are valid - under the separation of concerns philosophy those should be checked by the NonEmpty Validator.  This is also the way the Email Validator works, so best to keep them consistent.  Do people generally agree with this approach?

2 questions before I create a patch for it (as long as I don't get slammed ;-)

1) Should I be doing anything to protect people who have relied on it's NonEmpty check in an earlier version?

2) Are there some test cases somewhere that I should be updating to submit along with the patch?

Cheers,

Tim.

[Ken Egozi]

+1 for the separation.

[josh robb]

+1 - there's been a few conversations about NonEmpty before.

Backwards compat - no idea.

j.

[Hamilton Verissimo]

I think all validators have tests cases. At least they are supposed to.

On 5/9/07, Tim Haines <tmha...@gmail.com> wrote:

--
Cheers,
hamilton verissimo
ham...@castlestronghold.com
http://www.castlestronghold.com/

[Roelof Blom]

Tim,

1) I feel you should just make the change and document it in the CHANGES.txt
2) DateValidatorTestCase should be modified to check the new behavior.

Shouldn't DateTimeValidator also behave in the same way as DateValidator?

-- Roelof.

On 5/9/07, Ken Egozi < ego...@gmail.com> wrote:

[Brian]

Seems lots of validators return false for nulls, including Decimal,
Integer, Double, etc.. The separation makes sense -- a bunch of
validators will need to be updated.

Quick survey of validators that may need updating: CollectionNotEmpty,
DateTime, Date, Decimal, Double, Integer, Single.

If no one else is tackling it I'll whip up a patch...

[Brian]

Patch follows.

Index: Castle.Components.Validator.Tests/ValidatorTests/
DateTimeValidatorTestCase.cs
===================================================================
--- Castle.Components.Validator.Tests/ValidatorTests/
DateTimeValidatorTestCase.cs (revision 3772)
+++ Castle.Components.Validator.Tests/ValidatorTests/
DateTimeValidatorTestCase.cs (working copy)
@@ -43,7 +43,6 @@
Assert.IsFalse(validator.IsValid(target, "122"));
Assert.IsFalse(validator.IsValid(target, "99/99/99"));
Assert.IsFalse(validator.IsValid(target, "99-99-99"));
- Assert.IsFalse(validator.IsValid(target, null));
Assert.IsFalse(validator.IsValid(target, ""));
}

@@ -53,6 +52,7 @@
Assert.IsTrue(validator.IsValid(target, "01/12/2004"));
Assert.IsTrue(validator.IsValid(target, "07/16/79"));
Assert.IsTrue(validator.IsValid(target, "2007-01-14T12:05:25"));
+ Assert.IsTrue(validator.IsValid(target, null));
}

public class TestTarget
Index: Castle.Components.Validator.Tests/ValidatorTests/
DateValidatorTestCase.cs
===================================================================
--- Castle.Components.Validator.Tests/ValidatorTests/
DateValidatorTestCase.cs (revision 3772)
+++ Castle.Components.Validator.Tests/ValidatorTests/
DateValidatorTestCase.cs (working copy)
@@ -44,7 +44,6 @@
Assert.IsFalse(validator.IsValid(target, "99/99/99"));
Assert.IsFalse(validator.IsValid(target, "99-99-99"));
Assert.IsFalse(validator.IsValid(target, "2007-01-14T12:05:25"));
- Assert.IsFalse(validator.IsValid(target, null));
Assert.IsFalse(validator.IsValid(target, ""));
}

@@ -53,6 +52,7 @@
{
Assert.IsTrue(validator.IsValid(target, "01/12/2004"));
Assert.IsTrue(validator.IsValid(target, "07/16/79"));
+ Assert.IsTrue(validator.IsValid(target, null));
}

public class TestTarget
Index: Castle.Components.Validator.Tests/ValidatorTests/
DecimalValidatorTestCase.cs
===================================================================
--- Castle.Components.Validator.Tests/ValidatorTests/
DecimalValidatorTestCase.cs (revision 3772)
+++ Castle.Components.Validator.Tests/ValidatorTests/
DecimalValidatorTestCase.cs (working copy)
@@ -39,7 +39,6 @@
public void InvalidDecimal()
{
Assert.IsFalse(validator.IsValid(target, "abc"));
- Assert.IsFalse(validator.IsValid(target, null));
Assert.IsFalse(validator.IsValid(target, ""));
}

@@ -50,6 +49,7 @@
Assert.IsTrue(validator.IsValid(target, "100.45"));
Assert.IsTrue(validator.IsValid(target, "-99.8"));
Assert.IsTrue(validator.IsValid(target, "-99"));
+ Assert.IsTrue(validator.IsValid(target, null));
}

public class TestTarget
Index: Castle.Components.Validator.Tests/ValidatorTests/
DoubleValidatorTestCase.cs
===================================================================
--- Castle.Components.Validator.Tests/ValidatorTests/
DoubleValidatorTestCase.cs (revision 3772)
+++ Castle.Components.Validator.Tests/ValidatorTests/
DoubleValidatorTestCase.cs (working copy)
@@ -39,7 +39,6 @@
public void InvalidDouble()
{
Assert.IsFalse(validator.IsValid(target, "abc"));
- Assert.IsFalse(validator.IsValid(target, null));
Assert.IsFalse(validator.IsValid(target, ""));
}

@@ -50,6 +49,7 @@
Assert.IsTrue(validator.IsValid(target, "100.45155"));
Assert.IsTrue(validator.IsValid(target, "-99.8"));
Assert.IsTrue(validator.IsValid(target, "-99"));
+ Assert.IsTrue(validator.IsValid(target, null));
}

public class TestTarget
Index: Castle.Components.Validator.Tests/ValidatorTests/
IntegerValidatorTestCase.cs
===================================================================
--- Castle.Components.Validator.Tests/ValidatorTests/
IntegerValidatorTestCase.cs (revision 3772)
+++ Castle.Components.Validator.Tests/ValidatorTests/
IntegerValidatorTestCase.cs (working copy)
@@ -41,7 +41,6 @@
Assert.IsFalse(validator.IsValid(target, "abc"));
Assert.IsFalse(validator.IsValid(target, "100.11"));
Assert.IsFalse(validator.IsValid(target, "-99.8"));
- Assert.IsFalse(validator.IsValid(target, null));
Assert.IsFalse(validator.IsValid(target, ""));
}

@@ -50,6 +49,7 @@
{
Assert.IsTrue(validator.IsValid(target, "100"));
Assert.IsTrue(validator.IsValid(target, "-99"));
+ Assert.IsTrue(validator.IsValid(target, null));
}

public class TestTarget
Index: Castle.Components.Validator.Tests/ValidatorTests/
SingleValidatorTestCase.cs
===================================================================
--- Castle.Components.Validator.Tests/ValidatorTests/
SingleValidatorTestCase.cs (revision 3772)
+++ Castle.Components.Validator.Tests/ValidatorTests/
SingleValidatorTestCase.cs (working copy)
@@ -40,7 +40,6 @@
public void InvalidSingle()
{
Assert.IsFalse(validator.IsValid(target, "abc"));
- Assert.IsFalse(validator.IsValid(target, null));
Assert.IsFalse(validator.IsValid(target, ""));
}

@@ -51,6 +50,7 @@
Assert.IsTrue(validator.IsValid(target, "100.11002"));
Assert.IsTrue(validator.IsValid(target, "-99.8"));
Assert.IsTrue(validator.IsValid(target, "-99"));
+ Assert.IsTrue(validator.IsValid(target, null));
}

public class TestTarget
Index: Castle.Components.Validator/Validators/DateTimeValidator.cs
===================================================================
--- Castle.Components.Validator/Validators/DateTimeValidator.cs
(revision 3772)
+++ Castle.Components.Validator/Validators/DateTimeValidator.cs
(working copy)
@@ -35,7 +35,7 @@
/// </returns>
public override bool IsValid(object instance, object fieldValue)
{
- if (fieldValue == null) return false;
+ if (fieldValue == null) return true;

DateTime datetimeValue;
return DateTime.TryParse(fieldValue.ToString(), out
datetimeValue);
Index: Castle.Components.Validator/Validators/DateValidator.cs
===================================================================
--- Castle.Components.Validator/Validators/DateValidator.cs (revision
3772)
+++ Castle.Components.Validator/Validators/DateValidator.cs (working
copy)
@@ -35,7 +35,7 @@
/// </returns>
public override bool IsValid(object instance, object fieldValue)
{
- if (fieldValue == null) return false;
+ if (fieldValue == null) return true;

DateTime datetimeValue;
bool valid = DateTime.TryParse(fieldValue.ToString(), out
datetimeValue);
Index: Castle.Components.Validator/Validators/DecimalValidator.cs
===================================================================
--- Castle.Components.Validator/Validators/DecimalValidator.cs
(revision 3772)
+++ Castle.Components.Validator/Validators/DecimalValidator.cs
(working copy)
@@ -35,7 +35,7 @@
/// </returns>
public override bool IsValid(object instance, object fieldValue)
{
- if (fieldValue == null) return false;
+ if (fieldValue == null) return true;

Decimal decimalValue;
return Decimal.TryParse(fieldValue.ToString(), out decimalValue);
Index: Castle.Components.Validator/Validators/DoubleValidator.cs
===================================================================
--- Castle.Components.Validator/Validators/DoubleValidator.cs
(revision 3772)
+++ Castle.Components.Validator/Validators/DoubleValidator.cs (working
copy)
@@ -35,7 +35,7 @@
/// </returns>
public override bool IsValid(object instance, object fieldValue)
{
- if (fieldValue == null) return false;
+ if (fieldValue == null) return true;

Double doubleValue;
return Double.TryParse(fieldValue.ToString(), out doubleValue);
Index: Castle.Components.Validator/Validators/IntegerValidator.cs
===================================================================
--- Castle.Components.Validator/Validators/IntegerValidator.cs
(revision 3772)
+++ Castle.Components.Validator/Validators/IntegerValidator.cs
(working copy)
@@ -35,7 +35,7 @@
/// </returns>
public override bool IsValid(object instance, object fieldValue)
{
- if (fieldValue == null) return false;
+ if (fieldValue == null) return true;

string stringValue = fieldValue.ToString();

Index: Castle.Components.Validator/Validators/SingleValidator.cs
===================================================================
--- Castle.Components.Validator/Validators/SingleValidator.cs
(revision 3772)
+++ Castle.Components.Validator/Validators/SingleValidator.cs (working
copy)
@@ -36,7 +36,7 @@
/// </returns>
public override bool IsValid(object instance, object fieldValue)
{
- if (fieldValue == null) return false;
+ if (fieldValue == null) return true;

Single doubleValue;
return Single.TryParse(fieldValue.ToString(), out doubleValue);

[Hamilton Verissimo]

Cool, Brian. But can you send it as an attachment instead?

Thanks

[Tim Haines]

Hey Brian,

Nice start.  There were a couple of other small changes I had in the date validator, which may need to be applied to the other validators too.  In the ApplyWebValidation function, "generator.SetAsRequired (BuildErrorMessage());" needs to be removed.  I'd was also returning true for empty strings, and updated a couple of the comment lines:

-        /// Null or empty value NOT allowed.
+        /// Null or empty value are valid.  Use a NonEmpty Validator to check for null or empty.

-        /// <c>true</c> if the value is accepted (has passed the validation test)
+        /// <c>true</c> if the value is accepted (has passed the validation test) or is null/empty
 


Cheers,

Tim.

                        Assert.IsFalse(validator.IsValid (target, "99-99-99"));

                        Assert.IsFalse (validator.IsValid(target, ""));

                }

@@ -50,6 +49,7 @@
                        Assert.IsTrue(validator.IsValid(target, "100.45155"));

                        Assert.IsTrue (validator.IsValid(target, "-99.8"));

                        Assert.IsTrue(validator.IsValid(target, "-99"));
+                       Assert.IsTrue(validator.IsValid(target, null));
                }

                public class TestTarget
Index: Castle.Components.Validator.Tests/ValidatorTests/
IntegerValidatorTestCase.cs
===================================================================
--- Castle.Components.Validator.Tests /ValidatorTests/
IntegerValidatorTestCase.cs     (revision 3772)
+++ Castle.Components.Validator.Tests/ValidatorTests/
IntegerValidatorTestCase.cs     (working copy)
@@ -41,7 +41,6 @@
                         Assert.IsFalse(validator.IsValid(target, "abc"));
                        Assert.IsFalse(validator.IsValid(target, "100.11"));

                        Assert.IsFalse(validator.IsValid(target, "- 99.8"));

-                       Assert.IsFalse(validator.IsValid(target, null));
                        Assert.IsFalse(validator.IsValid(target, ""));
                }

@@ -50,6 +49,7 @@
                {
                        Assert.IsTrue(validator.IsValid(target, "100"));
                        Assert.IsTrue(validator.IsValid(target, "-99"));

+                       Assert.IsTrue (validator.IsValid(target, null));

                }

                public class TestTarget
Index: Castle.Components.Validator.Tests/ValidatorTests/
SingleValidatorTestCase.cs
===================================================================
--- Castle.Components.Validator.Tests/ValidatorTests/
SingleValidatorTestCase.cs      (revision 3772)
+++ Castle.Components.Validator.Tests/ValidatorTests/
SingleValidatorTestCase.cs      (working copy)
@@ -40,7 +40,6 @@
                public void InvalidSingle()
                {
                        Assert.IsFalse(validator.IsValid(target, "abc"));
-                       Assert.IsFalse(validator.IsValid(target, null));
                        Assert.IsFalse(validator.IsValid(target, ""));
                }

@@ -51,6 +50,7 @@
                        Assert.IsTrue(validator.IsValid(target, "100.11002"));
                        Assert.IsTrue(validator.IsValid(target, "-99.8"));
                        Assert.IsTrue(validator.IsValid(target, "-99"));

+                       Assert.IsTrue(validator.IsValid (target, null));

[Yuri]

Hi all,

These changes seems to revert the changes made for JIRA-issue COMP-21.
The NOT empty validation was introduced on these validators because
they are set automatically if you don't have any validation attribute
on a property.
So if have you have for instance a decimal without any validation
attribute on it, it will have a DecimalValidator assigned to it.
I agree with the separation of concern argument. But if you don't
specify a validation attribute nulls are allowed on this decimal ?!?!?
To get the desired behavior you will need both the NotEmpty and the
DecimalValidator. At this moment you don't need any attribute.
If you need empty values you'll have to use a NullableDecimal, which
will get NullableDecimalValidator automatically if the rest of the
patch for COMP-21 is applied.
Awaiting your reactions.

Cheers,
Yuri

[Tim Haines]

Hey Yuri,

Interesting - I didn't know they were applied by default.  IMHO if validators are going to be added automatically, it seems to me that it would be more sensible to apply a Decimal Validator, and then depending on the nullness of the type, also apply the not empty validator.  That way the decimal validator can also be used for nullable Decimals. 

I'm not aware of the background discussion though - I'd be surprised if you hadn't already covered this if it had been discussed?

Tim.

[Yuri]

Hi Tim,

I introduced the NOT empty validation on these validators, because I
felt these fields can NOT be empty.
I didn't really think about the separation of concern.
If we all agree, I'm happy to revert these validators to their old
state and adjust the ValidationRunner, so that the NonEmptyValidator
is also applied depending on the nullness of the type.
Either this or the introduction of the NullableTypeValidators.
Your votes please!

Cheers,
Yuri

P.S. I'm voting for separation of concern, in spite of the extra work
for me.

[Hamilton Verissimo]

I'm fine with both options. It really doesn't make sense to have value
types as null, right? I'm not sure I like that validators are assigned
automatically. Yes, I know I did that. But that don't make it a good
idea :-)

[Jason Nelson]

Yeah - automatically attaching the validators breaks down in a good deal of cases. Not sure if it's a good convention or not.

[Hamilton Verissimo]

Fair enough. Going to remove that.

[Yuri]

Removing automatic assignment doesn't sound good to me.
It means you'll have to put attributes on all properties if you're
using this feature now.
Make it optional?

Cheers,
Yuri

[Hamilton Verissimo]

Okay. Gonna revert the commit and set the default to false.

On 5/14/07, Yuri <yu...@xs4all.nl> wrote:
>

> Removing automatic assignment doesn't sound good to me.

> It means that if you're using this feature now, you will have to put
> attributes on all the properties you didn't have an attribute on.
> I think it should remain optional.
>
> Cheers,
> Yuri

[Hamilton Verissimo]

For the records: done.

On 5/14/07, Yuri <yu...@xs4all.nl> wrote:
>

> Removing automatic assignment doesn't sound good to me.
> It means that if you're using this feature now, you will have to put
> attributes on all the properties you didn't have an attribute on.
> I think it should remain optional.
>
> Cheers,
> Yuri
>
>
> >
>