rzo1 commented on code in PR #144:
URL: https://github.com/apache/openjpa/pull/144#discussion_r3437896915
##########
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/BatchingPreparedStatementManagerImpl.java:
##########
@@ -254,13 +254,22 @@ private void flushSingleRow(RowImpl row,
PreparedStatement ps)
row.flush(ps, _dict, _store);
int count = executeUpdate(ps, row.getSQL(_dict), row);
if (count != 1) {
- logSQLWarnings(ps);
- Object failed = row.getFailedObject();
- if (failed != null)
- _exceptions.add(new OptimisticException(failed));
- else if (row.getAction() == Row.ACTION_INSERT)
- throw new SQLException(_loc.get("update-failed-no-failed-obj",
- String.valueOf(count), row.getSQL(_dict)).getMessage());
+ // For DELETE actions, tolerate count=0 because the row may
+ // have already been removed by a database-level ON DELETE
+ // CASCADE triggered by a related row's deletion.
+ if (count == 0 && row.getAction() == Row.ACTION_DELETE) {
Review Comment:
Good catch - I think as written this is too broad.
This was added for two TCK tests (persistMX1Test2, cascadeAllMX1Test2) where
a parent and child are removed in the same tx and the FK has a DB-level ON
DELETE CASCADE. OpenJPA flushes the parent DELETE first, the DB cascades and
removes the child row, and then OpenJPA's own DELETE of the child returns count
== 0. Before this change that 0 always produced a spurious OptimisticException,
because RowManagerImpl.getRow() sets a non-null failed object on essentially
every managed row; so the old code couldn't tell "benign already-gone" from a
real conflict either.
But you're right on the spec point (just checked again): count == 0 on a
DELETE also legitimately means an optimistic-lock failure. For a @Version
entity the SQL is DELETE .. WHERE id = ? AND version = ?, and 0 rows there is
a concurrent update/delete that the spec says must surface as
OptimisticLockException.
As currently written we'd swallow that, and the caller loses the only signal
we had - exactly your "how do I know it was a no-op" concern. The TCK entities
here are non-versioned, which is why it slipped through.
We could gate/check for an entity having no version field; tolerate count ==
0 on DELETE only when there's no @Version (so there's no optimistic signal to
honor), and otherwise keep the existing OptimisticException path for
flushAndUpdate, flushSingleRow, and checkUpdateCount case 0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
