From b7f7fc7241d6e04ece6a11080bf5a72938b8280a Mon Sep 17 00:00:00 2001
From: Joas Schilling <nickvergessen@owncloud.com>
Date: Fri, 18 Mar 2016 13:22:05 +0100
Subject: [PATCH] Do not create a new job when it failed to connect atm

---
 .../backgroundjob/getsharedsecret.php         | 25 ++++++++++-------
 .../backgroundjob/requestsharedsecret.php     | 23 +++++++++-------
 .../backgroundjob/getsharedsecrettest.php     | 27 ++++++++++++++-----
 .../backgroundjob/requestsharedsecrettest.php | 26 +++++++++++++-----
 4 files changed, 68 insertions(+), 33 deletions(-)

diff --git a/apps/federation/backgroundjob/getsharedsecret.php b/apps/federation/backgroundjob/getsharedsecret.php
index d3dd00c74a..66ab082c1a 100644
--- a/apps/federation/backgroundjob/getsharedsecret.php
+++ b/apps/federation/backgroundjob/getsharedsecret.php
@@ -25,12 +25,13 @@ namespace OCA\Federation\BackgroundJob;
 
 use GuzzleHttp\Exception\ClientException;
 use OC\BackgroundJob\JobList;
-use OC\BackgroundJob\QueuedJob;
+use OC\BackgroundJob\Job;
 use OCA\Federation\DbHandler;
 use OCA\Federation\TrustedServers;
 use OCP\AppFramework\Http;
 use OCP\BackgroundJob\IJobList;
 use OCP\Http\Client\IClient;
+use OCP\Http\Client\IResponse;
 use OCP\ILogger;
 use OCP\IURLGenerator;
 
@@ -41,7 +42,7 @@ use OCP\IURLGenerator;
  *
  * @package OCA\Federation\Backgroundjob
  */
