Review request for 7145913 CachedRowSetSwriter.insertNewRow() throws SQLException

Lance Andersen - Oracle Lance.Andersen at oracle.com
Wed Jun 13 11:04:22 UTC 2012


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




More information about the core-libs-dev mailing list