Pagination fixes #127

cmurphy · 2023-09-18T22:09:11Z

Allow for filtering/sorting by reserved fields

Certain fields like "type" in an object are reserved by
rancher/apiserver and so are transmuted to an underscore-prefixed
version of the field name, e.g. "_type", which is what appears in the
HTTP response output. Objects like Secrets, which have a built-in Type
field, are subject to this.

Without this patch, filtering and sorting are done on the object prior
to the field renaming, so filtering by "_type" would result in no
filtering happening even though the output returned to the user does
contain "_type". This change ensures that the field name changes happen
before filtering and sorting is started.

Add support for filtering/sorting by array index

For an object like

"status": {
    "podIPs": [
        "10.0.0.1",
        "10.0.0.2",
        "10.0.0.3",
    ]
}

add the ability to filter or sort by a single element in the array list:

GET /pods?filter=status.podIPs.2=10.0.0.2
GET /pods?sort=status.podIPs.2

rancher/rancher#42767
Depends on rancher/wrangler#321

cmurphy · 2023-09-18T22:09:26Z

go.mod

@@ -6,6 +6,7 @@ replace (
 	github.com/crewjam/saml => github.com/rancher/saml v0.2.0
 	github.com/knative/pkg => github.com/rancher/pkg v0.0.0-20181214184433-b04c0947ad2f
 	github.com/matryer/moq => github.com/rancher/moq v0.0.0-20190404221404-ee5226d43009
+	github.com/rancher/wrangler => github.com/cmurphy/wrangler v0.8.1-0.20230918211505-408ec3feeca5


Fix this before merging

JonCrowther · 2023-09-19T14:33:19Z

pkg/stores/partition/listprocessor/processor.go

@@ -260,17 +260,15 @@ func getLimit(apiOp *types.APIRequest) int {

 // FilterList accepts a channel of unstructured objects and a slice of filters and returns the filtered list.


This is no longer a channel

Suggested change

// FilterList accepts a channel of unstructured objects and a slice of filters and returns the filtered list.

// FilterList accepts a slice of unstructured objects and a slice of filters and returns the filtered list.

JonCrowther · 2023-09-19T14:35:39Z

pkg/stores/partition/listprocessor/processor.go

-				result = append(result, item)
-			}
+	for _, item := range list {
+		if len(filters) == 0 {


Since filters never changes in size, this could be moved out of the for loop

if len(filters) == 0 { return list }

JonCrowther · 2023-09-19T14:42:44Z

pkg/stores/partition/store.go

@@ -124,7 +124,7 @@ func (s *Store) ByID(apiOp *types.APIRequest, schema *types.APISchema, id string
 	if err != nil {
 		return types.APIObject{}, err
 	}
-	return toAPI(schema, obj, warnings), nil
+	return toAPI(schema, moveToUnderscore(obj), warnings), nil


If this looks up a single object, do we need to do moveToUnderscore? I noticed we don't for List and Watch

The single object could contain a reserved key, so it needs to be moved. For List, moveToUnderscore was moved out of toAPI and into preprocessList. for Watch, it's under toAPIEvent.

For an object like ``` "status": { "podIPs": [ "10.0.0.1", "10.0.0.2", "10.0.0.3", ] } ``` add the ability to filter or sort by a single element in the array list: ``` GET /pods?filter=status.podIPs.2=10.0.0.2 GET /pods?sort=status.podIPs.2 ```

Certain fields like "type" in an object are reserved by rancher/apiserver and so are transmuted to an underscore-prefixed version of the field name, e.g. "_type", which is what appears in the HTTP response output. Objects like Secrets, which have a built-in Type field, are subject to this. Without this patch, filtering and sorting are done on the object prior to the field renaming, so filtering by "_type" would result in no filtering happening even though the output returned to the user does contain "_type". This change ensures that the field name changes happen before filtering and sorting is started.

tomleb

LGTM

go.mod

tomleb · 2024-04-10T13:58:23Z

pkg/stores/partition/listprocessor/processor.go

+	if len(filters) == 0 {
+		return list
+	}


Observation: The behavior is slightly different than before because here we're returning the same list (with the same storage behind) whereas before we were returning a new list (different storage).

This doesn't seem to be an issue in this case, and we're doing the same in other places (eg: FilterByProjectsAndNamespaces)

Before I think we had a difference in return vs input type, so that wasn't really possible there. Ordinarily I would fix a small inconsistency like this to avoid difficult-to-locate bugs (like someone changing the original list in a go-routine and not realizing that it affected other values down-the-line), but I think there's a performance impact here to consider - copying the items to another list would have a non-zero performance impact. I would also expect the "no filters" case to be the default for the time being, so I'd like to keep this as-is and avoid a costly copy operation.

WDYT?

I'm okay as is. I think consistency would be better from my POV. The costly copy was already there, so adding it back wouldn't make rancher perform worse than it already is. We could benchmark it and see how much optimizing this improves the performance in the grand scheme of things but I'd rather do that as a separate issue and as a need basis.

Without benchmark, it's usually difficult to know if an optimization improves or not. For example, the compiler could already be optimizing this part of the code. It's hard to tell without benchmarks.

pkg/stores/partition/listprocessor/processor_test.go

Also adds a new test for a nested array/map filter.

JonCrowther

Refreshing my approval. Needs a rebase but otherwise LGTM

cmurphy requested review from JonCrowther and KevinJoiner September 18, 2023 22:09

cmurphy commented Sep 18, 2023

View reviewed changes

JonCrowther reviewed Sep 19, 2023

View reviewed changes

cmurphy force-pushed the pagination-fixes branch from a915db5 to 9376bdd Compare September 19, 2023 19:02

JonCrowther approved these changes Sep 19, 2023

View reviewed changes

cmurphy assigned MbolotSuse Sep 25, 2023

samjustus requested review from rmweir and removed request for KevinJoiner September 29, 2023 14:11

MbolotSuse force-pushed the pagination-fixes branch from 9376bdd to 0fd93e0 Compare April 9, 2024 21:29

cmurphy requested a review from a team as a code owner April 9, 2024 21:29

cmurphy added 2 commits April 9, 2024 16:31

MbolotSuse force-pushed the pagination-fixes branch from 0fd93e0 to 1b0c8fa Compare April 9, 2024 21:32

MbolotSuse requested a review from tomleb April 10, 2024 13:15

tomleb approved these changes Apr 10, 2024

View reviewed changes

MbolotSuse force-pushed the pagination-fixes branch from 1b0c8fa to 6ab1b12 Compare April 10, 2024 18:12

Changing wrangler imports to use new function

6ab1b12

Also adds a new test for a nested array/map filter.

MbolotSuse requested a review from tomleb April 10, 2024 18:12

JonCrowther approved these changes Apr 14, 2024

View reviewed changes

tomleb approved these changes Apr 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pagination fixes #127

Pagination fixes #127

cmurphy commented Sep 18, 2023 •

edited

Loading

cmurphy Sep 18, 2023

JonCrowther Sep 19, 2023

JonCrowther Sep 19, 2023

JonCrowther Sep 19, 2023

cmurphy Sep 19, 2023

tomleb left a comment

tomleb Apr 10, 2024

MbolotSuse Apr 10, 2024

tomleb Apr 10, 2024

tomleb Apr 10, 2024

JonCrowther left a comment

		@@ -260,17 +260,15 @@ func getLimit(apiOp *types.APIRequest) int {

		// FilterList accepts a channel of unstructured objects and a slice of filters and returns the filtered list.

	// FilterList accepts a channel of unstructured objects and a slice of filters and returns the filtered list.
	// FilterList accepts a slice of unstructured objects and a slice of filters and returns the filtered list.

Pagination fixes #127

Are you sure you want to change the base?

Pagination fixes #127

Conversation

cmurphy commented Sep 18, 2023 • edited Loading

Allow for filtering/sorting by reserved fields

Add support for filtering/sorting by array index

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomleb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonCrowther left a comment

Choose a reason for hiding this comment

cmurphy commented Sep 18, 2023 •

edited

Loading