allow admins to enable medial search on group and user by karakayasemi · Pull Request #34659 · owncloud/core

karakayasemi · 2019-03-02T21:41:27Z

Description

allow admin to enable medial search on group and user

Related Issue

Fixes #33883 and https://github.com/owncloud/enterprise/issues/2311

Motivation and Context

we have accounts.enable_medial_search but we should move to one option respected globally for users, and one for groups, making the results more consistent.

How Has This Been Tested?

Create a group with "test" gid,
add 'groups.enable_medial_search' => true line to config
Sharing search dialog should list "test" group when you write "es"
change config as 'groups.enable_medial_search' => false
Sharing search dialog should not list "test" group when you write "es"

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:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)

codecov · 2019-03-02T22:10:16Z

Codecov Report

Merging #34659 into master will increase coverage by <.01%.
The diff coverage is 76%.

@@             Coverage Diff              @@
##             master   #34659      +/-   ##
============================================
+ Coverage     65.25%   65.25%   +<.01%     
- Complexity    18458    18463       +5     
============================================
  Files          1207     1207              
  Lines         69895    69914      +19     
  Branches       1280     1280              
============================================
+ Hits          45608    45621      +13     
- Misses        23915    23921       +6     
  Partials        372      372

Flag	Coverage Δ	Complexity Δ
#javascript	`53.04% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.65% <76%> (ø)`	`18463 <0> (+5)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/User/Database.php	`71.65% <70%> (-0.85%)`	`46 <0> (+2)`
lib/private/Group/Database.php	`96.26% <80%> (-2.1%)`	`27 <0> (+3)`

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 03e6987...3e97be5. Read the comment docs.

codecov · 2019-03-02T22:10:17Z

Codecov Report

Merging #34659 into master will increase coverage by <.01%.
The diff coverage is 76%.

@@             Coverage Diff              @@
##             master   #34659      +/-   ##
============================================
+ Coverage     65.25%   65.25%   +<.01%     
- Complexity    18458    18463       +5     
============================================
  Files          1207     1207              
  Lines         69895    69914      +19     
  Branches       1280     1280              
============================================
+ Hits          45608    45621      +13     
- Misses        23915    23921       +6     
  Partials        372      372

Flag	Coverage Δ	Complexity Δ
#javascript	`53.04% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.65% <76%> (ø)`	`18463 <0> (+5)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/User/Database.php	`71.65% <70%> (-0.85%)`	`46 <0> (+2)`
lib/private/Group/Database.php	`96.26% <80%> (-2.1%)`	`27 <0> (+3)`

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 03e6987...3e97be5. Read the comment docs.

codecov · 2019-03-02T22:10:31Z

Codecov Report

Merging #34659 into master will decrease coverage by 0.01%.
The diff coverage is 81.25%.

@@             Coverage Diff              @@
##             master   #34659      +/-   ##
============================================
- Coverage      65.3%   65.28%   -0.02%     
- Complexity    18478    18487       +9     
============================================
  Files          1209     1210       +1     
  Lines         69964    70009      +45     
  Branches       1280     1280              
============================================
+ Hits          45689    45707      +18     
- Misses        23903    23930      +27     
  Partials        372      372

Flag	Coverage Δ	Complexity Δ
#javascript	`53.04% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.69% <81.25%> (-0.02%)`	`18487 <1> (+9)`

