MGMT-16307: Optimize GraphQL search using filters #41
Conversation
|
@danielerez: This pull request references MGMT-16307 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
danielerez
force-pushed
the
graphql_filters
branch
from
123c67c
to
d9c95f7
Compare
internal/search/graphql_test.go
Outdated
| []*model.SearchFilter{ | ||
| { | ||
| Property: "cluster", | ||
| Values: []*string{swag.String("=spoke0")}, |
There was a problem hiding this comment.
In other tests we are using the k8s.io/utils/ptr. It is not very different, but has the advantage of generics, so you can use the same ptr.To function for all types:
Values: []*string{ptr.To("=spoke0")},There was a problem hiding this comment.
oh, nice:) changed.
| @@ -31,6 +31,9 @@ type ListRequest struct { | |||
|
|
|||
| // ParentID is the identifier of the parent collection. | |||
| ParentID string | |||
|
|
|||
| // Filter is the expression for filtering the results. | |||
| Filters []string | |||
There was a problem hiding this comment.
The request already contains a Selector field with the result of parsing the filters parameter. Can you use that?
There was a problem hiding this comment.
Good idea, looks much cleaner.
internal/service/resource_handler.go
Outdated
| @@ -52,6 +53,7 @@ type ResourceHandler struct { | |||
| backendClient *http.Client | |||
| jsonAPI jsoniter.API | |||
| selectorEvaluator *search.SelectorEvaluator | |||
| selectorParser *search.SelectorParser | |||
There was a problem hiding this comment.
Not completely sure, but I think that if you use the Selector field of the request you will not need this additional parser because the selector is already parser.
There was a problem hiding this comment.
Right, removed.
internal/search/graphql.go
Outdated
| License. | ||
| */ | ||
|
|
||
| package search |
There was a problem hiding this comment.
I think it would be good to keep in the "search" package only the code related to search mechanisms defined in the specification, independent of the implementation of the backend. Can we move this code to a separate package? For example internal/graphql?
There was a problem hiding this comment.
Good point. Moved.
Optimized the filtering of search results in the resource server. I.e. instead of filtering the items after fetching a response, used the filter functionality of GraphQL search query API. This change includes the required mapping from 'filter' query param (as defined in the NFV spec) into SearchFilter as used in GraphQL. For any unsupported search operator by GraphQL, we fallback to the regular selector filtering.
danielerez
force-pushed
the
graphql_filters
branch
from
d9c95f7
to
ddc4c8a
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
Optimized the filtering of search results in the resource server. I.e. instead of filtering the items after fetching a response, used the filter functionality of GraphQL search query API. This change includes the required mapping from 'filter' query param (as defined in the NFV spec) into SearchFilter as used in GraphQL.
Note: for any unsupported search operator by GraphQL, we fallback to the regular selector filtering.