Revision 703731

[David E Jones]

Scott, are you referring to the call to PreparedStatement.setString on  
SQLProcessor line 553? In other words, the difference in the Postgres  
JDBC driver is that they used to auto-convert from a String to  
whatever the database needs in the setString method, but now they don't?

What I'm proposing is to fix this further up the stack in the  
GenericEntity.set method instead of lower down. I guess either way we  
have the two options I mentioned earlier:

1. fail fast(er) by having OFBiz throw an exception
2. try to auto-convert

#1 will result, as Adrian mentioned, in a bunch of possibly  
undiscovered places where the wrong object type is passed in, causing  
OFBiz to not function until these are cleaned up. On the other hand,  
in order to reliably work on most databases these need to be fixed  
sooner or later.

#2 has the problem, also as Adrian mentioned, of possibly not doing  
the conversion correctly, especially in cases where locale-sensitive  
parsing is needed or there is a special format string or something.

In spite of the short-term pain my vote is still for #1, and to do it  
higher up in the GenericEntity.set method in order to fail right when  
it's set, not later on when the GenericDelegator method is called, so  
that we know where the bad object type is being passed.

-David


On Oct 13, 2008, at 7:14 PM, Scott Gray wrote:

> Here's the problem as I see it:
> Postgresql used to allow you to specify parameter types which did not
> match the column type, in the timestamp case if you passed in a
> parameter specified as varchar it would automatically attempt to
> convert it to a timestamp.  Now Postgresql requires that you either
> pass in the correct parameter type for the column or otherwise pass
> the parameter type as unknown/unspecified.  Postgresql still doesn't
> care whether you pass in a string or a timestamp but you MUST specify
> the correct sql type.
>
> If letting the database take care of the type conversion has never
> been a problem before, why do we need to worry about it now?  Remember
> the only problem here is that we are passing in a string specified as
> varchar instead of a string specified as java.sql.Types.Timestamp.
>
> Regards
> Scott
>
> 2008/10/14 Adrian Crum <[hidden email]>:
>> I understood your alternatives. I apologize for being unclear.
>>
>> You said, "or to just throw an exception when the wrong data type  
>> is passed
>> like the commented out code in GenericEntity referenced above does."
>>
>> And I'm saying we could do that, but it's going to get interesting  
>> because a
>> lot of code passes Strings into the entity engine. In other words,  
>> if we
>> throw a "hard" exception, there is a good chance that much of OFBiz  
>> won't
>> run.
>>
>> Personally, I'd like to see better handling of object types in the  
>> higher
>> level code. I fixed a problem over the weekend that was caused by  
>> this very
>> thing (passing Strings into the entity engine).
>>
>> -Adrian
>>
>>
>> David E Jones wrote:
>>>
>>> I'm referring to the exception I described in the  
>>> GenericEntity.java file,
>>> lines 410-415. Right now it is just a warning in the log (and has  
>>> been for
>>> years). The reason to do it there instead of letting the JDBC  
>>> driver do it
>>> is that not all developers will test on all possible databases,  
>>> and this
>>> will help avoid errors as people use different databases in
>>> development/testing or deployment.
>>>
>>> I tried to describe two possible approaches to improve this  
>>> situation in
>>> my first message. Please let me know if those alternatives were  
>>> not clear
>>> and I'll try to explain them better.
>>>
>>> -David
>>>
>>>
>>> On Oct 13, 2008, at 5:22 PM, Adrian Crum wrote:
>>>
>>>> Wasn't that being done already? Jacques started these changes  
>>>> based on
>>>> exceptions being thrown by his JDBC driver. Other DBs seemed to  
>>>> be working
>>>> okay.
>>>>
>>>> I guess we could throw our own exception to keep things  
>>>> consistent across
>>>> databases. It will be interesting to see how that affects  
>>>> existing code.
>>>>
>>>> -Adrian
>>>>
>>>>
>>>> David E Jones wrote:
>>>>>
>>>>> That is a very good point, and I agree.
>>>>> To me that means we go with the fail-fast approach and have it  
>>>>> throw an
>>>>> exception if you pass in something that is not the correct  
>>>>> object type.
>>>>> Is that what you meant?
>>>>> -David
>>>>> On Oct 13, 2008, at 4:57 PM, Adrian Crum wrote:
>>>>>>
>>>>>> I would prefer handling object types in higher level code - due  
>>>>>> to
>>>>>> ambiguity in how some types would be converted from a String  
>>>>>> (currency,
>>>>>> date, time, etc).
>>>>>>
>>>>>> -Adrian
>>>>>>
>>>>>> David E Jones wrote:
>>>>>>>
>>>>>>> This has been a potential problem for a while, but the  
>>>>>>> decision was
>>>>>>> made quite a while back to not enforce the correct object type.
>>>>>>> There is code in the GenericEntity.java file (lines 410-415)  
>>>>>>> to check
>>>>>>> to see if the value passed in matches the object type for the  
>>>>>>> field it is
>>>>>>> being set on.
>>>>>>> The best way to solve this problem is a good question. It  
>>>>>>> might be
>>>>>>> good to introduce some automatic type conversion, or to just  
>>>>>>> throw an
>>>>>>> exception when the wrong data type is passed like the  
>>>>>>> commented out code in
>>>>>>> GenericEntity referenced above does.
>>>>>>> The most common form of automatic conversion is from String to
>>>>>>> whatever the field's object type is, and that is done by the
>>>>>>> GenericEntity.setString method.
>>>>>>> So, what do people think about this? Should we do an automatic  
>>>>>>> type
>>>>>>> conversion to try to fix programming errors automatically, or  
>>>>>>> do a fail-fast
>>>>>>> by throwing an exception when the object type is wrong? I  
>>>>>>> suppose the
>>>>>>> current fail-quiet (in the log) and fail loudly on certain  
>>>>>>> databases later
>>>>>>> is not the best option...
>>>>>>> -David
>>>>>>> On Oct 12, 2008, at 2:20 PM, Scott Gray wrote:
>>>>>>>>
>>>>>>>> I've had a look into this and I can't see anything related to  
>>>>>>>> Groovy
>>>>>>>> that is making this necessary, it appears to be entirely a  
>>>>>>>> postgresql
>>>>>>>> issue.
>>>>>>>>
>>>>>>>> When executing a prepared statement postgresql seems to  
>>>>>>>> require that
>>>>>>>> the parameter list sql types match the column types.  So the  
>>>>>>>> problem
>>>>>>>> isn't that we are passing in a string but that we are setting  
>>>>>>>> the sql
>>>>>>>> type to character varying by using  
>>>>>>>> PreparedStatement.setString().
>>>>>>>>
>>>>>>>> Here's a patch that fixes the issue but I'm not really  
>>>>>>>> confident
>>>>>>>> enough to commit it, it would be great to get some comments  
>>>>>>>> from
>>>>>>>> people who know more about this kind of thing:
>>>>>>>>
>>>>>>>> Index: framework/entity/src/org/ofbiz/entity/jdbc/
>>>>>>>> SQLProcessor.java
>>>>>>>> =
>>>>>>>> =
>>>>>>>> =
>>>>>>>> =
>>>>>>>> ===============================================================
>>>>>>>> --- framework/entity/src/org/ofbiz/entity/jdbc/
>>>>>>>> SQLProcessor.java
>>>>>>>> (revision
>>>>>>>> 703572)
>>>>>>>> +++ framework/entity/src/org/ofbiz/entity/jdbc/
>>>>>>>> SQLProcessor.java
>>>>>>>> (working copy)
>>>>>>>> @@ -592,6 +592,22 @@
>>>>>>>>  *
>>>>>>>>  * @throws SQLException
>>>>>>>>  */
>>>>>>>> +    public void setValueTimestampString(String field) throws
>>>>>>>> SQLException {
>>>>>>>> +        if (field != null) {
>>>>>>>> +            _ps.setObject(_ind, field, Types.TIMESTAMP);
>>>>>>>> +        } else {
>>>>>>>> +            _ps.setNull(_ind, Types.TIMESTAMP);
>>>>>>>> +        }
>>>>>>>> +        _ind++;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    /**
>>>>>>>> +     * Set the next binding variable of the currently active
>>>>>>>> prepared
>>>>>>>> statement.
>>>>>>>> +     *
>>>>>>>> +     * @param field
>>>>>>>> +     *
>>>>>>>> +     * @throws SQLException
>>>>>>>> +     */
>>>>>>>> public void setValue(java.sql.Time field) throws SQLException {
>>>>>>>>     if (field != null) {
>>>>>>>>         _ps.setTime(_ind, field);
>>>>>>>> Index: framework/entity/src/org/ofbiz/entity/jdbc/
>>>>>>>> SqlJdbcUtil.java
>>>>>>>> =
>>>>>>>> =
>>>>>>>> =
>>>>>>>> =
>>>>>>>> ===============================================================
>>>>>>>> --- framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>> (revision
>>>>>>>> 703572)
>>>>>>>> +++ framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>> (working copy)
>>>>>>>> @@ -731,6 +731,9 @@
>>>>>>>>                 fieldClassName = "byte[]";
>>>>>>>>             }
>>>>>>>>
>>>>>>>> +                if ("java.sql.Timestamp".equals(fieldType)) {
>>>>>>>> +                    fieldClassName = fieldType;
>>>>>>>> +                }
>>>>>>>>             if (Debug.verboseOn()) Debug.logVerbose("type of
>>>>>>>> field " + entityName + "." + modelField.getName() +
>>>>>>>>                     " is " + fieldClassName + ", was  
>>>>>>>> expecting "
>>>>>>>> + mft.getJavaType() + "; this may " +
>>>>>>>>                     "indicate an error in the configuration  
>>>>>>>> or in
>>>>>>>> the class, and may result " +
>>>>>>>> @@ -749,7 +752,11 @@
>>>>>>>>             break;
>>>>>>>>
>>>>>>>>         case 2:
>>>>>>>> -                sqlP.setValue((java.sql.Timestamp)  
>>>>>>>> fieldValue);
>>>>>>>> +                if (fieldValue instanceof String) {
>>>>>>>> +                    sqlP.setValueTimestampString((String)
>>>>>>>> fieldValue);
>>>>>>>> +                } else {
>>>>>>>> +                    sqlP.setValue((java.sql.Timestamp)  
>>>>>>>> fieldValue);
>>>>>>>> +                }
>>>>>>>>             break;
>>>>>>>>
>>>>>>>>         case 3:
>>>>>>>>
>>>>>>>> Regards
>>>>>>>> Scott
>>>>>>>>
>>>>>>>>
>>>>>>>> 2008/10/13 Jacques Le Roux <[hidden email]>:
>>>>>>>>>
>>>>>>>>> Done in revision: 703816
>>>>>>>>> It was not possible for PackingSlip.groovy and
>>>>>>>>> FindInventoryEventPlan.groovy. Because there the date string  
>>>>>>>>> is
>>>>>>>>> build
>>>>>>>>> dynamically in the Groovy file
>>>>>>>>>
>>>>>>>>> Jacques
>>>>>>>>>
>>>>>>>>> From: "Jacques Le Roux" <[hidden email]>
>>>>>>>>>>
>>>>>>>>>> Adrian,
>>>>>>>>>>
>>>>>>>>>> Yes good idea indeed, I will do that
>>>>>>>>>>
>>>>>>>>>> Jacques
>>>>>>>>>>
>>>>>>>>>> From: "Adrian Crum" <[hidden email]>
>>>>>>>>>>>
>>>>>>>>>>> Jacques,
>>>>>>>>>>>
>>>>>>>>>>> Instead of modifying the groovy files, try specifying the  
>>>>>>>>>>> data
>>>>>>>>>>> type in
>>>>>>>>>>> the screen widget.
>>>>>>>>>>>
>>>>>>>>>>> Example in ReportFinancialSummaryScreens.xml:
>>>>>>>>>>>
>>>>>>>>>>> <set field="fromDate" from-field="parameters.fromDate"
>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>> <set field="thruDate" from-field="parameters.thruDate"
>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>> <script
>>>>>>>>>>>
>>>>>>>>>>> location="component://accounting/webapp/accounting/WEB-INF/
>>>>>>>>>>> actions/reports/TransactionTotals.groovy"/>
>>>>>>>>>>>
>>>>>>>>>>> -Adrian
>>>>>>>>>
>>>>>>>>>
>>>
>>>
>>

[Scott Gray]

Inline:

2008/10/14 David E Jones <[hidden email]>:
>
> Scott, are you referring to the call to PreparedStatement.setString on
> SQLProcessor line 553? In other words, the difference in the Postgres JDBC
> driver is that they used to auto-convert from a String to whatever the
> database needs in the setString method, but now they don't?

When we call PreparedStatement.setString() the JDBC driver now adds
something like String:varchar to the parameter list where it used to
add String:unspecified which meant the database would try and take
care of it, but with varchar it just says "wrong!" and throws an
error.  However if we call PreparedStatement.setObject() passing in
the correct sql type then we get String:timestamp and everything works
fine.

So I guess what I'm saying is that I think java type conversion
(String vs Timestamp) is a separate topic and the problem here is
simply the sql type that we are specifying (by calling the various
PreparedStatement methods) does not match the column type.  We could
probably solve the postgresql issue by simply calling
setObject(Types.OTHER) whenever the value type doesn't match entity
field type.


> What I'm proposing is to fix this further up the stack in the
> GenericEntity.set method instead of lower down.

That's fine for when we are using the GenericEntity but we'd also need
to add similar code to the EntityCondition stuff for find queries
(which is where Jacques noticed the problem, I haven't checked for the
error on any operations involving GenericEntity).

Regards
Scott

> On Oct 13, 2008, at 7:14 PM, Scott Gray wrote:
>
>> Here's the problem as I see it:
>> Postgresql used to allow you to specify parameter types which did not
>> match the column type, in the timestamp case if you passed in a
>> parameter specified as varchar it would automatically attempt to
>> convert it to a timestamp.  Now Postgresql requires that you either
>> pass in the correct parameter type for the column or otherwise pass
>> the parameter type as unknown/unspecified.  Postgresql still doesn't
>> care whether you pass in a string or a timestamp but you MUST specify
>> the correct sql type.
>>
>> If letting the database take care of the type conversion has never
>> been a problem before, why do we need to worry about it now?  Remember
>> the only problem here is that we are passing in a string specified as
>> varchar instead of a string specified as java.sql.Types.Timestamp.
>>
>> Regards
>> Scott
>>
>> 2008/10/14 Adrian Crum <[hidden email]>:
>>>
>>> I understood your alternatives. I apologize for being unclear.
>>>
>>> You said, "or to just throw an exception when the wrong data type is
>>> passed
>>> like the commented out code in GenericEntity referenced above does."
>>>
>>> And I'm saying we could do that, but it's going to get interesting
>>> because a
>>> lot of code passes Strings into the entity engine. In other words, if we
>>> throw a "hard" exception, there is a good chance that much of OFBiz won't
>>> run.
>>>
>>> Personally, I'd like to see better handling of object types in the higher
>>> level code. I fixed a problem over the weekend that was caused by this
>>> very
>>> thing (passing Strings into the entity engine).
>>>
>>> -Adrian
>>>
>>>
>>> David E Jones wrote:
>>>>
>>>> I'm referring to the exception I described in the GenericEntity.java
>>>> file,
>>>> lines 410-415. Right now it is just a warning in the log (and has been
>>>> for
>>>> years). The reason to do it there instead of letting the JDBC driver do
>>>> it
>>>> is that not all developers will test on all possible databases, and this
>>>> will help avoid errors as people use different databases in
>>>> development/testing or deployment.
>>>>
>>>> I tried to describe two possible approaches to improve this situation in
>>>> my first message. Please let me know if those alternatives were not
>>>> clear
>>>> and I'll try to explain them better.
>>>>
>>>> -David
>>>>
>>>>
>>>> On Oct 13, 2008, at 5:22 PM, Adrian Crum wrote:
>>>>
>>>>> Wasn't that being done already? Jacques started these changes based on
>>>>> exceptions being thrown by his JDBC driver. Other DBs seemed to be
>>>>> working
>>>>> okay.
>>>>>
>>>>> I guess we could throw our own exception to keep things consistent
>>>>> across
>>>>> databases. It will be interesting to see how that affects existing
>>>>> code.
>>>>>
>>>>> -Adrian
>>>>>
>>>>>
>>>>> David E Jones wrote:
>>>>>>
>>>>>> That is a very good point, and I agree.
>>>>>> To me that means we go with the fail-fast approach and have it throw
>>>>>> an
>>>>>> exception if you pass in something that is not the correct object
>>>>>> type.
>>>>>> Is that what you meant?
>>>>>> -David
>>>>>> On Oct 13, 2008, at 4:57 PM, Adrian Crum wrote:
>>>>>>>
>>>>>>> I would prefer handling object types in higher level code - due to
>>>>>>> ambiguity in how some types would be converted from a String
>>>>>>> (currency,
>>>>>>> date, time, etc).
>>>>>>>
>>>>>>> -Adrian
>>>>>>>
>>>>>>> David E Jones wrote:
>>>>>>>>
>>>>>>>> This has been a potential problem for a while, but the decision was
>>>>>>>> made quite a while back to not enforce the correct object type.
>>>>>>>> There is code in the GenericEntity.java file (lines 410-415) to
>>>>>>>> check
>>>>>>>> to see if the value passed in matches the object type for the field
>>>>>>>> it is
>>>>>>>> being set on.
>>>>>>>> The best way to solve this problem is a good question. It might be
>>>>>>>> good to introduce some automatic type conversion, or to just throw
>>>>>>>> an
>>>>>>>> exception when the wrong data type is passed like the commented out
>>>>>>>> code in
>>>>>>>> GenericEntity referenced above does.
>>>>>>>> The most common form of automatic conversion is from String to
>>>>>>>> whatever the field's object type is, and that is done by the
>>>>>>>> GenericEntity.setString method.
>>>>>>>> So, what do people think about this? Should we do an automatic type
>>>>>>>> conversion to try to fix programming errors automatically, or do a
>>>>>>>> fail-fast
>>>>>>>> by throwing an exception when the object type is wrong? I suppose
>>>>>>>> the
>>>>>>>> current fail-quiet (in the log) and fail loudly on certain databases
>>>>>>>> later
>>>>>>>> is not the best option...
>>>>>>>> -David
>>>>>>>> On Oct 12, 2008, at 2:20 PM, Scott Gray wrote:
>>>>>>>>>
>>>>>>>>> I've had a look into this and I can't see anything related to
>>>>>>>>> Groovy
>>>>>>>>> that is making this necessary, it appears to be entirely a
>>>>>>>>> postgresql
>>>>>>>>> issue.
>>>>>>>>>
>>>>>>>>> When executing a prepared statement postgresql seems to require
>>>>>>>>> that
>>>>>>>>> the parameter list sql types match the column types.  So the
>>>>>>>>> problem
>>>>>>>>> isn't that we are passing in a string but that we are setting the
>>>>>>>>> sql
>>>>>>>>> type to character varying by using PreparedStatement.setString().
>>>>>>>>>
>>>>>>>>> Here's a patch that fixes the issue but I'm not really confident
>>>>>>>>> enough to commit it, it would be great to get some comments from
>>>>>>>>> people who know more about this kind of thing:
>>>>>>>>>
>>>>>>>>> Index: framework/entity/src/org/ofbiz/entity/jdbc/SQLProcessor.java
>>>>>>>>> ===================================================================
>>>>>>>>> --- framework/entity/src/org/ofbiz/entity/jdbc/SQLProcessor.java
>>>>>>>>> (revision
>>>>>>>>> 703572)
>>>>>>>>> +++ framework/entity/src/org/ofbiz/entity/jdbc/SQLProcessor.java
>>>>>>>>> (working copy)
>>>>>>>>> @@ -592,6 +592,22 @@
>>>>>>>>>  *
>>>>>>>>>  * @throws SQLException
>>>>>>>>>  */
>>>>>>>>> +    public void setValueTimestampString(String field) throws
>>>>>>>>> SQLException {
>>>>>>>>> +        if (field != null) {
>>>>>>>>> +            _ps.setObject(_ind, field, Types.TIMESTAMP);
>>>>>>>>> +        } else {
>>>>>>>>> +            _ps.setNull(_ind, Types.TIMESTAMP);
>>>>>>>>> +        }
>>>>>>>>> +        _ind++;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    /**
>>>>>>>>> +     * Set the next binding variable of the currently active
>>>>>>>>> prepared
>>>>>>>>> statement.
>>>>>>>>> +     *
>>>>>>>>> +     * @param field
>>>>>>>>> +     *
>>>>>>>>> +     * @throws SQLException
>>>>>>>>> +     */
>>>>>>>>> public void setValue(java.sql.Time field) throws SQLException {
>>>>>>>>>    if (field != null) {
>>>>>>>>>        _ps.setTime(_ind, field);
>>>>>>>>> Index: framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>> ===================================================================
>>>>>>>>> --- framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>> (revision
>>>>>>>>> 703572)
>>>>>>>>> +++ framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>> (working copy)
>>>>>>>>> @@ -731,6 +731,9 @@
>>>>>>>>>                fieldClassName = "byte[]";
>>>>>>>>>            }
>>>>>>>>>
>>>>>>>>> +                if ("java.sql.Timestamp".equals(fieldType)) {
>>>>>>>>> +                    fieldClassName = fieldType;
>>>>>>>>> +                }
>>>>>>>>>            if (Debug.verboseOn()) Debug.logVerbose("type of
>>>>>>>>> field " + entityName + "." + modelField.getName() +
>>>>>>>>>                    " is " + fieldClassName + ", was expecting "
>>>>>>>>> + mft.getJavaType() + "; this may " +
>>>>>>>>>                    "indicate an error in the configuration or in
>>>>>>>>> the class, and may result " +
>>>>>>>>> @@ -749,7 +752,11 @@
>>>>>>>>>            break;
>>>>>>>>>
>>>>>>>>>        case 2:
>>>>>>>>> -                sqlP.setValue((java.sql.Timestamp) fieldValue);
>>>>>>>>> +                if (fieldValue instanceof String) {
>>>>>>>>> +                    sqlP.setValueTimestampString((String)
>>>>>>>>> fieldValue);
>>>>>>>>> +                } else {
>>>>>>>>> +                    sqlP.setValue((java.sql.Timestamp)
>>>>>>>>> fieldValue);
>>>>>>>>> +                }
>>>>>>>>>            break;
>>>>>>>>>
>>>>>>>>>        case 3:
>>>>>>>>>
>>>>>>>>> Regards
>>>>>>>>> Scott
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 2008/10/13 Jacques Le Roux <[hidden email]>:
>>>>>>>>>>
>>>>>>>>>> Done in revision: 703816
>>>>>>>>>> It was not possible for PackingSlip.groovy and
>>>>>>>>>> FindInventoryEventPlan.groovy. Because there the date string is
>>>>>>>>>> build
>>>>>>>>>> dynamically in the Groovy file
>>>>>>>>>>
>>>>>>>>>> Jacques
>>>>>>>>>>
>>>>>>>>>> From: "Jacques Le Roux" <[hidden email]>
>>>>>>>>>>>
>>>>>>>>>>> Adrian,
>>>>>>>>>>>
>>>>>>>>>>> Yes good idea indeed, I will do that
>>>>>>>>>>>
>>>>>>>>>>> Jacques
>>>>>>>>>>>
>>>>>>>>>>> From: "Adrian Crum" <[hidden email]>
>>>>>>>>>>>>
>>>>>>>>>>>> Jacques,
>>>>>>>>>>>>
>>>>>>>>>>>> Instead of modifying the groovy files, try specifying the data
>>>>>>>>>>>> type in
>>>>>>>>>>>> the screen widget.
>>>>>>>>>>>>
>>>>>>>>>>>> Example in ReportFinancialSummaryScreens.xml:
>>>>>>>>>>>>
>>>>>>>>>>>> <set field="fromDate" from-field="parameters.fromDate"
>>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>>> <set field="thruDate" from-field="parameters.thruDate"
>>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>>> <script
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> location="component://accounting/webapp/accounting/WEB-INF/actions/reports/TransactionTotals.groovy"/>
>>>>>>>>>>>>
>>>>>>>>>>>> -Adrian
>>>>>>>>>>
>>>>>>>>>>
>>>>
>>>>
>>>
>
>

[David E Jones]

On Oct 13, 2008, at 10:10 PM, Scott Gray wrote:

> Inline:
>
> 2008/10/14 David E Jones <[hidden email]>:
>>
>> Scott, are you referring to the call to PreparedStatement.setString  
>> on
>> SQLProcessor line 553? In other words, the difference in the  
>> Postgres JDBC
>> driver is that they used to auto-convert from a String to whatever  
>> the
>> database needs in the setString method, but now they don't?
>
> When we call PreparedStatement.setString() the JDBC driver now adds
> something like String:varchar to the parameter list where it used to
> add String:unspecified which meant the database would try and take
> care of it, but with varchar it just says "wrong!" and throws an
> error.  However if we call PreparedStatement.setObject() passing in
> the correct sql type then we get String:timestamp and everything works
> fine.
>
> So I guess what I'm saying is that I think java type conversion
> (String vs Timestamp) is a separate topic and the problem here is
> simply the sql type that we are specifying (by calling the various
> PreparedStatement methods) does not match the column type.  We could
> probably solve the postgresql issue by simply calling
> setObject(Types.OTHER) whenever the value type doesn't match entity
> field type.

The real problem is we're passing bad data back to the database and  
expecting it to deal with it. Right now the entity engine is just  
warning about such problems in the log, allowing them to go back to  
the JDBC driver and/or database where they become a real problem.

If we do something like always call setObject then chances are good  
we'll have problems with other databases. If I remember right we used  
to do that generically and it caused problems and we had to move to  
the current approach of using object time specific methods.

>> What I'm proposing is to fix this further up the stack in the
>> GenericEntity.set method instead of lower down.
>
> That's fine for when we are using the GenericEntity but we'd also need
> to add similar code to the EntityCondition stuff for find queries
> (which is where Jacques noticed the problem, I haven't checked for the
> error on any operations involving GenericEntity).

Good point, we'll have to make sure the object types passed into those  
are valid as well.

-David

>> On Oct 13, 2008, at 7:14 PM, Scott Gray wrote:
>>
>>> Here's the problem as I see it:
>>> Postgresql used to allow you to specify parameter types which did  
>>> not
>>> match the column type, in the timestamp case if you passed in a
>>> parameter specified as varchar it would automatically attempt to
>>> convert it to a timestamp.  Now Postgresql requires that you either
>>> pass in the correct parameter type for the column or otherwise pass
>>> the parameter type as unknown/unspecified.  Postgresql still doesn't
>>> care whether you pass in a string or a timestamp but you MUST  
>>> specify
>>> the correct sql type.
>>>
>>> If letting the database take care of the type conversion has never
>>> been a problem before, why do we need to worry about it now?  
>>> Remember
>>> the only problem here is that we are passing in a string specified  
>>> as
>>> varchar instead of a string specified as java.sql.Types.Timestamp.
>>>
>>> Regards
>>> Scott
>>>
>>> 2008/10/14 Adrian Crum <[hidden email]>:
>>>>
>>>> I understood your alternatives. I apologize for being unclear.
>>>>
>>>> You said, "or to just throw an exception when the wrong data type  
>>>> is
>>>> passed
>>>> like the commented out code in GenericEntity referenced above  
>>>> does."
>>>>
>>>> And I'm saying we could do that, but it's going to get interesting
>>>> because a
>>>> lot of code passes Strings into the entity engine. In other  
>>>> words, if we
>>>> throw a "hard" exception, there is a good chance that much of  
>>>> OFBiz won't
>>>> run.
>>>>
>>>> Personally, I'd like to see better handling of object types in  
>>>> the higher
>>>> level code. I fixed a problem over the weekend that was caused by  
>>>> this
>>>> very
>>>> thing (passing Strings into the entity engine).
>>>>
>>>> -Adrian
>>>>
>>>>
>>>> David E Jones wrote:
>>>>>
>>>>> I'm referring to the exception I described in the  
>>>>> GenericEntity.java
>>>>> file,
>>>>> lines 410-415. Right now it is just a warning in the log (and  
>>>>> has been
>>>>> for
>>>>> years). The reason to do it there instead of letting the JDBC  
>>>>> driver do
>>>>> it
>>>>> is that not all developers will test on all possible databases,  
>>>>> and this
>>>>> will help avoid errors as people use different databases in
>>>>> development/testing or deployment.
>>>>>
>>>>> I tried to describe two possible approaches to improve this  
>>>>> situation in
>>>>> my first message. Please let me know if those alternatives were  
>>>>> not
>>>>> clear
>>>>> and I'll try to explain them better.
>>>>>
>>>>> -David
>>>>>
>>>>>
>>>>> On Oct 13, 2008, at 5:22 PM, Adrian Crum wrote:
>>>>>
>>>>>> Wasn't that being done already? Jacques started these changes  
>>>>>> based on
>>>>>> exceptions being thrown by his JDBC driver. Other DBs seemed to  
>>>>>> be
>>>>>> working
>>>>>> okay.
>>>>>>
>>>>>> I guess we could throw our own exception to keep things  
>>>>>> consistent
>>>>>> across
>>>>>> databases. It will be interesting to see how that affects  
>>>>>> existing
>>>>>> code.
>>>>>>
>>>>>> -Adrian
>>>>>>
>>>>>>
>>>>>> David E Jones wrote:
>>>>>>>
>>>>>>> That is a very good point, and I agree.
>>>>>>> To me that means we go with the fail-fast approach and have it  
>>>>>>> throw
>>>>>>> an
>>>>>>> exception if you pass in something that is not the correct  
>>>>>>> object
>>>>>>> type.
>>>>>>> Is that what you meant?
>>>>>>> -David
>>>>>>> On Oct 13, 2008, at 4:57 PM, Adrian Crum wrote:
>>>>>>>>
>>>>>>>> I would prefer handling object types in higher level code -  
>>>>>>>> due to
>>>>>>>> ambiguity in how some types would be converted from a String
>>>>>>>> (currency,
>>>>>>>> date, time, etc).
>>>>>>>>
>>>>>>>> -Adrian
>>>>>>>>
>>>>>>>> David E Jones wrote:
>>>>>>>>>
>>>>>>>>> This has been a potential problem for a while, but the  
>>>>>>>>> decision was
>>>>>>>>> made quite a while back to not enforce the correct object  
>>>>>>>>> type.
>>>>>>>>> There is code in the GenericEntity.java file (lines 410-415)  
>>>>>>>>> to
>>>>>>>>> check
>>>>>>>>> to see if the value passed in matches the object type for  
>>>>>>>>> the field
>>>>>>>>> it is
>>>>>>>>> being set on.
>>>>>>>>> The best way to solve this problem is a good question. It  
>>>>>>>>> might be
>>>>>>>>> good to introduce some automatic type conversion, or to just  
>>>>>>>>> throw
>>>>>>>>> an
>>>>>>>>> exception when the wrong data type is passed like the  
>>>>>>>>> commented out
>>>>>>>>> code in
>>>>>>>>> GenericEntity referenced above does.
>>>>>>>>> The most common form of automatic conversion is from String to
>>>>>>>>> whatever the field's object type is, and that is done by the
>>>>>>>>> GenericEntity.setString method.
>>>>>>>>> So, what do people think about this? Should we do an  
>>>>>>>>> automatic type
>>>>>>>>> conversion to try to fix programming errors automatically,  
>>>>>>>>> or do a
>>>>>>>>> fail-fast
>>>>>>>>> by throwing an exception when the object type is wrong? I  
>>>>>>>>> suppose
>>>>>>>>> the
>>>>>>>>> current fail-quiet (in the log) and fail loudly on certain  
>>>>>>>>> databases
>>>>>>>>> later
>>>>>>>>> is not the best option...
>>>>>>>>> -David
>>>>>>>>> On Oct 12, 2008, at 2:20 PM, Scott Gray wrote:
>>>>>>>>>>
>>>>>>>>>> I've had a look into this and I can't see anything related to
>>>>>>>>>> Groovy
>>>>>>>>>> that is making this necessary, it appears to be entirely a
>>>>>>>>>> postgresql
>>>>>>>>>> issue.
>>>>>>>>>>
>>>>>>>>>> When executing a prepared statement postgresql seems to  
>>>>>>>>>> require
>>>>>>>>>> that
>>>>>>>>>> the parameter list sql types match the column types.  So the
>>>>>>>>>> problem
>>>>>>>>>> isn't that we are passing in a string but that we are  
>>>>>>>>>> setting the
>>>>>>>>>> sql
>>>>>>>>>> type to character varying by using  
>>>>>>>>>> PreparedStatement.setString().
>>>>>>>>>>
>>>>>>>>>> Here's a patch that fixes the issue but I'm not really  
>>>>>>>>>> confident
>>>>>>>>>> enough to commit it, it would be great to get some comments  
>>>>>>>>>> from
>>>>>>>>>> people who know more about this kind of thing:
>>>>>>>>>>
>>>>>>>>>> Index: framework/entity/src/org/ofbiz/entity/jdbc/
>>>>>>>>>> SQLProcessor.java
>>>>>>>>>> =
>>>>>>>>>> =
>>>>>>>>>> =
>>>>>>>>>> =
>>>>>>>>>> =
>>>>>>>>>> =
>>>>>>>>>> =============================================================
>>>>>>>>>> --- framework/entity/src/org/ofbiz/entity/jdbc/
>>>>>>>>>> SQLProcessor.java
>>>>>>>>>> (revision
>>>>>>>>>> 703572)
>>>>>>>>>> +++ framework/entity/src/org/ofbiz/entity/jdbc/
>>>>>>>>>> SQLProcessor.java
>>>>>>>>>> (working copy)
>>>>>>>>>> @@ -592,6 +592,22 @@
>>>>>>>>>> *
>>>>>>>>>> * @throws SQLException
>>>>>>>>>> */
>>>>>>>>>> +    public void setValueTimestampString(String field) throws
>>>>>>>>>> SQLException {
>>>>>>>>>> +        if (field != null) {
>>>>>>>>>> +            _ps.setObject(_ind, field, Types.TIMESTAMP);
>>>>>>>>>> +        } else {
>>>>>>>>>> +            _ps.setNull(_ind, Types.TIMESTAMP);
>>>>>>>>>> +        }
>>>>>>>>>> +        _ind++;
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>> +    /**
>>>>>>>>>> +     * Set the next binding variable of the currently active
>>>>>>>>>> prepared
>>>>>>>>>> statement.
>>>>>>>>>> +     *
>>>>>>>>>> +     * @param field
>>>>>>>>>> +     *
>>>>>>>>>> +     * @throws SQLException
>>>>>>>>>> +     */
>>>>>>>>>> public void setValue(java.sql.Time field) throws  
>>>>>>>>>> SQLException {
>>>>>>>>>>   if (field != null) {
>>>>>>>>>>       _ps.setTime(_ind, field);
>>>>>>>>>> Index: framework/entity/src/org/ofbiz/entity/jdbc/
>>>>>>>>>> SqlJdbcUtil.java
>>>>>>>>>> =
>>>>>>>>>> =
>>>>>>>>>> =
>>>>>>>>>> =
>>>>>>>>>> =
>>>>>>>>>> =
>>>>>>>>>> =============================================================
>>>>>>>>>> --- framework/entity/src/org/ofbiz/entity/jdbc/
>>>>>>>>>> SqlJdbcUtil.java
>>>>>>>>>> (revision
>>>>>>>>>> 703572)
>>>>>>>>>> +++ framework/entity/src/org/ofbiz/entity/jdbc/
>>>>>>>>>> SqlJdbcUtil.java
>>>>>>>>>> (working copy)
>>>>>>>>>> @@ -731,6 +731,9 @@
>>>>>>>>>>               fieldClassName = "byte[]";
>>>>>>>>>>           }
>>>>>>>>>>
>>>>>>>>>> +                if  
>>>>>>>>>> ("java.sql.Timestamp".equals(fieldType)) {
>>>>>>>>>> +                    fieldClassName = fieldType;
>>>>>>>>>> +                }
>>>>>>>>>>           if (Debug.verboseOn()) Debug.logVerbose("type of
>>>>>>>>>> field " + entityName + "." + modelField.getName() +
>>>>>>>>>>                   " is " + fieldClassName + ", was  
>>>>>>>>>> expecting "
>>>>>>>>>> + mft.getJavaType() + "; this may " +
>>>>>>>>>>                   "indicate an error in the configuration  
>>>>>>>>>> or in
>>>>>>>>>> the class, and may result " +
>>>>>>>>>> @@ -749,7 +752,11 @@
>>>>>>>>>>           break;
>>>>>>>>>>
>>>>>>>>>>       case 2:
>>>>>>>>>> -                sqlP.setValue((java.sql.Timestamp)  
>>>>>>>>>> fieldValue);
>>>>>>>>>> +                if (fieldValue instanceof String) {
>>>>>>>>>> +                    sqlP.setValueTimestampString((String)
>>>>>>>>>> fieldValue);
>>>>>>>>>> +                } else {
>>>>>>>>>> +                    sqlP.setValue((java.sql.Timestamp)
>>>>>>>>>> fieldValue);
>>>>>>>>>> +                }
>>>>>>>>>>           break;
>>>>>>>>>>
>>>>>>>>>>       case 3:
>>>>>>>>>>
>>>>>>>>>> Regards
>>>>>>>>>> Scott
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 2008/10/13 Jacques Le Roux <[hidden email]>:
>>>>>>>>>>>
>>>>>>>>>>> Done in revision: 703816
>>>>>>>>>>> It was not possible for PackingSlip.groovy and
>>>>>>>>>>> FindInventoryEventPlan.groovy. Because there the date  
>>>>>>>>>>> string is
>>>>>>>>>>> build
>>>>>>>>>>> dynamically in the Groovy file
>>>>>>>>>>>
>>>>>>>>>>> Jacques
>>>>>>>>>>>
>>>>>>>>>>> From: "Jacques Le Roux" <[hidden email]>
>>>>>>>>>>>>
>>>>>>>>>>>> Adrian,
>>>>>>>>>>>>
>>>>>>>>>>>> Yes good idea indeed, I will do that
>>>>>>>>>>>>
>>>>>>>>>>>> Jacques
>>>>>>>>>>>>
>>>>>>>>>>>> From: "Adrian Crum" <[hidden email]>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Jacques,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Instead of modifying the groovy files, try specifying  
>>>>>>>>>>>>> the data
>>>>>>>>>>>>> type in
>>>>>>>>>>>>> the screen widget.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Example in ReportFinancialSummaryScreens.xml:
>>>>>>>>>>>>>
>>>>>>>>>>>>> <set field="fromDate" from-field="parameters.fromDate"
>>>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>>>> <set field="thruDate" from-field="parameters.thruDate"
>>>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>>>> <script
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> location="component://accounting/webapp/accounting/WEB-
>>>>>>>>>>>>> INF/actions/reports/TransactionTotals.groovy"/>
>>>>>>>>>>>>>
>>>>>>>>>>>>> -Adrian
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>
>>>>>
>>>>
>>
>>

[Scott Gray]

Ok I see your point, thanks for explaining.

#1 makes sense but it will be interesting to see how many bugs are
created, also doesn't the service engine currently perform automatic
conversion without any problems?

Regards
Scott

2008/10/14 David E Jones <[hidden email]>:

>
> On Oct 13, 2008, at 10:10 PM, Scott Gray wrote:
>
>> Inline:
>>
>> 2008/10/14 David E Jones <[hidden email]>:
>>>
>>> Scott, are you referring to the call to PreparedStatement.setString on
>>> SQLProcessor line 553? In other words, the difference in the Postgres
>>> JDBC
>>> driver is that they used to auto-convert from a String to whatever the
>>> database needs in the setString method, but now they don't?
>>
>> When we call PreparedStatement.setString() the JDBC driver now adds
>> something like String:varchar to the parameter list where it used to
>> add String:unspecified which meant the database would try and take
>> care of it, but with varchar it just says "wrong!" and throws an
>> error.  However if we call PreparedStatement.setObject() passing in
>> the correct sql type then we get String:timestamp and everything works
>> fine.
>>
>> So I guess what I'm saying is that I think java type conversion
>> (String vs Timestamp) is a separate topic and the problem here is
>> simply the sql type that we are specifying (by calling the various
>> PreparedStatement methods) does not match the column type.  We could
>> probably solve the postgresql issue by simply calling
>> setObject(Types.OTHER) whenever the value type doesn't match entity
>> field type.
>
> The real problem is we're passing bad data back to the database and
> expecting it to deal with it. Right now the entity engine is just warning
> about such problems in the log, allowing them to go back to the JDBC driver
> and/or database where they become a real problem.
>
> If we do something like always call setObject then chances are good we'll
> have problems with other databases. If I remember right we used to do that
> generically and it caused problems and we had to move to the current
> approach of using object time specific methods.
>
>>> What I'm proposing is to fix this further up the stack in the
>>> GenericEntity.set method instead of lower down.
>>
>> That's fine for when we are using the GenericEntity but we'd also need
>> to add similar code to the EntityCondition stuff for find queries
>> (which is where Jacques noticed the problem, I haven't checked for the
>> error on any operations involving GenericEntity).
>
> Good point, we'll have to make sure the object types passed into those are
> valid as well.
>
> -David
>
>
>
>>> On Oct 13, 2008, at 7:14 PM, Scott Gray wrote:
>>>
>>>> Here's the problem as I see it:
>>>> Postgresql used to allow you to specify parameter types which did not
>>>> match the column type, in the timestamp case if you passed in a
>>>> parameter specified as varchar it would automatically attempt to
>>>> convert it to a timestamp.  Now Postgresql requires that you either
>>>> pass in the correct parameter type for the column or otherwise pass
>>>> the parameter type as unknown/unspecified.  Postgresql still doesn't
>>>> care whether you pass in a string or a timestamp but you MUST specify
>>>> the correct sql type.
>>>>
>>>> If letting the database take care of the type conversion has never
>>>> been a problem before, why do we need to worry about it now?  Remember
>>>> the only problem here is that we are passing in a string specified as
>>>> varchar instead of a string specified as java.sql.Types.Timestamp.
>>>>
>>>> Regards
>>>> Scott
>>>>
>>>> 2008/10/14 Adrian Crum <[hidden email]>:
>>>>>
>>>>> I understood your alternatives. I apologize for being unclear.
>>>>>
>>>>> You said, "or to just throw an exception when the wrong data type is
>>>>> passed
>>>>> like the commented out code in GenericEntity referenced above does."
>>>>>
>>>>> And I'm saying we could do that, but it's going to get interesting
>>>>> because a
>>>>> lot of code passes Strings into the entity engine. In other words, if
>>>>> we
>>>>> throw a "hard" exception, there is a good chance that much of OFBiz
>>>>> won't
>>>>> run.
>>>>>
>>>>> Personally, I'd like to see better handling of object types in the
>>>>> higher
>>>>> level code. I fixed a problem over the weekend that was caused by this
>>>>> very
>>>>> thing (passing Strings into the entity engine).
>>>>>
>>>>> -Adrian
>>>>>
>>>>>
>>>>> David E Jones wrote:
>>>>>>
>>>>>> I'm referring to the exception I described in the GenericEntity.java
>>>>>> file,
>>>>>> lines 410-415. Right now it is just a warning in the log (and has been
>>>>>> for
>>>>>> years). The reason to do it there instead of letting the JDBC driver
>>>>>> do
>>>>>> it
>>>>>> is that not all developers will test on all possible databases, and
>>>>>> this
>>>>>> will help avoid errors as people use different databases in
>>>>>> development/testing or deployment.
>>>>>>
>>>>>> I tried to describe two possible approaches to improve this situation
>>>>>> in
>>>>>> my first message. Please let me know if those alternatives were not
>>>>>> clear
>>>>>> and I'll try to explain them better.
>>>>>>
>>>>>> -David
>>>>>>
>>>>>>
>>>>>> On Oct 13, 2008, at 5:22 PM, Adrian Crum wrote:
>>>>>>
>>>>>>> Wasn't that being done already? Jacques started these changes based
>>>>>>> on
>>>>>>> exceptions being thrown by his JDBC driver. Other DBs seemed to be
>>>>>>> working
>>>>>>> okay.
>>>>>>>
>>>>>>> I guess we could throw our own exception to keep things consistent
>>>>>>> across
>>>>>>> databases. It will be interesting to see how that affects existing
>>>>>>> code.
>>>>>>>
>>>>>>> -Adrian
>>>>>>>
>>>>>>>
>>>>>>> David E Jones wrote:
>>>>>>>>
>>>>>>>> That is a very good point, and I agree.
>>>>>>>> To me that means we go with the fail-fast approach and have it throw
>>>>>>>> an
>>>>>>>> exception if you pass in something that is not the correct object
>>>>>>>> type.
>>>>>>>> Is that what you meant?
>>>>>>>> -David
>>>>>>>> On Oct 13, 2008, at 4:57 PM, Adrian Crum wrote:
>>>>>>>>>
>>>>>>>>> I would prefer handling object types in higher level code - due to
>>>>>>>>> ambiguity in how some types would be converted from a String
>>>>>>>>> (currency,
>>>>>>>>> date, time, etc).
>>>>>>>>>
>>>>>>>>> -Adrian
>>>>>>>>>
>>>>>>>>> David E Jones wrote:
>>>>>>>>>>
>>>>>>>>>> This has been a potential problem for a while, but the decision
>>>>>>>>>> was
>>>>>>>>>> made quite a while back to not enforce the correct object type.
>>>>>>>>>> There is code in the GenericEntity.java file (lines 410-415) to
>>>>>>>>>> check
>>>>>>>>>> to see if the value passed in matches the object type for the
>>>>>>>>>> field
>>>>>>>>>> it is
>>>>>>>>>> being set on.
>>>>>>>>>> The best way to solve this problem is a good question. It might be
>>>>>>>>>> good to introduce some automatic type conversion, or to just throw
>>>>>>>>>> an
>>>>>>>>>> exception when the wrong data type is passed like the commented
>>>>>>>>>> out
>>>>>>>>>> code in
>>>>>>>>>> GenericEntity referenced above does.
>>>>>>>>>> The most common form of automatic conversion is from String to
>>>>>>>>>> whatever the field's object type is, and that is done by the
>>>>>>>>>> GenericEntity.setString method.
>>>>>>>>>> So, what do people think about this? Should we do an automatic
>>>>>>>>>> type
>>>>>>>>>> conversion to try to fix programming errors automatically, or do a
>>>>>>>>>> fail-fast
>>>>>>>>>> by throwing an exception when the object type is wrong? I suppose
>>>>>>>>>> the
>>>>>>>>>> current fail-quiet (in the log) and fail loudly on certain
>>>>>>>>>> databases
>>>>>>>>>> later
>>>>>>>>>> is not the best option...
>>>>>>>>>> -David
>>>>>>>>>> On Oct 12, 2008, at 2:20 PM, Scott Gray wrote:
>>>>>>>>>>>
>>>>>>>>>>> I've had a look into this and I can't see anything related to
>>>>>>>>>>> Groovy
>>>>>>>>>>> that is making this necessary, it appears to be entirely a
>>>>>>>>>>> postgresql
>>>>>>>>>>> issue.
>>>>>>>>>>>
>>>>>>>>>>> When executing a prepared statement postgresql seems to require
>>>>>>>>>>> that
>>>>>>>>>>> the parameter list sql types match the column types.  So the
>>>>>>>>>>> problem
>>>>>>>>>>> isn't that we are passing in a string but that we are setting the
>>>>>>>>>>> sql
>>>>>>>>>>> type to character varying by using PreparedStatement.setString().
>>>>>>>>>>>
>>>>>>>>>>> Here's a patch that fixes the issue but I'm not really confident
>>>>>>>>>>> enough to commit it, it would be great to get some comments from
>>>>>>>>>>> people who know more about this kind of thing:
>>>>>>>>>>>
>>>>>>>>>>> Index:
>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SQLProcessor.java
>>>>>>>>>>>
>>>>>>>>>>> ===================================================================
>>>>>>>>>>> --- framework/entity/src/org/ofbiz/entity/jdbc/SQLProcessor.java
>>>>>>>>>>> (revision
>>>>>>>>>>> 703572)
>>>>>>>>>>> +++ framework/entity/src/org/ofbiz/entity/jdbc/SQLProcessor.java
>>>>>>>>>>> (working copy)
>>>>>>>>>>> @@ -592,6 +592,22 @@
>>>>>>>>>>> *
>>>>>>>>>>> * @throws SQLException
>>>>>>>>>>> */
>>>>>>>>>>> +    public void setValueTimestampString(String field) throws
>>>>>>>>>>> SQLException {
>>>>>>>>>>> +        if (field != null) {
>>>>>>>>>>> +            _ps.setObject(_ind, field, Types.TIMESTAMP);
>>>>>>>>>>> +        } else {
>>>>>>>>>>> +            _ps.setNull(_ind, Types.TIMESTAMP);
>>>>>>>>>>> +        }
>>>>>>>>>>> +        _ind++;
>>>>>>>>>>> +    }
>>>>>>>>>>> +
>>>>>>>>>>> +    /**
>>>>>>>>>>> +     * Set the next binding variable of the currently active
>>>>>>>>>>> prepared
>>>>>>>>>>> statement.
>>>>>>>>>>> +     *
>>>>>>>>>>> +     * @param field
>>>>>>>>>>> +     *
>>>>>>>>>>> +     * @throws SQLException
>>>>>>>>>>> +     */
>>>>>>>>>>> public void setValue(java.sql.Time field) throws SQLException {
>>>>>>>>>>>  if (field != null) {
>>>>>>>>>>>      _ps.setTime(_ind, field);
>>>>>>>>>>> Index:
>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>>>>
>>>>>>>>>>> ===================================================================
>>>>>>>>>>> --- framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>>>> (revision
>>>>>>>>>>> 703572)
>>>>>>>>>>> +++ framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>>>> (working copy)
>>>>>>>>>>> @@ -731,6 +731,9 @@
>>>>>>>>>>>              fieldClassName = "byte[]";
>>>>>>>>>>>          }
>>>>>>>>>>>
>>>>>>>>>>> +                if ("java.sql.Timestamp".equals(fieldType)) {
>>>>>>>>>>> +                    fieldClassName = fieldType;
>>>>>>>>>>> +                }
>>>>>>>>>>>          if (Debug.verboseOn()) Debug.logVerbose("type of
>>>>>>>>>>> field " + entityName + "." + modelField.getName() +
>>>>>>>>>>>                  " is " + fieldClassName + ", was expecting "
>>>>>>>>>>> + mft.getJavaType() + "; this may " +
>>>>>>>>>>>                  "indicate an error in the configuration or in
>>>>>>>>>>> the class, and may result " +
>>>>>>>>>>> @@ -749,7 +752,11 @@
>>>>>>>>>>>          break;
>>>>>>>>>>>
>>>>>>>>>>>      case 2:
>>>>>>>>>>> -                sqlP.setValue((java.sql.Timestamp) fieldValue);
>>>>>>>>>>> +                if (fieldValue instanceof String) {
>>>>>>>>>>> +                    sqlP.setValueTimestampString((String)
>>>>>>>>>>> fieldValue);
>>>>>>>>>>> +                } else {
>>>>>>>>>>> +                    sqlP.setValue((java.sql.Timestamp)
>>>>>>>>>>> fieldValue);
>>>>>>>>>>> +                }
>>>>>>>>>>>          break;
>>>>>>>>>>>
>>>>>>>>>>>      case 3:
>>>>>>>>>>>
>>>>>>>>>>> Regards
>>>>>>>>>>> Scott
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 2008/10/13 Jacques Le Roux <[hidden email]>:
>>>>>>>>>>>>
>>>>>>>>>>>> Done in revision: 703816
>>>>>>>>>>>> It was not possible for PackingSlip.groovy and
>>>>>>>>>>>> FindInventoryEventPlan.groovy. Because there the date string is
>>>>>>>>>>>> build
>>>>>>>>>>>> dynamically in the Groovy file
>>>>>>>>>>>>
>>>>>>>>>>>> Jacques
>>>>>>>>>>>>
>>>>>>>>>>>> From: "Jacques Le Roux" <[hidden email]>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Adrian,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yes good idea indeed, I will do that
>>>>>>>>>>>>>
>>>>>>>>>>>>> Jacques
>>>>>>>>>>>>>
>>>>>>>>>>>>> From: "Adrian Crum" <[hidden email]>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Jacques,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Instead of modifying the groovy files, try specifying the data
>>>>>>>>>>>>>> type in
>>>>>>>>>>>>>> the screen widget.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Example in ReportFinancialSummaryScreens.xml:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> <set field="fromDate" from-field="parameters.fromDate"
>>>>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>>>>> <set field="thruDate" from-field="parameters.thruDate"
>>>>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>>>>> <script
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> location="component://accounting/webapp/accounting/WEB-INF/actions/reports/TransactionTotals.groovy"/>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> -Adrian
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>
>>>
>
>

[David E Jones]

I wouldn't say that bugs would be "created" by this change, more than  
existing bugs would be exposed (that are only visible on certain  
databases, etc).

The service engine doesn't actually do automated mapping, is the  
service event handler that does this for web requests before the  
actual call to the service engine. In fact when a service is called  
through the service engine it will return an error if the object types  
of values passed in don't match what is in the service definition. The  
difference is that it was that way from the beginning, which isn't the  
case with certain parts of the entity engine (mainly in order to not  
cause the problem of exposing bugs that were dormant most of the  
time... which isn't really good in my current opinion).

-David


On Oct 14, 2008, at 2:09 AM, Scott Gray wrote:

> Ok I see your point, thanks for explaining.
>
> #1 makes sense but it will be interesting to see how many bugs are
> created, also doesn't the service engine currently perform automatic
> conversion without any problems?
>
> Regards
> Scott
>
> 2008/10/14 David E Jones <[hidden email]>:
>>
>> On Oct 13, 2008, at 10:10 PM, Scott Gray wrote:
>>
>>> Inline:
>>>
>>> 2008/10/14 David E Jones <[hidden email]>:
>>>>
>>>> Scott, are you referring to the call to  
>>>> PreparedStatement.setString on
>>>> SQLProcessor line 553? In other words, the difference in the  
>>>> Postgres
>>>> JDBC
>>>> driver is that they used to auto-convert from a String to  
>>>> whatever the
>>>> database needs in the setString method, but now they don't?
>>>
>>> When we call PreparedStatement.setString() the JDBC driver now adds
>>> something like String:varchar to the parameter list where it used to
>>> add String:unspecified which meant the database would try and take
>>> care of it, but with varchar it just says "wrong!" and throws an
>>> error.  However if we call PreparedStatement.setObject() passing in
>>> the correct sql type then we get String:timestamp and everything  
>>> works
>>> fine.
>>>
>>> So I guess what I'm saying is that I think java type conversion
>>> (String vs Timestamp) is a separate topic and the problem here is
>>> simply the sql type that we are specifying (by calling the various
>>> PreparedStatement methods) does not match the column type.  We could
>>> probably solve the postgresql issue by simply calling
>>> setObject(Types.OTHER) whenever the value type doesn't match entity
>>> field type.
>>
>> The real problem is we're passing bad data back to the database and
>> expecting it to deal with it. Right now the entity engine is just  
>> warning
>> about such problems in the log, allowing them to go back to the  
>> JDBC driver
>> and/or database where they become a real problem.
>>
>> If we do something like always call setObject then chances are good  
>> we'll
>> have problems with other databases. If I remember right we used to  
>> do that
>> generically and it caused problems and we had to move to the current
>> approach of using object time specific methods.
>>
>>>> What I'm proposing is to fix this further up the stack in the
>>>> GenericEntity.set method instead of lower down.
>>>
>>> That's fine for when we are using the GenericEntity but we'd also  
>>> need
>>> to add similar code to the EntityCondition stuff for find queries
>>> (which is where Jacques noticed the problem, I haven't checked for  
>>> the
>>> error on any operations involving GenericEntity).
>>
>> Good point, we'll have to make sure the object types passed into  
>> those are
>> valid as well.
>>
>> -David
>>
>>
>>
>>>> On Oct 13, 2008, at 7:14 PM, Scott Gray wrote:
>>>>
>>>>> Here's the problem as I see it:
>>>>> Postgresql used to allow you to specify parameter types which  
>>>>> did not
>>>>> match the column type, in the timestamp case if you passed in a
>>>>> parameter specified as varchar it would automatically attempt to
>>>>> convert it to a timestamp.  Now Postgresql requires that you  
>>>>> either
>>>>> pass in the correct parameter type for the column or otherwise  
>>>>> pass
>>>>> the parameter type as unknown/unspecified.  Postgresql still  
>>>>> doesn't
>>>>> care whether you pass in a string or a timestamp but you MUST  
>>>>> specify
>>>>> the correct sql type.
>>>>>
>>>>> If letting the database take care of the type conversion has never
>>>>> been a problem before, why do we need to worry about it now?  
>>>>> Remember
>>>>> the only problem here is that we are passing in a string  
>>>>> specified as
>>>>> varchar instead of a string specified as java.sql.Types.Timestamp.
>>>>>
>>>>> Regards
>>>>> Scott
>>>>>
>>>>> 2008/10/14 Adrian Crum <[hidden email]>:
>>>>>>
>>>>>> I understood your alternatives. I apologize for being unclear.
>>>>>>
>>>>>> You said, "or to just throw an exception when the wrong data  
>>>>>> type is
>>>>>> passed
>>>>>> like the commented out code in GenericEntity referenced above  
>>>>>> does."
>>>>>>
>>>>>> And I'm saying we could do that, but it's going to get  
>>>>>> interesting
>>>>>> because a
>>>>>> lot of code passes Strings into the entity engine. In other  
>>>>>> words, if
>>>>>> we
>>>>>> throw a "hard" exception, there is a good chance that much of  
>>>>>> OFBiz
>>>>>> won't
>>>>>> run.
>>>>>>
>>>>>> Personally, I'd like to see better handling of object types in  
>>>>>> the
>>>>>> higher
>>>>>> level code. I fixed a problem over the weekend that was caused  
>>>>>> by this
>>>>>> very
>>>>>> thing (passing Strings into the entity engine).
>>>>>>
>>>>>> -Adrian
>>>>>>
>>>>>>
>>>>>> David E Jones wrote:
>>>>>>>
>>>>>>> I'm referring to the exception I described in the  
>>>>>>> GenericEntity.java
>>>>>>> file,
>>>>>>> lines 410-415. Right now it is just a warning in the log (and  
>>>>>>> has been
>>>>>>> for
>>>>>>> years). The reason to do it there instead of letting the JDBC  
>>>>>>> driver
>>>>>>> do
>>>>>>> it
>>>>>>> is that not all developers will test on all possible  
>>>>>>> databases, and
>>>>>>> this
>>>>>>> will help avoid errors as people use different databases in
>>>>>>> development/testing or deployment.
>>>>>>>
>>>>>>> I tried to describe two possible approaches to improve this  
>>>>>>> situation
>>>>>>> in
>>>>>>> my first message. Please let me know if those alternatives  
>>>>>>> were not
>>>>>>> clear
>>>>>>> and I'll try to explain them better.
>>>>>>>
>>>>>>> -David
>>>>>>>
>>>>>>>
>>>>>>> On Oct 13, 2008, at 5:22 PM, Adrian Crum wrote:
>>>>>>>
>>>>>>>> Wasn't that being done already? Jacques started these changes  
>>>>>>>> based
>>>>>>>> on
>>>>>>>> exceptions being thrown by his JDBC driver. Other DBs seemed  
>>>>>>>> to be
>>>>>>>> working
>>>>>>>> okay.
>>>>>>>>
>>>>>>>> I guess we could throw our own exception to keep things  
>>>>>>>> consistent
>>>>>>>> across
>>>>>>>> databases. It will be interesting to see how that affects  
>>>>>>>> existing
>>>>>>>> code.
>>>>>>>>
>>>>>>>> -Adrian
>>>>>>>>
>>>>>>>>
>>>>>>>> David E Jones wrote:
>>>>>>>>>
>>>>>>>>> That is a very good point, and I agree.
>>>>>>>>> To me that means we go with the fail-fast approach and have  
>>>>>>>>> it throw
>>>>>>>>> an
>>>>>>>>> exception if you pass in something that is not the correct  
>>>>>>>>> object
>>>>>>>>> type.
>>>>>>>>> Is that what you meant?
>>>>>>>>> -David
>>>>>>>>> On Oct 13, 2008, at 4:57 PM, Adrian Crum wrote:
>>>>>>>>>>
>>>>>>>>>> I would prefer handling object types in higher level code -  
>>>>>>>>>> due to
>>>>>>>>>> ambiguity in how some types would be converted from a String
>>>>>>>>>> (currency,
>>>>>>>>>> date, time, etc).
>>>>>>>>>>
>>>>>>>>>> -Adrian
>>>>>>>>>>
>>>>>>>>>> David E Jones wrote:
>>>>>>>>>>>
>>>>>>>>>>> This has been a potential problem for a while, but the  
>>>>>>>>>>> decision
>>>>>>>>>>> was
>>>>>>>>>>> made quite a while back to not enforce the correct object  
>>>>>>>>>>> type.
>>>>>>>>>>> There is code in the GenericEntity.java file (lines  
>>>>>>>>>>> 410-415) to
>>>>>>>>>>> check
>>>>>>>>>>> to see if the value passed in matches the object type for  
>>>>>>>>>>> the
>>>>>>>>>>> field
>>>>>>>>>>> it is
>>>>>>>>>>> being set on.
>>>>>>>>>>> The best way to solve this problem is a good question. It  
>>>>>>>>>>> might be
>>>>>>>>>>> good to introduce some automatic type conversion, or to  
>>>>>>>>>>> just throw
>>>>>>>>>>> an
>>>>>>>>>>> exception when the wrong data type is passed like the  
>>>>>>>>>>> commented
>>>>>>>>>>> out
>>>>>>>>>>> code in
>>>>>>>>>>> GenericEntity referenced above does.
>>>>>>>>>>> The most common form of automatic conversion is from  
>>>>>>>>>>> String to
>>>>>>>>>>> whatever the field's object type is, and that is done by the
>>>>>>>>>>> GenericEntity.setString method.
>>>>>>>>>>> So, what do people think about this? Should we do an  
>>>>>>>>>>> automatic
>>>>>>>>>>> type
>>>>>>>>>>> conversion to try to fix programming errors automatically,  
>>>>>>>>>>> or do a
>>>>>>>>>>> fail-fast
>>>>>>>>>>> by throwing an exception when the object type is wrong? I  
>>>>>>>>>>> suppose
>>>>>>>>>>> the
>>>>>>>>>>> current fail-quiet (in the log) and fail loudly on certain
>>>>>>>>>>> databases
>>>>>>>>>>> later
>>>>>>>>>>> is not the best option...
>>>>>>>>>>> -David
>>>>>>>>>>> On Oct 12, 2008, at 2:20 PM, Scott Gray wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> I've had a look into this and I can't see anything  
>>>>>>>>>>>> related to
>>>>>>>>>>>> Groovy
>>>>>>>>>>>> that is making this necessary, it appears to be entirely a
>>>>>>>>>>>> postgresql
>>>>>>>>>>>> issue.
>>>>>>>>>>>>
>>>>>>>>>>>> When executing a prepared statement postgresql seems to  
>>>>>>>>>>>> require
>>>>>>>>>>>> that
>>>>>>>>>>>> the parameter list sql types match the column types.  So  
>>>>>>>>>>>> the
>>>>>>>>>>>> problem
>>>>>>>>>>>> isn't that we are passing in a string but that we are  
>>>>>>>>>>>> setting the
>>>>>>>>>>>> sql
>>>>>>>>>>>> type to character varying by using  
>>>>>>>>>>>> PreparedStatement.setString().
>>>>>>>>>>>>
>>>>>>>>>>>> Here's a patch that fixes the issue but I'm not really  
>>>>>>>>>>>> confident
>>>>>>>>>>>> enough to commit it, it would be great to get some  
>>>>>>>>>>>> comments from
>>>>>>>>>>>> people who know more about this kind of thing:
>>>>>>>>>>>>
>>>>>>>>>>>> Index:
>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/
>>>>>>>>>>>> SQLProcessor.java
>>>>>>>>>>>>
>>>>>>>>>>>> =
>>>>>>>>>>>> =
>>>>>>>>>>>> =
>>>>>>>>>>>> =
>>>>>>>>>>>> =
>>>>>>>>>>>> =
>>>>>>>>>>>> =
>>>>>>>>>>>> =
>>>>>>>>>>>> ===========================================================
>>>>>>>>>>>> --- framework/entity/src/org/ofbiz/entity/jdbc/
>>>>>>>>>>>> SQLProcessor.java
>>>>>>>>>>>> (revision
>>>>>>>>>>>> 703572)
>>>>>>>>>>>> +++ framework/entity/src/org/ofbiz/entity/jdbc/
>>>>>>>>>>>> SQLProcessor.java
>>>>>>>>>>>> (working copy)
>>>>>>>>>>>> @@ -592,6 +592,22 @@
>>>>>>>>>>>> *
>>>>>>>>>>>> * @throws SQLException
>>>>>>>>>>>> */
>>>>>>>>>>>> +    public void setValueTimestampString(String field)  
>>>>>>>>>>>> throws
>>>>>>>>>>>> SQLException {
>>>>>>>>>>>> +        if (field != null) {
>>>>>>>>>>>> +            _ps.setObject(_ind, field, Types.TIMESTAMP);
>>>>>>>>>>>> +        } else {
>>>>>>>>>>>> +            _ps.setNull(_ind, Types.TIMESTAMP);
>>>>>>>>>>>> +        }
>>>>>>>>>>>> +        _ind++;
>>>>>>>>>>>> +    }
>>>>>>>>>>>> +
>>>>>>>>>>>> +    /**
>>>>>>>>>>>> +     * Set the next binding variable of the currently  
>>>>>>>>>>>> active
>>>>>>>>>>>> prepared
>>>>>>>>>>>> statement.
>>>>>>>>>>>> +     *
>>>>>>>>>>>> +     * @param field
>>>>>>>>>>>> +     *
>>>>>>>>>>>> +     * @throws SQLException
>>>>>>>>>>>> +     */
>>>>>>>>>>>> public void setValue(java.sql.Time field) throws  
>>>>>>>>>>>> SQLException {
>>>>>>>>>>>> if (field != null) {
>>>>>>>>>>>>     _ps.setTime(_ind, field);
>>>>>>>>>>>> Index:
>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>>>>>
>>>>>>>>>>>> =
>>>>>>>>>>>> =
>>>>>>>>>>>> =
>>>>>>>>>>>> =
>>>>>>>>>>>> =
>>>>>>>>>>>> =
>>>>>>>>>>>> =
>>>>>>>>>>>> =
>>>>>>>>>>>> ===========================================================
>>>>>>>>>>>> --- framework/entity/src/org/ofbiz/entity/jdbc/
>>>>>>>>>>>> SqlJdbcUtil.java
>>>>>>>>>>>> (revision
>>>>>>>>>>>> 703572)
>>>>>>>>>>>> +++ framework/entity/src/org/ofbiz/entity/jdbc/
>>>>>>>>>>>> SqlJdbcUtil.java
>>>>>>>>>>>> (working copy)
>>>>>>>>>>>> @@ -731,6 +731,9 @@
>>>>>>>>>>>>             fieldClassName = "byte[]";
>>>>>>>>>>>>         }
>>>>>>>>>>>>
>>>>>>>>>>>> +                if  
>>>>>>>>>>>> ("java.sql.Timestamp".equals(fieldType)) {
>>>>>>>>>>>> +                    fieldClassName = fieldType;
>>>>>>>>>>>> +                }
>>>>>>>>>>>>         if (Debug.verboseOn()) Debug.logVerbose("type of
>>>>>>>>>>>> field " + entityName + "." + modelField.getName() +
>>>>>>>>>>>>                 " is " + fieldClassName + ", was  
>>>>>>>>>>>> expecting "
>>>>>>>>>>>> + mft.getJavaType() + "; this may " +
>>>>>>>>>>>>                 "indicate an error in the configuration  
>>>>>>>>>>>> or in
>>>>>>>>>>>> the class, and may result " +
>>>>>>>>>>>> @@ -749,7 +752,11 @@
>>>>>>>>>>>>         break;
>>>>>>>>>>>>
>>>>>>>>>>>>     case 2:
>>>>>>>>>>>> -                sqlP.setValue((java.sql.Timestamp)  
>>>>>>>>>>>> fieldValue);
>>>>>>>>>>>> +                if (fieldValue instanceof String) {
>>>>>>>>>>>> +                    sqlP.setValueTimestampString((String)
>>>>>>>>>>>> fieldValue);
>>>>>>>>>>>> +                } else {
>>>>>>>>>>>> +                    sqlP.setValue((java.sql.Timestamp)
>>>>>>>>>>>> fieldValue);
>>>>>>>>>>>> +                }
>>>>>>>>>>>>         break;
>>>>>>>>>>>>
>>>>>>>>>>>>     case 3:
>>>>>>>>>>>>
>>>>>>>>>>>> Regards
>>>>>>>>>>>> Scott
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> 2008/10/13 Jacques Le Roux <[hidden email]>:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Done in revision: 703816
>>>>>>>>>>>>> It was not possible for PackingSlip.groovy and
>>>>>>>>>>>>> FindInventoryEventPlan.groovy. Because there the date  
>>>>>>>>>>>>> string is
>>>>>>>>>>>>> build
>>>>>>>>>>>>> dynamically in the Groovy file
>>>>>>>>>>>>>
>>>>>>>>>>>>> Jacques
>>>>>>>>>>>>>
>>>>>>>>>>>>> From: "Jacques Le Roux" <[hidden email]>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Adrian,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Yes good idea indeed, I will do that
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Jacques
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> From: "Adrian Crum" <[hidden email]>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Jacques,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Instead of modifying the groovy files, try specifying  
>>>>>>>>>>>>>>> the data
>>>>>>>>>>>>>>> type in
>>>>>>>>>>>>>>> the screen widget.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Example in ReportFinancialSummaryScreens.xml:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> <set field="fromDate" from-field="parameters.fromDate"
>>>>>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>>>>>> <set field="thruDate" from-field="parameters.thruDate"
>>>>>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>>>>>> <script
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> location="component://accounting/webapp/accounting/WEB-
>>>>>>>>>>>>>>> INF/actions/reports/TransactionTotals.groovy"/>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -Adrian
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>>
>>
>>

[Scott Gray]

Inline:

2008/10/14 David E Jones <[hidden email]>:
>
> I wouldn't say that bugs would be "created" by this change, more than
> existing bugs would be exposed (that are only visible on certain databases,
> etc).

You know what I mean though, whatever we call them there is going to
be a lot to fix.


> The service engine doesn't actually do automated mapping, is the service
> event handler that does this for web requests before the actual call to the
> service engine. In fact when a service is called through the service engine
> it will return an error if the object types of values passed in don't match
> what is in the service definition. The difference is that it was that way
> from the beginning, which isn't the case with certain parts of the entity
> engine (mainly in order to not cause the problem of exposing bugs that were
> dormant most of the time... which isn't really good in my current opinion).

The service event handler, that's what I meant :-)
The guess the point I was trying to make was that with so many
services being called via the event handler aren't we making a pretty
huge exception to the rule that we're trying to enforce?  The
performFind service would need to use auto-conversion as well so add
that the mix and you've got a pretty big chunk of the database
operations being performed with auto-converted parameters.

Regards
Scott

>
> On Oct 14, 2008, at 2:09 AM, Scott Gray wrote:
>
>> Ok I see your point, thanks for explaining.
>>
>> #1 makes sense but it will be interesting to see how many bugs are
>> created, also doesn't the service engine currently perform automatic
>> conversion without any problems?
>>
>> Regards
>> Scott
>>
>> 2008/10/14 David E Jones <[hidden email]>:
>>>
>>> On Oct 13, 2008, at 10:10 PM, Scott Gray wrote:
>>>
>>>> Inline:
>>>>
>>>> 2008/10/14 David E Jones <[hidden email]>:
>>>>>
>>>>> Scott, are you referring to the call to PreparedStatement.setString on
>>>>> SQLProcessor line 553? In other words, the difference in the Postgres
>>>>> JDBC
>>>>> driver is that they used to auto-convert from a String to whatever the
>>>>> database needs in the setString method, but now they don't?
>>>>
>>>> When we call PreparedStatement.setString() the JDBC driver now adds
>>>> something like String:varchar to the parameter list where it used to
>>>> add String:unspecified which meant the database would try and take
>>>> care of it, but with varchar it just says "wrong!" and throws an
>>>> error.  However if we call PreparedStatement.setObject() passing in
>>>> the correct sql type then we get String:timestamp and everything works
>>>> fine.
>>>>
>>>> So I guess what I'm saying is that I think java type conversion
>>>> (String vs Timestamp) is a separate topic and the problem here is
>>>> simply the sql type that we are specifying (by calling the various
>>>> PreparedStatement methods) does not match the column type.  We could
>>>> probably solve the postgresql issue by simply calling
>>>> setObject(Types.OTHER) whenever the value type doesn't match entity
>>>> field type.
>>>
>>> The real problem is we're passing bad data back to the database and
>>> expecting it to deal with it. Right now the entity engine is just warning
>>> about such problems in the log, allowing them to go back to the JDBC
>>> driver
>>> and/or database where they become a real problem.
>>>
>>> If we do something like always call setObject then chances are good we'll
>>> have problems with other databases. If I remember right we used to do
>>> that
>>> generically and it caused problems and we had to move to the current
>>> approach of using object time specific methods.
>>>
>>>>> What I'm proposing is to fix this further up the stack in the
>>>>> GenericEntity.set method instead of lower down.
>>>>
>>>> That's fine for when we are using the GenericEntity but we'd also need
>>>> to add similar code to the EntityCondition stuff for find queries
>>>> (which is where Jacques noticed the problem, I haven't checked for the
>>>> error on any operations involving GenericEntity).
>>>
>>> Good point, we'll have to make sure the object types passed into those
>>> are
>>> valid as well.
>>>
>>> -David
>>>
>>>
>>>
>>>>> On Oct 13, 2008, at 7:14 PM, Scott Gray wrote:
>>>>>
>>>>>> Here's the problem as I see it:
>>>>>> Postgresql used to allow you to specify parameter types which did not
>>>>>> match the column type, in the timestamp case if you passed in a
>>>>>> parameter specified as varchar it would automatically attempt to
>>>>>> convert it to a timestamp.  Now Postgresql requires that you either
>>>>>> pass in the correct parameter type for the column or otherwise pass
>>>>>> the parameter type as unknown/unspecified.  Postgresql still doesn't
>>>>>> care whether you pass in a string or a timestamp but you MUST specify
>>>>>> the correct sql type.
>>>>>>
>>>>>> If letting the database take care of the type conversion has never
>>>>>> been a problem before, why do we need to worry about it now?  Remember
>>>>>> the only problem here is that we are passing in a string specified as
>>>>>> varchar instead of a string specified as java.sql.Types.Timestamp.
>>>>>>
>>>>>> Regards
>>>>>> Scott
>>>>>>
>>>>>> 2008/10/14 Adrian Crum <[hidden email]>:
>>>>>>>
>>>>>>> I understood your alternatives. I apologize for being unclear.
>>>>>>>
>>>>>>> You said, "or to just throw an exception when the wrong data type is
>>>>>>> passed
>>>>>>> like the commented out code in GenericEntity referenced above does."
>>>>>>>
>>>>>>> And I'm saying we could do that, but it's going to get interesting
>>>>>>> because a
>>>>>>> lot of code passes Strings into the entity engine. In other words, if
>>>>>>> we
>>>>>>> throw a "hard" exception, there is a good chance that much of OFBiz
>>>>>>> won't
>>>>>>> run.
>>>>>>>
>>>>>>> Personally, I'd like to see better handling of object types in the
>>>>>>> higher
>>>>>>> level code. I fixed a problem over the weekend that was caused by
>>>>>>> this
>>>>>>> very
>>>>>>> thing (passing Strings into the entity engine).
>>>>>>>
>>>>>>> -Adrian
>>>>>>>
>>>>>>>
>>>>>>> David E Jones wrote:
>>>>>>>>
>>>>>>>> I'm referring to the exception I described in the GenericEntity.java
>>>>>>>> file,
>>>>>>>> lines 410-415. Right now it is just a warning in the log (and has
>>>>>>>> been
>>>>>>>> for
>>>>>>>> years). The reason to do it there instead of letting the JDBC driver
>>>>>>>> do
>>>>>>>> it
>>>>>>>> is that not all developers will test on all possible databases, and
>>>>>>>> this
>>>>>>>> will help avoid errors as people use different databases in
>>>>>>>> development/testing or deployment.
>>>>>>>>
>>>>>>>> I tried to describe two possible approaches to improve this
>>>>>>>> situation
>>>>>>>> in
>>>>>>>> my first message. Please let me know if those alternatives were not
>>>>>>>> clear
>>>>>>>> and I'll try to explain them better.
>>>>>>>>
>>>>>>>> -David
>>>>>>>>
>>>>>>>>
>>>>>>>> On Oct 13, 2008, at 5:22 PM, Adrian Crum wrote:
>>>>>>>>
>>>>>>>>> Wasn't that being done already? Jacques started these changes based
>>>>>>>>> on
>>>>>>>>> exceptions being thrown by his JDBC driver. Other DBs seemed to be
>>>>>>>>> working
>>>>>>>>> okay.
>>>>>>>>>
>>>>>>>>> I guess we could throw our own exception to keep things consistent
>>>>>>>>> across
>>>>>>>>> databases. It will be interesting to see how that affects existing
>>>>>>>>> code.
>>>>>>>>>
>>>>>>>>> -Adrian
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> David E Jones wrote:
>>>>>>>>>>
>>>>>>>>>> That is a very good point, and I agree.
>>>>>>>>>> To me that means we go with the fail-fast approach and have it
>>>>>>>>>> throw
>>>>>>>>>> an
>>>>>>>>>> exception if you pass in something that is not the correct object
>>>>>>>>>> type.
>>>>>>>>>> Is that what you meant?
>>>>>>>>>> -David
>>>>>>>>>> On Oct 13, 2008, at 4:57 PM, Adrian Crum wrote:
>>>>>>>>>>>
>>>>>>>>>>> I would prefer handling object types in higher level code - due
>>>>>>>>>>> to
>>>>>>>>>>> ambiguity in how some types would be converted from a String
>>>>>>>>>>> (currency,
>>>>>>>>>>> date, time, etc).
>>>>>>>>>>>
>>>>>>>>>>> -Adrian
>>>>>>>>>>>
>>>>>>>>>>> David E Jones wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> This has been a potential problem for a while, but the decision
>>>>>>>>>>>> was
>>>>>>>>>>>> made quite a while back to not enforce the correct object type.
>>>>>>>>>>>> There is code in the GenericEntity.java file (lines 410-415) to
>>>>>>>>>>>> check
>>>>>>>>>>>> to see if the value passed in matches the object type for the
>>>>>>>>>>>> field
>>>>>>>>>>>> it is
>>>>>>>>>>>> being set on.
>>>>>>>>>>>> The best way to solve this problem is a good question. It might
>>>>>>>>>>>> be
>>>>>>>>>>>> good to introduce some automatic type conversion, or to just
>>>>>>>>>>>> throw
>>>>>>>>>>>> an
>>>>>>>>>>>> exception when the wrong data type is passed like the commented
>>>>>>>>>>>> out
>>>>>>>>>>>> code in
>>>>>>>>>>>> GenericEntity referenced above does.
>>>>>>>>>>>> The most common form of automatic conversion is from String to
>>>>>>>>>>>> whatever the field's object type is, and that is done by the
>>>>>>>>>>>> GenericEntity.setString method.
>>>>>>>>>>>> So, what do people think about this? Should we do an automatic
>>>>>>>>>>>> type
>>>>>>>>>>>> conversion to try to fix programming errors automatically, or do
>>>>>>>>>>>> a
>>>>>>>>>>>> fail-fast
>>>>>>>>>>>> by throwing an exception when the object type is wrong? I
>>>>>>>>>>>> suppose
>>>>>>>>>>>> the
>>>>>>>>>>>> current fail-quiet (in the log) and fail loudly on certain
>>>>>>>>>>>> databases
>>>>>>>>>>>> later
>>>>>>>>>>>> is not the best option...
>>>>>>>>>>>> -David
>>>>>>>>>>>> On Oct 12, 2008, at 2:20 PM, Scott Gray wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> I've had a look into this and I can't see anything related to
>>>>>>>>>>>>> Groovy
>>>>>>>>>>>>> that is making this necessary, it appears to be entirely a
>>>>>>>>>>>>> postgresql
>>>>>>>>>>>>> issue.
>>>>>>>>>>>>>
>>>>>>>>>>>>> When executing a prepared statement postgresql seems to require
>>>>>>>>>>>>> that
>>>>>>>>>>>>> the parameter list sql types match the column types.  So the
>>>>>>>>>>>>> problem
>>>>>>>>>>>>> isn't that we are passing in a string but that we are setting
>>>>>>>>>>>>> the
>>>>>>>>>>>>> sql
>>>>>>>>>>>>> type to character varying by using
>>>>>>>>>>>>> PreparedStatement.setString().
>>>>>>>>>>>>>
>>>>>>>>>>>>> Here's a patch that fixes the issue but I'm not really
>>>>>>>>>>>>> confident
>>>>>>>>>>>>> enough to commit it, it would be great to get some comments
>>>>>>>>>>>>> from
>>>>>>>>>>>>> people who know more about this kind of thing:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Index:
>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SQLProcessor.java
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ===================================================================
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SQLProcessor.java
>>>>>>>>>>>>> (revision
>>>>>>>>>>>>> 703572)
>>>>>>>>>>>>> +++
>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SQLProcessor.java
>>>>>>>>>>>>> (working copy)
>>>>>>>>>>>>> @@ -592,6 +592,22 @@
>>>>>>>>>>>>> *
>>>>>>>>>>>>> * @throws SQLException
>>>>>>>>>>>>> */
>>>>>>>>>>>>> +    public void setValueTimestampString(String field) throws
>>>>>>>>>>>>> SQLException {
>>>>>>>>>>>>> +        if (field != null) {
>>>>>>>>>>>>> +            _ps.setObject(_ind, field, Types.TIMESTAMP);
>>>>>>>>>>>>> +        } else {
>>>>>>>>>>>>> +            _ps.setNull(_ind, Types.TIMESTAMP);
>>>>>>>>>>>>> +        }
>>>>>>>>>>>>> +        _ind++;
>>>>>>>>>>>>> +    }
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +    /**
>>>>>>>>>>>>> +     * Set the next binding variable of the currently active
>>>>>>>>>>>>> prepared
>>>>>>>>>>>>> statement.
>>>>>>>>>>>>> +     *
>>>>>>>>>>>>> +     * @param field
>>>>>>>>>>>>> +     *
>>>>>>>>>>>>> +     * @throws SQLException
>>>>>>>>>>>>> +     */
>>>>>>>>>>>>> public void setValue(java.sql.Time field) throws SQLException {
>>>>>>>>>>>>> if (field != null) {
>>>>>>>>>>>>>    _ps.setTime(_ind, field);
>>>>>>>>>>>>> Index:
>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ===================================================================
>>>>>>>>>>>>> --- framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>>>>>> (revision
>>>>>>>>>>>>> 703572)
>>>>>>>>>>>>> +++ framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>>>>>> (working copy)
>>>>>>>>>>>>> @@ -731,6 +731,9 @@
>>>>>>>>>>>>>            fieldClassName = "byte[]";
>>>>>>>>>>>>>        }
>>>>>>>>>>>>>
>>>>>>>>>>>>> +                if ("java.sql.Timestamp".equals(fieldType)) {
>>>>>>>>>>>>> +                    fieldClassName = fieldType;
>>>>>>>>>>>>> +                }
>>>>>>>>>>>>>        if (Debug.verboseOn()) Debug.logVerbose("type of
>>>>>>>>>>>>> field " + entityName + "." + modelField.getName() +
>>>>>>>>>>>>>                " is " + fieldClassName + ", was expecting "
>>>>>>>>>>>>> + mft.getJavaType() + "; this may " +
>>>>>>>>>>>>>                "indicate an error in the configuration or in
>>>>>>>>>>>>> the class, and may result " +
>>>>>>>>>>>>> @@ -749,7 +752,11 @@
>>>>>>>>>>>>>        break;
>>>>>>>>>>>>>
>>>>>>>>>>>>>    case 2:
>>>>>>>>>>>>> -                sqlP.setValue((java.sql.Timestamp)
>>>>>>>>>>>>> fieldValue);
>>>>>>>>>>>>> +                if (fieldValue instanceof String) {
>>>>>>>>>>>>> +                    sqlP.setValueTimestampString((String)
>>>>>>>>>>>>> fieldValue);
>>>>>>>>>>>>> +                } else {
>>>>>>>>>>>>> +                    sqlP.setValue((java.sql.Timestamp)
>>>>>>>>>>>>> fieldValue);
>>>>>>>>>>>>> +                }
>>>>>>>>>>>>>        break;
>>>>>>>>>>>>>
>>>>>>>>>>>>>    case 3:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Regards
>>>>>>>>>>>>> Scott
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2008/10/13 Jacques Le Roux <[hidden email]>:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Done in revision: 703816
>>>>>>>>>>>>>> It was not possible for PackingSlip.groovy and
>>>>>>>>>>>>>> FindInventoryEventPlan.groovy. Because there the date string
>>>>>>>>>>>>>> is
>>>>>>>>>>>>>> build
>>>>>>>>>>>>>> dynamically in the Groovy file
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Jacques
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> From: "Jacques Le Roux" <[hidden email]>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Adrian,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Yes good idea indeed, I will do that
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Jacques
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> From: "Adrian Crum" <[hidden email]>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Jacques,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Instead of modifying the groovy files, try specifying the
>>>>>>>>>>>>>>>> data
>>>>>>>>>>>>>>>> type in
>>>>>>>>>>>>>>>> the screen widget.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Example in ReportFinancialSummaryScreens.xml:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> <set field="fromDate" from-field="parameters.fromDate"
>>>>>>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>>>>>>> <set field="thruDate" from-field="parameters.thruDate"
>>>>>>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>>>>>>> <script
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> location="component://accounting/webapp/accounting/WEB-INF/actions/reports/TransactionTotals.groovy"/>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> -Adrian
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>
>

[Adrian Crum]

In addition, the service event handler has the information needed to
perform a proper conversion (locale and time zone). The entity engine
does not.

-Adrian

David E Jones wrote:

>
> I wouldn't say that bugs would be "created" by this change, more than
> existing bugs would be exposed (that are only visible on certain
> databases, etc).
>
> The service engine doesn't actually do automated mapping, is the service
> event handler that does this for web requests before the actual call to
> the service engine. In fact when a service is called through the service
> engine it will return an error if the object types of values passed in
> don't match what is in the service definition. The difference is that it
> was that way from the beginning, which isn't the case with certain parts
> of the entity engine (mainly in order to not cause the problem of
> exposing bugs that were dormant most of the time... which isn't really
> good in my current opinion).
>
> -David
>
>
> On Oct 14, 2008, at 2:09 AM, Scott Gray wrote:
>
>> Ok I see your point, thanks for explaining.
>>
>> #1 makes sense but it will be interesting to see how many bugs are
>> created, also doesn't the service engine currently perform automatic
>> conversion without any problems?
>>
>> Regards
>> Scott
>>
>> 2008/10/14 David E Jones <[hidden email]>:
>>>
>>> On Oct 13, 2008, at 10:10 PM, Scott Gray wrote:
>>>
>>>> Inline:
>>>>
>>>> 2008/10/14 David E Jones <[hidden email]>:
>>>>>
>>>>> Scott, are you referring to the call to PreparedStatement.setString on
>>>>> SQLProcessor line 553? In other words, the difference in the Postgres
>>>>> JDBC
>>>>> driver is that they used to auto-convert from a String to whatever the
>>>>> database needs in the setString method, but now they don't?
>>>>
>>>> When we call PreparedStatement.setString() the JDBC driver now adds
>>>> something like String:varchar to the parameter list where it used to
>>>> add String:unspecified which meant the database would try and take
>>>> care of it, but with varchar it just says "wrong!" and throws an
>>>> error.  However if we call PreparedStatement.setObject() passing in
>>>> the correct sql type then we get String:timestamp and everything works
>>>> fine.
>>>>
>>>> So I guess what I'm saying is that I think java type conversion
>>>> (String vs Timestamp) is a separate topic and the problem here is
>>>> simply the sql type that we are specifying (by calling the various
>>>> PreparedStatement methods) does not match the column type.  We could
>>>> probably solve the postgresql issue by simply calling
>>>> setObject(Types.OTHER) whenever the value type doesn't match entity
>>>> field type.
>>>
>>> The real problem is we're passing bad data back to the database and
>>> expecting it to deal with it. Right now the entity engine is just
>>> warning
>>> about such problems in the log, allowing them to go back to the JDBC
>>> driver
>>> and/or database where they become a real problem.
>>>
>>> If we do something like always call setObject then chances are good
>>> we'll
>>> have problems with other databases. If I remember right we used to do
>>> that
>>> generically and it caused problems and we had to move to the current
>>> approach of using object time specific methods.
>>>
>>>>> What I'm proposing is to fix this further up the stack in the
>>>>> GenericEntity.set method instead of lower down.
>>>>
>>>> That's fine for when we are using the GenericEntity but we'd also need
>>>> to add similar code to the EntityCondition stuff for find queries
>>>> (which is where Jacques noticed the problem, I haven't checked for the
>>>> error on any operations involving GenericEntity).
>>>
>>> Good point, we'll have to make sure the object types passed into
>>> those are
>>> valid as well.
>>>
>>> -David
>>>
>>>
>>>
>>>>> On Oct 13, 2008, at 7:14 PM, Scott Gray wrote:
>>>>>
>>>>>> Here's the problem as I see it:
>>>>>> Postgresql used to allow you to specify parameter types which did not
>>>>>> match the column type, in the timestamp case if you passed in a
>>>>>> parameter specified as varchar it would automatically attempt to
>>>>>> convert it to a timestamp.  Now Postgresql requires that you either
>>>>>> pass in the correct parameter type for the column or otherwise pass
>>>>>> the parameter type as unknown/unspecified.  Postgresql still doesn't
>>>>>> care whether you pass in a string or a timestamp but you MUST specify
>>>>>> the correct sql type.
>>>>>>
>>>>>> If letting the database take care of the type conversion has never
>>>>>> been a problem before, why do we need to worry about it now?  
>>>>>> Remember
>>>>>> the only problem here is that we are passing in a string specified as
>>>>>> varchar instead of a string specified as java.sql.Types.Timestamp.
>>>>>>
>>>>>> Regards
>>>>>> Scott
>>>>>>
>>>>>> 2008/10/14 Adrian Crum <[hidden email]>:
>>>>>>>
>>>>>>> I understood your alternatives. I apologize for being unclear.
>>>>>>>
>>>>>>> You said, "or to just throw an exception when the wrong data type is
>>>>>>> passed
>>>>>>> like the commented out code in GenericEntity referenced above does."
>>>>>>>
>>>>>>> And I'm saying we could do that, but it's going to get interesting
>>>>>>> because a
>>>>>>> lot of code passes Strings into the entity engine. In other
>>>>>>> words, if
>>>>>>> we
>>>>>>> throw a "hard" exception, there is a good chance that much of OFBiz
>>>>>>> won't
>>>>>>> run.
>>>>>>>
>>>>>>> Personally, I'd like to see better handling of object types in the
>>>>>>> higher
>>>>>>> level code. I fixed a problem over the weekend that was caused by
>>>>>>> this
>>>>>>> very
>>>>>>> thing (passing Strings into the entity engine).
>>>>>>>
>>>>>>> -Adrian
>>>>>>>
>>>>>>>
>>>>>>> David E Jones wrote:
>>>>>>>>
>>>>>>>> I'm referring to the exception I described in the
>>>>>>>> GenericEntity.java
>>>>>>>> file,
>>>>>>>> lines 410-415. Right now it is just a warning in the log (and
>>>>>>>> has been
>>>>>>>> for
>>>>>>>> years). The reason to do it there instead of letting the JDBC
>>>>>>>> driver
>>>>>>>> do
>>>>>>>> it
>>>>>>>> is that not all developers will test on all possible databases, and
>>>>>>>> this
>>>>>>>> will help avoid errors as people use different databases in
>>>>>>>> development/testing or deployment.
>>>>>>>>
>>>>>>>> I tried to describe two possible approaches to improve this
>>>>>>>> situation
>>>>>>>> in
>>>>>>>> my first message. Please let me know if those alternatives were not
>>>>>>>> clear
>>>>>>>> and I'll try to explain them better.
>>>>>>>>
>>>>>>>> -David
>>>>>>>>
>>>>>>>>
>>>>>>>> On Oct 13, 2008, at 5:22 PM, Adrian Crum wrote:
>>>>>>>>
>>>>>>>>> Wasn't that being done already? Jacques started these changes
>>>>>>>>> based
>>>>>>>>> on
>>>>>>>>> exceptions being thrown by his JDBC driver. Other DBs seemed to be
>>>>>>>>> working
>>>>>>>>> okay.
>>>>>>>>>
>>>>>>>>> I guess we could throw our own exception to keep things consistent
>>>>>>>>> across
>>>>>>>>> databases. It will be interesting to see how that affects existing
>>>>>>>>> code.
>>>>>>>>>
>>>>>>>>> -Adrian
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> David E Jones wrote:
>>>>>>>>>>
>>>>>>>>>> That is a very good point, and I agree.
>>>>>>>>>> To me that means we go with the fail-fast approach and have it
>>>>>>>>>> throw
>>>>>>>>>> an
>>>>>>>>>> exception if you pass in something that is not the correct object
>>>>>>>>>> type.
>>>>>>>>>> Is that what you meant?
>>>>>>>>>> -David
>>>>>>>>>> On Oct 13, 2008, at 4:57 PM, Adrian Crum wrote:
>>>>>>>>>>>
>>>>>>>>>>> I would prefer handling object types in higher level code -
>>>>>>>>>>> due to
>>>>>>>>>>> ambiguity in how some types would be converted from a String
>>>>>>>>>>> (currency,
>>>>>>>>>>> date, time, etc).
>>>>>>>>>>>
>>>>>>>>>>> -Adrian
>>>>>>>>>>>
>>>>>>>>>>> David E Jones wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> This has been a potential problem for a while, but the decision
>>>>>>>>>>>> was
>>>>>>>>>>>> made quite a while back to not enforce the correct object type.
>>>>>>>>>>>> There is code in the GenericEntity.java file (lines 410-415) to
>>>>>>>>>>>> check
>>>>>>>>>>>> to see if the value passed in matches the object type for the
>>>>>>>>>>>> field
>>>>>>>>>>>> it is
>>>>>>>>>>>> being set on.
>>>>>>>>>>>> The best way to solve this problem is a good question. It
>>>>>>>>>>>> might be
>>>>>>>>>>>> good to introduce some automatic type conversion, or to just
>>>>>>>>>>>> throw
>>>>>>>>>>>> an
>>>>>>>>>>>> exception when the wrong data type is passed like the commented
>>>>>>>>>>>> out
>>>>>>>>>>>> code in
>>>>>>>>>>>> GenericEntity referenced above does.
>>>>>>>>>>>> The most common form of automatic conversion is from String to
>>>>>>>>>>>> whatever the field's object type is, and that is done by the
>>>>>>>>>>>> GenericEntity.setString method.
>>>>>>>>>>>> So, what do people think about this? Should we do an automatic
>>>>>>>>>>>> type
>>>>>>>>>>>> conversion to try to fix programming errors automatically,
>>>>>>>>>>>> or do a
>>>>>>>>>>>> fail-fast
>>>>>>>>>>>> by throwing an exception when the object type is wrong? I
>>>>>>>>>>>> suppose
>>>>>>>>>>>> the
>>>>>>>>>>>> current fail-quiet (in the log) and fail loudly on certain
>>>>>>>>>>>> databases
>>>>>>>>>>>> later
>>>>>>>>>>>> is not the best option...
>>>>>>>>>>>> -David
>>>>>>>>>>>> On Oct 12, 2008, at 2:20 PM, Scott Gray wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> I've had a look into this and I can't see anything related to
>>>>>>>>>>>>> Groovy
>>>>>>>>>>>>> that is making this necessary, it appears to be entirely a
>>>>>>>>>>>>> postgresql
>>>>>>>>>>>>> issue.
>>>>>>>>>>>>>
>>>>>>>>>>>>> When executing a prepared statement postgresql seems to
>>>>>>>>>>>>> require
>>>>>>>>>>>>> that
>>>>>>>>>>>>> the parameter list sql types match the column types.  So the
>>>>>>>>>>>>> problem
>>>>>>>>>>>>> isn't that we are passing in a string but that we are
>>>>>>>>>>>>> setting the
>>>>>>>>>>>>> sql
>>>>>>>>>>>>> type to character varying by using
>>>>>>>>>>>>> PreparedStatement.setString().
>>>>>>>>>>>>>
>>>>>>>>>>>>> Here's a patch that fixes the issue but I'm not really
>>>>>>>>>>>>> confident
>>>>>>>>>>>>> enough to commit it, it would be great to get some comments
>>>>>>>>>>>>> from
>>>>>>>>>>>>> people who know more about this kind of thing:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Index:
>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SQLProcessor.java
>>>>>>>>>>>>>
>>>>>>>>>>>>> ===================================================================
>>>>>>>>>>>>>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SQLProcessor.java
>>>>>>>>>>>>> (revision
>>>>>>>>>>>>> 703572)
>>>>>>>>>>>>> +++
>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SQLProcessor.java
>>>>>>>>>>>>> (working copy)
>>>>>>>>>>>>> @@ -592,6 +592,22 @@
>>>>>>>>>>>>> *
>>>>>>>>>>>>> * @throws SQLException
>>>>>>>>>>>>> */
>>>>>>>>>>>>> +    public void setValueTimestampString(String field) throws
>>>>>>>>>>>>> SQLException {
>>>>>>>>>>>>> +        if (field != null) {
>>>>>>>>>>>>> +            _ps.setObject(_ind, field, Types.TIMESTAMP);
>>>>>>>>>>>>> +        } else {
>>>>>>>>>>>>> +            _ps.setNull(_ind, Types.TIMESTAMP);
>>>>>>>>>>>>> +        }
>>>>>>>>>>>>> +        _ind++;
>>>>>>>>>>>>> +    }
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +    /**
>>>>>>>>>>>>> +     * Set the next binding variable of the currently active
>>>>>>>>>>>>> prepared
>>>>>>>>>>>>> statement.
>>>>>>>>>>>>> +     *
>>>>>>>>>>>>> +     * @param field
>>>>>>>>>>>>> +     *
>>>>>>>>>>>>> +     * @throws SQLException
>>>>>>>>>>>>> +     */
>>>>>>>>>>>>> public void setValue(java.sql.Time field) throws
>>>>>>>>>>>>> SQLException {
>>>>>>>>>>>>> if (field != null) {
>>>>>>>>>>>>>     _ps.setTime(_ind, field);
>>>>>>>>>>>>> Index:
>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>>>>>>
>>>>>>>>>>>>> ===================================================================
>>>>>>>>>>>>>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>>>>>> (revision
>>>>>>>>>>>>> 703572)
>>>>>>>>>>>>> +++
>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>>>>>> (working copy)
>>>>>>>>>>>>> @@ -731,6 +731,9 @@
>>>>>>>>>>>>>             fieldClassName = "byte[]";
>>>>>>>>>>>>>         }
>>>>>>>>>>>>>
>>>>>>>>>>>>> +                if ("java.sql.Timestamp".equals(fieldType)) {
>>>>>>>>>>>>> +                    fieldClassName = fieldType;
>>>>>>>>>>>>> +                }
>>>>>>>>>>>>>         if (Debug.verboseOn()) Debug.logVerbose("type of
>>>>>>>>>>>>> field " + entityName + "." + modelField.getName() +
>>>>>>>>>>>>>                 " is " + fieldClassName + ", was expecting "
>>>>>>>>>>>>> + mft.getJavaType() + "; this may " +
>>>>>>>>>>>>>                 "indicate an error in the configuration or in
>>>>>>>>>>>>> the class, and may result " +
>>>>>>>>>>>>> @@ -749,7 +752,11 @@
>>>>>>>>>>>>>         break;
>>>>>>>>>>>>>
>>>>>>>>>>>>>     case 2:
>>>>>>>>>>>>> -                sqlP.setValue((java.sql.Timestamp)
>>>>>>>>>>>>> fieldValue);
>>>>>>>>>>>>> +                if (fieldValue instanceof String) {
>>>>>>>>>>>>> +                    sqlP.setValueTimestampString((String)
>>>>>>>>>>>>> fieldValue);
>>>>>>>>>>>>> +                } else {
>>>>>>>>>>>>> +                    sqlP.setValue((java.sql.Timestamp)
>>>>>>>>>>>>> fieldValue);
>>>>>>>>>>>>> +                }
>>>>>>>>>>>>>         break;
>>>>>>>>>>>>>
>>>>>>>>>>>>>     case 3:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Regards
>>>>>>>>>>>>> Scott
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2008/10/13 Jacques Le Roux <[hidden email]>:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Done in revision: 703816
>>>>>>>>>>>>>> It was not possible for PackingSlip.groovy and
>>>>>>>>>>>>>> FindInventoryEventPlan.groovy. Because there the date
>>>>>>>>>>>>>> string is
>>>>>>>>>>>>>> build
>>>>>>>>>>>>>> dynamically in the Groovy file
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Jacques
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> From: "Jacques Le Roux" <[hidden email]>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Adrian,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Yes good idea indeed, I will do that
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Jacques
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> From: "Adrian Crum" <[hidden email]>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Jacques,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Instead of modifying the groovy files, try specifying
>>>>>>>>>>>>>>>> the data
>>>>>>>>>>>>>>>> type in
>>>>>>>>>>>>>>>> the screen widget.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Example in ReportFinancialSummaryScreens.xml:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> <set field="fromDate" from-field="parameters.fromDate"
>>>>>>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>>>>>>> <set field="thruDate" from-field="parameters.thruDate"
>>>>>>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>>>>>>> <script
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> location="component://accounting/webapp/accounting/WEB-INF/actions/reports/TransactionTotals.groovy"/>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> -Adrian
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>
>

[Adrian Crum]

Scott Gray wrote:
> The service event handler, that's what I meant :-)
> The guess the point I was trying to make was that with so many
> services being called via the event handler aren't we making a pretty
> huge exception to the rule that we're trying to enforce?  The
> performFind service would need to use auto-conversion as well so add
> that the mix and you've got a pretty big chunk of the database
> operations being performed with auto-converted parameters.

Funny you brought up the performFind service - I just fixed that, by
converting the object types before creating the EntityCondition objects.

-Adrian

[Adrian Crum]

David E Jones wrote:

> What I'm proposing is to fix this further up the stack in the
> GenericEntity.set method instead of lower down. I guess either way we
> have the two options I mentioned earlier:
>
> 1. fail fast(er) by having OFBiz throw an exception
> 2. try to auto-convert
>
> #1 will result, as Adrian mentioned, in a bunch of possibly undiscovered
> places where the wrong object type is passed in, causing OFBiz to not
> function until these are cleaned up. On the other hand, in order to
> reliably work on most databases these need to be fixed sooner or later.
>
> #2 has the problem, also as Adrian mentioned, of possibly not doing the
> conversion correctly, especially in cases where locale-sensitive parsing
> is needed or there is a special format string or something.
>
> In spite of the short-term pain my vote is still for #1, and to do it
> higher up in the GenericEntity.set method in order to fail right when
> it's set, not later on when the GenericDelegator method is called, so
> that we know where the bad object type is being passed.

My vote is for #1 also. I agree it's going to be a pain, but I believe
it's the responsible and reliable thing to do.

-Adrian

[Jacques Le Roux]

From: "Adrian Crum" <[hidden email]>

> David E Jones wrote:
>> What I'm proposing is to fix this further up the stack in the GenericEntity.set method instead of lower down. I guess either way
>> we have the two options I mentioned earlier:
>>
>> 1. fail fast(er) by having OFBiz throw an exception
>> 2. try to auto-convert
>>
>> #1 will result, as Adrian mentioned, in a bunch of possibly undiscovered places where the wrong object type is passed in, causing
>> OFBiz to not function until these are cleaned up. On the other hand, in order to reliably work on most databases these need to be
>> fixed sooner or later.
>>
>> #2 has the problem, also as Adrian mentioned, of possibly not doing the conversion correctly, especially in cases where
>> locale-sensitive parsing is needed or there is a special format string or something.
>>
>> In spite of the short-term pain my vote is still for #1, and to do it higher up in the GenericEntity.set method in order to fail
>> right when it's set, not later on when the GenericDelegator method is called, so that we know where the bad object type is being
>> passed.
>
> My vote is for #1 also. I agree it's going to be a pain, but I believe it's the responsible and reliable thing to do.

Maybe we could add an interrupting tool (at different code points if needed) on the demo server which would catch and put in log all
points needed to be changed before really changing them ?

Hope I"m clear

Jacques

> -Adrian
>

[Jacques Le Roux]

I mean in a specific log

Please let me know if this makes sense

From: "Jacques Le Roux" <[hidden email]>
> Maybe we could add an interrupting tool (at different code points if needed) on the demo server which would catch and put in log
> all points needed to be changed before really changing them ?
>
> Hope I"m clear
>
> Jacques

[Scott Gray]

Ok it makes more sense to me now, conversion needs to take place at a
level where the relevant information is still available so doing it on
a low level isn't really an option (even though that's been allowed to
happen up until now).  I guess I was having trouble understanding why
we needed to do all this work when everything had been running fine,
but I see now that it wasn't really running that fine.

Thanks
Scott

2008/10/15 Adrian Crum <[hidden email]>:

> In addition, the service event handler has the information needed to perform
> a proper conversion (locale and time zone). The entity engine does not.
>
> -Adrian
>
> David E Jones wrote:
>>
>> I wouldn't say that bugs would be "created" by this change, more than
>> existing bugs would be exposed (that are only visible on certain databases,
>> etc).
>>
>> The service engine doesn't actually do automated mapping, is the service
>> event handler that does this for web requests before the actual call to the
>> service engine. In fact when a service is called through the service engine
>> it will return an error if the object types of values passed in don't match
>> what is in the service definition. The difference is that it was that way
>> from the beginning, which isn't the case with certain parts of the entity
>> engine (mainly in order to not cause the problem of exposing bugs that were
>> dormant most of the time... which isn't really good in my current opinion).
>>
>> -David
>>
>>
>> On Oct 14, 2008, at 2:09 AM, Scott Gray wrote:
>>
>>> Ok I see your point, thanks for explaining.
>>>
>>> #1 makes sense but it will be interesting to see how many bugs are
>>> created, also doesn't the service engine currently perform automatic
>>> conversion without any problems?
>>>
>>> Regards
>>> Scott
>>>
>>> 2008/10/14 David E Jones <[hidden email]>:
>>>>
>>>> On Oct 13, 2008, at 10:10 PM, Scott Gray wrote:
>>>>
>>>>> Inline:
>>>>>
>>>>> 2008/10/14 David E Jones <[hidden email]>:
>>>>>>
>>>>>> Scott, are you referring to the call to PreparedStatement.setString on
>>>>>> SQLProcessor line 553? In other words, the difference in the Postgres
>>>>>> JDBC
>>>>>> driver is that they used to auto-convert from a String to whatever the
>>>>>> database needs in the setString method, but now they don't?
>>>>>
>>>>> When we call PreparedStatement.setString() the JDBC driver now adds
>>>>> something like String:varchar to the parameter list where it used to
>>>>> add String:unspecified which meant the database would try and take
>>>>> care of it, but with varchar it just says "wrong!" and throws an
>>>>> error.  However if we call PreparedStatement.setObject() passing in
>>>>> the correct sql type then we get String:timestamp and everything works
>>>>> fine.
>>>>>
>>>>> So I guess what I'm saying is that I think java type conversion
>>>>> (String vs Timestamp) is a separate topic and the problem here is
>>>>> simply the sql type that we are specifying (by calling the various
>>>>> PreparedStatement methods) does not match the column type.  We could
>>>>> probably solve the postgresql issue by simply calling
>>>>> setObject(Types.OTHER) whenever the value type doesn't match entity
>>>>> field type.
>>>>
>>>> The real problem is we're passing bad data back to the database and
>>>> expecting it to deal with it. Right now the entity engine is just
>>>> warning
>>>> about such problems in the log, allowing them to go back to the JDBC
>>>> driver
>>>> and/or database where they become a real problem.
>>>>
>>>> If we do something like always call setObject then chances are good
>>>> we'll
>>>> have problems with other databases. If I remember right we used to do
>>>> that
>>>> generically and it caused problems and we had to move to the current
>>>> approach of using object time specific methods.
>>>>
>>>>>> What I'm proposing is to fix this further up the stack in the
>>>>>> GenericEntity.set method instead of lower down.
>>>>>
>>>>> That's fine for when we are using the GenericEntity but we'd also need
>>>>> to add similar code to the EntityCondition stuff for find queries
>>>>> (which is where Jacques noticed the problem, I haven't checked for the
>>>>> error on any operations involving GenericEntity).
>>>>
>>>> Good point, we'll have to make sure the object types passed into those
>>>> are
>>>> valid as well.
>>>>
>>>> -David
>>>>
>>>>
>>>>
>>>>>> On Oct 13, 2008, at 7:14 PM, Scott Gray wrote:
>>>>>>
>>>>>>> Here's the problem as I see it:
>>>>>>> Postgresql used to allow you to specify parameter types which did not
>>>>>>> match the column type, in the timestamp case if you passed in a
>>>>>>> parameter specified as varchar it would automatically attempt to
>>>>>>> convert it to a timestamp.  Now Postgresql requires that you either
>>>>>>> pass in the correct parameter type for the column or otherwise pass
>>>>>>> the parameter type as unknown/unspecified.  Postgresql still doesn't
>>>>>>> care whether you pass in a string or a timestamp but you MUST specify
>>>>>>> the correct sql type.
>>>>>>>
>>>>>>> If letting the database take care of the type conversion has never
>>>>>>> been a problem before, why do we need to worry about it now?
>>>>>>>  Remember
>>>>>>> the only problem here is that we are passing in a string specified as
>>>>>>> varchar instead of a string specified as java.sql.Types.Timestamp.
>>>>>>>
>>>>>>> Regards
>>>>>>> Scott
>>>>>>>
>>>>>>> 2008/10/14 Adrian Crum <[hidden email]>:
>>>>>>>>
>>>>>>>> I understood your alternatives. I apologize for being unclear.
>>>>>>>>
>>>>>>>> You said, "or to just throw an exception when the wrong data type is
>>>>>>>> passed
>>>>>>>> like the commented out code in GenericEntity referenced above does."
>>>>>>>>
>>>>>>>> And I'm saying we could do that, but it's going to get interesting
>>>>>>>> because a
>>>>>>>> lot of code passes Strings into the entity engine. In other words,
>>>>>>>> if
>>>>>>>> we
>>>>>>>> throw a "hard" exception, there is a good chance that much of OFBiz
>>>>>>>> won't
>>>>>>>> run.
>>>>>>>>
>>>>>>>> Personally, I'd like to see better handling of object types in the
>>>>>>>> higher
>>>>>>>> level code. I fixed a problem over the weekend that was caused by
>>>>>>>> this
>>>>>>>> very
>>>>>>>> thing (passing Strings into the entity engine).
>>>>>>>>
>>>>>>>> -Adrian
>>>>>>>>
>>>>>>>>
>>>>>>>> David E Jones wrote:
>>>>>>>>>
>>>>>>>>> I'm referring to the exception I described in the
>>>>>>>>> GenericEntity.java
>>>>>>>>> file,
>>>>>>>>> lines 410-415. Right now it is just a warning in the log (and has
>>>>>>>>> been
>>>>>>>>> for
>>>>>>>>> years). The reason to do it there instead of letting the JDBC
>>>>>>>>> driver
>>>>>>>>> do
>>>>>>>>> it
>>>>>>>>> is that not all developers will test on all possible databases, and
>>>>>>>>> this
>>>>>>>>> will help avoid errors as people use different databases in
>>>>>>>>> development/testing or deployment.
>>>>>>>>>
>>>>>>>>> I tried to describe two possible approaches to improve this
>>>>>>>>> situation
>>>>>>>>> in
>>>>>>>>> my first message. Please let me know if those alternatives were not
>>>>>>>>> clear
>>>>>>>>> and I'll try to explain them better.
>>>>>>>>>
>>>>>>>>> -David
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Oct 13, 2008, at 5:22 PM, Adrian Crum wrote:
>>>>>>>>>
>>>>>>>>>> Wasn't that being done already? Jacques started these changes
>>>>>>>>>> based
>>>>>>>>>> on
>>>>>>>>>> exceptions being thrown by his JDBC driver. Other DBs seemed to be
>>>>>>>>>> working
>>>>>>>>>> okay.
>>>>>>>>>>
>>>>>>>>>> I guess we could throw our own exception to keep things consistent
>>>>>>>>>> across
>>>>>>>>>> databases. It will be interesting to see how that affects existing
>>>>>>>>>> code.
>>>>>>>>>>
>>>>>>>>>> -Adrian
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> David E Jones wrote:
>>>>>>>>>>>
>>>>>>>>>>> That is a very good point, and I agree.
>>>>>>>>>>> To me that means we go with the fail-fast approach and have it
>>>>>>>>>>> throw
>>>>>>>>>>> an
>>>>>>>>>>> exception if you pass in something that is not the correct object
>>>>>>>>>>> type.
>>>>>>>>>>> Is that what you meant?
>>>>>>>>>>> -David
>>>>>>>>>>> On Oct 13, 2008, at 4:57 PM, Adrian Crum wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> I would prefer handling object types in higher level code - due
>>>>>>>>>>>> to
>>>>>>>>>>>> ambiguity in how some types would be converted from a String
>>>>>>>>>>>> (currency,
>>>>>>>>>>>> date, time, etc).
>>>>>>>>>>>>
>>>>>>>>>>>> -Adrian
>>>>>>>>>>>>
>>>>>>>>>>>> David E Jones wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> This has been a potential problem for a while, but the decision
>>>>>>>>>>>>> was
>>>>>>>>>>>>> made quite a while back to not enforce the correct object type.
>>>>>>>>>>>>> There is code in the GenericEntity.java file (lines 410-415) to
>>>>>>>>>>>>> check
>>>>>>>>>>>>> to see if the value passed in matches the object type for the
>>>>>>>>>>>>> field
>>>>>>>>>>>>> it is
>>>>>>>>>>>>> being set on.
>>>>>>>>>>>>> The best way to solve this problem is a good question. It might
>>>>>>>>>>>>> be
>>>>>>>>>>>>> good to introduce some automatic type conversion, or to just
>>>>>>>>>>>>> throw
>>>>>>>>>>>>> an
>>>>>>>>>>>>> exception when the wrong data type is passed like the commented
>>>>>>>>>>>>> out
>>>>>>>>>>>>> code in
>>>>>>>>>>>>> GenericEntity referenced above does.
>>>>>>>>>>>>> The most common form of automatic conversion is from String to
>>>>>>>>>>>>> whatever the field's object type is, and that is done by the
>>>>>>>>>>>>> GenericEntity.setString method.
>>>>>>>>>>>>> So, what do people think about this? Should we do an automatic
>>>>>>>>>>>>> type
>>>>>>>>>>>>> conversion to try to fix programming errors automatically, or
>>>>>>>>>>>>> do a
>>>>>>>>>>>>> fail-fast
>>>>>>>>>>>>> by throwing an exception when the object type is wrong? I
>>>>>>>>>>>>> suppose
>>>>>>>>>>>>> the
>>>>>>>>>>>>> current fail-quiet (in the log) and fail loudly on certain
>>>>>>>>>>>>> databases
>>>>>>>>>>>>> later
>>>>>>>>>>>>> is not the best option...
>>>>>>>>>>>>> -David
>>>>>>>>>>>>> On Oct 12, 2008, at 2:20 PM, Scott Gray wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I've had a look into this and I can't see anything related to
>>>>>>>>>>>>>> Groovy
>>>>>>>>>>>>>> that is making this necessary, it appears to be entirely a
>>>>>>>>>>>>>> postgresql
>>>>>>>>>>>>>> issue.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> When executing a prepared statement postgresql seems to
>>>>>>>>>>>>>> require
>>>>>>>>>>>>>> that
>>>>>>>>>>>>>> the parameter list sql types match the column types.  So the
>>>>>>>>>>>>>> problem
>>>>>>>>>>>>>> isn't that we are passing in a string but that we are setting
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>> sql
>>>>>>>>>>>>>> type to character varying by using
>>>>>>>>>>>>>> PreparedStatement.setString().
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Here's a patch that fixes the issue but I'm not really
>>>>>>>>>>>>>> confident
>>>>>>>>>>>>>> enough to commit it, it would be great to get some comments
>>>>>>>>>>>>>> from
>>>>>>>>>>>>>> people who know more about this kind of thing:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Index:
>>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SQLProcessor.java
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ===================================================================
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SQLProcessor.java
>>>>>>>>>>>>>> (revision
>>>>>>>>>>>>>> 703572)
>>>>>>>>>>>>>> +++
>>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SQLProcessor.java
>>>>>>>>>>>>>> (working copy)
>>>>>>>>>>>>>> @@ -592,6 +592,22 @@
>>>>>>>>>>>>>> *
>>>>>>>>>>>>>> * @throws SQLException
>>>>>>>>>>>>>> */
>>>>>>>>>>>>>> +    public void setValueTimestampString(String field) throws
>>>>>>>>>>>>>> SQLException {
>>>>>>>>>>>>>> +        if (field != null) {
>>>>>>>>>>>>>> +            _ps.setObject(_ind, field, Types.TIMESTAMP);
>>>>>>>>>>>>>> +        } else {
>>>>>>>>>>>>>> +            _ps.setNull(_ind, Types.TIMESTAMP);
>>>>>>>>>>>>>> +        }
>>>>>>>>>>>>>> +        _ind++;
>>>>>>>>>>>>>> +    }
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +    /**
>>>>>>>>>>>>>> +     * Set the next binding variable of the currently active
>>>>>>>>>>>>>> prepared
>>>>>>>>>>>>>> statement.
>>>>>>>>>>>>>> +     *
>>>>>>>>>>>>>> +     * @param field
>>>>>>>>>>>>>> +     *
>>>>>>>>>>>>>> +     * @throws SQLException
>>>>>>>>>>>>>> +     */
>>>>>>>>>>>>>> public void setValue(java.sql.Time field) throws SQLException
>>>>>>>>>>>>>> {
>>>>>>>>>>>>>> if (field != null) {
>>>>>>>>>>>>>>    _ps.setTime(_ind, field);
>>>>>>>>>>>>>> Index:
>>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ===================================================================
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>>>>>>> (revision
>>>>>>>>>>>>>> 703572)
>>>>>>>>>>>>>> +++
>>>>>>>>>>>>>> framework/entity/src/org/ofbiz/entity/jdbc/SqlJdbcUtil.java
>>>>>>>>>>>>>> (working copy)
>>>>>>>>>>>>>> @@ -731,6 +731,9 @@
>>>>>>>>>>>>>>            fieldClassName = "byte[]";
>>>>>>>>>>>>>>        }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> +                if ("java.sql.Timestamp".equals(fieldType)) {
>>>>>>>>>>>>>> +                    fieldClassName = fieldType;
>>>>>>>>>>>>>> +                }
>>>>>>>>>>>>>>        if (Debug.verboseOn()) Debug.logVerbose("type of
>>>>>>>>>>>>>> field " + entityName + "." + modelField.getName() +
>>>>>>>>>>>>>>                " is " + fieldClassName + ", was expecting "
>>>>>>>>>>>>>> + mft.getJavaType() + "; this may " +
>>>>>>>>>>>>>>                "indicate an error in the configuration or in
>>>>>>>>>>>>>> the class, and may result " +
>>>>>>>>>>>>>> @@ -749,7 +752,11 @@
>>>>>>>>>>>>>>        break;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>    case 2:
>>>>>>>>>>>>>> -                sqlP.setValue((java.sql.Timestamp)
>>>>>>>>>>>>>> fieldValue);
>>>>>>>>>>>>>> +                if (fieldValue instanceof String) {
>>>>>>>>>>>>>> +                    sqlP.setValueTimestampString((String)
>>>>>>>>>>>>>> fieldValue);
>>>>>>>>>>>>>> +                } else {
>>>>>>>>>>>>>> +                    sqlP.setValue((java.sql.Timestamp)
>>>>>>>>>>>>>> fieldValue);
>>>>>>>>>>>>>> +                }
>>>>>>>>>>>>>>        break;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>    case 3:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Regards
>>>>>>>>>>>>>> Scott
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 2008/10/13 Jacques Le Roux <[hidden email]>:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Done in revision: 703816
>>>>>>>>>>>>>>> It was not possible for PackingSlip.groovy and
>>>>>>>>>>>>>>> FindInventoryEventPlan.groovy. Because there the date string
>>>>>>>>>>>>>>> is
>>>>>>>>>>>>>>> build
>>>>>>>>>>>>>>> dynamically in the Groovy file
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Jacques
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> From: "Jacques Le Roux" <[hidden email]>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Adrian,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Yes good idea indeed, I will do that
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Jacques
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> From: "Adrian Crum" <[hidden email]>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Jacques,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Instead of modifying the groovy files, try specifying the
>>>>>>>>>>>>>>>>> data
>>>>>>>>>>>>>>>>> type in
>>>>>>>>>>>>>>>>> the screen widget.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Example in ReportFinancialSummaryScreens.xml:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> <set field="fromDate" from-field="parameters.fromDate"
>>>>>>>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>>>>>>>> <set field="thruDate" from-field="parameters.thruDate"
>>>>>>>>>>>>>>>>> type="Timestamp"/>
>>>>>>>>>>>>>>>>> <script
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> location="component://accounting/webapp/accounting/WEB-INF/actions/reports/TransactionTotals.groovy"/>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> -Adrian
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
>

[Adrian Crum]

Scott Gray wrote:
> Ok it makes more sense to me now, conversion needs to take place at a
> level where the relevant information is still available so doing it on
> a low level isn't really an option (even though that's been allowed to
> happen up until now).  I guess I was having trouble understanding why
> we needed to do all this work when everything had been running fine,
> but I see now that it wasn't really running that fine.

To be precise, some things in OFBiz have not been running fine since
September 2006 - https://issues.apache.org/jira/browse/OFBIZ-221.

-Adrian

[Scott Gray]

Thanks Adrian, I understand the implications now.

So what's the next step, should we add type checking to the
EntityConditions and then perhaps only enforce the correct types on
some volunteer machines until we can fix a good portion of the problem
code and then move it to the trunk?

Regards
Scott

2008/10/15 Adrian Crum <[hidden email]>:

> Scott Gray wrote:
>>
>> Ok it makes more sense to me now, conversion needs to take place at a
>> level where the relevant information is still available so doing it on
>> a low level isn't really an option (even though that's been allowed to
>> happen up until now).  I guess I was having trouble understanding why
>> we needed to do all this work when everything had been running fine,
>> but I see now that it wasn't really running that fine.
>
> To be precise, some things in OFBiz have not been running fine since
> September 2006 - https://issues.apache.org/jira/browse/OFBIZ-221.
>
> -Adrian
>

[Adrian Crum]

We could add a warning message to the EntityCondition classes (like the
one on line 411 of GenericEntity.java), then start checking the logs for
the warning messages.

-Adrian

Scott Gray wrote:

> Thanks Adrian, I understand the implications now.
>
> So what's the next step, should we add type checking to the
> EntityConditions and then perhaps only enforce the correct types on
> some volunteer machines until we can fix a good portion of the problem
> code and then move it to the trunk?
>
> Regards
> Scott
>
> 2008/10/15 Adrian Crum <[hidden email]>:
>> Scott Gray wrote:
>>> Ok it makes more sense to me now, conversion needs to take place at a
>>> level where the relevant information is still available so doing it on
>>> a low level isn't really an option (even though that's been allowed to
>>> happen up until now).  I guess I was having trouble understanding why
>>> we needed to do all this work when everything had been running fine,
>>> but I see now that it wasn't really running that fine.
>> To be precise, some things in OFBiz have not been running fine since
>> September 2006 - https://issues.apache.org/jira/browse/OFBIZ-221.
>>
>> -Adrian
>>
>

[Jacques Le Roux]

Yes, I suggested to create a separate log for that (easier) and to check it daily on the demo server

Jacques

From: "Adrian Crum" <[hidden email]>

> We could add a warning message to the EntityCondition classes (like the
> one on line 411 of GenericEntity.java), then start checking the logs for
> the warning messages.
>
> -Adrian
>
> Scott Gray wrote:
>> Thanks Adrian, I understand the implications now.
>>
>> So what's the next step, should we add type checking to the
>> EntityConditions and then perhaps only enforce the correct types on
>> some volunteer machines until we can fix a good portion of the problem
>> code and then move it to the trunk?
>>
>> Regards
>> Scott
>>
>> 2008/10/15 Adrian Crum <[hidden email]>:
>>> Scott Gray wrote:
>>>> Ok it makes more sense to me now, conversion needs to take place at a
>>>> level where the relevant information is still available so doing it on
>>>> a low level isn't really an option (even though that's been allowed to
>>>> happen up until now).  I guess I was having trouble understanding why
>>>> we needed to do all this work when everything had been running fine,
>>>> but I see now that it wasn't really running that fine.
>>> To be precise, some things in OFBiz have not been running fine since
>>> September 2006 - https://issues.apache.org/jira/browse/OFBIZ-221.
>>>
>>> -Adrian
>>>
>>
>

[Jacques Le Roux]

Maybe even easier, keep the specific log on a reasonnable period. Then remove duplicate log lines and fix in code. This will not
ensure no points are missing but it should help

Jacques

From: "Jacques Le Roux" <[hidden email]>

> Yes, I suggested to create a separate log for that (easier) and to check it daily on the demo server
>
> Jacques
>
> From: "Adrian Crum" <[hidden email]>
>> We could add a warning message to the EntityCondition classes (like the one on line 411 of GenericEntity.java), then start
>> checking the logs for the warning messages.
>>
>> -Adrian
>>
>> Scott Gray wrote:
>>> Thanks Adrian, I understand the implications now.
>>>
>>> So what's the next step, should we add type checking to the
>>> EntityConditions and then perhaps only enforce the correct types on
>>> some volunteer machines until we can fix a good portion of the problem
>>> code and then move it to the trunk?
>>>
>>> Regards
>>> Scott
>>>
>>> 2008/10/15 Adrian Crum <[hidden email]>:
>>>> Scott Gray wrote:
>>>>> Ok it makes more sense to me now, conversion needs to take place at a
>>>>> level where the relevant information is still available so doing it on
>>>>> a low level isn't really an option (even though that's been allowed to
>>>>> happen up until now).  I guess I was having trouble understanding why
>>>>> we needed to do all this work when everything had been running fine,
>>>>> but I see now that it wasn't really running that fine.
>>>> To be precise, some things in OFBiz have not been running fine since
>>>> September 2006 - https://issues.apache.org/jira/browse/OFBIZ-221.
>>>>
>>>> -Adrian
>>>>
>>>
>>
>

[Adrian Crum]

I spent some time looking through the entity condition code and I don't
think it is possible to set up a mismatched data type warning in the log.

Here's what I'm thinking:

1. Add a method to EntityUtil that will accept a Map, a
GenericDelegator, an entity name, a Locale, and a TimeZone. The method
would go through the Map and convert the Map values to the correct types.

2. Go through the *.java and *.groovy files and have them use the new
utility method where applicable.

3. Go through the simple method and screen widget code that constructs
entity conditions and have those bits of code convert data types before
constructing the entity conditions.

This won't catch all of the potential problems, but it should cover most
of them.

Let me know what you think.

-Adrian

Adrian Crum wrote:

> We could add a warning message to the EntityCondition classes (like the
> one on line 411 of GenericEntity.java), then start checking the logs for
> the warning messages.
>
> -Adrian
>
> Scott Gray wrote:
>> Thanks Adrian, I understand the implications now.
>>
>> So what's the next step, should we add type checking to the
>> EntityConditions and then perhaps only enforce the correct types on
>> some volunteer machines until we can fix a good portion of the problem
>> code and then move it to the trunk?
>>
>> Regards
>> Scott
>>
>> 2008/10/15 Adrian Crum <[hidden email]>:
>>> Scott Gray wrote:
>>>> Ok it makes more sense to me now, conversion needs to take place at a
>>>> level where the relevant information is still available so doing it on
>>>> a low level isn't really an option (even though that's been allowed to
>>>> happen up until now).  I guess I was having trouble understanding why
>>>> we needed to do all this work when everything had been running fine,
>>>> but I see now that it wasn't really running that fine.
>>> To be precise, some things in OFBiz have not been running fine since
>>> September 2006 - https://issues.apache.org/jira/browse/OFBIZ-221.
>>>
>>> -Adrian
>>>
>>
>

[David E Jones]

I was thinking that there may not be too many of these, but after  
playing around with it I now know I was wrong!

There will be quite a few, many caused by the Double/BigDecimal issue,  
but probably many others that are related to a String being passed in  
where another object is needed (ie to represent the parsed string).

We could do some stuff for Double vs BigDecimal. Right now I'm  
thinking that maybe we should change all fieldtype*.xml files to refer  
to BigDecimal instead of Double so that all warnings (or future  
exceptions) will push us toward BigDecimal instead of Double.

Any thoughts for/against that? We can change the fieldtype*.xml files  
without causing any problems right now (or it shouldn't anyway...).

-David


On Oct 15, 2008, at 10:36 AM, Adrian Crum wrote:

> I spent some time looking through the entity condition code and I  
> don't think it is possible to set up a mismatched data type warning  
> in the log.
>
> Here's what I'm thinking:
>
> 1. Add a method to EntityUtil that will accept a Map, a  
> GenericDelegator, an entity name, a Locale, and a TimeZone. The  
> method would go through the Map and convert the Map values to the  
> correct types.
>
> 2. Go through the *.java and *.groovy files and have them use the  
> new utility method where applicable.
>
> 3. Go through the simple method and screen widget code that  
> constructs entity conditions and have those bits of code convert  
> data types before constructing the entity conditions.
>
> This won't catch all of the potential problems, but it should cover  
> most of them.
>
> Let me know what you think.
>
> -Adrian
>
> Adrian Crum wrote:
>> We could add a warning message to the EntityCondition classes (like  
>> the one on line 411 of GenericEntity.java), then start checking the  
>> logs for the warning messages.
>> -Adrian
>> Scott Gray wrote:
>>> Thanks Adrian, I understand the implications now.
>>>
>>> So what's the next step, should we add type checking to the
>>> EntityConditions and then perhaps only enforce the correct types on
>>> some volunteer machines until we can fix a good portion of the  
>>> problem
>>> code and then move it to the trunk?
>>>
>>> Regards
>>> Scott
>>>
>>> 2008/10/15 Adrian Crum <[hidden email]>:
>>>> Scott Gray wrote:
>>>>> Ok it makes more sense to me now, conversion needs to take place  
>>>>> at a
>>>>> level where the relevant information is still available so doing  
>>>>> it on
>>>>> a low level isn't really an option (even though that's been  
>>>>> allowed to
>>>>> happen up until now).  I guess I was having trouble  
>>>>> understanding why
>>>>> we needed to do all this work when everything had been running  
>>>>> fine,
>>>>> but I see now that it wasn't really running that fine.
>>>> To be precise, some things in OFBiz have not been running fine  
>>>> since
>>>> September 2006 - https://issues.apache.org/jira/browse/OFBIZ-221.
>>>>
>>>> -Adrian
>>>>
>>>

[Adrian Crum-2]

Anything done to move it along is okay with me.

I spent a day on steps 1 and 3 that I proposed. I couldn't get the code to work right away and I needed to get other things done, so I stopped working on it. I could post a patch if anyone wants to take a stab at it.

-Adrian


--- On Sun, 10/19/08, David E Jones <[hidden email]> wrote:

> From: David E Jones <[hidden email]>
> Subject: Re: Automatic Type Conversion for GenericEntity/Value.set (was Re: Revision 703731)
> To: [hidden email]
> Date: Sunday, October 19, 2008, 1:54 AM
> I was thinking that there may not be too many of these, but
> after  
> playing around with it I now know I was wrong!
>
> There will be quite a few, many caused by the
> Double/BigDecimal issue,  
> but probably many others that are related to a String being
> passed in  
> where another object is needed (ie to represent the parsed
> string).
>
> We could do some stuff for Double vs BigDecimal. Right now
> I'm  
> thinking that maybe we should change all fieldtype*.xml
> files to refer  
> to BigDecimal instead of Double so that all warnings (or
> future  
> exceptions) will push us toward BigDecimal instead of
> Double.
>
> Any thoughts for/against that? We can change the
> fieldtype*.xml files  
> without causing any problems right now (or it shouldn't
> anyway...).
>
> -David
>
>
> On Oct 15, 2008, at 10:36 AM, Adrian Crum wrote:
>
> > I spent some time looking through the entity condition
> code and I  
> > don't think it is possible to set up a mismatched
> data type warning  
> > in the log.
> >
> > Here's what I'm thinking:
> >
> > 1. Add a method to EntityUtil that will accept a Map,
> a  
> > GenericDelegator, an entity name, a Locale, and a
> TimeZone. The  
> > method would go through the Map and convert the Map
> values to the  
> > correct types.
> >
> > 2. Go through the *.java and *.groovy files and have
> them use the  
> > new utility method where applicable.
> >
> > 3. Go through the simple method and screen widget code
> that  
> > constructs entity conditions and have those bits of
> code convert  
> > data types before constructing the entity conditions.
> >
> > This won't catch all of the potential problems,
> but it should cover  
> > most of them.
> >
> > Let me know what you think.
> >
> > -Adrian
> >
> > Adrian Crum wrote:
> >> We could add a warning message to the
> EntityCondition classes (like  
> >> the one on line 411 of GenericEntity.java), then
> start checking the  
> >> logs for the warning messages.
> >> -Adrian
> >> Scott Gray wrote:
> >>> Thanks Adrian, I understand the implications
> now.
> >>>
> >>> So what's the next step, should we add
> type checking to the
> >>> EntityConditions and then perhaps only enforce
> the correct types on
> >>> some volunteer machines until we can fix a
> good portion of the  
> >>> problem
> >>> code and then move it to the trunk?
> >>>
> >>> Regards
> >>> Scott
> >>>
> >>> 2008/10/15 Adrian Crum
> <[hidden email]>:
> >>>> Scott Gray wrote:
> >>>>> Ok it makes more sense to me now,
> conversion needs to take place  
> >>>>> at a
> >>>>> level where the relevant information
> is still available so doing  
> >>>>> it on
> >>>>> a low level isn't really an option
> (even though that's been  
> >>>>> allowed to
> >>>>> happen up until now).  I guess I was
> having trouble  
> >>>>> understanding why
> >>>>> we needed to do all this work when
> everything had been running  
> >>>>> fine,
> >>>>> but I see now that it wasn't
> really running that fine.
> >>>> To be precise, some things in OFBiz have
> not been running fine  
> >>>> since
> >>>> September 2006 -
> https://issues.apache.org/jira/browse/OFBIZ-221.
> >>>>
> >>>> -Adrian
> >>>>
> >>>

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around
http://mail.yahoo.com