refactor: clean code as feedbacks

core/asset/delete_asset_expr.go

@@ -1,14 +1,19 @@
 package asset
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 
 	"github.com/goto/compass/pkg/generichelper"
 	"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
 }

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)
 	}()

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.

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"

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")

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
 		}

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,
 		},
 		{

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 {

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)
-			}
-		})
-	}
-}