Merge pull request #3 from cultuurnet/feature/III-2464

III-2464 Return null when geocoder throws no result exception.
cultuurnet · Jan 24, 2018 · 865d849 · 865d849
2 parents 014de06 + 531503a
commit 865d849
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 19 deletions.
diff --git a/src/CachedGeocodingService.php b/src/CachedGeocodingService.php
@@ -9,6 +9,8 @@
 
 class CachedGeocodingService implements GeocodingServiceInterface
 {
+    const NO_COORDINATES_FOUND = 'NO_COORDINATES_FOUND';
+
     /**
      * @var GeocodingServiceInterface
      */
@@ -30,8 +32,7 @@ public function __construct(GeocodingServiceInterface $geocodingService, Cache $
     }
 
     /**
-     * @param string $address
-     * @return Coordinates
+     * @inheritdoc
      */
     public function getCoordinates($address)
     {
@@ -40,6 +41,12 @@ public function getCoordinates($address)
         if ($encodedCacheData) {
             $cacheData = json_decode($encodedCacheData, true);
 
+            // Some addresses have no coordinates, to cache these addresses 'NO_COORDINATES_FOUND' is used as value.
+            // When the 'NO_COORDINATES_FOUND' cached value is found null is returned as coordinate.
+            if (self::NO_COORDINATES_FOUND === $cacheData) {
+                return null;
+            }
+
             if (isset($cacheData['lat']) && isset($cacheData['long'])) {
                 return new Coordinates(
                     new Latitude((double) $cacheData['lat']),
@@ -50,14 +57,17 @@ public function getCoordinates($address)
 
         $coordinates = $this->geocodingService->getCoordinates($address);
 
-        $encodedCacheData = json_encode(
-            [
+        // Some addresses have no coordinates, to cache these addresses 'NO_COORDINATES_FOUND' is used as value.
+        // When null is passed in as the coordinates, then 'NO_COORDINATES_FOUND' is stored as cache value.
+        $cacheData = self::NO_COORDINATES_FOUND;
+        if ($coordinates) {
+            $cacheData = [
                 'lat' => $coordinates->getLatitude()->toDouble(),
                 'long' => $coordinates->getLongitude()->toDouble(),
-            ]
-        );
+            ];
+        }
 
-        $this->cache->save($address, $encodedCacheData);
+        $this->cache->save($address, json_encode($cacheData));
 
         return $coordinates;
     }

diff --git a/src/DefaultGeocodingService.php b/src/DefaultGeocodingService.php
@@ -5,7 +5,9 @@
 use CultuurNet\Geocoding\Coordinate\Coordinates;
 use CultuurNet\Geocoding\Coordinate\Latitude;
 use CultuurNet\Geocoding\Coordinate\Longitude;
+use Geocoder\Exception\NoResultException;
 use Geocoder\GeocoderInterface;
+use Psr\Log\LoggerInterface;
 
 class DefaultGeocodingService implements GeocodingServiceInterface
 {
@@ -14,26 +16,41 @@ class DefaultGeocodingService implements GeocodingServiceInterface
      */
     private $geocoder;
 
+    /**
+     * @var LoggerInterface
+     */
+    private $logger;
+
     /**
      * @param GeocoderInterface $geocoder
+     * @param LoggerInterface $logger
      */
-    public function __construct(GeocoderInterface $geocoder)
-    {
+    public function __construct(
+        GeocoderInterface $geocoder,
+        LoggerInterface $logger
+    ) {
         $this->geocoder = $geocoder;
+        $this->logger = $logger;
     }
 
     /**
-     * @param string $address
-     * @return Coordinates
+     * @inheritdoc
      */
     public function getCoordinates($address)
     {
-        $result = $this->geocoder->geocode($address);
-        $coordinates = $result->getCoordinates();
+        try {
+            $result = $this->geocoder->geocode($address);
+            $coordinates = $result->getCoordinates();
 
-        return new Coordinates(
-            new Latitude((double) $coordinates[0]),
-            new Longitude((double) $coordinates[1])
-        );
+            return new Coordinates(
+                new Latitude((double)$coordinates[0]),
+                new Longitude((double)$coordinates[1])
+            );
+        } catch (NoResultException $exception) {
+            $this->logger->error(
+                'No results for address: "' . $address . '". Exception message: ' . $exception->getMessage()
+            );
+            return null;
+        }
     }
 }
diff --git a/src/GeocodingServiceInterface.php b/src/GeocodingServiceInterface.php
@@ -7,8 +7,12 @@
 interface GeocodingServiceInterface
 {
     /**
+     * Gets the coordinates of the given address.
+     * Returns null when no coordinates are found for the given address.
+     * This can happen in case of a wrong/unknown address.
+     *
      * @param $address
-     * @return Coordinates
+     * @return Coordinates|null
      */
     public function getCoordinates($address);
 }
diff --git a/tests/CachedGeocodingServiceTest.php b/tests/CachedGeocodingServiceTest.php
@@ -54,4 +54,23 @@ public function it_returns_cached_coordinates_if_possible()
         $this->assertEquals($expectedCoordinates, $freshCoordinates);
         $this->assertEquals($expectedCoordinates, $cachedCoordinates);
     }
+
+    /**
+     * @test
+     */
+    public function it_also_caches_when_no_coordinates_were_found()
+    {
+        $address = 'Eikelberg (achter de bibliotheek), 8340 Sijsele (Damme), BE';
+
+        $this->decoratee->expects($this->once())
+            ->method('getCoordinates')
+            ->with($address)
+            ->willReturn(null);
+
+        $freshCoordinates = $this->service->getCoordinates($address);
+        $cachedCoordinates = $this->service->getCoordinates($address);
+
+        $this->assertNull($freshCoordinates);
+        $this->assertNull($cachedCoordinates);
+    }
 }
diff --git a/tests/DefaultGeocodingServiceTest.php b/tests/DefaultGeocodingServiceTest.php
@@ -5,8 +5,10 @@
 use CultuurNet\Geocoding\Coordinate\Coordinates;
 use CultuurNet\Geocoding\Coordinate\Latitude;
 use CultuurNet\Geocoding\Coordinate\Longitude;
+use Geocoder\Exception\NoResultException;
 use Geocoder\GeocoderInterface;
 use Geocoder\Result\Geocoded;
+use Psr\Log\LoggerInterface;
 
 class DefaultGeocodingServiceTest extends \PHPUnit_Framework_TestCase
 {
@@ -15,6 +17,11 @@ class DefaultGeocodingServiceTest extends \PHPUnit_Framework_TestCase
      */
     private $geocoder;
 
+    /**
+     * @var LoggerInterface|\PHPUnit_Framework_MockObject_MockObject
+     */
+    private $logger;
+
     /**
      * @var DefaultGeocodingService
      */
@@ -23,7 +30,9 @@ class DefaultGeocodingServiceTest extends \PHPUnit_Framework_TestCase
     public function setUp()
     {
         $this->geocoder = $this->createMock(GeocoderInterface::class);
-        $this->service = new DefaultGeocodingService($this->geocoder);
+        $this->logger = $this->createMock(LoggerInterface::class);
+
+        $this->service = new DefaultGeocodingService($this->geocoder, $this->logger);
     }
 
     /**
@@ -58,4 +67,27 @@ public function it_returns_coordinates()
 
         $this->assertEquals($expectedCoordinates, $actualCoordinates);
     }
+
+    /**
+     * @test
+     */
+    public function it_returns_null_on_no_result_exception_from_geocoder()
+    {
+        $address = 'Eikelberg (achter de bibliotheek), 8340 Sijsele (Damme), BE';
+
+        $this->geocoder->expects($this->once())
+            ->method('geocode')
+            ->with($address)
+            ->willThrowException(
+                new NoResultException('Could not execute query')
+            );
+
+        $this->logger->expects($this->once())
+            ->method('error')
+            ->with('No results for address: "'. $address . '". Exception message: Could not execute query');
+
+        $actualCoordinates = $this->service->getCoordinates($address);
+
+        $this->assertNull($actualCoordinates);
+    }
 }