Merge pull request #176 from acelaya/feature/1.10.2

feature/1.10.2

CHANGELOG.md

@@ -1,5 +1,33 @@
 # CHANGELOG
 
+## 1.10.2 - 2018-08-04
+
+#### Added
+
+* *Nothing*
+
+#### Changed
+
+* *Nothing*
+
+#### Deprecated
+
+* *Nothing*
+
+#### Removed
+
+* *Nothing*
+
+#### Fixed
+
+* [#177](https://github.com/shlinkio/shlink/issues/177) Fixed `[GET] /short-codes` endpoint returning a 500 status code when trying to filter by `tags` and `searchTerm` at the same time.
+* [#175](https://github.com/shlinkio/shlink/issues/175) Fixed error introduced in previous version, where you could end up banned from the service used to resolve IP address locations.
+
+    In order to fix that, just fill [this form](http://ip-api.com/docs/unban) including your server's IP address and your server should be unbanned.
+    
+    In order to prevent this, after resolving 150 IP addresses, shlink now waits 1 minute before trying to resolve any more addresses.
+
+
 ## 1.10.1 - 2018-08-02
 
 #### Added

docs/swagger/paths/v1_short-codes.json

@@ -25,7 +25,7 @@
                 }
             },
             {
-                "name": "tags",
+                "name": "tags[]",
                 "in": "query",
                 "description": "A list of tags used to filter the resultset. Only short URLs tagged with at least one of the provided tags will be returned. (Since v1.3.0)",
                 "required": false,

module/CLI/lang/es.po

@@ -1,15 +1,15 @@
 msgid ""
 msgstr ""
 "Project-Id-Version: Shlink 1.0\n"
-"POT-Creation-Date: 2018-01-21 09:36+0100\n"
-"PO-Revision-Date: 2018-01-21 09:39+0100\n"
+"POT-Creation-Date: 2018-08-04 16:35+0200\n"
+"PO-Revision-Date: 2018-08-04 16:37+0200\n"
 "Last-Translator: Alejandro Celaya <alejandro@alejandrocelaya.com>\n"
 "Language-Team: \n"
 "Language: es_ES\n"
 "MIME-Version: 1.0\n"
 "Content-Type: text/plain; charset=UTF-8\n"
 "Content-Transfer-Encoding: 8bit\n"
-"X-Generator: Poedit 2.0.4\n"
+"X-Generator: Poedit 2.0.6\n"
 "X-Poedit-Basepath: ..\n"
 "Plural-Forms: nplurals=2; plural=(n != 1);\n"
 "X-Poedit-SourceCharset: UTF-8\n"
@@ -317,6 +317,13 @@ msgstr "Ignorada IP de localhost"
 msgid "Address located at \"%s\""
 msgstr "Dirección localizada en \"%s\""
 
+msgid "An error occurred while locating IP"
+msgstr "Se produjo un error al localizar la IP"
+
+#, php-format
+msgid "IP location resolver limit reached. Waiting %s seconds..."
+msgstr "Limite del localizador de IPs alcanzado. Esperando %s segundos..."
+
 msgid "Finished processing all IPs"
 msgstr "Finalizado el procesado de todas las IPs"
 

module/CLI/src/Command/Visit/ProcessVisitsCommand.php

@@ -55,6 +55,7 @@ class ProcessVisitsCommand extends Command
         $io = new SymfonyStyle($input, $output);
         $visits = $this->visitService->getUnlocatedVisits();
 
+        $count = 0;
         foreach ($visits as $visit) {
             $ipAddr = $visit->getRemoteAddr();
             $io->write(\sprintf('%s <info>%s</info>', $this->translator->translate('Processing IP'), $ipAddr));
@@ -65,6 +66,7 @@ class ProcessVisitsCommand extends Command
                 continue;
             }
 
+            $count++;
             try {
                 $result = $this->ipLocationResolver->resolveIpLocation($ipAddr);
 
@@ -85,6 +87,16 @@ class ProcessVisitsCommand extends Command
                     $this->getApplication()->renderException($e, $output);
                 }
             }
+
+            if ($count === $this->ipLocationResolver->getApiLimit()) {
+                $count = 0;
+                $seconds = $this->ipLocationResolver->getApiInterval();
+                $io->note(\sprintf(
+                    $this->translator->translate('IP location resolver limit reached. Waiting %s seconds...'),
+                    $seconds
+                ));
+                \sleep($seconds);
+            }
         }
 
         $io->success($this->translator->translate('Finished processing all IPs'));

