Still looking for reviewer Best Lance
Begin forwarded message: > From: Lance Andersen - Oracle <lance.ander...@oracle.com> > Date: May 31, 2012 6:16:16 PM EDT > Cc: core-libs-dev core-libs-dev <core-libs-dev@openjdk.java.net> > Subject: Re: Review request for 7145913 CachedRowSetSwriter.insertNewRow() > throws SQLException > > 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
