Inline:
2008/10/14 David E Jones <[EMAIL PROTECTED]>:
>
> 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 <[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
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>
>
