Skip to content

Commit

Permalink
feat: add validation in deletion query and remove redundant code
Browse files Browse the repository at this point in the history
  • Loading branch information
Muhammad Luthfi Fahlevi committed Aug 12, 2024
1 parent 57897ea commit e4903d0
Show file tree
Hide file tree
Showing 15 changed files with 264 additions and 126 deletions.
7 changes: 6 additions & 1 deletion internal/server/v1beta1/asset.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,12 +308,17 @@ func (server *APIServer) DeleteAsset(ctx context.Context, req *compassv1beta1.De
}

func (server *APIServer) DeleteAssets(ctx context.Context, req *compassv1beta1.DeleteAssetsRequest) (*compassv1beta1.DeleteAssetsResponse, error) {
var affectedRows uint32
_, err := server.ValidateUserInCtx(ctx)
if err != nil {
return nil, err
}
defer func() {
server.logger.Warn("the number of affected rows is %d", affectedRows)
}()

affectedRows, err := server.assetService.DeleteAssets(ctx, req.QueryExpr, req.DryRun)
server.logger.Warn("delete request: %v", req)
affectedRows, err = server.assetService.DeleteAssets(ctx, req.QueryExpr, req.DryRun)
if err != nil {
return nil, status.Error(codes.InvalidArgument, err.Error())
}
Expand Down
26 changes: 3 additions & 23 deletions internal/store/elasticsearch/discovery_repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"time"

"github.com/goto/compass/core/asset"
generichelper "github.com/goto/compass/pkg/generic_helper"
queryexpr "github.com/goto/compass/pkg/query_expr"
"github.com/goto/salt/log"
)
Expand All @@ -26,26 +25,6 @@ type DiscoveryRepository struct {
columnSearchExclusionList []string
}

type DeleteAssetESExpr struct {
queryexpr.ESExpr
}

func (d *DeleteAssetESExpr) Validate() error {
identifiers, err := queryexpr.GetIdentifiers(d.String())
if err != nil {
return err
}

mustExist := generichelper.Contains(identifiers, "refreshed_at") &&
generichelper.Contains(identifiers, "type") &&
generichelper.Contains(identifiers, "service")
if !mustExist {
return fmt.Errorf("must exists these identifiers: refreshed_at, type. Current identifiers: %v", identifiers)
}

return nil
}

