From a1f881c16b7eb122506d4d0871289e518a783a3f Mon Sep 17 00:00:00 2001 From: Daniele Barbaro Date: Mon, 19 Oct 2026 14:27:38 +0200 Subject: [PATCH] fix(query): throw an http 400 for a malformed filter value --- README.md | 4 +- docs/querying.md | 22 ++++---- docs/upgrading.md | 9 +-- docs/values.md | 2 +- .../InvalidFilterValueException.php | 22 ++++++++ src/Query/CustomFieldFilter.php | 45 ++++++++++----- tests/Feature/FilterValueExceptionTest.php | 56 +++++++++++++++++++ 7 files changed, 126 insertions(+), 34 deletions(-) create mode 100644 src/Exceptions/InvalidFilterValueException.php create mode 100644 tests/Feature/FilterValueExceptionTest.php diff --git a/README.md b/README.md index 6f281f3..44fd6ab 100644 --- a/README.md +++ b/README.md @@ -181,8 +181,8 @@ SQLite, and on PHP 8.4 against MySQL 8.4 and PostgreSQL 17. `value_text` and `value_json` are not indexed. - Deleting a host model deletes its values, but a bulk delete through the query builder fires no model events and leaves them behind. -- An invalid filter value throws Spatie's `InvalidFilterValue`, which is not an HTTP - exception and renders as a 500 until you map it to a 400. +- An invalid filter value throws `PlinCode\CustomFields\Exceptions\InvalidFilterValueException`, + a Spatie `InvalidFilterValue` that Laravel renders as a 400 out of the box. - Stored values are not encrypted. - On SQLite a `decimal` has floating point affinity, so very long values lose precision. diff --git a/docs/querying.md b/docs/querying.md index 164aaeb..196f879 100644 --- a/docs/querying.md +++ b/docs/querying.md @@ -137,7 +137,7 @@ Values for `equals`, `in`, `greater_than`, `less_than` and `between` are validat the `rules()` of the field type, then normalised through its `serialize()`, exactly like a write. A value that fails, an empty element inside a list, a list given to `greater_than` or `less_than`, and a `between` with anything other than two values all throw -`Spatie\QueryBuilder\Exceptions\InvalidFilterValue`. So `filter[cf_age]=abc` on a `number`, +`PlinCode\CustomFields\Exceptions\InvalidFilterValueException`. So `filter[cf_age]=abc` on a `number`, `filter[cf_date-of-birth]=2024-13-45`, `filter[cf_risk-level]=closed` on a `select` without that key, `filter[cf_age]=1,,2` and `filter[cf_date-of-birth:between]=,2024-01-31` are all refused instead of matching the wrong rows. An empty `contains` value, such as @@ -148,19 +148,17 @@ validated this way. A filter sent with no value at all, such as `filter[cf_age]=`, is different. Laravel's `ConvertEmptyStringsToNull` middleware turns the empty query string value into `null`, and Spatie skips a filter whose value is `null`, so the filter is ignored silently. It is -refused with `InvalidFilterValue` only when your application removes that middleware. +refused with `InvalidFilterValueException` only when your application removes that middleware. -Unlike `InvalidFilterQuery`, `InvalidFilterValue` is not an HTTP exception, so Laravel -renders it as a 500 unless you map it. Map it to a 400 in `bootstrap/app.php`: +A `null` element inside a list is refused too. Under the default middleware +`filter[cf_age][]=` reaches the filter as `[null]`, and so does the empty bound of +`filter[cf_age:between][]=1&filter[cf_age:between][]=`. Only a bare `null`, the whole +filter value, is skipped. -```php -use Illuminate\Foundation\Configuration\Exceptions; -use Illuminate\Http\JsonResponse; -use Spatie\QueryBuilder\Exceptions\InvalidFilterValue; - -// inside withExceptions(function (Exceptions $exceptions) { ... }) -$exceptions->render(fn (InvalidFilterValue $e): JsonResponse => new JsonResponse(['message' => $e->getMessage()], 400)); -``` +`InvalidFilterValueException` extends Spatie's `InvalidFilterValue`, so an existing +`catch (InvalidFilterValue)` keeps working, and it implements Symfony's +`HttpExceptionInterface` with status 400, like Spatie's `InvalidFilterQuery`. Laravel +renders it as a 400 response out of the box, with no mapping in `bootstrap/app.php`. A date or datetime filter value goes through the same normalisation as a stored value, so `filter[cf_date-of-birth]=03/02/1985` matches a record stored as `1985-03-02`, and a diff --git a/docs/upgrading.md b/docs/upgrading.md index 83698db..00b08d8 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -75,17 +75,18 @@ they do not reload the host, which no longer exists by then. Values for `equals`, `in`, `greater_than`, `less_than` and `between` are validated against the rules of the field type before they reach the query. A failing value, or an empty -element inside a list such as `1,,2`, throws Spatie's `InvalidFilterValue`. Before, +element inside a list such as `1,,2`, throws `InvalidFilterValueException`. Before, `?filter[cf_age]=abc` matched the records whose age is `0`, and a malformed date or decimal failed inside PostgreSQL with a 500. A filter with no value at all, `?filter[cf_age]=`, is turned into `null` by Laravel's `ConvertEmptyStringsToNull` middleware and ignored by -Spatie; only an application without that middleware gets `InvalidFilterValue` for it. A +Spatie; only an application without that middleware gets `InvalidFilterValueException` for it. A `select` filter with a key outside the options, a `boolean` filter such as `maybe` and a `number` filter such as `1.5` are refused too, instead of returning an empty result or a silently cast one. -`InvalidFilterValue` is not an HTTP exception, so it renders as a 500 unless you map it to -a 400, see [invalid filter values](querying.md#invalid-filter-values). +`InvalidFilterValueException` extends Spatie's `InvalidFilterValue` and is an HTTP exception +with status 400, so Laravel renders it as a 400 without a mapping, see +[invalid filter values](querying.md#invalid-filter-values). ## Typed column indexes (0.7.0) diff --git a/docs/values.md b/docs/values.md index 1d17d08..8649412 100644 --- a/docs/values.md +++ b/docs/values.md @@ -170,7 +170,7 @@ A value that passes is normalised before it is stored: | `InvalidArgumentException` | The `CustomFieldFilter` and `CustomFieldSorter` constructors | An operation outside the ten filter operations, an operation the type does not declare, or a sort on a type that does not declare `sort`. | 500 | | `InvalidArgumentException` | The service provider | `key_type` or `morph_key_type` is not `id`, `uuid` or `ulid`. | The application fails at boot | | `Illuminate\Database\UniqueConstraintViolationException` | `CustomField::create()`, `update()` | Two definitions of the same entity are given the same name, on create or by renaming one, or an explicit slug that is already taken. The uniqueness is enforced by the database. | 409 or 422, after you catch it | -| `Spatie\QueryBuilder\Exceptions\InvalidFilterValue` | Request filters | A filter value fails the rules of the field type, a list holds an empty element, or the value has the wrong count for its operation. A filter with no value at all is ignored under Laravel's default middleware. | 400, once you map it, see [invalid filter values](querying.md#invalid-filter-values) | +| `PlinCode\CustomFields\Exceptions\InvalidFilterValueException` | Request filters | A filter value fails the rules of the field type, a list holds an empty element, or the value has the wrong count for its operation. A filter with no value at all is ignored under Laravel's default middleware. | 400, see [invalid filter values](querying.md#invalid-filter-values) | `UnknownCustomFieldException` extends `InvalidArgumentException` and `ModelNotPersistedException` extends `RuntimeException`, so an existing catch on the parent diff --git a/src/Exceptions/InvalidFilterValueException.php b/src/Exceptions/InvalidFilterValueException.php new file mode 100644 index 0000000..95eba6c --- /dev/null +++ b/src/Exceptions/InvalidFilterValueException.php @@ -0,0 +1,22 @@ + */ + public function getHeaders(): array + { + return []; + } +} diff --git a/src/Query/CustomFieldFilter.php b/src/Query/CustomFieldFilter.php index be2f00e..e1ff58b 100644 --- a/src/Query/CustomFieldFilter.php +++ b/src/Query/CustomFieldFilter.php @@ -11,8 +11,8 @@ use Illuminate\Support\Facades\Validator; use InvalidArgumentException; use PlinCode\CustomFields\Contracts\FieldType; +use PlinCode\CustomFields\Exceptions\InvalidFilterValueException; use PlinCode\CustomFields\Models\CustomField; -use Spatie\QueryBuilder\Exceptions\InvalidFilterValue; use Spatie\QueryBuilder\Filters\Filter; /** @@ -161,10 +161,6 @@ private function applyContains(Builder $inner, string $column, mixed $value): vo $pattern = new Expression("? escape '".self::LIKE_ESCAPE."'"); $items = array_map($this->text(...), $this->many($value)); - if (in_array('', $items, true)) { - throw InvalidFilterValue::make($this->describe($value)); - } - $inner->where(function (Builder $group) use ($column, $pattern, $items): void { foreach ($items as $item) { $group->orWhere($column, 'like', $pattern) @@ -248,7 +244,7 @@ private function many(mixed $value): array private function single(mixed $value): mixed { if (is_array($value)) { - throw InvalidFilterValue::make($this->describe($value)); + throw InvalidFilterValueException::make($this->describe($value)); } return $value; @@ -260,7 +256,7 @@ private function range(mixed $value): array $values = is_array($value) ? array_values($value) : explode(',', $this->text($value)); if (count($values) !== 2) { - throw InvalidFilterValue::make($this->describe($value)); + throw InvalidFilterValueException::make($this->describe($value)); } return $this->castMany($values); @@ -272,20 +268,18 @@ private function cast(mixed $value): mixed return null; } - if ($value === '') { - throw InvalidFilterValue::make($this->describe($value)); - } + $this->requireValue($value); $validator = Validator::make(['value' => $value], ['value' => $this->fieldType()->rules($this->field)]); if ($validator->fails()) { - throw InvalidFilterValue::make($this->describe($value)); + throw InvalidFilterValueException::make($this->describe($value)); } try { return $this->fieldType()->serialize($value, $this->field); } catch (InvalidFormatException) { - throw InvalidFilterValue::make($this->describe($value)); + throw InvalidFilterValueException::make($this->describe($value)); } } @@ -295,16 +289,37 @@ private function cast(mixed $value): mixed */ private function castMany(array $values): array { - return array_values(array_map($this->cast(...), $values)); + return array_values(array_map(function (mixed $value): mixed { + if ($value === null) { + throw InvalidFilterValueException::make($this->describe($value)); + } + + return $this->cast($value); + }, $values)); } private function text(mixed $value): string { if (! is_scalar($value)) { - throw InvalidFilterValue::make($this->describe($value)); + throw InvalidFilterValueException::make($this->describe($value)); + } + + return $this->requireValue((string) $value); + } + + /** + * @template T + * + * @param T $value + * @return T + */ + private function requireValue(mixed $value): mixed + { + if ($value === '') { + throw InvalidFilterValueException::make($this->describe($value)); } - return (string) $value; + return $value; } private function describe(mixed $value): string diff --git a/tests/Feature/FilterValueExceptionTest.php b/tests/Feature/FilterValueExceptionTest.php new file mode 100644 index 0000000..d5cad65 --- /dev/null +++ b/tests/Feature/FilterValueExceptionTest.php @@ -0,0 +1,56 @@ + 'article', 'name' => 'Age', 'type' => 'number']); + CustomField::create(['entity_type' => 'article', 'name' => 'Due', 'type' => 'date']); + + Route::get('/articles', fn (Request $request) => QueryBuilder::for(Article::class, $request) + ->allowedFilters(...CustomFields::filtersFor(Article::class)) + ->pluck('title'))->middleware(ConvertEmptyStringsToNull::class); +}); + +it('is a spatie invalid filter value that renders as an http 400', function (): void { + $exception = InvalidFilterValueException::make('abc'); + + expect($exception)->toBeInstanceOf(InvalidFilterValue::class) + ->toBeInstanceOf(HttpExceptionInterface::class) + ->and($exception->getStatusCode())->toBe(400) + ->and($exception->getHeaders())->toBe([]) + ->and($exception->getMessage())->toBe(InvalidFilterValue::make('abc')->getMessage()); +}); + +it('answers a malformed filter value with a 400 and a valid one with a 200', function (): void { + Article::create(['title' => 'adult'])->setCustomFields(['age' => 1]); + + $this->getJson('/articles?filter[cf_age]=abc')->assertStatus(400); + $this->getJson('/articles?filter[cf_age]=1')->assertOk()->assertExactJson(['adult']); +}); + +it('answers a bad between bound with a 400', function (): void { + $this->getJson('/articles?filter[cf_due:between]=2024-01-01,zzz')->assertStatus(400); + $this->getJson('/articles?filter[cf_due:between]=2024-01-01,2024-12-31')->assertOk(); +}); + +it('answers a null element inside a list with a 400', function (): void { + Article::create(['title' => 'adult'])->setCustomFields(['age' => 1]); + + $this->getJson('/articles?filter[cf_age][]=')->assertStatus(400); + $this->getJson('/articles?filter[cf_age:in][]=1&filter[cf_age:in][]=')->assertStatus(400); + $this->getJson('/articles?filter[cf_age:between][]=1&filter[cf_age:between][]=')->assertStatus(400); + $this->getJson('/articles?filter[cf_age][]=1&filter[cf_age][]=2')->assertOk()->assertExactJson(['adult']); +});