Merge pull request #364 from acelaya/bugfix/non-locatable-addresses

Bugfix/non locatable addresses

CHANGELOG.md

@@ -4,6 +4,29 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org).
 
+## [Unreleased]
+
+#### Added
+
+* *Nothing*
+
+#### Changed
+
+* *Nothing*
+
+#### Deprecated
+
+* *Nothing*
+
+#### Removed
+
+* *Nothing*
+
+#### Fixed
+
+* [#362](https://github.com/shlinkio/shlink/issues/362) Fixed all visits without an IP address being processed every time the `visit:process` command is executed.
+
+
 ## 1.16.0 - 2019-02-23
 
 #### Added

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

@@ -65,7 +65,11 @@ class ProcessVisitsCommand extends Command
             $this->visitService->locateUnlocatedVisits(
                 [$this, 'getGeolocationDataForVisit'],
                 function (VisitLocation $location) use ($output) {
-                    $output->writeln(sprintf(' [<info>Address located at "%s"</info>]', $location->getCountryName()));
+                    if (! $location->isEmpty()) {
+                        $output->writeln(
+                            sprintf(' [<info>Address located at "%s"</info>]', $location->getCountryName())
+                        );
+                    }
                 }
             );
 
@@ -83,14 +87,14 @@ class ProcessVisitsCommand extends Command
                 '<comment>Ignored visit with no IP address</comment>',
                 OutputInterface::VERBOSITY_VERBOSE
             );
-            throw new IpCannotBeLocatedException('Ignored visit with no IP address');
+            throw IpCannotBeLocatedException::forEmptyAddress();
         }
 
         $ipAddr = $visit->getRemoteAddr();
         $this->output->write(sprintf('Processing IP <fg=blue>%s</>', $ipAddr));
         if ($ipAddr === IpAddress::LOCALHOST) {
             $this->output->writeln(' [<comment>Ignored localhost address</comment>]');
-            throw new IpCannotBeLocatedException('Ignored localhost address');
+            throw IpCannotBeLocatedException::forLocalhost();
         }
 
         try {
@@ -101,7 +105,7 @@ class ProcessVisitsCommand extends Command
                 $this->getApplication()->renderException($e, $this->output);
             }
 
-            throw new IpCannotBeLocatedException('An error occurred while locating IP', $e->getCode(), $e);
+            throw IpCannotBeLocatedException::forError($e);
         }
     }
 }

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

@@ -116,6 +116,11 @@ class ProcessVisitsCommandTest extends TestCase
 
         $output = $this->commandTester->getDisplay();
         $this->assertStringContainsString($message, $output);
+        if (empty($address)) {
+            $this->assertStringNotContainsString('Processing IP', $output);
+        } else {
+            $this->assertStringContainsString('Processing IP', $output);
+        }
         $locateVisits->shouldHaveBeenCalledOnce();
         $resolveIpLocation->shouldNotHaveBeenCalled();
     }

module/Core/src/Entity/VisitLocation.php

@@ -72,4 +72,16 @@ class VisitLocation extends AbstractEntity implements VisitLocationInterface
             'timezone' => $this->timezone,
         ];
     }
+
+    public function isEmpty(): bool
+    {
+        return
+            $this->countryCode === '' &&
+            $this->countryName === '' &&
+            $this->regionName === '' &&
+            $this->cityName === '' &&
+            ((float) $this->latitude) === 0.0 &&
+            ((float) $this->longitude) === 0.0 &&
+            $this->timezone === '';
+    }
 }

module/Core/src/Exception/IpCannotBeLocatedException.php

@@ -3,6 +3,43 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\Core\Exception;
 
