perf: Skip photos related properties in custom properties

[juliusknorr]

We probably should think about better ways to generally exclude known ones (maybe we can even just skip any in the oc or nc namespace, but this saves 1 query per file in WebDAV search requests that the photos app is doing:

https://blackfire.io/profiles/compare/53aede2e-4560-4dff-8dcc-80e925e2fc4a/graph

[artonge]

I think we should rather not request them in Photos.

[artonge]

• [stable31] fix(perf): Do not request faces properties in search requests photos#3030

[juliusknorr]

Agreed that photos can avoid, but this one might still be relevant to avoid other cases where anyone requests those properties either through SEARCH but also through a PROPFIND. The Photos PR would still be affected by the N+1 queries when getting the collection

[artonge]

but this one might still be relevant to avoid other cases where anyone requests those properties either through SEARCH but also through a PROPFIND

I think we should rather fix those rather than polluting server with 3rd party properties.

The Photos PR would still be affected by the N+1 queries when getting the collection

When getting the collection, the properties will be handled by the photo propfind plugin, so it should not get affected as the CustomPropertiesBackend will only handle 404 properties:

server/apps/dav/lib/DAV/CustomPropertiesBackend.php

Lines 150 to 157 in d5be952

	$requestedProps = $propFind->get404Properties();
	// these might appear
	$requestedProps = array_diff(
	$requestedProps,
	self::IGNORED_PROPERTIES,
	);
	$requestedProps = array_filter(

[juliusknorr]

I think we should rather fix those rather than polluting server with 3rd party properties.
When getting the collection, the properties will be handled by the photo propfind plugin, so it should not get affected as the CustomPropertiesBackend will only handle 404 properties:

The main risk I see is that those queries would still happen if a client behaves badly and requests those even if the app providing them is not handling them or disabled.

However maybe we should rethink rather how the filtering is done. The use cases of the custom properties are rather specific, on our instance those are the ones stored:

MariaDB [oc]> select count(*) as c, propertyname from oc_properties group by propertyname order by c;
+------+--------------------------------------------------------------+
| c    | propertyname                                                 |
+------+--------------------------------------------------------------+
|    1 | {http://owncloud.org/ns}mymetadata                           |
|    1 | {urn:ietf:params:xml:ns:caldav}calendar-description          |
|    2 | {urn:ietf:params:xml:ns:caldav}schedule-default-calendar-URL |
|    4 | {urn:schemas-microsoft-com:}Win32CreationTime                |
|    4 | {urn:schemas-microsoft-com:}Win32LastAccessTime              |
|    4 | {urn:schemas-microsoft-com:}Win32LastModifiedTime            |
|    4 | {urn:schemas-microsoft-com:}Win32FileAttributes              |
|    9 | {urn:ietf:params:xml:ns:caldav}schedule-calendar-transp      |
|   24 | {http://owncloud.org/ns}enabled                              |
|   28 | {urn:ietf:params:xml:ns:caldav}default-alarm-vevent-date     |
|   29 | {urn:ietf:params:xml:ns:caldav}default-alarm-vevent-datetime |
|  105 | {DAV:}displayname                                            |
|  135 | {urn:ietf:params:xml:ns:caldav}calendar-availability         |
|  203 | {urn:ietf:params:xml:ns:caldav}calendar-timezone             |
|  344 | {http://apple.com/ns/ical/}calendar-color                    |
| 3263 | {http://apple.com/ns/ical/}calendar-order                    |
| 4833 | {http://owncloud.org/ns}calendar-enabled                     |
+------+--------------------------------------------------------------+

A general allow list does not make much sense there I think, but I was thinking about having one for the owncloud and nextcloud namespaces. For those we should know exactly which ones we want to read from there and can just skip the others.

Another topic i remember from the past is that returning null is a common issue in other propfind plugins that then leads to a 404 property that the custom properties backend is still trying to handle (not for photos ones though, those all seem to return proper values).

[come-nc]

This looks ugly as hell, and it’s not clear to me why such a list is needed instead of having a list of non-ignored props instead, or something even cleaner.

But the list is already there and ugly, should not harm much to extend it, if it improves perf let’s go, it can be replaced by a proper solution later.

Addressed

[blizzz]

/backport to stable31

[blizzz]

/backport to stable30

[blizzz]

/backport to stable29