From 2cd7997e6b4cdde18cfe56e34e3559dd7cc00f00 Mon Sep 17 00:00:00 2001 From: Eric Promislow Date: Fri, 13 Jun 2025 10:18:48 -0700 Subject: [PATCH] Fix non-vai sorting involving string arrays, add tests. (#618) - Bump wrangler to get fix for 'GetValueFromAny'. This adds string arrays to the types it supports. - Use a newer go library sort function to maintain a stable sort - Use the map-sort-map pattern to avoid repeatedly calculating the same underlying value for `object[arg1][arg2][...]` --- go.mod | 2 +- go.sum | 4 +- .../partition/listprocessor/processor.go | 65 +++- .../partition/listprocessor/processor_test.go | 303 ++++++++++++++++++ 4 files changed, 357 insertions(+), 17 deletions(-) diff --git a/go.mod b/go.mod index 553f6d77..369c2fe3 100644 --- a/go.mod +++ b/go.mod @@ -26,7 +26,7 @@ require ( github.com/rancher/lasso v0.2.2 github.com/rancher/norman v0.6.0 github.com/rancher/remotedialer v0.4.5-rc.1 - github.com/rancher/wrangler/v3 v3.2.1-rc.5 + github.com/rancher/wrangler/v3 v3.2.1 github.com/sirupsen/logrus v1.9.3 github.com/stretchr/testify v1.10.0 github.com/urfave/cli/v2 v2.27.5 diff --git a/go.sum b/go.sum index 72b50f48..176ae1fd 100644 --- a/go.sum +++ b/go.sum @@ -230,8 +230,8 @@ github.com/rancher/norman v0.6.0 h1:8CyY9cVcw0L+YYZRGvXHPMENefLaDkmi6vpscrVjXew= github.com/rancher/norman v0.6.0/go.mod h1:CQ0/1CoEqbeJXvOXuRQvsmiBd7QIgxZG5IvQFOPAHOg= github.com/rancher/remotedialer v0.4.5-rc.1 h1:LwGrOqt6hrKjf9Er5LBEKP6xk21RFF7PO2d0+E+mavk= github.com/rancher/remotedialer v0.4.5-rc.1/go.mod h1:x31ZR9714VzudfHVke40+WN5wDSDckxjRGr1bWgpgc0= -github.com/rancher/wrangler/v3 v3.2.1-rc.5 h1:hbhOcF0YyQty7PzTpEQpRdH4DZ74bWDkCp2a4TrUeRA= -github.com/rancher/wrangler/v3 v3.2.1-rc.5/go.mod h1:RV8kkv5br5HaxXWamIbr95pOjvVeoC5CeBldcdw5Fv0= +github.com/rancher/wrangler/v3 v3.2.1 h1:V51PnoGb8bZ5jJdxFlqKQApzWdSp4sEy5OPGuEGqVbI= +github.com/rancher/wrangler/v3 v3.2.1/go.mod h1:RV8kkv5br5HaxXWamIbr95pOjvVeoC5CeBldcdw5Fv0= github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec h1:W09IVJc94icq4NjY3clb7Lk8O1qJ8BdBEF8z0ibU0rE= github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec/go.mod h1:qqbHyh8v60DhA7CoWK5oRCqLrMHRGoxYCSS9EjAz6Eo= github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8= diff --git a/pkg/stores/partition/listprocessor/processor.go b/pkg/stores/partition/listprocessor/processor.go index 9b39d084..f2385838 100644 --- a/pkg/stores/partition/listprocessor/processor.go +++ b/pkg/stores/partition/listprocessor/processor.go @@ -4,6 +4,7 @@ package listprocessor import ( "fmt" "regexp" + "slices" "sort" "strconv" "strings" @@ -13,6 +14,7 @@ import ( "github.com/rancher/wrangler/v3/pkg/data" "github.com/rancher/wrangler/v3/pkg/data/convert" corecontrollers "github.com/rancher/wrangler/v3/pkg/generated/controllers/core/v1" + "github.com/sirupsen/logrus" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) @@ -353,26 +355,61 @@ func matchesAll(obj map[string]interface{}, filters []OrFilter) bool { } // SortList sorts the slice by the provided sort criteria. +// Convert the sorted list so we calculate the sort values only once per node, +// making this an O(n) operation rather than an O(n**2) one. +// +// list of u.U => list of { *u.U, []stringValues } => +// sorted list of { *u.U, []stringValues } => +// final list of u.U from the above sorted list +// +// We do this because it's much faster to compare two string arrays then to compare +// the string arrays based on walking objects repeatedly func SortList(list []unstructured.Unstructured, s Sort) []unstructured.Unstructured { if len(s.Fields) == 0 { return list } - sort.Slice(list, func(i, j int) bool { - leftNode := list[i].Object - rightNode := list[j].Object - for i, field := range s.Fields { - leftValue := convert.ToString(data.GetValueN(leftNode, field...)) - rightValue := convert.ToString(data.GetValueN(rightNode, field...)) - if leftValue != rightValue { - if s.Orders[i] == ASC { - return leftValue < rightValue - } - return rightValue < leftValue + type transformedData struct { + originalItem *unstructured.Unstructured + stringValues []string + } + transformedList := make([]transformedData, len(list)) + for i := range list { + td := &transformedList[i] + td.originalItem = &list[i] + td.stringValues = make([]string, len(s.Fields)) + for j, fields := range s.Fields { + val, ok := data.GetValueFromAny(list[i].Object, fields...) + if ok { + td.stringValues[j] = val.(string) + } else { + logrus.Debugf("Failed to walk list item %d with fields %s", i, fields) + td.stringValues[j] = "" } } - return false - }) - return list + } + // We can't use slices.Compare(leftNode.stringValues, rightNode.stringValues) + // because some comparisons might use ASC-order, and others DESC-order. + sortFunc := func(leftNode, rightNode transformedData) int { + for i := range leftNode.stringValues { + diff := strings.Compare(leftNode.stringValues[i], rightNode.stringValues[i]) + if diff == 0 { + // the two substrings are the same, so continue walking the arrays + continue + } + if s.Orders[i] == DESC { + // reversing sort order just means inverting the comparison value + diff *= -1 + } + return diff + } + return 0 + } + slices.SortStableFunc(transformedList, sortFunc) + newList := make([]unstructured.Unstructured, len(list)) + for i := range transformedList { + newList[i] = *transformedList[i].originalItem + } + return newList } // PaginateList returns a subset of the result based on the pagination criteria as well as the total number of pages the caller can expect. diff --git a/pkg/stores/partition/listprocessor/processor_test.go b/pkg/stores/partition/listprocessor/processor_test.go index 173690db..21bc22ab 100644 --- a/pkg/stores/partition/listprocessor/processor_test.go +++ b/pkg/stores/partition/listprocessor/processor_test.go @@ -2362,6 +2362,309 @@ func TestSortList(t *testing.T) { }, }, }, + { + name: "sort metadata.fields[1]", + objects: []unstructured.Unstructured{ + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "fuji", + "fields": []string{ + "a1", + "position3", // this is where we expect to see this block after sorting + }, + }, + "data": map[string]interface{}{ + "color": "pink", + }, + }, + }, + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "honeycrisp", + "fields": []string{ + "a2", + "position1", + }, + }, + "data": map[string]interface{}{ + "color": "pink", + }, + }, + }, + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "granny-smith", + "fields": []string{ + "a3", + "position2", + }, + }, + "data": map[string]interface{}{ + "color": "green", + }, + }, + }, + }, + sort: Sort{ + Fields: [][]string{{"metadata", "fields", "1"}}, + Orders: []SortOrder{ASC}, + }, + want: []unstructured.Unstructured{ + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "honeycrisp", + "fields": []string{ + "a2", + "position1", + }, + }, + "data": map[string]interface{}{ + "color": "pink", + }, + }, + }, + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "granny-smith", + "fields": []string{ + "a3", + "position2", + }, + }, + "data": map[string]interface{}{ + "color": "green", + }, + }, + }, + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "fuji", + "fields": []string{ + "a1", + "position3", + }, + }, + "data": map[string]interface{}{ + "color": "pink", + }, + }, + }, + }, + }, + { + name: "sort metadata.fields[1] reverse order", + objects: []unstructured.Unstructured{ + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "fuji", + "fields": []string{ + "a1", + "position3", // this is where we expect to see this block after sorting, but in reverse + }, + }, + "data": map[string]interface{}{ + "color": "pink", + }, + }, + }, + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "honeycrisp", + "fields": []string{ + "a2", + "position1", + }, + }, + "data": map[string]interface{}{ + "color": "pink", + }, + }, + }, + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "granny-smith", + "fields": []string{ + "a3", + "position2", + }, + }, + "data": map[string]interface{}{ + "color": "green", + }, + }, + }, + }, + sort: Sort{ + Fields: [][]string{{"metadata", "fields", "1"}}, + Orders: []SortOrder{DESC}, + }, + want: []unstructured.Unstructured{ + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "fuji", + "fields": []string{ + "a1", + "position3", + }, + }, + "data": map[string]interface{}{ + "color": "pink", + }, + }, + }, + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "granny-smith", + "fields": []string{ + "a3", + "position2", + }, + }, + "data": map[string]interface{}{ + "color": "green", + }, + }, + }, + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "honeycrisp", + "fields": []string{ + "a2", + "position1", + }, + }, + "data": map[string]interface{}{ + "color": "pink", + }, + }, + }, + }, + }, + { + name: "sort metadata.fields[1], preserve ties", + objects: []unstructured.Unstructured{ + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "fuji", + "fields": []string{ + "a1", + "position3", // this is where we expect to see this block after sorting + }, + }, + "data": map[string]interface{}{ + "color": "pink", + }, + }, + }, + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "honeycrisp", + "fields": []string{ + "a2", + "position1", + }, + }, + "data": map[string]interface{}{ + "color": "pink", + }, + }, + }, + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "granny-smith", + "fields": []string{ + "a3", + "position1", // but should be second + }, + }, + "data": map[string]interface{}{ + "color": "green", + }, + }, + }, + }, + sort: Sort{ + Fields: [][]string{{"metadata", "fields", "1"}}, + Orders: []SortOrder{ASC}, + }, + want: []unstructured.Unstructured{ + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "honeycrisp", + "fields": []string{ + "a2", + "position1", + }, + }, + "data": map[string]interface{}{ + "color": "pink", + }, + }, + }, + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "granny-smith", + "fields": []string{ + "a3", + "position1", + }, + }, + "data": map[string]interface{}{ + "color": "green", + }, + }, + }, + { + Object: map[string]interface{}{ + "kind": "apple", + "metadata": map[string]interface{}{ + "name": "fuji", + "fields": []string{ + "a1", + "position3", + }, + }, + "data": map[string]interface{}{ + "color": "pink", + }, + }, + }, + }, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) {