+use Throwable;
+
 class IpCannotBeLocatedException extends RuntimeException
 {
+    /** @var bool */
+    private $isNonLocatableAddress;
+
+    public function __construct(
+        bool $isNonLocatableAddress,
+        string $message,
+        int $code = 0,
+        ?Throwable $previous = null
+    ) {
+        $this->isNonLocatableAddress = $isNonLocatableAddress;
+        parent::__construct($message, $code, $previous);
+    }
+
+    public static function forEmptyAddress(): self
+    {
+        return new self(true, 'Ignored visit with no IP address');
+    }
+
+    public static function forLocalhost(): self
+    {
+        return new self(true, 'Ignored localhost address');
+    }
+
+    public static function forError(Throwable $e): self
+    {
+        return new self(false, 'An error occurred while locating IP', $e->getCode(), $e);
+    }
+
+    /**
+     * Tells if this error belongs to an address that will never be possible locate
+     */
+    public function isNonLocatableAddress(): bool
+    {
+        return $this->isNonLocatableAddress;
+    }
 }

module/Core/src/Service/VisitService.php

@@ -30,12 +30,18 @@ class VisitService implements VisitServiceInterface
 
         foreach ($results as $visit) {
             $count++;
+
             try {
                 /** @var Location $location */
                 $location = $geolocateVisit($visit);
             } catch (IpCannotBeLocatedException $e) {
-                // Skip if the visit's IP could not be located
+                if (! $e->isNonLocatableAddress()) {
-                continue;
+                    // Skip if the visit's IP could not be located because of an error
+                    continue;
+                }
+
+                // If the IP address is non-locatable, locate it as empty to prevent next processes to pick it again
+                $location = Location::emptyInstance();
             }
 
             $location = new VisitLocation($location);

module/Core/test/Entity/VisitLocationTest.php

@@ -18,4 +18,28 @@ class VisitLocationTest extends TestCase
         $this->assertSame('1000.7', $location->getLatitude());
         $this->assertSame('-2000.4', $location->getLongitude());
     }
+
+    /**
+     * @test
+     * @dataProvider provideArgs
+     */
+    public function isEmptyReturnsTrueWhenAllValuesAreEmpty(array $args, bool $isEmpty): void
+    {
+        $payload = new Location(...$args);
+        $location = new VisitLocation($payload);
+
+        $this->assertEquals($isEmpty, $location->isEmpty());
+    }
+
+    public function provideArgs(): iterable
+    {
+        yield [['', '', '', '', 0.0, 0.0, ''], true];
+        yield [['', '', '', '', 0.0, 0.0, 'dd'], false];
+        yield [['', '', '', 'dd', 0.0, 0.0, ''], false];
+        yield [['', '', 'dd', '', 0.0, 0.0, ''], false];
+        yield [['', 'dd', '', '', 0.0, 0.0, ''], false];
+        yield [['dd', '', '', '', 0.0, 0.0, ''], false];
+        yield [['', '', '', '', 1.0, 0.0, ''], false];
+        yield [['', '', '', '', 0.0, 1.0, ''], false];
+    }
 }

module/Core/test/Exception/IpCannotBeLocatedExceptionTest.php

