Re: Automatic Type Conversion for GenericEntity/Value.set (was Re: Revision 703731)

[David E Jones]

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