feat: add propFindUnfiltered public method to Client #1518

phil-davis · 2023-11-24T07:05:13Z

add an extra unit test to cover the existing behaviour of propFind when the response has multiple resources listed, and the resources have different status.
refactor out the common code that we want to keep using into doPropFind
add propFindUnfiltered method that returns all the propfind data, including for resources that had a non-200 status, and a unit test to cover it.
return both properties and status in propFindUnfiltered
The propfind data was returned (key-value array of properties and their values). But the individual statuses were not anywhere in the returned data. The caller is going to want to be able to find out the status of the propfind of each resource. Where to put that in the returned data, without polluting the key-values of the properties themselves? I have made the returned array have:

array of resources keyed by the resource name
inside that, and array with keys 'properties' and 'status'
'properties' has the key-value array of properties and their values
'status' has the status returned for that resource

Question: I have currently assumed that there is just one status value for each resource - for example "file1.txt" might have got a 418 "I'm a teapot" status, and there are a list of properties that were returned. But is it possible that each property might have a different status?
(some properties were "easy" for the server to get, and have 200 status, other properties were "secure" and had a 403 "forbidden" status for the requesting user, other properties got 404 "not found" because that file in the folder did not have that meta-data...)

codecov · 2023-11-24T07:07:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b397796) 97.22% compared to head (2f6266b) 97.22%.

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #1518   +/-   ##
=========================================
  Coverage     97.22%   97.22%           
- Complexity     2831     2837    +6     
=========================================
  Files           175      175           
  Lines          9010     9025   +15     
=========================================
+ Hits           8760     8775   +15     
  Misses          250      250

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lib/DAV/Client.php

This returns all the properties received in the server response, even for resources that have a status that is not 200 (success).

phil-davis · 2023-11-24T07:55:37Z

@individual-it please have a look and comment

individual-it · 2023-11-24T08:29:16Z

I understand the RFC that every propstat section can have a different status:

https://www.rfc-editor.org/rfc/rfc4918#section-9.1.2

