Skip to content
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

perf(dav): Do not call general setupFS on ever dav auth #36626

Merged
merged 1 commit into from
Feb 9, 2023

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Feb 9, 2023

This could save setting up the full file system and instead use setupForPath. Unsure about possible side effects, webdav works here still, but let's see what the tests have to say.

Local comparison:

Screenshot 2023-02-09 at 08 57 22

Perftesting

Checklist

@juliusknorr

This comment was marked as outdated.

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would need to be tested carefully, do we know for sure those calls are not needed?

@juliusknorr juliusknorr force-pushed the perf/dav-auth-setupfs branch from 2e27b93 to 7b413a4 Compare February 9, 2023 09:19
@@ -117,15 +117,15 @@ public function testValidateUserPassOfAlreadyDAVAuthenticatedUser(): void {
$user = $this->getMockBuilder(IUser::class)
->disableOriginalConstructor()
->getMock();
$user->expects($this->exactly(2))
$user->expects($this->exactly(1))
Copy link
Member

@icewind1991 icewind1991 Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo we should get of these exactly calls altogether. Unless the test is actually testing the amount of times a method is called, it's just implementation details leaking out into tests needlessly.

Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fine, automatic filesystem setup should handle all cases now

@juliusknorr juliusknorr merged commit dd891e7 into master Feb 9, 2023
@juliusknorr juliusknorr deleted the perf/dav-auth-setupfs branch February 9, 2023 12:41
@blizzz blizzz mentioned this pull request Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants