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

Bugfix/not load groups if not necessary #35822

Merged
merged 1 commit into from
Jul 16, 2019

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Jul 15, 2019

please note this targets stable10 already - I will not create PRs for master anymore due to your plans to kill master branch

Description

Even if no read only groups are defined (~99,9% of all instance) all groups for the current user will be loaded.
In case of LDAP this means loading the groups from there and this has a major performance impact.

How Has This Been Tested?

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:

@DeepDiver1975
Copy link
Member Author

smb_windows tests are failing -> ignore

$userGroups
);
$readOnlyGroupMemberships = [];
if ($readOnlyGroups) {
Copy link
Member

Choose a reason for hiding this comment

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

it might be worthy to include a comment to remind that an empty array is considered "false" and, as such, we won't hit the group manager (typical PHP things)

@phil-davis
Copy link
Contributor

phil-davis commented Jul 16, 2019

smb_windows tests are failing -> ignore

It got the "hung restore step in drone" problem. Not even an smb_winddows issue.

I restarted the drone job. That should get green CI.

@codecov
Copy link

codecov bot commented Jul 16, 2019

Codecov Report

Merging #35822 into stable10 will decrease coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff               @@
##             stable10   #35822      +/-   ##
==============================================
- Coverage       65.09%   65.04%   -0.06%     
- Complexity      20239    20268      +29     
==============================================
  Files            1300     1301       +1     
  Lines           77238    77387     +149     
  Branches         1301     1301              
==============================================
+ Hits            50279    50333      +54     
- Misses          26574    26669      +95     
  Partials          385      385
Flag Coverage Δ Complexity Δ
#javascript 53.85% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.21% <100%> (-0.07%) 20268 <0> (+29)
Impacted Files Coverage Δ Complexity Δ
lib/private/legacy/util.php 70.28% <100%> (+0.1%) 240 <0> (+1) ⬆️
apps/files_external/lib/config.php 11.76% <0%> (-23.24%) 19% <0%> (+19%)
apps/federatedfilesharing/lib/Notifications.php 38.33% <0%> (-3.44%) 48% <0%> (+3%)
apps/dav/appinfo/v1/publicwebdav.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
...s/dav/lib/Files/Sharing/PublicLinkEventsPlugin.php 100% <0%> (ø) 6% <0%> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50df15a...11106ea. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jul 16, 2019

Codecov Report

Merging #35822 into stable10 will decrease coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff               @@
##             stable10   #35822      +/-   ##
==============================================
- Coverage       65.09%   65.04%   -0.06%     
- Complexity      20239    20268      +29     
==============================================
  Files            1300     1301       +1     
  Lines           77238    77387     +149     
  Branches         1301     1301              
==============================================
+ Hits            50279    50333      +54     
- Misses          26574    26669      +95     
  Partials          385      385
Flag Coverage Δ Complexity Δ
#javascript 53.85% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.21% <100%> (-0.07%) 20268 <0> (+29)
Impacted Files Coverage Δ Complexity Δ
lib/private/legacy/util.php 70.28% <100%> (+0.1%) 240 <0> (+1) ⬆️
apps/files_external/lib/config.php 11.76% <0%> (-23.24%) 19% <0%> (+19%)
apps/federatedfilesharing/lib/Notifications.php 38.33% <0%> (-3.44%) 48% <0%> (+3%)
apps/dav/appinfo/v1/publicwebdav.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
...s/dav/lib/Files/Sharing/PublicLinkEventsPlugin.php 100% <0%> (ø) 6% <0%> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50df15a...11106ea. Read the comment docs.

@DeepDiver1975 DeepDiver1975 merged commit 806010c into stable10 Jul 16, 2019
@delete-merged-branch delete-merged-branch bot deleted the bugfix/not-load-groups-if-not-necessary branch July 16, 2019 06:39
@davitol davitol mentioned this pull request Sep 3, 2019
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants