Fwd: Review request for 7145913 CachedRowSetSwriter.insertNewRow() throws SQLException

[Lance Andersen - Oracle]

Still looking for reviewer

Best
Lance

Begin forwarded message:

> From: Lance Andersen - Oracle <Lance.Andersen at oracle.com>
> Date: May 31, 2012 6:16:16 PM EDT
> Cc: core-libs-dev core-libs-dev <core-libs-dev at 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.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?
>> 
>> * 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.Andersen at oracle.com
> 


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