Impacted Files	Coverage Δ	Complexity Δ
lib/private/User/Database.php	`71.65% <70%> (-0.85%)`	`46 <0> (+2)`
lib/private/Group/Database.php	`96.42% <86.36%> (-1.94%)`	`28 <1> (+4)`
lib/private/Server.php	`86.56% <0%> (-0.12%)`	`253% <0%> (ø)`
core/register_command.php	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
core/Command/Db/GenerateChangeScript.php	`0% <0%> (ø)`	`3% <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 a26c6e6...106930e. Read the comment docs.

jvillafanez · 2019-03-06T10:03:09Z

 		if ($search !== '') {
-			$parameters[] = '%' . $this->dbConn->escapeLikeParameter($search) . '%';
+			$search = \OC::$server->getDatabaseConnection()->escapeLikeParameter($search);
+			$allowMedialSearches = \OC::$server->getConfig()->getSystemValue("accounts.enable_medial_search", true);


Try to inject the services

I injected services in this class and applied fixDI hack on changed functions. But, User/Database class a bit tricky, I don't prefer to touch this class for dependency injection in the scope of this pull request.

@karakayasemi you can simply add a "TODO: inject" then

jvillafanez · 2019-03-06T10:05:33Z

If the option will be used only for DB groups, it should be clarified in the sample file.

karakayasemi · 2019-03-08T15:44:42Z

If the option will be used only for DB groups, it should be clarified in the sample file.

A sentence was added for this purpose to sample.config.php.

jvillafanez · 2019-03-11T13:36:34Z

A sentence was added for this purpose to sample.config.php.

If not sure if it's clear enough...
We have several backends (DB, LDAP, shibboleth, ...). That flag will only apply to the DB backend, and not any other.

codecov · 2019-03-12T15:22:57Z

Codecov Report

Merging #34659 into master will increase coverage by <.01%.
The diff coverage is 91.66%.

@@             Coverage Diff              @@
##             master   #34659      +/-   ##
============================================
+ Coverage      65.3%    65.3%   +<.01%     
- Complexity    18479    18481       +2     
============================================
  Files          1209     1209              
  Lines         69971    69981      +10     
  Branches       1280     1280              
============================================
+ Hits          45695    45704       +9     
- Misses        23904    23905       +1     
  Partials        372      372

Flag	Coverage Δ	Complexity Δ
#javascript	`53.04% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.71% <91.66%> (ø)`	`18481 <1> (+2)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Group/Database.php	`97.72% <91.66%> (-0.64%)`	`26 <1> (+2)`

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 4ac13e2...7fe2f35. Read the comment docs.

karakayasemi · 2019-03-12T15:42:22Z

If not sure if it's clear enough...
We have several backends (DB, LDAP, shibboleth, ...). That flag will only apply to the DB backend, and not any other.

@jvillafanez, as far as I understand from the following sentences of this comment #33883 (comment), it should apply to only DB backend:

The key thing should be that within ownCloud - all account searches are only hitting the database so we have control over the expectations for the search time.

If you have any suggestion, we can improve the explanation line of config.sample.php

jvillafanez · 2019-03-13T08:34:09Z

The "accounts.enable_medial_search" key has to apply only to account searches, so using it against an specific backend is wrong.

I'm not sure if using medial search for users inside the DB backend (not in accounts) has any use. We'll need to sync the users into the account table, and the user manager checks against the account table, so a "userbackend.db.enable_medial_search" key affecting just the DB backend has a very limited usage.
For groups is different because we're still checking against the backend.

* Allow medial search on group properties like gid and other search terms.
* Allows finding 'test' when searching for 'es'.
* It is used in group database queries and may slow down group search.

What about

* Allow medial search on the group id. Allows finding 'test' when searching for 'es'.
* This is only used in the DB group backend (local groups), and won't be used against LDAP
* Shibboleth or any other group backend.

It's more clear that it will apply on the group id (there is no other usage right now), and also that it will apply only to the DB backend.

karakayasemi · 2019-03-13T17:09:38Z

@jvillafanez I changed the sample.config.php as you suggest. Please review again. Thanks.

jvillafanez · 2019-03-14T08:40:10Z

 			$search = \OC::$server->getDatabaseConnection()->escapeLikeParameter($search);
-			$parameters[] = '%' . $search . '%';
-			$parameters[] = '%' . $search . '%';
+			$allowMedialSearches = \OC::$server->getConfig()->getSystemValue("accounts.enable_medial_search", true);


I'm still very concern about using this key here. I'd prefer to use a new one just to be used for this case instead of reusing another key which was created for another purpose.

