Detectability methods rowUpdated, rowInserted, rowDeleted can be called from illegal states in both clients

• Key: DERBY-1323
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Minor
• Assignee: Dag H. Wanvik
• Reporter: Dag H. Wanvik
• Component/s: JDBC, Network Client
• Affects Version/s: 10.2.1.6
• Fix Version/s: 10.2.1.6

Description

Please see enclosed repro.

These detectability methods fail to check that they can be called in
some states. In the repro, calls are allowed while on insert row and
when after last row. These should both fail. Both clients have the
same problem. All three detectability methods have the same problem.
(repro only shows it for a subset of the cases).

Comments

[Dag H. Wanvik]

This patch adds checks for the cases described below, for both
clients, in the JDBC ResultSet API methods rowUpdated, rowDeleted and
rowInserted.

In all cases, the exception "no current row" (SQLState 24000) is
thrown. A new test has been added to jdbcapi/SURTest.java to exercise
all cases.

The JDBC API and specification does not state explicitly what to do in
these situations, as far as I can see. For the "forward only" case, a
slight, albeit unlikely regression is introduced. I will back out this
change if people think it is risky. I view it as a correction, though.
This is the reason for the changed master files for
lang/updatableResultSet.java.

Now, the cases:

For result sets of type TYPE_SCROLL_INSENSITIVE and CONCUR_UPDATABLE:
- before positioning
- on insertRow
- on beforeFirst row
- on afterLast row
- after deleteRow, before new positioning

For result sets of type TYPE_FORWARD_ONLY and CONCUR_UPDATABLE:
- before positioning (***)
- on insertRow
- after updateRow, before new positioning (*)
- after deleteRow, before new positioning (**)
- after rs emptied out (***)

Detectability is currently not implemented for "forward only" result
sets and the methods always used to return false, even in the states
indicated. The new checks added by this patch (needed for scrollable
result sets) introduce a slight regression, in that calls may now
throw an exception. Cases (*) and (**) broke the regression test as
mentioned, but I think the use of detection methods in these states is
rather meaningless, since Derby can no longer access a row in these
cases anyway - until a next() is performed. Even less problematic
should be {TYPE_FORWARD_ONLY, CONCUR_READ_ONLY} in the cases (***).

I have run derbyall with no related errors and the patch is ready for
review.

[Fernanda Pizzorno]

Review:

The patch looks good. I just have a couple of questions.

1. java/client/org/apache/derby/client/am/ResultSet.java

This check has been added in several places:

+ if (isOnInsertRow_ || !isValidCursorPosition_) {
+ throw new SqlException
+ (agent_.logWriter_,
+ new ClientMessageId(SQLState.NO_CURRENT_ROW));
+ }

Could it be put in a private method?

2. java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/SURTest.java

Is it really necessary to use reflection?

[Dag H. Wanvik]

This is version 2 of this patch. It addresses both comments of Fernanda's review.
Changes were only made to client code and SURTest.java, so i ran
derbynetclientmats again as well as SURTest on embedded without
problems(*).

(*) derbynetclientmats failed on NSinSameJVM.java and DerbyNetNewServer,
though - I don't think it is related; I ran on a sane build from the classes directory with
Sun JDK 1.5.

[Fernanda Pizzorno]

All of my comments have been addressed in the DERBY-1323-2.diff.

The following changes seem unnecessary to me:
[...]
+ checkForClosedResultSet();
+ checkPositionedOnPlainRow();
+
             boolean rowInserted = false;
- checkForClosedResultSet();
[...]
- boolean rowDeleted = (resultSetType_ == ResultSet.TYPE_SCROLL_INSENSITIVE) ?
- cursor_.getIsUpdateDeleteHole() :
- false;
+ boolean rowDeleted =
+ (resultSetType_ == ResultSet.TYPE_SCROLL_INSENSITIVE) ?
+ cursor_.getIsUpdateDeleteHole() : false;
[...]

I have seen comments against cosmetic changes before, but I am not sure we have a policy against it.

[Bernt M. Johnsen]

Committed revision 408875.

[Dag H. Wanvik]

Release note for this issue:


PROBLEM

For a JDBC ResultSet with type TYPE_FORWARD_ONLY, the methods
rowUpdated, rowDeleted and rowInserted could previously be called
while not on a row, i.e. before positioning in the result set, while
on insertRow, after updateRow before new positioning, after deleteRow
before new positioning and when after last row. This is now
disallowed.

SYMPTOMS

Calls to any of these methods while not on a row will now throw
SQLException with SQLState 24000.

CAUSE

Derby now disallows these calls when not on a row.

SOLUTION

Change the application to not call these methods unless on a row. Note
that using them at all is rather meaningless for a ResultSet of type
TYPE_FORWARD_ONLY since the returned result will always be 'false'.
This is because once you modify a row, it can no longer be accessed,
you need to move to the next row, if there is one, to get a new
current row. Presently in Derby, these methods are only really
meaningfully useed for result sets of type TYPE_SCROLL_INSENSITIVE and
of concurrency CONCUR_UPDATABLE in which case updated and deleted rows
can be detected.

WORKAROUND

None.