Introduce pagination in files-filter report #26507

DeepDiver1975 · 2016-10-28T15:59:08Z

Description

Allow pagination when listing files

ToDos

integration tests
add cache for file list
integrate on client side

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

mention-bot · 2016-10-28T15:59:10Z

@DeepDiver1975, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81 and @nickvergessen to be potential reviewers.

PVince81 · 2016-11-09T17:13:19Z

for #13915

PVince81 · 2016-12-06T17:41:49Z

add cache for file list

memcache ? 😦

PVince81 · 2016-12-06T17:42:45Z

apps/dav/lib/Connector/Sabre/FilesReportPlugin.php

-					$requestedProps[] = $propVal['name'];
-				}
+		$requestedProps = $report->properties;
+		$filterRules = $report->filters;


really ? did the format change ?

PVince81 · 2016-12-06T17:43:17Z

apps/dav/lib/Files/Xml/FilterRequest.php

+use Sabre\Xml\Reader;
+use Sabre\Xml\XmlDeserializable;
+
+class FilterRequest implements XmlDeserializable {


oohhhh, clever

PVince81 · 2016-12-06T17:43:59Z

Code looks good so far

PVince81 · 2016-12-19T13:50:17Z

apps/dav/lib/Connector/Sabre/FilesReportPlugin.php

-			throw new PreconditionFailed('Cannot filter by non-existing tag', 0, $e);
+		if (!is_null($report->limit)) {
+			$length = $report->limit['size'];
+			$offset = $report->limit['page'] * $length;


The first page should be 1, not 0.

How about using limit and offset instead ? Might be easier to grasp...

PVince81 · 2016-12-19T13:51:40Z

Test report report.xml:

<?xml version="1.0" encoding="utf-8" ?>
<oc:filter-files xmlns:a="DAV:" xmlns:oc="http://owncloud.org/ns" >
        <a:prop>
                <a:getetag/>
        </a:prop>
        <oc:limit page="0" size="10" />
</oc:filter-files>

% curl -u admin:admin -X REPORT -H "Content-Type: text/xml" --data-binary "@report.xml" 'http://localhost/owncloud/remote.php/dav/files/admin/'

PVince81 · 2016-12-19T13:52:58Z

Some concerns: the regular filter report returns a flat list of files from all folders. This flat list can be paginated in this report, this is fine.

However we will also need a paginated call for folder-specific contents, without recursing. Should we make a separate report time for this maybe ? Or add a property <oc:recursive /> to make it a flat list, and by default only return for the current folder ?

PVince81 · 2016-12-19T18:00:27Z

@DeepDiver1975 I've rebased this.

Also I discovered today that Sabre has a shorter way of compiling results from multiple nodes using getPropertiesForMultiplePaths. Since we're touching this report anyway, I added it here f4aa5d2

There will still be a performance issue here because we need to resolve every node individually.
Ideally for pagination we might already know when we're fetching from a single folder, so this fetching should be optimizable. Might be doable using the IMultiget interface which getPropertiesForMultiplePaths uses internally: https://github.com/fruux/sabre-dav/blob/master/lib/DAV/IMultiGet.php#L34. When implementing this, we could sort the results by folder and optimize fetching props of specific grouped folders.

Side note: if we ever implement Webdav sync it also uses IMultiGet 😉

PVince81 · 2016-12-19T18:02:29Z

sort out "recursive" vs "flat files" question: Introduce pagination in files-filter report #26507 (comment)
properties resolving perf: Introduce pagination in files-filter report #26507 (comment)
fix page number to start with 1: Introduce pagination in files-filter report #26507 (comment)
use DAV:limit for the limit ? (see https://tools.ietf.org/search/rfc5323#section-5.17)

PVince81 · 2016-12-19T18:24:48Z

Arghh, no, not good. getPropertiesForMultiplePaths would redo a getChild() on the parent nodes because we haven't created any Sabre node yet for all the results.

I suspect that the old implementation also suffered from this. Since we have the files API node already, we should already be able to wrap it (it's a FileInfo after all) into a Sabre node and treat it directly as a result. No need for DB refetching...

I'll revert my commit because it will degrade the performance. From what I see we already had created the matching Sabre File and Directory in findNodesByFileIds.

More about this in a minute after the revert...

PVince81 · 2016-12-19T18:30:04Z

So, I've reverted the commit instead of deleting it, in case we still want it. (Squashing will kill it later).

Now looking at findNodesByFileIds(): this has BAD performance because it will resolve every fileid using Folder::getById(). Now for arbitrary results like "/test/x1", "/test/sub/x2", "/abc/def" it makes sense as we're not in the same folder. This was valid for the "filter by tags" and "filter by favorite" feature in which results are usually not inside the same folder.

However if we want REPORT to be used for pagination on simple folders, then we need a different approach there. Usually calling getChildren() on the parent is more efficient as it does a single DB query for getDirectoryContents().

So to solve the above question we really need to discuss "recursive / flattened / arbitrary results" vs "only show results from the current folder".

One idea would be to provide one generic REPORT type that only does a PROPFIND on the files endpoints (or even any part of the tree!) and paginates whatever the PROPFIND / getChildren() returns. All results are children of the current node.

Then we provide another REPORT, which is the one from this PR, to really do search-like queries. In such queries the results are not limited to the current folder, so we have to use getById() for every result.

Or a hybrid variant is to have a single REPORT type but with a oc:recursive property. When unset, we'd resort to a simple getChildren() + post-filter on the results. When set, we'd do the other approach that gets the file ids of the results first, then resolve then. Note that both implementations are completely different.

@DeepDiver1975 thoughts ?

PVince81 · 2016-12-19T18:38:02Z

I wrote down some ideas about implementing a generic pagination report that would work for every PROPFIND-enabled endpoint: #26840

PVince81 · 2016-12-19T18:49:59Z

Regarding recursive vs children, I think we could even call them differently:

recursive: is more a SEARCH operation because you're searching for all nodes that match a specific criteria in all subcollections. So the approach is more like asking an index (if we had one) to give results in many folders and then compile these results in one result set.
children: is more of a filter where you basically just get the contents of the current collection (children) and then apply a filter on their properties. All data is already there within the current collection, so no need for an index, just need to remove the nodes from the result that do not match the filter criteria.

PVince81 · 2016-12-19T19:17:41Z

So for "recursive" vs "flat" the answer from @DeepDiver1975 was using the "Depth" header.
Which means that to search on a single collection one specifies "Depth: 1" and in all subcollections "Depth: infinite".

PVince81 · 2017-02-28T18:54:49Z

Rebased.

Also I found a way to simplify the response code to use a more Sabr-y way to do it: c355695

All discovered while working on the customgroups REPORT: owncloud/customgroups#27.

Note: in the customgroups pagination I used a different format for the search pattern + pagination:

<?xml version="1.0" encoding="utf-8" ?>
<oc:search-query xmlns:a="DAV:" xmlns:oc="http://owncloud.org/ns">
        <a:prop>
                <oc:role></oc:role>
                <oc:display-name></oc:display-name>
        </a:prop>
        <oc:search>
                <oc:pattern>us</oc:pattern>
                <oc:limit>5</oc:limit>
                <oc:offset>0</oc:offset>
        </oc:search>
</oc:search-query>

PVince81 · 2017-03-15T11:20:41Z

How about we finish this without the cache ? It will be crude but at least it will make it possible for clients to at least request the first page with limit.

This will also properly return 404 properties

PVince81 · 2017-03-17T18:43:14Z

Rebased and pushed some changes:

pagination (offset+limit) now works also for tags/favorite
disabled case where no filter is specified because getChildren() was not enough and would only search one level deep. Actually we need to use Node::search() but when I tried I noticed that it was missing attributes in its FileInfo properties (like owner) which makes it currently unusable. Something to dig into in the future (<= @DeepDiver1975)
added unit tests

Next up: integration tests for pagination when filtering.

PVince81 · 2017-03-20T11:12:21Z

21:23:41     /var/lib/jenkins/workspace/owncloud-core_core_PR-26507-XBSZ3PUQKGNP3NP62UQHR4G7JKGONNKXFRY3NXEAITQYFZVK5JDQ/tests/integration/features/favorites.feature:81
21:23:41     /var/lib/jenkins/workspace/owncloud-core_core_PR-26507-XBSZ3PUQKGNP3NP62UQHR4G7JKGONNKXFRY3NXEAITQYFZVK5JDQ/tests/integration/features/favorites.feature:93

PVince81 · 2017-03-20T11:32:32Z

Fixed integration tests and also added a new integration test for pagination.

@DeepDiver1975 @jvillafanez please review

jvillafanez · 2017-03-21T11:32:24Z

apps/dav/lib/Connector/Sabre/FilesReportPlugin.php

+		$requestedProps = $report->properties;
+		$filterRules = $report->filters;
+
+		if (empty($filterRules['systemtag']) && is_null($filterRules['favorite'])) {


Maybe we should refine the condition... empty is always scary.

Not sure... "systemtag" here is an array so empty fits but "favorite" is only a single string.

if "systemtag" will be always an array no matter what, then it's fine. Something similar for "favorite" (wondering if '' or '0' would behave as we expect).
Anyway a comment would be useful to clarify any doubt, mainly to say that this is on purpose.

jvillafanez · 2017-03-21T11:40:28Z

apps/dav/lib/Connector/Sabre/FilesReportPlugin.php

-		}
+			// TODO: if depth one, could use a different search algo based on getChildren() ...
+			// TODO: Depth header filtering?
+			$results = $folder->search($report->search['pattern']);


remove dead code

boohoo, ok I'll move it to another WIP PR

jvillafanez · 2017-03-21T12:14:27Z

apps/dav/lib/Files/Xml/FilterRequest.php

+			}
+		}
+
+		$obj = new self();


Any reason to use self instead of explicitly calling the current class?

Written by @DeepDiver1975 and likely copied from other Sabre deserializer classes where self was used. More convenient than rewriting the current class name I guess.

If it was copied from Sabre, i guess it's fine. Don't like it, but ok.

jvillafanez · 2017-03-21T12:19:26Z

tests/integration/features/bootstrap/WebDav.php

-									' . $filterRules . '
-								 </oc:filter-rules>
-							 </oc:filter-files>';
+					<oc:filter-files xmlns:a="DAV:" xmlns:oc="http://owncloud.org/ns" >


We have to find a better way to generate this.

Agreed. I'll let @owncloud/qa go wild to improve this separately.

jvillafanez · 2017-03-21T12:24:13Z

apps/dav/lib/Connector/Sabre/FilesReportPlugin.php

+			return new File($this->fileView, $filesNode);
+		} else if ($filesNode instanceof \OCP\Files\Folder) {
+			return new Directory($this->fileView, $filesNode);
+		}


We should log something here unless this null is properly detected and reported. I wouldn't like to miss this case if something goes horribly wrong.

I'll throw a "WTF this can't be" style exception

Any chance that this exception is catched in the middle and doesn't show up? just making sure this will be visible.

It's wrapped in a SabreException later upstream:

<?xml version="1.0" encoding="utf-8"?> <d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns"> <s:exception>Exception</s:exception> <s:message>Unrecognized Files API node returned, aborting</s:message> </d:error>

jvillafanez · 2017-03-21T12:27:13Z

I haven't checked deeply the FilesReportPlugin.php file, in case someone wants to recheck that file

PVince81 · 2017-03-21T14:25:40Z

I haven't checked deeply the FilesReportPlugin.php file, in case someone wants to recheck that file

There aren't many changes there so I think it's ok.

Basically the response stays the same except that the array we pass there is sliced.
Most of the added logic is to slice at the right moment.

@jvillafanez adjusted, see last two commits

PVince81 · 2017-03-22T09:58:24Z

Final review ? @jvillafanez @DeepDiver1975
Thanks.

jvillafanez · 2017-03-22T12:53:53Z

#26507 (comment) and #26507 (comment) . Other than that 👍

- disable REPORT without filter Because we need to use Node::search($pattern) to find all matching nodes in all the subfolder recursively, but the result nodes contain incomplete information like owner. - remove unneeded trailing slash when buildling response href This got obsoleted when switching to generateMultiStatus() - add integration pagination test for favorites REPORT - throw exception for unknown node types in FilesReportPlugin

PVince81 · 2017-03-22T17:28:54Z

Added comment, squashed my commits into a smaller set.

Since there are no actual code changes -> instamerge

PVince81 · 2017-03-22T17:35:49Z

Raised doc ticket with some instructions: https://github.com/owncloud/documentation/issues/2950

lock · 2019-08-03T17:02:06Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

DeepDiver1975 added the 2 - Developing label Oct 28, 2016

DeepDiver1975 added this to the 9.2 milestone Oct 28, 2016

DeepDiver1975 force-pushed the filelist-pagination-webdav-report branch from d67ffa0 to 980d966 Compare November 4, 2016 16:02

PVince81 mentioned this pull request Nov 9, 2016

Paginated requests for the file list #13915

Closed

DeepDiver1975 mentioned this pull request Nov 11, 2016

Making Favorites Consistent Across Platforms #24671

Closed

DeepDiver1975 force-pushed the filelist-pagination-webdav-report branch from 980d966 to a108477 Compare November 11, 2016 12:45

butonic assigned DeepDiver1975 Nov 30, 2016

PVince81 reviewed Dec 6, 2016

View reviewed changes

PVince81 reviewed Dec 19, 2016

View reviewed changes

PVince81 force-pushed the filelist-pagination-webdav-report branch from a108477 to f4aa5d2 Compare December 19, 2016 17:57

PVince81 force-pushed the filelist-pagination-webdav-report branch from 1d87fb6 to c355695 Compare February 28, 2017 18:52

DeepDiver1975 and others added 2 commits March 17, 2017 16:50

Introduce pagination in files-filter report

b315bff

Use a more API-style way to generate REPORT PROPFIND

5505a2d

This will also properly return 404 properties

PVince81 force-pushed the filelist-pagination-webdav-report branch from c355695 to a7ed90f Compare March 17, 2017 18:42

PVince81 changed the title ~~[WIP] Introduce pagination in files-filter report~~ Introduce pagination in files-filter report Mar 20, 2017

PVince81 added 3 - To Review and removed 2 - Developing labels Mar 20, 2017

PVince81 assigned PVince81 and unassigned DeepDiver1975 Mar 20, 2017

jvillafanez reviewed Mar 21, 2017

View reviewed changes

PVince81 force-pushed the filelist-pagination-webdav-report branch from 9a23cd4 to fad6a05 Compare March 22, 2017 17:28

PVince81 merged commit b31294a into master Mar 22, 2017

PVince81 deleted the filelist-pagination-webdav-report branch March 22, 2017 17:29

michaelstingl mentioned this pull request Sep 5, 2017

Add a search field above the TableView - text search owncloud/ios-legacy#17

Closed

5 tasks

lock bot locked as resolved and limited conversation to collaborators Aug 3, 2019

Introduce pagination in files-filter report #26507

Introduce pagination in files-filter report #26507

Conversation

DeepDiver1975 commented Oct 28, 2016 • edited by PVince81 Loading

Description

ToDos

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

mention-bot commented Oct 28, 2016

PVince81 commented Nov 9, 2016

PVince81 commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Dec 6, 2016

Choose a reason for hiding this comment

PVince81 commented Dec 19, 2016

PVince81 commented Dec 19, 2016

PVince81 commented Dec 19, 2016

PVince81 commented Dec 19, 2016

PVince81 commented Dec 19, 2016

PVince81 commented Dec 19, 2016 • edited Loading

PVince81 commented Dec 19, 2016

PVince81 commented Dec 19, 2016

PVince81 commented Dec 19, 2016

PVince81 commented Feb 28, 2017

PVince81 commented Mar 15, 2017

PVince81 commented Mar 17, 2017

PVince81 commented Mar 20, 2017

PVince81 commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvillafanez Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvillafanez commented Mar 21, 2017

PVince81 commented Mar 21, 2017

PVince81 commented Mar 22, 2017

jvillafanez commented Mar 22, 2017

PVince81 commented Mar 22, 2017

PVince81 commented Mar 22, 2017

lock bot commented Aug 3, 2019

DeepDiver1975 commented Oct 28, 2016 •

edited by PVince81

Loading

PVince81 commented Dec 19, 2016 •

edited

Loading

jvillafanez Mar 22, 2017 •

edited

Loading