Skip to content

feat(dav): introduce paginate with custom headers - #48662

Merged
sorbaugh merged 2 commits into
masterfrom
feat/dav-pagination
Dec 20, 2024
Merged

feat(dav): introduce paginate with custom headers#48662
sorbaugh merged 2 commits into
masterfrom
feat/dav-pagination

Conversation

@Altahrim

@AltahrimAltahrim commented Oct 11, 2024

Copy link
Copy Markdown
Collaborator

Summary

Checklist

@AltahrimAltahrim added this to the Nextcloud 31 milestone Oct 11, 2024
@AltahrimAltahrim self-assigned this Oct 11, 2024
@icewind1991

Copy link
Copy Markdown
Member

pushed a minor change to better type return arrays

@icewind1991

Copy link
Copy Markdown
Member

Changed the queries to use DateTimes instead of timestamps for the insert_time to make them work with postgresql

@Altahrim
Altahrimforce-pushed the feat/dav-pagination branch 3 times, most recently from 08fffbc to d5bf59fCompareNovember 19, 2024 10:03
@AltahrimAltahrim added the pending documentation This pull request needs an associated documentation update label Dec 3, 2024
@AltahrimAltahrim added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 4, 2024
@Altahrim
Altahrim marked this pull request as ready for review December 4, 2024 08:56
@AltahrimAltahrim added the php Pull requests that update Php code label Dec 4, 2024
@Altahrim
Altahrim requested review from a team, nfebe and sorbaugh and removed request for a teamDecember 4, 2024 08:56
Comment threadapps/dav/lib/Migration/Version1032Date20241011093632.php Outdated
@AltahrimAltahrim added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 11, 2024
@Altahrim
Altahrimforce-pushed the feat/dav-pagination branch 2 times, most recently from 7f6548c to b0afbceCompareDecember 11, 2024 10:26
@AltahrimAltahrim added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 16, 2024
Comment threadapps/dav/lib/Paginate/PaginateCache.php Outdated
Comment threadapps/dav/lib/Paginate/PaginateCache.php Outdated
$logger,
$eventDispatcher,
));
$this->server->addPlugin(\OCP\Server::get(PaginatePlugin::class));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this also be added to ServerFactory?
(in apps/dav/lib/Connector/Sabre)

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

I don't know if there is a clear rule for that :/

@come-nccome-ncDec 19, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do not remember the details, I think @skjnldsv knows.
From what I recall one is used for public pages and the other one for authenticated webdav or something like that? Which is why they do not have the exact same list of plugins loaded.
But quite frankly it should all be moved to the server factory, even if it stays 2 separate methods if needed.

Comment threadapps/dav/lib/SystemTag/SystemTagList.php

@come-nccome-nc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you detail the performance and resources implication of this?
Any (paginated) propfind will add all its results to the cache (redis?), how big of an impact is that? That can be a huge list depending on the number of files in the folder, right?

@Altahrim

Copy link
Copy Markdown
CollaboratorAuthor

Difficult to give a precise impact. It will depends of number of items in a directory, number of paginated requests… We implement this as a test to have first answers about it.
It's stored in cache to have faster results and also limit the impact on Nextcloud (compared to database)

@come-nc

Copy link
Copy Markdown
Contributor

Difficult to give a precise impact. It will depends of number of items in a directory, number of paginated requests… We implement this as a test to have first answers about it. It's stored in cache to have faster results and also limit the impact on Nextcloud (compared to database)

But there is no trigger to enable/disable it?

@Altahrim

Copy link
Copy Markdown
CollaboratorAuthor

Only client will use it for now, and not everywhere

Signed-off-by: Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
Signed-off-by: Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
@sorbaugh
sorbaugh merged commit 2d76d13 into masterDec 20, 2024
@sorbaugh
sorbaugh deleted the feat/dav-pagination branch December 20, 2024 13:51
@skjnldsvskjnldsv mentioned this pull request Jan 7, 2025
@ChristophWurst

Copy link
Copy Markdown
Member

@Altahrim since offsets are typically not scalable, at least for database operations, I'm wondering how the file list pagination behaves as the page number increases? Do you still have to build and discard entries lower than the offset or does the caching avoid rebuilding of the list?
For example, you have a directory with 1M files, and request page 101 with pages of 100 entries. Are the previous 10k entries still fetched?
I couldn't figure it out from just reading the code.

https://use-the-index-luke.com/sql/partial-results/fetch-next-page

@Altahrim

Copy link
Copy Markdown
CollaboratorAuthor

Once the cache is built, we'll fetch only the corresponding results from cache: https://github.com/nextcloud/server/pull/48662/files#diff-7dd44509fa2becb8b56ada8a254c14836090dd56baba7f4a10abb040cfa85351R62-R65

It's trickier when we build the cache since we always fetch all matching entries to cache them…

@susnuxsusnux moved this from 🏗️ In progress to ☑️ Done in 📁 Files teamMar 14, 2025
@AltahrimAltahrim removed the pending documentation This pull request needs an associated documentation update label Mar 31, 2025
@Altahrim

Copy link
Copy Markdown
CollaboratorAuthor

Documentation: nextcloud/documentation#12977

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviewsfeature: davphpPull requests that update Php code

Projects

Status: ☑️ Done
Archived in project

Development

Successfully merging this pull request may close these issues.

8 participants

@Altahrim@icewind1991@come-nc@ChristophWurst@skjnldsv@sorbaugh@susnux@marinofaggiana