module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php

@@ -33,6 +33,8 @@ class ProcessVisitsCommandTest extends TestCase
     {
         $this->visitService = $this->prophesize(VisitService::class);
         $this->ipResolver = $this->prophesize(IpApiLocationResolver::class);
+        $this->ipResolver->getApiLimit()->willReturn(10000000000);
+
         $command = new ProcessVisitsCommand(
             $this->visitService->reveal(),
             $this->ipResolver->reveal(),
@@ -95,4 +97,41 @@ class ProcessVisitsCommandTest extends TestCase
         $output = $this->commandTester->getDisplay();
         $this->assertTrue(strpos($output, 'Ignored localhost address') > 0);
     }
+
+    /**
+     * @test
+     */
+    public function sleepsEveryTimeTheApiLimitIsReached()
+    {
+        $visits = [
+            (new Visit())->setRemoteAddr('1.2.3.4'),
+            (new Visit())->setRemoteAddr('4.3.2.1'),
+            (new Visit())->setRemoteAddr('12.34.56.78'),
+            (new Visit())->setRemoteAddr('1.2.3.4'),
+            (new Visit())->setRemoteAddr('4.3.2.1'),
+            (new Visit())->setRemoteAddr('12.34.56.78'),
+            (new Visit())->setRemoteAddr('1.2.3.4'),
+            (new Visit())->setRemoteAddr('4.3.2.1'),
+            (new Visit())->setRemoteAddr('12.34.56.78'),
+            (new Visit())->setRemoteAddr('4.3.2.1'),
+        ];
+        $apiLimit = 3;
+
+        $this->visitService->getUnlocatedVisits()->willReturn($visits);
+        $this->visitService->saveVisit(Argument::any())->will(function () {
+        });
+
+        $getApiLimit = $this->ipResolver->getApiLimit()->willReturn($apiLimit);
+        $getApiInterval = $this->ipResolver->getApiInterval()->willReturn(0);
+        $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([])
+            ->shouldBeCalledTimes(count($visits));
+
+        $this->commandTester->execute([
+            'command' => 'visit:process',
+        ]);
+
+        $getApiLimit->shouldHaveBeenCalledTimes(\count($visits));
+        $getApiInterval->shouldHaveBeenCalledTimes(\round(\count($visits) / $apiLimit));
+        $resolveIpLocation->shouldHaveBeenCalledTimes(\count($visits));
+    }
 }

module/Common/src/Service/IpApiLocationResolver.php

@@ -48,4 +48,24 @@ class IpApiLocationResolver implements IpLocationResolverInterface
             'time_zone' => $entry['timezone'] ?? '',
         ];
     }
+
+    /**
+     * Returns the interval in seconds that needs to be waited when the API limit is reached
+     *
+     * @return int
+     */
+    public function getApiInterval(): int
+    {
+        return 65; // ip-api interval is 1 minute. Return 5 extra seconds just in case
+    }
+
+    /**
+     * Returns the limit of requests that can be performed to the API in a specific interval, or null if no limit exists
+     *
+     * @return int|null
+     */
+    public function getApiLimit(): ?int
+    {
+        return 145; // ip-api limit is 150 requests per minute. Leave 5 less requests just in case
+    }
 }

module/Common/src/Service/IpLocationResolverInterface.php

@@ -13,4 +13,18 @@ interface IpLocationResolverInterface
      * @throws WrongIpException
      */
     public function resolveIpLocation(string $ipAddress): array;
+
+    /**
+     * Returns the interval in seconds that needs to be waited when the API limit is reached
+     *
+     * @return int
+     */
+    public function getApiInterval(): int;
+
+    /**
+     * Returns the limit of requests that can be performed to the API in a specific interval, or null if no limit exists
+     *
+     * @return int|null
+     */
+    public function getApiLimit(): ?int;
 }

module/Common/test/Service/IpApiLocationResolverTest.php

