Hi Paul, Thank you for taking the time to review the code.
I made the change you suggested below http://cr.openjdk.java.net/~lancea/7145913/webrev.02 Let me know if you are good with the change and I will get this puppy put back. Best Lance On Jun 13, 2012, at 3:53 AM, Paul Sandoz wrote: > Hi Lance, > > It looks OK to me, however i don't know much about JDBC. Much cleaner than > before. > > Very minor point, you can shoot me for being pedantic!: from line 894 you can > do the following since the return value from pstmt.executeUpdate() is never > used: > > 894 try { > 895 // int i; > 896 for (int i = 1; i <= icolCount; i++) { > 897 Object obj = crs.getObject(i); > 898 if (obj != null) { > 899 pstmt.setObject(i, obj); > 900 } else { > 901 > pstmt.setNull(i,crs.getMetaData().getColumnType(i)); > 902 } > 903 } > 904 > 905 pstmt.executeUpdate(); > 906 return false; > 907 > 908 } catch (SQLException ex) { > Paul. > > On Jun 1, 2012, at 12:16 AM, Lance Andersen - Oracle wrote: > >> Here is the revision using the try with resources as David suggested >> >> http://cr.openjdk.java.net/~lancea/7145913/webrev.01/ >> >> Best >> Lance >> On May 31, 2012, at 3:10 PM, David Schlosnagle wrote: >> >>> On Thu, May 31, 2012 at 1:50 PM, Lance Andersen - Oracle >>> <lance.ander...@oracle.com> wrote: >>>> Hi, >>>> >>>> Looking for a reviewer for 7145913. This addresses an issue with where a >>>> SQLException could be thrown when writing a row back with an >>>> auto-incremented column on some databases. >>>> >>>> Webrev is http://cr.openjdk.java.net/~lancea/7145913/webrev.00/. RowSet >>>> TCK, sqe tests and unit tests all pass with this fix. >>> >>> Hi Lance, >>> >>> I had a few quick comments: >>> >>> * Do you need the multiple calls to CachedResultSet.getObject for the >>> primary >>> key, or would it be worthwhile to store the result in a local variable? >>> >>> * This seems to be pre-existing before your changes, but the >>> PreparedStatement >>> pstmtSel and ResultSet rs, rs2 used within the insertNewRow method are never >>> closed, which could lead to resource leaks. >>> >>> * Minor nit, the indentation in this file seems off, especially for the >>> method >>> JavaDoc. >>> >>> I've included a minor cleanup below of the insertNewRow method using >>> try-with-resources. >>> >>> Thanks, >>> Dave >>> >>> >>> /** >>> * Inserts a row that has been inserted into the given >>> * <code>CachedRowSet</code> object into the data source from which >>> * the rowset is derived, returning <code>false</code> if the insertion >>> * was successful. >>> * >>> * @param crs the <code>CachedRowSet</code> object that has had a >>> row inserted >>> * and to whose underlying data source the row will be inserted >>> * @param pstmt the <code>PreparedStatement</code> object that will be used >>> * to execute the insertion >>> * @return <code>false</code> to indicate that the insertion was >>> successful; >>> * <code>true</code> otherwise >>> * @throws SQLException if a database access error occurs >>> */ >>> private boolean insertNewRow(CachedRowSet crs, >>> PreparedStatement pstmt, CachedRowSetImpl crsRes) throws SQLException { >>> >>> boolean returnVal = false; >>> >>> try (PreparedStatement pstmtSel = con.prepareStatement(selectCmd, >>> ResultSet.TYPE_SCROLL_SENSITIVE, >>> ResultSet.CONCUR_READ_ONLY); >>> ResultSet rs = pstmtSel.executeQuery(); >>> ResultSet rs2 = con.getMetaData().getPrimaryKeys(null, null, >>> crs.getTableName()) >>> ) { >>> >>> ResultSetMetaData rsmd = crs.getMetaData(); >>> int icolCount = rsmd.getColumnCount(); >>> String[] primaryKeys = new String[icolCount]; >>> int k = 0; >>> while (rs2.next()) { >>> primaryKeys[k] = rs2.getString("COLUMN_NAME"); >>> k++; >>> } >>> >>> if (rs.next()) { >>> for (String pkName : primaryKeys) { >>> if (!isPKNameValid(pkName, rsmd)) { >>> >>> /* We came here as one of the the primary keys >>> * of the table is not present in the cached >>> * rowset object, it should be an autoincrement column >>> * and not included while creating CachedRowSet >>> * Object, proceed to check for other primary keys >>> */ >>> continue; >>> } >>> >>> Object crsPK = crs.getObject(pkName); >>> if (crsPK == null) { >>> /* >>> * It is possible that the PK is null on some databases >>> * and will be filled in at insert time (MySQL >>> for example) >>> */ >>> break; >>> } >>> >>> String rsPK = rs.getObject(pkName).toString(); >>> if (crsPK.toString().equals(rsPK)) { >>> returnVal = true; >>> this.crsResolve.moveToInsertRow(); >>> for (int i = 1; i <= icolCount; i++) { >>> String colname = >>> (rs.getMetaData()).getColumnName(i); >>> if (colname.equals(pkName)) >>> this.crsResolve.updateObject(i,rsPK); >>> else >>> this.crsResolve.updateNull(i); >>> } >>> this.crsResolve.insertRow(); >>> this.crsResolve.moveToCurrentRow(); >>> } >>> } >>> } >>> >>> if (returnVal) { >>> return returnVal; >>> } >>> >>> try { >>> int i; >>> for (i = 1; i <= icolCount; i++) { >>> Object obj = crs.getObject(i); >>> if (obj != null) { >>> pstmt.setObject(i, obj); >>> } else { >>> pstmt.setNull(i,crs.getMetaData().getColumnType(i)); >>> } >>> } >>> >>> i = pstmt.executeUpdate(); >>> return false; >>> >>> } catch (SQLException ex) { >>> /* >>> * Cursor will come here if executeUpdate fails. >>> * There can be many reasons why the insertion failed, >>> * one can be violation of primary key. >>> * Hence we cannot exactly identify why the insertion failed >>> * Present the current row as a null row to the user. >>> **/ >>> this.crsResolve.moveToInsertRow(); >>> >>> for (int i = 1; i <= icolCount; i++) { >>> this.crsResolve.updateNull(i); >>> } >>> >>> this.crsResolve.insertRow(); >>> this.crsResolve.moveToCurrentRow(); >>> >>> return true; >>> } >>> } >>> } >> >> >> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 >> Oracle Java Engineering >> 1 Network Drive >> Burlington, MA 01803 >> lance.ander...@oracle.com >> > Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
