Merge pull request #2224 from wallabag/avoid-exception-import

Avoid breaking import when fetching fail

6 files changed, 154 insertions, 32 deletions

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 @@
+<?php
+
+namespace Wallabag\ImportBundle\Import;
+
+use Psr\Log\LoggerInterface;
+use Psr\Log\NullLogger;
+use Doctrine\ORM\EntityManager;
+use Wallabag\CoreBundle\Helper\ContentProxy;
+use Wallabag\CoreBundle\Entity\Entry;
+
+abstract class AbstractImport implements ImportInterface
+{
+    protected $em;
+    protected $logger;
+    protected $contentProxy;
+
+    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;
+    }
+
+    /**
+     * 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
--- /dev/null
+++ b/tests/Wallabag/ImportBundle/fixtures/wallabag-v2-empty.json