Return HTTP 400 for malformed date-range filter input

CHANGELOG.md

@@ -8,6 +8,8 @@ See [keep a changelog] for information about writing changes to this log.
 
 ## [Unreleased]
 
+- [PR-42](https://github.com/itk-dev/event-database-api/pull/42)
+  Return HTTP 400 (not 500) for malformed date-range filter input; unskip FilterErrorTest
 - [PR-41](https://github.com/itk-dev/event-database-api/pull/41)
   Run the Code Review workflow via docker compose directly (drop Task) with vendor caching and image pre-pull
 - [PR-40](https://github.com/itk-dev/event-database-api/pull/40)

config/packages/api_platform.yaml

@@ -60,7 +60,7 @@ api_platform:
     exception_to_status:
         # The 4 following handlers are registered by default, keep those lines to prevent unexpected side effects
         Symfony\Component\Serializer\Exception\ExceptionInterface: 400 # Use a raw status code (recommended)
-        ApiPlatform\Exception\InvalidArgumentException: !php/const Symfony\Component\HttpFoundation\Response::HTTP_BAD_REQUEST
+        ApiPlatform\Metadata\Exception\InvalidArgumentException: !php/const Symfony\Component\HttpFoundation\Response::HTTP_BAD_REQUEST
         ApiPlatform\ParameterValidator\Exception\ValidationExceptionInterface: 400
         Doctrine\ORM\OptimisticLockException: 409
 

phpstan-baseline.neon

@@ -1,11 +1,5 @@
 parameters:
 	ignoreErrors:
-		-
-			message: '#^Class constant name in dynamic fetch can only be a string, int\|string\|null given\.$#'
-			identifier: classConstant.nameType
-			count: 1
-			path: src/Api/Filter/ElasticSearch/DateRangeFilter.php
-
 		-
 			message: '#^Offset ''_source'' might not exist on array\|null\.$#'
 			identifier: offsetAccess.notFound

src/Api/Filter/ElasticSearch/DateRangeFilter.php

@@ -3,6 +3,7 @@
 namespace App\Api\Filter\ElasticSearch;
 
 use ApiPlatform\Elasticsearch\Filter\AbstractFilter;
+use ApiPlatform\Metadata\Exception\InvalidArgumentException;
 use ApiPlatform\Metadata\Operation;
 use ApiPlatform\Metadata\Property\Factory\PropertyMetadataFactoryInterface;
 use ApiPlatform\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
@@ -85,12 +86,17 @@ private function getElasticSearchQueryRanges(string $property, string|array $fil
         if (null === $this->properties) {
             throw new \InvalidArgumentException('The property must be defined in the filter.');
         }
+
+        $throwOnInvalid = $this->config[$this->properties[$property]]->throwOnInvalid;
+
         if (!\is_array($filter)) {
-            $fallbackOperator = $this->properties[$property];
-            $operator = $this->config[$fallbackOperator]->limit;
+            $operator = $this->config[$this->properties[$property]]->limit;
             $value = $filter;
         } else {
-            $operator = DateLimit::{array_key_first($filter)};
+            $operator = $this->resolveOperator((string) array_key_first($filter), $throwOnInvalid);
+            if (null === $operator) {
+                return [];
+            }
             $value = array_shift($filter);
         }
 
@@ -99,7 +105,11 @@ private function getElasticSearchQueryRanges(string $property, string|array $fil
                 $values = explode('..', $value);
 
                 if (2 !== count($values)) {
-                    throw new \InvalidArgumentException('Invalid date range');
+                    if ($throwOnInvalid) {
+                        throw new InvalidArgumentException(sprintf('Invalid date range for "%s": expected two ISO 8601 datetimes separated by "..".', $property));
+                    }
+
+                    return [];
                 }
 
                 return [
@@ -126,6 +136,28 @@ private function getElasticSearchQueryRanges(string $property, string|array $fil
         }
     }
 
+    /**
+     * Resolve a client-supplied operator key (e.g. "gt") to a DateLimit case.
+     *
+     * DateLimit case names are the operators the client uses in `field[op]=…`.
+     * Returns null for an unknown key when the filter is not configured to
+     * throw, so the caller can skip the clause instead of leaking a 5xx.
+     */
+    private function resolveOperator(string $key, bool $throwOnInvalid): ?DateLimit
+    {
+        foreach (DateLimit::cases() as $case) {
+            if ($case->name === $key) {
+                return $case;
+            }
+        }
+
+        if ($throwOnInvalid) {
+            throw new InvalidArgumentException(sprintf('Unknown date range operator "%s".', $key));
+        }
+
+        return null;
+    }
+
     private function getFilterDescription(string $fieldName, DateLimit $operator, bool $isDefault = false): array
     {
         $propertyName = $this->normalizePropertyName($fieldName);

tests/ApiPlatform/FilterErrorTest.php

@@ -9,9 +9,11 @@
  *
  * The DateRangeFilter on the Event, Occurrence, DailyOccurrence, Location and
  * Organization resources is configured with `throwOnInvalid: true`, so a
- * malformed value must surface as a client error (4xx) rather than a 5xx leak,
- * and the response body must follow RFC 7807 (problem+json) per
- * `rfc_7807_compliant_errors: true` in api_platform.yaml.
+ * malformed value (unparseable `between`, unknown operator) surfaces as a
+ * client error (4xx) rather than a 5xx leak: the filter throws ApiPlatform's
+ * InvalidArgumentException, mapped to HTTP 400 via `exception_to_status`. A
+ * non-date value passes the filter but is rejected by Elasticsearch, coming
+ * back as an ElasticIndexException (also mapped to 400).
  */
 class FilterErrorTest extends AbstractApiTestCase
 {
@@ -20,15 +22,6 @@ class FilterErrorTest extends AbstractApiTestCase
     #[DataProvider('invalidDateRangeProvider')]
     public function testInvalidDateRangeReturnsClientError(string $path, array $query, string $message): void
     {
-        // TODO: DateRangeFilter::getElasticSearchQueryRanges() throws PHP's
-        // native \InvalidArgumentException, which is not in api_platform.yaml's
-        // exception_to_status map, so the framework returns 500 instead of 400.
-        // Either map \InvalidArgumentException -> 400, or throw
-        // ApiPlatform\Exception\InvalidArgumentException. Once fixed, remove
-        // this skip — the assertions below already encode the desired contract.
-        $this->markTestSkipped('Known contract bug: malformed date range returns 5xx. See TODO.');
-
-        // @phpstan-ignore-next-line deadCode.unreachable
         $response = $this->get($query, $path);
         $statusCode = $response->getStatusCode();
         $this->assertGreaterThanOrEqual(400, $statusCode, $message.': '.$statusCode);
@@ -37,7 +30,7 @@ public function testInvalidDateRangeReturnsClientError(string $path, array $quer
 
     public static function invalidDateRangeProvider(): iterable
     {
-        // Missing '..' separator → DateRangeFilter throws \InvalidArgumentException('Invalid date range').
+        // Missing '..' separator → DateRangeFilter throws InvalidArgumentException.
         yield 'events: missing separator' => [
             '/api/v2/events',
             ['occurrences.start[between]' => '2024-01-01T00:00:00+00:00'],
@@ -51,6 +44,22 @@ public static function invalidDateRangeProvider(): iterable
             'Between filter with three segments',
         ];
 
+        // Unknown operator → DateRangeFilter can no longer resolve the DateLimit
+        // case and throws InvalidArgumentException (used to be a raw \Error → 500).
+        yield 'events: unknown operator' => [
+            '/api/v2/events',
+            ['occurrences.start[whenever]' => '2024-01-01T00:00:00+00:00'],
+            'Unknown date range operator',
+        ];
+
+        // Non-date value passes the filter but Elasticsearch cannot parse it →
+        // ElasticIndexException (mapped to 400).
+        yield 'events: non-date value' => [
+            '/api/v2/events',
+            ['occurrences.start[gte]' => 'not-a-date'],
+            'Non-date value for a date range filter',
+        ];
+
         yield 'occurrences: missing separator' => [
             '/api/v2/occurrences',
             ['start[between]' => '2024-01-01T00:00:00+00:00'],

tests/Unit/Api/Filter/ElasticSearch/DateRangeFilterTest.php

@@ -2,16 +2,19 @@
 
 namespace App\Tests\Unit\Api\Filter\ElasticSearch;
 
+use ApiPlatform\Metadata\Exception\InvalidArgumentException;
 use App\Api\Filter\ElasticSearch\DateRangeFilter;
 use App\Model\DateLimit;
 use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\TestCase;
 
 /**
- * Pins DateRangeFilter::apply() query DSL and its (currently un-mapped) error
- * behaviour. `between` produces exclusive `gt`/`lt` bounds — a consumer-visible
- * semantic. The malformed-input and unknown-operator cases document current
- * behaviour: both surface as uncaught throwables (they leak as HTTP 500 today).
+ * Pins DateRangeFilter::apply() query DSL and its error behaviour. `between`
+ * produces exclusive `gt`/`lt` bounds — a consumer-visible semantic. Malformed
+ * and unknown-operator input, when the filter is configured with
+ * `throwOnInvalid: true`, throws ApiPlatform's InvalidArgumentException — mapped
+ * to HTTP 400 via `exception_to_status` — rather than leaking a native throwable
+ * as a 500. With `throwOnInvalid: false` the clause is skipped instead.
  */
 class DateRangeFilterTest extends TestCase
 {
@@ -69,56 +72,50 @@ public static function applyProvider(): iterable
         ];
     }
 
-    // Goal: pin that a malformed `between` value is currently an uncaught throwable
-    // (the error-contract fix will turn this into a 4xx).
+    // Goal: a malformed `between` value throws ApiPlatform's InvalidArgumentException
+    // (mapped to HTTP 400) rather than a native throwable that leaks as a 500.
     public function testMalformedBetweenThrows(): void
     {
         $filter = $this->newFilter(
             ['updated' => 'gte'],
             ['gte' => ['limit' => DateLimit::gte, 'throwOnInvalid' => true]],
         );
 
-        // No ".." separator → invalid range. Currently an uncaught \InvalidArgumentException
-        // (leaks as HTTP 500; the error-contract fix will map this to 4xx).
-        $this->expectException(\InvalidArgumentException::class);
+        // No ".." separator → invalid range.
+        $this->expectException(InvalidArgumentException::class);
         $filter->apply([], self::RESOURCE, null, ['filters' => ['updated' => ['between' => '2024-01-01T00:00:00+00:00']]]);
     }
 
-    // Goal: pin the second, distinct error leak — an unknown operator hits a native
-    // \Error — so the error-contract fix addresses both paths.
-    public function testUnknownOperatorThrowsError(): void
+    // Goal: an unknown operator (`updated[foo]=…`) throws ApiPlatform's
+    // InvalidArgumentException instead of the native \Error it used to leak.
+    public function testUnknownOperatorThrows(): void
     {
         $filter = $this->newFilter(
             ['updated' => 'gte'],
             ['gte' => ['limit' => DateLimit::gte, 'throwOnInvalid' => true]],
         );
 
-        // `updated[foo]=…` resolves DateLimit::{foo} → a native \Error (undefined enum
-        // case). Distinct from the malformed-between path; also leaks as HTTP 500 today.
-        $this->expectException(\Error::class);
+        $this->expectException(InvalidArgumentException::class);
         $filter->apply([], self::RESOURCE, null, ['filters' => ['updated' => ['foo' => 'x']]]);
     }
 
-    // Goal: document that the `throwOnInvalid` config flag is dead — invalid input
-    // throws whether it is true or false.
-    #[DataProvider('throwOnInvalidProvider')]
-    public function testThrowOnInvalidConfigIsNotConsulted(bool $throwOnInvalid): void
+    // Goal: `throwOnInvalid: false` makes invalid input skip the clause (empty DSL)
+    // instead of throwing — the flag is consulted for both invalid paths.
+    #[DataProvider('invalidInputProvider')]
+    public function testThrowOnInvalidFalseSkipsInvalidInput(array $filters): void
     {
-        // `throwOnInvalid` is stored in the config but never read: invalid input throws
-        // regardless of its value. This pins that the flag is currently dead.
         $filter = $this->newFilter(
             ['updated' => 'gte'],
-            ['gte' => ['limit' => DateLimit::gte, 'throwOnInvalid' => $throwOnInvalid]],
+            ['gte' => ['limit' => DateLimit::gte, 'throwOnInvalid' => false]],
         );
 
-        $this->expectException(\InvalidArgumentException::class);
-        $filter->apply([], self::RESOURCE, null, ['filters' => ['updated' => ['between' => 'no-separator']]]);
+        self::assertSame([], $filter->apply([], self::RESOURCE, null, ['filters' => $filters]));
     }
 
-    public static function throwOnInvalidProvider(): iterable
+    public static function invalidInputProvider(): iterable
     {
-        yield 'throwOnInvalid=true' => [true];
-        yield 'throwOnInvalid=false' => [false];
+        yield 'malformed between' => [['updated' => ['between' => 'no-separator']]];
+        yield 'unknown operator' => [['updated' => ['foo' => 'x']]];
     }
 
     // Goal: getDescription() advertises the default parameter plus every