feat: add $nin filter operator for exclusion queries - #12
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/filter.ts">
<violation number="1" location="src/filter.ts:126">
P1: In-memory `$nin` returns `true` for missing/null fields, but the SQL `NOT IN` returns `NULL` (falsy). This causes Upstash/Cloudflare drivers to include records that SQLite/PG drivers would exclude when the filtered field doesn't exist.
Add a guard to align with SQL semantics, similar to the numeric operators (`$gt`, `$lt`, etc.) which check `typeof actual`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/filter.test.ts (1)
352-362: Good coverage of$ninbehavior.Tests cover the essential cases. Consider adding a test for missing field behavior to document the expected semantics:
it('$nin matches when field is missing', () => { expect(matchesFilter({ x: { $nin: ['a', 'b'] } }, { other: 1 })).toBe(true) })This would clarify that missing fields pass
$ninfilters (sinceundefinedis not in the exclusion list).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/filter.test.ts` around lines 352 - 362, Add a unit test in test/filter.test.ts that asserts matchesFilter handles missing fields for $nin correctly: create a new it block (e.g., it('$nin matches when field is missing')) that calls matchesFilter({ x: { $nin: ['a', 'b'] } }, { other: 1 }) and expects true to document that undefined fields are not considered in the exclusion list; place it alongside the existing $nin tests near the matchesFilter tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/filter.ts`:
- Around line 62-67: The $nin SQL generation currently produces "ref NOT IN
(...)" which excludes rows where ref IS NULL, causing inconsistency with the
in-memory matchOp (which treats missing/undefined as included); update the
op.$nin branch in the filter generator so that when op.$nin is non-empty it
returns SQL that treats NULL as included — i.e., emit "(<ref> NOT IN (?, ...) OR
<ref> IS NULL)" with params copied from op.$nin (keep the existing empty-array
short-circuit returning "1 = 1"). Target the code handling op.$nin (the op.$nin
map/placeholder logic and returned sql/params) to make this change.
---
Nitpick comments:
In `@test/filter.test.ts`:
- Around line 352-362: Add a unit test in test/filter.test.ts that asserts
matchesFilter handles missing fields for $nin correctly: create a new it block
(e.g., it('$nin matches when field is missing')) that calls matchesFilter({ x: {
$nin: ['a', 'b'] } }, { other: 1 }) and expects true to document that undefined
fields are not considered in the exclusion list; place it alongside the existing
$nin tests near the matchesFilter tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 872ef335-acdd-4499-9a85-495fe131f68b
📒 Files selected for processing (3)
src/filter.tssrc/types.tstest/filter.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/filter.test.ts (1)
476-487: LGTM — Integration test validates end-to-end$ninbehavior.Consider adding a test with a document that has a missing
typefield to verify SQL NULL behavior at the integration level (optional).📋 Optional test enhancement
it('filters by $nin', async () => { const db = await sqliteFts({ path: ':memory:' }) await db.index([ { id: '1', content: 'hello world', metadata: { type: 'markdown' } }, { id: '2', content: 'hello earth', metadata: { type: 'code' } }, { id: '3', content: 'hello mars', metadata: { type: 'docs' } }, + { id: '4', content: 'hello void', metadata: { other: 'field' } }, // missing type ]) const results = await db.search('hello', { filter: { type: { $nin: ['code', 'docs'] } }, returnMetadata: true }) expect(results).toHaveLength(1) expect(results[0]!.id).toBe('1') + // id: '4' excluded due to NULL NOT IN (...) → NULL (falsy) await db.close?.() }),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/filter.test.ts` around lines 476 - 487, Add an integration assertion that verifies SQL NULL handling for $nin: modify the 'filters by $nin' test using sqliteFts() and db.search to include an additional document without metadata.type (e.g., { id: '4', content: 'hello void', metadata: {} } or no metadata) and assert whether it is returned by the $nin filter; update the expect checks to explicitly assert the presence/absence of that document (so the test documents SQL NULL behavior for db.search with filter: { type: { $nin: [...] } }).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/filter.test.ts`:
- Around line 476-487: Add an integration assertion that verifies SQL NULL
handling for $nin: modify the 'filters by $nin' test using sqliteFts() and
db.search to include an additional document without metadata.type (e.g., { id:
'4', content: 'hello void', metadata: {} } or no metadata) and assert whether it
is returned by the $nin filter; update the expect checks to explicitly assert
the presence/absence of that document (so the test documents SQL NULL behavior
for db.search with filter: { type: { $nin: [...] } }).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1a36001-a0c4-4e76-ab2f-f639c524429e
📒 Files selected for processing (2)
src/filter.tstest/filter.test.ts
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/filter.ts">
<violation number="1" location="src/filter.ts:126">
P2: The null guard breaks the `$nin: []` edge case for missing fields. SQL compiles `$nin: []` to `1 = 1` (matches everything including NULL fields), but the in-memory path now returns `false` when the field is absent. Short-circuit the empty-array case before the null check.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/filter.test.ts (1)
478-489: Prefertry/finallyfor DB cleanup in async integration test.If an assertion fails,
db.closewon’t run, which can leak handles and make test runs flaky.Suggested change
it('filters by $nin', async () => { const db = await sqliteFts({ path: ':memory:' }) - await db.index([ - { id: '1', content: 'hello world', metadata: { type: 'markdown' } }, - { id: '2', content: 'hello earth', metadata: { type: 'code' } }, - { id: '3', content: 'hello mars', metadata: { type: 'docs' } }, - ]) - const results = await db.search('hello', { filter: { type: { $nin: ['code', 'docs'] } }, returnMetadata: true }) - expect(results).toHaveLength(1) - expect(results[0]!.id).toBe('1') - await db.close?.() + try { + await db.index([ + { id: '1', content: 'hello world', metadata: { type: 'markdown' } }, + { id: '2', content: 'hello earth', metadata: { type: 'code' } }, + { id: '3', content: 'hello mars', metadata: { type: 'docs' } }, + ]) + const results = await db.search('hello', { filter: { type: { $nin: ['code', 'docs'] } }, returnMetadata: true }) + expect(results).toHaveLength(1) + expect(results[0]!.id).toBe('1') + } + finally { + await db.close?.() + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/filter.test.ts` around lines 478 - 489, The test for $nin should ensure the in-memory DB is always closed: wrap the async test body that creates sqliteFts() (the db variable used with db.index, db.search) in a try/finally so db.close is invoked in the finally block (call db.close?.() if present); keep the assertions inside the try and move the existing await db.close?.() into finally to guarantee cleanup even on assertion failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/filter.test.ts`:
- Around line 478-489: The test for $nin should ensure the in-memory DB is
always closed: wrap the async test body that creates sqliteFts() (the db
variable used with db.index, db.search) in a try/finally so db.close is invoked
in the finally block (call db.close?.() if present); keep the assertions inside
the try and move the existing await db.close?.() into finally to guarantee
cleanup even on assertion failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e83a9577-96e5-4ca9-a90e-c25f8bf4fb45
📒 Files selected for processing (2)
src/filter.tstest/filter.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/filter.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
$inhas been there from the start, but the complementary$nin(not in) was missing. Excluding a set of values is common enough that having to negate$inmanually felt off, so this adds$ninas a first-class filter operator.Covers SQL compilation for both SQLite (
json_extract) and PostgreSQL (->>), in-memorymatchesFilter, edge case for empty arrays ($nin: []matches everything, mirrors$in: []matching nothing), and an integration test against sqlite-fts.Linked Issues
Additional context
Follows the same pattern as
$inexactly. Empty$nin: []compiles to1 = 1(always true), which is the logical complement of$in: []compiling to1 = 0.Summary by CodeRabbit