rancher/steve
1
0
Fork 0
mirror of https://github.com/rancher/steve.git synced 2025-09-15 06:49:27 +00:00
Code Issues Projects Releases Wiki Activity

Fix the ListOptions sort field.

Browse Source 
Instead of making it a single array-ish field, convert it into a
true array of Sort Directives.  Easier to read, less bending backwards.
This commit is contained in:
Eric Promislow
2025-04-24 14:57:32 -07:00
parent f73b1e35b6
commit cc08df3708
5 changed files with 150 additions and 134 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
pkg/sqlcache/informer/listoption_indexer.go
View File

@@ -6,13 +6,13 @@ import (
"encoding/gob" "encoding/gob"
"errors" "errors"
"fmt" "fmt"
"github.com/rancher/steve/pkg/sqlcache/sqltypes"
"regexp" "regexp"
"sort" "sort"
"strconv" "strconv"
"strings" "strings"
"github.com/rancher/steve/pkg/sqlcache/db/transaction" "github.com/rancher/steve/pkg/sqlcache/db/transaction"
"github.com/rancher/steve/pkg/sqlcache/sqltypes"
"github.com/sirupsen/logrus" "github.com/sirupsen/logrus"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/cache"
@@ -389,26 +389,24 @@ func (l *ListOptionIndexer) constructQuery(lo sqltypes.ListOptions, partitions [
countParams := params[:] countParams := params[:]
// 3- Sorting: ORDER BY clauses (from lo.Sort) // 3- Sorting: ORDER BY clauses (from lo.Sort)
if len(lo.Sort.Fields) != len(lo.Sort.Orders) { if len(lo.SortList.SortDirectives) > 0 {
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{} orderByClauses := []string{}
for i, field := range lo.Sort.Fields { for _, sortDirective := range lo.SortList.SortDirectives {
if isLabelsFieldList(field) { fields := sortDirective.Fields
clause, sortParam, err := buildSortLabelsClause(field[2], joinTableIndexByLabelName, lo.Sort.Orders[i] == sqltypes.ASC) if isLabelsFieldList(fields) {
clause, sortParam, err := buildSortLabelsClause(fields[2], joinTableIndexByLabelName, sortDirective.Order == sqltypes.ASC)
if err != nil { if err != nil {
return nil, err return nil, err
} }
orderByClauses = append(orderByClauses, clause) orderByClauses = append(orderByClauses, clause)
params = append(params, sortParam) params = append(params, sortParam)
} else { } else {
columnName := toColumnName(field) columnName := toColumnName(fields)
if err := l.validateColumn(columnName); err != nil { if err := l.validateColumn(columnName); err != nil {
return queryInfo, err return queryInfo, err
} }
direction := "ASC" direction := "ASC"
if lo.Sort.Orders[i] == sqltypes.DESC { if sortDirective.Order == sqltypes.DESC {
direction = "DESC" direction = "DESC"
} }
orderByClauses = append(orderByClauses, fmt.Sprintf(`f."%s" %s`, columnName, direction)) 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. // created in Store.ListByPartitions, and that ends up calling ListOptionIndexer.ConstructQuery.
// No other goroutines access this object. // No other goroutines access this object.
func ensureSortLabelsAreSelected(lo *sqltypes.ListOptions) { func ensureSortLabelsAreSelected(lo *sqltypes.ListOptions) {
if len(lo.Sort.Fields) == 0 { if len(lo.SortList.SortDirectives) == 0 {
return return
} }
unboundSortLabels := make(map[string]bool) unboundSortLabels := make(map[string]bool)
for _, fieldList := range lo.Sort.Fields { for _, sortDirective := range lo.SortList.SortDirectives {
if isLabelsFieldList(fieldList) { fields := sortDirective.Fields
unboundSortLabels[fieldList[2]] = true if isLabelsFieldList(fields) {
unboundSortLabels[fields[2]] = true
} }
} }
if len(unboundSortLabels) == 0 { if len(unboundSortLabels) == 0 {

157
pkg/sqlcache/informer/listoption_indexer_test.go
View File

@@ -647,9 +647,13 @@ func TestListByOptions(t *testing.T) {
tests = append(tests, testCase{ 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", description: "ListByOptions with only one Sort.Field set should sort on that field only, in ascending order in prepared sql.Stmt",
listOptions: sqltypes.ListOptions{ listOptions: sqltypes.ListOptions{
Sort: sqltypes.Sort{ SortList: sqltypes.SortList{
Fields: [][]string{{"metadata", "somefield"}}, SortDirectives: []sqltypes.Sort{
Orders: []sqltypes.SortOrder{sqltypes.ASC}, {
Fields: []string{"metadata", "somefield"},
Order: sqltypes.ASC,
},
},
}, },
}, },
partitions: []partition.Partition{}, partitions: []partition.Partition{},
@@ -670,9 +674,13 @@ func TestListByOptions(t *testing.T) {
tests = append(tests, testCase{ tests = append(tests, testCase{
description: "sort one field descending", description: "sort one field descending",
listOptions: sqltypes.ListOptions{ listOptions: sqltypes.ListOptions{
Sort: sqltypes.Sort{ SortList: sqltypes.SortList{
Fields: [][]string{{"metadata", "somefield"}}, SortDirectives: []sqltypes.Sort{
Orders: []sqltypes.SortOrder{sqltypes.DESC}, {
Fields: []string{"metadata", "somefield"},
Order: sqltypes.DESC,
},
},
}, },
}, },
partitions: []partition.Partition{}, partitions: []partition.Partition{},
@@ -693,9 +701,13 @@ func TestListByOptions(t *testing.T) {
tests = append(tests, testCase{ tests = append(tests, testCase{
description: "sort one unbound label descending", description: "sort one unbound label descending",
listOptions: sqltypes.ListOptions{ listOptions: sqltypes.ListOptions{
Sort: sqltypes.Sort{ SortList: sqltypes.SortList{
Fields: [][]string{{"metadata", "labels", "flip"}}, SortDirectives: []sqltypes.Sort{
Orders: []sqltypes.SortOrder{sqltypes.DESC}, {
Fields: []string{"metadata", "labels", "flip"},
Order: sqltypes.DESC,
},
},
}, },
}, },
partitions: []partition.Partition{}, partitions: []partition.Partition{},
@@ -718,9 +730,17 @@ func TestListByOptions(t *testing.T) {
tests = append(tests, testCase{ 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", 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{ listOptions: sqltypes.ListOptions{
Sort: sqltypes.Sort{ SortList: sqltypes.SortList{
Fields: [][]string{{"metadata", "fields", "3"}, {"metadata", "labels", "stub.io/candy"}}, SortDirectives: []sqltypes.Sort{
Orders: []sqltypes.SortOrder{sqltypes.ASC, sqltypes.ASC}, {
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]"}, extraIndexedFields: []string{"metadata.fields[3]", "metadata.labels[stub.io/candy]"},
@@ -742,9 +762,17 @@ func TestListByOptions(t *testing.T) {
tests = append(tests, testCase{ 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", 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{ listOptions: sqltypes.ListOptions{
Sort: sqltypes.Sort{ SortList: sqltypes.SortList{
Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}}, SortDirectives: []sqltypes.Sort{
Orders: []sqltypes.SortOrder{sqltypes.ASC, sqltypes.ASC}, {
Fields: []string{"metadata", "somefield"},
Order: sqltypes.ASC,
},
{
Fields: []string{"status", "someotherfield"},
Order: sqltypes.ASC,
},
},
}, },
}, },
partitions: []partition.Partition{}, partitions: []partition.Partition{},
@@ -763,9 +791,17 @@ func TestListByOptions(t *testing.T) {
tests = append(tests, testCase{ 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", 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{ listOptions: sqltypes.ListOptions{
Sort: sqltypes.Sort{ SortList: sqltypes.SortList{
Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}}, SortDirectives: []sqltypes.Sort{
Orders: []sqltypes.SortOrder{sqltypes.DESC, sqltypes.ASC}, {
Fields: []string{"metadata", "somefield"},
Order: sqltypes.DESC,
},
{
Fields: []string{"status", "someotherfield"},
Order: sqltypes.ASC,
},
},
}, },
}, },
partitions: []partition.Partition{}, partitions: []partition.Partition{},
@@ -781,27 +817,6 @@ func TestListByOptions(t *testing.T) {
expectedErr: nil, 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{ tests = append(tests, testCase{
description: "ListByOptions with Pagination.PageSize set should set limit to PageSize in prepared sql.Stmt", description: "ListByOptions with Pagination.PageSize set should set limit to PageSize in prepared sql.Stmt",
listOptions: sqltypes.ListOptions{ listOptions: sqltypes.ListOptions{
@@ -1548,9 +1563,13 @@ func TestConstructQuery(t *testing.T) {
}, },
}, },
}, },
Sort: sqltypes.Sort{ SortList: sqltypes.SortList{
Fields: [][]string{{"metadata", "queryField1"}}, SortDirectives: []sqltypes.Sort{
Orders: []sqltypes.SortOrder{sqltypes.ASC}, {
Fields: []string{"metadata", "queryField1"},
Order: sqltypes.ASC,
},
},
}, },
}, },
partitions: []partition.Partition{}, partitions: []partition.Partition{},
@@ -1566,27 +1585,16 @@ func TestConstructQuery(t *testing.T) {
expectedErr: nil, 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{ tests = append(tests, testCase{
description: "TestConstructQuery: sort on label statements with no query", description: "TestConstructQuery: sort on label statements with no query",
listOptions: sqltypes.ListOptions{ listOptions: sqltypes.ListOptions{
Sort: sqltypes.Sort{ SortList: sqltypes.SortList{
Fields: [][]string{{"metadata", "labels", "this"}}, SortDirectives: []sqltypes.Sort{
Orders: []sqltypes.SortOrder{sqltypes.ASC}, {
Fields: []string{"metadata", "labels", "this"},
Order: sqltypes.ASC,
},
},
}, },
}, },
partitions: []partition.Partition{}, partitions: []partition.Partition{},
@@ -1621,9 +1629,17 @@ func TestConstructQuery(t *testing.T) {
}, },
}, },
}, },
Sort: sqltypes.Sort{ SortList: sqltypes.SortList{
Fields: [][]string{{"metadata", "labels", "this"}, {"status", "queryField2"}}, SortDirectives: []sqltypes.Sort{
Orders: []sqltypes.SortOrder{sqltypes.ASC, sqltypes.DESC}, {
Fields: []string{"metadata", "labels", "this"},
Order: sqltypes.ASC,
},
{
Fields: []string{"status", "queryField2"},
Order: sqltypes.DESC,
},
},
}, },
}, },
partitions: []partition.Partition{}, partitions: []partition.Partition{},
@@ -1640,21 +1656,6 @@ func TestConstructQuery(t *testing.T) {
expectedErr: nil, 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() t.Parallel()
for _, test := range tests { for _, test := range tests {
t.Run(test.description, func(t *testing.T) { t.Run(test.description, func(t *testing.T) {

16
pkg/sqlcache/sqltypes/types.go
View File

@@ -28,7 +28,7 @@ type ListOptions struct {
ChunkSize int ChunkSize int
Resume string Resume string
Filters []OrFilter Filters []OrFilter
Sort Sort SortList SortList
Pagination Pagination Pagination Pagination
} }
@@ -40,10 +40,10 @@ type ListOptions struct {
// //
// If more than one value is given for the `Match` field, we do an "IN (<values>)" test // If more than one value is given for the `Match` field, we do an "IN (<values>)" test
type Filter struct { type Filter struct {
Field []string Field []string
Matches []string Matches []string
Op Op Op Op
Partial bool Partial bool
} }
// OrFilter represents a set of possible fields to filter by, where an item may match any filter in the set to be included in the result. // OrFilter represents a set of possible fields to filter by, where an item may match any filter in the set to be included in the result.
@@ -57,8 +57,8 @@ type OrFilter struct {
// The order is represented by prefixing the sort key by '-', e.g. sort=-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 // e.g. To sort internal clusters first followed by clusters in alpha order: sort=-spec.internal,spec.displayName
type Sort struct { type Sort struct {
Fields [][]string Fields []string
Orders []SortOrder Order SortOrder
} }
type SortList struct { type SortList struct {
@@ -68,7 +68,7 @@ type SortList struct {
// Pagination represents how to return paginated results. // Pagination represents how to return paginated results.
type Pagination struct { type Pagination struct {
PageSize int PageSize int
Page int Page int
} }
func NewSortList() *SortList { func NewSortList() *SortList {

20
pkg/stores/sqlpartition/listprocessor/processor.go
View File

@@ -50,15 +50,6 @@ var mapK8sOpToRancherOp = map[selection.Operator]sqltypes.Op{
selection.GreaterThan: sqltypes.Gt, selection.GreaterThan: sqltypes.Gt,
} }
// ListOptions represents the query parameters that may be included in a list request.
type ListOptions struct {
ChunkSize int
Resume string
Filters []sqltypes.OrFilter
Sort sqltypes.Sort
Pagination sqltypes.Pagination
}
type Cache interface { type Cache interface {
// ListByOptions returns objects according to the specified list options and partitions. // ListByOptions returns objects according to the specified list options and partitions.
// Specifically: // Specifically:
@@ -118,9 +109,9 @@ func ParseQuery(apiOp *types.APIRequest, namespaceCache Cache) (sqltypes.ListOpt
} }
opts.Filters = filterOpts opts.Filters = filterOpts
sortOpts := sqltypes.Sort{}
sortKeys := q.Get(sortParam) sortKeys := q.Get(sortParam)
if sortKeys != "" { if sortKeys != "" {
sortList := *sqltypes.NewSortList()
sortParts := strings.Split(sortKeys, ",") sortParts := strings.Split(sortKeys, ",")
for _, sortPart := range sortParts { for _, sortPart := range sortParts {
field := sortPart field := sortPart
@@ -131,13 +122,16 @@ func ParseQuery(apiOp *types.APIRequest, namespaceCache Cache) (sqltypes.ListOpt
field = field[1:] field = field[1:]
} }
if len(field) > 0 { if len(field) > 0 {
sortOpts.Fields = append(sortOpts.Fields, queryhelper.SafeSplit(field)) sortDirective := sqltypes.Sort{
sortOpts.Orders = append(sortOpts.Orders, sortOrder) Fields: queryhelper.SafeSplit(field),
Order: sortOrder,
}
sortList.SortDirectives = append(sortList.SortDirectives, sortDirective)
} }
} }
} }
opts.SortList = sortList
} }
opts.Sort = sortOpts
var err error var err error
pagination := sqltypes.Pagination{} pagination := sqltypes.Pagination{}

64
pkg/stores/sqlpartition/listprocessor/processor_test.go
View File

@@ -744,10 +744,13 @@ func TestParseQuery(t *testing.T) {
}, },
expectedLO: sqltypes.ListOptions{ expectedLO: sqltypes.ListOptions{
ChunkSize: defaultLimit, ChunkSize: defaultLimit,
Sort: sqltypes.Sort{ SortList: sqltypes.SortList{
Fields: [][]string{ SortDirectives: []sqltypes.Sort{
{"metadata", "name"}}, {
Orders: []sqltypes.SortOrder{sqltypes.ASC}, Fields: []string{"metadata", "name"},
Order: sqltypes.ASC,
},
},
}, },
Filters: make([]sqltypes.OrFilter, 0), Filters: make([]sqltypes.OrFilter, 0),
Pagination: sqltypes.Pagination{ Pagination: sqltypes.Pagination{
@@ -765,9 +768,13 @@ func TestParseQuery(t *testing.T) {
}, },
expectedLO: sqltypes.ListOptions{ expectedLO: sqltypes.ListOptions{
ChunkSize: defaultLimit, ChunkSize: defaultLimit,
Sort: sqltypes.Sort{ SortList: sqltypes.SortList{
Fields: [][]string{{"metadata", "name"}}, SortDirectives: []sqltypes.Sort{
Orders: []sqltypes.SortOrder{sqltypes.DESC}, {
Fields: []string{"metadata", "name"},
Order: sqltypes.DESC,
},
},
}, },
Filters: make([]sqltypes.OrFilter, 0), Filters: make([]sqltypes.OrFilter, 0),
Pagination: sqltypes.Pagination{ Pagination: sqltypes.Pagination{
@@ -785,14 +792,16 @@ func TestParseQuery(t *testing.T) {
}, },
expectedLO: sqltypes.ListOptions{ expectedLO: sqltypes.ListOptions{
ChunkSize: defaultLimit, ChunkSize: defaultLimit,
Sort: sqltypes.Sort{ SortList: sqltypes.SortList{
Fields: [][]string{ SortDirectives: []sqltypes.Sort{
{"metadata", "name"}, {
{"spec", "something"}, Fields: []string{"metadata", "name"},
}, Order: sqltypes.DESC,
Orders: []sqltypes.SortOrder{ },
sqltypes.DESC, {
sqltypes.ASC, Fields: []string{"spec", "something"},
Order: sqltypes.ASC,
},
}, },
}, },
Filters: make([]sqltypes.OrFilter, 0), Filters: make([]sqltypes.OrFilter, 0),
@@ -811,12 +820,25 @@ func TestParseQuery(t *testing.T) {
}, },
expectedLO: sqltypes.ListOptions{ expectedLO: sqltypes.ListOptions{
ChunkSize: defaultLimit, ChunkSize: defaultLimit,
Sort: sqltypes.Sort{ SortList: sqltypes.SortList{
Fields: [][]string{{"metadata", "labels", "beef.cattle.io/snort"}, SortDirectives: []sqltypes.Sort{
{"metadata", "labels", "steer"}, {
{"metadata", "labels", "bossie.cattle.io/moo"}, Fields: []string{"metadata", "labels", "beef.cattle.io/snort"},
{"spec", "something"}}, Order: sqltypes.DESC,
Orders: []sqltypes.SortOrder{sqltypes.DESC, sqltypes.ASC, sqltypes.ASC, sqltypes.ASC}, },
{
Fields: []string{"metadata", "labels", "steer"},
Order: sqltypes.ASC,
},
{
Fields: []string{"metadata", "labels", "bossie.cattle.io/moo"},
Order: sqltypes.ASC,
},
{
Fields: []string{"spec", "something"},
Order: sqltypes.ASC,
},
},
}, },
Filters: make([]sqltypes.OrFilter, 0), Filters: make([]sqltypes.OrFilter, 0),
Pagination: sqltypes.Pagination{ Pagination: sqltypes.Pagination{