Skip to content
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

CalDAV to sync properly when limit is set in PDO backend #1248

Merged
merged 1 commit into from
Dec 2, 2021

Conversation

nhirokinet
Copy link
Contributor

@nhirokinet nhirokinet commented Feb 16, 2020

Hi,

I altered some codes to make CalDAV sync properly even when limit is set. Changes is mainly as below two:

  • Return syncToken when limit is set. Current code returns the syncToken of the latest server state. Changed to return the syncToken corresponding to the truncated sync result.
    • When the result limit is designated by client, this is not the consistent as returned result.
    • In RFC6578 section 3.6: the DAV:sync-token value returned in the response MUST represent the correct state for the partial set of changes returned.
    • So, when truncation occurs, client will miss the truncated event even in later sync, since the client receives the syncToken of not received events, and asks server to send the event after that next time.
  • When response is truncated, 507 record is added to the response.
    • Defined in RFC6578 section 3.6
    • With this, the client can recognize the result is truncated, and can retry until the client syncs with the latest state.
    • For example, OneCalendar requested once I press sync button in current code. In new code OneCalendar repeated until it reaches the latest state, so I just had to press sync button once.

OneCalendar in Windows Store sends a request with limit 1000, and I checked using this, but with the changes below:

  • I defanged the code of Authentication because OneCalendar did not send a request with Digest authentication.
  • I overrided the limit to 2 in lib/CalDAV/Backend/PDO.php while testing.

I think CardDAV has the same issue, but I did not find an environment to reproduce and test with, so I wrote a code only for CalDAV, PDO backend.

This is my first code around WebDAV, so could you take a look including my understanding is correct or not?

@codecov
Copy link

codecov bot commented Feb 16, 2020

Codecov Report

Merging #1248 (69f114f) into master (d1a3e9a) will increase coverage by 0.01%.
The diff coverage is 96.55%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1248      +/-   ##
============================================
+ Coverage     97.35%   97.37%   +0.01%     
- Complexity     2810     2817       +7     
============================================
  Files           174      174              
  Lines          7979     7996      +17     
============================================
+ Hits           7768     7786      +18     
+ Misses          211      210       -1     
Impacted Files Coverage Δ
lib/CalDAV/Calendar.php 96.03% <ø> (ø)
lib/CalDAV/Backend/PDO.php 99.38% <95.45%> (+0.22%) ⬆️
lib/DAV/Sync/Plugin.php 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1a3e9a...69f114f. Read the comment docs.

@evert
Copy link
Member

evert commented Feb 16, 2020

It's a bit hard for me to eye this and see if it's correct, but it looks good. I feel that this can use a good amount of unittests to ensure that edge cases are sufficiently covered and increase confidence.

Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

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

Please add unit tests! THX

@nhirokinet nhirokinet force-pushed the feature/caldav-sync-limit branch 4 times, most recently from bad5b51 to cc150c0 Compare February 17, 2020 15:58
@nhirokinet
Copy link
Contributor Author

I added test case for each of two changes.

@nhirokinet
Copy link
Contributor Author

@DeepDiver1975 I added some tests after your comments, should I add some more change?

@nhirokinet
Copy link
Contributor Author

Hi, is there something I can do to advance this pull request?
CI fails, but it seems to be some new timezone test, which this pull request does not touch.

@nhirokinet
Copy link
Contributor Author

@DeepDiver1975 Hi, do you have some comments about added tests?

@nhirokinet
Copy link
Contributor Author

@phil-davis @DeepDiver1975 Hi, do you know the status about this? I added some tests according to your comment, long ago, that I'm not sure about my memory, anything more I should do?

tests/Sabre/DAV/Sync/PluginTest.php Outdated Show resolved Hide resolved
@phil-davis
Copy link
Contributor

Rebased and squashed. LGTM, thanks. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants