Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Fix sorting bug for named entities #394

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix sorting bug for named entities #394

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
19c0934
Fix sorting bug for named entities
pmahindrakar-oss Apr 7, 2022
7462000
lint fixes
pmahindrakar-oss Apr 7, 2022
b0cde31
Fixed filtering
pmahindrakar-oss Apr 8, 2022
47bb2ec
removed unused
pmahindrakar-oss Apr 8, 2022
0e3915c
using distinct to filter results
pmahindrakar-oss Apr 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions pkg/common/sorting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,22 @@ const gormAscending = "%s asc"

type SortParameter interface {
GetGormOrderExpr() string
GetSortKey() string
}

type sortParamImpl struct {
gormOrderExpression string
sortKey string
}

func (s *sortParamImpl) GetGormOrderExpr() string {
return s.gormOrderExpression
}

func (s *sortParamImpl) GetSortKey() string {
return s.sortKey
}

func NewSortParameter(sort admin.Sort) (SortParameter, error) {
var gormOrderExpression string
switch sort.Direction {
Expand All @@ -35,5 +41,6 @@ func NewSortParameter(sort admin.Sort) (SortParameter, error) {
}
return &sortParamImpl{
gormOrderExpression: gormOrderExpression,
sortKey: sort.Key,
}, nil
}
2 changes: 2 additions & 0 deletions pkg/common/sorting_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ func TestSortParameter_Ascending(t *testing.T) {
})
assert.Nil(t, err)
assert.Equal(t, "name asc", sortParameter.GetGormOrderExpr())
assert.Equal(t, "name", sortParameter.GetSortKey())
}

func TestSortParameter_Descending(t *testing.T) {
Expand All @@ -23,4 +24,5 @@ func TestSortParameter_Descending(t *testing.T) {
})
assert.Nil(t, err)
assert.Equal(t, "project desc", sortParameter.GetGormOrderExpr())
assert.Equal(t, "project", sortParameter.GetSortKey())
}
14 changes: 9 additions & 5 deletions pkg/repositories/gormimpl/named_entity_repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ import (
"context"
"fmt"

"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/core"
"google.golang.org/grpc/codes"

"github.com/flyteorg/flyteadmin/pkg/common"
adminErrors "github.com/flyteorg/flyteadmin/pkg/errors"
"github.com/flyteorg/flyteadmin/pkg/repositories/errors"
"github.com/flyteorg/flyteadmin/pkg/repositories/interfaces"
"github.com/flyteorg/flyteadmin/pkg/repositories/models"
"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/core"
"github.com/flyteorg/flytestdlib/promutils"

"google.golang.org/grpc/codes"
"gorm.io/gorm"
)

