Merge pull request #210 from cultuurnet/III-4788-api-problem-types

III-4788 Change error responses to return correct API problems
cultuurnet · Jun 23, 2022 · 7bd25c9 · 7bd25c9
2 parents 0426c8c + 84e8207
commit 7bd25c9
Show file tree

Hide file tree

Showing 14 changed files with 158 additions and 40 deletions.
diff --git a/app/Error/ApiExceptionHandler.php b/app/Error/ApiExceptionHandler.php
@@ -5,11 +5,14 @@
 namespace CultuurNet\UDB3\SearchService\Error;
 
 use Crell\ApiProblem\ApiProblem;
+use CultuurNet\UDB3\Search\ConvertsToApiProblem;
 use CultuurNet\UDB3\Search\Http\ResponseFactory;
 use CultuurNet\UDB3\Search\Json;
 use Elasticsearch\Common\Exceptions\ElasticsearchException;
 use Error;
 use Fig\Http\Message\StatusCodeInterface;
+use League\Route\Http\Exception\MethodNotAllowedException;
+use League\Route\Http\Exception\NotFoundException;
 use Whoops\Handler\Handler;
 use Zend\HttpHandlerRunner\Emitter\EmitterInterface;
 
@@ -39,7 +42,7 @@ public function handle(): ?int
         $problem = $this->createNewApiProblem($jsonSerializableException);
 
         $this->emitter->emit(
-            ResponseFactory::jsonLd(
+            ResponseFactory::apiProblem(
                 $problem->asArray(),
                 $problem->getStatus()
             )
@@ -55,6 +58,22 @@ private function createNewApiProblem(\Throwable $throwable): ApiProblem
                 ->setStatus(StatusCodeInterface::STATUS_INTERNAL_SERVER_ERROR);
         }
 
+        if ($throwable instanceof ConvertsToApiProblem) {
+            return $throwable->convertToApiProblem();
+        }
+
+        if ($throwable instanceof NotFoundException) {
+            $problem = new ApiProblem('Not Found', 'https://api.publiq.be/probs/url/not-found');
+            $problem->setStatus(404);
+            return $problem;
+        }
+
+        if ($throwable instanceof MethodNotAllowedException) {
+            $problem = new ApiProblem('Method not allowed', 'https://api.publiq.be/probs/method/not-allowed');
+            $problem->setStatus(405);
+            return $problem;
+        }
+
         $problem = new ApiProblem($throwable->getMessage());
         $problem->setStatus($throwable->getCode() ?: StatusCodeInterface::STATUS_BAD_REQUEST);
         return $problem;

diff --git a/app/RoutingServiceProvider.php b/app/RoutingServiceProvider.php
@@ -85,24 +85,38 @@ function () {
                     )
                 );
 
-                // Register the OPTIONS method for every route to make the CORS middleware registered above work.
-                $router->options('/{path:.*}', static function () {
-                    return new Response(StatusCodeInterface::STATUS_NO_CONTENT);
-                });
+                $optionsResponse = static fn () => new Response(StatusCodeInterface::STATUS_NO_CONTENT);
 
+                // Register the OPTIONS method for every route to make the CORS middleware registered above work.
                 $router->get('/organizers', OrganizerSearchController::class);
-                $router->get('/offers', ['offer_controller', '__invoke']);
-                $router->get('/events', ['event_controller', '__invoke']);
-                $router->get('/places', ['place_controller', '__invoke']);
-                $router->get('/event', ['event_controller', '__invoke']);
-                $router->get('/place', ['place_controller', '__invoke']);
-
+                $router->options('/organizers', $optionsResponse);
                 $router->get('/organizers/', OrganizerSearchController::class);
+                $router->options('/organizers/', $optionsResponse);
+
+                $router->get('/offers', ['offer_controller', '__invoke']);
+                $router->options('/offers', $optionsResponse);
                 $router->get('/offers/', ['offer_controller', '__invoke']);
+                $router->options('/offers/', $optionsResponse);
+
+                $router->get('/events', ['event_controller', '__invoke']);
+                $router->options('/events', $optionsResponse);
                 $router->get('/events/', ['event_controller', '__invoke']);
+                $router->options('/events/', $optionsResponse);
+
+                $router->get('/places', ['place_controller', '__invoke']);
+                $router->options('/places', $optionsResponse);
                 $router->get('/places/', ['place_controller', '__invoke']);
