You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by David E Jones <de...@me.com> on 2011/02/13 18:24:27 UTC
Re: svn commit: r1070230 -
/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
Please revert this change.
Changing things this way completely breaks the whole idea of the "defaultEmptyOK" variable. If someone wants this to behave differently then they can change it locally (patch or subclass). It should NOT be changed in the project.
The whole idea with these validations is that you get one error if a field is empty (if it is not allowed to be empty) and another different error if the formatting is incorrect.
With this change it is now IMPOSSIBLE to have a field checked by these methods that is allowed to be empty.
The current code is not broken. The problem is a lack of understanding of how these validation methods are designed.
Jacques: please revert this and also please understand and test before committing contributed patches. Don't trust the description alone, look at the code and test it!
-David
On Feb 13, 2011, at 4:55 AM, jleroux@apache.org wrote:
> Author: jleroux
> Date: Sun Feb 13 12:55:45 2011
> New Revision: 1070230
>
> URL: http://svn.apache.org/viewvc?rev=1070230&view=rev
> Log:
> A patch from Stephen Rufle "Blank year in UtilValidate.isYear should return false" (https://issues.apache.org/jira/browse/OFBIZ-4171) - OFBIZ-4171
>
> UtilValidate.isYear returns true for a blank year which the calling function UtilValidate.isDate(String, String, String) tries to parse. This causes an exception to be thrown
> ValidateMethod.java:96 :ERROR] [ValidateMethod.exec] Error in validation method isDateAfterToday of class org.ofbiz.base.util.UtilValidate: null
> I found this error when trying to add a new credit card in the eCommerce checkout flow.
> 1. Add a product to the cart
> 2. login as any user I used "admin"
> 3. Checkout Step "Shipping Address" (Step 1: Where shall we ship it?)
> * Click Next
> 4. Checkout Step "Shipping Options" (Step 2: How shall we ship it?)
> * Choose "UPS Air"
> * Click Next
> 5. Checkout Step "Payment Options" (Step 3: How shall you pay?)
> 6. Create "Credit Card"
> * Fill Name
> * Card Type "Visa"
> * Card Number "4111111111111111"
> * Expiration Date Month drop-down 01
> * Expiration Date Year drop-down leave blank
> * Choose billing address
> * Click "Save" button
>
> Should see "Error in validation method isDateAfterToday of class org.ofbiz.base.util.UtilValidate: null"
>
> My fix is to change isYear, isMonth, and isDay to return false when a blank value is entered.
> After I make my change the message is "The expiration date is before today"
>
> Modified:
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
>
> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java?rev=1070230&r1=1070229&r2=1070230&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java (original)
> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java Sun Feb 13 12:55:45 2011
> @@ -746,7 +746,7 @@ public class UtilValidate {
> * to use 4-digit year numbers everywhere.
> */
> public static boolean isYear(String s) {
> - if (isEmpty(s)) return defaultEmptyOK;
> + if (isEmpty(s)) return false;
>
> if (!isNonnegativeInteger(s)) return false;
> return ((s.length() == 2) || (s.length() == 4));
> @@ -771,13 +771,13 @@ public class UtilValidate {
>
> /** isMonth returns true if string s is a valid month number between 1 and 12. */
> public static boolean isMonth(String s) {
> - if (isEmpty(s)) return defaultEmptyOK;
> + if (isEmpty(s)) return false;
> return isIntegerInRange(s, 1, 12);
> }
>
> /** isDay returns true if string s is a valid day number between 1 and 31. */
> public static boolean isDay(String s) {
> - if (isEmpty(s)) return defaultEmptyOK;
> + if (isEmpty(s)) return false;
> return isIntegerInRange(s, 1, 31);
> }
>
>
>
Re: svn commit: r1070230 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
Reverted at r1070282 for the trunk, r1070284 for R10.04
I must say I was a was too quick on this one. I only trusted the idea and did not notice the defaultEmptyOK variable, sorry.
Jacques
From: "David E Jones" <de...@me.com>
> Please revert this change.
>
> Changing things this way completely breaks the whole idea of the "defaultEmptyOK" variable. If someone wants this to behave
> differently then they can change it locally (patch or subclass). It should NOT be changed in the project.
>
> The whole idea with these validations is that you get one error if a field is empty (if it is not allowed to be empty) and another
> different error if the formatting is incorrect.
>
> With this change it is now IMPOSSIBLE to have a field checked by these methods that is allowed to be empty.
>
> The current code is not broken. The problem is a lack of understanding of how these validation methods are designed.
>
> Jacques: please revert this and also please understand and test before committing contributed patches. Don't trust the description
> alone, look at the code and test it!
>
> -David
>
>
> On Feb 13, 2011, at 4:55 AM, jleroux@apache.org wrote:
>
>> Author: jleroux
>> Date: Sun Feb 13 12:55:45 2011
>> New Revision: 1070230
>>
>> URL: http://svn.apache.org/viewvc?rev=1070230&view=rev
>> Log:
>> A patch from Stephen Rufle "Blank year in UtilValidate.isYear should return false"
>> (https://issues.apache.org/jira/browse/OFBIZ-4171) - OFBIZ-4171
>>
>> UtilValidate.isYear returns true for a blank year which the calling function UtilValidate.isDate(String, String, String) tries to
>> parse. This causes an exception to be thrown
>> ValidateMethod.java:96 :ERROR] [ValidateMethod.exec] Error in validation method isDateAfterToday of class
>> org.ofbiz.base.util.UtilValidate: null
>> I found this error when trying to add a new credit card in the eCommerce checkout flow.
>> 1. Add a product to the cart
>> 2. login as any user I used "admin"
>> 3. Checkout Step "Shipping Address" (Step 1: Where shall we ship it?)
>> * Click Next
>> 4. Checkout Step "Shipping Options" (Step 2: How shall we ship it?)
>> * Choose "UPS Air"
>> * Click Next
>> 5. Checkout Step "Payment Options" (Step 3: How shall you pay?)
>> 6. Create "Credit Card"
>> * Fill Name
>> * Card Type "Visa"
>> * Card Number "4111111111111111"
>> * Expiration Date Month drop-down 01
>> * Expiration Date Year drop-down leave blank
>> * Choose billing address
>> * Click "Save" button
>>
>> Should see "Error in validation method isDateAfterToday of class org.ofbiz.base.util.UtilValidate: null"
>>
>> My fix is to change isYear, isMonth, and isDay to return false when a blank value is entered.
>> After I make my change the message is "The expiration date is before today"
>>
>> Modified:
>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
>>
>> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java?rev=1070230&r1=1070229&r2=1070230&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java (original)
>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java Sun Feb 13 12:55:45 2011
>> @@ -746,7 +746,7 @@ public class UtilValidate {
>> * to use 4-digit year numbers everywhere.
>> */
>> public static boolean isYear(String s) {
>> - if (isEmpty(s)) return defaultEmptyOK;
>> + if (isEmpty(s)) return false;
>>
>> if (!isNonnegativeInteger(s)) return false;
>> return ((s.length() == 2) || (s.length() == 4));
>> @@ -771,13 +771,13 @@ public class UtilValidate {
>>
>> /** isMonth returns true if string s is a valid month number between 1 and 12. */
>> public static boolean isMonth(String s) {
>> - if (isEmpty(s)) return defaultEmptyOK;
>> + if (isEmpty(s)) return false;
>> return isIntegerInRange(s, 1, 12);
>> }
>>
>> /** isDay returns true if string s is a valid day number between 1 and 31. */
>> public static boolean isDay(String s) {
>> - if (isEmpty(s)) return defaultEmptyOK;
>> + if (isEmpty(s)) return false;
>> return isIntegerInRange(s, 1, 31);
>> }
>>
>>
>>
>
Re: svn commit: r1070230 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
Stephen,
What means David is that UtilValidate.isDate(), and its variants isDateBeforeToday() and isDateAfterToday which call isDate(), are
used in screens, forms and minilang (look for isDate in *.xml).
You can't rely only on compilation and make methods private
Jacques
From: "Stephen Rufle" <sr...@salmonllc.com>
>I did see the defaultEmptyOK in each of the methods I changed, my patch fixes the issue in
>org.ofbiz.base.util.UtilValidate.isDate(String, String, String) where the lines
>
> int intYear = Integer.parseInt(year);
> int intMonth = Integer.parseInt(month);
> int intDay = Integer.parseInt(day);
>
> should only get parse-able values. Current OOTB does not allow the "catch invalid years" logic to correctly protect the parsing.
> The three methods that are modified are only called from UtilValidate.isDate(String, String, String), so it seems like it should
> be ok. I also tried an experiment locally I made those methods private and I do not get an compile errors, so I believe my change
> is isolated to just fixing the issue I saw and has no ill side effects.
>
> On 2/13/2011 10:24 AM, David E Jones wrote:
>>
>> Please revert this change.
>>
>> Changing things this way completely breaks the whole idea of the
>> "defaultEmptyOK" variable. If someone wants this to behave
>> differently then they can change it locally (patch or subclass). It
>> should NOT be changed in the project.
>>
>> The whole idea with these validations is that you get one error if a
>> field is empty (if it is not allowed to be empty) and another
>> different error if the formatting is incorrect.
>>
>> With this change it is now IMPOSSIBLE to have a field checked by
>> these methods that is allowed to be empty.
>>
>> The current code is not broken. The problem is a lack of
>> understanding of how these validation methods are designed.
>>
>> Jacques: please revert this and also please understand and test
>> before committing contributed patches. Don't trust the description
>> alone, look at the code and test it!
>>
>> -David
>>
>>
>> On Feb 13, 2011, at 4:55 AM, jleroux@apache.org wrote:
>>
>>> Author: jleroux Date: Sun Feb 13 12:55:45 2011 New Revision:
>>> 1070230
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1070230&view=rev Log: A patch
>>> from Stephen Rufle "Blank year in UtilValidate.isYear should return
>>> false" (https://issues.apache.org/jira/browse/OFBIZ-4171) -
>>> OFBIZ-4171
>>>
>>> UtilValidate.isYear returns true for a blank year which the calling
>>> function UtilValidate.isDate(String, String, String) tries to
>>> parse. This causes an exception to be thrown ValidateMethod.java:96
>>> :ERROR] [ValidateMethod.exec] Error in validation method
>>> isDateAfterToday of class org.ofbiz.base.util.UtilValidate: null I
>>> found this error when trying to add a new credit card in the
>>> eCommerce checkout flow. 1. Add a product to the cart 2. login as
>>> any user I used "admin" 3. Checkout Step "Shipping Address" (Step
>>> 1: Where shall we ship it?) * Click Next 4. Checkout Step "Shipping
>>> Options" (Step 2: How shall we ship it?) * Choose "UPS Air" * Click
>>> Next 5. Checkout Step "Payment Options" (Step 3: How shall you
>>> pay?) 6. Create "Credit Card" * Fill Name * Card Type "Visa" * Card
>>> Number "4111111111111111" * Expiration Date Month drop-down 01 *
>>> Expiration Date Year drop-down leave blank * Choose billing
>>> address * Click "Save" button
>>>
>>> Should see "Error in validation method isDateAfterToday of class
>>> org.ofbiz.base.util.UtilValidate: null"
>>>
>>> My fix is to change isYear, isMonth, and isDay to return false when
>>> a blank value is entered. After I make my change the message is
>>> "The expiration date is before today"
>>>
>>> Modified:
>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
>>>
>>>
>>>
> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java?rev=1070230&r1=1070229&r2=1070230&view=diff
>>>
>>>
> ==============================================================================
>>> ---
>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
>>> (original) +++
>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
>>> Sun Feb 13 12:55:45 2011 @@ -746,7 +746,7 @@ public class
>>> UtilValidate { * to use 4-digit year numbers everywhere. */ public
>>> static boolean isYear(String s) { - if (isEmpty(s)) return
>>> defaultEmptyOK; + if (isEmpty(s)) return false;
>>>
>>> if (!isNonnegativeInteger(s)) return false; return ((s.length() ==
>>> 2) || (s.length() == 4)); @@ -771,13 +771,13 @@ public class
>>> UtilValidate {
>>>
>>> /** isMonth returns true if string s is a valid month number
>>> between 1 and 12. */ public static boolean isMonth(String s) { -
>>> if (isEmpty(s)) return defaultEmptyOK; + if (isEmpty(s))
>>> return false; return isIntegerInRange(s, 1, 12); }
>>>
>>> /** isDay returns true if string s is a valid day number between 1
>>> and 31. */ public static boolean isDay(String s) { - if
>>> (isEmpty(s)) return defaultEmptyOK; + if (isEmpty(s)) return
>>> false; return isIntegerInRange(s, 1, 31); }
>>>
>>>
>>>
>>
>>
>>
>
> --
> Stephen P Rufle
> srufle@salmonllc.com
> O:480-626-8022
> Yahoo IM: stephen_rufle
> AOL IM: stephen1rufle
>
Re: svn commit: r1070230 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
Posted by Stephen Rufle <sr...@salmonllc.com>.
I did see the defaultEmptyOK in each of the methods I changed, my patch
fixes the issue in org.ofbiz.base.util.UtilValidate.isDate(String,
String, String) where the lines
int intYear = Integer.parseInt(year);
int intMonth = Integer.parseInt(month);
int intDay = Integer.parseInt(day);
should only get parse-able values. Current OOTB does not allow the
"catch invalid years" logic to correctly protect the parsing. The three
methods that are modified are only called from
UtilValidate.isDate(String, String, String), so it seems like it should
be ok. I also tried an experiment locally I made those methods private
and I do not get an compile errors, so I believe my change is isolated
to just fixing the issue I saw and has no ill side effects.
On 2/13/2011 10:24 AM, David E Jones wrote:
>
> Please revert this change.
>
> Changing things this way completely breaks the whole idea of the
> "defaultEmptyOK" variable. If someone wants this to behave
> differently then they can change it locally (patch or subclass). It
> should NOT be changed in the project.
>
> The whole idea with these validations is that you get one error if a
> field is empty (if it is not allowed to be empty) and another
> different error if the formatting is incorrect.
>
> With this change it is now IMPOSSIBLE to have a field checked by
> these methods that is allowed to be empty.
>
> The current code is not broken. The problem is a lack of
> understanding of how these validation methods are designed.
>
> Jacques: please revert this and also please understand and test
> before committing contributed patches. Don't trust the description
> alone, look at the code and test it!
>
> -David
>
>
> On Feb 13, 2011, at 4:55 AM, jleroux@apache.org wrote:
>
>> Author: jleroux Date: Sun Feb 13 12:55:45 2011 New Revision:
>> 1070230
>>
>> URL: http://svn.apache.org/viewvc?rev=1070230&view=rev Log: A patch
>> from Stephen Rufle "Blank year in UtilValidate.isYear should return
>> false" (https://issues.apache.org/jira/browse/OFBIZ-4171) -
>> OFBIZ-4171
>>
>> UtilValidate.isYear returns true for a blank year which the calling
>> function UtilValidate.isDate(String, String, String) tries to
>> parse. This causes an exception to be thrown ValidateMethod.java:96
>> :ERROR] [ValidateMethod.exec] Error in validation method
>> isDateAfterToday of class org.ofbiz.base.util.UtilValidate: null I
>> found this error when trying to add a new credit card in the
>> eCommerce checkout flow. 1. Add a product to the cart 2. login as
>> any user I used "admin" 3. Checkout Step "Shipping Address" (Step
>> 1: Where shall we ship it?) * Click Next 4. Checkout Step "Shipping
>> Options" (Step 2: How shall we ship it?) * Choose "UPS Air" * Click
>> Next 5. Checkout Step "Payment Options" (Step 3: How shall you
>> pay?) 6. Create "Credit Card" * Fill Name * Card Type "Visa" * Card
>> Number "4111111111111111" * Expiration Date Month drop-down 01 *
>> Expiration Date Year drop-down leave blank * Choose billing
>> address * Click "Save" button
>>
>> Should see "Error in validation method isDateAfterToday of class
>> org.ofbiz.base.util.UtilValidate: null"
>>
>> My fix is to change isYear, isMonth, and isDay to return false when
>> a blank value is entered. After I make my change the message is
>> "The expiration date is before today"
>>
>> Modified:
>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
>>
>>
>>
Modified:
ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java?rev=1070230&r1=1070229&r2=1070230&view=diff
>>
>>
==============================================================================
>> ---
>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
>> (original) +++
>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilValidate.java
>> Sun Feb 13 12:55:45 2011 @@ -746,7 +746,7 @@ public class
>> UtilValidate { * to use 4-digit year numbers everywhere. */ public
>> static boolean isYear(String s) { - if (isEmpty(s)) return
>> defaultEmptyOK; + if (isEmpty(s)) return false;
>>
>> if (!isNonnegativeInteger(s)) return false; return ((s.length() ==
>> 2) || (s.length() == 4)); @@ -771,13 +771,13 @@ public class
>> UtilValidate {
>>
>> /** isMonth returns true if string s is a valid month number
>> between 1 and 12. */ public static boolean isMonth(String s) { -
>> if (isEmpty(s)) return defaultEmptyOK; + if (isEmpty(s))
>> return false; return isIntegerInRange(s, 1, 12); }
>>
>> /** isDay returns true if string s is a valid day number between 1
>> and 31. */ public static boolean isDay(String s) { - if
>> (isEmpty(s)) return defaultEmptyOK; + if (isEmpty(s)) return
>> false; return isIntegerInRange(s, 1, 31); }
>>
>>
>>
>
>
>
--
Stephen P Rufle
srufle@salmonllc.com
O:480-626-8022
Yahoo IM: stephen_rufle
AOL IM: stephen1rufle