-
Notifications
You must be signed in to change notification settings - Fork 78
fix: prevent 'Cannot use gte on field undefined' error in splitRange #907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
When using eq() with undefined on a compound index followed by pagination, the splitRange function would attempt to access indexFields[i] beyond the array bounds. This occurred because the commonPrefix loop slices indexFields while processing equality constraints, but makeCompare and the middleRange code didn't check array bounds before accessing indexFields[i]. This fix adds bounds checking in: 1. makeCompare loop and condition (lines 85, 88) 2. middleRange construction (line 116) Also adds a regression test for pagination with eq(undefined) on a compound index to prevent this issue from recurring. Fixes the error: 'Cannot use gte on field "undefined"'
📝 WalkthroughWalkthroughThis change fixes a pagination bug in the stream helper where querying with Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/convex-helpers/server/stream.test.ts`:
- Around line 817-856: Prettier formatting failed for the new test "pagination
with eq(undefined) on compound index"; run your project's Prettier (or
formatter) on packages/convex-helpers/server/stream.test.ts and reformat the new
test block so indentation and spacing match the repo style (normalize
indentation around the test definition and the block using defineSchema,
defineTable, convexTest, stream, and query.paginate); then re-run lint/CI to
ensure the file passes formatting checks before reopening the PR.
| test("pagination with eq(undefined) on compound index", async () => { | ||
| // Regression test for bug where eq(field, undefined) on a compound index | ||
| // would cause "Cannot use gte on field 'undefined'" error during pagination | ||
| const testSchema = defineSchema({ | ||
| items: defineTable({ | ||
| softDeletedAt: v.optional(v.number()), | ||
| updatedAt: v.number(), | ||
| }).index("by_active_updated", ["softDeletedAt", "updatedAt"]), | ||
| }); | ||
| const t = convexTest(testSchema, modules); | ||
| await t.run(async (ctx) => { | ||
| // Insert some active (non-deleted) items | ||
| await ctx.db.insert("items", { updatedAt: 1 }); | ||
| await ctx.db.insert("items", { updatedAt: 2 }); | ||
| await ctx.db.insert("items", { updatedAt: 3 }); | ||
| // Insert some soft-deleted items | ||
| await ctx.db.insert("items", { softDeletedAt: 100, updatedAt: 4 }); | ||
|
|
||
| const query = stream(ctx.db, testSchema) | ||
| .query("items") | ||
| .withIndex("by_active_updated", (q) => q.eq("softDeletedAt", undefined)) | ||
| .order("desc"); | ||
|
|
||
| // This should not throw "Cannot use gte on field 'undefined'" | ||
| const result = await query.paginate({ numItems: 2, cursor: null }); | ||
| expect(result.page.length).toBe(2); | ||
| expect(result.page[0]!.updatedAt).toBe(3); | ||
| expect(result.page[1]!.updatedAt).toBe(2); | ||
| expect(result.isDone).toBe(false); | ||
|
|
||
| // Continue pagination | ||
| const page2 = await query.paginate({ | ||
| numItems: 2, | ||
| cursor: result.continueCursor, | ||
| }); | ||
| expect(page2.page.length).toBe(1); | ||
| expect(page2.page[0]!.updatedAt).toBe(1); | ||
| expect(page2.isDone).toBe(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix Prettier failure in the new regression test.
CI reports a Prettier formatting failure for this file; please run Prettier (or your formatter) to normalize the indentation in this new test block.
🤖 Prompt for AI Agents
In `@packages/convex-helpers/server/stream.test.ts` around lines 817 - 856,
Prettier formatting failed for the new test "pagination with eq(undefined) on
compound index"; run your project's Prettier (or formatter) on
packages/convex-helpers/server/stream.test.ts and reformat the new test block so
indentation and spacing match the repo style (normalize indentation around the
test definition and the block using defineSchema, defineTable, convexTest,
stream, and query.paginate); then re-run lint/CI to ensure the file passes
formatting checks before reopening the PR.
|
should I review even though it's a draft? |
|
yes please! I was hoping to get the prettier pushed but haven't had a second to do that yet |
| const range = commonPrefix.slice(); | ||
| let i = 0; | ||
| for (; i < key.length - 1; i++) { | ||
| for (; i < key.length - 1 && i < indexFields.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would key be longer than indexFields? That seems like a bug - unless they indexFields don't include _creationTime, _id, which would be important to respect here. If we want to add more safety let's assert that it's the case, vs. not constraining the range as much as expected
| }); | ||
| }); | ||
|
|
||
| test("pagination with eq(undefined) on compound index", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test passes without the proposed changes, so I think we need another test that previously failed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| } else if (startBound.length === 0) { | ||
| middleRange = makeCompare(endBoundType, endBound); | ||
| } else { | ||
| } else if (indexFields.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, not sure how this could happen, so would rather assert - not sure how we would get here
| middleRange = commonPrefix.slice(); | ||
| middleRange.push([startBoundType, indexFields[0]!, startValue]); | ||
| middleRange.push([endBoundType, indexFields[0]!, endValue]); | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would love to have some unit tests just on splitRange to validate that it's doing what it's supposed to for the four variants we expect here -

When using eq() with undefined on a compound index followed by pagination,
the splitRange function would attempt to access indexFields[i] beyond the
array bounds. This occurred because the commonPrefix loop slices indexFields
while processing equality constraints, but makeCompare and the middleRange
code didn't check array bounds before accessing indexFields[i].
This fix adds bounds checking in:
Also adds a regression test for pagination with eq(undefined) on a
compound index to prevent this issue from recurring.
Fixes the error: 'Cannot use gte on field "undefined"'
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Summary by CodeRabbit
Bug Fixes
Tests