In PROPFIND responses, information about individual properties is
returned inside 'propstat' elements (see [Section 14.22](https://www.rfc-editor.org/rfc/rfc4918#section-14.22)), each
containing an individual 'status' element containing information
about the properties appearing in it.

Here a real-live example from ocis:
request data:

<?xml version="1.0"?>
<d:propfind  xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns">
  <d:prop>
    <oc:permissions />
    <oc:favorite />
    <oc:fileid />
    <oc:file-parent />
    <oc:name />
    <d:lockdiscovery />
    <d:activelock />
    <oc:owner-id />
    <oc:owner-display-name />
    <oc:shareid />
    <oc:shareroot />
    <oc:share-types />
    <oc:privatelink />
    <d:getcontentlength />
    <oc:size />
    <d:getlastmodified />
    <d:getetag />
    <d:getcontenttype />
    <d:resourcetype />
    <oc:downloadURL />
    <oc:tags />
  </d:prop>
</d:propfind>

response:

<d:multistatus
	xmlns:s="http://sabredav.org/ns"
	xmlns:d="DAV:"
	xmlns:oc="http://owncloud.org/ns">
	<d:response>
		<d:href>/remote.php/dav/spaces/d0acb8b8-9e0e-4e71-8756-3dc6a19b29bd$6a0d4b53-4a10-4007-96ff-00b454f08c4c/pp/</d:href>
		<d:propstat>
			<d:prop>
				<oc:permissions>RDNVCKZP</oc:permissions>
				<oc:favorite>0</oc:favorite>
				<oc:fileid>d0acb8b8-9e0e-4e71-8756-3dc6a19b29bd$6a0d4b53-4a10-4007-96ff-00b454f08c4c!cf6698bd-7665-4587-8264-7250bcdf431a</oc:fileid>
				<oc:file-parent>d0acb8b8-9e0e-4e71-8756-3dc6a19b29bd$6a0d4b53-4a10-4007-96ff-00b454f08c4c!6a0d4b53-4a10-4007-96ff-00b454f08c4c</oc:file-parent>
				<oc:name>pp</oc:name>
				<oc:owner-id>admin</oc:owner-id>
				<oc:owner-display-name>Admin</oc:owner-display-name>
				<oc:privatelink>https://host.docker.internal:9200/f/d0acb8b8-9e0e-4e71-8756-3dc6a19b29bd$6a0d4b53-4a10-4007-96ff-00b454f08c4c%21cf6698bd-7665-4587-8264-7250bcdf431a</oc:privatelink>
				<oc:size>4434005</oc:size>
				<d:getlastmodified>Fri, 24 Nov 2023 08:25:32 GMT</d:getlastmodified>
				<d:getetag>"c3bab89a0551248f15b240de8688484d"</d:getetag>
				<d:resourcetype>
					<d:collection/>
				</d:resourcetype>
				<oc:tags></oc:tags>
			</d:prop>
			<d:status>HTTP/1.1 200 OK</d:status>
		</d:propstat>
		<d:propstat>
			<d:prop>
				<d:lockdiscovery></d:lockdiscovery>
				<d:activelock></d:activelock>
				<oc:shareroot></oc:shareroot>
				<oc:share-types></oc:share-types>
				<d:getcontentlength></d:getcontentlength>
				<d:getcontenttype></d:getcontenttype>
				<oc:downloadURL></oc:downloadURL>
			</d:prop>
			<d:status>HTTP/1.1 404 Not Found</d:status>
		</d:propstat>
	</d:response>
	<d:response>
		<d:href>/remote.php/dav/spaces/d0acb8b8-9e0e-4e71-8756-3dc6a19b29bd$6a0d4b53-4a10-4007-96ff-00b454f08c4c/pp/welcome%20%282%29.txt</d:href>
		<d:propstat>
			<d:prop>
				<oc:permissions>RDNVWZP</oc:permissions>
				<oc:favorite>0</oc:favorite>
				<oc:fileid>d0acb8b8-9e0e-4e71-8756-3dc6a19b29bd$6a0d4b53-4a10-4007-96ff-00b454f08c4c!3272aa8c-01f6-4abd-9f28-43e69f0d2adb</oc:fileid>
				<oc:file-parent>d0acb8b8-9e0e-4e71-8756-3dc6a19b29bd$6a0d4b53-4a10-4007-96ff-00b454f08c4c!cf6698bd-7665-4587-8264-7250bcdf431a</oc:file-parent>
				<oc:name>welcome (2).txt</oc:name>
				<oc:owner-id>admin</oc:owner-id>
				<oc:owner-display-name>Admin</oc:owner-display-name>
				<oc:privatelink>https://host.docker.internal:9200/f/d0acb8b8-9e0e-4e71-8756-3dc6a19b29bd$6a0d4b53-4a10-4007-96ff-00b454f08c4c%213272aa8c-01f6-4abd-9f28-43e69f0d2adb</oc:privatelink>
				<d:getcontentlength>16</d:getcontentlength>
				<oc:size>16</oc:size>
				<d:getlastmodified>Wed, 15 Nov 2023 10:14:50 GMT</d:getlastmodified>
				<d:getetag>"be7799f33fcbb8964bdc16df9bed98a0"</d:getetag>
				<d:getcontenttype>text/plain</d:getcontenttype>
				<d:resourcetype></d:resourcetype>
				<oc:tags></oc:tags>
			</d:prop>
			<d:status>HTTP/1.1 200 OK</d:status>
		</d:propstat>
		<d:propstat>
			<d:prop>
				<d:lockdiscovery></d:lockdiscovery>
				<d:activelock></d:activelock>
				<oc:shareroot></oc:shareroot>
				<oc:share-types></oc:share-types>
				<oc:downloadURL></oc:downloadURL>
			</d:prop>
			<d:status>HTTP/1.1 404 Not Found</d:status>
		</d:propstat>
	</d:response>
	<d:response>
		<d:href>/remote.php/dav/spaces/d0acb8b8-9e0e-4e71-8756-3dc6a19b29bd$6a0d4b53-4a10-4007-96ff-00b454f08c4c/pp/artur.jpg</d:href>
		<d:propstat>
			<d:prop>
				<oc:permissions>RDNVWZP</oc:permissions>
				<oc:favorite>0</oc:favorite>
				<oc:fileid>d0acb8b8-9e0e-4e71-8756-3dc6a19b29bd$6a0d4b53-4a10-4007-96ff-00b454f08c4c!fe4f74f2-1c71-466e-9ba4-f15d967df14d</oc:fileid>
				<oc:file-parent>d0acb8b8-9e0e-4e71-8756-3dc6a19b29bd$6a0d4b53-4a10-4007-96ff-00b454f08c4c!cf6698bd-7665-4587-8264-7250bcdf431a</oc:file-parent>
				<oc:name>artur.jpg</oc:name>
				<oc:owner-id>admin</oc:owner-id>
				<oc:owner-display-name>Admin</oc:owner-display-name>
				<oc:privatelink>https://host.docker.internal:9200/f/d0acb8b8-9e0e-4e71-8756-3dc6a19b29bd$6a0d4b53-4a10-4007-96ff-00b454f08c4c%21fe4f74f2-1c71-466e-9ba4-f15d967df14d</oc:privatelink>
				<d:getcontentlength>4433989</d:getcontentlength>
				<oc:size>4433989</oc:size>
				<d:getlastmodified>Thu, 23 Jan 2020 06:46:01 GMT</d:getlastmodified>
				<d:getetag>"96446fb9dca48b364a4473f88211e92f"</d:getetag>
				<d:getcontenttype>image/jpeg</d:getcontenttype>
				<d:resourcetype></d:resourcetype>
				<oc:tags></oc:tags>
			</d:prop>
			<d:status>HTTP/1.1 425 TOO EARLY</d:status>
		</d:propstat>
	</d:response>
</d:multistatus>

individual-it · 2023-11-24T08:31:02Z

What about returning not an array but an object, at least on the highest level, so that there is no need to know the properties and status keys?

phil-davis · 2023-11-24T08:40:22Z

What about returning not an array but an object, at least on the highest level, so that there is no need to know the properties and status keys?

yes, get rid of the hardcoded keys.

and potentially each property has an associated status. So maybe we should return the data in a similar structure to what the XML has - groups of prop that have the same status are reported together in a propstat

phil-davis · 2023-11-24T10:52:44Z

I adjusted testPropFindUnfiltered so that the propfind data has some examples where the same resource gives both a set of properties with status 200, and a set of properties with status 404. Only the first group, with status 200, is returned.

Currently each key in the returned array is the href of the resource, and the value is an array with 2 keys, 'properties' and 'status'.

To return all the data, we would have to add another level of array, so that there can be multiple ('properties', 'status') sets.

phil-davis · 2023-11-24T11:25:27Z

@individual-it I added a commit that enhances the returned data - now the data attached to each href of a resource has an array of (possibly multiple) arrays that have the group of 'properties' and 'status' value. So attached to the href of each resource, the caller can find the group of properties with the status of each group - for example, most properties might have status 200, but then some properties might have status 404 (the property is not found for that resource)

This makes all the data from the server available in the return to the caller.

(And this could be refactored again to have status as the key, with the properties underneath that have that status - that avoids having keywords 'status' and 'properties' in the returned data. That structure would be a lot like what parseMultiStatus already creates.

phil-davis · 2023-11-27T04:26:06Z

And this could be refactored again to have status as the key, with the properties underneath that have that status

I did that in PR #1519 to show what the code looks like.

Or we can design some more object-oriented solution that returns an array of "property" objects for each resource. And each "property" object would have a "value" and a "status", and the consumer of the returned data can find out value and status with getValue and getStatus calls.

phil-davis · 2023-12-11T13:54:39Z

The solution in #1519 was the one used.

phil-davis added 2 commits November 24, 2023 11:44

test: add unit test for PROPFIND with varying status values

f8abffa

refactor: split out some code into doPropFind

ebdd39f

phil-davis marked this pull request as draft November 24, 2023 07:06

phil-davis commented Nov 24, 2023

View reviewed changes

lib/DAV/Client.php Outdated Show resolved Hide resolved

phil-davis commented Nov 24, 2023

View reviewed changes

lib/DAV/Client.php Outdated Show resolved Hide resolved

phil-davis force-pushed the issue-1041 branch from 5a84c8c to 0cb06ea Compare November 24, 2023 07:12

feat: add propFindUnfiltered public method

170e16b

This returns all the properties received in the server response, even for resources that have a status that is not 200 (success).

phil-davis force-pushed the issue-1041 branch from 0cb06ea to 170e16b Compare November 24, 2023 07:26

feat: return both properties and status in propFindUnfiltered

43c8cf1

test: adjust testPropFindUnfiltered to show multi-propstat behavior

68e2ad4

feat: return all property-status combinations from PROPFIND

2f6266b

phil-davis mentioned this pull request Nov 27, 2023

feat: return PROPFIND properties indexed by status #1519

Merged

phil-davis closed this Dec 11, 2023

phil-davis deleted the issue-1041 branch December 11, 2023 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add propFindUnfiltered public method to Client #1518

feat: add propFindUnfiltered public method to Client #1518

phil-davis commented Nov 24, 2023 •

edited

Loading

codecov bot commented Nov 24, 2023 •

edited

Loading

phil-davis commented Nov 24, 2023

individual-it commented Nov 24, 2023

individual-it commented Nov 24, 2023

phil-davis commented Nov 24, 2023

phil-davis commented Nov 24, 2023

phil-davis commented Nov 24, 2023

phil-davis commented Nov 27, 2023 •

edited

Loading

phil-davis commented Dec 11, 2023

feat: add propFindUnfiltered public method to Client #1518

feat: add propFindUnfiltered public method to Client #1518

Conversation

phil-davis commented Nov 24, 2023 • edited Loading

codecov bot commented Nov 24, 2023 • edited Loading

Codecov Report

phil-davis commented Nov 24, 2023

individual-it commented Nov 24, 2023

individual-it commented Nov 24, 2023

phil-davis commented Nov 24, 2023

phil-davis commented Nov 24, 2023

phil-davis commented Nov 24, 2023

phil-davis commented Nov 27, 2023 • edited Loading

phil-davis commented Dec 11, 2023

phil-davis commented Nov 24, 2023 •

edited

Loading

codecov bot commented Nov 24, 2023 •

edited

Loading

phil-davis commented Nov 27, 2023 •

edited

Loading