Skip to content

Conversation

@nickvergessen
Copy link
Member

Summary

Explanation Screenshot
Previously even when the precondition did not match, the call "passed" when the after value was the expected one. This however can lead to race conditions, duplicate code executions and other things. grafik

Checklist

Previously even when the precondition did not match, the call "passed"
when the after value was the expected one. This however can lead to
race conditions, duplicate code excutions and other things.

Signed-off-by: Joas Schilling <[email protected]>
@nickvergessen nickvergessen added bug 3. to review Waiting for reviews labels Sep 13, 2024
@nickvergessen nickvergessen added this to the Nextcloud 31 milestone Sep 13, 2024
@nickvergessen nickvergessen self-assigned this Sep 13, 2024
@nickvergessen nickvergessen requested review from a team, come-nc, skjnldsv and yemkareems and removed request for a team September 13, 2024 07:44
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make any sense without a transaction or a lock around it anyway? What prevents the DB value to be changed by another process between precondition check and value update?

@nickvergessen
Copy link
Member Author

What prevents the DB value to be changed by another process between precondition check and value update?

There is another check inside that takes care of it (no changed row while the precondition is in the WHERE clause):

if ($affected === 0 && !empty($updatePreconditionValues)) {
throw new PreConditionNotMetException();
}

@nickvergessen nickvergessen merged commit df155fa into master Sep 13, 2024
@nickvergessen nickvergessen deleted the bugfix/noid/throw-precondition-failure-when-not-matching branch September 13, 2024 11:39
@nickvergessen
Copy link
Member Author

/backport to stable30

@nickvergessen
Copy link
Member Author

/backport to stable29

@nickvergessen
Copy link
Member Author

/backport to stable28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants