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 PostgresJDBCdriver is that they used to auto-convert from a String to whatever thedatabase 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 inthe correct sql type then we get String:timestamp and everything worksfine. 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 andexpecting 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 driverand/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 thatgenerically 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 needto add similar code to the EntityCondition stuff for find queries(which is where Jacques noticed the problem, I haven't checked for theerror on any operations involving GenericEntity).Good point, we'll have to make sure the object types passed into those arevalid as well. -DavidOn 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 notmatch the column type, in the timestamp case if you passed in a parameter specified as varchar it would automatically attempt toconvert 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 specifythe correct sql type. If letting the database take care of the type conversion has neverbeen 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 asvarchar 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 ispassedlike the commented out code in GenericEntity referenced above does."And I'm saying we could do that, but it's going to get interestingbecause alot of code passes Strings into the entity engine. In other words, ifwethrow a "hard" exception, there is a good chance that much of OFBizwon't run.Personally, I'd like to see better handling of object types in thehigherlevel code. I fixed a problem over the weekend that was caused by thisvery thing (passing Strings into the entity engine). -Adrian David E Jones wrote:I'm referring to the exception I described in the GenericEntity.javafile,lines 410-415. Right now it is just a warning in the log (and has beenforyears). The reason to do it there instead of letting the JDBC driverdo itis that not all developers will test on all possible databases, andthis will help avoid errors as people use different databases in development/testing or deployment.I tried to describe two possible approaches to improve this situationinmy first message. Please let me know if those alternatives were notclear 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 basedonexceptions being thrown by his JDBC driver. Other DBs seemed to beworking okay.I guess we could throw our own exception to keep things consistentacrossdatabases. It will be interesting to see how that affects existingcode. -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 throwanexception if you pass in something that is not the correct objecttype. 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 toambiguity 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 decisionwasmade quite a while back to not enforce the correct object type. There is code in the GenericEntity.java file (lines 410-415) tocheckto see if the value passed in matches the object type for thefield 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 throwanexception when the wrong data type is passed like the commentedout code in GenericEntity referenced above does.The most common form of automatic conversion is from String towhatever 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 automatictypeconversion to try to fix programming errors automatically, or do afail-fastby throwing an exception when the object type is wrong? I supposethe 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 toGroovy that is making this necessary, it appears to be entirely a postgresql issue.When executing a prepared statement postgresql seems to requirethatthe parameter list sql types match the column types. So theproblemisn't that we are passing in a string but that we are setting thesqltype 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 frompeople 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) throwsSQLException { + if (field != null) { + _ps.setObject(_ind, field, Types.TIMESTAMP); + } else { + _ps.setNull(_ind, Types.TIMESTAMP); + } + _ind++; + } + + /**+ * Set the next binding variable of the currently activeprepared 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 inthe 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 andFindInventoryEventPlan.groovy. Because there the date string isbuild 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 datatype 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"/> <scriptlocation="component://accounting/webapp/accounting/WEB- INF/actions/reports/TransactionTotals.groovy"/>-Adrian
