Skip to content

fix: handle nullable filter edge cases for empty and not-null values#1051

Merged
freekmurze merged 2 commits intospatie:mainfrom
isaackaara:fix/nullable-filter-edge-cases
Mar 16, 2026
Merged

fix: handle nullable filter edge cases for empty and not-null values#1051
freekmurze merged 2 commits intospatie:mainfrom
isaackaara:fix/nullable-filter-edge-cases

Conversation

@isaackaara
Copy link
Contributor

Summary

Fixes two edge cases in FiltersOperator when using dynamic operator filters with ->nullable():

Edge Case 1: Empty filter value

Query: ?filter[my_filter]=
Before: Operator resolved to empty string ""
After: Operator defaults to FilterOperator::EQUAL, producing WHERE column IS NULL

Edge Case 2: Not-null filter with empty value

Query: ?filter[my_filter]=<>
Before: Value was empty string "", producing WHERE column <> ''
After: Value converts to null, producing WHERE column IS NOT NULL

Changes

  • Added elseif clause to default to EQUAL operator when value is null in dynamic filter mode
  • Added post-processing to convert empty string values to null
  • Added two Pest tests covering both scenarios

Credits

Both issues were identified and diagnosed by @pimdongit in #1047 (comment). They provided the proposed fixes -- this PR implements them with test coverage.

Ref: #1047

Fixes two edge cases in FiltersOperator when using dynamic operator filters
with nullable():

1. Empty filter value (?filter[x]=) now defaults to EQUAL operator instead
   of empty string, correctly producing WHERE x IS NULL
2. Not-null filter (?filter[x]=<>) now correctly uses null value instead of
   empty string, producing WHERE x IS NOT NULL

Both issues were identified by @pimdongit in spatie#1047.

Ref: spatie#1047 (comment)
@isaackaara isaackaara force-pushed the fix/nullable-filter-edge-cases branch from 0649f9c to 631bebd Compare March 13, 2026 14:15
Copy link
Member

@freekmurze freekmurze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR and the tests! The edge cases you're fixing are valid.

However, the if ($value === '') { $value = null; } check on line 46-48 is unconditional, meaning it applies to all operator filters, not just dynamic ones. This could change behavior for non-dynamic filters:

For example, AllowedFilter::operator('name', FilterOperator::EQUAL) with ?filter[name]=:

  • Before: WHERE name = '' (filter for empty string)
  • After: WHERE name IS NULL (different semantics)

The empty-string-to-null conversion should only happen for dynamic filters, where it's the result of stripping the operator prefix (e.g., <> stripped from <> leaves '').

Could you move the check inside the dynamic filter branch instead?

} elseif ($this->filterOperator->isDynamic() && $value !== null) {
    $filterOperator = $this->getDynamicFilterOperator($value);
    $this->removeDynamicFilterOperatorFromValue($value, $filterOperator);
    if ($value === '') {
        $value = null;
    }
} elseif ($this->filterOperator->isDynamic() && $value === null) {
    $filterOperator = FilterOperator::EQUAL;
}

Per maintainer feedback, the conversion should only apply when a dynamic
operator prefix is stripped (e.g. '<>' stripped from '<>' leaves ''),
not for all operator filters. Moves the check inside the isDynamic &&
value !== null branch.
@isaackaara
Copy link
Contributor Author

Thanks for the clear explanation, @freekmurze. You're right that the unconditional check would change semantics for non-dynamic filters like AllowedFilter::operator('name', FilterOperator::EQUAL) with an empty string value.

Moved the empty-string-to-null conversion inside the isDynamic() && $value !== null branch, so it only fires after stripping the operator prefix leaves an empty string. All existing and new tests pass.

Copy link
Member

@freekmurze freekmurze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the fix and the tests!

@freekmurze freekmurze merged commit 8ec7246 into spatie:main Mar 16, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants