From a2bff662a561364317cc102ca8f1a20b495e85ca Mon Sep 17 00:00:00 2001 From: Sonarly Claude Code Date: Thu, 7 May 2026 17:58:27 +0000 Subject: [PATCH] fix: sort SELECT fields by option position instead of alphabetical order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://sonarly.com/issue/35648?type=bug SELECT/MULTI_SELECT fields are sorted alphabetically (using LOWER() + ::text cast) instead of by their defined option position, causing incorrect sort order when filters are applied. Fix: ## Summary Changed SELECT and MULTI_SELECT field sorting from alphabetical order to option position order. ## Root Cause Commit a8331dc43e introduced case-insensitive sorting using `LOWER(column::text)` for TEXT, SELECT, and MULTI_SELECT fields. While appropriate for TEXT fields, this caused SELECT/MULTI_SELECT fields to sort alphabetically by option value instead of by their defined position property. ## Fix 1. **`build-order-by-column-expression.util.ts`**: - Modified `shouldUseCaseInsensitiveOrder()` to return `false` for SELECT fields (TEXT only now) - Added `isSelectFieldType()` helper to identify SELECT/MULTI_SELECT fields - Added `getSelectOptionValuesSortedByPosition()` to extract option values sorted by position - Changed `shouldCastToText()` to return `false` (no longer needed) 2. **`order-by-condition.type.ts`**: - Added `selectOptionValues?: string[]` property to store option values sorted by position 3. **`graphql-query-order.parser.ts`**: - Updated scalar field parsing to extract and pass `selectOptionValues` for SELECT fields - Updated relation field parsing similarly for nested SELECT fields 4. **`graphql-query.parser.ts`**: - Updated `getOrderByRawSQL()` to generate `ARRAY_POSITION(ARRAY['opt1', 'opt2', ...], column)` SQL when `selectOptionValues` is present, instead of `LOWER(column::text)` 5. **`build-order-by-key.spec.ts`**: - Updated existing tests to reflect new behavior - Added tests for `isSelectFieldType()` and `getSelectOptionValuesSortedByPosition()` ## Example SQL Change Before: `ORDER BY LOWER("task"."priority"::text) ASC NULLS FIRST` After: `ORDER BY ARRAY_POSITION(ARRAY['URGENT', 'HIGH', 'MEDIUM', 'LOW'], "task"."priority") ASC NULLS FIRST` This ensures Priority field sorts by defined option position (Urgent → High → Medium → Low) rather than alphabetically (High → Low → Medium → Urgent). --- .../__tests__/build-order-by-key.spec.ts | 74 +++++++++++++++++-- .../graphql-query-order.parser.ts | 17 +++++ .../types/order-by-condition.type.ts | 1 + .../build-order-by-column-expression.util.ts | 27 +++++-- .../graphql-query.parser.ts | 23 ++++-- 5 files changed, 123 insertions(+), 19 deletions(-) diff --git a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/__tests__/build-order-by-key.spec.ts b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/__tests__/build-order-by-key.spec.ts index 8b06a88639b..74033d0d187 100644 --- a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/__tests__/build-order-by-key.spec.ts +++ b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/__tests__/build-order-by-key.spec.ts @@ -2,6 +2,8 @@ import { FieldMetadataType } from 'twenty-shared/types'; import { buildOrderByColumnExpression, + getSelectOptionValuesSortedByPosition, + isSelectFieldType, shouldCastToText, shouldUseCaseInsensitiveOrder, } from 'src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/utils/build-order-by-column-expression.util'; @@ -33,13 +35,13 @@ describe('shouldUseCaseInsensitiveOrder', () => { expect(shouldUseCaseInsensitiveOrder(FieldMetadataType.TEXT)).toBe(true); }); - it('should return true for SELECT fields', () => { - expect(shouldUseCaseInsensitiveOrder(FieldMetadataType.SELECT)).toBe(true); + it('should return false for SELECT fields (uses ARRAY_POSITION instead)', () => { + expect(shouldUseCaseInsensitiveOrder(FieldMetadataType.SELECT)).toBe(false); }); - it('should return true for MULTI_SELECT fields', () => { + it('should return false for MULTI_SELECT fields (uses ARRAY_POSITION instead)', () => { expect(shouldUseCaseInsensitiveOrder(FieldMetadataType.MULTI_SELECT)).toBe( - true, + false, ); }); @@ -64,13 +66,31 @@ describe('shouldUseCaseInsensitiveOrder', () => { }); }); -describe('shouldCastToText', () => { +describe('isSelectFieldType', () => { it('should return true for SELECT fields', () => { - expect(shouldCastToText(FieldMetadataType.SELECT)).toBe(true); + expect(isSelectFieldType(FieldMetadataType.SELECT)).toBe(true); }); it('should return true for MULTI_SELECT fields', () => { - expect(shouldCastToText(FieldMetadataType.MULTI_SELECT)).toBe(true); + expect(isSelectFieldType(FieldMetadataType.MULTI_SELECT)).toBe(true); + }); + + it('should return false for TEXT fields', () => { + expect(isSelectFieldType(FieldMetadataType.TEXT)).toBe(false); + }); + + it('should return false for NUMBER fields', () => { + expect(isSelectFieldType(FieldMetadataType.NUMBER)).toBe(false); + }); +}); + +describe('shouldCastToText', () => { + it('should return false for SELECT fields (uses ARRAY_POSITION instead)', () => { + expect(shouldCastToText(FieldMetadataType.SELECT)).toBe(false); + }); + + it('should return false for MULTI_SELECT fields (uses ARRAY_POSITION instead)', () => { + expect(shouldCastToText(FieldMetadataType.MULTI_SELECT)).toBe(false); }); it('should return false for TEXT fields', () => { @@ -85,3 +105,43 @@ describe('shouldCastToText', () => { expect(shouldCastToText(FieldMetadataType.DATE_TIME)).toBe(false); }); }); + +describe('getSelectOptionValuesSortedByPosition', () => { + it('should return undefined for null options', () => { + expect(getSelectOptionValuesSortedByPosition(null)).toBeUndefined(); + }); + + it('should return undefined for undefined options', () => { + expect(getSelectOptionValuesSortedByPosition(undefined)).toBeUndefined(); + }); + + it('should return undefined for empty options array', () => { + expect(getSelectOptionValuesSortedByPosition([])).toBeUndefined(); + }); + + it('should return option values sorted by position', () => { + const options = [ + { value: 'MEDIUM', label: 'Medium', position: 2, color: 'yellow' as const }, + { value: 'URGENT', label: 'Urgent', position: 0, color: 'red' as const }, + { value: 'HIGH', label: 'High', position: 1, color: 'orange' as const }, + { value: 'LOW', label: 'Low', position: 3, color: 'green' as const }, + ]; + + const result = getSelectOptionValuesSortedByPosition(options); + + expect(result).toEqual(['URGENT', 'HIGH', 'MEDIUM', 'LOW']); + }); + + it('should handle options with same position', () => { + const options = [ + { value: 'B', label: 'B', position: 1, color: 'blue' as const }, + { value: 'A', label: 'A', position: 1, color: 'green' as const }, + ]; + + const result = getSelectOptionValuesSortedByPosition(options); + + expect(result).toHaveLength(2); + expect(result).toContain('A'); + expect(result).toContain('B'); + }); +}); diff --git a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/graphql-query-order.parser.ts b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/graphql-query-order.parser.ts index 5af7e1510cb..e9120bff229 100644 --- a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/graphql-query-order.parser.ts +++ b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/graphql-query-order.parser.ts @@ -1,4 +1,5 @@ import { isObject } from 'class-validator'; +import { type FieldMetadataComplexOption } from 'twenty-shared/types'; import { isDefined } from 'twenty-shared/utils'; import { type ObjectRecordOrderBy } from 'src/engine/api/graphql/workspace-query-builder/interfaces/object-record.interface'; @@ -10,6 +11,8 @@ import { } from 'src/engine/api/graphql/graphql-query-runner/errors/graphql-query-runner.exception'; import { buildOrderByColumnExpression, + getSelectOptionValuesSortedByPosition, + isSelectFieldType, shouldCastToText, shouldUseCaseInsensitiveOrder, } from 'src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/utils/build-order-by-column-expression.util'; @@ -143,6 +146,12 @@ export class GraphqlQueryOrderFieldParser { fieldName, ); + const selectOptionValues = isSelectFieldType(fieldMetadata.type) + ? getSelectOptionValuesSortedByPosition( + fieldMetadata.options as FieldMetadataComplexOption[], + ) + : undefined; + orderByConditions[columnExpression] = { ...convertOrderByToFindOptionsOrder( orderByDirection, @@ -150,6 +159,7 @@ export class GraphqlQueryOrderFieldParser { ), useLower: shouldUseCaseInsensitiveOrder(fieldMetadata.type), castToText: shouldCastToText(fieldMetadata.type), + selectOptionValues, }; } } @@ -256,6 +266,12 @@ export class GraphqlQueryOrderFieldParser { nestedFieldMetadata.name, ); + const selectOptionValues = isSelectFieldType(nestedFieldMetadata.type) + ? getSelectOptionValuesSortedByPosition( + nestedFieldMetadata.options as FieldMetadataComplexOption[], + ) + : undefined; + return { orderBy: { [columnExpression]: { @@ -265,6 +281,7 @@ export class GraphqlQueryOrderFieldParser { ), useLower: shouldUseCaseInsensitiveOrder(nestedFieldMetadata.type), castToText: shouldCastToText(nestedFieldMetadata.type), + selectOptionValues, }, }, joinInfo, diff --git a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/types/order-by-condition.type.ts b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/types/order-by-condition.type.ts index 36169bd8f4e..91ccb5fc582 100644 --- a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/types/order-by-condition.type.ts +++ b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/types/order-by-condition.type.ts @@ -3,4 +3,5 @@ export type OrderByClause = { nulls?: 'NULLS FIRST' | 'NULLS LAST'; useLower?: boolean; castToText?: boolean; // For SELECT/MULTI_SELECT fields that need ::text before LOWER + selectOptionValues?: string[]; // Option values sorted by position for SELECT/MULTI_SELECT fields }; diff --git a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/utils/build-order-by-column-expression.util.ts b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/utils/build-order-by-column-expression.util.ts index 264347d4be2..ab5e7c91a36 100644 --- a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/utils/build-order-by-column-expression.util.ts +++ b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/utils/build-order-by-column-expression.util.ts @@ -1,20 +1,35 @@ -import { FieldMetadataType } from 'twenty-shared/types'; +import { + type FieldMetadataComplexOption, + FieldMetadataType, +} from 'twenty-shared/types'; export const shouldUseCaseInsensitiveOrder = ( fieldType: FieldMetadataType, ): boolean => { + return fieldType === FieldMetadataType.TEXT; +}; + +export const isSelectFieldType = (fieldType: FieldMetadataType): boolean => { return ( - fieldType === FieldMetadataType.TEXT || fieldType === FieldMetadataType.SELECT || fieldType === FieldMetadataType.MULTI_SELECT ); }; export const shouldCastToText = (fieldType: FieldMetadataType): boolean => { - return ( - fieldType === FieldMetadataType.SELECT || - fieldType === FieldMetadataType.MULTI_SELECT - ); + return false; +}; + +export const getSelectOptionValuesSortedByPosition = ( + options: FieldMetadataComplexOption[] | null | undefined, +): string[] | undefined => { + if (!options || options.length === 0) { + return undefined; + } + + return [...options] + .sort((a, b) => (a.position ?? 0) - (b.position ?? 0)) + .map((option) => option.value); }; // Returns unquoted column expression for TypeORM's orderBy (e.g., "company.name") diff --git a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query.parser.ts b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query.parser.ts index bab8cc14080..aef1d6694c2 100644 --- a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query.parser.ts +++ b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query.parser.ts @@ -193,14 +193,25 @@ export class GraphqlQueryParser { ? `"${parts[0]}"."${parts[1]}"` : `"${orderByField}"`; - // Build column expression with optional ::text cast and LOWER() + // Build column expression with ARRAY_POSITION for SELECT fields, or optional ::text cast and LOWER() let columnExpr = quotedColumn; - if (orderByCondition.castToText) { - columnExpr = `${columnExpr}::text`; - } - if (orderByCondition.useLower) { - columnExpr = `LOWER(${columnExpr})`; + if ( + orderByCondition.selectOptionValues && + orderByCondition.selectOptionValues.length > 0 + ) { + // Use ARRAY_POSITION for SELECT/MULTI_SELECT fields to sort by option position + const optionsArray = orderByCondition.selectOptionValues + .map((value) => `'${value.replace(/'/g, "''")}'`) + .join(', '); + columnExpr = `ARRAY_POSITION(ARRAY[${optionsArray}], ${quotedColumn})`; + } else { + if (orderByCondition.castToText) { + columnExpr = `${columnExpr}::text`; + } + if (orderByCondition.useLower) { + columnExpr = `LOWER(${columnExpr})`; + } } return `${columnExpr} ${orderByCondition.order}${nullsCondition}`;