From: Jeremy Benoist Date: Fri, 19 Aug 2016 21:52:19 +0000 (+0200) Subject: Avoid breaking import when fetching fail X-Git-Tag: 2.0.7~4^2 X-Git-Url: https://git.immae.eu/?a=commitdiff_plain;h=19d9efab32b5c6403e9ee95fb70a2ce56a27f14b;p=github%2Fwallabag%2Fwallabag.git Avoid breaking import when fetching fail graby will throw an Exception in some case (like a bad url, a restricted url or a secured pdf). Import doesn't handle that case and break the whole import. With that commit the import isn't stopped but the entry is just skipped. Also, as a bonus, I've added extra test on WallabagImportV2 when the json is empty. --- diff --git a/src/Wallabag/ImportBundle/Import/AbstractImport.php b/src/Wallabag/ImportBundle/Import/AbstractImport.php new file mode 100644 index 00000000..14377a35 --- /dev/null +++ b/src/Wallabag/ImportBundle/Import/AbstractImport.php @@ -0,0 +1,47 @@ +em = $em; + $this->logger = new NullLogger(); + $this->contentProxy = $contentProxy; + } + + public function setLogger(LoggerInterface $logger) + { + $this->logger = $logger; + } + + /** + * Fetch content from the ContentProxy (using graby). + * If it fails return false instead of the updated entry. + * + * @param Entry $entry Entry to update + * @param string $url Url to grab content for + * @param array $content An array with AT LEAST keys title, html, url, language & content_type to skip the fetchContent from the url + * + * @return Entry|false + */ + protected function fetchContent(Entry $entry, $url, array $content = []) + { + try { + return $this->contentProxy->updateEntry($entry, $url, $content); + } catch (\Exception $e) { + return false; + } + } +} diff --git a/src/Wallabag/ImportBundle/Import/PocketImport.php b/src/Wallabag/ImportBundle/Import/PocketImport.php index 29361a32..678d016a 100644 --- a/src/Wallabag/ImportBundle/Import/PocketImport.php +++ b/src/Wallabag/ImportBundle/Import/PocketImport.php @@ -2,7 +2,6 @@ namespace Wallabag\ImportBundle\Import; -use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; use Doctrine\ORM\EntityManager; use GuzzleHttp\Client; @@ -12,12 +11,9 @@ use Wallabag\CoreBundle\Entity\Entry; use Wallabag\CoreBundle\Helper\ContentProxy; use Craue\ConfigBundle\Util\Config; -class PocketImport implements ImportInterface +class PocketImport extends AbstractImport { private $user; - private $em; - private $contentProxy; - private $logger; private $client; private $consumerKey; private $skippedEntries = 0; @@ -34,11 +30,6 @@ class PocketImport implements ImportInterface $this->logger = new NullLogger(); } - public function setLogger(LoggerInterface $logger) - { - $this->logger = $logger; - } - /** * {@inheritdoc} */ @@ -219,7 +210,13 @@ class PocketImport implements ImportInterface } $entry = new Entry($this->user); - $entry = $this->contentProxy->updateEntry($entry, $url); + $entry = $this->fetchContent($entry, $url); + + // jump to next entry in case of problem while getting content + if (false === $entry) { + ++$this->skippedEntries; + continue; + } // 0, 1, 2 - 1 if the item is archived - 2 if the item should be deleted if ($pocketEntry['status'] == 1 || $this->markAsRead) { diff --git a/src/Wallabag/ImportBundle/Import/WallabagImport.php b/src/Wallabag/ImportBundle/Import/WallabagImport.php index 65803823..a1cc085b 100644 --- a/src/Wallabag/ImportBundle/Import/WallabagImport.php +++ b/src/Wallabag/ImportBundle/Import/WallabagImport.php @@ -2,19 +2,12 @@ namespace Wallabag\ImportBundle\Import; -use Psr\Log\LoggerInterface; -use Psr\Log\NullLogger; -use Doctrine\ORM\EntityManager; use Wallabag\CoreBundle\Entity\Entry; use Wallabag\UserBundle\Entity\User; -use Wallabag\CoreBundle\Helper\ContentProxy; -abstract class WallabagImport implements ImportInterface +abstract class WallabagImport extends AbstractImport { protected $user; - protected $em; - protected $logger; - protected $contentProxy; protected $skippedEntries = 0; protected $importedEntries = 0; protected $filepath; @@ -35,18 +28,6 @@ abstract class WallabagImport implements ImportInterface '', ]; - public function __construct(EntityManager $em, ContentProxy $contentProxy) - { - $this->em = $em; - $this->logger = new NullLogger(); - $this->contentProxy = $contentProxy; - } - - public function setLogger(LoggerInterface $logger) - { - $this->logger = $logger; - } - /** * We define the user in a custom call because on the import command there is no logged in user. * So we can't retrieve user from the `security.token_storage` service. @@ -159,12 +140,18 @@ abstract class WallabagImport implements ImportInterface $data = $this->prepareEntry($importedEntry, $this->markAsRead); - $entry = $this->contentProxy->updateEntry( + $entry = $this->fetchContent( new Entry($this->user), $importedEntry['url'], $data ); + // jump to next entry in case of problem while getting content + if (false === $entry) { + ++$this->skippedEntries; + continue; + } + if (array_key_exists('tags', $data)) { $this->contentProxy->assignTagsToEntry( $entry, diff --git a/tests/Wallabag/ImportBundle/Import/PocketImportTest.php b/tests/Wallabag/ImportBundle/Import/PocketImportTest.php index 41f9b51f..8534e1c8 100644 --- a/tests/Wallabag/ImportBundle/Import/PocketImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/PocketImportTest.php @@ -390,4 +390,55 @@ class PocketImportTest extends \PHPUnit_Framework_TestCase $this->assertContains('PocketImport: Failed to import', $records[0]['message']); $this->assertEquals('ERROR', $records[0]['level_name']); } + + public function testImportWithExceptionFromGraby() + { + $client = new Client(); + + $mock = new Mock([ + new Response(200, ['Content-Type' => 'application/json'], Stream::factory(json_encode(['access_token' => 'wunderbar_token']))), + new Response(200, ['Content-Type' => 'application/json'], Stream::factory(' + { + "status": 1, + "list": { + "229279689": { + "resolved_url": "http://www.grantland.com/blog/the-triangle/post/_/id/38347/ryder-cup-preview" + } + } + } + ')), + ]); + + $client->getEmitter()->attach($mock); + + $pocketImport = $this->getPocketImport(); + + $entryRepo = $this->getMockBuilder('Wallabag\CoreBundle\Repository\EntryRepository') + ->disableOriginalConstructor() + ->getMock(); + + $entryRepo->expects($this->once()) + ->method('findByUrlAndUserId') + ->will($this->onConsecutiveCalls(false, true)); + + $this->em + ->expects($this->once()) + ->method('getRepository') + ->willReturn($entryRepo); + + $entry = new Entry($this->user); + + $this->contentProxy + ->expects($this->once()) + ->method('updateEntry') + ->will($this->throwException(new \Exception())); + + $pocketImport->setClient($client); + $pocketImport->authorize('wunderbar_code'); + + $res = $pocketImport->import(); + + $this->assertTrue($res); + $this->assertEquals(['skipped' => 1, 'imported' => 0], $pocketImport->getSummary()); + } } diff --git a/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php b/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php index 8ec66b12..4a45e0f0 100644 --- a/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php @@ -143,4 +143,44 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->assertContains('WallabagImport: user is not defined', $records[0]['message']); $this->assertEquals('ERROR', $records[0]['level_name']); } + + public function testImportEmptyFile() + { + $wallabagV2Import = $this->getWallabagV2Import(); + $wallabagV2Import->setFilepath(__DIR__.'/../fixtures/wallabag-v2-empty.json'); + + $res = $wallabagV2Import->import(); + + $this->assertFalse($res); + $this->assertEquals(['skipped' => 0, 'imported' => 0], $wallabagV2Import->getSummary()); + } + + public function testImportWithExceptionFromGraby() + { + $wallabagV2Import = $this->getWallabagV2Import(); + $wallabagV2Import->setFilepath(__DIR__.'/../fixtures/wallabag-v2.json'); + + $entryRepo = $this->getMockBuilder('Wallabag\CoreBundle\Repository\EntryRepository') + ->disableOriginalConstructor() + ->getMock(); + + $entryRepo->expects($this->exactly(24)) + ->method('findByUrlAndUserId') + ->will($this->onConsecutiveCalls(false, true, false)); + + $this->em + ->expects($this->any()) + ->method('getRepository') + ->willReturn($entryRepo); + + $this->contentProxy + ->expects($this->exactly(2)) + ->method('updateEntry') + ->will($this->throwException(new \Exception())); + + $res = $wallabagV2Import->import(); + + $this->assertTrue($res); + $this->assertEquals(['skipped' => 24, 'imported' => 0], $wallabagV2Import->getSummary()); + } } diff --git a/tests/Wallabag/ImportBundle/fixtures/wallabag-v2-empty.json b/tests/Wallabag/ImportBundle/fixtures/wallabag-v2-empty.json new file mode 100644 index 00000000..e69de29b