+                $router->options('/places/', $optionsResponse);
+
+                $router->get('/event', ['event_controller', '__invoke']);
+                $router->options('/event', $optionsResponse);
                 $router->get('/event/', ['event_controller', '__invoke']);
+                $router->options('/event/', $optionsResponse);
+
+                $router->get('/place', ['place_controller', '__invoke']);
+                $router->options('/place', $optionsResponse);
                 $router->get('/place/', ['place_controller', '__invoke']);
+                $router->options('/place/', $optionsResponse);
 
                 return $router;
             }

diff --git a/src/AbstractQueryParameterException.php b/src/AbstractQueryParameterException.php
@@ -0,0 +1,22 @@
+<?php
+
+declare(strict_types=1);
+
+namespace CultuurNet\UDB3\Search;
+
+use Crell\ApiProblem\ApiProblem;
+use InvalidArgumentException;
+
+abstract class AbstractQueryParameterException extends InvalidArgumentException implements ConvertsToApiProblem
+{
+    public function convertToApiProblem(): ApiProblem
+    {
+        $problem = new ApiProblem(
+            'Not Found',
+            'https://api.publiq.be/probs/url/not-found'
+        );
+        $problem->setStatus(404);
+        $problem->setDetail($this->message);
+        return $problem;
+    }
+}
diff --git a/src/ConvertsToApiProblem.php b/src/ConvertsToApiProblem.php
@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+namespace CultuurNet\UDB3\Search;
+
+use Crell\ApiProblem\ApiProblem;
+
+interface ConvertsToApiProblem
+{
+    public function convertToApiProblem(): ApiProblem;
+}
diff --git a/src/ElasticSearch/Organizer/ElasticSearchOrganizerQueryBuilder.php b/src/ElasticSearch/Organizer/ElasticSearchOrganizerQueryBuilder.php
@@ -48,9 +48,20 @@ public function withAutoCompleteFilter(string $input): ElasticSearchOrganizerQue
         return $this->withMatchPhraseQuery('name.nl.autocomplete', $input);
     }
 
