Review request for 7145913 CachedRowSetSwriter.insertNewRow() throws SQLException (original) (raw)

Lance Andersen - Oracle Lance.Andersen at oracle.com
Thu May 31 19:48:13 UTC 2012

Hi David,

Thanks for the feedback On May 31, 2012, at 3:10 PM, David Schlosnagle wrote:

On Thu, May 31, 2012 at 1:50 PM, Lance Andersen - Oracle <Lance.Andersen at 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?

I do not think it will be a huge performance gain as typically there is just 1 column for the PK, occasionally 2 seldom more.

* 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.

Yep, that dates back to when this method was written and should be cleaned up as part of this change.

* Minor nit, the indentation in this file seems off, especially for the method JavaDoc.

True. I do not want to touch the entire class via this bug but makes sense to clean up the indentation for the comments for this method.

I can address other formatting via another bug as it keeps it cleaner as to why changes are being made.

I've included a minor cleanup below of the insertNewRow method using try-with-resources.

Thanks for the suggestion and tweak

Best Lance

Thanks, Dave

/** * Inserts a row that has been inserted into the given * CachedRowSet object into the data source from which * the rowset is derived, returning false if the insertion * was successful. * * @param crs the CachedRowSet object that has had a row inserted * and to whose underlying data source the row will be inserted * @param pstmt the PreparedStatement object that will be used * to execute the insertion * @return false to indicate that the insertion was successful; * true 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.TYPESCROLLSENSITIVE, ResultSet.CONCURREADONLY); 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("COLUMNNAME"); 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.Andersen at oracle.com

More information about the core-libs-dev mailing list