diff --git a/CHANGELOG.md b/CHANGELOG.md index d403998..6aefa07 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/config/packages/api_platform.yaml b/config/packages/api_platform.yaml index 3b0081b..416fa4c 100644 --- a/config/packages/api_platform.yaml +++ b/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 diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 16d805c..28235fd 100644 --- a/phpstan-baseline.neon +++ b/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 diff --git a/src/Api/Filter/ElasticSearch/DateRangeFilter.php b/src/Api/Filter/ElasticSearch/DateRangeFilter.php index cb6a0ef..2dde5b4 100644 --- a/src/Api/Filter/ElasticSearch/DateRangeFilter.php +++ b/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); diff --git a/tests/ApiPlatform/FilterErrorTest.php b/tests/ApiPlatform/FilterErrorTest.php index 9ffe865..e16d05b 100644 --- a/tests/ApiPlatform/FilterErrorTest.php +++ b/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'], diff --git a/tests/Unit/Api/Filter/ElasticSearch/DateRangeFilterTest.php b/tests/Unit/Api/Filter/ElasticSearch/DateRangeFilterTest.php index fb8cfff..78cc677 100644 --- a/tests/Unit/Api/Filter/ElasticSearch/DateRangeFilterTest.php +++ b/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,8 +72,8 @@ 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( @@ -78,47 +81,41 @@ public function testMalformedBetweenThrows(): void ['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