New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfix/36644 pruneOutdatedSyncTokens deletes all entries #38639
Bugfix/36644 pruneOutdatedSyncTokens deletes all entries #38639
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I have posted a couple of comments on the CalDavBackend.php
file. The same applies to CardDavBackend.php
.
EDIT : Make sure to lint your code as well to fix other CI failures.
@tcitworld Thanks for the feedback, I'll have a look in the evening. Do you know by chance how I can run the linters using vs code and the dev container? Also I just noticed that you tagged this for Version 28. Do you plan to backport this to 26 / 27? |
/backport to stable27 |
/backport to stable26 |
Unfortunately I don't know. However, it seems the only issues you have should be resolved alongside with the issues I pointed out above.
Yup, that's planned. |
Wow, good catch! |
Signed-off-by: Christof Arnosti <charno@charno.ch>
More detailed testing (request from Issue) and cleanups are done. Now for hoping linter / psalm is happy |
@kesselb Can you also add / push the changes to CardDavBackend? I'm away from my dev workstation for the next days |
I'll do some cleanup on weekend (Port changes done to Cal to Card, squishing formatting commits), then from my side I think it is finished |
I have done the formatting changes, now hopefully everything should be fine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Squash commits and fix sign-off and that's perfect.
Thanks a lot for that!
pruneOutdatedSyncTokens accidentally deletes all entries of the calendarchanges table instead of leaving $limit elements in the table Signed-off-by: Christof Arnosti <charno@charno.ch>
Thanks for reviewing! I'm happy when this bug is gone, this one was especially painful for me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works. Nice catch!
/backport to stable25 |
CI failure unrelated |
Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 |
/backport to stable26 |
/backport to stable27 |
Thanks for merging!
This will probably not be possible since the functionality this fixes is not present in 25 |
The backport to stable27 failed. Please do this backport manually. # Switch to the target branch and update it
git checkout stable27
git pull origin stable27
# Create the new backport branch
git checkout -b fix/foo-stable27
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123
# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27 More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport |
1 similar comment
The backport to stable27 failed. Please do this backport manually. # Switch to the target branch and update it
git checkout stable27
git pull origin stable27
# Create the new backport branch
git checkout -b fix/foo-stable27
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123
# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27 More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport |
The backport to stable26 failed. Please do this backport manually. # Switch to the target branch and update it
git checkout stable26
git pull origin stable26
# Create the new backport branch
git checkout -b fix/foo-stable26
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123
# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable26 More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport |
1 similar comment
The backport to stable26 failed. Please do this backport manually. # Switch to the target branch and update it
git checkout stable26
git pull origin stable26
# Create the new backport branch
git checkout -b fix/foo-stable26
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123
# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable26 More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport |
The backport to stable25 failed. Please do this backport manually. # Switch to the target branch and update it
git checkout stable25
git pull origin stable25
# Create the new backport branch
git checkout -b fix/foo-stable25
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123
# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable25 More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport |
Summary
With Nextcloud 26, CalDAV / CardDAV Clients which are using SyncToken (for example Thunderbird) don't receive calendar / contact updates which happened before the last run of
pruneOutdatedSyncTokens
. During collaborative research we found out thatpruneOutdatedSyncTokens
does delete all entries of calendarchanges / addressbookchanges by accident.This pull request changes the
pruneOutdatedSyncTokens
functions to first select the highest ID in the*changes
table and use it in a WHERE clause during deletion of the tokens.Sine calendar on Thunderbird / maybe other clients are regularly missing events I would say this is a quite high impact bug. For me and others, calendar syncing is currently practically unusable.
Checklist