Sanity check filtering/sorting options in list requests. (#625)

If sort by or filtering criteria is specified in conjunction with a next
page token, ensure they match up, otherwise return an error.

Also, change the errors to be InvalidInputErrors instead of standard Go
string errors to be consistent with the rest of apiserver.

WORKSPACE

@@ -873,3 +873,15 @@ go_repository(
     importpath = "go.uber.org/zap",
     tag = "v1.9.1",
 )
+
+go_repository(
+    name = "com_github_google_pprof",
+    commit = "3ea8567a2e57",
+    importpath = "github.com/google/pprof",
+)
+
+go_repository(
+    name = "org_golang_x_arch",
+    commit = "5a4828bb7045",
+    importpath = "golang.org/x/arch",
+)

backend/src/apiserver/filter/BUILD.bazel

@@ -7,6 +7,7 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//backend/api:go_default_library",
+        "//backend/src/common/util:go_default_library",
         "@com_github_golang_protobuf//ptypes:go_default_library_gen",
         "@com_github_masterminds_squirrel//:go_default_library",
     ],

backend/src/apiserver/list/BUILD.bazel

@@ -20,7 +20,9 @@ go_test(
     deps = [
         "//backend/api:go_default_library",
         "//backend/src/apiserver/filter:go_default_library",
+        "//backend/src/common/util:go_default_library",
         "@com_github_google_go_cmp//cmp:go_default_library",
+        "@com_github_google_go_cmp//cmp/cmpopts:go_default_library",
         "@com_github_masterminds_squirrel//:go_default_library",
     ],
 )

backend/src/apiserver/list/list.go

@@ -86,12 +86,20 @@ type Options struct {
 	*token
 }
 
