[full-ci] Enable streaming for propfind

[DeepDiver1975]

Description

Propfind requests will now be streamed to reduce memory usage with large responses.
Additionally, the new config dav.propfind.depth_infinity has been added to tell clients whether depth=infinity is allowed for propfind requests. It defaults to true.

#36336 reloaded
Fixes #14531

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised: Run config-to-docs after PR #has been merged docs#4127
• Changelog item

[michaelstingl]

Please add capability for the clients, so they don't meltdown servers without this improvement.

[michaelstingl]

@DeepDiver1975 @hodyroff what's next? Can be released in next iOS version, but there's no capability yet. When can this be released?

[DeepDiver1975]

When can this be released?

Once these things are taken care of:

• capability
• add dedicated acceptance tests for depth infinity (especially exceptional scenarios)
• fix any issues which arise (basically fix all acceptance tests when streaming is enabled on all requests/responses)
• enable streaming only if depth: infinity is used (risk minimization)

total effort: ~2 weeks

[ownclouders]

💥 Acceptance tests pipeline apiProxySmoke-8-5-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/32901/172/1

[JammingBen]

@DeepDiver1975

Regarding those invalid XML responses:

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\NotFound</s:exception>
  <s:message>Principal with name dff not found</s:message>
</d:error>
<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns"% 

I think the problem exists in the DAV lib: https://github.com/sabre-io/dav/blob/master/lib/DAV/Server.php#L1640. $w->openUri('php://output'); will be called, then $this->writeMultiStatus($w, $fileProperties, $strip404s);. This is the place where exceptions will be thrown in case of an invalid request. Because exceptions are not catched here, $w->flush(); will never be called -> the stream doesn't end properly -> bad.

With something like this it works for me:

try {
    $this->writeMultiStatus($w, $fileProperties, $strip404s);
} catch (\Exception $e) {
    $w->flush();
    throw $e;
}

2 things I'm currently not sure of:

• Why are our errors thrown in writeMultiStatus()? Is this the "normal"/recommended way?
• Can we hook into this call somehow to fix it? Doesn't look like so to me...

[DeepDiver1975]

My expectation would be that writeMultiStatus never throws an exceptions.

This is what I was mentioning earlier: we need to make sure that either all conditions which could throw an exception are caught earlier.
Or that in case of an exception within writeMultiStatus we catch it locally and write the corresponding status within the multi status

[JammingBen]

@DeepDiver1975 Pipeline runs now, please have a look. We basically just added a preflight check if the tree is traversable and if the resource has read permissions in case of a share.

Regarding the open to-dos, please verify the following assumptions:

• capability -> we just add depth_infinity to the capabilities and set it to true
• enable streaming only if depth: infinity is used (risk minimization) -> That's outdated, we always enable streaming
• Unit tests need to be added
• Acceptance tests need to be added (maybe @phil-davis and his team can support us here)

[DeepDiver1975]

* `capability` -> we just add `depth_infinity` to the capabilities and set it to `true`

please double check if this shall be configurable @michaelstingl @pmaier1

* `enable streaming only if depth: infinity is used (risk minimization)` -> That's outdated, we always enable streaming

👍

* Unit tests need to be added

👍

* Acceptance tests need to be added (maybe @phil-davis and his team can support us here)

👍

[pmaier1]

please double check if this shall be configurable @michaelstingl @pmaier1

Please make it configurable, default on. So that we have an easy way to disable it if we encounter issues.

[AlexAndBear]

@pmaier1 @DeepDiver1975 what should happen if depth_infinity is set to false and the client does a propfind with this header, should we throw an error?

[pmaier1]

@pmaier1 @DeepDiver1975 what should happen if depth_infinity is set to false and the client does a propfind with this header, should we throw an error?

How do we handle it today?

[JammingBen]

PROPFIND with depth=infinity fails if dav.propfind.depth_infinity is set to false
PROPFIND with depth=infinity succeeds if dav.propfind.depth_infinity is set to true (which is default)

I adjusted an existing acceptance test to match these scenarios.

Is there an acceptance test for propfind on a folder which has external storages inside which are (temporarily) not available?

I couldn't find it, but maybe @phil-davis knows.

[IljaN]

Just some optional remarks/thought. LGTM 👍

[phil-davis]

Is there an acceptance test for propfind on a folder which has external storages inside which are (temporarily) not available?

Nothing that I am aware of. I don't remember ever coding something that would "pull out the external storage backend" during a test scenario.

[phil-davis]

LGTM

[JammingBen]

Nothing that I am aware of. I don't remember ever coding something that would "pull out the external storage backend" during a test scenario.

@phil-davis Could you help us implementing such test?

done

[phil-davis]

Could you help us implementing such test?

@JammingBen we can try. Please create an issue and assign me. If we can do it with "local storage" then it might be easy enough (rename the local storage folder, try some API actions, rename the folder back again, try the API again)

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

72.0% Coverage
0.0% Duplication

[jnweiger]

Capability for recursive propfind confirmed in 10.9.0 beta1:

curl -k -s -u admin:admin 'https://localhost/ocs/v1.php/cloud/capabilities?format=json' | jq .ocs.data.capabilities.dav.propfind
{
  "depth_infinity": true
}

But no capabiltiy seen, that mentions streaming. Probably OK.

[jnweiger]

Confirmed fixed in 10.9.0 RC2

• On a tree with 40000 files and folders, the first output appears conistently within 10 seconds:
• curl -k -s -u admin:admin -XPROPFIND https://$server/remote.php/dav/files/admin/Deep -H "Depth:infinity" | pv > /dev/null
• total wall clock time: 43 sec.