-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[full-ci] Enable streaming for propfind #38583
[full-ci] Enable streaming for propfind #38583
Conversation
fe0870d
to
5681f1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add capability for the clients, so they don't meltdown servers without this improvement.
@DeepDiver1975 @hodyroff what's next? Can be released in next iOS version, but there's no capability yet. When can this be released? |
Once these things are taken care of:
total effort: ~2 weeks |
5681f1a
to
8e4c4a0
Compare
💥 Acceptance tests pipeline apiProxySmoke-8-5-mariadb10.2-php7.4 failed. The build has been cancelled. |
Regarding those invalid XML responses:
I think the problem exists in the DAV lib: https://github.com/sabre-io/dav/blob/master/lib/DAV/Server.php#L1640. With something like this it works for me:
2 things I'm currently not sure of:
|
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. |
a15a344
to
c49dc1c
Compare
f149d0b
to
d49e3c3
Compare
c8f8cef
to
9932411
Compare
@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:
|
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. |
0d9d5be
to
9932411
Compare
@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? |
I adjusted an existing acceptance test to match these scenarios.
I couldn't find it, but maybe @phil-davis knows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some optional remarks/thought. LGTM 👍
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@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) |
8233bf3
to
daa44f4
Compare
daa44f4
to
300d314
Compare
300d314
to
2914ec2
Compare
Kudos, SonarCloud Quality Gate passed! |
Capability for recursive propfind confirmed in 10.9.0 beta1:
But no capabiltiy seen, that mentions streaming. Probably OK. |
Confirmed fixed in 10.9.0 RC2
|
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 whetherdepth=infinity
is allowed for propfind requests. It defaults to true.#36336 reloaded
Fixes #14531
Types of changes
Checklist:
config-to-docs
after PR #has been merged docs#4127