Skip to content

always query the lookup server in a global scale setup #11714

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.

Sign up for GitHub

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
schiessle merged 4 commits into from
Oct 15, 2018
Merged

always query the lookup server in a global scale setup #11714

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3fa13e7
always query the lookup server in a global scale setup and have a nic…
schiessle Oct 9, 2018
9e76d40
adjust tests
schiessle Oct 9, 2018
fd5fc24
some small fixes and improvements
schiessle Oct 10, 2018
1b0b159
add more tests
schiessle Oct 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions lib/private/Collaboration/Collaborators/LookupPlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@
use OCP\Collaboration\Collaborators\ISearchPlugin;
use OCP\Collaboration\Collaborators\ISearchResult;
use OCP\Collaboration\Collaborators\SearchResultType;
use OCP\Federation\ICloudIdManager;
use OCP\Http\Client\IClientService;
use OCP\IConfig;
use OCP\IUserSession;
use OCP\Share;

class LookupPlugin implements ISearchPlugin {
Expand All @@ -37,14 +39,27 @@ class LookupPlugin implements ISearchPlugin {
private $config;
/** @var IClientService */
private $clientService;
/** @var string remote part of the current user's cloud id */
private $currentUserRemote;
/** @var ICloudIdManager */
private $cloudIdManager;

public function __construct(IConfig $config, IClientService $clientService) {
public function __construct(IConfig $config,
IClientService $clientService,
IUserSession $userSession,
ICloudIdManager $cloudIdManager) {
$this->config = $config;
$this->clientService = $clientService;
$this->cloudIdManager = $cloudIdManager;
$currentUserCloudId = $userSession->getUser()->getCloudId();
$this->currentUserRemote = $cloudIdManager->resolveCloudId($currentUserCloudId)->getRemote();
}

public function search($search, $limit, $offset, ISearchResult $searchResult) {
if ($this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') !== 'yes') {
$isGlobalScaleEnabled = $this->config->getSystemValue('gs.enabled', false);
$isLookupServerEnabled = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no');
// if case of Global Scale we always search the lookup server
if ($isLookupServerEnabled !== 'yes' && !$isGlobalScaleEnabled) {
Copy link
Member

@MorrisJobke MorrisJobke Oct 10, 2018

Choose a reason for hiding this comment

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

Would be a lot more readable if the === 'yes' is added to line 60 and then the if statement only holds !$isLookupServerEnabled && !$isGlobalScaleEnabled.

Sorry for nitpicking, but readable code is worth it IMO.

Copy link
Member Author

@schiessle schiessle Oct 10, 2018

Choose a reason for hiding this comment

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

good idea... will add it.

return false;
}

Expand All @@ -65,8 +80,12 @@ public function search($search, $limit, $offset, ISearchResult $searchResult) {
$body = json_decode($response->getBody(), true);

foreach ($body as $lookup) {
$remote = $this->cloudIdManager->resolveCloudId($lookup['federationId'])->getRemote();
Copy link
Member

@nickvergessen nickvergessen Oct 10, 2018

Choose a reason for hiding this comment

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

try-catch

if ($this->currentUserRemote === $remote) continue;
Copy link
Member

@nickvergessen nickvergessen Oct 10, 2018

Choose a reason for hiding this comment

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

newline + brackets

$name = isset($lookup['name']['value']) ? $lookup['name']['value'] : '';
Copy link
Member

@nickvergessen nickvergessen Oct 10, 2018

Choose a reason for hiding this comment

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

use ?? operator, see backport

Copy link
Member Author

@schiessle schiessle Oct 10, 2018

Choose a reason for hiding this comment

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

yes, but honestly I consider the other one more readable so I will stick to it for now. All other points are addressed... Thanks for all the feedback!

$label = empty($name) ? $lookup['federationId'] : $name . ' (' . $lookup['federationId'] . ')';
$result[] = [
'label' => $lookup['federationId'],
'label' => $label,
'value' => [
'shareType' => Share::SHARE_TYPE_REMOTE,
'shareWith' => $lookup['federationId'],
Expand Down
33 changes: 31 additions & 2 deletions tests/lib/Collaboration/Collaborators/LookupPluginTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,17 @@


use OC\Collaboration\Collaborators\LookupPlugin;
use OC\Federation\CloudId;
use OCP\Collaboration\Collaborators\ISearchResult;
use OCP\Collaboration\Collaborators\SearchResultType;
use OCP\Federation\ICloudId;
use OCP\Federation\ICloudIdManager;
use OCP\Http\Client\IClient;
use OCP\Http\Client\IClientService;
use OCP\Http\Client\IResponse;
use OCP\IConfig;
use OCP\IUser;
use OCP\IUserSession;
use OCP\Share;
use Test\TestCase;

Expand All @@ -40,16 +45,36 @@ class LookupPluginTest extends TestCase {
protected $config;
/** @var IClientService|\PHPUnit_Framework_MockObject_MockObject */
protected $clientService;
/** @var IUserSession|\PHPUnit_Framework_MockObject_MockObject */
protected $userSession;
/** @var ICloudIdManager|\PHPUnit_Framework_MockObject_MockObject */
protected $cloudIdManager;
/** @var LookupPlugin */
protected $plugin;

public function setUp() {
parent::setUp();

$this->userSession = $this->createMock(IUserSession::class);
$this->cloudIdManager = $this->createMock(ICloudIdManager::class);
$this->config = $this->createMock(IConfig::class);
$this->clientService = $this->createMock(IClientService::class);
$cloudId = $this->createMock(ICloudId::class);
$cloudId->expects($this->any())->method('getRemote')->willReturn('myNextcloud.net');
$user = $this->createMock(IUser::class);
$user->expects($this->any())->method('getCloudId')->willReturn('[email protected]');
$this->userSession->expects($this->any())->method('getUser')
->willReturn($user);
$this->cloudIdManager->expects($this->any())->method('resolveCloudId')
->willReturnCallback(function($cloudId) {
if ($cloudId === '[email protected]') {
return new CloudId('[email protected]', 'user', 'myNextcloud.net');
}
return new CloudId('[email protected]', 'user', 'someNextcloud.net');
});


$this->plugin = new LookupPlugin($this->config, $this->clientService);
$this->plugin = new LookupPlugin($this->config, $this->clientService, $this->userSession, $this->cloudIdManager);
}

/**
Expand All @@ -69,7 +94,11 @@ public function testSearch(array $searchParams) {
->method('getAppValue')
->with('files_sharing', 'lookupServerEnabled', 'no')
->willReturn('yes');
$this->config->expects($this->once())
$this->config->expects($this->at(0))
->method('getSystemValue')
->with('gs.enabled', false)
Copy link
Member

@blizzz blizzz Oct 12, 2018

Choose a reason for hiding this comment

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

can we have tests with gs.enabled = true, too, please?

Copy link
Member Author

@schiessle schiessle Oct 12, 2018

Choose a reason for hiding this comment

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

This will produce the exact same results, because the lookup server is enabled in all test cases 😉 But I added another test (see last commit) which might do what you meant...

Copy link
Member

@blizzz blizzz Oct 12, 2018

Choose a reason for hiding this comment

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

thanks :)

Copy link
Member Author

@schiessle schiessle Oct 15, 2018

Choose a reason for hiding this comment

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

@blizzz does this count as 👍 😉

->willReturn(false);
$this->config->expects($this->at(2))
->method('getSystemValue')
->with('lookup_server', 'https://lookup.nextcloud.com')
->willReturn($searchParams['server']);
Expand Down