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 <[EMAIL PROTECTED]>:
> 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 <[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
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
>
