[
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
