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.

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

Jump to bottom

Migrate federation application to LoggerInterface #39018

Merged
merged 1 commit into from
Jun 29, 2023
Merged

Migrate federation application to LoggerInterface #39018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
89 changes: 28 additions & 61 deletions apps/federation/lib/BackgroundJob/RequestSharedSecret.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2016, ownCloud, Inc.
*
* @author Bjoern Schiessle <[email protected]>
* @author Björn Schießle <[email protected]>
* @author Christoph Wurst <[email protected]>
* @author Côme Chilliet <[email protected]>
* @author Joas Schilling <[email protected]>
* @author Lukas Reschke <[email protected]>
* @author Morris Jobke <[email protected]>
Expand Down Expand Up @@ -37,9 +41,9 @@
use OCP\BackgroundJob\Job;
use OCP\Http\Client\IClient;
use OCP\Http\Client\IClientService;
use OCP\ILogger;
use OCP\IURLGenerator;
use OCP\OCS\IDiscoveryService;
use Psr\Log\LoggerInterface;

/**
* Class RequestSharedSecret
Expand All @@ -49,74 +53,37 @@
* @package OCA\Federation\Backgroundjob
*/
class RequestSharedSecret extends Job {
private IClient $httpClient;

/** @var IClient */
private $httpClient;

/** @var IJobList */
private $jobList;

/** @var IURLGenerator */
private $urlGenerator;

/** @var TrustedServers */
private $trustedServers;

/** @var IDiscoveryService */
private $ocsDiscoveryService;

/** @var ILogger */
private $logger;
protected bool $retainJob = false;

/** @var bool */
protected $retainJob = false;
private string $defaultEndPoint = '/ocs/v2.php/apps/federation/api/v1/request-shared-secret';

private $defaultEndPoint = '/ocs/v2.php/apps/federation/api/v1/request-shared-secret';
/** @var int 30 day = 2592000sec */
private int $maxLifespan = 2592000;

/** @var int 30 day = 2592000sec */
private $maxLifespan = 2592000;

/**
* RequestSharedSecret constructor.
*
* @param IClientService $httpClientService
* @param IURLGenerator $urlGenerator
* @param IJobList $jobList
* @param TrustedServers $trustedServers
* @param IDiscoveryService $ocsDiscoveryService
* @param ILogger $logger
* @param ITimeFactory $timeFactory
*/
public function __construct(
IClientService $httpClientService,
IURLGenerator $urlGenerator,
IJobList $jobList,
TrustedServers $trustedServers,
IDiscoveryService $ocsDiscoveryService,
ILogger $logger,
ITimeFactory $timeFactory
private IURLGenerator $urlGenerator,
private IJobList $jobList,
private TrustedServers $trustedServers,
private IDiscoveryService $ocsDiscoveryService,
private LoggerInterface $logger,
ITimeFactory $timeFactory,
) {
parent::__construct($timeFactory);
$this->httpClient = $httpClientService->newClient();
$this->jobList = $jobList;
$this->urlGenerator = $urlGenerator;
$this->logger = $logger;
$this->ocsDiscoveryService = $ocsDiscoveryService;
$this->trustedServers = $trustedServers;
}


/**
* run the job, then remove it from the joblist
*
* @param IJobList $jobList
* @param ILogger|null $logger
*/
public function execute(IJobList $jobList, ILogger $logger = null) {
public function start(IJobList $jobList): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a public method, just double checking to ensure you have checked for references in other parts of the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the parent class OCP\BackgroundJob\Job is migrating from execute to start since 25, I think we should even deprecate execute since it’s always a problem for migrating away from ILogger.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an issue for that? If not I could create one and deprecate it rn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I just saw that execute is already deprecated in IJob interface.
Maybe it would still be a good idea to mark it as deprecated in Job class as well.

$target = $this->argument['url'];
// only execute if target is still in the list of trusted domains
if ($this->trustedServers->isTrustedServer($target)) {
$this->parentExecute($jobList, $logger);
$this->parentStart($jobList);
}

$jobList->remove($this, $this->argument);
Expand All @@ -127,15 +94,17 @@ public function execute(IJobList $jobList, ILogger $logger = null) {
}

/**
* call execute() method of parent
*
* @param IJobList $jobList
* @param ILogger $logger
* Call start() method of parent
* Useful for unit tests
*/
protected function parentExecute($jobList, $logger) {
parent::execute($jobList, $logger);
protected function parentStart(IJobList $jobList): void {
parent::start($jobList);
}

/**
* @param array $argument
* @return void
*/
protected function run($argument) {
$target = $argument['url'];
$created = isset($argument['created']) ? (int)$argument['created'] : $this->time->getTime();
Expand Down Expand Up @@ -185,7 +154,7 @@ protected function run($argument) {
$this->logger->info('Could not connect to ' . $target, ['app' => 'federation']);
} catch (\Throwable $e) {
$status = Http::STATUS_INTERNAL_SERVER_ERROR;
$this->logger->logException($e, ['app' => 'federation']);
$this->logger->error($e->getMessage(), ['app' => 'federation', 'exception' => $e]);
}

// if we received a unexpected response we try again later
Expand All @@ -199,10 +168,8 @@ protected function run($argument) {

/**
* re-add background job
*
* @param array $argument
*/
protected function reAddJob(array $argument) {
protected function reAddJob(array $argument): void {
$url = $argument['url'];
$created = isset($argument['created']) ? (int)$argument['created'] : $this->time->getTime();
$token = $argument['token'];
Expand Down
40 changes: 20 additions & 20 deletions apps/federation/tests/BackgroundJob/RequestSharedSecretTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,38 +34,38 @@
use OCP\Http\Client\IClient;
use OCP\Http\Client\IClientService;
use OCP\Http\Client\IResponse;
use OCP\ILogger;
use OCP\IURLGenerator;
use OCP\OCS\IDiscoveryService;
use PHPUnit\Framework\MockObject\MockObject;
use Psr\Log\LoggerInterface;
use Test\TestCase;

class RequestSharedSecretTest extends TestCase {

/** @var \PHPUnit\Framework\MockObject\MockObject|IClientService */
/** @var MockObject|IClientService */
private $httpClientService;

/** @var \PHPUnit\Framework\MockObject\MockObject|IClient */
/** @var MockObject|IClient */
private $httpClient;

/** @var \PHPUnit\Framework\MockObject\MockObject|IJobList */
/** @var MockObject|IJobList */
private $jobList;

/** @var \PHPUnit\Framework\MockObject\MockObject|IURLGenerator */
/** @var MockObject|IURLGenerator */
private $urlGenerator;

/** @var \PHPUnit\Framework\MockObject\MockObject|TrustedServers */
/** @var MockObject|TrustedServers */
private $trustedServers;

/** @var \PHPUnit\Framework\MockObject\MockObject|IResponse */
/** @var MockObject|IResponse */
private $response;

/** @var \PHPUnit\Framework\MockObject\MockObject|IDiscoveryService */
/** @var MockObject|IDiscoveryService */
private $discoveryService;

/** @var \PHPUnit\Framework\MockObject\MockObject|ILogger */
/** @var MockObject|LoggerInterface */
private $logger;

/** @var \PHPUnit\Framework\MockObject\MockObject|ITimeFactory */
/** @var MockObject|ITimeFactory */
private $timeFactory;

/** @var RequestSharedSecret */
Expand All @@ -82,7 +82,7 @@ protected function setUp(): void {
->disableOriginalConstructor()->getMock();
$this->response = $this->getMockBuilder(IResponse::class)->getMock();
$this->discoveryService = $this->getMockBuilder(IDiscoveryService::class)->getMock();
$this->logger = $this->createMock(ILogger::class);
$this->logger = $this->createMock(LoggerInterface::class);
$this->timeFactory = $this->createMock(ITimeFactory::class);

$this->discoveryService->expects($this->any())->method('discover')->willReturn([]);
Expand All @@ -100,13 +100,13 @@ protected function setUp(): void {
}

/**
* @dataProvider dataTestExecute
* @dataProvider dataTestStart
*
* @param bool $isTrustedServer
* @param bool $retainBackgroundJob
*/
public function testExecute($isTrustedServer, $retainBackgroundJob) {
/** @var RequestSharedSecret |\PHPUnit\Framework\MockObject\MockObject $requestSharedSecret */
public function testStart($isTrustedServer, $retainBackgroundJob) {
/** @var RequestSharedSecret |MockObject $requestSharedSecret */
$requestSharedSecret = $this->getMockBuilder('OCA\Federation\BackgroundJob\RequestSharedSecret')
->setConstructorArgs(
[
Expand All @@ -118,15 +118,15 @@ public function testExecute($isTrustedServer, $retainBackgroundJob) {
$this->logger,
$this->timeFactory
]
)->setMethods(['parentExecute'])->getMock();
)->setMethods(['parentStart'])->getMock();
$this->invokePrivate($requestSharedSecret, 'argument', [['url' => 'url', 'token' => 'token']]);

$this->trustedServers->expects($this->once())->method('isTrustedServer')
->with('url')->willReturn($isTrustedServer);
if ($isTrustedServer) {
$requestSharedSecret->expects($this->once())->method('parentExecute');
$requestSharedSecret->expects($this->once())->method('parentStart');
} else {
$requestSharedSecret->expects($this->never())->method('parentExecute');
$requestSharedSecret->expects($this->never())->method('parentStart');
}
$this->invokePrivate($requestSharedSecret, 'retainJob', [$retainBackgroundJob]);
$this->jobList->expects($this->once())->method('remove');
Expand All @@ -148,10 +148,10 @@ public function testExecute($isTrustedServer, $retainBackgroundJob) {
$this->jobList->expects($this->never())->method('add');
}

$requestSharedSecret->execute($this->jobList);
$requestSharedSecret->start($this->jobList);
}

public function dataTestExecute() {
public function dataTestStart() {
return [
[true, true],
[true, false],
Expand Down