Skip to content

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

Merged
MorrisJobke merged 1 commit into
masterfrom
bugfix/9339/initial_collection_sync_caldav_carddav
Jul 18, 2019
Merged

LIMIT is no column but a SQL keyword, allow limit on initial sync#10842
MorrisJobke merged 1 commit into
masterfrom
bugfix/9339/initial_collection_sync_caldav_carddav

Conversation

@georgehrke

Copy link
Copy Markdown
Member

fixes#9339

please test @rfc2822

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

@nickvergessennickvergessen left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
MemberAuthor

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

@rfc2822

Copy link
Copy Markdown
Contributor

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)?

@MorrisJobkeMorrisJobke mentioned this pull request Aug 26, 2018
6 tasks
@nickvergessennickvergessen mentioned this pull request Sep 19, 2018
@blizzz

Copy link
Copy Markdown
Member

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

@rullzer

Copy link
Copy Markdown
Member

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.

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

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code looks good 👍

Comment threadapps/dav/lib/CardDAV/CardDavBackend.php Outdated
@rullzer

Copy link
Copy Markdown
Member

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

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

Copy link
Copy Markdown
Member

@georgehrke ^ ?

@MorrisJobkeMorrisJobke mentioned this pull request Nov 8, 2018
24 tasks
@MorrisJobke

Copy link
Copy Markdown
Member

Moved to 15.0.1

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

Copy link
Copy Markdown
Member

Needs some work, on it ...

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

@georgehrke

Copy link
Copy Markdown
MemberAuthor

@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
Copy Markdown
Member

Let me put it onto my ToDo list for tomorrow.

Tomorrow was yesterday, right? 😉

@georgehrke
georgehrkeforce-pushed the bugfix/9339/initial_collection_sync_caldav_carddav branch 4 times, most recently from ae1bfb9 to 5966286CompareJuly 18, 2019 10:03
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@georgehrke
georgehrkeforce-pushed the bugfix/9339/initial_collection_sync_caldav_carddav branch from 5966286 to 9f6dd51CompareJuly 18, 2019 10:42
@georgehrke

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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

@georgehrkegeorgehrke removed the 2. developing Work in progress label Jul 18, 2019

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code makes sense 👍

@MorrisJobkeMorrisJobke 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 masterJul 18, 2019
@MorrisJobke
MorrisJobke deleted the bugfix/9339/initial_collection_sync_caldav_carddav branch July 18, 2019 17:01
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finishbugfeature: 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

@georgehrke@rfc2822@blizzz@rullzer@MorrisJobke@nickvergessen