diff --git a/internal/server/v1beta1/asset.go b/internal/server/v1beta1/asset.go index 93c2acac..2abf5a31 100644 --- a/internal/server/v1beta1/asset.go +++ b/internal/server/v1beta1/asset.go @@ -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()) } diff --git a/internal/store/elasticsearch/discovery_repository.go b/internal/store/elasticsearch/discovery_repository.go index 9acb1ce2..0cc0362f 100644 --- a/internal/store/elasticsearch/discovery_repository.go +++ b/internal/store/elasticsearch/discovery_repository.go @@ -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" ) @@ -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, @@ -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 { diff --git a/internal/store/elasticsearch/discovery_repository_test.go b/internal/store/elasticsearch/discovery_repository_test.go index ab4ca9cb..dacd0101 100644 --- a/internal/store/elasticsearch/discovery_repository_test.go +++ b/internal/store/elasticsearch/discovery_repository_test.go @@ -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) diff --git a/internal/store/postgres/asset_repository.go b/internal/store/postgres/asset_repository.go index ff6324f6..816f85c5 100644 --- a/internal/store/postgres/asset_repository.go +++ b/internal/store/postgres/asset_repository.go @@ -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" @@ -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)) @@ -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 { @@ -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 { diff --git a/pkg/generic_helper/generic_helper.go b/pkg/generic_helper/generic_helper.go index 178f1114..768a0006 100644 --- a/pkg/generic_helper/generic_helper.go +++ b/pkg/generic_helper/generic_helper.go @@ -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 { @@ -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 +} diff --git a/pkg/generic_helper/generic_helper_test.go b/pkg/generic_helper/generic_helper_test.go new file mode 100644 index 00000000..f8004b0c --- /dev/null +++ b/pkg/generic_helper/generic_helper_test.go @@ -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 +} diff --git a/pkg/query_expr/delete_asset_expr.go b/pkg/query_expr/delete_asset_expr.go new file mode 100644 index 00000000..ec224628 --- /dev/null +++ b/pkg/query_expr/delete_asset_expr.go @@ -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 +} diff --git a/pkg/query_expr/es_expr_test.go b/pkg/query_expr/es_expr_test.go index 93a2a75f..272c31c6 100644 --- a/pkg/query_expr/es_expr_test.go +++ b/pkg/query_expr/es_expr_test.go @@ -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 }) } diff --git a/pkg/query_expr/es_test_data/equals-or-not-in-condition.json b/pkg/query_expr/es_test_data/equals-or-not-in-condition.json deleted file mode 100644 index 6942cafb..00000000 --- a/pkg/query_expr/es_test_data/equals-or-not-in-condition.json +++ /dev/null @@ -1,28 +0,0 @@ -{ - "query": { - "bool": { - "should": [ - { - "term": { - "name": "John" - } - }, - { - "bool": { - "must_not": [ - { - "terms": { - "service": [ - "test1", - "test2", - "test3" - ] - } - } - ] - } - } - ] - } - } -} \ No newline at end of file diff --git a/pkg/query_expr/es_test_data/in-condition.json b/pkg/query_expr/es_test_data/in-condition.json deleted file mode 100644 index 33fd2db7..00000000 --- a/pkg/query_expr/es_test_data/in-condition.json +++ /dev/null @@ -1,11 +0,0 @@ -{ - "query": { - "terms": { - "service": [ - "test1", - "test2", - "test3" - ] - } - } -} \ No newline at end of file diff --git a/pkg/query_expr/es_test_data/lt-condition.json b/pkg/query_expr/es_test_data/lt-condition.json deleted file mode 100644 index d29da73a..00000000 --- a/pkg/query_expr/es_test_data/lt-condition.json +++ /dev/null @@ -1,9 +0,0 @@ -{ - "query": { - "range": { - "updated_at": { - "lt": "2024-04-05 23:59:59" - } - } - } -} \ No newline at end of file diff --git a/pkg/query_expr/query_expr.go b/pkg/query_expr/query_expr.go index cbddcbfd..bb471c05 100644 --- a/pkg/query_expr/query_expr.go +++ b/pkg/query_expr/query_expr.go @@ -9,12 +9,13 @@ import ( ) type ExprStr interface { + String() string ToQuery() (string, error) Validate() error } type ExprVisitor struct { - Identifiers []string + IdentifiersWithOperator map[string]string // Key: Identifier, Value: Operator } type ExprParam map[string]interface{} @@ -33,19 +34,24 @@ func ValidateAndGetQueryFromExpr(exprStr ExprStr) (string, error) { // Visit is implementation Visitor interface from expr-lang/expr lib, used by ast.Walk func (s *ExprVisitor) Visit(node *ast.Node) { //nolint:gocritic - if n, ok := (*node).(*ast.IdentifierNode); ok { - s.Identifiers = append(s.Identifiers, n.Value) + if n, ok := (*node).(*ast.BinaryNode); ok { + if left, ok := (n.Left).(*ast.IdentifierNode); ok { + s.IdentifiersWithOperator[left.Value] = n.Operator + } + if right, ok := (n.Right).(*ast.IdentifierNode); ok { + s.IdentifiersWithOperator[right.Value] = n.Operator + } } } -func GetIdentifiers(queryExpr string) ([]string, error) { +func GetIdentifiersMap(queryExpr string) (map[string]string, error) { queryExprParsed, err := GetTreeNodeFromQueryExpr(queryExpr) if err != nil { return nil, err } - queryExprVisitor := &ExprVisitor{} + queryExprVisitor := &ExprVisitor{IdentifiersWithOperator: make(map[string]string)} ast.Walk(&queryExprParsed, queryExprVisitor) - return queryExprVisitor.Identifiers, nil + return queryExprVisitor.IdentifiersWithOperator, nil } func GetTreeNodeFromQueryExpr(queryExpr string) (ast.Node, error) { diff --git a/pkg/query_expr/query_expr_test.go b/pkg/query_expr/query_expr_test.go index 748628fa..f7d21599 100644 --- a/pkg/query_expr/query_expr_test.go +++ b/pkg/query_expr/query_expr_test.go @@ -1,33 +1,41 @@ -package queryexpr +package queryexpr_test import ( "reflect" "testing" + + queryexpr "github.com/goto/compass/pkg/query_expr" ) -func TestGetIdentifiers(t *testing.T) { +func TestGetIdentifiersMap(t *testing.T) { tests := []struct { name string expr string - want []string + want map[string]string wantErr bool }{ { name: "got 0 identifier test", expr: `findLast([1, 2, 3, 4], # > 2)`, - want: nil, + want: map[string]string{}, wantErr: false, }, { - name: "got 1 identifiers test", - expr: `updated_at < '2024-04-05 23:59:59'`, - want: []string{"updated_at"}, + name: "got 1 identifiers test", + expr: `updated_at < '2024-04-05 23:59:59'`, + want: map[string]string{ + "updated_at": "<", + }, wantErr: false, }, { - name: "got 3 identifiers test", - expr: `(identifier1 == !(findLast([1, 2, 3, 4], # > 2) == 4)) && identifier2 != 'John' || identifier3 == "hallo"`, - want: []string{"identifier1", "identifier2", "identifier3"}, + name: "got 3 identifiers test", + expr: `(identifier1 == !(findLast([1, 2, 3, 4], # > 2) == 4)) && identifier2 != 'John' || identifier3 == "hallo"`, + want: map[string]string{ + "identifier1": "==", + "identifier2": "!=", + "identifier3": "==", + }, wantErr: false, }, { @@ -39,13 +47,13 @@ func TestGetIdentifiers(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := GetIdentifiers(tt.expr) + got, err := queryexpr.GetIdentifiersMap(tt.expr) if (err != nil) != tt.wantErr { - t.Errorf("GetIdentifiers() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("GetIdentifiersMap() error = %v, wantErr %v", err, tt.wantErr) return } if !reflect.DeepEqual(got, tt.want) { - t.Errorf("GetIdentifiers() got = %v, want %v", got, tt.want) + t.Errorf("GetIdentifiersMap() got = %v, want %v", got, tt.want) } }) } @@ -79,7 +87,7 @@ func TestGetQueryExprResult(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := GetQueryExprResult(tt.expr) + got, err := queryexpr.GetQueryExprResult(tt.expr) if (err != nil) != tt.wantErr { t.Errorf("GetQueryExprResult() error = %v, wantErr %v", err, tt.wantErr) return @@ -119,7 +127,7 @@ func TestGetTreeNodeFromQueryExpr(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := GetTreeNodeFromQueryExpr(tt.expr) + got, err := queryexpr.GetTreeNodeFromQueryExpr(tt.expr) if (err != nil) != tt.wantErr { t.Errorf("GetTreeNodeFromQueryExpr() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/pkg/query_expr/sql_expr.go b/pkg/query_expr/sql_expr.go index ec5e3c52..6fecc196 100644 --- a/pkg/query_expr/sql_expr.go +++ b/pkg/query_expr/sql_expr.go @@ -127,14 +127,15 @@ func (s *SQLExpr) patchUnaryNode(n *ast.UnaryNode) error { switch n.Operator { case "not": binaryNode, ok := (n.Node).(*ast.BinaryNode) - if ok && strings.ToUpper(binaryNode.Operator) == "IN" { + if !ok { + return s.unsupportedQueryError(n) + } + if strings.ToUpper(binaryNode.Operator) == "IN" { ast.Patch(&n.Node, &ast.BinaryNode{ Operator: "not in", Left: binaryNode.Left, Right: binaryNode.Right, }) - } else { - return s.unsupportedQueryError(n) } case "!": switch nodeV := n.Node.(type) { diff --git a/pkg/query_expr/sql_expr_test.go b/pkg/query_expr/sql_expr_test.go index fa4eb52e..16728676 100644 --- a/pkg/query_expr/sql_expr_test.go +++ b/pkg/query_expr/sql_expr_test.go @@ -93,6 +93,5 @@ func TestSQLExpr_Validate(t *testing.T) { if err := (&expr).Validate(); err != nil { t.Errorf("Validate() error = %v, wantErr %v", err, nil) } - return }) }