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 <[EMAIL PROTECTED]>:
>
> On Oct 13, 2008, at 10:10 PM, Scott Gray wrote:
>
>> 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.
>
> 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 <[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
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>
>>>
>
>