Expand All @@ -33,12 +33,15 @@ func getSubQueryJoin(db *gorm.DB, tableName string, input interfaces.ListNamedEn
Table(tableName).
Where(map[string]interface{}{Project: input.Project, Domain: input.Domain}).
Limit(input.Limit).
Offset(input.Offset).
Group(identifierGroupBy)
Offset(input.Offset)

// Apply consistent sort ordering.
if input.SortParameter != nil {
identifierGroupByWithOrderKey := fmt.Sprintf("%s, %s, %s, %s", Project, Domain, Name, input.SortParameter.GetSortKey())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we check that the sort parameter isn't already in (Project, Domain, Name)?

Copy link
Contributor Author

@pmahindrakar-oss pmahindrakar-oss Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have removed this functionality of dynamically adding the column and instead made it part of the identifierGroupBy which takes care of fixing ListWorkflowIds , ListLaunchPlanIds, ListTaskIdentifiers

tx = tx.Group(identifierGroupByWithOrderKey)
tx = tx.Order(input.SortParameter.GetGormOrderExpr())
} else {
tx = tx.Group(identifierGroupBy)
}

return db.Joins(fmt.Sprintf(joinString, input.ResourceType), tx)
Expand Down Expand Up @@ -191,6 +194,7 @@ func (r *NamedEntityRepo) List(ctx context.Context, input interfaces.ListNamedEn
}
// Apply sort ordering.
if input.SortParameter != nil {
tx = tx.Group(input.SortParameter.GetSortKey())
tx = tx.Order(input.SortParameter.GetGormOrderExpr())
}

Expand Down
114 changes: 109 additions & 5 deletions pkg/repositories/gormimpl/named_entity_repo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,20 @@ package gormimpl

import (
"context"
"fmt"
"testing"

"github.com/flyteorg/flyteadmin/pkg/common"

"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"

mocket "github.com/Selvatico/go-mocket"
adminErrors "github.com/flyteorg/flyteadmin/pkg/errors"
"github.com/flyteorg/flyteadmin/pkg/repositories/errors"
"github.com/flyteorg/flyteadmin/pkg/repositories/interfaces"
"github.com/flyteorg/flyteadmin/pkg/repositories/models"
"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
mockScope "github.com/flyteorg/flytestdlib/promutils"

mocket "github.com/Selvatico/go-mocket"
"github.com/stretchr/testify/assert"
"google.golang.org/grpc/codes"
)

func getMockNamedEntityResponseFromDb(expected models.NamedEntity) map[string]interface{} {
Expand Down Expand Up @@ -155,7 +157,7 @@ func TestListNamedEntity(t *testing.T) {
mockQuery := GlobalMock.NewMock()

mockQuery.WithQuery(
`SELECT entities.project,entities.domain,entities.name,'2' AS resource_type,named_entity_metadata.description,named_entity_metadata.state FROM "named_entity_metadata" RIGHT JOIN (SELECT project,domain,name FROM "workflows" WHERE "domain" = $1 AND "project" = $2 GROUP BY project, domain, name ORDER BY name desc LIMIT 20) AS entities ON named_entity_metadata.resource_type = 2 AND named_entity_metadata.project = entities.project AND named_entity_metadata.domain = entities.domain AND named_entity_metadata.name = entities.name GROUP BY entities.project, entities.domain, entities.name, named_entity_metadata.description, named_entity_metadata.state ORDER BY name desc`).WithReply(results)
`SELECT entities.project,entities.domain,entities.name,'2' AS resource_type,named_entity_metadata.description,named_entity_metadata.state FROM "named_entity_metadata" RIGHT JOIN (SELECT project,domain,name FROM "workflows" WHERE "domain" = $1 AND "project" = $2 GROUP BY project, domain, name, name ORDER BY name desc LIMIT 20) AS entities ON named_entity_metadata.resource_type = 2 AND named_entity_metadata.project = entities.project AND named_entity_metadata.domain = entities.domain AND named_entity_metadata.name = entities.name GROUP BY "name",entities.project, entities.domain, entities.name, named_entity_metadata.description, named_entity_metadata.state ORDER BY name desc`).WithReply(results)

sortParameter, _ := common.NewSortParameter(admin.Sort{
Direction: admin.Sort_DESCENDING,
Expand All @@ -173,3 +175,105 @@ func TestListNamedEntity(t *testing.T) {
assert.NoError(t, err)
assert.Len(t, output.Entities, 1)
}

func TestListNamedEntityTxErrorCases(t *testing.T) {
metadataRepo := NewNamedEntityRepo(GetDbForTest(t), errors.NewTestErrorTransformer(), mockScope.NewTestScope())
GlobalMock := mocket.Catcher.Reset()
GlobalMock.Logging = true
mockQuery := GlobalMock.NewMock()

mockQuery.WithQuery(
`SELECT entities.project,entities.domain,entities.name,'2' AS resource_type,named_entity_metadata.description,named_entity_metadata.state FROM "named_entity_metadata" RIGHT JOIN (SELECT project,domain,name FROM "workflows" WHERE "domain" = $1 AND "project" = $2 GROUP BY project, domain, name, name ORDER BY name desc LIMIT 20) AS entities ON named_entity_metadata.resource_type = 2 AND named_entity_metadata.project = entities.project AND named_entity_metadata.domain = entities.domain AND named_entity_metadata.name = entities.name GROUP BY "name",entities.project, entities.domain, entities.name, named_entity_metadata.description, named_entity_metadata.state ORDER BY name desc`).WithError(fmt.Errorf("failed"))

sortParameter, _ := common.NewSortParameter(admin.Sort{
Direction: admin.Sort_DESCENDING,
Key: "name",
})
output, err := metadataRepo.List(context.Background(), interfaces.ListNamedEntityInput{
ResourceType: resourceType,
Project: "admintests",
Domain: "development",
ListResourceInput: interfaces.ListResourceInput{
Limit: 20,
SortParameter: sortParameter,
},
})
assert.Equal(t, "Test transformer failed to find transformation to apply", err.Error())
assert.Len(t, output.Entities, 0)
}

func TestListNamedEntityInputErrorCases(t *testing.T) {
type test struct {
input interfaces.ListNamedEntityInput
wantedError error
wantedLength int
}

sortParameter, _ := common.NewSortParameter(admin.Sort{
Direction: admin.Sort_DESCENDING,
Key: "name",
})

tests := []test{
{
input: interfaces.ListNamedEntityInput{
ResourceType: resourceType,
Project: "",
Domain: "development",
ListResourceInput: interfaces.ListResourceInput{
Limit: 20,
SortParameter: sortParameter,
},
},
wantedError: errors.GetInvalidInputError(Project),
wantedLength: 0,
},
{
input: interfaces.ListNamedEntityInput{
ResourceType: resourceType,
Project: "project",
Domain: "",
ListResourceInput: interfaces.ListResourceInput{
Limit: 20,
SortParameter: sortParameter,
},
},
wantedError: errors.GetInvalidInputError(Domain),
wantedLength: 0,
},
{
input: interfaces.ListNamedEntityInput{
ResourceType: resourceType,
Project: "project",
Domain: "development",
ListResourceInput: interfaces.ListResourceInput{
Limit: 0,
SortParameter: sortParameter,
},
},
wantedError: errors.GetInvalidInputError(limit),
wantedLength: 0,
},
{
input: interfaces.ListNamedEntityInput{
ResourceType: -1,
Project: "project",
Domain: "development",
ListResourceInput: interfaces.ListResourceInput{
Limit: 20,
SortParameter: sortParameter,
},
},
wantedError: adminErrors.NewFlyteAdminErrorf(codes.InvalidArgument,
"Cannot list entity names for resource type: %v", -1),
wantedLength: 0,
},
}

metadataRepo := NewNamedEntityRepo(GetDbForTest(t), errors.NewTestErrorTransformer(), mockScope.NewTestScope())
for _, tc := range tests {
output, err := metadataRepo.List(context.Background(), tc.input)
assert.Len(t, output.Entities, tc.wantedLength)
assert.Equal(t, tc.wantedError, err)
}
}