sql: bugfix: return total resource count correctly (#236)

* sql: drop dead code Signed-off-by: Silvio Moioli <[email protected]> * sql: bugfix: return total resource count correctly Signed-off-by: Silvio Moioli <[email protected]> * adapt tests Signed-off-by: Silvio Moioli <[email protected]> * adapt mocks Signed-off-by: Silvio Moioli <[email protected]> * TEMP: remove this when bumping lasso to include rancher/lasso#84 Signed-off-by: Silvio Moioli <[email protected]> * Use latest lasso instead of fork --------- Signed-off-by: Silvio Moioli <[email protected]> Co-authored-by: Tom Lebreux <[email protected]>
rancher · Jul 5, 2024 · 0841e03 · 0841e03
1 parent 88fd70a
commit 0841e03
Show file tree

Hide file tree

Showing 13 changed files with 71 additions and 53 deletions.
diff --git a/go.mod b/go.mod
@@ -22,10 +22,10 @@ require (
 	github.com/rancher/apiserver v0.0.0-20240604183424-8c448886365e
 	github.com/rancher/dynamiclistener v0.6.0-rc1
 	github.com/rancher/kubernetes-provider-detector v0.1.5
-	github.com/rancher/lasso v0.0.0-20240603075835-701e919d08b7
+	github.com/rancher/lasso v0.0.0-20240705194423-b2a060d103c1
 	github.com/rancher/norman v0.0.0-20240604183301-20cd23aadce1
 	github.com/rancher/remotedialer v0.3.2
-	github.com/rancher/wrangler/v3 v3.0.0-rc2
+	github.com/rancher/wrangler/v3 v3.0.0-rc3
 	github.com/sirupsen/logrus v1.9.3
 	github.com/stretchr/testify v1.9.0
 	github.com/urfave/cli v1.22.14

diff --git a/go.sum b/go.sum
@@ -192,14 +192,14 @@ github.com/rancher/dynamiclistener v0.6.0-rc1 h1:Emwf9o7PMLdQNv4lvFx7xJKxDuDa4Y6
 github.com/rancher/dynamiclistener v0.6.0-rc1/go.mod h1:BIPgJ8xFSUyuTyGvRMVt++S1qjD3+7Ptvq1TXl6hcTM=
 github.com/rancher/kubernetes-provider-detector v0.1.5 h1:hWRAsWuJOemzGjz/XrbTlM7QmfO4OedvFE3QwXiH60I=
 github.com/rancher/kubernetes-provider-detector v0.1.5/go.mod h1:ypuJS7kP7rUiAn330xG46mj+Nhvym05GM8NqMVekpH0=
-github.com/rancher/lasso v0.0.0-20240603075835-701e919d08b7 h1:E5AeOkylBXf4APhnHgDvePdtpxOfIjhKnxfjm4sDIEk=
-github.com/rancher/lasso v0.0.0-20240603075835-701e919d08b7/go.mod h1:v0FJLrmL4m6zdWfIB0/qo7qN5QIjVMFyvFGaw8uyWsA=
+github.com/rancher/lasso v0.0.0-20240705194423-b2a060d103c1 h1:vv1jDlYbd4KhGbPNxmjs8CYgEHUrQm2bMtmULfXJ6iw=
+github.com/rancher/lasso v0.0.0-20240705194423-b2a060d103c1/go.mod h1:A/y3BLQkxZXYD60MNDRwAG9WGxXfvd6Z6gWR/a8wPw8=
 github.com/rancher/norman v0.0.0-20240604183301-20cd23aadce1 h1:7g0yOiUGfT4zK4N9H0PSijnS/e2YrObi4Gj19JgE1L8=
 github.com/rancher/norman v0.0.0-20240604183301-20cd23aadce1/go.mod h1:sGnN5ayvAHLfInOFZ4N1fzZw1IMy3i+9PZA7IxlPsRg=
 github.com/rancher/remotedialer v0.3.2 h1:kstZbRwPS5gPWpGg8VjEHT2poHtArs+Fc317YM8JCzU=
 github.com/rancher/remotedialer v0.3.2/go.mod h1:Ys004RpJuTLSm+k4aYUCoFiOOad37ubYev3TkOFg/5w=
-github.com/rancher/wrangler/v3 v3.0.0-rc2 h1:XGSPPp6GXELqlLvwJp5MsdqyCPu6SCA4UKJ7rQJzE40=
-github.com/rancher/wrangler/v3 v3.0.0-rc2/go.mod h1:f54hh7gFkwwbjsieT2b63FowzTU8FvrBonPe//0CIXo=
+github.com/rancher/wrangler/v3 v3.0.0-rc3 h1:OqAmpNRZC+aIz5NXBUTqETpC2uKOrNa4xpEY3uo5uk0=
+github.com/rancher/wrangler/v3 v3.0.0-rc3/go.mod h1:Dfckuuq7MJk2JWVBDywRlZXMxEyPxHy4XqGrPEzu5Eg=
 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/sqlpartition/listprocessor/processor.go b/pkg/stores/sqlpartition/listprocessor/processor.go
@@ -44,8 +44,13 @@ type ListOptions struct {
 }
 
 type Cache interface {
-	// ListByOptions returns objects according to the specified list options and partitions
-	ListByOptions(ctx context.Context, lo informer.ListOptions, partitions []partition.Partition, namespace string) (*unstructured.UnstructuredList, string, error)
+	// ListByOptions returns objects according to the specified list options and partitions.
+	// Specifically:
+	//   - an unstructured list of resources belonging to any of the specified partitions
+	//   - 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 informer.ListOptions, partitions []partition.Partition, namespace string) (*unstructured.UnstructuredList, int, string, error)
 }
 
 // ParseQuery parses the query params of a request and returns a ListOptions.
@@ -160,7 +165,7 @@ func getLimit(apiOp *types.APIRequest) int {
 func parseNamespaceOrProjectFilters(ctx context.Context, projOrNS string, op informer.Op, namespaceInformer Cache) ([]informer.Filter, error) {
 	var filters []informer.Filter
 	for _, pn := range strings.Split(projOrNS, ",") {
-		uList, _, err := namespaceInformer.ListByOptions(ctx, informer.ListOptions{
+		uList, _, _, err := namespaceInformer.ListByOptions(ctx, informer.ListOptions{
 			Filters: []informer.OrFilter{
 				{
 					Filters: []informer.Filter{

diff --git a/pkg/stores/sqlpartition/listprocessor/processor_test.go b/pkg/stores/sqlpartition/listprocessor/processor_test.go
@@ -101,7 +101,7 @@ func TestParseQuery(t *testing.T) {
 						},
 					},
 				},
-			}, []partition.Partition{{Passthrough: true}}, "").Return(list, "", nil)
+			}, []partition.Partition{{Passthrough: true}}, "").Return(list, len(list.Items), "", nil)
 			return nsc
 		},
 	})
@@ -151,7 +151,7 @@ func TestParseQuery(t *testing.T) {
 						},
 					},
 				},
-			}, []partition.Partition{{Passthrough: true}}, "").Return(nil, "", fmt.Errorf("error"))
+			}, []partition.Partition{{Passthrough: true}}, "").Return(nil, 0, "", fmt.Errorf("error"))
 			return nsi
 		},
 	})
@@ -204,7 +204,7 @@ func TestParseQuery(t *testing.T) {
 						},
 					},
 				},
-			}, []partition.Partition{{Passthrough: true}}, "").Return(list, "", nil)
+			}, []partition.Partition{{Passthrough: true}}, "").Return(list, len(list.Items), "", nil)
 			return nsi
 		},
 	})

diff --git a/pkg/stores/sqlpartition/listprocessor/proxy_mocks_test.go b/pkg/stores/sqlpartition/listprocessor/proxy_mocks_test.go
diff --git a/pkg/stores/sqlpartition/partition_mocks_test.go b/pkg/stores/sqlpartition/partition_mocks_test.go
diff --git a/pkg/stores/sqlpartition/partitioner.go b/pkg/stores/sqlpartition/partitioner.go
@@ -28,7 +28,7 @@ type UnstructuredStore interface {
 	Update(apiOp *types.APIRequest, schema *types.APISchema, data types.APIObject, id string) (*unstructured.Unstructured, []types.Warning, error)
 	Delete(apiOp *types.APIRequest, schema *types.APISchema, id string) (*unstructured.Unstructured, []types.Warning, error)
 
-	ListByPartitions(apiOp *types.APIRequest, schema *types.APISchema, partitions []partition.Partition) ([]unstructured.Unstructured, string, error)
+	ListByPartitions(apiOp *types.APIRequest, schema *types.APISchema, partitions []partition.Partition) ([]unstructured.Unstructured, int, string, error)
 	WatchByPartitions(apiOp *types.APIRequest, schema *types.APISchema, wr types.WatchRequest, partitions []partition.Partition) (chan watch.Event, error)
 }
 

diff --git a/pkg/stores/sqlpartition/store.go b/pkg/stores/sqlpartition/store.go
@@ -76,12 +76,12 @@ func (s *Store) List(apiOp *types.APIRequest, schema *types.APISchema) (types.AP
 
 	store := s.Partitioner.Store()
 
-	list, continueToken, err := store.ListByPartitions(apiOp, schema, partitions)
+	list, total, continueToken, err := store.ListByPartitions(apiOp, schema, partitions)
 	if err != nil {
 		return result, err
 	}
 
-	result.Count = len(list)
+	result.Count = total
 
 	for _, item := range list {
 		item := item.DeepCopy()

diff --git a/pkg/stores/sqlpartition/store_test.go b/pkg/stores/sqlpartition/store_test.go
@@ -88,7 +88,7 @@ func TestList(t *testing.T) {
 			}
 			p.EXPECT().All(req, schema, "list", "").Return(partitions, nil)
 			p.EXPECT().Store().Return(us)
-			us.EXPECT().ListByPartitions(req, schema, partitions).Return(uListToReturn, "", nil)
+			us.EXPECT().ListByPartitions(req, schema, partitions).Return(uListToReturn, len(uListToReturn), "", nil)
 			l, err := s.List(req, schema)
 			assert.Nil(t, err)
 			assert.Equal(t, expectedAPIObjList, l)
@@ -125,7 +125,7 @@ func TestList(t *testing.T) {
 			partitions := make([]partition.Partition, 0)
 			p.EXPECT().All(req, schema, "list", "").Return(partitions, nil)
 			p.EXPECT().Store().Return(us)
-			us.EXPECT().ListByPartitions(req, schema, partitions).Return(nil, "", fmt.Errorf("error"))
+			us.EXPECT().ListByPartitions(req, schema, partitions).Return(nil, 0, "", fmt.Errorf("error"))
 			_, err := s.List(req, schema)
 			assert.NotNil(t, err)
 		},

diff --git a/pkg/stores/sqlproxy/proxy_mocks_test.go b/pkg/stores/sqlproxy/proxy_mocks_test.go
diff --git a/pkg/stores/sqlproxy/proxy_store.go b/pkg/stores/sqlproxy/proxy_store.go
@@ -92,9 +92,13 @@ type SchemaColumnSetter interface {
 }
 
 type Cache interface {
-	// ListByOptions returns objects according to the specified list options and partitions
-	// see ListOptionIndexer.ListByOptions
-	ListByOptions(ctx context.Context, lo informer.ListOptions, partitions []partition.Partition, namespace string) (*unstructured.UnstructuredList, string, error)
+	// ListByOptions returns objects according to the specified list options and partitions.
+	// Specifically:
+	//   - an unstructured list of resources belonging to any of the specified partitions
+	//   - 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 informer.ListOptions, partitions []partition.Partition, namespace string) (*unstructured.UnstructuredList, int, string, error)
 }
 
 // WarningBuffer holds warnings that may be returned from the kubernetes api
@@ -603,35 +607,39 @@ func (s *Store) Delete(apiOp *types.APIRequest, schema *types.APISchema, id stri
 	return obj, buffer, nil
 }
 
-// ListByPartitions returns an unstructured list of resources belonging to any of the specified partitions
-func (s *Store) ListByPartitions(apiOp *types.APIRequest, schema *types.APISchema, partitions []partition.Partition) ([]unstructured.Unstructured, string, error) {
+// ListByPartitions returns:
+//   - an unstructured list of resources belonging to any of the specified partitions
+//   - the total number of resources (returned list might be a subset depending on pagination options in apiOp)
+//   - 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 (s *Store) ListByPartitions(apiOp *types.APIRequest, schema *types.APISchema, partitions []partition.Partition) ([]unstructured.Unstructured, int, string, error) {
 	opts, err := listprocessor.ParseQuery(apiOp, s.namespaceCache)
 	if err != nil {
-		return nil, "", err
+		return nil, 0, "", err
 	}
 	// warnings from inside the informer are discarded
 	buffer := WarningBuffer{}
 	client, err := s.clientGetter.TableAdminClient(apiOp, schema, "", &buffer)
 	if err != nil {
-		return nil, "", err
+		return nil, 0, "", err
 	}
 	fields := getFieldsFromSchema(schema)
 	fields = append(fields, getFieldForGVK(attributes.GVK(schema))...)
 
 	inf, err := s.cacheFactory.CacheFor(fields, &tablelistconvert.Client{ResourceInterface: client}, attributes.GVK(schema), attributes.Namespaced(schema))
 	if err != nil {
-		return nil, "", err
+		return nil, 0, "", err
 	}
 
-	list, 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.InvalidColumnErr) {
-			return nil, "", apierror.NewAPIError(validation.InvalidBodyContent, err.Error())
+			return nil, 0, "", apierror.NewAPIError(validation.InvalidBodyContent, err.Error())
 		}
-		return nil, "", err
+		return nil, 0, "", err
 	}
 
-	return list.Items, continueToken, nil
+	return list.Items, total, continueToken, nil
 }
 
 // WatchByPartitions returns a channel of events for a list or resource belonging to any of the specified partitions

diff --git a/pkg/stores/sqlproxy/proxy_store_test.go b/pkg/stores/sqlproxy/proxy_store_test.go
@@ -231,10 +231,11 @@ 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([][]string{{"some", "field"}, {"gvk", "specific", "fields"}}, &tablelistconvert.Client{ResourceInterface: ri}, attributes.GVK(schema), attributes.Namespaced(schema)).Return(c, nil)
-			bloi.EXPECT().ListByOptions(req.Context(), opts, partitions, req.Namespace).Return(listToReturn, "", nil)
-			list, contToken, err := s.ListByPartitions(req, schema, partitions)
+			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)
+			assert.Equal(t, len(expectedItems), total)
 			assert.Equal(t, "", contToken)
 		},
 	})
@@ -293,11 +294,11 @@ func TestListByPartitions(t *testing.T) {
 			// items is equal to the list returned by ListByParititons doesn't ensure no mutation happened
 			copy(listToReturn.Items, expectedItems)
 
-			nsi.EXPECT().ListByOptions(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, "", fmt.Errorf("error")).Times(2)
+			nsi.EXPECT().ListByOptions(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, 0, "", fmt.Errorf("error")).Times(2)
 			_, err := listprocessor.ParseQuery(req, nsi)
 			assert.NotNil(t, err)
 
-			_, _, err = s.ListByPartitions(req, schema, partitions)
+			_, _, _, err = s.ListByPartitions(req, schema, partitions)
 			assert.NotNil(t, err)
 		},
 	})
@@ -360,7 +361,7 @@ func TestListByPartitions(t *testing.T) {
 			assert.Nil(t, err)
 			cg.EXPECT().TableAdminClient(req, schema, "", &WarningBuffer{}).Return(nil, fmt.Errorf("error"))
 
-			_, _, err = s.ListByPartitions(req, schema, partitions)
+			_, _, _, err = s.ListByPartitions(req, schema, partitions)
 			assert.NotNil(t, err)
 		},
 	})
@@ -425,7 +426,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([][]string{{"some", "field"}, {"gvk", "specific", "fields"}}, &tablelistconvert.Client{ResourceInterface: ri}, attributes.GVK(schema), attributes.Namespaced(schema)).Return(factory.Cache{}, fmt.Errorf("error"))
 
-			_, _, err = s.ListByPartitions(req, schema, partitions)
+			_, _, _, err = s.ListByPartitions(req, schema, partitions)
 			assert.NotNil(t, err)
 		},
 	})
@@ -496,9 +497,9 @@ 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([][]string{{"some", "field"}, {"gvk", "specific", "fields"}}, &tablelistconvert.Client{ResourceInterface: ri}, attributes.GVK(schema), attributes.Namespaced(schema)).Return(c, nil)
-			bloi.EXPECT().ListByOptions(req.Context(), opts, partitions, req.Namespace).Return(nil, "", fmt.Errorf("error"))
+			bloi.EXPECT().ListByOptions(req.Context(), opts, partitions, req.Namespace).Return(nil, 0, "", fmt.Errorf("error"))
 
-			_, _, err = s.ListByPartitions(req, schema, partitions)
+			_, _, _, err = s.ListByPartitions(req, schema, partitions)
 			assert.NotNil(t, err)
 		},
 	})

diff --git a/pkg/stores/sqlproxy/sql_informer_mocks_test.go b/pkg/stores/sqlproxy/sql_informer_mocks_test.go