From 303fce44f487e50a09910acdb28bc6c99b4b04b8 Mon Sep 17 00:00:00 2001 From: Victor Dubiniuk Date: Tue, 25 Nov 2014 22:41:15 +0300 Subject: [PATCH 1/5] Use httphelper and cache response even when it empty --- core/ajax/update.php | 5 ++++- core/command/upgrade.php | 2 +- lib/private/templatelayout.php | 2 +- lib/private/updater.php | 40 +++++++++++++++++----------------- 4 files changed, 26 insertions(+), 23 deletions(-) diff --git a/core/ajax/update.php b/core/ajax/update.php index 419992c9891..85d5dc83ccf 100644 --- a/core/ajax/update.php +++ b/core/ajax/update.php @@ -9,7 +9,10 @@ if (OC::checkUpgrade(false)) { $l = new \OC_L10N('core'); $eventSource = \OC::$server->createEventSource(); - $updater = new \OC\Updater(\OC_Log::$object); + $updater = new \OC\Updater( + \OC::$server->getHTTPHelper(), + \OC_Log::$object + ); $updater->listen('\OC\Updater', 'maintenanceStart', function () use ($eventSource, $l) { $eventSource->send('success', (string)$l->t('Turned on maintenance mode')); }); diff --git a/core/command/upgrade.php b/core/command/upgrade.php index aaeb63a3124..1d74ad0a90e 100644 --- a/core/command/upgrade.php +++ b/core/command/upgrade.php @@ -84,7 +84,7 @@ class Upgrade extends Command { if(\OC::checkUpgrade(false)) { $self = $this; - $updater = new Updater(); + $updater = new Updater(\OC::$server->getHTTPHelper()); $updater->setSimulateStepEnabled($simulateStepEnabled); $updater->setUpdateStepEnabled($updateStepEnabled); diff --git a/lib/private/templatelayout.php b/lib/private/templatelayout.php index a066f90bb23..aefb93ec266 100644 --- a/lib/private/templatelayout.php +++ b/lib/private/templatelayout.php @@ -44,7 +44,7 @@ class OC_TemplateLayout extends OC_Template { // Update notification if($this->config->getSystemValue('updatechecker', true) === true && OC_User::isAdminUser(OC_User::getUser())) { - $updater = new \OC\Updater(); + $updater = new \OC\Updater(\OC::$server->getHTTPHelper()); $data = $updater->check(); if(isset($data['version']) && $data['version'] != '' and $data['version'] !== Array()) { diff --git a/lib/private/updater.php b/lib/private/updater.php index e07ff03ffc4..5846a6a655a 100644 --- a/lib/private/updater.php +++ b/lib/private/updater.php @@ -25,6 +25,11 @@ class Updater extends BasicEmitter { * @var \OC\Log $log */ private $log; + + /** + * @var \OC\HTTPHelper $helper; + */ + private $httpHelper; private $simulateStepEnabled; @@ -33,7 +38,8 @@ class Updater extends BasicEmitter { /** * @param \OC\Log $log */ - public function __construct($log = null) { + public function __construct($httpHelper, $log = null) { + $this->httpHelper = $httpHelper; $this->log = $log; $this->simulateStepEnabled = true; $this->updateStepEnabled = true; @@ -95,30 +101,24 @@ class Updater extends BasicEmitter { $url = $updaterUrl . '?version=' . $versionString; // set a sensible timeout of 10 sec to stay responsive even if the update server is down. - $ctx = stream_context_create( - array( - 'http' => array( - 'timeout' => 10 - ) - ) - ); - $xml = @file_get_contents($url, 0, $ctx); - if ($xml == false) { - return array(); - } - $loadEntities = libxml_disable_entity_loader(true); - $data = @simplexml_load_string($xml); - libxml_disable_entity_loader($loadEntities); $tmp = array(); - $tmp['version'] = $data->version; - $tmp['versionstring'] = $data->versionstring; - $tmp['url'] = $data->url; - $tmp['web'] = $data->web; + $xml = $this->httpHelper->getUrlContent($url); + if ($xml !== false) { + $loadEntities = libxml_disable_entity_loader(true); + $data = @simplexml_load_string($xml); + libxml_disable_entity_loader($loadEntities); + + $tmp['version'] = $data->version; + $tmp['versionstring'] = $data->versionstring; + $tmp['url'] = $data->url; + $tmp['web'] = $data->web; + } else { + $data = array(); + } // Cache the result \OC_Appconfig::setValue('core', 'lastupdateResult', json_encode($data)); - return $tmp; } From 2a3f5ccae3f43c4a34c6aff536ded9e802d7523d Mon Sep 17 00:00:00 2001 From: Victor Dubiniuk Date: Wed, 26 Nov 2014 19:18:22 +0300 Subject: [PATCH 2/5] Test OC\Updater::check --- tests/lib/updater.php | 60 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 58 insertions(+), 2 deletions(-) diff --git a/tests/lib/updater.php b/tests/lib/updater.php index 155dccf78a7..5a05bc09bb7 100644 --- a/tests/lib/updater.php +++ b/tests/lib/updater.php @@ -26,9 +26,65 @@ class UpdaterTest extends \Test\TestCase { /** * @dataProvider versionCompatibilityTestData */ - function testIsUpgradePossible($oldVersion, $newVersion, $result) { - $updater = new Updater(); + public function testIsUpgradePossible($oldVersion, $newVersion, $result) { + $updater = new Updater(\OC::$server->getHTTPHelper()); $this->assertSame($result, $updater->isUpgradePossible($oldVersion, $newVersion)); } + + + public function testCheck(){ + $httpHelper = $this->getMockBuilder('\OC\HTTPHelper') + ->getMock(); + + $httpHelper->method('getUrlContent') + ->willReturn( + '' + ) + ; + + $updater = new Updater($httpHelper); + // Invalidate cache + \OC_Appconfig::setValue('core', 'lastupdatedat', 0); + $result = $updater->check(); + $this->assertContains('version', $result); + $this->assertContains('versionstring', $result); + $this->assertContains('url', $result); + $this->assertContains('web', $result); + $this->assertEmpty($result['version']); + $this->assertEmpty($result['versionstring']); + $this->assertEmpty($result['url']); + $this->assertEmpty($result['web']); + + // Invalidate cache + \OC_Appconfig::setValue('core', 'lastupdatedat', 0); + $httpHelper->method('getUrlContent') + ->willReturn('') + ; + + $emptyResult = $updater->check(); + $this->assertEmpty($emptyResult); + + // Invalidate cache + \OC_Appconfig::setValue('core', 'lastupdatedat', 0); + $httpHelper->method('getUrlContent') + ->willReturn(' + + 7.0.3.4 + ownCloud 7.0.3 + http://download.owncloud.org/community/owncloud-7.0.3.zip + http://owncloud.org/ +') + ; + + $newResult = $updater->check(); + $this->assertContains('version', $newResult); + $this->assertContains('versionstring', $newResult); + $this->assertContains('url', $newResult); + $this->assertContains('web', $newResult); + $this->assertEqual('7.0.3.4', $newResult['version']); + $this->assertEqual('ownCloud 7.0.3', $newResult['versionstring']); + $this->assertEqual('http://download.owncloud.org/community/owncloud-7.0.3.zip', $newResult['url']); + $this->assertEqual('http://owncloud.org/', $newResult['web']); + } } \ No newline at end of file From 81d571241991926f8820c43020a0250eb305a0b0 Mon Sep 17 00:00:00 2001 From: Victor Dubiniuk Date: Mon, 8 Dec 2014 17:38:56 +0300 Subject: [PATCH 3/5] Fix tests. Add two more test cases --- lib/private/updater.php | 13 +++--- tests/lib/updater.php | 91 +++++++++++++++++++++++------------------ 2 files changed, 59 insertions(+), 45 deletions(-) diff --git a/lib/private/updater.php b/lib/private/updater.php index 5846a6a655a..266cf9cc89f 100644 --- a/lib/private/updater.php +++ b/lib/private/updater.php @@ -104,15 +104,16 @@ class Updater extends BasicEmitter { $tmp = array(); $xml = $this->httpHelper->getUrlContent($url); - if ($xml !== false) { + if ($xml) { $loadEntities = libxml_disable_entity_loader(true); $data = @simplexml_load_string($xml); libxml_disable_entity_loader($loadEntities); - - $tmp['version'] = $data->version; - $tmp['versionstring'] = $data->versionstring; - $tmp['url'] = $data->url; - $tmp['web'] = $data->web; + if ($data !== false) { + $tmp['version'] = $data->version; + $tmp['versionstring'] = $data->versionstring; + $tmp['url'] = $data->url; + $tmp['web'] = $data->web; + } } else { $data = array(); } diff --git a/tests/lib/updater.php b/tests/lib/updater.php index 5a05bc09bb7..832f60fac06 100644 --- a/tests/lib/updater.php +++ b/tests/lib/updater.php @@ -33,58 +33,71 @@ class UpdaterTest extends \Test\TestCase { public function testCheck(){ - $httpHelper = $this->getMockBuilder('\OC\HTTPHelper') - ->getMock(); + // Valid XML. Empty values + $updater = $this->getUpdaterMock( + '' + ); + $result = array_map('strval', $updater->check()); - $httpHelper->method('getUrlContent') - ->willReturn( - '' - ) - ; - - $updater = new Updater($httpHelper); - // Invalidate cache - \OC_Appconfig::setValue('core', 'lastupdatedat', 0); - $result = $updater->check(); - $this->assertContains('version', $result); - $this->assertContains('versionstring', $result); - $this->assertContains('url', $result); - $this->assertContains('web', $result); + $this->assertArrayHasKey('version', $result); + $this->assertArrayHasKey('versionstring', $result); + $this->assertArrayHasKey('url', $result); + $this->assertArrayHasKey('web', $result); $this->assertEmpty($result['version']); $this->assertEmpty($result['versionstring']); $this->assertEmpty($result['url']); $this->assertEmpty($result['web']); - // Invalidate cache - \OC_Appconfig::setValue('core', 'lastupdatedat', 0); - $httpHelper->method('getUrlContent') - ->willReturn('') - ; - - $emptyResult = $updater->check(); + // Empty feed + $emptyUpdater = $this->getUpdaterMock(''); + $emptyResult = $emptyUpdater->check(); $this->assertEmpty($emptyResult); - - // Invalidate cache - \OC_Appconfig::setValue('core', 'lastupdatedat', 0); - $httpHelper->method('getUrlContent') - ->willReturn(' + + // Error while fetching new contents e.g. too many redirects + $falseUpdater = $this->getUpdaterMock(false); + $falseResult = $falseUpdater->check(); + $this->assertEmpty($falseResult); + + // Valid XML. New version available + $newUpdater = $this->getUpdaterMock( + ' 7.0.3.4 ownCloud 7.0.3 http://download.owncloud.org/community/owncloud-7.0.3.zip http://owncloud.org/ -') +' + ); + $newResult = array_map('strval', $newUpdater->check()); + + $this->assertArrayHasKey('version', $newResult); + $this->assertArrayHasKey('versionstring', $newResult); + $this->assertArrayHasKey('url', $newResult); + $this->assertArrayHasKey('web', $newResult); + $this->assertEquals('7.0.3.4', $newResult['version']); + $this->assertEquals('ownCloud 7.0.3', $newResult['versionstring']); + $this->assertEquals('http://download.owncloud.org/community/owncloud-7.0.3.zip', $newResult['url']); + $this->assertEquals('http://owncloud.org/', $newResult['web']); + + // Invalid XML + $invalidUpdater = $this->getUpdaterMock('OMG!'); + $invalidResult = $invalidUpdater->check(); + $this->assertEmpty($invalidResult); + } + + protected function getUpdaterMock($content){ + // Invalidate cache + \OC_Appconfig::setValue('core', 'lastupdatedat', 0); + + $mockedHTTPHelper = $this->getMockBuilder('\OC\HTTPHelper') + ->setConstructorArgs(array(\OC::$server->getConfig())) + ->getMock() ; - $newResult = $updater->check(); - $this->assertContains('version', $newResult); - $this->assertContains('versionstring', $newResult); - $this->assertContains('url', $newResult); - $this->assertContains('web', $newResult); - $this->assertEqual('7.0.3.4', $newResult['version']); - $this->assertEqual('ownCloud 7.0.3', $newResult['versionstring']); - $this->assertEqual('http://download.owncloud.org/community/owncloud-7.0.3.zip', $newResult['url']); - $this->assertEqual('http://owncloud.org/', $newResult['web']); + $mockedHTTPHelper->method('getUrlContent') + ->willReturn($content) + ; + return new Updater($mockedHTTPHelper); } -} \ No newline at end of file +} From c9fd3c9d2960c6457440427513f04a4f338a23d5 Mon Sep 17 00:00:00 2001 From: Victor Dubiniuk Date: Wed, 10 Dec 2014 01:13:38 +0300 Subject: [PATCH 4/5] Inject config --- core/ajax/update.php | 3 ++- core/command/upgrade.php | 2 +- lib/private/templatelayout.php | 2 +- lib/private/updater.php | 26 +++++++++++-------- tests/lib/updater.php | 47 +++++++++++++++++++--------------- 5 files changed, 46 insertions(+), 34 deletions(-) diff --git a/core/ajax/update.php b/core/ajax/update.php index 85d5dc83ccf..5a9288a6381 100644 --- a/core/ajax/update.php +++ b/core/ajax/update.php @@ -10,7 +10,8 @@ if (OC::checkUpgrade(false)) { $l = new \OC_L10N('core'); $eventSource = \OC::$server->createEventSource(); $updater = new \OC\Updater( - \OC::$server->getHTTPHelper(), + \OC::$server->getHTTPHelper(), + \OC::$server->getAppConfig(), \OC_Log::$object ); $updater->listen('\OC\Updater', 'maintenanceStart', function () use ($eventSource, $l) { diff --git a/core/command/upgrade.php b/core/command/upgrade.php index 1d74ad0a90e..6e5ac1c5c9a 100644 --- a/core/command/upgrade.php +++ b/core/command/upgrade.php @@ -84,7 +84,7 @@ class Upgrade extends Command { if(\OC::checkUpgrade(false)) { $self = $this; - $updater = new Updater(\OC::$server->getHTTPHelper()); + $updater = new Updater(\OC::$server->getHTTPHelper(), \OC::$server->getAppConfig()); $updater->setSimulateStepEnabled($simulateStepEnabled); $updater->setUpdateStepEnabled($updateStepEnabled); diff --git a/lib/private/templatelayout.php b/lib/private/templatelayout.php index aefb93ec266..fa025721e53 100644 --- a/lib/private/templatelayout.php +++ b/lib/private/templatelayout.php @@ -44,7 +44,7 @@ class OC_TemplateLayout extends OC_Template { // Update notification if($this->config->getSystemValue('updatechecker', true) === true && OC_User::isAdminUser(OC_User::getUser())) { - $updater = new \OC\Updater(\OC::$server->getHTTPHelper()); + $updater = new \OC\Updater(\OC::$server->getHTTPHelper(), \OC::$server->getAppConfig()); $data = $updater->check(); if(isset($data['version']) && $data['version'] != '' and $data['version'] !== Array()) { diff --git a/lib/private/updater.php b/lib/private/updater.php index 266cf9cc89f..6272f77cfc2 100644 --- a/lib/private/updater.php +++ b/lib/private/updater.php @@ -30,6 +30,11 @@ class Updater extends BasicEmitter { * @var \OC\HTTPHelper $helper; */ private $httpHelper; + + /** + * @var \OCP\IAppConfig; + */ + private $config; private $simulateStepEnabled; @@ -38,9 +43,10 @@ class Updater extends BasicEmitter { /** * @param \OC\Log $log */ - public function __construct($httpHelper, $log = null) { + public function __construct($httpHelper, $config, $log = null) { $this->httpHelper = $httpHelper; $this->log = $log; + $this->config = $config; $this->simulateStepEnabled = true; $this->updateStepEnabled = true; } @@ -75,23 +81,23 @@ class Updater extends BasicEmitter { public function check($updaterUrl = null) { // Look up the cache - it is invalidated all 30 minutes - if ((\OC_Appconfig::getValue('core', 'lastupdatedat') + 1800) > time()) { - return json_decode(\OC_Appconfig::getValue('core', 'lastupdateResult'), true); + if (($this->config->getValue('core', 'lastupdatedat') + 1800) > time()) { + return json_decode($this->config->getValue('core', 'lastupdateResult'), true); } if (is_null($updaterUrl)) { $updaterUrl = 'https://apps.owncloud.com/updater.php'; } - \OC_Appconfig::setValue('core', 'lastupdatedat', time()); + $this->config->setValue('core', 'lastupdatedat', time()); - if (\OC_Appconfig::getValue('core', 'installedat', '') == '') { - \OC_Appconfig::setValue('core', 'installedat', microtime(true)); + if ($this->config->getValue('core', 'installedat', '') == '') { + $this->config->setValue('core', 'installedat', microtime(true)); } $version = \OC_Util::getVersion(); - $version['installed'] = \OC_Appconfig::getValue('core', 'installedat'); - $version['updated'] = \OC_Appconfig::getValue('core', 'lastupdatedat'); + $version['installed'] = $this->config->getValue('core', 'installedat'); + $version['updated'] = $this->config->getValue('core', 'lastupdatedat'); $version['updatechannel'] = \OC_Util::getChannel(); $version['edition'] = \OC_Util::getEditionString(); $version['build'] = \OC_Util::getBuild(); @@ -119,7 +125,7 @@ class Updater extends BasicEmitter { } // Cache the result - \OC_Appconfig::setValue('core', 'lastupdateResult', json_encode($data)); + $this->config->setValue('core', 'lastupdateResult', json_encode($data)); return $tmp; } @@ -219,7 +225,7 @@ class Updater extends BasicEmitter { $repair->run(); //Invalidate update feed - \OC_Appconfig::setValue('core', 'lastupdatedat', 0); + $this->config->setValue('core', 'lastupdatedat', 0); // only set the final version if everything went well \OC_Config::setValue('version', implode('.', \OC_Util::getVersion())); diff --git a/tests/lib/updater.php b/tests/lib/updater.php index 832f60fac06..dc1e9e5446f 100644 --- a/tests/lib/updater.php +++ b/tests/lib/updater.php @@ -27,13 +27,28 @@ class UpdaterTest extends \Test\TestCase { * @dataProvider versionCompatibilityTestData */ public function testIsUpgradePossible($oldVersion, $newVersion, $result) { - $updater = new Updater(\OC::$server->getHTTPHelper()); + $updater = new Updater(\OC::$server->getHTTPHelper(), \OC::$server->getConfig()); $this->assertSame($result, $updater->isUpgradePossible($oldVersion, $newVersion)); } + public function testBrokenXmlResponse(){ + $invalidUpdater = $this->getUpdaterMock('OMG!'); + $invalidResult = $invalidUpdater->check(); + $this->assertEmpty($invalidResult); + } - public function testCheck(){ - // Valid XML. Empty values + public function testEmptyResponse(){ + $emptyUpdater = $this->getUpdaterMock(''); + $emptyResult = $emptyUpdater->check(); + $this->assertEmpty($emptyResult); + + // Error while fetching new contents e.g. too many redirects + $falseUpdater = $this->getUpdaterMock(false); + $falseResult = $falseUpdater->check(); + $this->assertEmpty($falseResult); + } + + public function testValidEmptyXmlResponse(){ $updater = $this->getUpdaterMock( '' ); @@ -47,18 +62,9 @@ class UpdaterTest extends \Test\TestCase { $this->assertEmpty($result['versionstring']); $this->assertEmpty($result['url']); $this->assertEmpty($result['web']); - - // Empty feed - $emptyUpdater = $this->getUpdaterMock(''); - $emptyResult = $emptyUpdater->check(); - $this->assertEmpty($emptyResult); - - // Error while fetching new contents e.g. too many redirects - $falseUpdater = $this->getUpdaterMock(false); - $falseResult = $falseUpdater->check(); - $this->assertEmpty($falseResult); - - // Valid XML. New version available + } + + public function testValidUpdateResponse(){ $newUpdater = $this->getUpdaterMock( ' @@ -78,15 +84,14 @@ class UpdaterTest extends \Test\TestCase { $this->assertEquals('ownCloud 7.0.3', $newResult['versionstring']); $this->assertEquals('http://download.owncloud.org/community/owncloud-7.0.3.zip', $newResult['url']); $this->assertEquals('http://owncloud.org/', $newResult['web']); - - // Invalid XML - $invalidUpdater = $this->getUpdaterMock('OMG!'); - $invalidResult = $invalidUpdater->check(); - $this->assertEmpty($invalidResult); } protected function getUpdaterMock($content){ // Invalidate cache + $mockedAppConfig = $this->getMockBuilder('\OC\AppConfig') + ->disableOriginalConstructor() + ->getMock() + ; \OC_Appconfig::setValue('core', 'lastupdatedat', 0); $mockedHTTPHelper = $this->getMockBuilder('\OC\HTTPHelper') @@ -97,7 +102,7 @@ class UpdaterTest extends \Test\TestCase { $mockedHTTPHelper->method('getUrlContent') ->willReturn($content) ; - return new Updater($mockedHTTPHelper); + return new Updater($mockedHTTPHelper, $mockedAppConfig); } } From adab0ca98a9ab8f3d4f27909c6d46bb170595d20 Mon Sep 17 00:00:00 2001 From: Victor Dubiniuk Date: Wed, 10 Dec 2014 02:21:10 +0300 Subject: [PATCH 5/5] Remove leftover --- tests/lib/updater.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/lib/updater.php b/tests/lib/updater.php index dc1e9e5446f..2dab2750dcd 100644 --- a/tests/lib/updater.php +++ b/tests/lib/updater.php @@ -92,7 +92,6 @@ class UpdaterTest extends \Test\TestCase { ->disableOriginalConstructor() ->getMock() ; - \OC_Appconfig::setValue('core', 'lastupdatedat', 0); $mockedHTTPHelper = $this->getMockBuilder('\OC\HTTPHelper') ->setConstructorArgs(array(\OC::$server->getConfig()))