fix(memory): exclude null rows from negation operators#869
Conversation
Memory adapter's negation operators (notEqual, notBetween, notStartsWith, notEndsWith, notContains, notSearch) previously matched rows where the attribute was null, diverging from MariaDB / MySQL / Postgres / SQLite which treat NULL under three-valued logic — `WHERE col != x` evaluates to NULL for null rows and excludes them from the result set. Same applies to object-typed notEqual / notContains: Postgres' NOT (NULL @> x) evaluates to NULL, excluding the row. Adds testNegationOperatorsExcludeNullRows covering all six scalar operators against a row with null-valued attributes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe Memory adapter's query matching logic for negated operators is updated to align with SQL three-valued logic, where Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.51)PHPStan was skipped because the sandbox runner could not parse its output. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Greptile SummaryThis PR fixes the Memory adapter's handling of null-valued rows under negation operators ( Confidence Score: 4/5Safe to merge; all scalar negation fixes are correct and the behavioural regression is well-targeted. Only P2 findings (missing test coverage for two JSONB-path cases). The production code changes are logically sound and consistent with SQL semantics across all touched operators. tests/e2e/Adapter/MemoryTest.php — JSONB/object-path branches for Important Files Changed
Reviews (1): Last reviewed commit: "fix(memory): exclude null rows from nega..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $assertOnlyValueRow('notEqual', $database->find('nullable', [ | ||
| Query::notEqual('name', 'bob'), | ||
| ])); | ||
|
|
||
| $assertOnlyValueRow('notBetween', $database->find('nullable', [ | ||
| Query::notBetween('score', 100, 200), | ||
| ])); | ||
|
|
||
| $assertOnlyValueRow('notStartsWith', $database->find('nullable', [ | ||
| Query::notStartsWith('name', 'zz'), | ||
| ])); | ||
|
|
||
| $assertOnlyValueRow('notEndsWith', $database->find('nullable', [ | ||
| Query::notEndsWith('name', 'zz'), | ||
| ])); | ||
|
|
||
| $assertOnlyValueRow('notContains', $database->find('nullable', [ | ||
| Query::notContains('bio', ['nope']), | ||
| ])); | ||
|
|
||
| $assertOnlyValueRow('notSearch', $database->find('nullable', [ | ||
| Query::notSearch('bio', 'unrelated'), | ||
| ])); | ||
| } | ||
| } |
There was a problem hiding this comment.
The PR also changes matchesObjectQuery for TYPE_NOT_EQUAL and TYPE_NOT_CONTAINS in the object/JSONB path (lines ~2945 and ~2984 of Memory.php), but no test in testNegationOperatorsExcludeNullRows exercises those branches. A document with a null JSON/array-typed attribute filtered with notEqual or notContains through the object path would go untested, leaving those two fixes without regression coverage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/Adapter/MemoryTest.php (1)
878-956: Add regression coverage for object/JSON negation paths changed in this PR.This test validates scalar negation semantics well, but it does not exercise
matchesObject()paths updated forTYPE_NOT_EQUALandTYPE_NOT_CONTAINS. Please add at least one null-vs-non-null object attribute case so both changed surfaces are guarded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/MemoryTest.php` around lines 878 - 956, Add a null-vs-non-null object/JSON attribute case in testNegationOperatorsExcludeNullRows so the updated matchesObject() paths for TYPE_NOT_EQUAL and TYPE_NOT_CONTAINS are exercised: extend the 'nullable' collection schema with an object/JSON field (e.g., "meta" or "settings"), insert one document with that field set to a non-null object and one with it set to null, then add assertions using Query::notEqual('meta', ...) and Query::notContains('meta', ...) (mirroring the existing notEqual/notContains calls) to assert only the non-null document is returned; this will cover the matchesObject() branches touched by TYPE_NOT_EQUAL and TYPE_NOT_CONTAINS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/Adapter/MemoryTest.php`:
- Around line 878-956: Add a null-vs-non-null object/JSON attribute case in
testNegationOperatorsExcludeNullRows so the updated matchesObject() paths for
TYPE_NOT_EQUAL and TYPE_NOT_CONTAINS are exercised: extend the 'nullable'
collection schema with an object/JSON field (e.g., "meta" or "settings"), insert
one document with that field set to a non-null object and one with it set to
null, then add assertions using Query::notEqual('meta', ...) and
Query::notContains('meta', ...) (mirroring the existing notEqual/notContains
calls) to assert only the non-null document is returned; this will cover the
matchesObject() branches touched by TYPE_NOT_EQUAL and TYPE_NOT_CONTAINS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 303ba14d-c74b-45e5-bbd4-970c693af995
📒 Files selected for processing (2)
src/Database/Adapter/Memory.phptests/e2e/Adapter/MemoryTest.php
1 participant
Summary
Greptile P1 follow-up to #860. The Memory adapter included null-valued rows in negation operator results, diverging from every SQL adapter:
notEqual,notBetween,notStartsWith,notEndsWith(scalar)notContains,notSearch(scalar)notEqual,notContains(object/JSONB context)In MariaDB / MySQL / Postgres / SQLite,
WHERE col != xevaluates to NULL for null rows under three-valued logic, so they are excluded. The Memory adapter was returningtrue(or relying on! matches(...)flippingfalsetotrue), causing null rows to leak into negation results.Each branch now short-circuits on
$value === null(or$haystack === nullin the object path) and returnsfalse, matching SQL semantics.Test plan
testNegationOperatorsExcludeNullRows— verifies all six scalar negation operators exclude a row with null-valued attributes🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
notEqual,notBetween,notStartsWith,notEndsWith,notContains,notSearch) to properly handle null values—these operators now return false when the queried value is null.Tests