diff --git a/pkg/cache/sql/informer/listoption_indexer.go b/pkg/cache/sql/informer/listoption_indexer.go index 028873f3..3290cf70 100644 --- a/pkg/cache/sql/informer/listoption_indexer.go +++ b/pkg/cache/sql/informer/listoption_indexer.go @@ -381,39 +381,29 @@ func (l *ListOptionIndexer) constructQuery(lo ListOptions, partitions []partitio } } - // 3- Sorting: ORDER BY clauses (from lo.Sort) - orderByClauses := []string{} - if len(lo.Sort.PrimaryField) > 0 { - columnName := toColumnName(lo.Sort.PrimaryField) - if err := l.validateColumn(columnName); err != nil { - return queryInfo, err - } - - direction := "ASC" - if lo.Sort.PrimaryOrder == DESC { - direction = "DESC" - } - orderByClauses = append(orderByClauses, fmt.Sprintf(`f."%s" %s`, columnName, direction)) - } - if len(lo.Sort.SecondaryField) > 0 { - columnName := toColumnName(lo.Sort.SecondaryField) - if err := l.validateColumn(columnName); err != nil { - return queryInfo, err - } - - direction := "ASC" - if lo.Sort.SecondaryOrder == DESC { - direction = "DESC" - } - orderByClauses = append(orderByClauses, fmt.Sprintf(`f."%s" %s`, columnName, direction)) - } - // before proceeding, save a copy of the query and params without LIMIT/OFFSET/ORDER info // for COUNTing all results later countQuery := fmt.Sprintf("SELECT COUNT(*) FROM (%s)", query) countParams := params[:] - if len(orderByClauses) > 0 { + // 3- Sorting: ORDER BY clauses (from lo.Sort) + if len(lo.Sort.Fields) != len(lo.Sort.Orders) { + return nil, fmt.Errorf("sort fields length %d != sort orders length %d", len(lo.Sort.Fields), len(lo.Sort.Orders)) + } + if len(lo.Sort.Fields) > 0 { + orderByClauses := []string{} + for i, field := range lo.Sort.Fields { + columnName := toColumnName(field) + if err := l.validateColumn(columnName); err != nil { + return queryInfo, err + } + + direction := "ASC" + if lo.Sort.Orders[i] == DESC { + direction = "DESC" + } + orderByClauses = append(orderByClauses, fmt.Sprintf(`f."%s" %s`, columnName, direction)) + } query += "\n ORDER BY " query += strings.Join(orderByClauses, ", ") } else { diff --git a/pkg/cache/sql/informer/listoption_indexer_test.go b/pkg/cache/sql/informer/listoption_indexer_test.go index eec6d943..fe7a038f 100644 --- a/pkg/cache/sql/informer/listoption_indexer_test.go +++ b/pkg/cache/sql/informer/listoption_indexer_test.go @@ -590,10 +590,11 @@ func TestListByOptions(t *testing.T) { }) tests = append(tests, testCase{ - description: "ListByOptions with Sort.PrimaryField set only should sort on that field only, in ascending order in prepared sql.Stmt", + description: "ListByOptions with only one Sort.Field set should sort on that field only, in ascending order in prepared sql.Stmt", listOptions: ListOptions{ Sort: Sort{ - PrimaryField: []string{"metadata", "somefield"}, + Fields: [][]string{{"metadata", "somefield"}}, + Orders: []SortOrder{ASC}, }, }, partitions: []partition.Partition{}, @@ -610,31 +611,36 @@ func TestListByOptions(t *testing.T) { expectedContToken: "", expectedErr: nil, }) + tests = append(tests, testCase{ - description: "ListByOptions with Sort.SecondaryField set only should sort on that field only, in ascending order in prepared sql.Stmt", + description: "sort one field descending", listOptions: ListOptions{ Sort: Sort{ - SecondaryField: []string{"metadata", "somefield"}, + Fields: [][]string{{"metadata", "somefield"}}, + Orders: []SortOrder{DESC}, }, }, partitions: []partition.Partition{}, - ns: "", + ns: "test5a", expectedStmt: `SELECT o.object, o.objectnonce, o.dekid FROM "something" o JOIN "something_fields" f ON o.key = f.key WHERE + (f."metadata.namespace" = ?) AND (FALSE) - ORDER BY f."metadata.somefield" ASC`, + ORDER BY f."metadata.somefield" DESC`, + expectedStmtArgs: []any{"test5a"}, returnList: []any{&unstructured.Unstructured{Object: unstrTestObjectMap}, &unstructured.Unstructured{Object: unstrTestObjectMap}}, expectedList: &unstructured.UnstructuredList{Object: map[string]interface{}{"items": []map[string]interface{}{unstrTestObjectMap, unstrTestObjectMap}}, Items: []unstructured.Unstructured{{Object: unstrTestObjectMap}, {Object: unstrTestObjectMap}}}, expectedContToken: "", expectedErr: nil, }) + tests = append(tests, testCase{ - description: "ListByOptions with Sort.PrimaryField and Sort.SecondaryField set should sort on PrimaryField in ascending order first and then sort on SecondaryField in ascending order in prepared sql.Stmt", + description: "ListByOptions sorting on two fields should sort on the first field in ascending order first and then sort on the second field in ascending order in prepared sql.Stmt", listOptions: ListOptions{ Sort: Sort{ - PrimaryField: []string{"metadata", "somefield"}, - SecondaryField: []string{"status", "someotherfield"}, + Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}}, + Orders: []SortOrder{ASC, ASC}, }, }, partitions: []partition.Partition{}, @@ -649,13 +655,13 @@ func TestListByOptions(t *testing.T) { expectedContToken: "", expectedErr: nil, }) + tests = append(tests, testCase{ - description: "ListByOptions with Sort.PrimaryField and Sort.SecondaryField set and PrimaryOrder set to DESC should sort on PrimaryField in descending order first and then sort on SecondaryField in ascending order in prepared sql.Stmt", + description: "ListByOptions sorting on two fields should sort on the first field in descending order first and then sort on the second field in ascending order in prepared sql.Stmt", listOptions: ListOptions{ Sort: Sort{ - PrimaryField: []string{"metadata", "somefield"}, - SecondaryField: []string{"status", "someotherfield"}, - PrimaryOrder: DESC, + Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}}, + Orders: []SortOrder{DESC, ASC}, }, }, partitions: []partition.Partition{}, @@ -670,31 +676,13 @@ func TestListByOptions(t *testing.T) { expectedContToken: "", expectedErr: nil, }) + tests = append(tests, testCase{ - description: "ListByOptions with Sort.SecondaryField set and Sort.PrimaryOrder set to descending should sort on that SecondaryField in ascending order only and ignore PrimaryOrder in prepared sql.Stmt", - listOptions: ListOptions{ - Sort: Sort{ - SecondaryField: []string{"status", "someotherfield"}, - PrimaryOrder: DESC, - }, - }, - partitions: []partition.Partition{}, - ns: "", - expectedStmt: `SELECT o.object, o.objectnonce, o.dekid FROM "something" o - JOIN "something_fields" f ON o.key = f.key - WHERE - (FALSE) - ORDER BY f."status.someotherfield" ASC`, - returnList: []any{&unstructured.Unstructured{Object: unstrTestObjectMap}, &unstructured.Unstructured{Object: unstrTestObjectMap}}, - expectedList: &unstructured.UnstructuredList{Object: map[string]interface{}{"items": []map[string]interface{}{unstrTestObjectMap, unstrTestObjectMap}}, Items: []unstructured.Unstructured{{Object: unstrTestObjectMap}, {Object: unstrTestObjectMap}}}, - expectedContToken: "", - expectedErr: nil, - }) - tests = append(tests, testCase{ - description: "ListByOptions with Sort.PrimaryOrder set only should sort on default primary and secondary fields in ascending order in prepared sql.Stmt", + description: "ListByOptions sorting when # fields != # sort orders should return an error", listOptions: ListOptions{ Sort: Sort{ - PrimaryOrder: DESC, + Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}}, + Orders: []SortOrder{DESC, ASC, ASC}, }, }, partitions: []partition.Partition{}, @@ -703,12 +691,13 @@ func TestListByOptions(t *testing.T) { JOIN "something_fields" f ON o.key = f.key WHERE (FALSE) - ORDER BY f."metadata.name" ASC `, + ORDER BY f."metadata.somefield" DESC, f."status.someotherfield" ASC`, returnList: []any{&unstructured.Unstructured{Object: unstrTestObjectMap}, &unstructured.Unstructured{Object: unstrTestObjectMap}}, expectedList: &unstructured.UnstructuredList{Object: map[string]interface{}{"items": []map[string]interface{}{unstrTestObjectMap, unstrTestObjectMap}}, Items: []unstructured.Unstructured{{Object: unstrTestObjectMap}, {Object: unstrTestObjectMap}}}, expectedContToken: "", - expectedErr: nil, + expectedErr: fmt.Errorf("sort fields length 2 != sort orders length 3"), }) + tests = append(tests, testCase{ description: "ListByOptions with Pagination.PageSize set should set limit to PageSize in prepared sql.Stmt", listOptions: ListOptions{ @@ -1433,6 +1422,36 @@ func TestConstructQuery(t *testing.T) { expectedErr: nil, }) + tests = append(tests, testCase{ + description: "ConstructQuery: sorting when # fields < # sort orders should return an error", + listOptions: ListOptions{ + Sort: Sort{ + Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}}, + Orders: []SortOrder{DESC, ASC, ASC}, + }, + }, + partitions: []partition.Partition{}, + ns: "", + expectedStmt: "", + expectedStmtArgs: []any{}, + expectedErr: fmt.Errorf("sort fields length 2 != sort orders length 3"), + }) + + tests = append(tests, testCase{ + description: "ConstructQuery: sorting when # fields > # sort orders should return an error", + listOptions: ListOptions{ + Sort: Sort{ + Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}, {"metadata", "labels", "a1"}, {"metadata", "labels", "a2"}}, + Orders: []SortOrder{DESC, ASC, ASC}, + }, + }, + partitions: []partition.Partition{}, + ns: "", + expectedStmt: "", + expectedStmtArgs: []any{}, + expectedErr: fmt.Errorf("sort fields length 4 != sort orders length 3"), + }) + t.Parallel() for _, test := range tests { t.Run(test.description, func(t *testing.T) { diff --git a/pkg/cache/sql/informer/listoptions.go b/pkg/cache/sql/informer/listoptions.go index b0a433f6..71bf6f6e 100644 --- a/pkg/cache/sql/informer/listoptions.go +++ b/pkg/cache/sql/informer/listoptions.go @@ -55,11 +55,10 @@ type OrFilter struct { // The subfield to sort by is represented in a request query using . notation, e.g. 'metadata.name'. // The subfield is internally represented as a slice, e.g. [metadata, name]. // The order is represented by prefixing the sort key by '-', e.g. sort=-metadata.name. +// e.g. To sort internal clusters first followed by clusters in alpha order: sort=-spec.internal,spec.displayName type Sort struct { - PrimaryField []string - SecondaryField []string - PrimaryOrder SortOrder - SecondaryOrder SortOrder + Fields [][]string + Orders []SortOrder } // Pagination represents how to return paginated results.