From ed7b8efe6268092dbb068db0837972098bf8d050 Mon Sep 17 00:00:00 2001 From: Muhammad Luthfi Fahlevi Date: Wed, 21 Aug 2024 18:16:32 +0700 Subject: [PATCH] refactor: clean code as feedbacks --- core/asset/delete_asset_expr.go | 11 +++-- internal/server/v1beta1/asset.go | 2 +- internal/store/postgres/asset_repository.go | 10 +--- .../store/postgres/lineage_repository_test.go | 34 ++++++++++++++ pkg/generichelper/generic_helper.go | 6 +-- pkg/queryexpr/es_expr.go | 17 ++++--- pkg/queryexpr/es_expr_test.go | 7 +-- pkg/queryexpr/query_expr.go | 47 +++++++++++++------ pkg/queryexpr/query_expr_test.go | 40 ---------------- pkg/queryexpr/sql_expr.go | 6 +-- pkg/queryexpr/sql_expr_test.go | 4 +- 11 files changed, 95 insertions(+), 89 deletions(-) diff --git a/core/asset/delete_asset_expr.go b/core/asset/delete_asset_expr.go index 5b2b8259..b4477249 100644 --- a/core/asset/delete_asset_expr.go +++ b/core/asset/delete_asset_expr.go @@ -1,6 +1,7 @@ package asset import ( + "errors" "fmt" "strings" @@ -8,7 +9,11 @@ import ( "github.com/goto/compass/pkg/queryexpr" ) -var assetJSONTagsSchema = generichelper.GetJSONTags(Asset{}) +var ( + assetJSONTagsSchema = generichelper.GetJSONTags(Asset{}) + errTypeOrServiceHasWrongOperator = errors.New("identifier type and service must be equals (==) or IN operator") + errMissRequiredIdentifier = errors.New("must exists these identifiers: refreshed_at, type, and service") +) type DeleteAssetExpr struct { queryexpr.ExprStr @@ -41,7 +46,7 @@ func (DeleteAssetExpr) isRequiredIdentifiersExist(identifiersWithOperator map[st } mustExist := isExist("refreshed_at") && isExist("type") && isExist("service") if !mustExist { - return fmt.Errorf("must exists these identifiers: refreshed_at, type, and service") + return errMissRequiredIdentifier } return nil } @@ -51,7 +56,7 @@ func (DeleteAssetExpr) isUsingRightOperator(identifiersWithOperator map[string]s return identifiersWithOperator[jsonTag] == "==" || strings.ToUpper(identifiersWithOperator[jsonTag]) == "IN" } if !isOperatorEqualsOrIn("type") || !isOperatorEqualsOrIn("service") { - return fmt.Errorf("identifier type and service must be equals (==) or IN operator") + return errTypeOrServiceHasWrongOperator } return nil } diff --git a/internal/server/v1beta1/asset.go b/internal/server/v1beta1/asset.go index 32b89be0..5be91565 100644 --- a/internal/server/v1beta1/asset.go +++ b/internal/server/v1beta1/asset.go @@ -314,7 +314,7 @@ func (server *APIServer) DeleteAssets(ctx context.Context, req *compassv1beta1.D } defer func() { server.logger.Warn("delete assets by query", - "the number of affected rows is", affectedRows, + "affected rows", affectedRows, "query delete", req.QueryExpr, "dry run", req.DryRun) }() diff --git a/internal/store/postgres/asset_repository.go b/internal/store/postgres/asset_repository.go index 6090b925..17753cf3 100644 --- a/internal/store/postgres/asset_repository.go +++ b/internal/store/postgres/asset_repository.go @@ -6,7 +6,6 @@ import ( "encoding/json" "errors" "fmt" - "log" "strings" "time" @@ -18,8 +17,6 @@ import ( "github.com/r3labs/diff/v2" ) -const batchSize = 1000 - // AssetRepository is a type that manages user operation to the primary database type AssetRepository struct { client *Client @@ -395,18 +392,13 @@ func (r *AssetRepository) DeleteByQueryExpr(ctx context.Context, queryExpr query urns, err = r.deleteByQueryAndReturnURNS(ctx, query) if err != nil { - log.Printf("Failed to delete by query expr: %v", err) return err } return nil }) - if err != nil { - return nil, err - } - - return urns, nil + return urns, err } // deleteByQueryAndReturnURNS remove all assets that match to query and return array of urn of asset that deleted. diff --git a/internal/store/postgres/lineage_repository_test.go b/internal/store/postgres/lineage_repository_test.go index 271f7abb..52f282b2 100644 --- a/internal/store/postgres/lineage_repository_test.go +++ b/internal/store/postgres/lineage_repository_test.go @@ -119,6 +119,40 @@ func (r *LineageRepositoryTestSuite) TestDeleteByURN() { }) } +func (r *LineageRepositoryTestSuite) TestDeleteByURNs() { + r.Run("should delete assets from lineage", func() { + nodeURN1a := "table-1a" + nodeURN1b := "table-1b" + nodeURNs := []string{nodeURN1a, nodeURN1b} + + // create initial + err := r.repository.Upsert(r.ctx, nodeURN1a, []string{"table-2"}, []string{"table-3"}) + r.NoError(err) + err = r.repository.Upsert(r.ctx, nodeURN1b, []string{"table-2"}, []string{"table-3"}) + r.NoError(err) + + err = r.repository.DeleteByURNs(r.ctx, nodeURNs) + r.NoError(err) + + graph, err := r.repository.GetGraph(r.ctx, nodeURN1a, asset.LineageQuery{}) + r.Require().NoError(err) + r.compareGraphs(asset.LineageGraph{}, graph) + + graph, err = r.repository.GetGraph(r.ctx, nodeURN1b, asset.LineageQuery{}) + r.Require().NoError(err) + r.compareGraphs(asset.LineageGraph{}, graph) + }) + + r.Run("delete when URNs has no lineage", func() { + nodeURN1a := "table-1a" + nodeURN1b := "table-1b" + nodeURNs := []string{nodeURN1a, nodeURN1b} + + err := r.repository.DeleteByURNs(r.ctx, nodeURNs) + r.NoError(err) + }) +} + func (r *LineageRepositoryTestSuite) TestUpsert() { r.Run("should insert all as graph if upstreams and downstreams are new", func() { nodeURN := "table-1" diff --git a/pkg/generichelper/generic_helper.go b/pkg/generichelper/generic_helper.go index 768a0006..58d83b9a 100644 --- a/pkg/generichelper/generic_helper.go +++ b/pkg/generichelper/generic_helper.go @@ -1,12 +1,10 @@ package generichelper -import ( - "reflect" -) +import "reflect" // Contains checks if a target item exists in an array of any type. // -// Example +// Example: // // names := []string{"Alice", "Bob", "Carol"} // result := Contains(names, "Bob") diff --git a/pkg/queryexpr/es_expr.go b/pkg/queryexpr/es_expr.go index 9c0d804c..c177cf77 100644 --- a/pkg/queryexpr/es_expr.go +++ b/pkg/queryexpr/es_expr.go @@ -2,7 +2,6 @@ package queryexpr import ( "encoding/json" - "errors" "fmt" "github.com/expr-lang/expr/ast" @@ -29,7 +28,7 @@ func (e ESExpr) ToQuery() (string, error) { } esQuery, ok := esQueryInterface.(map[string]interface{}) if !ok { - return "", errors.New("failed to generate Elasticsearch query") + return "", errFailedGenerateESQuery } esQuery = map[string]interface{}{"query": esQuery} @@ -50,7 +49,7 @@ func (ESExpr) Validate() error { // TODO: implement translator for node type that still not covered right now. func (e ESExpr) translateToEsQuery(node ast.Node) (interface{}, error) { if node == nil { - return nil, fmt.Errorf("cannot convert nil to Elasticsearch query") + return nil, errCannotConvertNilQuery } switch n := (node).(type) { case *ast.BinaryNode: @@ -77,7 +76,7 @@ func (e ESExpr) translateToEsQuery(node ast.Node) (interface{}, error) { case *ast.ConstantNode: return n.Value, nil case *ast.BuiltinNode, *ast.ConditionalNode: - result, err := GetQueryExprResult(n.String()) + result, err := getQueryExprResult(n.String()) if err != nil { return nil, err } @@ -108,7 +107,7 @@ func (e ESExpr) binaryNodeToEsQuery(n *ast.BinaryNode) (interface{}, error) { // if leftStr, ok := left.(string); ok { return e.termQuery(leftStr, right), nil } - result, err := GetQueryExprResult(n.String()) + result, err := getQueryExprResult(n.String()) if err != nil { return nil, err } @@ -118,7 +117,7 @@ func (e ESExpr) binaryNodeToEsQuery(n *ast.BinaryNode) (interface{}, error) { // if leftStr, ok := left.(string); ok { return e.mustNotQuery(leftStr, right), nil } - result, err := GetQueryExprResult(n.String()) + result, err := getQueryExprResult(n.String()) if err != nil { return nil, err } @@ -128,7 +127,7 @@ func (e ESExpr) binaryNodeToEsQuery(n *ast.BinaryNode) (interface{}, error) { // if leftStr, ok := left.(string); ok { return e.rangeQuery(leftStr, e.operatorToEsQuery(n.Operator), right), nil } - result, err := GetQueryExprResult(n.String()) + result, err := getQueryExprResult(n.String()) if err != nil { return nil, err } @@ -138,14 +137,14 @@ func (e ESExpr) binaryNodeToEsQuery(n *ast.BinaryNode) (interface{}, error) { // if leftStr, ok := left.(string); ok { return e.termsQuery(leftStr, right), nil } - result, err := GetQueryExprResult(n.String()) + result, err := getQueryExprResult(n.String()) if err != nil { return nil, err } return result, nil default: - result, err := GetQueryExprResult(n.String()) + result, err := getQueryExprResult(n.String()) if err != nil { return nil, err } diff --git a/pkg/queryexpr/es_expr_test.go b/pkg/queryexpr/es_expr_test.go index 67209952..b8a7ad37 100644 --- a/pkg/queryexpr/es_expr_test.go +++ b/pkg/queryexpr/es_expr_test.go @@ -69,9 +69,10 @@ func TestESExpr_ToQuery(t *testing.T) { wantErr: false, }, { - name: "complex query expression that can directly produce a value regarding time", - expr: queryexpr.ESExpr(`refreshed_at <= (now() - duration('1h'))`), - want: fmt.Sprintf(`{"query":{"range":{"refreshed_at":{"lte":"%v"}}}}`, time.Now().Add(-1*time.Hour).Format(time.RFC3339)), + name: "complex query expression that can directly produce a value regarding time", + expr: queryexpr.ESExpr(`refreshed_at <= (date("2024-08-21T01:00:00Z") - duration('1h'))`), + want: fmt.Sprintf(`{"query":{"range":{"refreshed_at":{"lte":"%v"}}}}`, + time.Date(2024, 8, 21, 0, 0, 0, 0, time.UTC).Format(time.RFC3339)), wantErr: false, }, { diff --git a/pkg/queryexpr/query_expr.go b/pkg/queryexpr/query_expr.go index 879c061a..c8e9975b 100644 --- a/pkg/queryexpr/query_expr.go +++ b/pkg/queryexpr/query_expr.go @@ -1,6 +1,7 @@ package queryexpr import ( + "errors" "fmt" "strings" "time" @@ -10,17 +11,22 @@ import ( "github.com/expr-lang/expr/parser" ) +var ( + errFailedGenerateESQuery = errors.New("failed to generate Elasticsearch query") + errCannotConvertNilQuery = errors.New("cannot convert nil to query") +) + type ExprStr interface { String() string ToQuery() (string, error) Validate() error } -type ExprVisitor struct { - IdentifiersWithOperator map[string]string // Key: Identifier, Value: Operator +type exprVisitor struct { + identifiersWithOperator map[string]string // Key: Identifier, Value: Operator } -type ExprParam map[string]interface{} +type exprParam map[string]interface{} func ValidateAndGetQueryFromExpr(exprStr ExprStr) (string, error) { if err := exprStr.Validate(); err != nil { @@ -35,24 +41,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 +func (s *exprVisitor) Visit(node *ast.Node) { //nolint:gocritic switch n := (*node).(type) { case *ast.BinaryNode: if left, ok := (n.Left).(*ast.IdentifierNode); ok { - s.IdentifiersWithOperator[left.Value] = n.Operator + s.identifiersWithOperator[left.Value] = n.Operator } if right, ok := (n.Right).(*ast.IdentifierNode); ok { - s.IdentifiersWithOperator[right.Value] = n.Operator + s.identifiersWithOperator[right.Value] = n.Operator } case *ast.UnaryNode: if binaryNode, ok := (n.Node).(*ast.BinaryNode); ok { if strings.ToUpper(binaryNode.Operator) == "IN" { notInOperator := "NOT IN" if left, ok := (binaryNode.Left).(*ast.IdentifierNode); ok { - s.IdentifiersWithOperator[left.Value] = notInOperator + s.identifiersWithOperator[left.Value] = notInOperator } if right, ok := (binaryNode.Right).(*ast.IdentifierNode); ok { - s.IdentifiersWithOperator[right.Value] = notInOperator + s.identifiersWithOperator[right.Value] = notInOperator } } } @@ -64,9 +70,9 @@ func GetIdentifiersMap(queryExpr string) (map[string]string, error) { if err != nil { return nil, err } - queryExprVisitor := &ExprVisitor{IdentifiersWithOperator: make(map[string]string)} + queryExprVisitor := &exprVisitor{identifiersWithOperator: make(map[string]string)} ast.Walk(&queryExprParsed, queryExprVisitor) - return queryExprVisitor.IdentifiersWithOperator, nil + return queryExprVisitor.identifiersWithOperator, nil } func getTreeNodeFromQueryExpr(queryExpr string) (ast.Node, error) { @@ -78,16 +84,27 @@ func getTreeNodeFromQueryExpr(queryExpr string) (ast.Node, error) { return parsed.Node, nil } -func GetQueryExprResult(fn string) (any, error) { - env := make(ExprParam) - compile, err := expr.Compile(fn) +// getQueryExprResult used for getting the result of query expr operation. +// The playground can be accessed at https://expr-lang.org/playground +// +// Example: +// +// queryExprOperation := findLast([1, 2, 3, 4], # > 2) +// result := getQueryExprResult(queryExprOperation) +// +// Result: +// +// 4 +func getQueryExprResult(queryExprOperation string) (any, error) { + env := make(exprParam) + compile, err := expr.Compile(queryExprOperation) if err != nil { - return nil, fmt.Errorf("failed to compile function '%s': %w", fn, err) + return nil, fmt.Errorf("failed to compile function '%s': %w", queryExprOperation, err) } result, err := expr.Run(compile, env) if err != nil { - return nil, fmt.Errorf("failed to evaluate function '%s': %w", fn, err) + return nil, fmt.Errorf("failed to evaluate function '%s': %w", queryExprOperation, err) } if t, ok := result.(time.Time); ok { diff --git a/pkg/queryexpr/query_expr_test.go b/pkg/queryexpr/query_expr_test.go index 02c36cfc..0e40d83f 100644 --- a/pkg/queryexpr/query_expr_test.go +++ b/pkg/queryexpr/query_expr_test.go @@ -58,43 +58,3 @@ func TestGetIdentifiersMap(t *testing.T) { }) } } - -func TestGetQueryExprResult(t *testing.T) { - tests := []struct { - name string - expr string - want any - wantErr bool - }{ - { - name: "return a value from func", - expr: `findLast([1, 2, 3, 4], # > 2)`, - want: 4, - wantErr: false, - }, - { - name: "return a value func equation", - expr: `false == !true`, - want: true, - wantErr: false, - }, - { - name: "got error due to can NOT directly produce a value", - expr: `updated_at < '2024-04-05 23:59:59'`, - want: nil, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := queryexpr.GetQueryExprResult(tt.expr) - if (err != nil) != tt.wantErr { - t.Errorf("GetQueryExprResult() error = %v, wantErr %v", err, tt.wantErr) - return - } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("GetQueryExprResult() got = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/pkg/queryexpr/sql_expr.go b/pkg/queryexpr/sql_expr.go index 8b1d5fe0..0418f865 100644 --- a/pkg/queryexpr/sql_expr.go +++ b/pkg/queryexpr/sql_expr.go @@ -36,7 +36,7 @@ func (SQLExpr) Validate() error { // TODO: implement translator for node type that still not covered right now. func (s SQLExpr) convertToSQL(node ast.Node, stringBuilder *strings.Builder) error { if node == nil { - return fmt.Errorf("cannot convert nil to SQL query") + return errCannotConvertNilQuery } switch n := (node).(type) { case *ast.BinaryNode: @@ -106,7 +106,7 @@ func (s SQLExpr) binaryNodeToSQLQuery(n *ast.BinaryNode, stringBuilder *strings. } func (SQLExpr) getQueryExprResult(fn string, stringBuilder *strings.Builder) error { - result, err := GetQueryExprResult(fn) + result, err := getQueryExprResult(fn) if err != nil { return err } @@ -153,7 +153,7 @@ func (s SQLExpr) patchUnaryNode(n *ast.UnaryNode) error { Value: !nodeV.Value, }) default: - result, err := GetQueryExprResult(n.String()) + result, err := getQueryExprResult(n.String()) if err != nil { return err } diff --git a/pkg/queryexpr/sql_expr_test.go b/pkg/queryexpr/sql_expr_test.go index 96cee837..2ca2e7a9 100644 --- a/pkg/queryexpr/sql_expr_test.go +++ b/pkg/queryexpr/sql_expr_test.go @@ -70,8 +70,8 @@ func TestSQLExpr_ToQuery(t *testing.T) { }, { name: "complex query expression that can directly produce a value regarding time", - expr: queryexpr.SQLExpr(`refreshed_at <= (now() - duration('1h'))`), - want: fmt.Sprintf("(refreshed_at <= '%s')", time.Now().Add(-1*time.Hour).Format(time.RFC3339)), + expr: queryexpr.SQLExpr(`refreshed_at <= (date("2024-08-21T01:00:00Z") - duration('1h'))`), + want: fmt.Sprintf("(refreshed_at <= '%s')", time.Date(2024, 8, 21, 0, 0, 0, 0, time.UTC).Format(time.RFC3339)), wantErr: false, }, {