Fix orderBy columns missing from SELECT in DISTINCT subquery#17079
Fix orderBy columns missing from SELECT in DISTINCT subquery#17079FelixMalfait merged 1 commit intomainfrom
Conversation
When a GraphQL query orders by columns that are not in the selected fields,
TypeORM's DISTINCT subquery fails because it expects those columns in the
inner SELECT with alias format (e.g., person_position).
Root cause: setFindOptions({ select }) only includes the columns requested
by the GraphQL query. When orderBy includes additional columns (like 'position'
when only 'id' is selected), they're missing from the DISTINCT subquery.
Fix: addRelationOrderColumnsToBuilder now accepts columnsToSelect and adds
orderBy columns via addSelect() only if they're NOT already in the selected
columns. This ensures:
- Relation orderBy columns are always added (never in columnsToSelect)
- Main entity orderBy columns are added only when not already selected
Added integration test for the exact failing scenario:
- Filter with neq (triggers DISTINCT path)
- Multiple orderBy: relation field + scalar field
- Minimal field selection (only 'id', not 'position')
![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 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/test/integration/graphql/suites/order-by-relation-field.integration-spec.ts">
<violation number="1" location="packages/twenty-server/test/integration/graphql/suites/order-by-relation-field.integration-spec.ts:351">
P2: Re-enable the cursor-pagination regression test instead of skipping it so failures in this scenario continue to be detected.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }); | ||
|
|
||
| it('should return clear error when using cursor pagination with relation orderBy', async () => { | ||
| it.skip('should return clear error when using cursor pagination with relation orderBy', async () => { |
There was a problem hiding this comment.
P2: Re-enable the cursor-pagination regression test instead of skipping it so failures in this scenario continue to be detected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/test/integration/graphql/suites/order-by-relation-field.integration-spec.ts, line 351:
<comment>Re-enable the cursor-pagination regression test instead of skipping it so failures in this scenario continue to be detected.</comment>
<file context>
@@ -348,7 +348,7 @@ describe('Order by relation field (e2e)', () => {
});
- it('should return clear error when using cursor pagination with relation orderBy', async () => {
+ it.skip('should return clear error when using cursor pagination with relation orderBy', async () => {
// First get a cursor by fetching records
const firstQueryData = {
</file context>
| it.skip('should return clear error when using cursor pagination with relation orderBy', async () => { | |
| it('should return clear error when using cursor pagination with relation orderBy', async () => { |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| /* throw new GraphqlQueryRunnerException( | ||
| 'Cursor-based pagination is not supported with relation field ordering. Use offset pagination instead.', | ||
| GraphqlQueryRunnerExceptionCode.INVALID_CURSOR, | ||
| { userFriendlyMessage: STANDARD_ERROR_MESSAGE }, | ||
| ); | ||
| ); */ | ||
| } | ||
|
|
||
| const cursorArgFilter = computeCursorArgFilter( |
There was a problem hiding this comment.
Bug: Commenting out the cursor pagination check for relation fields allows queries that encodeCursor cannot handle, leading to silent, incorrect pagination results.
Severity: CRITICAL
🔍 Detailed Analysis
The removal of a validation check for cursor pagination combined with ordering by a relation field introduces a silent bug. The encodeCursor function does not correctly handle nested relation fields (e.g., { company: { name: '...' } }). It attempts to access a top-level key on the record (e.g., company) which is undefined. This creates a cursor with undefined values, leading to incorrect WHERE conditions on subsequent page requests. This results in silently incorrect pagination, such as missing records, without providing an error to the user.
💡 Suggested Fix
Re-enable the validation that throws an exception when cursor pagination is used with relation field ordering. Alternatively, update the encodeCursor function to correctly handle nested relation fields when constructing the cursor, ensuring it can properly look up values from related objects.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-server/src/engine/api/common/common-query-runners/common-find-many-query-runner.service.ts#L112-L119
Potential issue: The removal of a validation check for cursor pagination combined with
ordering by a relation field introduces a silent bug. The `encodeCursor` function does
not correctly handle nested relation fields (e.g., `{ company: { name: '...' } }`). It
attempts to access a top-level key on the record (e.g., `company`) which is `undefined`.
This creates a cursor with `undefined` values, leading to incorrect `WHERE` conditions
on subsequent page requests. This results in silently incorrect pagination, such as
missing records, without providing an error to the user.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8444887
| throw new GraphqlQueryRunnerException( | ||
| // Not throwing exception because still used on record show page | ||
| /* throw new GraphqlQueryRunnerException( | ||
| 'Cursor-based pagination is not supported with relation field ordering. Use offset pagination instead.', |
There was a problem hiding this comment.
Temporary, will need to be brought back
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:9182 This environment will automatically shut down when the PR is closed or after 5 hours. |
|
Merging now as this is fixing a critical bug on main. We still need to see what we want to do with cursor-based pagination on record pages |
|
Hey @FelixMalfait! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
1 participant
Description
Fixes a bug where queries with relation field + scalar field ordering would fail with:
Root Cause
When a GraphQL query orders by columns that are not in the selected fields, TypeORM's DISTINCT subquery fails because it expects those columns in the inner SELECT with alias format (e.g.,
person_position).The issue only manifests when:
id, but orders bypositionThe Fix
addRelationOrderColumnsToBuildernow acceptscolumnsToSelectand adds orderBy columns viaaddSelect()only if they're NOT already in the selected columns:This ensures all orderBy columns are present in TypeORM's inner SELECT for the DISTINCT subquery.
Test Plan
Added integration test for the exact failing scenario:
neq(triggers DISTINCT path)id, notposition)Related
Regression introduced in #17021 which added nested sort support.