-    public function withWebsiteFilter(Url $url): ElasticSearchOrganizerQueryBuilder
-    {
-        return $this->withMatchQuery('url', $url->getNormalizedUrl());
+    public function withWebsiteFilter(string $url): ElasticSearchOrganizerQueryBuilder
+    {
+        // Try to normalize the URL to return as many relevant results as possible.
+        // If the URL could not be parsed, use it as given. This may result in 0 results if the URL is really invalid,
+        // but that is logical since no organizers have that URL then. And in the case that an (older) organizer has
+        // an invalid URL, this still makes it possible to look it up.
+        try {
+            $urlObject = new Url($url);
+            $normalizedUrl = $urlObject->getNormalizedUrl();
+        } catch (\InvalidArgumentException $e) {
+            $normalizedUrl = $url;
+        }
+
+        return $this->withMatchQuery('url', $normalizedUrl);
     }
 
     public function withDomainFilter(string $domain): ElasticSearchOrganizerQueryBuilder

diff --git a/src/Http/OrganizerSearchController.php b/src/Http/OrganizerSearchController.php
@@ -6,7 +6,6 @@
 
 use CultuurNet\UDB3\Search\Address\PostalCode;
 use CultuurNet\UDB3\Search\Creator;
-use CultuurNet\UDB3\Search\ElasticSearch\JsonDocument\Properties\Url;
 use CultuurNet\UDB3\Search\ElasticSearch\Organizer\ElasticSearchOrganizerQueryBuilder;
 use CultuurNet\UDB3\Search\Http\Authentication\Consumer;
 use CultuurNet\UDB3\Search\Http\Organizer\RequestParser\OrganizerRequestParser;
@@ -101,9 +100,7 @@ public function __invoke(ApiRequestInterface $request): ResponseInterface
         }
 
         if ($request->hasQueryParam('website')) {
-            $queryBuilder = $queryBuilder->withWebsiteFilter(
-                new Url($request->getQueryParam('website'))
-            );
+            $queryBuilder = $queryBuilder->withWebsiteFilter($request->getQueryParam('website'));
         }
 
         if ($request->hasQueryParam('domain')) {

diff --git a/src/Http/ResponseFactory.php b/src/Http/ResponseFactory.php
@@ -19,9 +19,25 @@ final class ResponseFactory
      */
     public static function jsonLd($data, int $code = StatusCodeInterface::STATUS_OK): ResponseInterface
     {
+        return self::jsonWithCustomContentType('application/ld+json', $data, $code);
+    }
+
+    /**
+     * @param array|object $data
+     */
+    public static function apiProblem($data, int $code = StatusCodeInterface::STATUS_OK): ResponseInterface
+    {
+        return self::jsonWithCustomContentType('application/problem+json', $data, $code);
+    }
+
+    private static function jsonWithCustomContentType(
+        string $contentType,
+        $data,
+        int $code = StatusCodeInterface::STATUS_OK
+    ): ResponseInterface {
         $response = new Response($code);
 
-        $response = $response->withAddedHeader('Content-Type', 'application/ld+json');
+        $response = $response->withAddedHeader('Content-Type', $contentType);
 
         $body = $response->getBody();
         $body->write(Json::encodeWithOptions($data, self::JSON_OPTIONS));

diff --git a/src/MissingParameter.php b/src/MissingParameter.php
@@ -4,8 +4,6 @@
 
 namespace CultuurNet\UDB3\Search;
 
-use InvalidArgumentException;
-
-final class MissingParameter extends InvalidArgumentException
+final class MissingParameter extends AbstractQueryParameterException
 {
 }
diff --git a/src/Organizer/OrganizerQueryBuilderInterface.php b/src/Organizer/OrganizerQueryBuilderInterface.php
@@ -7,7 +7,6 @@
 use CultuurNet\UDB3\Search\Address\PostalCode;
 use CultuurNet\UDB3\Search\Country;
 use CultuurNet\UDB3\Search\Creator;
-use CultuurNet\UDB3\Search\ElasticSearch\JsonDocument\Properties\Url;
 use CultuurNet\UDB3\Search\GeoBoundsParameters;
 use CultuurNet\UDB3\Search\GeoDistanceParameters;
 use CultuurNet\UDB3\Search\Label\LabelName;
@@ -20,7 +19,7 @@ interface OrganizerQueryBuilderInterface extends QueryBuilder
 {
     public function withAutoCompleteFilter(string $input): OrganizerQueryBuilderInterface;
 
-    public function withWebsiteFilter(Url $url): OrganizerQueryBuilderInterface;
+    public function withWebsiteFilter(string $url): OrganizerQueryBuilderInterface;
 
     public function withDomainFilter(string $domain): OrganizerQueryBuilderInterface;
 

diff --git a/src/UnsupportedParameter.php b/src/UnsupportedParameter.php
@@ -4,8 +4,6 @@
 
 namespace CultuurNet\UDB3\Search;
 
-use InvalidArgumentException;
-
-final class UnsupportedParameter extends InvalidArgumentException
+final class UnsupportedParameter extends AbstractQueryParameterException
 {
 }
diff --git a/src/UnsupportedParameterValue.php b/src/UnsupportedParameterValue.php
@@ -4,8 +4,6 @@
 
 namespace CultuurNet\UDB3\Search;
 
-use InvalidArgumentException;
-
-final class UnsupportedParameterValue extends InvalidArgumentException
+final class UnsupportedParameterValue extends AbstractQueryParameterException
 {
 }
diff --git a/tests/ElasticSearch/Organizer/ElasticSearchOrganizerQueryBuilderTest.php b/tests/ElasticSearch/Organizer/ElasticSearchOrganizerQueryBuilderTest.php
@@ -9,7 +9,6 @@
 use CultuurNet\UDB3\Search\Creator;
 use CultuurNet\UDB3\Search\ElasticSearch\AbstractElasticSearchQueryBuilderTest;
 use CultuurNet\UDB3\Search\ElasticSearch\ElasticSearchDistance;
-use CultuurNet\UDB3\Search\ElasticSearch\JsonDocument\Properties\Url;
 use CultuurNet\UDB3\Search\ElasticSearch\LuceneQueryString;
 use CultuurNet\UDB3\Search\GeoBoundsParameters;
 use CultuurNet\UDB3\Search\Geocoding\Coordinate\Coordinates;
@@ -169,10 +168,10 @@ public function it_should_build_a_query_with_an_autocomplete_filter(): void
     /**
      * @test
      */
-    public function it_should_build_a_query_with_a_website_filter(): void
+    public function it_should_build_a_query_with_a_website_filter_and_normalize_it_if_it_is_a_valid_url(): void
     {
         $builder = (new ElasticSearchOrganizerQueryBuilder())
-            ->withWebsiteFilter(new Url('http://foo.bar'));
+            ->withWebsiteFilter('http://foo.bar');
 
         $expectedQueryArray = [
             '_source' => ['@id', '@type', 'originalEncodedJsonLd', 'regions'],
@@ -203,6 +202,43 @@ public function it_should_build_a_query_with_a_website_filter(): void
         $this->assertEquals($expectedQueryArray, $actualQueryArray);
     }
 
+    /**
+     * @test
+     */
+    public function it_does_not_throw_for_an_invalid_url_as_website(): void
+    {
+        $builder = (new ElasticSearchOrganizerQueryBuilder())
+            ->withWebsiteFilter('foobar');
+
+        $expectedQueryArray = [
+            '_source' => ['@id', '@type', 'originalEncodedJsonLd', 'regions'],
+            'from' => 0,
+            'size' => 30,
+            'query' => [
+                'bool' => [
+                    'must' => [
+                        [
+                            'match_all' => (object) [],
+                        ],
+                    ],
+                    'filter' => [
+                        [
+                            'match' => [
+                                'url' => [
+                                    'query' => 'foobar',
+                                ],
+                            ],
+                        ],
+                    ],
+                ],
+            ],
+        ];
+
+        $actualQueryArray = $builder->build();
+
+        $this->assertEquals($expectedQueryArray, $actualQueryArray);
+    }
+
     /**
      * @test
      */
@@ -282,7 +318,7 @@ public function it_should_build_a_query_with_multiple_filters(): void
             ->withStart(new Start(30))
             ->withLimit(new Limit(10))
             ->withAutoCompleteFilter('foo')
-            ->withWebsiteFilter(new Url('http://foo.bar'));
+            ->withWebsiteFilter('http://foo.bar');
 
         $expectedQueryArray = [
             '_source' => ['@id', '@type', 'originalEncodedJsonLd', 'regions'],
@@ -918,7 +954,7 @@ public function it_should_always_return_a_clone_for_each_mutation(): void
             ->withStart(new Start(30))
             ->withLimit(new Limit(10))
             ->withAutoCompleteFilter('foo')
-            ->withWebsiteFilter(new Url('http://foo.bar'));
+            ->withWebsiteFilter('http://foo.bar');
 
         $expectedQueryArray = [
             '_source' => ['@id', '@type', 'originalEncodedJsonLd', 'regions'],

diff --git a/tests/Http/MockOrganizerQueryBuilder.php b/tests/Http/MockOrganizerQueryBuilder.php
@@ -8,7 +8,6 @@
 use CultuurNet\UDB3\Search\Address\PostalCode;
 use CultuurNet\UDB3\Search\Country;
 use CultuurNet\UDB3\Search\Creator;
-use CultuurNet\UDB3\Search\ElasticSearch\JsonDocument\Properties\Url;
 use CultuurNet\UDB3\Search\GeoBoundsParameters;
 use CultuurNet\UDB3\Search\GeoDistanceParameters;
 use CultuurNet\UDB3\Search\Label\LabelName;
@@ -39,10 +38,10 @@ public function withAutoCompleteFilter(string $input): MockOrganizerQueryBuilder
         return $c;
     }
 
-    public function withWebsiteFilter(Url $url): MockOrganizerQueryBuilder
+    public function withWebsiteFilter(string $url): MockOrganizerQueryBuilder
     {
         $c = clone $this;
-        $c->mockQuery['website'] = $url->toString();
+        $c->mockQuery['website'] = $url;
         return $c;
     }
 

diff --git a/tests/Http/OrganizerSearchControllerTest.php b/tests/Http/OrganizerSearchControllerTest.php
@@ -7,7 +7,6 @@
 use CultuurNet\UDB3\Search\Address\PostalCode;
 use CultuurNet\UDB3\Search\Country;
 use CultuurNet\UDB3\Search\Creator;
-use CultuurNet\UDB3\Search\ElasticSearch\JsonDocument\Properties\Url;
 use CultuurNet\UDB3\Search\Facet\FacetFilter;
 use CultuurNet\UDB3\Search\Facet\FacetNode;
 use CultuurNet\UDB3\Search\Geocoding\Coordinate\Coordinates;
@@ -122,7 +121,7 @@ public function it_returns_a_paged_collection_of_search_results_based_on_request
                 new Language('nl'),
                 new Language('en')
             )
-            ->withWebsiteFilter(new Url('http://foo.bar'))
+            ->withWebsiteFilter('http://foo.bar')
             ->withDomainFilter('www.publiq.be')
             ->withPostalCodeFilter(new PostalCode('3000'))
             ->withAddressCountryFilter(new Country('NL'))