Sort-indirect PR broken into smaller parts: part 2/6 - fix the Sort part of ListOptions (#611)

* Move types related to list options and sql queries into their own package.

The problem having these in the informer package is that eventually code
in other packages will need to import `informer` only for constants or types,
but some members of the informer package may already depend on those. Best to
move type definitions into their own simpler package.

* Fix the ListOptions sort field.

Instead of making it a single array-ish field, convert it into a
true array of Sort Directives.  Easier to read, less bending backwards.

* Rebasing (or human error) duplicated 'NewSortList'.

pkg/sqlcache/informer/listoption_indexer.go

@@ -389,26 +389,24 @@ func (l *ListOptionIndexer) constructQuery(lo sqltypes.ListOptions, partitions [
 	countParams := params[:]
 
 	// 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 {
+	if len(lo.SortList.SortDirectives) > 0 {
 		orderByClauses := []string{}
-		for i, field := range lo.Sort.Fields {
-			if isLabelsFieldList(field) {
-				clause, sortParam, err := buildSortLabelsClause(field[2], joinTableIndexByLabelName, lo.Sort.Orders[i] == sqltypes.ASC)
+		for _, sortDirective := range lo.SortList.SortDirectives {
+			fields := sortDirective.Fields
+			if isLabelsFieldList(fields) {
+				clause, sortParam, err := buildSortLabelsClause(fields[2], joinTableIndexByLabelName, sortDirective.Order == sqltypes.ASC)
 				if err != nil {
 					return nil, err
 				}
 				orderByClauses = append(orderByClauses, clause)
 				params = append(params, sortParam)
 			} else {
-				columnName := toColumnName(field)
+				columnName := toColumnName(fields)
 				if err := l.validateColumn(columnName); err != nil {
 					return queryInfo, err
 				}
 				direction := "ASC"
-				if lo.Sort.Orders[i] == sqltypes.DESC {
+				if sortDirective.Order == sqltypes.DESC {
 					direction = "DESC"
 				}
 				orderByClauses = append(orderByClauses, fmt.Sprintf(`f."%s" %s`, columnName, direction))
@@ -596,13 +594,14 @@ func buildSortLabelsClause(labelName string, joinTableIndexByLabelName map[strin
 // created in Store.ListByPartitions, and that ends up calling ListOptionIndexer.ConstructQuery.
 // No other goroutines access this object.
 func ensureSortLabelsAreSelected(lo *sqltypes.ListOptions) {
-	if len(lo.Sort.Fields) == 0 {
+	if len(lo.SortList.SortDirectives) == 0 {
 		return
 	}
 	unboundSortLabels := make(map[string]bool)
-	for _, fieldList := range lo.Sort.Fields {
-		if isLabelsFieldList(fieldList) {
-			unboundSortLabels[fieldList[2]] = true
+	for _, sortDirective := range lo.SortList.SortDirectives {
+		fields := sortDirective.Fields
+		if isLabelsFieldList(fields) {
+			unboundSortLabels[fields[2]] = true
 		}
 	}
 	if len(unboundSortLabels) == 0 {

pkg/sqlcache/informer/listoption_indexer_test.go

@@ -647,9 +647,13 @@ func TestListByOptions(t *testing.T) {
 	tests = append(tests, testCase{
 		description: "ListByOptions with only one Sort.Field set should sort on that field only, in ascending order in prepared sql.Stmt",
 		listOptions: sqltypes.ListOptions{
-			Sort: sqltypes.Sort{
-				Fields: [][]string{{"metadata", "somefield"}},
-				Orders: []sqltypes.SortOrder{sqltypes.ASC},
+			SortList: sqltypes.SortList{
+				SortDirectives: []sqltypes.Sort{
+					{
+						Fields: []string{"metadata", "somefield"},
+						Order:  sqltypes.ASC,
+					},
+				},
 			},
 		},
 		partitions: []partition.Partition{},
@@ -670,9 +674,13 @@ func TestListByOptions(t *testing.T) {
 	tests = append(tests, testCase{
 		description: "sort one field descending",
 		listOptions: sqltypes.ListOptions{
-			Sort: sqltypes.Sort{
-				Fields: [][]string{{"metadata", "somefield"}},
-				Orders: []sqltypes.SortOrder{sqltypes.DESC},
+			SortList: sqltypes.SortList{
+				SortDirectives: []sqltypes.Sort{
+					{
+						Fields: []string{"metadata", "somefield"},
+						Order:  sqltypes.DESC,
+					},
+				},
 			},
 		},
 		partitions: []partition.Partition{},
@@ -693,9 +701,13 @@ func TestListByOptions(t *testing.T) {
 	tests = append(tests, testCase{
 		description: "sort one unbound label descending",
 		listOptions: sqltypes.ListOptions{
-			Sort: sqltypes.Sort{
-				Fields: [][]string{{"metadata", "labels", "flip"}},
-				Orders: []sqltypes.SortOrder{sqltypes.DESC},
+			SortList: sqltypes.SortList{
+				SortDirectives: []sqltypes.Sort{
+					{
+						Fields: []string{"metadata", "labels", "flip"},
+						Order:  sqltypes.DESC,
+					},
+				},
 			},
 		},
 		partitions: []partition.Partition{},
@@ -718,9 +730,17 @@ func TestListByOptions(t *testing.T) {
 	tests = append(tests, testCase{
 		description: "ListByOptions sorting on two complex fields should sort on the first field in ascending order first and then sort on the second labels field in ascending order in prepared sql.Stmt",
 		listOptions: sqltypes.ListOptions{
-			Sort: sqltypes.Sort{
-				Fields: [][]string{{"metadata", "fields", "3"}, {"metadata", "labels", "stub.io/candy"}},
-				Orders: []sqltypes.SortOrder{sqltypes.ASC, sqltypes.ASC},
+			SortList: sqltypes.SortList{
+				SortDirectives: []sqltypes.Sort{
+					{
+						Fields: []string{"metadata", "fields", "3"},
+						Order:  sqltypes.ASC,
+					},
+					{
+						Fields: []string{"metadata", "labels", "stub.io/candy"},
+						Order:  sqltypes.ASC,
+					},
+				},
 			},
 		},
 		extraIndexedFields: []string{"metadata.fields[3]", "metadata.labels[stub.io/candy]"},
@@ -742,9 +762,17 @@ func TestListByOptions(t *testing.T) {
 	tests = append(tests, testCase{
 		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: sqltypes.ListOptions{
-			Sort: sqltypes.Sort{
-				Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}},
-				Orders: []sqltypes.SortOrder{sqltypes.ASC, sqltypes.ASC},
+			SortList: sqltypes.SortList{
+				SortDirectives: []sqltypes.Sort{
+					{
+						Fields: []string{"metadata", "somefield"},
+						Order:  sqltypes.ASC,
+					},
+					{
+						Fields: []string{"status", "someotherfield"},
+						Order:  sqltypes.ASC,
+					},
+				},
 			},
 		},
 		partitions: []partition.Partition{},
@@ -763,9 +791,17 @@ func TestListByOptions(t *testing.T) {
 	tests = append(tests, testCase{
 		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: sqltypes.ListOptions{
-			Sort: sqltypes.Sort{
-				Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}},
-				Orders: []sqltypes.SortOrder{sqltypes.DESC, sqltypes.ASC},
+			SortList: sqltypes.SortList{
+				SortDirectives: []sqltypes.Sort{
+					{
+						Fields: []string{"metadata", "somefield"},
+						Order:  sqltypes.DESC,
+					},
+					{
+						Fields: []string{"status", "someotherfield"},
+						Order:  sqltypes.ASC,
+					},
+				},
 			},
 		},
 		partitions: []partition.Partition{},
@@ -781,27 +817,6 @@ func TestListByOptions(t *testing.T) {
 		expectedErr:       nil,
 	})
 
-	tests = append(tests, testCase{
-		description: "ListByOptions sorting when # fields != # sort orders should return an error",
-		listOptions: sqltypes.ListOptions{
-			Sort: sqltypes.Sort{
-				Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}},
-				Orders: []sqltypes.SortOrder{sqltypes.DESC, sqltypes.ASC, sqltypes.ASC},
-			},
-		},
-		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."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:       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: sqltypes.ListOptions{
@@ -1548,9 +1563,13 @@ func TestConstructQuery(t *testing.T) {
 					},
 				},
 			},
-			Sort: sqltypes.Sort{
-				Fields: [][]string{{"metadata", "queryField1"}},
-				Orders: []sqltypes.SortOrder{sqltypes.ASC},
+			SortList: sqltypes.SortList{
+				SortDirectives: []sqltypes.Sort{
+					{
+						Fields: []string{"metadata", "queryField1"},
+						Order:  sqltypes.ASC,
+					},
+				},
 			},
 		},
 		partitions: []partition.Partition{},
@@ -1566,27 +1585,16 @@ func TestConstructQuery(t *testing.T) {
 		expectedErr:      nil,
 	})
 
-	tests = append(tests, testCase{
-		description: "ConstructQuery: sorting when # fields < # sort orders should return an error",
-		listOptions: sqltypes.ListOptions{
-			Sort: sqltypes.Sort{
-				Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}},
-				Orders: []sqltypes.SortOrder{sqltypes.DESC, sqltypes.ASC, sqltypes.ASC},
-			},
-		},
-		partitions:       []partition.Partition{},
-		ns:               "",
-		expectedStmt:     "",
-		expectedStmtArgs: []any{},
-		expectedErr:      fmt.Errorf("sort fields length 2 != sort orders length 3"),
-	})
-
 	tests = append(tests, testCase{
 		description: "TestConstructQuery: sort on label statements with no query",
 		listOptions: sqltypes.ListOptions{
-			Sort: sqltypes.Sort{
-				Fields: [][]string{{"metadata", "labels", "this"}},
-				Orders: []sqltypes.SortOrder{sqltypes.ASC},
+			SortList: sqltypes.SortList{
+				SortDirectives: []sqltypes.Sort{
+					{
+						Fields: []string{"metadata", "labels", "this"},
+						Order:  sqltypes.ASC,
+					},
+				},
 			},
 		},
 		partitions: []partition.Partition{},
@@ -1621,9 +1629,17 @@ func TestConstructQuery(t *testing.T) {
 					},
 				},
 			},
-			Sort: sqltypes.Sort{
-				Fields: [][]string{{"metadata", "labels", "this"}, {"status", "queryField2"}},
-				Orders: []sqltypes.SortOrder{sqltypes.ASC, sqltypes.DESC},
+			SortList: sqltypes.SortList{
+				SortDirectives: []sqltypes.Sort{
+					{
+						Fields: []string{"metadata", "labels", "this"},
+						Order:  sqltypes.ASC,
+					},
+					{
+						Fields: []string{"status", "queryField2"},
+						Order:  sqltypes.DESC,
+					},
+				},
 			},
 		},
 		partitions: []partition.Partition{},
@@ -1640,21 +1656,6 @@ func TestConstructQuery(t *testing.T) {
 		expectedErr:      nil,
 	})
 
-	tests = append(tests, testCase{
-		description: "ConstructQuery: sorting when # fields > # sort orders should return an error",
-		listOptions: sqltypes.ListOptions{
-			Sort: sqltypes.Sort{
-				Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}, {"metadata", "labels", "a1"}, {"metadata", "labels", "a2"}},
-				Orders: []sqltypes.SortOrder{sqltypes.DESC, sqltypes.ASC, sqltypes.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) {

pkg/sqlcache/sqltypes/types.go

@@ -28,7 +28,7 @@ type ListOptions struct {
 	ChunkSize  int
 	Resume     string
 	Filters    []OrFilter
-	Sort       Sort
+	SortList   SortList
 	Pagination Pagination
 }
 
@@ -57,8 +57,8 @@ type OrFilter struct {
 // 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 {
-	Fields [][]string
-	Orders []SortOrder
+	Fields []string
+	Order  SortOrder
 }
 
 type SortList struct {