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