we shouldn't touch / reuse the old keys and only work with new ones as discussed here: #33883 (comment)

I just realized that this function is only used in the account sync operation. I believe, this time I understand @jvillafanez's concern.
@PVince81, IMHO, there is no much benefit on adding a key like userbackend.db.enable_medial_search as @jvillafanez said. I removed User/Database changes from PR.

karakayasemi · 2019-03-15T10:37:24Z

@ownclouders rebase

ownclouders · 2019-03-15T10:37:27Z

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

ownclouders · 2019-03-15T10:38:32Z

Automated rebase with GitMate.io was successful! 🎉

karakayasemi added the 3 - To Review label Mar 2, 2019

karakayasemi self-assigned this Mar 2, 2019

karakayasemi requested review from PVince81 and tomneedham March 2, 2019 21:41

karakayasemi requested review from VicDeo and jvillafanez March 6, 2019 08:16

jvillafanez reviewed Mar 6, 2019

View reviewed changes

karakayasemi force-pushed the medial-search branch 2 times, most recently from b550a54 to 7a58236 Compare March 8, 2019 15:43

karakayasemi requested a review from jvillafanez March 8, 2019 15:44

jvillafanez reviewed Mar 11, 2019

View reviewed changes

Comment thread lib/private/Group/Database.php Outdated

karakayasemi force-pushed the medial-search branch from 7a58236 to 106930e Compare March 12, 2019 15:22

karakayasemi requested a review from jvillafanez March 12, 2019 15:44

karakayasemi force-pushed the medial-search branch from 106930e to c3e7f03 Compare March 13, 2019 17:08

jvillafanez reviewed Mar 14, 2019

View reviewed changes

karakayasemi force-pushed the medial-search branch from c3e7f03 to a1b2fe7 Compare March 14, 2019 17:15

jvillafanez approved these changes Mar 15, 2019

View reviewed changes

allow admin to enable medial search on group and user

7fe2f35

ownclouders force-pushed the medial-search branch from a1b2fe7 to 7fe2f35 Compare March 15, 2019 10:38

karakayasemi added the backport-request label Mar 15, 2019

karakayasemi merged commit d6505e9 into master Mar 15, 2019

delete-merged-branch Bot deleted the medial-search branch March 15, 2019 17:19

karakayasemi mentioned this pull request Mar 15, 2019

[stable10] allow admins to enable medial search on group and user #34779

Merged

10 tasks

karakayasemi removed the backport-request label Mar 15, 2019

haribhandari07 mentioned this pull request May 2, 2019

shareAutocompletion feature updated for not listing the user in autocomplete list in webui #35133

Merged

11 tasks

lock Bot locked as resolved and limited conversation to collaborators Mar 17, 2020

Uh oh!

Conversation

karakayasemi commented Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Open tasks:

Uh oh!

codecov Bot commented Mar 2, 2019

Codecov Report

Uh oh!

codecov Bot commented Mar 2, 2019

Codecov Report

Uh oh!

codecov Bot commented Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jvillafanez Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

karakayasemi Mar 8, 2019

Choose a reason for hiding this comment

Uh oh!

PVince81 Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

jvillafanez commented Mar 6, 2019

Uh oh!

karakayasemi commented Mar 8, 2019

Uh oh!

jvillafanez commented Mar 11, 2019

Uh oh!

Uh oh!

codecov Bot commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

karakayasemi commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvillafanez commented Mar 13, 2019

Uh oh!

karakayasemi commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvillafanez Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

PVince81 Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

karakayasemi Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

karakayasemi commented Mar 15, 2019

Uh oh!

ownclouders commented Mar 15, 2019

Uh oh!

ownclouders commented Mar 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

karakayasemi commented Mar 2, 2019 •

edited

Loading

codecov Bot commented Mar 2, 2019 •

edited

Loading

codecov Bot commented Mar 12, 2019 •

edited

Loading

karakayasemi commented Mar 12, 2019 •

edited

Loading

karakayasemi commented Mar 13, 2019 •

edited

Loading