Allow filtering the search results to the users home storage #17941

icewind1991 · 2019-11-14T15:21:12Z

This is done by adding a

<d:eq>
    <d:prop>
        <oc:owner-id/>
    </d:prop>
    <d:literal>$userId</d:literal>
</d:eq>

clause to the search query.

Searching by owner-id can only be done with the current user id
and the comparison can not be inside a <d:not> or <d:or> statement

Signed-off-by: Robin Appelman [email protected]

icewind1991 · 2019-11-14T15:22:40Z

Full search example

<?xml version="1.0" encoding="UTF-8"?>
<d:searchrequest xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns">
    <d:basicsearch>
        <d:select>
            <d:prop>
                <d:getcontenttype/>
            </d:prop>
        </d:select>
        <d:from>
            <d:scope>
                <d:href>/files/admin</d:href>
                <d:depth>infinity</d:depth>
            </d:scope>
        </d:from>
        <d:where>
            <d:and>
                <d:like>
                    <d:prop>
                        <d:getcontenttype/>
                    </d:prop>
                    <d:literal>image/%</d:literal>
                </d:like>
                <d:eq>
                    <d:prop>
                        <oc:owner-id/>
                    </d:prop>
                    <d:literal>admin</d:literal>
                </d:eq>
            </d:and>
        </d:where>
        <d:orderby/>
    </d:basicsearch>
</d:searchrequest>

lib/private/Files/Node/Folder.php

lib/private/Files/Cache/QuerySearchHelper.php

lib/private/Files/Node/Folder.php

kesselb · 2019-11-16T22:51:03Z

apps/dav/lib/Files/FileSearchBackend.php

+								$value = $operator->arguments[1]->value;
+
+								// to remove the comparison from the query, we replace it with an empty AND
+								$operator = new Operator(Operator::OPERATION_AND);


I'm not a fan. Could you consider to keep it? Sure we need owner as property at mapPropertyNameToColumn and still need to move $comparison->getField() === 'owner' before validateComparison. I think this is less hacky ;)

My original plan was something like that but you'll still need to remove the comparison from the query since there is no column related to the owner field, this way the logic is confirmed to a single place

Fine by me 👍 I see your point keeping the logic together. searchComparisonToDBExpr is more or less a generic method and adding "do nothing if owner field" there is bad. The fake operator is also bad. Guess it does not matter which bad we pick ;)

rullzer

Seems towork. But please have a look at the comments

skjnldsv · 2019-11-26T14:44:22Z

@icewind1991 rebased.
Please have a look at #17941 (comment) and let get this in :)

This is done by adding a ```xml <d:eq> <d:prop> <oc:owner-id/> </d:prop> <d:literal>$userId</d:literal> </d:eq> ``` clause to the search query. Searching by `owner-id` can only be done with the current user id and the comparison can not be inside a `<d:not>` or `<d:or>` statement Signed-off-by: Robin Appelman <[email protected]>

skjnldsv · 2019-12-03T12:50:04Z

Feature freeze is here, we need to get this in!

skjnldsv · 2019-12-03T12:51:47Z

Also, applying limit to a search with an ownerid filter doesn't respect the limit.
The ownerid filter is applied after the limit, so if I have 3 files and request only one, but the first one is not my file, I get no results.

skjnldsv · 2019-12-03T14:07:08Z

TEsts failures: @since or @deprecated tag is needed in PHPDoc for OCP\Files\Search\ISearchQuery::limitToHome

Signed-off-by: Robin Appelman <[email protected]>

icewind1991 · 2019-12-03T14:33:03Z

Also, applying limit to a search with an ownerid filter doesn't respect the limit.
The ownerid filter is applied after the limit, so if I have 3 files and request only one, but the first one is not my file, I get no results.

I can't reproduce this, "non owner" files are never searched and thus never part of the sorting or limit

skjnldsv · 2019-12-03T14:36:18Z

I can't reproduce this, "non owner" files are never searched and thus never part of the sorting or limit

I don't know, I added a limit and it was returning less entries than the said limit

file count = 10
limit = 5
files returned = 2
files returned without owner filter = 5

skjnldsv · 2019-12-03T14:45:06Z

