Skip to content

Bugfix/not load groups if not necessary#35822

Merged
DeepDiver1975 merged 1 commit into
stable10from
bugfix/not-load-groups-if-not-necessary
Jul 16, 2019
Merged

Bugfix/not load groups if not necessary#35822
DeepDiver1975 merged 1 commit into
stable10from
bugfix/not-load-groups-if-not-necessary

Conversation

@DeepDiver1975

@DeepDiver1975 DeepDiver1975 commented Jul 15, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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 DeepDiver1975 self-assigned this Jul 15, 2019
@DeepDiver1975

DeepDiver1975 commented Jul 15, 2019

Copy link
Copy Markdown
Member Author

smb_windows tests are failing -> ignore

jvillafanez
jvillafanez reviewed Jul 15, 2019
View reviewed changes
Comment thread lib/private/legacy/util.php
$userGroups
);
$readOnlyGroupMemberships = [];
if ($readOnlyGroups) {

@jvillafanez jvillafanez Jul 15, 2019

Copy link
Copy Markdown
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

phil-davis commented Jul 16, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

codecov Bot commented Jul 16, 2019

Copy link
Copy Markdown

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

codecov Bot commented Jul 16, 2019

Copy link
Copy Markdown

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
Closed
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jvillafanez jvillafanez jvillafanez approved these changes

Assignees

@DeepDiver1975 DeepDiver1975

Labels

3 - To Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@DeepDiver1975 @phil-davis @jvillafanez @micbar @patrickjahns