Skip to content

Default for propfind depth infinity should be false as in the past - #40016

Merged
phil-davis merged 2 commits into
masterfrom
e-5154-inf-depth-fix
Apr 29, 2022
Merged

Default for propfind depth infinity should be false as in the past#40016
phil-davis merged 2 commits into
masterfrom
e-5154-inf-depth-fix

Conversation

@mrow4a

@mrow4a mrow4a commented Apr 25, 2022

Copy link
Copy Markdown
Contributor

Change in https://github.com/owncloud/core/pull/38583/files#diff-efe547293d3c10c67f8014fd439977f1c8f1073dcb65a1a83dc33e7d266dec70R48 release for oc10.9 set dav.propfind.depth_infinity' => true as default and caused some performance issues on some setups. This PR brings back the default as in the past, as discussed in #38583 (comment).

  • review unit tests for potential adjustments

fixes https://github.com/owncloud/enterprise/issues/5154

@mrow4a mrow4a self-assigned this Apr 25, 2022
@mrow4a mrow4a changed the title default for propfind depth infinity should be false as in the past Apr 25, 2022
@mrow4a
mrow4a force-pushed the e-5154-inf-depth-fix branch from d2caa6e to 6b9ee22 Compare April 25, 2022 20:12
@ownclouders

ownclouders commented Apr 25, 2022

Copy link
Copy Markdown
Contributor

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

https://drone.owncloud.com/owncloud/core/35465/51

@mmattel

mmattel commented Apr 26, 2022

Copy link
Copy Markdown
Contributor

Hooking myself in to get notified when merged so we can start a config-to-docs run.
@EParzefall FYI

@phil-davis

phil-davis commented Apr 26, 2022

Copy link
Copy Markdown
Contributor

Note: I am working on the acceptance tests in PR #40021 so that I don't make a mess of this PR.

I will first get them passing, then sort out adding a few scenarios to test:
a) what happens when a client does send a depth infinity request, but depth infinity is disabled
b) enabling depth infinity and a few test cases to be confident that it generally works

Comment thread config/config.sample.php Outdated
@jvillafanez

Copy link
Copy Markdown
Member

The solution itself looks good, but I think we should clarify the doc in the config.sample

@mrow4a
mrow4a force-pushed the e-5154-inf-depth-fix branch from 6b9ee22 to e7c59af Compare April 26, 2022 19:53
@mrow4a mrow4a changed the title WIP: default for propfind depth infinity should be false as in the past Apr 26, 2022
@mrow4a
mrow4a requested a review from jvillafanez April 26, 2022 19:54
@mrow4a

mrow4a commented Apr 26, 2022

Copy link
Copy Markdown
Contributor Author

@phil-davis I think we should first have this change and then add acceptance on this basis.

@phil-davis

phil-davis commented Apr 27, 2022

Copy link
Copy Markdown
Contributor

@phil-davis I think we should first have this change and then add acceptance on this basis.

agree - we just need to do the minimal change to the acceptance tests so that the current tests pass. I will keep working on that in #40021

@phil-davis
phil-davis force-pushed the e-5154-inf-depth-fix branch from 64ba86b to 47a651c Compare April 29, 2022 10:57
@owncloud owncloud deleted a comment from update-docs Bot Apr 29, 2022
@phil-davis

Copy link
Copy Markdown
Contributor

I rebased and pushed the test changes from #40021

IMO CI should pass and we can merge.

@sonarqubecloud

Copy link
Copy Markdown
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants