From 58afe895465e89a40c4d63638b1797e2b4352b1b Mon Sep 17 00:00:00 2001 From: Ajay Gopinathan Date: Fri, 12 Apr 2019 11:22:43 -0700 Subject: [PATCH 1/2] Add filter to next page token so it applies to subsequently requested pages. --- backend/src/apiserver/list/list.go | 1 + backend/src/apiserver/list/list_test.go | 30 ++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/backend/src/apiserver/list/list.go b/backend/src/apiserver/list/list.go index d92eac6feab..5fa253de0e8 100644 --- a/backend/src/apiserver/list/list.go +++ b/backend/src/apiserver/list/list.go @@ -290,6 +290,7 @@ func (o *Options) nextPageToken(listable Listable) (*token, error) { KeyFieldName: listable.PrimaryKeyColumnName(), KeyFieldValue: keyField.Interface(), IsDesc: o.IsDesc, + Filter: o.Filter, }, nil } diff --git a/backend/src/apiserver/list/list_test.go b/backend/src/apiserver/list/list_test.go index c25a1a12adb..92f9be1341a 100644 --- a/backend/src/apiserver/list/list_test.go +++ b/backend/src/apiserver/list/list_test.go @@ -42,6 +42,17 @@ func (f *fakeListable) APIToModelFieldMap() map[string]string { func TestNextPageToken_ValidTokens(t *testing.T) { l := &fakeListable{PrimaryKey: "uuid123", FakeName: "Fake", CreatedTimestamp: 1234} + protoFilter := &api.Filter{Predicates: []*api.Predicate{ + &api.Predicate{ + Key: "name", + Op: api.Predicate_EQUALS, + Value: &api.Predicate_StringValue{StringValue: "SomeName"}, + }}} + testFilter, err := filter.New(protoFilter) + if err != nil { + t.Fatalf("failed to parse filter proto %+v: %v", protoFilter, err) + } + tests := []struct { inOpts *Options want *token @@ -82,12 +93,29 @@ func TestNextPageToken_ValidTokens(t *testing.T) { IsDesc: false, }, }, + { + inOpts: &Options{ + PageSize: 10, + token: &token{ + SortByFieldName: "FakeName", IsDesc: false, + Filter: testFilter, + }, + }, + want: &token{ + SortByFieldName: "FakeName", + SortByFieldValue: "Fake", + KeyFieldName: "PrimaryKey", + KeyFieldValue: "uuid123", + IsDesc: false, + Filter: testFilter, + }, + }, } for _, test := range tests { got, err := test.inOpts.nextPageToken(l) - if !cmp.Equal(got, test.want) || err != nil { + if !cmp.Equal(got, test.want, cmp.AllowUnexported(filter.Filter{})) || err != nil { t.Errorf("nextPageToken(%+v, %+v) =\nGot: %+v, %+v\nWant: %+v, \nDiff:\n%s", test.inOpts, l, got, err, test.want, cmp.Diff(test.want, got)) } From 4e6d7929aa3a70c051f6d036f1acbc7b2839b2b8 Mon Sep 17 00:00:00 2001 From: Ajay Gopinathan Date: Fri, 12 Apr 2019 13:39:32 -0700 Subject: [PATCH 2/2] Fix a bug where filter wasn't being serialized properly --- backend/src/apiserver/filter/BUILD.bazel | 1 + backend/src/apiserver/filter/filter.go | 65 +++++++++++++++++++ backend/src/apiserver/filter/filter_test.go | 44 +++++++++++++ backend/src/apiserver/list/list.go | 1 + backend/src/apiserver/list/list_test.go | 34 +++++++++- .../storage/experiment_store_test.go | 2 +- 6 files changed, 144 insertions(+), 3 deletions(-) diff --git a/backend/src/apiserver/filter/BUILD.bazel b/backend/src/apiserver/filter/BUILD.bazel index 45b437760c1..8b31454d028 100644 --- a/backend/src/apiserver/filter/BUILD.bazel +++ b/backend/src/apiserver/filter/BUILD.bazel @@ -8,6 +8,7 @@ go_library( deps = [ "//backend/api:go_default_library", "//backend/src/common/util:go_default_library", + "@com_github_golang_protobuf//jsonpb:go_default_library_gen", "@com_github_golang_protobuf//ptypes:go_default_library_gen", "@com_github_masterminds_squirrel//:go_default_library", ], diff --git a/backend/src/apiserver/filter/filter.go b/backend/src/apiserver/filter/filter.go index 8fc18a7c764..92075e1158e 100644 --- a/backend/src/apiserver/filter/filter.go +++ b/backend/src/apiserver/filter/filter.go @@ -17,9 +17,11 @@ package filter import ( + "encoding/json" "fmt" "github.com/Masterminds/squirrel" + "github.com/golang/protobuf/jsonpb" "github.com/golang/protobuf/ptypes" "github.com/kubeflow/pipelines/backend/src/common/util" @@ -43,6 +45,69 @@ type Filter struct { substring map[string]interface{} } +// filterForMarshaling is a helper struct for marshaling Filter into JSON. This +// is needed as we don't want to export the fields in Filter. +type filterForMarshaling struct { + FilterProto string + + EQ map[string]interface{} + NEQ map[string]interface{} + GT map[string]interface{} + GTE map[string]interface{} + LT map[string]interface{} + LTE map[string]interface{} + + IN map[string]interface{} + + SUBSTRING map[string]interface{} +} + +// MarshalJSON implements JSON Marshaler for Filter. +func (f *Filter) MarshalJSON() ([]byte, error) { + m := &jsonpb.Marshaler{} + s, err := m.MarshalToString(f.filterProto) + if err != nil { + return nil, err + } + return json.Marshal(&filterForMarshaling{ + FilterProto: s, + EQ: f.eq, + NEQ: f.neq, + GT: f.gt, + GTE: f.gte, + LT: f.lt, + LTE: f.lte, + IN: f.in, + SUBSTRING: f.substring, + }) +} + +// UnmarshalJSON implements JSON Unmarshaler for Filter. +func (f *Filter) UnmarshalJSON(b []byte) error { + ffm := filterForMarshaling{} + err := json.Unmarshal(b, &ffm) + if err != nil { + return err + } + + f.filterProto = &api.Filter{} + err = jsonpb.UnmarshalString(ffm.FilterProto, f.filterProto) + if err != nil { + return err + } + + f.eq = ffm.EQ + f.neq = ffm.NEQ + f.gt = ffm.GT + f.gte = ffm.GTE + f.lt = ffm.LT + f.lte = ffm.LTE + f.in = ffm.IN + f.substring = ffm.SUBSTRING + + return nil +} + // New creates a new Filter from parsing the API filter protocol buffer. func New(filterProto *api.Filter) (*Filter, error) { f := &Filter{ diff --git a/backend/src/apiserver/filter/filter_test.go b/backend/src/apiserver/filter/filter_test.go index ac859f139ed..09b4d1b433b 100644 --- a/backend/src/apiserver/filter/filter_test.go +++ b/backend/src/apiserver/filter/filter_test.go @@ -1,6 +1,7 @@ package filter import ( + "encoding/json" "testing" "github.com/Masterminds/squirrel" @@ -235,3 +236,46 @@ func TestAddToSelect(t *testing.T) { } } } + +func TestMarshalJSON(t *testing.T) { + f := &Filter{ + filterProto: &api.Filter{ + Predicates: []*api.Predicate{ + &api.Predicate{ + Key: "Name", Op: api.Predicate_EQUALS, + Value: &api.Predicate_StringValue{StringValue: "SomeName"}, + }, + }, + }, + eq: map[string]interface{}{"name": "SomeName"}, + } + + want := `{"FilterProto":"{\"predicates\":[{\"op\":\"EQUALS\",\"key\":\"Name\",\"stringValue\":\"SomeName\"}]}","EQ":{"name":"SomeName"},"NEQ":null,"GT":null,"GTE":null,"LT":null,"LTE":null,"IN":null,"SUBSTRING":null}` + + got, err := json.Marshal(f) + if err != nil || string(got) != want { + t.Errorf("json.Marshal(%+v):\n%s, %v\nWant:%s, nil error\n", f, got, err, want) + } +} + +func TestUnmarshalJSON(t *testing.T) { + in := `{"FilterProto":"{\"predicates\":[{\"op\":\"EQUALS\",\"key\":\"Name\",\"stringValue\":\"SomeName\"}]}","EQ":{"name":"SomeName"},"NEQ":null,"GT":null,"GTE":null,"LT":null,"LTE":null,"IN":null,"SUBSTRING":null}` + + want := &Filter{ + filterProto: &api.Filter{ + Predicates: []*api.Predicate{ + &api.Predicate{ + Key: "Name", Op: api.Predicate_EQUALS, + Value: &api.Predicate_StringValue{StringValue: "SomeName"}, + }, + }, + }, + eq: map[string]interface{}{"name": "SomeName"}, + } + + got := &Filter{} + err := json.Unmarshal([]byte(in), got) + if err != nil || !cmp.Equal(got, want, cmp.AllowUnexported(Filter{})) { + t.Errorf("json.Unmarshal(%+v):\nGot: %v, Error: %v\nWant:\n%+v, Error: nil\nDiff:%s\n", in, got, err, want, cmp.Diff(want, got, cmp.AllowUnexported(Filter{}))) + } +} diff --git a/backend/src/apiserver/list/list.go b/backend/src/apiserver/list/list.go index 5fa253de0e8..a33de86792b 100644 --- a/backend/src/apiserver/list/list.go +++ b/backend/src/apiserver/list/list.go @@ -76,6 +76,7 @@ func (t *token) marshal() (string, error) { if err != nil { return "", util.NewInternalServerError(err, "Failed to serialize page token.") } + // return string(b), nil return base64.StdEncoding.EncodeToString(b), nil } diff --git a/backend/src/apiserver/list/list_test.go b/backend/src/apiserver/list/list_test.go index 92f9be1341a..bf8055168a3 100644 --- a/backend/src/apiserver/list/list_test.go +++ b/backend/src/apiserver/list/list_test.go @@ -512,6 +512,17 @@ func TestAddPaginationAndFilterToSelect(t *testing.T) { } func TestTokenSerialization(t *testing.T) { + protoFilter := &api.Filter{Predicates: []*api.Predicate{ + &api.Predicate{ + Key: "name", + Op: api.Predicate_EQUALS, + Value: &api.Predicate_StringValue{StringValue: "SomeName"}, + }}} + testFilter, err := filter.New(protoFilter) + if err != nil { + t.Fatalf("failed to parse filter proto %+v: %v", protoFilter, err) + } + tests := []struct { in *token want *token @@ -546,6 +557,25 @@ func TestTokenSerialization(t *testing.T) { KeyFieldValue: float64(200), IsDesc: true}, }, + // has a filter. + { + in: &token{ + SortByFieldName: "SortField", + SortByFieldValue: 100, + KeyFieldName: "KeyField", + KeyFieldValue: 200, + IsDesc: true, + Filter: testFilter, + }, + want: &token{ + SortByFieldName: "SortField", + SortByFieldValue: float64(100), + KeyFieldName: "KeyField", + KeyFieldValue: float64(200), + IsDesc: true, + Filter: testFilter, + }, + }, } for _, test := range tests { @@ -558,9 +588,9 @@ func TestTokenSerialization(t *testing.T) { got := &token{} got.unmarshal(s) - if !cmp.Equal(got, test.want) { + if !cmp.Equal(got, test.want, cmp.AllowUnexported(filter.Filter{})) { t.Errorf("token.unmarshal(%q) =\nGot: %+v\nWant: %+v\nDiff:\n%s", - s, got, test.want, cmp.Diff(test.want, got)) + s, got, test.want, cmp.Diff(test.want, got, cmp.AllowUnexported(filter.Filter{}))) } } } diff --git a/backend/src/apiserver/storage/experiment_store_test.go b/backend/src/apiserver/storage/experiment_store_test.go index 9a8f381c25f..d23bf51fcab 100644 --- a/backend/src/apiserver/storage/experiment_store_test.go +++ b/backend/src/apiserver/storage/experiment_store_test.go @@ -369,5 +369,5 @@ func TestListExperiments_Filtering(t *testing.T) { // No more pages. assert.Equal(t, "", nextPageToken) assert.Equal(t, expected, experiments) - assert.Equal(t, 4, total_size) + assert.Equal(t, 3, total_size) }