Skip to content

obey user min search length configuration on remote shares #35977

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

Merged
merged 1 commit into from
Aug 6, 2019

Conversation

karakayasemi
Copy link
Contributor

@karakayasemi karakayasemi commented Aug 3, 2019

Description

Currently, sharing API is not respecting user.min_search_length configuration when returning remote users. This PR aims to fix this behavior.
The API should return exact matches even search_min_length is not enough. For that reason, the PR only changes behavior on the non-exact match scenario.

Related Issue

https://github.com/owncloud/enterprise/issues/3438

Motivation and Context

Fixing bug.

How Has This Been Tested?

Unit tests and manually with the following steps:

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:

@phil-davis
Copy link
Contributor

Note: I have asked for the drone JavaScript cache problem to be fixed. CI is broken because of that.

@jvillafanez
Copy link
Member

I think we should clarify first what is the expected behaviour.

If you're looking for users in the remote server, I expect the remote server to return users based on whatever is configured there. If the remote server has a value of user.min_search_length = 5, I expect the remote server to return users after filling the first 5 chars. After that, the search is done, so the local server shouldn't need to apply anything specific to the search.

Note that there are at least a couple of cases to check:

  • local = 5, remote = 2
  • local = 2, remote = 5

I guess this is for the first case where the local server has a greater restriction. However, since the search is done in the remote server, I think only the remote server's configuration should apply. If the local server wants to apply additional restrictions, I think it's better to use a different config key.


Alternatively, I think it's better if the local server doesn't make the request to the remote server in the first place. Basically, if the local configuration has user.min_search_length = 5, the local server won't make a request to the remote one until those chars are filled.
It isn't the local server the one filtering the results, but asking for a minimum search length to the remote server. I think this is a better approach without adding new configuration keys.

Both cases described above are handled the same way without any modification.

@codecov
Copy link

codecov bot commented Aug 5, 2019

Codecov Report

Merging #35977 into master will increase coverage by 0.26%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #35977      +/-   ##
============================================
+ Coverage     66.43%   66.69%   +0.26%     
- Complexity    20183    20184       +1     
============================================
  Files          1233     1233              
  Lines         68965    69001      +36     
============================================
+ Hits          45814    46020     +206     
+ Misses        23151    22981     -170
Flag Coverage Δ Complexity Δ
#phpunit 66.69% <100%> (+0.26%) 20184 <0> (+1) ⬆️
Impacted Files Coverage Δ Complexity Δ
...files_sharing/lib/Controller/ShareesController.php 90.34% <100%> (+0.07%) 105 <0> (+1) ⬆️
lib/private/Files/Cache/HomePropagator.php 88.88% <0%> (-11.12%) 3% <0%> (ø)
apps/files/lib/Command/Scan.php 71.91% <0%> (-9.17%) 74% <0%> (ø)
lib/private/Share/Share.php 71.61% <0%> (+0.2%) 507% <0%> (ø) ⬇️
apps/dav/lib/Connector/Sabre/File.php 84.19% <0%> (+0.64%) 115% <0%> (ø) ⬇️
apps/files_versions/lib/Storage.php 72.39% <0%> (+2.53%) 95% <0%> (ø) ⬇️
lib/private/legacy/util.php 70.28% <0%> (+2.54%) 240% <0%> (ø) ⬇️
lib/private/Files/Storage/Common.php 85.15% <0%> (+3.33%) 141% <0%> (ø) ⬇️
apps/files_trashbin/lib/Quota.php 92% <0%> (+4%) 9% <0%> (ø) ⬇️
lib/private/Files/Type/Detection.php 74.56% <0%> (+4.38%) 52% <0%> (ø) ⬇️
... and 9 more

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 851bd2e...6805173. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Aug 5, 2019

Codecov Report

Merging #35977 into master will increase coverage by 0.26%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #35977      +/-   ##
============================================
+ Coverage     66.43%   66.69%   +0.26%     
- Complexity    20183    20184       +1     
============================================
  Files          1233     1233              
  Lines         68965    69001      +36     
============================================
+ Hits          45814    46020     +206     
+ Misses        23151    22981     -170
Flag Coverage Δ Complexity Δ
#phpunit 66.69% <100%> (+0.26%) 20184 <0> (+1) ⬆️
Impacted Files Coverage Δ Complexity Δ
...files_sharing/lib/Controller/ShareesController.php 90.34% <100%> (+0.07%) 105 <0> (+1) ⬆️
lib/private/Files/Cache/HomePropagator.php 88.88% <0%> (-11.12%) 3% <0%> (ø)
apps/files/lib/Command/Scan.php 71.91% <0%> (-9.17%) 74% <0%> (ø)
lib/private/Share/Share.php 71.61% <0%> (+0.2%) 507% <0%> (ø) ⬇️
apps/dav/lib/Connector/Sabre/File.php 84.19% <0%> (+0.64%) 115% <0%> (ø) ⬇️
apps/files_versions/lib/Storage.php 72.39% <0%> (+2.53%) 95% <0%> (ø) ⬇️
lib/private/legacy/util.php 70.28% <0%> (+2.54%) 240% <0%> (ø) ⬇️
lib/private/Files/Storage/Common.php 85.15% <0%> (+3.33%) 141% <0%> (ø) ⬇️
apps/files_trashbin/lib/Quota.php 92% <0%> (+4%) 9% <0%> (ø) ⬇️
lib/private/Files/Type/Detection.php 74.56% <0%> (+4.38%) 52% <0%> (ø) ⬇️
... and 9 more

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 851bd2e...6805173. Read the comment docs.

@karakayasemi
Copy link
Contributor Author

@jvillafanez As far I see from the code, there is no request for the remote server. We are only searching in the local address book. If we have an exact match, we are returning the result whatever search length. It is intended behavior and I did not change this behavior.

If there is no exact match, currently, if there are half-matched results in contacts, the API returns these results. Also, if the search string matches with the remote user format, the API adds the string itself to returned results. I just added a length control for this non-exact match scenario.

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.

5 participants