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

LIMIT is no column but a SQL keyword, allow limit on initial sync #10842

Merged

Conversation

georgehrke
Copy link
Member

fixes #9339

please test @rfc2822

@georgehrke georgehrke added the 2. developing Work in progress label Aug 24, 2018
@georgehrke georgehrke added this to the Nextcloud 14.0.1 milestone Aug 24, 2018
@georgehrke georgehrke changed the title LIMIT is no column but a SQL feature, allow limit on initial sync LIMIT is no column but a SQL keyword, allow limit on initial sync Aug 24, 2018
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Should really move to the query builder at some time

@georgehrke
Copy link
Member Author

I will take out the additional limit code and put it into a dedicated pull-request because it needs more testing ...

@rfc2822
Copy link
Contributor

rfc2822 commented Aug 25, 2018

The first part (removing the `s) is working here fine :)


I'd not use the other part (allow limit on initial sync) this way because it returns the wrong sync-token ($currentToken instead of the sync-token that matches the sync state after applying the limit nresults) and thus breaks synchronization (which doesn't happen when the limit is silently ignored).

Support for client-requested limiting (truncating) is not required by the RFC, so maybe just throwing 507 on initial syncs with limit nresults would be a good idea (like https://tools.ietf.org/html/rfc6578#section-3.12)?

@MorrisJobke MorrisJobke mentioned this pull request Aug 26, 2018
6 tasks
@blizzz blizzz modified the milestones: Nextcloud 14.0.1, Nextcloud 15 Sep 19, 2018
@blizzz
Copy link
Member

blizzz commented Sep 19, 2018

set the milestone to 15 as this PR is against master.

@rullzer
Copy link
Member

rullzer commented Oct 4, 2018

Moved it to the query builder (just so we keep cleaning up bit by bit).
@georgehrke please have a look as well.

If all is green we just need to rebase.

@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 4, 2018
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code looks good 👍

@rullzer
Copy link
Member

rullzer commented Oct 4, 2018

CI is not happy. Seems that @georgehrke added the order by which is not present.

@rullzer rullzer added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 4, 2018
@MorrisJobke MorrisJobke mentioned this pull request Nov 7, 2018
29 tasks
@blizzz
Copy link
Member

blizzz commented Nov 7, 2018

@georgehrke ^ ?

@MorrisJobke MorrisJobke mentioned this pull request Nov 8, 2018
24 tasks
@MorrisJobke
Copy link
Member

Moved to 15.0.1

@MorrisJobke MorrisJobke added this to the Nextcloud 15.0.5 milestone Feb 7, 2019
@rullzer rullzer mentioned this pull request Mar 29, 2019
1 task
@MorrisJobke MorrisJobke mentioned this pull request Jul 15, 2019
28 tasks
@MorrisJobke
Copy link
Member

Needs some work, on it ...

@georgehrke What is the status? We are close to the beta 1. 17 or 18?

@georgehrke
Copy link
Member Author

@georgehrke What is the status? We are close to the beta 1. 17 or 18?

Let me put it onto my ToDo list for tomorrow.

@MorrisJobke
Copy link
Member

Let me put it onto my ToDo list for tomorrow.

Tomorrow was yesterday, right? 😉

@georgehrke georgehrke force-pushed the bugfix/9339/initial_collection_sync_caldav_carddav branch 4 times, most recently from ae1bfb9 to 5966286 Compare July 18, 2019 10:03
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@georgehrke georgehrke force-pushed the bugfix/9339/initial_collection_sync_caldav_carddav branch from 5966286 to 9f6dd51 Compare July 18, 2019 10:42
@georgehrke
Copy link
Member Author

Test fresh sync with limit:

curl -X REPORT -u admin:batQz-Nc6zK-din8a-pdXoC-QPeWH -d '<?xml version="1.0" encoding="utf-8" ?>
   <D:sync-collection xmlns:D="DAV:">
     <D:sync-token/>
     <D:sync-level>1</D:sync-level>
     <D:limit>
       <D:nresults>1</D:nresults>
     </D:limit>
     <D:prop>
       <D:getetag/>
     </D:prop>
   </D:sync-collection>' 'http://nextcloud.local/remote.php/dav/calendars/admin/personal/'

=> Throws an exception

Test fresh sync without limit:

curl -X REPORT -u admin:batQz-Nc6zK-din8a-pdXoC-QPeWH -d '<?xml version="1.0" encoding="utf-8" ?>
   <D:sync-collection xmlns:D="DAV:">
     <D:sync-token/>
     <D:sync-level>1</D:sync-level>
     <D:prop>
       <D:getetag/>
     </D:prop>
   </D:sync-collection>' 'http://nextcloud.local/remote.php/dav/calendars/admin/personal/'

=> Returns a list of all events

Test with sync-token and limit:

curl -X REPORT -u admin:batQz-Nc6zK-din8a-pdXoC-QPeWH -d '<?xml version="1.0" encoding="utf-8" ?>
   <D:sync-collection xmlns:D="DAV:">
     <D:sync-token>http://sabre.io/ns/sync/55</D:sync-token>
     <D:sync-level>1</D:sync-level>
     <D:limit>
       <D:nresults>1</D:nresults>
     </D:limit>
     <D:prop>
       <D:getetag/>
     </D:prop>
   </D:sync-collection>' 'http://nextcloud.local/remote.php/dav/calendars/admin/personal/'

=> Behaviour didn't change compared to master

Test with sync-token and without limit:

curl -X REPORT -u admin:batQz-Nc6zK-din8a-pdXoC-QPeWH -d '<?xml version="1.0" encoding="utf-8" ?>
   <D:sync-collection xmlns:D="DAV:">
     <D:sync-token>http://sabre.io/ns/sync/55</D:sync-token>
     <D:sync-level>1</D:sync-level>
     <D:limit>
       <D:nresults>1</D:nresults>
     </D:limit>
     <D:prop>
       <D:getetag/>
     </D:prop>
   </D:sync-collection>' 'http://nextcloud.local/remote.php/dav/calendars/admin/personal/'

=> Behaviour didn't change compared to master

@georgehrke
Copy link
Member Author

I changed this PR as per @rfc2822's suggestion.
When a client sends an initial sync-request with a limit, we respond with a {DAV:}:number-of-matches-within-limits error

@georgehrke georgehrke removed the 2. developing Work in progress label Jul 18, 2019
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code makes sense 👍

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 18, 2019
@MorrisJobke MorrisJobke merged commit b291d9b into master Jul 18, 2019
@MorrisJobke MorrisJobke deleted the bugfix/9339/initial_collection_sync_caldav_carddav branch July 18, 2019 17:01
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 feature: dav
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal server error on REPORT DAV:sync-collection with result limit
6 participants