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?
What I'm proposing is to fix this further up the stack in the
GenericEntity.set method instead of lower down. I guess either way we
have the two options I mentioned earlier:
1. fail fast(er) by having OFBiz throw an exception
2. try to auto-convert
#1 will result, as Adrian mentioned, in a bunch of possibly
undiscovered places where the wrong object type is passed in, causing
OFBiz to not function until these are cleaned up. On the other hand,
in order to reliably work on most databases these need to be fixed
sooner or later.
#2 has the problem, also as Adrian mentioned, of possibly not doing
the conversion correctly, especially in cases where locale-sensitive
parsing is needed or there is a special format string or something.
In spite of the short-term pain my vote is still for #1, and to do it
higher up in the GenericEntity.set method in order to fail right when
it's set, not later on when the GenericDelegator method is called, so
that we know where the bad object type is being passed.
-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