+// Matches returns trues if the sorting and filtering criteria in o matches that
+// of the one supplied in opts.
+func (o *Options) Matches(opts *Options) bool {
+	return o.SortByFieldName == opts.SortByFieldName &&
+		o.IsDesc == opts.IsDesc &&
+		reflect.DeepEqual(o.Filter, opts.Filter)
+}
+
 // NewOptionsFromToken creates a new Options struct from the passed in token
 // which represents the next page of results. An empty nextPageToken will result
 // in an error.
 func NewOptionsFromToken(nextPageToken string, pageSize int) (*Options, error) {
 	if nextPageToken == "" {
-		return nil, fmt.Errorf("cannot create list.Options from empty page token")
+		return nil, util.NewInvalidInputError("cannot create list.Options from empty page token")
 	}
 	pageSize, err := validatePageSize(pageSize)
 	if err != nil {
@@ -215,12 +223,12 @@ func (o *Options) nextPageToken(listable Listable) (*token, error) {
 
 	sortByField := elem.FieldByName(o.SortByFieldName)
 	if !sortByField.IsValid() {
-		return nil, fmt.Errorf("cannot sort by field %q on type %q", o.SortByFieldName, elemName)
+		return nil, util.NewInvalidInputError("cannot sort by field %q on type %q", o.SortByFieldName, elemName)
 	}
 
 	keyField := elem.FieldByName(listable.PrimaryKeyColumnName())
 	if !keyField.IsValid() {
-		return nil, fmt.Errorf("type %q does not have key field %q", elemName, o.KeyFieldName)
+		return nil, util.NewInvalidInputError("type %q does not have key field %q", elemName, o.KeyFieldName)
 	}
 
 	return &token{

backend/src/apiserver/list/list_test.go

@@ -1,14 +1,15 @@
 package list
 
 import (
-	"errors"
 	"reflect"
 	"testing"
 
 	"github.com/kubeflow/pipelines/backend/src/apiserver/filter"
+	"github.com/kubeflow/pipelines/backend/src/common/util"
 
 	sq "github.com/Masterminds/squirrel"
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	api "github.com/kubeflow/pipelines/backend/api/go_client"
 )
 
@@ -97,11 +98,11 @@ func TestNextPageToken_InvalidSortByField(t *testing.T) {
 	inOpts := &Options{
 		PageSize: 10, token: &token{SortByFieldName: "Timestamp", IsDesc: true},
 	}
-	want := errors.New(`cannot sort by field "Timestamp" on type "fakeListable"`)
+	want := util.NewInvalidInputError(`cannot sort by field "Timestamp" on type "fakeListable"`)
 
 	got, err := inOpts.nextPageToken(l)
 
-	if !reflect.DeepEqual(err, want) {
+	if !cmp.Equal(err, want, cmpopts.IgnoreUnexported(util.UserError{})) {
 		t.Errorf("nextPageToken(%+v, %+v) =\nGot: %+v, %v\nWant: _, %v",
 			inOpts, l, got, err, want)
 	}
@@ -533,3 +534,74 @@ func TestTokenSerialization(t *testing.T) {
 		}
 	}
 }
+
+func TestMatches(t *testing.T) {
+	protoFilter1 := &api.Filter{
+		Predicates: []*api.Predicate{
+			&api.Predicate{
+				Key:   "Name",
+				Op:    api.Predicate_EQUALS,
+				Value: &api.Predicate_StringValue{StringValue: "SomeName"},
+			},
+		},
+	}
+	f1, err := filter.New(protoFilter1)
+	if err != nil {
+		t.Fatalf("failed to parse filter proto %+v: %v", protoFilter1, err)
+	}
+
+	protoFilter2 := &api.Filter{
+		Predicates: []*api.Predicate{
+			&api.Predicate{
+				Key:   "Name",
+				Op:    api.Predicate_NOT_EQUALS, // Not equals as opposed to equals above.
+				Value: &api.Predicate_StringValue{StringValue: "SomeName"},
+			},
+		},
+	}
+	f2, err := filter.New(protoFilter2)
+	if err != nil {
+		t.Fatalf("failed to parse filter proto %+v: %v", protoFilter2, err)
+	}
+
+	tests := []struct {
+		o1   *Options
+		o2   *Options
+		want bool
+	}{
+		{
+			o1:   &Options{token: &token{SortByFieldName: "SortField1", IsDesc: true}},
+			o2:   &Options{token: &token{SortByFieldName: "SortField2", IsDesc: true}},
+			want: false,
+		},
+		{
+			o1:   &Options{token: &token{SortByFieldName: "SortField1", IsDesc: true}},
+			o2:   &Options{token: &token{SortByFieldName: "SortField1", IsDesc: true}},
+			want: true,
+		},
+		{
+			o1:   &Options{token: &token{SortByFieldName: "SortField1", IsDesc: true}},
+			o2:   &Options{token: &token{SortByFieldName: "SortField1", IsDesc: false}},
+			want: false,
+		},
+		{
+			o1:   &Options{token: &token{Filter: f1}},
+			o2:   &Options{token: &token{Filter: f1}},
+			want: true,
+		},
+		{
+			o1:   &Options{token: &token{Filter: f1}},
+			o2:   &Options{token: &token{Filter: f2}},
+			want: false,
+		},
+	}
+
+	for _, test := range tests {
+		got := test.o1.Matches(test.o2)
+
+		if got != test.want {
+			t.Errorf("Matches(%+v, %+v) = %v, Want nil %v", test.o1, test.o2, got, test.want)
+			continue
+		}
+	}
+}

backend/src/apiserver/server/experiment_server.go

@@ -5,7 +5,6 @@ import (
 
 	"github.com/golang/protobuf/ptypes/empty"
 	api "github.com/kubeflow/pipelines/backend/api/go_client"
-	"github.com/kubeflow/pipelines/backend/src/apiserver/list"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/model"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/resource"
 	"github.com/kubeflow/pipelines/backend/src/common/util"
@@ -39,19 +38,7 @@ func (s *ExperimentServer) GetExperiment(ctx context.Context, request *api.GetEx
 
 func (s *ExperimentServer) ListExperiment(ctx context.Context, request *api.ListExperimentsRequest) (
 	*api.ListExperimentsResponse, error) {
-	var opts *list.Options
-	var err error
-	if request.PageToken != "" {
-		opts, err = list.NewOptionsFromToken(request.PageToken, int(request.PageSize))
-	} else {
-		var f *api.Filter
-		f, err = parseAPIFilter(request.Filter)
-		if err != nil {
-			return nil, err
-		}
-
-		opts, err = list.NewOptions(&model.Experiment{}, int(request.PageSize), request.SortBy, f)
-	}
+	opts, err := validatedListOptions(&model.Experiment{}, request.PageToken, int(request.PageSize), request.SortBy, request.Filter)
 
 	if err != nil {
 		return nil, util.Wrap(err, "Failed to create list options")

backend/src/apiserver/server/job_server.go

@@ -19,7 +19,6 @@ import (
 
 	"github.com/golang/protobuf/ptypes/empty"
 	api "github.com/kubeflow/pipelines/backend/api/go_client"
-	"github.com/kubeflow/pipelines/backend/src/apiserver/list"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/model"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/resource"
 	"github.com/kubeflow/pipelines/backend/src/common/util"
@@ -51,19 +50,7 @@ func (s *JobServer) GetJob(ctx context.Context, request *api.GetJobRequest) (*ap
 }
 
 func (s *JobServer) ListJobs(ctx context.Context, request *api.ListJobsRequest) (*api.ListJobsResponse, error) {
-	var opts *list.Options
-	var err error
-	if request.PageToken != "" {
-		opts, err = list.NewOptionsFromToken(request.PageToken, int(request.PageSize))
-	} else {
-		var f *api.Filter
-		f, err = parseAPIFilter(request.Filter)
-		if err != nil {
-			return nil, err
-		}
-
-		opts, err = list.NewOptions(&model.Job{}, int(request.PageSize), request.SortBy, f)
-	}
+	opts, err := validatedListOptions(&model.Job{}, request.PageToken, int(request.PageSize), request.SortBy, request.Filter)
 
 	if err != nil {
 		return nil, util.Wrap(err, "Failed to create list options")

backend/src/apiserver/server/list_request_util.go

@@ -24,6 +24,7 @@ import (
 	"github.com/golang/protobuf/jsonpb"
 	api "github.com/kubeflow/pipelines/backend/api/go_client"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/common"
+	"github.com/kubeflow/pipelines/backend/src/apiserver/list"
 	"github.com/kubeflow/pipelines/backend/src/common/util"
 )
 
@@ -183,3 +184,37 @@ func parseAPIFilter(encoded string) (*api.Filter, error) {
 	}
 	return f, nil
 }
+
+func validatedListOptions(listable list.Listable, pageToken string, pageSize int, sortBy string, filterSpec string) (*list.Options, error) {
+	defaultOpts := func() (*list.Options, error) {
+		f, err := parseAPIFilter(filterSpec)
+		if err != nil {
+			return nil, err
+		}
+
+		return list.NewOptions(listable, pageSize, sortBy, f)
+	}
+
+	if pageToken == "" {
+		return defaultOpts()
+	}
+
+	opts, err := list.NewOptionsFromToken(pageToken, pageSize)
+	if err != nil {
+		return nil, err
+	}
+
+	if sortBy != "" || filterSpec != "" {
+		// Sanity check that these match the page token.
+		do, err := defaultOpts()
+		if err != nil {
+			return nil, err
+		}
+
+		if !opts.Matches(do) {
+			return nil, util.NewInvalidInputError("page token does not match the supplied sort by and/or filtering criteria. Either specify the same criteria or leave the latter empty if page token is specified.")
+		}
+	}
+
+	return opts, nil
+}

backend/src/apiserver/server/list_request_util_test.go

@@ -23,6 +23,7 @@ import (
 
 	api "github.com/kubeflow/pipelines/backend/api/go_client"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/common"
+	"github.com/kubeflow/pipelines/backend/src/apiserver/list"
 	"github.com/kubeflow/pipelines/backend/src/common/util"
 	"github.com/stretchr/testify/assert"
 	"google.golang.org/grpc/codes"
@@ -215,3 +216,49 @@ func TestParseAPIFilter_DecodesEncodedString(t *testing.T) {
 			in, got, err, want, cmp.Diff(want, got))
 	}
 }
+
+type fakeListable struct {
+	PrimaryKey       string
+	FakeName         string
+	CreatedTimestamp int64
+}
+
+func (f *fakeListable) PrimaryKeyColumnName() string {
+	return "PrimaryKey"
+}
+
+func (f *fakeListable) DefaultSortField() string {
+	return "CreatedTimestamp"
+}
+
+var fakeAPIToModelMap = map[string]string{
+	"timestamp": "CreatedTimestamp",
+	"name":      "FakeName",
+	"id":        "PrimaryKey",
+}
+
+func (f *fakeListable) APIToModelFieldMap() map[string]string {
+	return fakeAPIToModelMap
+}
+
+func TestValidatedListOptions_Errors(t *testing.T) {
+	opts, err := list.NewOptions(&fakeListable{}, 10, "name asc", nil)
+	if err != nil {
+		t.Fatalf("list.NewOptions() = _, %+v; Want nil error", err)
+	}
+
+	npt, err := opts.NextPageToken(&fakeListable{})
+	if err != nil {
+		t.Fatalf("opt.NextPageToken() = _, %+v; Want nil error", err)
+	}
+
+	_, err = validatedListOptions(&fakeListable{}, npt, 10, "name asc", "")
+	if err != nil {
+		t.Fatalf("validatedListOptions(fakeListable, 10, \"name asc\") = _, %+v; Want nil error", err)
+	}
+
+	_, err = validatedListOptions(&fakeListable{}, npt, 10, "name desc", "")
+	if err == nil {
+		t.Fatalf("validatedListOptions(fakeListable, 10, \"name desc\") = _, %+v; Want error", err)
+	}
+}

backend/src/apiserver/server/pipeline_server.go

@@ -22,7 +22,6 @@ import (
 
 	"github.com/golang/protobuf/ptypes/empty"
 	api "github.com/kubeflow/pipelines/backend/api/go_client"
-	"github.com/kubeflow/pipelines/backend/src/apiserver/list"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/model"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/resource"
 	"github.com/kubeflow/pipelines/backend/src/common/util"
@@ -72,19 +71,7 @@ func (s *PipelineServer) GetPipeline(ctx context.Context, request *api.GetPipeli
 }
 
 func (s *PipelineServer) ListPipelines(ctx context.Context, request *api.ListPipelinesRequest) (*api.ListPipelinesResponse, error) {
-	var opts *list.Options
-	var err error
-	if request.PageToken != "" {
-		opts, err = list.NewOptionsFromToken(request.PageToken, int(request.PageSize))
-	} else {
-		var f *api.Filter
-		f, err = parseAPIFilter(request.Filter)
-		if err != nil {
-			return nil, err
-		}
-
-		opts, err = list.NewOptions(&model.Pipeline{}, int(request.PageSize), request.SortBy, f)
-	}
+	opts, err := validatedListOptions(&model.Pipeline{}, request.PageToken, int(request.PageSize), request.SortBy, request.Filter)
 
 	if err != nil {
 		return nil, util.Wrap(err, "Failed to create list options")

backend/src/apiserver/server/run_server.go

@@ -19,7 +19,6 @@ import (
 
 	"github.com/golang/protobuf/ptypes/empty"
 	api "github.com/kubeflow/pipelines/backend/api/go_client"
-	"github.com/kubeflow/pipelines/backend/src/apiserver/list"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/model"
 	"github.com/kubeflow/pipelines/backend/src/apiserver/resource"
 	"github.com/kubeflow/pipelines/backend/src/common/util"
@@ -50,19 +49,7 @@ func (s *RunServer) GetRun(ctx context.Context, request *api.GetRunRequest) (*ap
 }
 
 func (s *RunServer) ListRuns(ctx context.Context, request *api.ListRunsRequest) (*api.ListRunsResponse, error) {
-	var opts *list.Options
-	var err error
-	if request.PageToken != "" {
-		opts, err = list.NewOptionsFromToken(request.PageToken, int(request.PageSize))
-	} else {
-		var f *api.Filter
-		f, err = parseAPIFilter(request.Filter)
-		if err != nil {
-			return nil, err
-		}
-
-		opts, err = list.NewOptions(&model.Run{}, int(request.PageSize), request.SortBy, f)
-	}
+	opts, err := validatedListOptions(&model.Run{}, request.PageToken, int(request.PageSize), request.SortBy, request.Filter)
 
 	if err != nil {
 		return nil, util.Wrap(err, "Failed to create list options")