Fix cursor-based pagination #16558
Fix cursor-based pagination #16558
Conversation
…on logic - Updated CommonFindManyQueryRunnerService to include orderBy argument. - Enhanced buildColumnsToSelect function to process orderBy input and extract relevant column names. - Introduced extractColumnNamesFromOrderBy utility to handle orderBy logic for composite fields.
- Introduced functions to build filters for null values in composite fields. - Enhanced the main filter function to handle cases where all subfields are null. - Added logic to include nulls in comparison based on order direction and computed operator.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![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 (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/engine/api/utils/build-cursor-composite-field-where-condition.utils.ts">
<violation number="1" location="packages/twenty-server/src/engine/api/utils/build-cursor-composite-field-where-condition.utils.ts:64">
P1: The `buildAllNullComparisonFilter` function returns `{ is: 'NULL' }` for the `lt` operator, which is incorrect. When the cursor is at a null value and pagination goes 'less than', the correct filter depends on NullsFirst/NullsLast ordering:
- NullsFirst: nothing comes before nulls → return empty filter or no matches
- NullsLast: non-nulls come before nulls → return `{ is: 'NOT NULL' }`
The function needs `orderByDirection` passed to it (not just `computedOperator`) to handle this correctly.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
...ges/twenty-server/src/engine/api/utils/build-cursor-composite-field-where-condition.utils.ts
Show resolved
Hide resolved
…elds - Updated test cases for computeCursorArgFilter to include null handling for firstName and lastName. - Modified filter logic to allow comparisons with null values alongside existing conditions.
- Added a `disabled` prop to Action, ActionButton, ActionDisplay, ActionListItem, and ActionDropdownItem components to prevent interaction when set to true. - Updated NavigateToNextRecordSingleRecordAction and NavigateToPreviousRecordSingleRecordAction to utilize the new disabled state based on navigation availability.
- Reformatted ActionButton and ActionListItem components for better code clarity by breaking props into multiple lines. - No functional changes were made; this is purely a formatting update.
…include null checks - Modified test snapshots to incorporate 'or' conditions for various address and name fields, allowing for comparisons with null values. - Ensured that all relevant properties now handle both greater than and null conditions in the test cases.
…ll handling - Updated test cases to include comprehensive checks for null values in orderBy conditions. - Added validation for orderBy structure and direction, ensuring robust error handling. - Enhanced assertions to accommodate both 'or' and 'and' conditions in the test logic.
📊 API Changes ReportGraphQL Schema ChangesGraphQL Schema Changes[error] Error: Unable to read JSON file: /home/runner/work/twenty/twenty/main-schema-introspection.json: Not valid JSON content GraphQL Metadata Schema ChangesGraphQL Metadata Schema Changes[error] Error: Unable to read JSON file: /home/runner/work/twenty/twenty/main-metadata-schema-introspection.json: Not valid JSON content REST API Analysis ErrorError OutputREST Metadata API Analysis ErrorError Output
|
- Added orderByDirection parameter to improve handling of null values in comparisons. - Updated logic to return an empty object when nulls should be prioritized based on order direction. - Adjusted conditions for 'lt' operator to ensure correct handling of NOT NULL comparisons.
…ition logic - Introduced isEmptyFilter function to check for empty filters. - Updated buildCursorCumulativeWhereCondition to utilize isEmptyFilter for cleaner condition handling. - Enhanced filtering of null conditions in the final output, improving overall performance and readability.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
abdulrahmancodes
added
-PR: awaiting review
and removed
-PR: wip
-PR: awaiting author
labels
There was a problem hiding this comment.
4 issues found across 14 files
Prompt for AI agents (all 4 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/engine/api/utils/build-cursor-composite-field-where-condition.utils.ts">
<violation number="1" location="packages/twenty-server/src/engine/api/utils/build-cursor-composite-field-where-condition.utils.ts:57">
P1: The `gt` operator case doesn't check null ordering and always returns `NOT NULL`. For NULLS LAST ordering with forward pagination from a null cursor, there are no records after null, so it should return `{}` (empty filter). Currently it returns `NOT NULL`, which would incorrectly fetch records that appear before the null cursor.</violation>
<violation number="2" location="packages/twenty-server/src/engine/api/utils/build-cursor-composite-field-where-condition.utils.ts:192">
P2: Filtering null values from `cursorEntries` causes equality conditions to miss `IS NULL` checks for partially null composite fields. For a cursor like `{firstName: 'John', lastName: null}`, the equality filter won't include a check for `lastName IS NULL`, potentially matching incorrect records.</violation>
</file>
<file name="packages/twenty-server/src/engine/api/utils/build-cursor-cumulative-where-conditions.utils.ts">
<violation number="1" location="packages/twenty-server/src/engine/api/utils/build-cursor-cumulative-where-conditions.utils.ts:77">
P3: Unreachable code: `andConditions.length === 0` can never be true at this point because `mainCondition` was just pushed to `andConditions` on line 74 (we only reach this code if `mainCondition` is not empty). This check should be removed.</violation>
</file>
<file name="packages/twenty-server/src/engine/api/common/common-query-runners/common-find-many-query-runner.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/api/common/common-query-runners/common-find-many-query-runner.service.ts:256">
P1: The condition `isDefined(direction)` is too broad and will match string enum values like `AscNullsLast` or `DescNullsLast`. When `Object.entries()` is called on a string, it iterates over characters, producing incorrect results. Use `typeof direction === 'object' && direction !== null` to properly detect composite fields.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| return null; | ||
| } | ||
|
|
||
| if (isNull(cursorValue[property.name])) { |
There was a problem hiding this comment.
P2: Filtering null values from cursorEntries causes equality conditions to miss IS NULL checks for partially null composite fields. For a cursor like {firstName: 'John', lastName: null}, the equality filter won't include a check for lastName IS NULL, potentially matching incorrect records.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/api/utils/build-cursor-composite-field-where-condition.utils.ts, line 192:
<comment>Filtering null values from `cursorEntries` causes equality conditions to miss `IS NULL` checks for partially null composite fields. For a cursor like `{firstName: 'John', lastName: null}`, the equality filter won't include a check for `lastName IS NULL`, potentially matching incorrect records.</comment>
<file context>
@@ -64,36 +156,62 @@ export const buildCursorCompositeFieldWhereCondition = ({
return null;
}
+ if (isNull(cursorValue[property.name])) {
+ return null;
+ }
</file context>
| if (computedOperator === 'gt') { | ||
| return { | ||
| [fieldKey]: { | ||
| [firstProperty.name]: { is: 'NOT NULL' }, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
P1: The gt operator case doesn't check null ordering and always returns NOT NULL. For NULLS LAST ordering with forward pagination from a null cursor, there are no records after null, so it should return {} (empty filter). Currently it returns NOT NULL, which would incorrectly fetch records that appear before the null cursor.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/api/utils/build-cursor-composite-field-where-condition.utils.ts, line 57:
<comment>The `gt` operator case doesn't check null ordering and always returns `NOT NULL`. For NULLS LAST ordering with forward pagination from a null cursor, there are no records after null, so it should return `{}` (empty filter). Currently it returns `NOT NULL`, which would incorrectly fetch records that appear before the null cursor.</comment>
<file context>
@@ -29,6 +31,96 @@ type BuildCursorCompositeFieldWhereConditionParams = {
+ computedOperator: string,
+ orderByDirection: OrderByDirection,
+): ObjectRecordFilter | Record<string, never> => {
+ if (computedOperator === 'gt') {
+ return {
+ [fieldKey]: {
</file context>
| if (computedOperator === 'gt') { | |
| return { | |
| [fieldKey]: { | |
| [firstProperty.name]: { is: 'NOT NULL' }, | |
| }, | |
| }; | |
| } | |
| if (computedOperator === 'gt') { | |
| const isNullsLast = | |
| orderByDirection === OrderByDirection.AscNullsLast || | |
| orderByDirection === OrderByDirection.DescNullsLast; | |
| if (isNullsLast) { | |
| return {}; | |
| } | |
| return { | |
| [fieldKey]: { | |
| [firstProperty.name]: { is: 'NOT NULL' }, | |
| }, | |
| }; | |
| } |
|
|
||
| andConditions.push(mainCondition); | ||
|
|
||
| if (andConditions.length === 0) { |
There was a problem hiding this comment.
P3: Unreachable code: andConditions.length === 0 can never be true at this point because mainCondition was just pushed to andConditions on line 74 (we only reach this code if mainCondition is not empty). This check should be removed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/api/utils/build-cursor-cumulative-where-conditions.utils.ts, line 77:
<comment>Unreachable code: `andConditions.length === 0` can never be true at this point because `mainCondition` was just pushed to `andConditions` on line 74 (we only reach this code if `mainCondition` is not empty). This check should be removed.</comment>
<file context>
@@ -34,41 +38,53 @@ export const buildCursorCumulativeWhereCondition = <
+
+ andConditions.push(mainCondition);
+
+ if (andConditions.length === 0) {
+ return null;
+ }
</file context>
...y-server/src/engine/api/common/common-query-runners/common-find-many-query-runner.service.ts
Outdated
Show resolved
Hide resolved
abdulrahmancodes
force-pushed
the
fix/cursor-pagination-order
branch
from
023ebc7 to
ea5ef71
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
...s/twenty-server/src/engine/api/graphql/graphql-query-runner/utils/build-columns-to-select.ts
Outdated
Show resolved
Hide resolved
...y-server/src/engine/api/common/common-query-runners/common-find-many-query-runner.service.ts
Outdated
Show resolved
Hide resolved
...ges/twenty-server/src/engine/api/utils/build-cursor-composite-field-where-condition.utils.ts
Show resolved
Hide resolved
- Added support for composite field metadata types in the column selection process. - Introduced a utility function to compute composite column names based on subfield properties. - Updated the logic to handle cases where composite types are not defined, ensuring robustness in column name extraction.
abdulrahmancodes
force-pushed
the
fix/cursor-pagination-order
branch
from
54f83a8 to
006fa43
Compare
|
Hi @abdulrahmancodes sorry for the delay. My main concern while reviewing this PR is that the filter built from the cursor looks odd. For example, if I navigate from a simple view to a show page, I should have indeed a request to find the next and previous record id, by using cursor after and before with limit 1, respectively. But if I look at the generated filters for this I end up with something like that : [
{
"position": {
"lt": 22
}
},
{
"and": [
{
"position": {
"eq": 22
}
},
{
"id": {
"lt": "50505050-0016-4e7c-8001-123456789abc"
}
}
]
}
]The problem is that a It works because the first filter is ok, but the second being an impossible filter, it will create technical debt and lead to bugs. I don't know if this comes from your PR originally but since we're working on this part of the code it is the best PR to fix this problem. |
Closes #6344
Issues Fixed
Cursor encoding was broken: The frontend only requested the
idfield inrecordGqlFields, but cursor encoding requires all orderBy field values.Cursor encoding was broken. The frontend only requested the
idfield inrecordGqlFieldsfor pagination queries, but cursor encoding requires all orderBy field values to generate valid cursors.Navigation buttons were not disabled when navigation was unavailable. The next/previous record navigation buttons remained enabled even when
recordAfter/recordBeforewas not available, leading to a poor user experience where clicking the buttons would have no effect.Changes
Updated
buildColumnsToSelectto automatically include orderBy fields in the SELECT statement:Updated
buildCursorCompositeFieldWhereConditionto properly handle null composite field values:is: 'NULL'instead ofeq: nullfor equality conditionsis: 'NOT NULL'oris: 'NULL'for comparison conditions when all sub-fields are nullNullsFirst/NullsLast) and pagination directionAdded disabled state support to Action component system:
Action,ActionDisplay,ActionButton,ActionListItem, andActionDropdownItemto accept and handledisabledpropNavigateToNextRecordSingleRecordActionandNavigateToPreviousRecordSingleRecordActionto usecanNavigateToNextRecordandcanNavigateToPreviousRecordflags to disable buttons when navigation is unavailableKnown Limitations
Navigation with Null Composite Field Values:
Navigation backward when records have null composite field values (e.g., amount) may skip records in certain scenarios. This occurs when:
AscNullsLastorderingMultiple approaches were attempted to fix this issue (including explicit
IS NOT NULLchecks, restructuring OR conditions, and separate null handling), but none fully resolved the skipping behavior without introducing other issues. This is a known limitation that requires further investigation. The current implementation handles most cases but may skip intermediate records in specific scenarios involving null composite field values.