Encode filter parameter as a base64-encoded JSON string in List requests #563

neuromage · 2018-12-18T19:29:32Z

Generate new swagger definitions and use these to generate the frontend
APIs using npm run apis.

This is to support filtering in List requests, as the current
grpc-gateway swagger generator tool does not support repeated fields in
requests used in GET endpoints.

Using POST lets us get around the problem in issue grpc-ecosystem/grpc-gateway#492

This change is

neuromage · 2018-12-18T19:30:06Z

/assign @yebrahim
/assign @rileyjbauer

yebrahim · 2018-12-18T21:33:44Z

The frontend needs to change the API calls to pass the object. I can help take a look at this soon.

yebrahim · 2018-12-18T21:47:08Z

frontend/src/apis/experiment/api.ts

-         * @param {*} [options] Override http request option.
-         * @throws {RequiredError}
-         */
-        createExperiment(body: ApiExperiment, options: any = {}): FetchArgs {


Why are these removed?

Thanks for the heads up. This was indeed messed up, because both create and list used the same endpoints with differing verbs (POST and GET).

I decided to go another route: encode the filter as a simple JSON string and put it in a single parameter. Maybe this was what you were alluding to earlier, and I think this should work better, as it doesn't break any of the existing semantics. See what you think.

yebrahim · 2018-12-19T00:44:15Z

backend/src/apiserver/server/experiment_server.go

@@ -44,7 +46,23 @@ func (s *ExperimentServer) ListExperiment(ctx context.Context, request *api.List
 	if request.PageToken != "" {
 		opts, err = list.NewOptionsFromToken(request.PageToken, int(request.PageSize))
 	} else {
-		opts, err = list.NewOptions(&model.Experiment{}, int(request.PageSize), request.SortBy, request.Filter)
+		var f *api.Filter


Any chance we can reuse this code instead of copying it?

Thanks! I consolidated this logic into a new function, and also added tests for it. PTAL.

yebrahim

/lgtm
just one minor comment.

yebrahim

/lgtm
just a minor comment.

yebrahim · 2018-12-19T18:28:05Z

backend/src/apiserver/server/list_request_util_test.go

+	}
+
+	got, err := parseAPIFilter(in)
+	if !cmp.Equal(got, want) || err != nil {


Why not just assert.Equal(...? It takes care of showing the compared objects in the error message right?

True, but I actually prefer the cmp package, as it prints a more precise diff. It's also maintained and recommended by the Go team.

rileyjbauer · 2018-12-19T19:58:14Z

backend/api/go_http_client/experiment_client/experiment_service/list_experiment_parameters.go

+
+// SetFilter adds the filter to the list experiment params
+func (o *ListExperimentParams) SetFilter(filter *string) {
+	o.Filter = filter


If this function is so simple, and not widely used, why not just inline it?

This is auto-generated by the swagger tool. Everything under api/go_http_client and api/go_client is autogenerated.

Ah, sorry, I didn't realize they were auto-generated

Sorry, yeah, it's definitely not obvious :-)

rileyjbauer · 2018-12-19T20:12:05Z

/lgtm

rileyjbauer · 2018-12-19T20:39:00Z

/test kubeflow-pipeline-e2e-test

IronPan · 2018-12-20T19:02:54Z

backend/src/apiserver/server/experiment_server.go

@@ -44,7 +44,12 @@ func (s *ExperimentServer) ListExperiment(ctx context.Context, request *api.List
 	if request.PageToken != "" {
 		opts, err = list.NewOptionsFromToken(request.PageToken, int(request.PageSize))
 	} else {
-		opts, err = list.NewOptions(&model.Experiment{}, int(request.PageSize), request.SortBy, request.Filter)
+		f, err := parseAPIFilter(request.Filter)


not quite understand the if else statement here. does it means only one of pagetoken and filter can exist?
Are filtered results paginated?

pagetoken encapsulates both sorting and filtering, so if you already have a non-empty page token, we ignore further filtering + sorting criteria.

IronPan · 2018-12-20T19:07:03Z

backend/src/apiserver/server/list_request_util.go

+		return nil, nil
+	}
+
+	errorF := func(err error) (*api.Filter, error) {


is any good reason of having this function instead of returning errors directly?

Avoids duplicate in error handling, since the message is the same. It's a very idiomatic Go thing to do.

IronPan · 2018-12-20T19:10:20Z

backend/src/apiserver/server/list_request_util.go

+	if err := jsonpb.UnmarshalString(string(b), f); err != nil {
+		return errorF(err)
+	}
+	return f, nil


can we add additional validation to make sure the filter format is correct?
e.g. value missing, op missing etc.

The api filter will get parsed into a filter.Filter object that can actually be used to do filtering. That parsing will sanity check to ensure the filter is well formed and return an error otherwise.

neuromage · 2018-12-21T21:44:09Z

Thanks @rileyjbauer for pointing out that the Filter definitions for Swagger went missing with this change. This was caused by the fact that Filter is only implicitly used by the API, so protoc-gen-swagger will not generate Swagger files correctly for this proto. I modified the Makefile to hack in a dummy service during generation to ensure we get the Swagger definitions. And I updated the npm run apis script to generate TS interface definitions for filter as well.

PTAL.

neuromage · 2018-12-21T21:59:35Z

/retest

yebrahim

Oof, that hack!
So the protoc->swagger tool can't generate a standalone message, and only works with service definitions?
What's the harm of leaving that service defined in the proto?

/lgtm

yebrahim · 2018-12-21T21:55:41Z

backend/api/Makefile

@@ -52,6 +52,13 @@ all: dependencies
 		--grpc-gateway_out=logtostderr=true:go_client \
 		*.proto

+  # Filter.proto is implicitly used by clients and server, and transmitted as a


nit: indentation (this file uses tabs for some reason).

nit: indentation (this file uses tabs for some reason).

It's not a nit. Makefile requires tabs for receipt prefixes and will misbehave if they are not there.

Weird, but good to know.

neuromage · 2018-12-21T22:04:38Z

Yeah, it's a pretty ugly hack.

I did have the dummy service in initially, but it was generating a corresponding client/server code in Go, which didn't seem like the right thing to do. Hence, this hack :-(

neuromage · 2018-12-22T16:00:26Z

/retest

rileyjbauer · 2018-12-22T19:34:07Z

/test kubeflow-pipeline-e2e-test

Generate new swagger definitions and use these to generate the frontend APIs using `npm run apis`. This is to support filtering in List requests, as the current grpc-gateway swagger generator tool does not support repeated fields in requests used in GET endpoints.

This lets us keep filter as a simple parameter in the ListXXX requests, and gets around having to use POST for List requests.

yebrahim · 2018-12-24T17:41:43Z

How about setting up "go vet -shadow" for catching these errors. I think shadowing is bad practice anyway.
@neuromage @IronPan

neuromage · 2018-12-24T18:11:55Z

Agreed! We can set this up in Travis as part of the presubmit. If you're ok with it, I can try to do that, and also fix any existing shadow errors.

yebrahim · 2018-12-24T19:29:25Z

I'd prefer it in a separate change, this one had already gotten big enough, but if you feel you want to add it here please feel free to.

yebrahim · 2018-12-25T06:40:33Z

/lgtm

neuromage · 2018-12-25T18:08:58Z

yes, definitely, I meant in a separate PR. This is one is good to go @yebrahim @IronPan

IronPan · 2019-01-02T19:31:17Z

/lgtm
/approve
assuming that you will have a separate PR to validate the query string as we discussed offline

If page token doesn't exist, validate the filter and sort by fields.
If page token exist, either
- filter and sort by fields exist and they are valid and equals to the one encoded in page token
- or they can be opt-out and no validation is needed in this case.

k8s-ci-robot · 2019-01-02T19:31:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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:

~~backend/OWNERS~~ [IronPan]
~~frontend/OWNERS~~ [IronPan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2019-01-02T19:31:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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:

~~backend/OWNERS~~ [IronPan]
~~frontend/OWNERS~~ [IronPan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…w#563)

k8s-ci-robot requested review from rileyjbauer and vicaire December 18, 2018 19:29

k8s-ci-robot added the size/XXL label Dec 18, 2018

k8s-ci-robot assigned rileyjbauer and yebrahim Dec 18, 2018

yebrahim reviewed Dec 18, 2018

View reviewed changes

neuromage changed the title ~~Make all ListXXX operations use POST instead of GET.~~ Encode filter parameter as a base64-encoded JSON string in List requests Dec 18, 2018

yebrahim reviewed Dec 19, 2018

View reviewed changes

k8s-ci-robot added the lgtm label Dec 19, 2018

yebrahim reviewed Dec 19, 2018

View reviewed changes

k8s-ci-robot removed lgtm labels Dec 19, 2018

yebrahim reviewed Dec 19, 2018

View reviewed changes

k8s-ci-robot added lgtm labels Dec 19, 2018

rileyjbauer reviewed Dec 19, 2018

View reviewed changes

IronPan requested changes Dec 20, 2018

View reviewed changes

IronPan reviewed Dec 20, 2018

View reviewed changes

k8s-ci-robot removed the lgtm label Dec 21, 2018

neuromage force-pushed the swagger-definitions branch from b76a827 to 83e2b89 Compare December 21, 2018 21:47

yebrahim approved these changes Dec 21, 2018

View reviewed changes

k8s-ci-robot added lgtm and removed lgtm labels Dec 21, 2018

neuromage added 11 commits December 22, 2018 17:41

Use base64-encoded JSON-stringified version of Filter instead.

730933f

This lets us keep filter as a simple parameter in the ListXXX requests, and gets around having to use POST for List requests.

refactor filter parsing to parseAPIFilter and add tests

296c8ab

Hack to ensure correct Swagger definitions are generated for Filter.

b05cbea

Fix merge conflicts with master after rebase

a641a7b

fix indentation

0e653a9

Fix hack so frontend apis compile.

830b275

print failing experiments

37951a7

try print again.

e9125b6

revert experiment_api_test

1ce0457

Use StdEncoding for base64 encoding

7ceddfa

neuromage force-pushed the swagger-definitions branch from e3bc065 to 7ceddfa Compare December 23, 2018 04:35

Fix nil pointer dereference error caused err variable shadowing

48608a9

k8s-ci-robot added the lgtm label Dec 25, 2018

k8s-ci-robot assigned IronPan Jan 2, 2019

k8s-ci-robot added the approved label Jan 2, 2019

k8s-ci-robot merged commit 8616398 into kubeflow:master Jan 2, 2019

rileyjbauer mentioned this pull request Jan 9, 2019

Support filtering in list APIs #270

Closed

HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024

Fix broken Kubeflow doc links to map with the KF 1.3 website (kubeflo…

8f2d07b

…w#563)

Encode filter parameter as a base64-encoded JSON string in List requests #563

Encode filter parameter as a base64-encoded JSON string in List requests #563

Conversation

neuromage commented Dec 18, 2018 • edited by jlewi Loading

neuromage commented Dec 18, 2018

yebrahim commented Dec 18, 2018

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

yebrahim left a comment

Choose a reason for hiding this comment

yebrahim 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rileyjbauer commented Dec 19, 2018

rileyjbauer commented Dec 19, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neuromage commented Dec 21, 2018

neuromage commented Dec 21, 2018

yebrahim 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

neuromage commented Dec 21, 2018

neuromage commented Dec 22, 2018

rileyjbauer commented Dec 22, 2018

yebrahim commented Dec 24, 2018

neuromage commented Dec 24, 2018

yebrahim commented Dec 24, 2018

yebrahim commented Dec 25, 2018

neuromage commented Dec 25, 2018

IronPan commented Jan 2, 2019

k8s-ci-robot commented Jan 2, 2019

k8s-ci-robot commented Jan 2, 2019

neuromage commented Dec 18, 2018 •

edited by jlewi

Loading