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

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

Merged
merged 2 commits into from
Dec 20, 2024
Merged

Conversation

Altahrim
Copy link
Collaborator

@Altahrim Altahrim commented Oct 11, 2024

Summary

Checklist

@Altahrim Altahrim added this to the Nextcloud 31 milestone Oct 11, 2024
@Altahrim Altahrim self-assigned this Oct 11, 2024
@sorbaugh sorbaugh requested a review from icewind1991 October 11, 2024 12:35
@Altahrim Altahrim force-pushed the feat/dav-pagination branch from 4279ff1 to 6a915b2 Compare October 11, 2024 14:10
@icewind1991
Copy link
Member

pushed a minor change to better type return arrays

@Altahrim Altahrim force-pushed the feat/dav-pagination branch from 582f9ee to 645f5f7 Compare October 30, 2024 13:08
@icewind1991
Copy link
Member

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

@Altahrim Altahrim force-pushed the feat/dav-pagination branch 3 times, most recently from 08fffbc to d5bf59f Compare November 19, 2024 10:03
@Altahrim Altahrim force-pushed the feat/dav-pagination branch from 6ee049a to 0026817 Compare December 3, 2024 15:45
@Altahrim Altahrim added the pending documentation This pull request needs an associated documentation update label Dec 3, 2024
@Altahrim Altahrim force-pushed the feat/dav-pagination branch from 0026817 to 16cbf43 Compare December 4, 2024 08:45
@Altahrim Altahrim 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
@Altahrim Altahrim 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 team December 4, 2024 08:56
@Altahrim Altahrim force-pushed the feat/dav-pagination branch from 16cbf43 to 19704f6 Compare December 10, 2024 13:20
@Altahrim Altahrim force-pushed the feat/dav-pagination branch from 19704f6 to 82a3d7c Compare December 11, 2024 09:47
@Altahrim Altahrim added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 11, 2024
@Altahrim Altahrim force-pushed the feat/dav-pagination branch 2 times, most recently from 7f6548c to b0afbce Compare December 11, 2024 10:26
@Altahrim Altahrim requested a review from skjnldsv December 11, 2024 14:53
@Altahrim Altahrim added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 16, 2024
apps/dav/lib/Paginate/PaginateCache.php Outdated Show resolved Hide resolved
apps/dav/lib/Paginate/PaginateCache.php Outdated Show resolved Hide resolved
@@ -228,6 +229,7 @@ public function __construct(
$logger,
$eventDispatcher,
));
$this->server->addPlugin(\OCP\Server::get(PaginatePlugin::class));
Copy link
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
Collaborator Author

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 :/

Copy link
Contributor

@come-nc come-nc Dec 19, 2024

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.

apps/dav/lib/SystemTag/SystemTagList.php Show resolved Hide resolved
@Altahrim Altahrim force-pushed the feat/dav-pagination branch from b0afbce to 3ac62bc Compare December 19, 2024 09:31
Copy link
Contributor

@come-nc come-nc left a comment

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
Collaborator Author

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
Contributor

come-nc commented Dec 19, 2024

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
Collaborator Author

Only client will use it for now, and not everywhere

@Altahrim Altahrim force-pushed the feat/dav-pagination branch from 3ac62bc to a14a598 Compare December 20, 2024 08:16
@sorbaugh sorbaugh merged commit 2d76d13 into master Dec 20, 2024
186 of 188 checks passed
@sorbaugh sorbaugh deleted the feat/dav-pagination branch December 20, 2024 13:51
@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
@ChristophWurst
Copy link
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
Collaborator Author

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…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews feature: dav pending documentation This pull request needs an associated documentation update php Pull requests that update Php code
Projects
Status: 🏗️ In progress
Status: ☑️ Done
Development

Successfully merging this pull request may close these issues.

7 participants