Closes #448 - Fix C7 Locking-Query Parameter-Binding#451
Conversation
|
| try (PreparedStatement preparedStatement = connection.prepareStatement(sql)) { | ||
| preparedStatement.setInt(1, remainingRetries); | ||
| if (lockDuration != null) { | ||
| preparedStatement.setTimestamp(1, Timestamp.from(Instant.now())); | ||
| preparedStatement.setTimestamp(2, Timestamp.from(Instant.now())); | ||
| } | ||
| ResultSet camundaTaskEventFilteredByRetriesResultSet = preparedStatement.executeQuery(); | ||
| camunda7TaskEventsFilteredByRetries = |
There was a problem hiding this comment.
Most critical finding.
Wrong Parameter-Binding did not set lock_expire < ? but falsely remaining_retires = ?.
Here is e.g. the Oracle prepared statement query:
public String getAvailableEventsFilteredByRetries(String schema) {
final String sql =
"select * from %s.event_store where remaining_retries = ? and (lock_expire "
+ "< ? or lock_expire is null) for update skip locked";
return String.format(sql, schema);I wonder here: this should have errored out due to non-binded parameter.
Yet the queries succeeded - signaling a default being set.
Depending on the default this may explain the observed bug with multiple Adapter-instances running in a cluster and processing the same events.
Debug-Logs of actually executed queries will help here.
There was a problem hiding this comment.
Extensive studies revealed that that code-path isn't walked without Example/Test-Setup.
Hence why the expected SQL-Binding error never came up.
There was a problem hiding this comment.
Depending on the default this may explain the observed bug with multiple Adapter-instances running in a cluster and processing the same events.
This can therefore be ruled out.
Subsequent PRs to highly likely show reason for the bug being wrong transaction boundaries.
| @Override | ||
| public String getAvailableEventsFilteredByRetries(String schema) { | ||
| final String sql = | ||
| "select * from %s.event_store where remaining_retries = ? and lock_expire " | ||
| + "< ? or lock_expire is null for update skip locked data"; | ||
| "select * from %s.event_store where remaining_retries = ? and (lock_expire " | ||
| + "< ? or lock_expire is null) for update skip locked data"; | ||
| return String.format(sql, schema); |
There was a problem hiding this comment.
Wrong precedence for all providers.
Probably not related to aforementioned bug - still semantically wrong query.
|
Well done and looks good so far. Do the added tests fail without your changes? |
Yeah they do. See https://github.com/kadai-io/KadaiAdapter/actions/runs/27818009070/job/82324158828 of sibling PR |
2 participants
Definition of Done
In ReviewCloses #ISSUE_ID - PROBLEM/SOLUTION