func NewDiscoveryRepository(cli *Client, logger log.Logger, requestTimeout time.Duration, colSearchExclusionList []string) *DiscoveryRepository {
return &DiscoveryRepository{
cli: cli,
Expand Down Expand Up @@ -171,8 +150,9 @@ func (repo *DiscoveryRepository) DeleteByQueryExpr(ctx context.Context, queryExp
return asset.ErrEmptyQuery
}

deleteAssetESExpr := &DeleteAssetESExpr{
queryexpr.ESExpr(queryExpr),
expr := queryexpr.ESExpr(queryExpr)
deleteAssetESExpr := &queryexpr.DeleteAssetExpr{
ExprStr: &expr,
}
esQuery, err := queryexpr.ValidateAndGetQueryFromExpr(deleteAssetESExpr)
if err != nil {
Expand Down
5 changes: 3 additions & 2 deletions internal/store/elasticsearch/discovery_repository_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -432,8 +432,9 @@ func TestDiscoveryRepositoryDeleteByQueryExpr(t *testing.T) {
err = repo.DeleteByQueryExpr(ctx, queryExpr)
assert.NoError(t, err)

deleteAssetESExpr := &store.DeleteAssetESExpr{
ESExpr: queryexpr.ESExpr(queryExpr),
expr := queryexpr.ESExpr(queryExpr)
deleteAssetESExpr := &queryexpr.DeleteAssetExpr{
ExprStr: &expr,
}
esQuery, _ := queryexpr.ValidateAndGetQueryFromExpr(deleteAssetESExpr)

Expand Down
32 changes: 6 additions & 26 deletions internal/store/postgres/asset_repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
sq "github.com/Masterminds/squirrel"
"github.com/goto/compass/core/asset"
"github.com/goto/compass/core/user"
generichelper "github.com/goto/compass/pkg/generic_helper"
queryexpr "github.com/goto/compass/pkg/query_expr"
"github.com/jmoiron/sqlx"
"github.com/r3labs/diff/v2"
Expand All @@ -29,27 +28,6 @@ type AssetRepository struct {
defaultUserProvider string
}

type DeleteAssetSQLExpr struct {
queryexpr.SQLExpr
}

func (d *DeleteAssetSQLExpr) Validate() error {
identifiers, err := queryexpr.GetIdentifiers(d.String())
if err != nil {
return err
}

mustExist := generichelper.Contains(identifiers, "refreshed_at") &&
generichelper.Contains(identifiers, "type") &&
generichelper.Contains(identifiers, "service")
if !mustExist {
return fmt.Errorf("must exist these identifiers: refreshed_at, type, and service. "+
"Current identifiers: %v", identifiers)
}

return nil
}

// GetAll retrieves list of assets with filters
func (r *AssetRepository) GetAll(ctx context.Context, flt asset.Filter) ([]asset.Asset, error) {
builder := r.getAssetSQL().Offset(uint64(flt.Offset))
Expand Down Expand Up @@ -142,8 +120,9 @@ func (r *AssetRepository) GetCount(ctx context.Context, flt asset.Filter) (int,
func (r *AssetRepository) GetCountByQueryExpr(ctx context.Context, queryExpr string, isDeleteExpr bool) (int, error) {
var sqlQuery string
if isDeleteExpr {
deleteExpr := &DeleteAssetSQLExpr{
queryexpr.SQLExpr(queryExpr),
expr := queryexpr.SQLExpr(queryExpr)
deleteExpr := &queryexpr.DeleteAssetExpr{
ExprStr: &expr,
}
query, err := queryexpr.ValidateAndGetQueryFromExpr(deleteExpr)
if err != nil {
Expand Down Expand Up @@ -424,8 +403,9 @@ func (r *AssetRepository) DeleteByURN(ctx context.Context, urn string) error {
func (r *AssetRepository) DeleteByQueryExpr(ctx context.Context, queryExpr string) ([]string, error) {
var allURNs []string
err := r.client.RunWithinTx(ctx, func(tx *sqlx.Tx) error {
deleteExpr := &DeleteAssetSQLExpr{
queryexpr.SQLExpr(queryExpr),
expr := queryexpr.SQLExpr(queryExpr)
deleteExpr := &queryexpr.DeleteAssetExpr{
ExprStr: &expr,
}
query, err := queryexpr.ValidateAndGetQueryFromExpr(deleteExpr)
if err != nil {
Expand Down
65 changes: 65 additions & 0 deletions pkg/generic_helper/generic_helper.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,19 @@
package generichelper

import (
"reflect"
)

// Contains checks if a target item exists in an array of any type.
//
// Example
//
// names := []string{"Alice", "Bob", "Carol"}
// result := Contains(names, "Bob")
//
// Result:
//
// true
func Contains[T comparable](arr []T, target T) bool {
for _, item := range arr {
if item == target {
Expand All @@ -9,3 +22,55 @@ func Contains[T comparable](arr []T, target T) bool {
}
return false
}

// GetJSONTags retrieves all JSON tag values from a struct.
// It returns a slice of JSON tag values extracted from the struct's fields.
//
// Example:
//
// type Person struct {
// ID int `json:"id"`
// Name string `json:"name"`
// Age int `json:"age"`
// CreatedAt string `json:"created_at"`
// }
//
// p := Person{}
// jsonTags := GetJSONTags(p)
//
// Result:
//
// ["id", "name", "age", "created_at"]
func GetJSONTags(v interface{}) []string {
var tags []string
val := reflect.ValueOf(v)
typ := val.Type()

for i := 0; i < typ.NumField(); i++ {
field := typ.Field(i)
jsonTag := field.Tag.Get("json")
if jsonTag != "" {
tags = append(tags, jsonTag)
}
}

return tags
}

// GetMapKeys is a generic function that extracts all keys from a map and returns them in a slice.
//
// Example:
//
// ageMap := map[string]int{"Alice": 30, "Bob": 25, "Carol": 27}
// keys := GetMapKeys(ageMap)
//
// Result:
//
// ["Alice", "Bob", "Carol"]
func GetMapKeys[K comparable, V any](m map[K]V) []K {
var keys []K
for key := range m {
keys = append(keys, key)
}
return keys
}
93 changes: 93 additions & 0 deletions pkg/generic_helper/generic_helper_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
package generichelper_test

import (
"reflect"
"testing"

generichelper "github.com/goto/compass/pkg/generic_helper"
)

func TestContains(t *testing.T) {
tests := []struct {
name string
arr []string
target string
expected bool
}{
{"Found", []string{"Alice", "Bob", "Carol"}, "Bob", true},
{"Not Found", []string{"Alice", "Bob", "Carol"}, "Dave", false},
{"Empty Array", []string{}, "Bob", false},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := generichelper.Contains(tt.arr, tt.target)
if result != tt.expected {
t.Errorf("Contains(%v, %v) = %v; want %v", tt.arr, tt.target, result, tt.expected)
}
})
}
}

func TestGetJSONTags(t *testing.T) {
type Person struct {
ID int `json:"id"`
Name string `json:"name"`
Age int `json:"age"`
CreatedAt string `json:"created_at"`
}

p := Person{}
expectedTags := []string{"id", "name", "age", "created_at"}

result := generichelper.GetJSONTags(p)
if !reflect.DeepEqual(result, expectedTags) {
t.Errorf("GetJSONTags(%v) = %v; want %v", p, result, expectedTags)
}
}

func TestGetMapKeys(t *testing.T) {
tests := []struct {
name string
input map[string]int
expected []string
}{
{"Simple Map", map[string]int{"Alice": 30, "Bob": 25, "Carol": 27}, []string{"Alice", "Bob", "Carol"}},
{"Empty Map", map[string]int{}, nil},
{"Single Element", map[string]int{"Alice": 30}, []string{"Alice"}},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := generichelper.GetMapKeys(tt.input)
if !CompareSlices(result, tt.expected) {
t.Errorf("GetMapKeys(%v) = %v; want %v", tt.input, result, tt.expected)
}
})
}
}

// CompareSlices checks if two slices contain the same elements, regardless of order.
func CompareSlices[T comparable](a, b []T) bool {
if a == nil && b == nil {
return true
}
if len(a) != len(b) {
return false
}

counts := make(map[T]int)

for _, item := range a {
counts[item]++
}

for _, item := range b {
if counts[item] == 0 {
return false
}
counts[item]--
}

return true
}
49 changes: 49 additions & 0 deletions pkg/query_expr/delete_asset_expr.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package queryexpr

import (
"fmt"

"github.com/goto/compass/core/asset"
generichelper "github.com/goto/compass/pkg/generic_helper"
)

type DeleteAssetExpr struct {
ExprStr
}

func (d DeleteAssetExpr) ToQuery() (string, error) {
return d.ExprStr.ToQuery()
}

func (d DeleteAssetExpr) Validate() error {
identifiersWithOperator, err := GetIdentifiersMap(d.ExprStr.String())
if err != nil {
return err
}

isExist := func(jsonTag string) bool {
return identifiersWithOperator[jsonTag] != ""
}
mustExist := isExist("refreshed_at") && isExist("type") && isExist("service")
if !mustExist {
return fmt.Errorf("must exists these identifiers: refreshed_at, type, and service")
}

getOperator := func(jsonTag string) string {
return identifiersWithOperator[jsonTag]
}
if getOperator("type") != "==" || getOperator("service") != "==" {
return fmt.Errorf("identifier type and service must be equals operator (==)")
}

identifiers := generichelper.GetMapKeys(identifiersWithOperator)
jsonTagsSchema := generichelper.GetJSONTags(asset.Asset{})
for _, identifier := range identifiers {
isFieldValid := generichelper.Contains(jsonTagsSchema, identifier)
if !isFieldValid {
return fmt.Errorf("%s is not a valid identifier", identifier)
}
}

return nil
}
1 change: 0 additions & 1 deletion pkg/query_expr/es_expr_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,5 @@ func TestESExpr_Validate(t *testing.T) {
if err := (&expr).Validate(); err != nil {
t.Errorf("Validate() error = %v, wantErr %v", err, nil)
}
return
})
}
28 changes: 0 additions & 28 deletions pkg/query_expr/es_test_data/equals-or-not-in-condition.json

This file was deleted.

Loading

0 comments on commit e4903d0

Please sign in to comment.