Skip to content

Commit

Permalink
Merge pull request #34427 from owncloud/stable10-fix-30190
Browse files Browse the repository at this point in the history 
[stable10]  Refactor infoparser
  • Loading branch information
Vincent Petry committed Feb 11, 2019
2 parents 4341b3d + f76f9cf commit 6506e5f
Show file tree
Hide file tree
Showing 10 changed files with 170 additions and 45 deletions.
15 changes: 12 additions & 3 deletions core/Command/App/CheckCode.php
View file
Expand Up @@ -28,6 +28,7 @@
use OC\App\CodeChecker\EmptyCheck;
use OC\App\CodeChecker\InfoChecker;
use OC\App\InfoParser;
use OCP\App\IAppManager;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputArgument;
use Symfony\Component\Console\Input\InputInterface;
Expand All @@ -39,15 +40,19 @@ class CheckCode extends Command {
/** @var InfoParser */
private $infoParser;

/** @var IAppManager */
private $appManager;

protected $checkers = [
'private' => '\OC\App\CodeChecker\PrivateCheck',
'deprecation' => '\OC\App\CodeChecker\DeprecationCheck',
'strong-comparison' => '\OC\App\CodeChecker\StrongComparisonCheck',
];

public function __construct(InfoParser $infoParser) {
public function __construct(InfoParser $infoParser, IAppManager $appManager) {
parent::__construct();
$this->infoParser = $infoParser;
$this->appManager = $appManager;
}

protected function configure() {
Expand Down Expand Up @@ -117,7 +122,11 @@ protected function execute(InputInterface $input, OutputInterface $output) {
$errors = $codeChecker->analyse($appId);

if (!$input->getOption('skip-validate-info')) {
$infoChecker = new InfoChecker($this->infoParser);
$infoChecker = new InfoChecker($this->infoParser, $this->appManager);

$infoChecker->listen('InfoChecker', 'invalidAppInfo', function ($appId) use ($output) {
$output->writeln("<error>$appId has invalid XML in appinfo.xml</error>");
});

$infoChecker->listen('InfoChecker', 'mandatoryFieldMissing', function ($key) use ($output) {
$output->writeln("<error>Mandatory field missing: $key</error>");
Expand Down Expand Up @@ -186,7 +195,7 @@ protected function execute(InputInterface $input, OutputInterface $output) {
* @param $output
*/
private function analyseUpdateFile($appId, OutputInterface $output) {
$appPath = \OC_App::getAppPath($appId);
$appPath = $this->appManager->getAppPath($appId);
if ($appPath === false) {
throw new \RuntimeException("No app with given id <$appId> known.");
}
Expand Down
6 changes: 4 additions & 2 deletions core/register_command.php
View file
Expand Up @@ -35,9 +35,11 @@
/** @var $application Symfony\Component\Console\Application */
$application->add(new OC\Core\Command\Status);
$application->add(new OC\Core\Command\Check(\OC::$server->getConfig()));
$infoParser = new \OC\App\InfoParser();
$application->add(new OC\Core\Command\App\CheckCode($infoParser));
$application->add(new OC\Core\Command\L10n\CreateJs());
$application->add(new OC\Core\Command\App\CheckCode(
new \OC\App\InfoParser(),
\OC::$server->getAppManager()
));
$application->add(new \OC\Core\Command\Integrity\SignApp(
\OC::$server->getIntegrityCodeChecker(),
new \OC\IntegrityCheck\Helpers\FileAccessHelper(),
Expand Down
6 changes: 5 additions & 1 deletion lib/private/App/AppManager.php
View file
Expand Up @@ -576,7 +576,11 @@ protected function findAppInDirectories($appId) {

$versionToLoad = [];
foreach ($possibleAppRoots as $possibleAppRoot) {
$version = $this->getAppVersionByPath($possibleAppRoot['path'] . '/' . $appId);
try {
$version = $this->getAppVersionByPath($possibleAppRoot['path'] . '/' . $appId);
} catch (\Exception $e) {
continue;
}
if (empty($versionToLoad) || \version_compare($version, $versionToLoad['version'], '>')) {
$versionToLoad = \array_merge($possibleAppRoot, ['version' => $version]);
$versionToLoad['path'] .= '/' . $appId;
Expand Down
21 changes: 18 additions & 3 deletions lib/private/App/CodeChecker/InfoChecker.php
View file
Expand Up @@ -24,12 +24,16 @@

use OC\App\InfoParser;
use OC\Hooks\BasicEmitter;
use OCP\App\IAppManager;

class InfoChecker extends BasicEmitter {

/** @var InfoParser */
private $infoParser;

/** @var IAppManager */
private $appManager;

private $mandatoryFields = [
'author',
'description',
Expand Down Expand Up @@ -61,23 +65,34 @@ class InfoChecker extends BasicEmitter {
'standalone',
];

public function __construct(InfoParser $infoParser) {
public function __construct(InfoParser $infoParser, IAppManager $appManager) {
$this->infoParser = $infoParser;
$this->appManager = $appManager;
}

/**
* @param string $appId
* @return array
*/
public function analyse($appId) {
$appPath = \OC_App::getAppPath($appId);
$appPath = $this->appManager->getAppPath($appId);
if ($appPath === false) {
throw new \RuntimeException("No app with given id <$appId> known.");
}

$errors = [];

$info = $this->infoParser->parse($appPath . '/appinfo/info.xml');
try {
$info = $this->infoParser->parse($appPath . '/appinfo/info.xml');
} catch (\Exception $e) {
$this->emit('InfoChecker', 'invalidAppInfo', [$appId]);
return [
[
'type' => 'invalidAppInfo',
'message' => "App <$appId> has invalid XML in appinfo.xml",
]
];
}

if (isset($info['dependencies']['owncloud']['@attributes']['min-version']) && ($info['requiremin'] || $info['require'])) {
$this->emit('InfoChecker', 'duplicateRequirement', ['min']);
Expand Down
23 changes: 15 additions & 8 deletions lib/private/App/InfoParser.php
View file
Expand Up @@ -25,15 +25,22 @@

namespace OC\App;

use InvalidArgumentException;
use OCP\App\AppNotFoundException;

class InfoParser {

/**
* @param string $file the xml file to be loaded
* @return null|array where null is an indicator for an error
* @return array
* @throws AppNotFoundException if file does not exist
* @throws InvalidArgumentException on malformed XML
*/
public function parse($file) {
if (!\file_exists($file)) {
return null;
if (!\is_file($file)) {
throw new AppNotFoundException(
\sprintf('%s does not exist', $file)
);
}

\libxml_use_internal_errors(true);
Expand All @@ -43,12 +50,12 @@ public function parse($file) {
\libxml_disable_entity_loader($loadEntities);
if ($xml === false) {
\libxml_clear_errors();
return null;
throw new InvalidArgumentException('Invalid XML');
}
$array = $this->xmlToArray($xml);

if ($array === null) {
return null;
if (!\is_array($array)) {
throw new InvalidArgumentException('Could not convert XML to array');
}

if (!\array_key_exists('info', $array)) {
Expand Down Expand Up @@ -129,9 +136,9 @@ public function parse($file) {

/**
* @param \SimpleXMLElement $xml
* @return array
* @return array|string
*/
public function xmlToArray($xml) {
protected function xmlToArray($xml) {
if (!$xml->children()) {
return (string)$xml;
}
Expand Down
7 changes: 6 additions & 1 deletion lib/private/legacy/app.php
View file
Expand Up @@ -642,7 +642,12 @@ public static function getAppInfo($appId, $path = false) {
}

$parser = new InfoParser();
$data = $parser->parse($file);
try {
$data = $parser->parse($file);
} catch (\Exception $e) {
\OC::$server->getLogger()->logException($e);
throw $e;
}

if (\is_array($data)) {
$data = OC_App::parseAppInfo($data);
Expand Down
48 changes: 48 additions & 0 deletions tests/Core/Command/Apps/CheckCodeTest.php
View file
@@ -0,0 +1,48 @@
<?php
/**
* @author Viktar Dubiniuk <dubiniuk@owncloud.com>
*
* @copyright Copyright (c) 2018, ownCloud GmbH
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/

namespace Tests\Core\Command\Config;

use OCP\App\IAppManager;
use OC\App\InfoParser;
use OC\Core\Command\App\CheckCode;
use OCP\App\AppNotFoundException;
use Symfony\Component\Console\Tester\CommandTester;
use Test\TestCase;

/**
* Class CheckCodeTest
*
* @group DB
*/
class CheckCodeTest extends TestCase {
/** @var CommandTester */
private $commandTester;

/**
* @expectedException \RuntimeException
*/
public function testWrongAppId() {
$command = new CheckCode(new InfoParser, \OC::$server->getAppManager());
$this->commandTester = new CommandTester($command);
$this->commandTester->execute(['app-id' => 'hui-buh']);
}
}
2 changes: 2 additions & 0 deletions tests/data/app/invalid-info2.xml
View file
@@ -0,0 +1,2 @@
<?xml version="1.0"?>
<info>surprise</info>
54 changes: 38 additions & 16 deletions tests/lib/App/CodeChecker/InfoCheckerTest.php
View file
Expand Up @@ -21,30 +21,33 @@

namespace Test\App\CodeChecker;

use OCP\App\AppNotFoundException;
use OCP\App\IAppManager;
use OC\App\CodeChecker\InfoChecker;
use OC\App\InfoParser;
use Test\TestCase;

class InfoCheckerTest extends TestCase {
/** @var InfoChecker */
protected $infoChecker;

public static function setUpBeforeClass() {
\OC::$APPSROOTS[] = [
'path' => \OC::$SERVERROOT . '/tests/apps',
'url' => '/apps-test',
'writable' => false,
];
}

public static function tearDownAfterClass() {
// remove last element
\array_pop(\OC::$APPSROOTS);
}
/** @var IAppManager | \PHPUnit_Framework_MockObject_MockObject */
protected $appManager;

protected function setUp() {
parent::setUp();
$this->infoChecker = new InfoChecker(new InfoParser());

$this->appManager = $this->getMockBuilder(IAppManager::class)
->disableOriginalConstructor()
->getMock();

$this->appManager->expects($this->any())
->method('getAppPath')
->will(
$this->returnCallback(
function ($appId) {
return \OC::$SERVERROOT . '/tests/apps/' . $appId;
}
)
);
}

public function appInfoData() {
Expand All @@ -65,8 +68,27 @@ public function appInfoData() {
* @param $expectedErrors
*/
public function testApps($appId, $expectedErrors) {
$errors = $this->infoChecker->analyse($appId);
$infoChecker = $this->getInfoChecker(new InfoParser());
$errors = $infoChecker->analyse($appId);

$this->assertEquals($expectedErrors, $errors);
}

public function testInvalidAppInfo() {
$infoParser = $this->getMockBuilder(InfoParser::class)
->getMock();
$infoParser->expects($this->any())
->method('parse')
->will($this->throwException(new AppNotFoundException()));

$infoChecker = $this->getInfoChecker($infoParser);
$errors = $infoChecker->analyse('testapp-infoxml');

$this->assertArrayHasKey('type', $errors[0]);
$this->assertEquals('invalidAppInfo', $errors[0]['type']);
}

private function getInfoChecker($infoParser) {
return new InfoChecker($infoParser, $this->appManager);
}
}
33 changes: 22 additions & 11 deletions tests/lib/App/InfoParserTest.php
View file
Expand Up @@ -22,23 +22,34 @@ public function setUp() {
$this->parser = new InfoParser();
}

public function testParsingValidXml() {
$expectedData = \json_decode(
\file_get_contents(OC::$SERVERROOT . "/tests/data/app/expected-info.json"),
true
);
$data = $this->parser->parse(OC::$SERVERROOT. "/tests/data/app/valid-info.xml");
$this->assertEquals($expectedData, $data);
}

/**
* @dataProvider providesInfoXml
* @expectedException \OCP\App\AppNotFoundException
*/
public function testParsingValidXml($expectedJson, $xmlFile) {
$expectedData = null;
if ($expectedJson !== null) {
$expectedData = \json_decode(\file_get_contents(OC::$SERVERROOT . "/tests/data/app/$expectedJson"), true);
}
$data = $this->parser->parse(OC::$SERVERROOT. "/tests/data/app/$xmlFile");
public function testParsingMissingXml() {
$this->parser->parse('none');
}

$this->assertEquals($expectedData, $data);
/**
* @dataProvider invalidXmlProvider
* @expectedException \InvalidArgumentException
*/
public function testParsingInvalidXml($xmlFile) {
$this->parser->parse(OC::$SERVERROOT. '/tests/data/app/' . $xmlFile);
}

public function providesInfoXml() {
public function invalidXmlProvider() {
return [
['expected-info.json', 'valid-info.xml'],
[null, 'invalid-info.xml'],
['invalid-info.xml'],
['invalid-info2.xml']
];
}
}

0 comments on commit 6506e5f

Please sign in to comment.