From 57ce685118c8a9391c69e4dfc36d78d667f68a9e Mon Sep 17 00:00:00 2001 From: Eric Promislow Date: Fri, 25 Apr 2025 11:19:34 -0700 Subject: [PATCH] Sort-indirect PR broken into smaller parts: part 3/6 - pass listOptions by reference (#612) * 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. * Pass the listOptions struct by reference to avoid copying. We never update the ListOptions struct once it's created so there's no need to pass it by value. This might be a near-useless optimization... --- pkg/sqlcache/informer/informer.go | 6 +++--- pkg/sqlcache/informer/informer_mocks_test.go | 2 +- pkg/sqlcache/informer/informer_test.go | 8 ++++---- pkg/sqlcache/informer/listoption_indexer.go | 6 +++--- pkg/sqlcache/informer/listoption_indexer_test.go | 4 ++-- pkg/sqlcache/integration_test.go | 4 ++-- pkg/stores/sqlpartition/listprocessor/processor.go | 4 ++-- pkg/stores/sqlpartition/listprocessor/processor_test.go | 6 +++--- pkg/stores/sqlpartition/listprocessor/proxy_mocks_test.go | 2 +- pkg/stores/sqlproxy/proxy_mocks_test.go | 2 +- pkg/stores/sqlproxy/proxy_store.go | 4 ++-- pkg/stores/sqlproxy/proxy_store_test.go | 6 +++--- pkg/stores/sqlproxy/sql_informer_mocks_test.go | 2 +- 13 files changed, 28 insertions(+), 28 deletions(-) diff --git a/pkg/sqlcache/informer/informer.go b/pkg/sqlcache/informer/informer.go index 7e240a23..dd162662 100644 --- a/pkg/sqlcache/informer/informer.go +++ b/pkg/sqlcache/informer/informer.go @@ -30,7 +30,7 @@ type Informer struct { } type ByOptionsLister interface { - ListByOptions(ctx context.Context, lo sqltypes.ListOptions, partitions []partition.Partition, namespace string) (*unstructured.UnstructuredList, int, string, error) + ListByOptions(ctx context.Context, lo *sqltypes.ListOptions, partitions []partition.Partition, namespace string) (*unstructured.UnstructuredList, int, string, error) } // this is set to a var so that it can be overridden by test code for mocking purposes @@ -66,7 +66,7 @@ func NewInformer(ctx context.Context, client dynamic.ResourceInterface, fields [ // copy of the known state of all objects (in an Indexer). // The resync period option here is passed from Informer to Reflector to periodically (re)-push all known // objects to the DeltaFIFO. That causes the periodic (re-)firing all registered handlers. - // sqltypes.In this case we are not registering any handlers to this particular informer, so re-syncing is a no-op. + // In this case we are not registering any handlers to this particular informer, so re-syncing is a no-op. // We therefore just disable it right away. resyncPeriod := time.Duration(0) @@ -103,7 +103,7 @@ func NewInformer(ctx context.Context, client dynamic.ResourceInterface, fields [ // - the total number of resources (returned list might be a subset depending on pagination options in lo) // - a continue token, if there are more pages after the returned one // - an error instead of all of the above if anything went wrong -func (i *Informer) ListByOptions(ctx context.Context, lo sqltypes.ListOptions, partitions []partition.Partition, namespace string) (*unstructured.UnstructuredList, int, string, error) { +func (i *Informer) ListByOptions(ctx context.Context, lo *sqltypes.ListOptions, partitions []partition.Partition, namespace string) (*unstructured.UnstructuredList, int, string, error) { return i.ByOptionsLister.ListByOptions(ctx, lo, partitions, namespace) } diff --git a/pkg/sqlcache/informer/informer_mocks_test.go b/pkg/sqlcache/informer/informer_mocks_test.go index 8e91e4fa..4cde0b67 100644 --- a/pkg/sqlcache/informer/informer_mocks_test.go +++ b/pkg/sqlcache/informer/informer_mocks_test.go @@ -43,7 +43,7 @@ func (m *MockByOptionsLister) EXPECT() *MockByOptionsListerMockRecorder { } // ListByOptions mocks base method. -func (m *MockByOptionsLister) ListByOptions(arg0 context.Context, arg1 sqltypes.ListOptions, arg2 []partition.Partition, arg3 string) (*unstructured.UnstructuredList, int, string, error) { +func (m *MockByOptionsLister) ListByOptions(arg0 context.Context, arg1 *sqltypes.ListOptions, arg2 []partition.Partition, arg3 string) (*unstructured.UnstructuredList, int, string, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ListByOptions", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(*unstructured.UnstructuredList) diff --git a/pkg/sqlcache/informer/informer_test.go b/pkg/sqlcache/informer/informer_test.go index bf3c8948..b030e1d4 100644 --- a/pkg/sqlcache/informer/informer_test.go +++ b/pkg/sqlcache/informer/informer_test.go @@ -325,8 +325,8 @@ func TestInformerListByOptions(t *testing.T) { } expectedTotal := len(expectedList.Items) expectedContinueToken := "123" - indexer.EXPECT().ListByOptions(context.Background(), lo, partitions, ns).Return(expectedList, expectedTotal, expectedContinueToken, nil) - list, total, continueToken, err := informer.ListByOptions(context.Background(), lo, partitions, ns) + indexer.EXPECT().ListByOptions(context.Background(), &lo, partitions, ns).Return(expectedList, expectedTotal, expectedContinueToken, nil) + list, total, continueToken, err := informer.ListByOptions(context.Background(), &lo, partitions, ns) assert.Nil(t, err) assert.Equal(t, expectedList, list) assert.Equal(t, len(expectedList.Items), total) @@ -340,8 +340,8 @@ func TestInformerListByOptions(t *testing.T) { lo := sqltypes.ListOptions{} var partitions []partition.Partition ns := "somens" - indexer.EXPECT().ListByOptions(context.Background(), lo, partitions, ns).Return(nil, 0, "", fmt.Errorf("error")) - _, _, _, err := informer.ListByOptions(context.Background(), lo, partitions, ns) + indexer.EXPECT().ListByOptions(context.Background(), &lo, partitions, ns).Return(nil, 0, "", fmt.Errorf("error")) + _, _, _, err := informer.ListByOptions(context.Background(), &lo, partitions, ns) assert.NotNil(t, err) }}) t.Parallel() diff --git a/pkg/sqlcache/informer/listoption_indexer.go b/pkg/sqlcache/informer/listoption_indexer.go index 412ab9ef..6aec0112 100644 --- a/pkg/sqlcache/informer/listoption_indexer.go +++ b/pkg/sqlcache/informer/listoption_indexer.go @@ -248,7 +248,7 @@ func (l *ListOptionIndexer) deleteLabels(key string, tx transaction.Client) erro // - the total number of resources (returned list might be a subset depending on pagination options in lo) // - a continue token, if there are more pages after the returned one // - an error instead of all of the above if anything went wrong -func (l *ListOptionIndexer) ListByOptions(ctx context.Context, lo sqltypes.ListOptions, partitions []partition.Partition, namespace string) (*unstructured.UnstructuredList, int, string, error) { +func (l *ListOptionIndexer) ListByOptions(ctx context.Context, lo *sqltypes.ListOptions, partitions []partition.Partition, namespace string) (*unstructured.UnstructuredList, int, string, error) { queryInfo, err := l.constructQuery(lo, partitions, namespace, db.Sanitize(l.GetName())) if err != nil { return nil, 0, "", err @@ -267,8 +267,8 @@ type QueryInfo struct { offset int } -func (l *ListOptionIndexer) constructQuery(lo sqltypes.ListOptions, partitions []partition.Partition, namespace string, dbName string) (*QueryInfo, error) { - ensureSortLabelsAreSelected(&lo) +func (l *ListOptionIndexer) constructQuery(lo *sqltypes.ListOptions, partitions []partition.Partition, namespace string, dbName string) (*QueryInfo, error) { + ensureSortLabelsAreSelected(lo) queryInfo := &QueryInfo{} queryUsesLabels := hasLabelFilter(lo.Filters) joinTableIndexByLabelName := make(map[string]int) diff --git a/pkg/sqlcache/informer/listoption_indexer_test.go b/pkg/sqlcache/informer/listoption_indexer_test.go index 16255ecd..4248edf1 100644 --- a/pkg/sqlcache/informer/listoption_indexer_test.go +++ b/pkg/sqlcache/informer/listoption_indexer_test.go @@ -978,7 +978,7 @@ func TestListByOptions(t *testing.T) { if len(test.extraIndexedFields) > 0 { lii.indexedFields = append(lii.indexedFields, test.extraIndexedFields...) } - queryInfo, err := lii.constructQuery(test.listOptions, test.partitions, test.ns, "something") + queryInfo, err := lii.constructQuery(&test.listOptions, test.partitions, test.ns, "something") if test.expectedErr != nil { assert.Equal(t, test.expectedErr, err) return @@ -1667,7 +1667,7 @@ func TestConstructQuery(t *testing.T) { Indexer: i, indexedFields: []string{"metadata.queryField1", "status.queryField2"}, } - queryInfo, err := lii.constructQuery(test.listOptions, test.partitions, test.ns, "something") + queryInfo, err := lii.constructQuery(&test.listOptions, test.partitions, test.ns, "something") if test.expectedErr != nil { assert.Equal(t, test.expectedErr, err) return diff --git a/pkg/sqlcache/integration_test.go b/pkg/sqlcache/integration_test.go index dcc13bbd..f88102e9 100644 --- a/pkg/sqlcache/integration_test.go +++ b/pkg/sqlcache/integration_test.go @@ -284,7 +284,7 @@ func (i *IntegrationSuite) TestSQLCacheFilters() { partitions := []partition.Partition{defaultPartition} ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) defer cancel() - cfgMaps, total, continueToken, err := cache.ListByOptions(ctx, options, partitions, testNamespace) + cfgMaps, total, continueToken, err := cache.ListByOptions(ctx, &options, partitions, testNamespace) i.Require().NoError(err) // since there's no additional pages, the continue token should be empty i.Require().Equal("", continueToken) @@ -338,7 +338,7 @@ func (i *IntegrationSuite) waitForCacheReady(readyResourceNames []string, namesp partitions := []partition.Partition{defaultPartition} cacheCtx, cacheCancel := context.WithTimeout(ctx, time.Second*5) defer cacheCancel() - currentResources, total, _, err := cache.ListByOptions(cacheCtx, options, partitions, namespace) + currentResources, total, _, err := cache.ListByOptions(cacheCtx, &options, partitions, namespace) if err != nil { // note that we don't return the error since that would stop the polling return false, nil diff --git a/pkg/stores/sqlpartition/listprocessor/processor.go b/pkg/stores/sqlpartition/listprocessor/processor.go index f847e7d8..f470b615 100644 --- a/pkg/stores/sqlpartition/listprocessor/processor.go +++ b/pkg/stores/sqlpartition/listprocessor/processor.go @@ -57,7 +57,7 @@ type Cache interface { // - the total number of resources (returned list might be a subset depending on pagination options in lo) // - a continue token, if there are more pages after the returned one // - an error instead of all of the above if anything went wrong - ListByOptions(ctx context.Context, lo sqltypes.ListOptions, partitions []partition.Partition, namespace string) (*unstructured.UnstructuredList, int, string, error) + ListByOptions(ctx context.Context, lo *sqltypes.ListOptions, partitions []partition.Partition, namespace string) (*unstructured.UnstructuredList, int, string, error) } func k8sOpToRancherOp(k8sOp selection.Operator) (sqltypes.Op, bool, error) { @@ -202,7 +202,7 @@ func splitQuery(query string) []string { func parseNamespaceOrProjectFilters(ctx context.Context, projOrNS string, op sqltypes.Op, namespaceInformer Cache) ([]sqltypes.Filter, error) { var filters []sqltypes.Filter for _, pn := range strings.Split(projOrNS, ",") { - uList, _, _, err := namespaceInformer.ListByOptions(ctx, sqltypes.ListOptions{ + uList, _, _, err := namespaceInformer.ListByOptions(ctx, &sqltypes.ListOptions{ Filters: []sqltypes.OrFilter{ { Filters: []sqltypes.Filter{ diff --git a/pkg/stores/sqlpartition/listprocessor/processor_test.go b/pkg/stores/sqlpartition/listprocessor/processor_test.go index 8048dd1e..bc912cc1 100644 --- a/pkg/stores/sqlpartition/listprocessor/processor_test.go +++ b/pkg/stores/sqlpartition/listprocessor/processor_test.go @@ -82,7 +82,7 @@ func TestParseQuery(t *testing.T) { }, } nsc := NewMockCache(gomock.NewController(t)) - nsc.EXPECT().ListByOptions(context.Background(), sqltypes.ListOptions{ + nsc.EXPECT().ListByOptions(context.Background(), &sqltypes.ListOptions{ Filters: []sqltypes.OrFilter{ { Filters: []sqltypes.Filter{ @@ -132,7 +132,7 @@ func TestParseQuery(t *testing.T) { errExpected: true, setupNSCache: func() Cache { nsi := NewMockCache(gomock.NewController(t)) - nsi.EXPECT().ListByOptions(context.Background(), sqltypes.ListOptions{ + nsi.EXPECT().ListByOptions(context.Background(), &sqltypes.ListOptions{ Filters: []sqltypes.OrFilter{ { Filters: []sqltypes.Filter{ @@ -185,7 +185,7 @@ func TestParseQuery(t *testing.T) { Items: []unstructured.Unstructured{}, } nsi := NewMockCache(gomock.NewController(t)) - nsi.EXPECT().ListByOptions(context.Background(), sqltypes.ListOptions{ + nsi.EXPECT().ListByOptions(context.Background(), &sqltypes.ListOptions{ Filters: []sqltypes.OrFilter{ { Filters: []sqltypes.Filter{ diff --git a/pkg/stores/sqlpartition/listprocessor/proxy_mocks_test.go b/pkg/stores/sqlpartition/listprocessor/proxy_mocks_test.go index 04a5cfb5..22c057be 100644 --- a/pkg/stores/sqlpartition/listprocessor/proxy_mocks_test.go +++ b/pkg/stores/sqlpartition/listprocessor/proxy_mocks_test.go @@ -43,7 +43,7 @@ func (m *MockCache) EXPECT() *MockCacheMockRecorder { } // ListByOptions mocks base method. -func (m *MockCache) ListByOptions(arg0 context.Context, arg1 sqltypes.ListOptions, arg2 []partition.Partition, arg3 string) (*unstructured.UnstructuredList, int, string, error) { +func (m *MockCache) ListByOptions(arg0 context.Context, arg1 *sqltypes.ListOptions, arg2 []partition.Partition, arg3 string) (*unstructured.UnstructuredList, int, string, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ListByOptions", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(*unstructured.UnstructuredList) diff --git a/pkg/stores/sqlproxy/proxy_mocks_test.go b/pkg/stores/sqlproxy/proxy_mocks_test.go index 8061bc84..e03065c9 100644 --- a/pkg/stores/sqlproxy/proxy_mocks_test.go +++ b/pkg/stores/sqlproxy/proxy_mocks_test.go @@ -51,7 +51,7 @@ func (m *MockCache) EXPECT() *MockCacheMockRecorder { } // ListByOptions mocks base method. -func (m *MockCache) ListByOptions(arg0 context.Context, arg1 sqltypes.ListOptions, arg2 []partition.Partition, arg3 string) (*unstructured.UnstructuredList, int, string, error) { +func (m *MockCache) ListByOptions(arg0 context.Context, arg1 *sqltypes.ListOptions, arg2 []partition.Partition, arg3 string) (*unstructured.UnstructuredList, int, string, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ListByOptions", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(*unstructured.UnstructuredList) diff --git a/pkg/stores/sqlproxy/proxy_store.go b/pkg/stores/sqlproxy/proxy_store.go index 3ffe751c..528996e8 100644 --- a/pkg/stores/sqlproxy/proxy_store.go +++ b/pkg/stores/sqlproxy/proxy_store.go @@ -217,7 +217,7 @@ type Cache interface { // - the total number of resources (returned list might be a subset depending on pagination options in lo) // - a continue token, if there are more pages after the returned one // - an error instead of all of the above if anything went wrong - ListByOptions(ctx context.Context, lo sqltypes.ListOptions, partitions []partition.Partition, namespace string) (*unstructured.UnstructuredList, int, string, error) + ListByOptions(ctx context.Context, lo *sqltypes.ListOptions, partitions []partition.Partition, namespace string) (*unstructured.UnstructuredList, int, string, error) } // WarningBuffer holds warnings that may be returned from the kubernetes api @@ -784,7 +784,7 @@ func (s *Store) ListByPartitions(apiOp *types.APIRequest, schema *types.APISchem return nil, 0, "", err } - list, total, continueToken, err := inf.ListByOptions(apiOp.Context(), opts, partitions, apiOp.Namespace) + list, total, continueToken, err := inf.ListByOptions(apiOp.Context(), &opts, partitions, apiOp.Namespace) if err != nil { if errors.Is(err, informer.ErrInvalidColumn) { return nil, 0, "", apierror.NewAPIError(validation.InvalidBodyContent, err.Error()) diff --git a/pkg/stores/sqlproxy/proxy_store_test.go b/pkg/stores/sqlproxy/proxy_store_test.go index a6f28d59..85cdb4ea 100644 --- a/pkg/stores/sqlproxy/proxy_store_test.go +++ b/pkg/stores/sqlproxy/proxy_store_test.go @@ -244,7 +244,7 @@ func TestListByPartitions(t *testing.T) { // This tests that fields are being extracted from schema columns and the type specific fields map cf.EXPECT().CacheFor(context.Background(), [][]string{{"some", "field"}, {`id`}, {`metadata`, `state`, `name`}, {"gvk", "specific", "fields"}}, gomock.Any(), &tablelistconvert.Client{ResourceInterface: ri}, attributes.GVK(schema), attributes.Namespaced(schema), true).Return(c, nil) tb.EXPECT().GetTransformFunc(attributes.GVK(schema)).Return(func(obj interface{}) (interface{}, error) { return obj, nil }) - bloi.EXPECT().ListByOptions(req.Context(), opts, partitions, req.Namespace).Return(listToReturn, len(listToReturn.Items), "", nil) + bloi.EXPECT().ListByOptions(req.Context(), &opts, partitions, req.Namespace).Return(listToReturn, len(listToReturn.Items), "", nil) list, total, contToken, err := s.ListByPartitions(req, schema, partitions) assert.Nil(t, err) assert.Equal(t, expectedItems, list) @@ -461,7 +461,7 @@ func TestListByPartitions(t *testing.T) { cf.EXPECT().CacheFor(context.Background(), [][]string{{"some", "field"}, {`id`}, {`metadata`, `state`, `name`}, {"gvk", "specific", "fields"}}, gomock.Any(), &tablelistconvert.Client{ResourceInterface: ri}, attributes.GVK(schema), attributes.Namespaced(schema), false).Return(c, nil) tb.EXPECT().GetTransformFunc(attributes.GVK(schema)).Return(func(obj interface{}) (interface{}, error) { return obj, nil }) - bloi.EXPECT().ListByOptions(req.Context(), opts, partitions, req.Namespace).Return(listToReturn, len(listToReturn.Items), "", nil) + bloi.EXPECT().ListByOptions(req.Context(), &opts, partitions, req.Namespace).Return(listToReturn, len(listToReturn.Items), "", nil) list, total, contToken, err := s.ListByPartitions(req, schema, partitions) assert.Nil(t, err) assert.Equal(t, expectedItems, list) @@ -610,7 +610,7 @@ func TestListByPartitions(t *testing.T) { cg.EXPECT().TableAdminClient(req, schema, "", &WarningBuffer{}).Return(ri, nil) // This tests that fields are being extracted from schema columns and the type specific fields map cf.EXPECT().CacheFor(context.Background(), [][]string{{"some", "field"}, {`id`}, {`metadata`, `state`, `name`}, {"gvk", "specific", "fields"}}, gomock.Any(), &tablelistconvert.Client{ResourceInterface: ri}, attributes.GVK(schema), attributes.Namespaced(schema), true).Return(c, nil) - bloi.EXPECT().ListByOptions(req.Context(), opts, partitions, req.Namespace).Return(nil, 0, "", fmt.Errorf("error")) + bloi.EXPECT().ListByOptions(req.Context(), &opts, partitions, req.Namespace).Return(nil, 0, "", fmt.Errorf("error")) tb.EXPECT().GetTransformFunc(attributes.GVK(schema)).Return(func(obj interface{}) (interface{}, error) { return obj, nil }) _, _, _, err = s.ListByPartitions(req, schema, partitions) diff --git a/pkg/stores/sqlproxy/sql_informer_mocks_test.go b/pkg/stores/sqlproxy/sql_informer_mocks_test.go index 2a51e2b3..ca237f8c 100644 --- a/pkg/stores/sqlproxy/sql_informer_mocks_test.go +++ b/pkg/stores/sqlproxy/sql_informer_mocks_test.go @@ -43,7 +43,7 @@ func (m *MockByOptionsLister) EXPECT() *MockByOptionsListerMockRecorder { } // ListByOptions mocks base method. -func (m *MockByOptionsLister) ListByOptions(arg0 context.Context, arg1 sqltypes.ListOptions, arg2 []partition.Partition, arg3 string) (*unstructured.UnstructuredList, int, string, error) { +func (m *MockByOptionsLister) ListByOptions(arg0 context.Context, arg1 *sqltypes.ListOptions, arg2 []partition.Partition, arg3 string) (*unstructured.UnstructuredList, int, string, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ListByOptions", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(*unstructured.UnstructuredList)