@@ -65,4 +65,20 @@ class IpApiLocationResolverTest extends TestCase
                                                             ->shouldBeCalledTimes(1);
         $this->ipResolver->resolveIpLocation('1.2.3.4');
     }
+
+    /**
+     * @test
+     */
+    public function getApiIntervalReturnsExpectedValue()
+    {
+        $this->assertEquals(65, $this->ipResolver->getApiInterval());
+    }
+
+    /**
+     * @test
+     */
+    public function getApiLimitReturnsExpectedValue()
+    {
+        $this->assertEquals(145, $this->ipResolver->getApiLimit());
+    }
 }

module/Core/src/Repository/ShortUrlRepository.php

@@ -93,7 +93,10 @@ class ShortUrlRepository extends EntityRepository implements ShortUrlRepositoryI
 
         // Apply search term to every searchable field if not empty
         if (! empty($searchTerm)) {
-            $qb->leftJoin('s.tags', 't');
+            // Left join with tags only if no tags were provided. In case of tags, an inner join will be done later
+            if (empty($tags)) {
+                $qb->leftJoin('s.tags', 't');
+            }
 
             $conditions = [
                 $qb->expr()->like('s.originalUrl', ':searchPattern'),

module/Core/test-func/Repository/ShortUrlRepositoryTest.php

@@ -5,15 +5,17 @@ namespace ShlinkioTest\Shlink\Core\Repository;
 
 use Doctrine\Common\Collections\ArrayCollection;
 use Shlinkio\Shlink\Core\Entity\ShortUrl;
+use Shlinkio\Shlink\Core\Entity\Tag;
 use Shlinkio\Shlink\Core\Entity\Visit;
 use Shlinkio\Shlink\Core\Repository\ShortUrlRepository;
 use ShlinkioTest\Shlink\Common\DbUnit\DatabaseTestCase;
 
 class ShortUrlRepositoryTest extends DatabaseTestCase
 {
-    const ENTITIES_TO_EMPTY = [
+    protected const ENTITIES_TO_EMPTY = [
         ShortUrl::class,
         Visit::class,
+        Tag::class,
     ];
 
     /**
@@ -79,4 +81,35 @@ class ShortUrlRepositoryTest extends DatabaseTestCase
 
         $this->assertEquals($count, $this->repo->countList());
     }
+
+    /**
+     * @test
+     */
+    public function findListProperlyFiltersByTagAndSearchTerm()
+    {
+        $tag = new Tag('bar');
+        $this->getEntityManager()->persist($tag);
+
+        $foo = new ShortUrl();
+        $foo->setOriginalUrl('foo')
+            ->setShortCode('foo')
+            ->setTags(new ArrayCollection([$tag]));
+        $this->getEntityManager()->persist($foo);
+
+        $bar = new ShortUrl();
+        $bar->setOriginalUrl('bar')
+            ->setShortCode('bar_very_long_text');
+        $this->getEntityManager()->persist($bar);
+
+        $foo2 = new ShortUrl();
+        $foo2->setOriginalUrl('foo_2')
+            ->setShortCode('foo_2');
+        $this->getEntityManager()->persist($foo2);
+
+        $this->getEntityManager()->flush();
+
+        $result = $this->repo->findList(null, null, 'foo', ['bar']);
+        $this->assertCount(1, $result);
+        $this->assertSame($foo, $result[0]);
+    }
 }

module/Core/test-func/Repository/TagRepositoryTest.php

@@ -9,7 +9,7 @@ use ShlinkioTest\Shlink\Common\DbUnit\DatabaseTestCase;
 
 class TagRepositoryTest extends DatabaseTestCase
 {
-    const ENTITIES_TO_EMPTY = [
+    protected const ENTITIES_TO_EMPTY = [
         Tag::class,
     ];
 

module/Core/test-func/Repository/VisitRepositoryTest.php

@@ -12,7 +12,7 @@ use ShlinkioTest\Shlink\Common\DbUnit\DatabaseTestCase;
 
 class VisitRepositoryTest extends DatabaseTestCase
 {
-    const ENTITIES_TO_EMPTY = [
+    protected const ENTITIES_TO_EMPTY = [
         VisitLocation::class,
         Visit::class,
         ShortUrl::class,