Validate sort order in admin.ResourceListRequest

go.mod

@@ -13,7 +13,7 @@ require (
 	github.com/cloudevents/sdk-go/v2 v2.8.0
 	github.com/coreos/go-oidc v2.2.1+incompatible
 	github.com/evanphx/json-patch v4.12.0+incompatible
-	github.com/flyteorg/flyteidl v1.5.14
+	github.com/flyteorg/flyteidl v1.5.17-0.20230822102414-1c76702b5f6a
 	github.com/flyteorg/flyteplugins v1.0.67
 	github.com/flyteorg/flytepropeller v1.1.98
 	github.com/flyteorg/flytestdlib v1.0.22

go.sum

@@ -295,6 +295,8 @@ github.com/felixge/httpsnoop v1.0.1 h1:lvB5Jl89CsZtGIWuTcDM1E/vkVs49/Ml7JJe07l8S
 github.com/felixge/httpsnoop v1.0.1/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U=
 github.com/flyteorg/flyteidl v1.5.14 h1:+3ewipoOp82fPyIVgvvrMq1lorl5Kz3Lh6sh/a9+loI=
 github.com/flyteorg/flyteidl v1.5.14/go.mod h1:EtE/muM2lHHgBabjYcxqe9TWeJSL0kXwbI0RgVwI4Og=
+github.com/flyteorg/flyteidl v1.5.17-0.20230822102414-1c76702b5f6a h1:eqQ7g71w8NG5fnTq1QiyPNoIaryNnU2wW/2DtATFHa0=
+github.com/flyteorg/flyteidl v1.5.17-0.20230822102414-1c76702b5f6a/go.mod h1:EtE/muM2lHHgBabjYcxqe9TWeJSL0kXwbI0RgVwI4Og=
 github.com/flyteorg/flyteplugins v1.0.67 h1:d2FXpwxQwX/k4YdmhuusykOemHb/cUTPEob4WBmdpjE=
 github.com/flyteorg/flyteplugins v1.0.67/go.mod h1:HHt4nKDKVwrZPKDsj99dNtDSIJL378xNotYMA3a/TFA=
 github.com/flyteorg/flytepropeller v1.1.98 h1:Zk2ENYB9VZRT5tFUIFjm+aCkr0TU2EuyJ5gh52fpLoA=
@@ -760,6 +762,7 @@ github.com/google/martian v2.1.0+incompatible/go.mod h1:9I4somxYTbIHy5NJKHRl3wXi
 github.com/google/martian/v3 v3.0.0/go.mod h1:y5Zk1BBys9G+gd6Jrk0W3cC1+ELVxBWuIGO+w/tUAp0=
 github.com/google/martian/v3 v3.1.0/go.mod h1:y5Zk1BBys9G+gd6Jrk0W3cC1+ELVxBWuIGO+w/tUAp0=
 github.com/google/martian/v3 v3.3.2 h1:IqNFLAmvJOgVlpdEBiQbDc2EwKW77amAycfTuWKdfvw=
+github.com/google/martian/v3 v3.3.2/go.mod h1:oBOf6HBosgwRXnUGWUB05QECsc6uvmMiJ3+6W4l/CUk=
 github.com/google/pprof v0.0.0-20181206194817-3ea8567a2e57/go.mod h1:zfwlbNMJ+OItoe0UupaVj+oy1omPYYDuagoSzA8v9mc=
 github.com/google/pprof v0.0.0-20190515194954-54271f7e092f/go.mod h1:zfwlbNMJ+OItoe0UupaVj+oy1omPYYDuagoSzA8v9mc=
 github.com/google/pprof v0.0.0-20191218002539-d4f498aebedc/go.mod h1:ZgVRPoUq/hfqzAqh7sHMqb3I9Rq5C59dIz2SbBwJ4eM=
@@ -785,6 +788,7 @@ github.com/googleapis/gax-go/v2 v2.0.4/go.mod h1:0Wqv26UfaUD9n4G6kQubkQ+KchISgw+
 github.com/googleapis/gax-go/v2 v2.0.5/go.mod h1:DWXyrwAJ9X0FpwwEdw+IPEYBICEFu5mhpdKc/us6bOk=
 github.com/googleapis/gax-go/v2 v2.7.1 h1:gF4c0zjUP2H/s/hEGyLA3I0fA2ZWjzYiONAD6cvPr8A=
 github.com/googleapis/gax-go/v2 v2.7.1/go.mod h1:4orTrqY6hXxxaUL4LHIPl6lGo8vAE38/qKbhSAKP6QI=
+github.com/googleapis/go-type-adapters v1.0.0/go.mod h1:zHW75FOG2aur7gAO2B+MLby+cLsWGBF62rFAi7WjWO4=
 github.com/googleapis/google-cloud-go-testing v0.0.0-20200911160855-bcd43fbb19e8/go.mod h1:dvDLG8qkwmyD9a/MJJN3XJcT3xFxOKAvTZGvuZmac9g=
 github.com/gopherjs/gopherjs v0.0.0-20181004151105-1babbf986f6f/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY=
 github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY=

pkg/clusterresource/impl/db_admin_data_provider.go

@@ -3,13 +3,15 @@ package impl
 import (
 	"context"
 
+	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
+
 	"github.com/flyteorg/flyteadmin/pkg/clusterresource/interfaces"
 	"github.com/flyteorg/flyteadmin/pkg/common"
 	managerInterfaces "github.com/flyteorg/flyteadmin/pkg/manager/interfaces"
+	"github.com/flyteorg/flyteadmin/pkg/repositories/gormimpl"
 	repositoryInterfaces "github.com/flyteorg/flyteadmin/pkg/repositories/interfaces"
 	"github.com/flyteorg/flyteadmin/pkg/repositories/transformers"
 	runtimeInterfaces "github.com/flyteorg/flyteadmin/pkg/runtime/interfaces"
-	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
 )
 
 // Implementation of an interfaces.FlyteAdminDataProvider which fetches data directly from the provided database connection.
@@ -46,14 +48,21 @@ func (p dbAdminProvider) getDomains() []*admin.Domain {
 	return domains
 }
 
+var descCreatedAtSortParam = admin.Sort{
+	Direction: admin.Sort_DESCENDING,
+	Key:       "created_at",
+}
+
+var descCreatedAtSortDBParam, _ = common.NewSortParameter(&descCreatedAtSortParam, gormimpl.ProjectColumns)
+
 func (p dbAdminProvider) GetProjects(ctx context.Context) (*admin.Projects, error) {
 	filter, err := common.NewSingleValueFilter(common.Project, common.NotEqual, "state", int32(admin.Project_ARCHIVED))
 	if err != nil {
 		return nil, err
 	}
 	projectModels, err := p.db.ProjectRepo().List(ctx, repositoryInterfaces.ListResourceInput{
-		SortParameter: descCreatedAtSortDBParam,
-		InlineFilters: []common.InlineFilter{filter},
+		SortParameters: descCreatedAtSortDBParam,
+		InlineFilters:  []common.InlineFilter{filter},
 	})
 	if err != nil {
 		return nil, err

pkg/clusterresource/impl/db_admin_data_provider_test.go

@@ -3,19 +3,22 @@ package impl
 import (
 	"context"
 	"errors"
+	"strings"
 	"testing"
 
+	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
+	"github.com/stretchr/testify/assert"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+
+	"github.com/flyteorg/flyteadmin/pkg/common"
 	"github.com/flyteorg/flyteadmin/pkg/manager/interfaces"
 	"github.com/flyteorg/flyteadmin/pkg/manager/mocks"
 	repoInterfaces "github.com/flyteorg/flyteadmin/pkg/repositories/interfaces"
 	repoMocks "github.com/flyteorg/flyteadmin/pkg/repositories/mocks"
 	"github.com/flyteorg/flyteadmin/pkg/repositories/models"
 	runtimeInterfaces "github.com/flyteorg/flyteadmin/pkg/runtime/interfaces"
 	configMocks "github.com/flyteorg/flyteadmin/pkg/runtime/mocks"
-	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
-	"github.com/stretchr/testify/assert"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 var errFoo = errors.New("foo")
@@ -104,7 +107,7 @@ func TestGetProjects(t *testing.T) {
 		mockRepo.(*repoMocks.MockRepository).ProjectRepoIface = &repoMocks.MockProjectRepo{
 			ListProjectsFunction: func(ctx context.Context, input repoInterfaces.ListResourceInput) ([]models.Project, error) {
 				assert.Len(t, input.InlineFilters, 1)
-				assert.Equal(t, input.SortParameter.GetGormOrderExpr(), "created_at desc")
+				assert.Equal(t, "created_at desc", sortParamsSQL(input.SortParameters))
 				return []models.Project{
 					{
 						Identifier: "flytesnacks",
@@ -141,3 +144,11 @@ func TestGetProjects(t *testing.T) {
 		assert.EqualError(t, err, errFoo.Error())
 	})
 }
+
+func sortParamsSQL(params []common.SortParameter) string {
+	sqls := make([]string, len(params))
+	for i, param := range params {
+		sqls[i] = param.GetGormOrderExpr()
+	}
+	return strings.Join(sqls, ", ")
+}

pkg/clusterresource/impl/shared.go

@@ -1,19 +1,11 @@
 package impl
 
 import (
-	"github.com/flyteorg/flyteadmin/pkg/common"
-	"github.com/flyteorg/flyteadmin/pkg/errors"
-	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
 	"google.golang.org/grpc/codes"
+
+	"github.com/flyteorg/flyteadmin/pkg/errors"
 )
 
 func NewMissingEntityError(entity string) error {
 	return errors.NewFlyteAdminErrorf(codes.NotFound, "Failed to find [%s]", entity)
 }
-
-var descCreatedAtSortParam = admin.Sort{
-	Direction: admin.Sort_DESCENDING,
-	Key:       "created_at",
-}
-
-var descCreatedAtSortDBParam, _ = common.NewSortParameter(descCreatedAtSortParam)

pkg/common/sorting.go

@@ -3,9 +3,12 @@ package common
 import (
 	"fmt"
 
-	"github.com/flyteorg/flyteadmin/pkg/errors"
+	"k8s.io/apimachinery/pkg/util/sets"
+
 	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
 	"google.golang.org/grpc/codes"
+
+	"github.com/flyteorg/flyteadmin/pkg/errors"
 )
 
 const gormDescending = "%s desc"
@@ -23,17 +26,46 @@ func (s *sortParamImpl) GetGormOrderExpr() string {
 	return s.gormOrderExpression
 }
 
-func NewSortParameter(sort admin.Sort) (SortParameter, error) {
+func NewSortParameter(sort *admin.Sort, allowed sets.String) ([]SortParameter, error) {
+	if sort == nil {
+		return nil, nil
+	}
+
+	key := sort.GetKey()
+	if !allowed.Has(key) {
+		return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument, "invalid sort_key: %s", key)
+	}
+
 	var gormOrderExpression string
-	switch sort.Direction {
+	switch sort.GetDirection() {
 	case admin.Sort_DESCENDING:
-		gormOrderExpression = fmt.Sprintf(gormDescending, sort.Key)
+		gormOrderExpression = fmt.Sprintf(gormDescending, key)
 	case admin.Sort_ASCENDING:
-		gormOrderExpression = fmt.Sprintf(gormAscending, sort.Key)
+		gormOrderExpression = fmt.Sprintf(gormAscending, key)
 	default:
 		return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument, "invalid sort order specified: %v", sort)
 	}
-	return &sortParamImpl{
-		gormOrderExpression: gormOrderExpression,
-	}, nil
+
+	return []SortParameter{&sortParamImpl{gormOrderExpression: gormOrderExpression}}, nil
+}
+
+func NewSortParameters(request *admin.ResourceListRequest, allowed sets.String) ([]SortParameter, error) {
+	if len(request.SortKeys) > 0 && request.SortBy != nil {
+		return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument, "cannot specify both sort_keys and sort_by")
+	}
+
+	if request.SortBy != nil {
+		request.SortKeys = append(request.SortKeys, request.SortBy)
+	}
+
+	sortParams := make([]SortParameter, 0, len(request.SortKeys))
+	for _, sortKey := range request.SortKeys {
+		params, err := NewSortParameter(sortKey, allowed)
+		if err != nil {
+			return sortParams, err
+		}
+		sortParams = append(sortParams, params...)
+	}
+
+	return sortParams, nil
 }

pkg/common/sorting_test.go

@@ -5,22 +5,93 @@ import (
 
 	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
 	"github.com/stretchr/testify/assert"
+	"google.golang.org/grpc/codes"
+	"k8s.io/apimachinery/pkg/util/sets"
+
+	"github.com/flyteorg/flyteadmin/pkg/errors"
 )
 
+func TestSortParameter_Empty(t *testing.T) {
+	sortParameter, err := NewSortParameter(nil, sets.NewString())
+
+	assert.NoError(t, err)
+	assert.Nil(t, sortParameter)
+}
+
+func TestSortParameter_InvalidSortKey(t *testing.T) {
+	expected := errors.NewFlyteAdminErrorf(codes.InvalidArgument, "invalid sort_key: wrong")
+
+	_, err := NewSortParameter(&admin.Sort{
+		Direction: admin.Sort_ASCENDING,
+		Key:       "wrong",
+	}, sets.NewString("name"))
+
+	assert.Equal(t, expected, err)
+}
+
 func TestSortParameter_Ascending(t *testing.T) {
-	sortParameter, err := NewSortParameter(admin.Sort{
+	sortParameter, err := NewSortParameter(&admin.Sort{
 		Direction: admin.Sort_ASCENDING,
 		Key:       "name",
-	})
-	assert.Nil(t, err)
-	assert.Equal(t, "name asc", sortParameter.GetGormOrderExpr())
+	}, sets.NewString("name"))
+
+	assert.NoError(t, err)
+	assert.Equal(t, "name asc", sortParameter[0].GetGormOrderExpr())
 }
 
 func TestSortParameter_Descending(t *testing.T) {
-	sortParameter, err := NewSortParameter(admin.Sort{
+	sortParameter, err := NewSortParameter(&admin.Sort{
 		Direction: admin.Sort_DESCENDING,
 		Key:       "project",
-	})
-	assert.Nil(t, err)
-	assert.Equal(t, "project desc", sortParameter.GetGormOrderExpr())
+	}, sets.NewString("project"))
+
+	assert.NoError(t, err)
+	assert.Equal(t, "project desc", sortParameter[0].GetGormOrderExpr())
+}
+
+func TestSortParameters_SingleAndMultipleSortKeys(t *testing.T) {
+	expected := errors.NewFlyteAdminErrorf(codes.InvalidArgument, "cannot specify both sort_keys and sort_by")
+
+	_, err := NewSortParameters(&admin.ResourceListRequest{
+		SortBy:   &admin.Sort{},
+		SortKeys: []*admin.Sort{{}},
+	}, sets.NewString())
+
+	assert.Equal(t, expected, err)
+}
+
+func TestSortParameters_SingleSortKey(t *testing.T) {
+	params, err := NewSortParameters(&admin.ResourceListRequest{
+		SortBy: &admin.Sort{Key: "foo"},
+	}, sets.NewString("foo"))
+
+	assert.NoError(t, err)
+	assert.Equal(t, "foo desc", params[0].GetGormOrderExpr())
+}
+
+func TestSortParameters_OK(t *testing.T) {
+	params, err := NewSortParameters(&admin.ResourceListRequest{
+		SortKeys: []*admin.Sort{
+			{Key: "key"},
+			{Key: "foo", Direction: admin.Sort_ASCENDING},
+		},
+	}, sets.NewString("key", "foo"))
+
+	assert.NoError(t, err)
+	if assert.Len(t, params, 2) {
+		assert.Equal(t, "key desc", params[0].GetGormOrderExpr())
+		assert.Equal(t, "foo asc", params[1].GetGormOrderExpr())
+	}
+}
+
+func TestSortParameters_Invalid(t *testing.T) {
+	expected := errors.NewFlyteAdminErrorf(codes.InvalidArgument, "invalid sort_key: foo")
+
+	_, err := NewSortParameters(&admin.ResourceListRequest{
+		SortKeys: []*admin.Sort{
+			{Key: "foo"},
+		},
+	}, sets.NewString("key"))
+
+	assert.Equal(t, expected, err)
 }

pkg/manager/impl/description_entity_manager.go

@@ -4,22 +4,22 @@ import (
 	"context"
 	"strconv"
 
+	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
 	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/core"
+	"github.com/flyteorg/flytestdlib/contextutils"
+	"github.com/flyteorg/flytestdlib/logger"
+	"github.com/flyteorg/flytestdlib/promutils"
+	"google.golang.org/grpc/codes"
 
 	"github.com/flyteorg/flyteadmin/pkg/common"
-
 	"github.com/flyteorg/flyteadmin/pkg/errors"
 	"github.com/flyteorg/flyteadmin/pkg/manager/impl/util"
 	"github.com/flyteorg/flyteadmin/pkg/manager/impl/validation"
 	"github.com/flyteorg/flyteadmin/pkg/manager/interfaces"
+	"github.com/flyteorg/flyteadmin/pkg/repositories/gormimpl"
 	repoInterfaces "github.com/flyteorg/flyteadmin/pkg/repositories/interfaces"
 	"github.com/flyteorg/flyteadmin/pkg/repositories/transformers"
 	runtimeInterfaces "github.com/flyteorg/flyteadmin/pkg/runtime/interfaces"
-	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
-	"github.com/flyteorg/flytestdlib/contextutils"
-	"github.com/flyteorg/flytestdlib/logger"
-	"github.com/flyteorg/flytestdlib/promutils"
-	"google.golang.org/grpc/codes"
 )
 
 type DescriptionEntityMetrics struct {
@@ -65,23 +65,21 @@ func (d *DescriptionEntityManager) ListDescriptionEntity(ctx context.Context, re
 		logger.Error(ctx, "failed to get database filter")
 		return nil, err
 	}
-	var sortParameter common.SortParameter
-	if request.SortBy != nil {
-		sortParameter, err = common.NewSortParameter(*request.SortBy)
-		if err != nil {
-			return nil, err
-		}
+	sortParameters, err := common.NewSortParameter(request.SortBy, gormimpl.DescriptionEntityColumns)
+	if err != nil {
+		return nil, err
 	}
+
 	offset, err := validation.ValidateToken(request.Token)
 	if err != nil {
 		return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument,
 			"invalid pagination token %s for ListWorkflows", request.Token)
 	}
 	listDescriptionEntitiesInput := repoInterfaces.ListResourceInput{
-		Limit:         int(request.Limit),
-		Offset:        offset,
-		InlineFilters: filters,
-		SortParameter: sortParameter,
+		Limit:          int(request.Limit),
+		Offset:         offset,
+		InlineFilters:  filters,
+		SortParameters: sortParameters,
 	}
 	output, err := d.db.DescriptionEntityRepo().List(ctx, listDescriptionEntitiesInput)
 	if err != nil {

pkg/manager/impl/description_entity_manager_test.go

@@ -4,15 +4,16 @@ import (
 	"context"
 	"testing"
 
+	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
+	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/core"
+	mockScope "github.com/flyteorg/flytestdlib/promutils"
+	"github.com/stretchr/testify/assert"
+
 	"github.com/flyteorg/flyteadmin/pkg/manager/impl/testutils"
 	"github.com/flyteorg/flyteadmin/pkg/repositories/interfaces"
 	repositoryMocks "github.com/flyteorg/flyteadmin/pkg/repositories/mocks"
 	runtimeInterfaces "github.com/flyteorg/flyteadmin/pkg/runtime/interfaces"
 	runtimeMocks "github.com/flyteorg/flyteadmin/pkg/runtime/mocks"
-	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
-	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/core"
-	mockScope "github.com/flyteorg/flytestdlib/promutils"
-	"github.com/stretchr/testify/assert"
 )
 
 var descriptionEntityIdentifier = core.Identifier{