@@ -0,0 +1,95 @@
+<?php
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Core\Exception;
+
+use Exception;
+use LogicException;
+use PHPUnit\Framework\TestCase;
+use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException;
+use Shlinkio\Shlink\Core\Exception\RuntimeException;
+use Throwable;
+use function count;
+use function func_get_args;
+use function random_int;
+
+class IpCannotBeLocatedExceptionTest extends TestCase
+{
+    /** @test */
+    public function forEmptyAddressInitializesException(): void
+    {
+        $e = IpCannotBeLocatedException::forEmptyAddress();
+
+        $this->assertTrue($e->isNonLocatableAddress());
+        $this->assertEquals('Ignored visit with no IP address', $e->getMessage());
+        $this->assertEquals(0, $e->getCode());
+        $this->assertNull($e->getPrevious());
+    }
+
+    /** @test */
+    public function forLocalhostInitializesException(): void
+    {
+        $e = IpCannotBeLocatedException::forLocalhost();
+
+        $this->assertTrue($e->isNonLocatableAddress());
+        $this->assertEquals('Ignored localhost address', $e->getMessage());
+        $this->assertEquals(0, $e->getCode());
+        $this->assertNull($e->getPrevious());
+    }
+
+    /**
+     * @test
+     * @dataProvider provideErrors
+     */
+    public function forErrorInitializesException(Throwable $prev): void
+    {
+        $e = IpCannotBeLocatedException::forError($prev);
+
+        $this->assertFalse($e->isNonLocatableAddress());
+        $this->assertEquals('An error occurred while locating IP', $e->getMessage());
+        $this->assertEquals($prev->getCode(), $e->getCode());
+        $this->assertSame($prev, $e->getPrevious());
+    }
+
+    public function provideErrors(): iterable
+    {
+        yield 'Simple exception with positive code' => [new Exception('Some message', 100)];
+        yield 'Runtime exception with negative code' => [new RuntimeException('Something went wrong', -50)];
+        yield 'Logic exception with default code' => [new LogicException('Conditions unmet')];
+    }
+
+    /**
+     * @test
+     * @dataProvider provideConstructorArgs
+     */
+    public function constructorInitializesException(): void
+    {
+        $args = func_get_args();
+        [$isNonLocatableAddress, $message] = $args;
+        $code = $args[2] ?? 0;
+        $prev = $args[3] ?? null;
+
+        switch (count($args)) {
+            case 2:
+                $e = new IpCannotBeLocatedException($isNonLocatableAddress, $message);
+                break;
+            case 3:
+                $e = new IpCannotBeLocatedException($isNonLocatableAddress, $message, $code);
+                break;
+            default:
+                $e = new IpCannotBeLocatedException($isNonLocatableAddress, $message, $code, $prev);
+        }
+
+        $this->assertEquals($isNonLocatableAddress, $e->isNonLocatableAddress());
+        $this->assertEquals($message, $e->getMessage());
+        $this->assertEquals($code, $e->getCode());
+        $this->assertEquals($prev, $e->getPrevious());
+    }
+
+    public function provideConstructorArgs(): iterable
+    {
+        yield 'without default args' => [true, 'Message'];
+        yield 'without prev' => [true, 'Message', random_int(1, 100)];
+        yield 'without all args' => [false, 'Foo', random_int(1, 100), new RuntimeException('Foo')];
+    }
+}

module/Core/test/Service/VisitServiceTest.php

@@ -70,8 +70,11 @@ class VisitServiceTest extends TestCase
         $clear->shouldHaveBeenCalledTimes(floor(count($unlocatedVisits) / 200) + 1);
     }
 
-    /** @test */
+    /**
-    public function visitsWhichCannotBeLocatedAreIgnored(): void
+     * @test
+     * @dataProvider provideIsNonLocatableAddress
+     */
+    public function visitsWhichCannotBeLocatedAreIgnoredOrLocatedAsEmpty(bool $isNonLocatableAddress): void
     {
         $unlocatedVisits = [
             new Visit(new ShortUrl('foo'), Visitor::emptyInstance()),
@@ -88,14 +91,20 @@ class VisitServiceTest extends TestCase
         $clear = $this->em->clear()->will(function () {
         });
 
-        $this->visitService->locateUnlocatedVisits(function () {
+        $this->visitService->locateUnlocatedVisits(function () use ($isNonLocatableAddress) {
-            throw new IpCannotBeLocatedException('Cannot be located');
+            throw new IpCannotBeLocatedException($isNonLocatableAddress, 'Cannot be located');
         });
 
         $findUnlocatedVisits->shouldHaveBeenCalledOnce();
         $getRepo->shouldHaveBeenCalledOnce();
-        $persist->shouldNotHaveBeenCalled();
+        $persist->shouldHaveBeenCalledTimes($isNonLocatableAddress ? 1 : 0);
         $flush->shouldHaveBeenCalledOnce();
         $clear->shouldHaveBeenCalledOnce();
     }
+
+    public function provideIsNonLocatableAddress(): iterable
+    {
+        yield 'The address is locatable' => [false];
+        yield 'The address is non-locatable' => [true];
+    }
 }