Inline:
2008/10/14 David E Jones <[EMAIL PROTECTED]>:
>
> 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 <[EMAIL PROTECTED]>:
>>>
>>> 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 <[EMAIL PROTECTED]>:
>>>>>>>>>>
>>>>>>>>>> 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" <[EMAIL PROTECTED]>
>>>>>>>>>>>
>>>>>>>>>>> Adrian,
>>>>>>>>>>>
>>>>>>>>>>> Yes good idea indeed, I will do that
>>>>>>>>>>>
>>>>>>>>>>> Jacques
>>>>>>>>>>>
>>>>>>>>>>> From: "Adrian Crum" <[EMAIL PROTECTED]>
>>>>>>>>>>>>
>>>>>>>>>>>> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>
>>>>
>>>
>
>
