Review request for 7145913 CachedRowSetSwriter.insertNewRow() throws SQLException

[Lance Andersen - Oracle]

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