<?xml version="1.0" encoding="UTF-8"?>
<d:searchrequest xmlns:d="DAV:" xmlns:nc="http://nextcloud.org/ns" xmlns:oc="http://owncloud.org/ns" xmlns:ocs="http://open-collaboration-services.org/ns">
	<d:basicsearch>
		<d:select>
			<d:prop>
				<d:getlastmodified />
				<d:getetag />
				<d:getcontenttype />
				<oc:fileid />
				<d:getcontentlength />
				<nc:has-preview />
				<oc:favorite />
				<d:resourcetype />
			</d:prop>
		</d:select>
		<d:from>
			<d:scope>
				<d:href>/files/admin</d:href>
				<d:depth>infinity</d:depth>
			</d:scope>
		</d:from>
		<d:where>
			<d:and>
				<d:or>
					<d:eq>
						<d:prop>
							<d:getcontenttype />
						</d:prop>
						<d:literal>image/jpeg</d:literal>
					</d:eq>
					<d:eq>
						<d:prop>
							<d:getcontenttype />
						</d:prop>
						<d:literal>video/mp4</d:literal>
					</d:eq>
					<d:eq>
						<d:prop>
							<d:getcontenttype />
						</d:prop>
						<d:literal>video/quicktime</d:literal>
					</d:eq>
				</d:or>
				<!-- removing this eq solves the limit issue -->
				<d:eq>
					<d:prop>
						<oc:owner-id />
					</d:prop>
					<d:literal>admin</d:literal>
				</d:eq>
			</d:and>
		</d:where>
		<d:orderby>
			<d:order>
				<d:prop>
					<d:getlastmodified />
				</d:prop>
				<d:descending />
			</d:order>
		</d:orderby>
		<d:limit>
			<d:nresults>5</d:nresults>
		</d:limit>
	</d:basicsearch>
</d:searchrequest>

icewind1991 · 2019-12-03T15:01:12Z

stupid question but just making sure, do you have more then 2 matching files in your home storage?

skjnldsv · 2019-12-03T16:03:20Z

stupid question but just making sure, do you have more then 2 matching files in your home storage?

yes removing the limite gives me 30 results ;)

icewind1991 · 2019-12-04T15:31:04Z

Can you step trough the code with a debugger from FileSearchBackend.php line 158 and see where the limiting is happening

skjnldsv · 2019-12-04T16:16:10Z

Can you step trough the code with a debugger from FileSearchBackend.php line 158 and see where the limiting is happening

The limit is applied since the beginning

with owner eq and limit to 5

with limit but without owner eq

with owner eq but without limit

Weird right? 🤔

skjnldsv · 2019-12-04T16:32:13Z

This has to come from the Folder->search, right?
I should get 92 nodes on the with owner eq and limit to 5, but only get 3

icewind1991 · 2019-12-04T17:29:36Z

it might be filtering out results from outside /<user>/files (thumbnails etc), can you check (Node/Folder.php line 250)

skjnldsv · 2019-12-04T17:35:02Z

Same order

with owner eq and limit to 5

with limit but without owner eq

with owner eq but without limit

skjnldsv · 2019-12-04T17:37:26Z

Looks like you're right!

server/lib/private/Files/Node/Folder.php

Lines 251 to 258 in c62637d

    
           foreach ($results as $result) { 
        
           	if ($internalRootLength === 0 or substr($result['path'], 0, $internalRootLength) === $internalPath) { 
        
           		$result['internalPath'] = $result['path']; 
        
           		$result['path'] = substr($result['path'], $internalRootLength); 
        
           		$result['storage'] = $storage; 
        
           		$files[] = new \OC\Files\FileInfo($this->path . '/' . $result['path'], $storage, $result['internalPath'], $result, $mount); 
        
           	} 
        
           }

is filtering out some entries

rullzer · 2019-12-05T10:04:48Z

Lets merge this for now and open an issue for the remaining fix during the beta

icewind1991 added the 3. to review Waiting for reviews label Nov 14, 2019

icewind1991 added this to the Nextcloud 18 milestone Nov 14, 2019

icewind1991 requested review from ArtificialOwl and rullzer November 14, 2019 15:21

This comment has been minimized.

Sign in to view