db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Fernanda Pizzorno (JIRA)" <derby-...@db.apache.org>
Subject [jira] Commented: (DERBY-1251) cancelRowUpdates() affects rows updated with updateRow() in scrollable updatable resultsets
Date Tue, 09 May 2006 14:50:22 GMT
    [ http://issues.apache.org/jira/browse/DERBY-1251?page=comments#action_12378628 ] 

Fernanda Pizzorno commented on DERBY-1251:
------------------------------------------

Review:

The patch looks good, here are some comments:

Index: java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java
===================================================================
@@ -180,10 +181,17 @@
[...]
+	/* updateRow is used to keep the values which are updated with updateXXX() 
+	 * calls. It is by both insertRow() and updateRow(). 
                           ^ used
+	 * It is initialized to null if the resultset is not updatable. 
+	 */
[...]
+	/* These are the columns which have been updated so far. 
+	 * Used to build UPDATE...WHERE CURRENT OF sql and 
                                                       *** could this be removed?
+	 * INSERT INTO .. statements.
+	 */

@@ -240,14 +248,26 @@
 
[...]
+		    columnGotUpdated = new boolean[columnCount];
+		    updateRow = factory.getValueRow(columnCount);
+			for (int i = 1; i <= columnGotUpdated.length; i++) {
+		        updateRow.setColumn(i, resultDescription.getColumnDescriptor(i).
+		            getType().getNull());
+		    }
+		    initializeUpdateRowModifiers();			
+		} else {
+			updateRow = null;

Could you use columnCount instead of columnGotUpdated.length on the third line?
There is a mix of tab and spaces being used on these rows.

@ -289,14 +309,18 @@
 	// onRow protects us from making requests of
 	// resultSet that would fail with NullPointerExceptions
 	// or milder problems due to not having a row.
[...]
+	protected final void checkOnRow() throws SQLException	{
+		if (currentRow.getRowArray() == null) {
 			throw newSQLException(SQLState.NO_CURRENT_ROW);
+		} 
+	}

This comments should be updated to reflect what is done in the method.

[...]

+	/**
+	 * Initializes the updateRow and columnGotUpdated fields
+	 */
+	private void initializeUpdateRowModifiers() {
+		currentRowHasBeenUpdated = false;
+		Arrays.fill(columnGotUpdated, false);
 	}

This comment should also be updated, updateRow is not being initialized.

[...]

@@ -3598,15 +3620,19 @@
                 }
                 rs.close();
                 rs.finish();
-                //After a delete, the ResultSet will be positioned right before 
-                //the next row.
-                rowData = null;
-                currentRow = null;
+                //For forward only resultsets, after a delete, 
+                // the ResultSet will be positioned right before the next row.
+                if (getType() == TYPE_FORWARD_ONLY) {
+                    currentRow.setRowArray(null);
+                } else {
+                    movePosition(RELATIVE, 0, "relative");
+                }

According to JDBC 3.0 specification section "14.2.4.2 Deleting a Row", after a
deleteRow has been called, the cursor will be positioned before the next valid
row. Is there a reason to make this comment specific to forward only result 
sets?


Index: java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/SURTest.java
===================================================================

@@ -242,6 +242,218 @@
         verifyTuple(rs);
         assertFailOnUpdate(rs);
     }
+
+    /**
+     * Test that you can correctly run multiple updateXXX() + updateRow() 
+     * combined with cancelRowUpdates().
+     */
+    public void testMultiUpdateRow1() 
+        throws SQLException 
+    {
[...]
+        assertEquals("Expected the resultset detect the updates of previous" + 
+                     "updateRow after cancelRowUpdates", newCol2, rs.getInt(2));
+        assertEquals("Expected the resultset detect the updates of previous" + 
+                     "updateRow after cancelRowUpdates", newCol2, rs.getInt(2));

I guess the intention was to check column 2 and 3 and not twice column 2.


+        assertTrue("Expected rs.rowUpdated() to be true after " + 
+                   "updateRow and cancelRowUpdates", rs.rowUpdated());
+        
+        rs.close();
+    }
+
+    /**
+     * Test that you can correctly run multiple updateNull() + updateRow() 
+     * combined with cancelRowUpdates().
+     */
+    public void testMultiUpdateRow2() 
+        throws SQLException 
+    {
[...]


+        assertEquals("Expected the resultset detect the updates of previous" + 
+                     "updateRow after cancelRowUpdates", 0, rs.getInt(2));
+        assertEquals("Expected the resultset detect the updates of previous" + 
+                     "updateRow after cancelRowUpdates", 0, rs.getInt(2));

Same here.

+        assertTrue("Expected rs.rowUpdated() to be true after " + 
+                   "updateRow and cancelRowUpdates", rs.rowUpdated());
+        
+        rs.close();
+    }
+
+    /**
+     * Test that you get cursor operation conflict warning if updating 
+     * a row which has been deleted from the table.
+     */



> cancelRowUpdates() affects rows updated with updateRow() in scrollable updatable resultsets
> -------------------------------------------------------------------------------------------
>
>          Key: DERBY-1251
>          URL: http://issues.apache.org/jira/browse/DERBY-1251
>      Project: Derby
>         Type: Bug

>   Components: JDBC, Network Client
>     Versions: 10.2.0.0
>     Reporter: Andreas Korneliussen
>     Assignee: Andreas Korneliussen
>     Priority: Minor
>  Attachments: DERBY-1251.diff, DERBY-1251.stat, DERBY-1251v2.diff, DERBY-1251v2.stat,
derbyall_report.txt, derbyall_report.txt
>
> If an application does the following:
> rs.updateInt(1, newValueCol1);
> rs.updateRow();
> rs.updateInt(2, newValueCol2);
> rs.cancelRowUpdates();
> Then, when calling rs.getInt(1), it will return the old value. Instead it should return
the new value.
> Workaround: after calling rs.updateRow(), the application could call rs.relative(0).
> This problem does not affect forward only resultsets, since after an updateRow() they
get positoned before the next row, leaving it impossible to do anything with the current row.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


Mime
View raw message