-class GetSharedSecret extends QueuedJob{
+class GetSharedSecret extends Job{
 
 	/** @var IClient */
 	private $httpClient;
@@ -61,6 +62,9 @@ class GetSharedSecret extends QueuedJob{
 	/** @var ILogger */
 	private $logger;
 
+	/** @var bool */
+	protected $retainJob = false;
+
 	private $endPoint = '/ocs/v2.php/apps/federation/api/v1/shared-secret?format=json';
 
 	/**
@@ -79,7 +83,7 @@ class GetSharedSecret extends QueuedJob{
 		IJobList $jobList = null,
 		TrustedServers $trustedServers = null,
 		ILogger $logger = null,
-		dbHandler $dbHandler = null
+		DbHandler $dbHandler = null
 	) {
 		$this->logger = $logger ? $logger : \OC::$server->getLogger();
 		$this->httpClient = $httpClient ? $httpClient : \OC::$server->getHTTPClientService()->newClient();
@@ -108,12 +112,15 @@ class GetSharedSecret extends QueuedJob{
 	 * @param ILogger $logger
 	 */
 	public function execute($jobList, ILogger $logger = null) {
-		$jobList->remove($this, $this->argument);
 		$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);
 		}
+
+		if (!$this->retainJob) {
+			$jobList->remove($this, $this->argument);
+		}
 	}
 
 	/**
@@ -132,6 +139,7 @@ class GetSharedSecret extends QueuedJob{
 		$source = rtrim($source, '/');
 		$token = $argument['token'];
 
+		$result = null;
 		try {
 			$result = $this->httpClient->get(
 				$target . $this->endPoint,
@@ -156,7 +164,7 @@ class GetSharedSecret extends QueuedJob{
 				$this->logger->logException($e, ['app' => 'federation']);
 			}
 		} catch (\Exception $e) {
-			$status = HTTP::STATUS_INTERNAL_SERVER_ERROR;
+			$status = Http::STATUS_INTERNAL_SERVER_ERROR;
 			$this->logger->logException($e, ['app' => 'federation']);
 		}
 
@@ -165,16 +173,13 @@ class GetSharedSecret extends QueuedJob{
 			$status !== Http::STATUS_OK
 			&& $status !== Http::STATUS_FORBIDDEN
 		) {
-			$this->jobList->add(
-				'OCA\Federation\BackgroundJob\GetSharedSecret',
-				$argument
-			);
+			$this->retainJob = true;
 		}  else {
 			// reset token if we received a valid response
 			$this->dbHandler->addToken($target, '');
 		}
 
-		if ($status === Http::STATUS_OK) {
+		if ($status === Http::STATUS_OK && $result instanceof IResponse) {
 			$body = $result->getBody();
 			$result = json_decode($body, true);
 			if (isset($result['ocs']['data']['sharedSecret'])) {
diff --git a/apps/federation/backgroundjob/requestsharedsecret.php b/apps/federation/backgroundjob/requestsharedsecret.php
index 79b55fe4ee..040e8e1d8e 100644
--- a/apps/federation/backgroundjob/requestsharedsecret.php
+++ b/apps/federation/backgroundjob/requestsharedsecret.php
@@ -27,7 +27,7 @@ namespace OCA\Federation\BackgroundJob;
 
 use GuzzleHttp\Exception\ClientException;
 use OC\BackgroundJob\JobList;
-use OC\BackgroundJob\QueuedJob;
+use OC\BackgroundJob\Job;
 use OCA\Federation\DbHandler;
 use OCA\Federation\TrustedServers;
 use OCP\AppFramework\Http;
@@ -43,7 +43,7 @@ use OCP\IURLGenerator;
  *
  * @package OCA\Federation\Backgroundjob
  */
-class RequestSharedSecret extends QueuedJob {
+class RequestSharedSecret extends Job {
 
 	/** @var IClient */
 	private $httpClient;
@@ -65,6 +65,9 @@ class RequestSharedSecret extends QueuedJob {
 	/** @var ILogger */
 	private $logger;
 
+	/** @var bool */
+	protected $retainJob = false;
+
 	/**
 	 * RequestSharedSecret constructor.
 	 *
@@ -79,7 +82,7 @@ class RequestSharedSecret extends QueuedJob {
 		IURLGenerator $urlGenerator = null,
 		IJobList $jobList = null,
 		TrustedServers $trustedServers = null,
-		dbHandler $dbHandler = null
+		DbHandler $dbHandler = null
 	) {
 		$this->httpClient = $httpClient ? $httpClient : \OC::$server->getHTTPClientService()->newClient();
 		$this->jobList = $jobList ? $jobList : \OC::$server->getJobList();
@@ -109,15 +112,20 @@ class RequestSharedSecret extends QueuedJob {
 	 * @param ILogger $logger
 	 */
 	public function execute($jobList, ILogger $logger = null) {
-		$jobList->remove($this, $this->argument);
 		$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);
 		}
+
+		if (!$this->retainJob) {
+			$jobList->remove($this, $this->argument);
+		}
 	}
 
 	/**
+	 * call execute() method of parent
+	 *
 	 * @param JobList $jobList
 	 * @param ILogger $logger
 	 */
@@ -155,7 +163,7 @@ class RequestSharedSecret extends QueuedJob {
 				$this->logger->logException($e, ['app' => 'federation']);
 			}
 		} catch (\Exception $e) {
-			$status = HTTP::STATUS_INTERNAL_SERVER_ERROR;
+			$status = Http::STATUS_INTERNAL_SERVER_ERROR;
 			$this->logger->logException($e, ['app' => 'federation']);
 		}
 
@@ -164,10 +172,7 @@ class RequestSharedSecret extends QueuedJob {
 			$status !== Http::STATUS_OK
 			&& $status !== Http::STATUS_FORBIDDEN
 		) {
-			$this->jobList->add(
-				'OCA\Federation\BackgroundJob\RequestSharedSecret',
-				$argument
-			);
+			$this->retainJob = true;
 		}
 
 		if ($status === Http::STATUS_FORBIDDEN) {
diff --git a/apps/federation/tests/backgroundjob/getsharedsecrettest.php b/apps/federation/tests/backgroundjob/getsharedsecrettest.php
index 08c8677415..25f7502741 100644
--- a/apps/federation/tests/backgroundjob/getsharedsecrettest.php
+++ b/apps/federation/tests/backgroundjob/getsharedsecrettest.php
@@ -95,8 +95,9 @@ class GetSharedSecretTest extends TestCase {
 	 * @dataProvider dataTestExecute
 	 *
 	 * @param bool $isTrustedServer
+	 * @param bool $retainBackgroundJob
 	 */
-	public function testExecute($isTrustedServer) {
+	public function testExecute($isTrustedServer, $retainBackgroundJob) {
 		/** @var GetSharedSecret |\PHPUnit_Framework_MockObject_MockObject $getSharedSecret */
 		$getSharedSecret = $this->getMockBuilder('OCA\Federation\BackgroundJob\GetSharedSecret')
 			->setConstructorArgs(
@@ -111,7 +112,6 @@ class GetSharedSecretTest extends TestCase {
 			)->setMethods(['parentExecute'])->getMock();
 		$this->invokePrivate($getSharedSecret, 'argument', [['url' => 'url']]);
 
-		$this->jobList->expects($this->once())->method('remove');
 		$this->trustedServers->expects($this->once())->method('isTrustedServer')
 			->with('url')->willReturn($isTrustedServer);
 		if ($isTrustedServer) {
@@ -119,6 +119,12 @@ class GetSharedSecretTest extends TestCase {
 		} else {
 			$getSharedSecret->expects($this->never())->method('parentExecute');
 		}
+		$this->invokePrivate($getSharedSecret, 'retainJob', [$retainBackgroundJob]);
+		if ($retainBackgroundJob) {
+			$this->jobList->expects($this->never())->method('remove');
+		} else {
+			$this->jobList->expects($this->once())->method('remove');
+		}
 
 		$getSharedSecret->execute($this->jobList);
 
@@ -126,8 +132,9 @@ class GetSharedSecretTest extends TestCase {
 
 	public function dataTestExecute() {
 		return [
-			[true],
-			[false]
+			[true, true],
+			[true, false],
+			[false, false],
 		];
 	}
 
@@ -167,12 +174,9 @@ class GetSharedSecretTest extends TestCase {
 			$statusCode !== Http::STATUS_OK
 			&& $statusCode !== Http::STATUS_FORBIDDEN
 		) {
-			$this->jobList->expects($this->once())->method('add')
-				->with('OCA\Federation\BackgroundJob\GetSharedSecret', $argument);
 			$this->dbHandler->expects($this->never())->method('addToken');
 		}  else {
 			$this->dbHandler->expects($this->once())->method('addToken')->with($target, '');
-			$this->jobList->expects($this->never())->method('add');
 		}
 
 		if ($statusCode === Http::STATUS_OK) {
@@ -185,6 +189,15 @@ class GetSharedSecretTest extends TestCase {
 		}
 
 		$this->invokePrivate($this->getSharedSecret, 'run', [$argument]);
+		if (
+			$statusCode !== Http::STATUS_OK
+			&& $statusCode !== Http::STATUS_FORBIDDEN
+		) {
+			$this->assertTrue($this->invokePrivate($this->getSharedSecret, 'retainJob'));
+		} else {
+			$this->assertFalse($this->invokePrivate($this->getSharedSecret, 'retainJob'));
+		}
+
 	}
 
 	public function dataTestRun() {
diff --git a/apps/federation/tests/backgroundjob/requestsharedsecrettest.php b/apps/federation/tests/backgroundjob/requestsharedsecrettest.php
index 45f79e0524..5b4a1f87a5 100644
--- a/apps/federation/tests/backgroundjob/requestsharedsecrettest.php
+++ b/apps/federation/tests/backgroundjob/requestsharedsecrettest.php
@@ -75,8 +75,9 @@ class RequestSharedSecretTest extends TestCase {
 	 * @dataProvider dataTestExecute
 	 *
 	 * @param bool $isTrustedServer
+	 * @param bool $retainBackgroundJob
 	 */
-	public function testExecute($isTrustedServer) {
+	public function testExecute($isTrustedServer, $retainBackgroundJob) {
 		/** @var RequestSharedSecret |\PHPUnit_Framework_MockObject_MockObject $requestSharedSecret */
 		$requestSharedSecret = $this->getMockBuilder('OCA\Federation\BackgroundJob\RequestSharedSecret')
 			->setConstructorArgs(
@@ -90,7 +91,6 @@ class RequestSharedSecretTest extends TestCase {
 			)->setMethods(['parentExecute'])->getMock();
 		$this->invokePrivate($requestSharedSecret, 'argument', [['url' => 'url']]);
 
-		$this->jobList->expects($this->once())->method('remove');
 		$this->trustedServers->expects($this->once())->method('isTrustedServer')
 			->with('url')->willReturn($isTrustedServer);
 		if ($isTrustedServer) {
@@ -98,6 +98,12 @@ class RequestSharedSecretTest extends TestCase {
 		} else {
 			$requestSharedSecret->expects($this->never())->method('parentExecute');
 		}
+		$this->invokePrivate($requestSharedSecret, 'retainJob', [$retainBackgroundJob]);
+		if ($retainBackgroundJob) {
+			$this->jobList->expects($this->never())->method('remove');
+		} else {
+			$this->jobList->expects($this->once())->method('remove');
+		}
 
 		$requestSharedSecret->execute($this->jobList);
 
@@ -105,8 +111,9 @@ class RequestSharedSecretTest extends TestCase {
 
 	public function dataTestExecute() {
 		return [
-			[true],
-			[false]
+			[true, true],
+			[true, false],
+			[false, false],
 		];
 	}
 
@@ -146,17 +153,22 @@ class RequestSharedSecretTest extends TestCase {
 			$statusCode !== Http::STATUS_OK
 			&& $statusCode !== Http::STATUS_FORBIDDEN
 		) {
-			$this->jobList->expects($this->once())->method('add')
-				->with('OCA\Federation\BackgroundJob\RequestSharedSecret', $argument);
 			$this->dbHandler->expects($this->never())->method('addToken');
 		}
 
 		if ($statusCode === Http::STATUS_FORBIDDEN) {
-			$this->jobList->expects($this->never())->method('add');
 			$this->dbHandler->expects($this->once())->method('addToken')->with($target, '');
 		}
 
 		$this->invokePrivate($this->requestSharedSecret, 'run', [$argument]);
+		if (
+			$statusCode !== Http::STATUS_OK
+			&& $statusCode !== Http::STATUS_FORBIDDEN
+		) {
+			$this->assertTrue($this->invokePrivate($this->requestSharedSecret, 'retainJob'));
+		} else {
+			$this->assertFalse($this->invokePrivate($this->requestSharedSecret, 'retainJob'));
+		}
 	}
 
 	public function dataTestRun() {