Skip to content

Conversation

@ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Apr 26, 2023

Summary

The transaction guarantees there are no two concurrent inserts for the same interaction.

I have not seen a conflict in production. I just noticed that this code is prone to fail in rare cases.

This is best reviewed as https://github.com/nextcloud/server/pull/37933/files?w=1

TODO

  • Span DB transaction for the read-write operation

Checklist

The transaction guarantees there are no two concurrent inserts for the
same interaction.

Signed-off-by: Christoph Wurst <[email protected]>
if ($event->getFederatedCloudId() !== null) {
$contact->setFederatedCloudId($event->getFederatedCloudId());
}
$contact->setLastContact($this->timeFactory->getTime());

Check notice

Code scanning / Psalm

DeprecatedMethod

The method OCP\AppFramework\Utility\ITimeFactory::getTime has been marked as deprecated
$event->getFederatedCloudId()
);
if (!empty($existing)) {
$now = $this->timeFactory->getTime();

Check notice

Code scanning / Psalm

DeprecatedMethod

The method OCP\AppFramework\Utility\ITimeFactory::getTime has been marked as deprecated
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

🧹

@st3iny st3iny added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 2, 2023
@st3iny st3iny added this to the Nextcloud 27 milestone May 2, 2023
This was referenced May 3, 2023
@ChristophWurst ChristophWurst merged commit 7e3d5d6 into master May 12, 2023
@ChristophWurst ChristophWurst deleted the fix/contactsinteraction/transactional-read-update-insert branch May 12, 2023 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish bug technical debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants