-
Notifications
You must be signed in to change notification settings - Fork 92
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -813,4 +813,45 @@ describe("stream", () => { | |
| ]); | ||
| }); | ||
| }); | ||
|
|
||
| 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); | ||
| }); | ||
|
Comment on lines
+817
to
+855
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,10 +82,10 @@ function splitRange( | |
| ) => { | ||
| const range = commonPrefix.slice(); | ||
| let i = 0; | ||
| for (; i < key.length - 1; i++) { | ||
| for (; i < key.length - 1 && i < indexFields.length; i++) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| range.push(["eq", indexFields[i]!, key[i]!]); | ||
| } | ||
| if (i < key.length) { | ||
| if (i < key.length && i < indexFields.length) { | ||
| range.push([boundType, indexFields[i]!, key[i]!]); | ||
| } | ||
| return range; | ||
|
|
@@ -111,12 +111,15 @@ function splitRange( | |
| middleRange = makeCompare(startBoundType, startBound); | ||
| } else if (startBound.length === 0) { | ||
| middleRange = makeCompare(endBoundType, endBound); | ||
| } else { | ||
| } else if (indexFields.length > 0) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| const startValue = startBound[0]!; | ||
| const endValue = endBound[0]!; | ||
| middleRange = commonPrefix.slice(); | ||
| middleRange.push([startBoundType, indexFields[0]!, startValue]); | ||
| middleRange.push([endBoundType, indexFields[0]!, endValue]); | ||
| } else { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 - |
||
| // All fields are in the common prefix, use it as the middle range | ||
| middleRange = commonPrefix.slice(); | ||
| } | ||
| const ranges = [...startRanges, middleRange, ...endRanges]; | ||
| if (order === "desc") { | ||
|
|
||
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.
👍