From 61835a50b93ee79a377928603a9d69a3b4d778f8 Mon Sep 17 00:00:00 2001 From: kakcy Date: Thu, 12 Dec 2024 18:24:52 +0900 Subject: [PATCH 1/5] refactor: add ListV2 for AutoOps and ProgressiveRollout --- pkg/autoops/api/api.go | 39 +++-- pkg/autoops/api/progressive_rollout.go | 92 ++++++++++- pkg/autoops/storage/v2/auto_ops_rule.go | 56 +++++++ pkg/autoops/storage/v2/auto_ops_rule_test.go | 73 +++++++++ pkg/autoops/storage/v2/progressive_rollout.go | 61 +++++++ .../storage/v2/progressive_rollout_test.go | 154 ++++++++++++++++++ pkg/storage/v2/mysql/query.go | 110 ++++++++++++- 7 files changed, 572 insertions(+), 13 deletions(-) diff --git a/pkg/autoops/api/api.go b/pkg/autoops/api/api.go index f306acdf5e..c1e6fe2647 100644 --- a/pkg/autoops/api/api.go +++ b/pkg/autoops/api/api.go @@ -1112,16 +1112,28 @@ func (s *AutoOpsService) listAutoOpsRules( localizer locale.Localizer, storage v2as.AutoOpsRuleStorage, ) ([]*autoopsproto.AutoOpsRule, string, error) { - whereParts := []mysql.WherePart{ - mysql.NewFilter("deleted", "=", false), - mysql.NewFilter("environment_id", "=", environmentId), + filters := []*mysql.FilterV2{ + { + Column: "deleted", + Operator: mysql.OperatorEqual, + Value: false, + }, + { + Column: "environment_id", + Operator: mysql.OperatorEqual, + Value: environmentId, + }, } fIDs := make([]interface{}, 0, len(featureIds)) for _, fID := range featureIds { fIDs = append(fIDs, fID) } + var infilter *mysql.InFilter if len(fIDs) > 0 { - whereParts = append(whereParts, mysql.NewInFilter("feature_id", fIDs)) + infilter = &mysql.InFilter{ + Column: "feature_id", + Values: fIDs, + } } limit := int(pageSize) if cursor == "" { @@ -1139,13 +1151,18 @@ func (s *AutoOpsService) listAutoOpsRules( return nil, "", dt.Err() } - autoOpsRules, nextCursor, err := storage.ListAutoOpsRules( - ctx, - whereParts, - nil, - limit, - offset, - ) + listOptions := &mysql.ListOptions{ + Limit: limit, + Offset: offset, + Filters: filters, + InFilter: infilter, + NullFilters: nil, + JSONFilters: nil, + SearchQuery: nil, + Orders: nil, + } + + autoOpsRules, nextCursor, err := storage.ListAutoOpsRulesV2(ctx, listOptions) if err != nil { s.logger.Error( "Failed to list autoOpsRules", diff --git a/pkg/autoops/api/progressive_rollout.go b/pkg/autoops/api/progressive_rollout.go index 46ade364fe..a3d0b1fc92 100644 --- a/pkg/autoops/api/progressive_rollout.go +++ b/pkg/autoops/api/progressive_rollout.go @@ -373,7 +373,7 @@ func (s *AutoOpsService) ListProgressiveRollouts( if err != nil { return nil, err } - progressiveRollout, totalCount, nextOffset, err := s.listProgressiveRollouts( + progressiveRollout, totalCount, nextOffset, err := s.listProgressiveRolloutsV2( ctx, req, localizer, @@ -681,6 +681,96 @@ func (s *AutoOpsService) listProgressiveRollouts( return progressiveRollouts, totalCount, nextOffset, nil } +func (s *AutoOpsService) listProgressiveRolloutsV2( + ctx context.Context, + req *autoopsproto.ListProgressiveRolloutsRequest, + localizer locale.Localizer, +) ([]*autoopsproto.ProgressiveRollout, int64, int, error) { + filters := []*mysql.FilterV2{ + { + Column: "environment_id", + Operator: mysql.OperatorEqual, + Value: req.EnvironmentId, + }, + } + limit := int(req.PageSize) + cursor := req.Cursor + if cursor == "" { + cursor = "0" + } + offset, err := strconv.Atoi(cursor) + if err != nil { + dt, err := statusProgressiveRolloutInvalidCursor.WithDetails(&errdetails.LocalizedMessage{ + Locale: localizer.GetLocale(), + Message: localizer.MustLocalizeWithTemplate(locale.InvalidArgumentError, "cursor"), + }) + if err != nil { + return nil, 0, 0, statusProgressiveRolloutInternal.Err() + } + return nil, 0, 0, dt.Err() + } + var inFilter *mysql.InFilter = nil + if len(req.FeatureIds) > 0 { + fIDs := s.convToInterfaceSlice(req.FeatureIds) + inFilter = &mysql.InFilter{ + Column: "feature_id", + Values: fIDs, + } + } + orders, err := s.newListProgressiveRolloutsOrdersMySQL( + req.OrderBy, + req.OrderDirection, + localizer, + ) + if err != nil { + s.logger.Error( + "Invalid argument", + log.FieldsFromImcomingContext(ctx).AddFields( + zap.Error(err), + zap.String("environmentId", req.EnvironmentId), + )..., + ) + return nil, 0, 0, err + } + if req.Type != nil { + filters = append(filters, &mysql.FilterV2{Column: "type", Operator: mysql.OperatorEqual, Value: req.Type}) + } + if req.Status != nil { + filters = append(filters, &mysql.FilterV2{Column: "status", Operator: mysql.OperatorEqual, Value: req.Status}) + } + listOptions := &mysql.ListOptions{ + Filters: filters, + Orders: orders, + InFilter: inFilter, + NullFilters: nil, + JSONFilters: nil, + SearchQuery: nil, + Limit: limit, + Offset: offset, + } + + storage := v2as.NewProgressiveRolloutStorage(s.mysqlClient) + progressiveRollouts, totalCount, nextOffset, err := storage.ListProgressiveRolloutsV2(ctx, listOptions) + if err != nil { + s.logger.Error( + "Failed to list progressive rollouts", + log.FieldsFromImcomingContext(ctx).AddFields( + zap.Error(err), + zap.String("environmentId", req.EnvironmentId), + )..., + ) + dt, err := statusProgressiveRolloutInternal.WithDetails(&errdetails.LocalizedMessage{ + Locale: localizer.GetLocale(), + Message: localizer.MustLocalize(locale.InternalServerError), + }) + if err != nil { + return nil, 0, 0, statusProgressiveRolloutInternal.Err() + } + return nil, 0, 0, dt.Err() + } + return progressiveRollouts, totalCount, nextOffset, nil +} + func (s *AutoOpsService) newListProgressiveRolloutsOrdersMySQL( orderBy autoopsproto.ListProgressiveRolloutsRequest_OrderBy, orderDirection autoopsproto.ListProgressiveRolloutsRequest_OrderDirection, diff --git a/pkg/autoops/storage/v2/auto_ops_rule.go b/pkg/autoops/storage/v2/auto_ops_rule.go index 4097a495bd..24dc7c1290 100644 --- a/pkg/autoops/storage/v2/auto_ops_rule.go +++ b/pkg/autoops/storage/v2/auto_ops_rule.go @@ -53,6 +53,10 @@ type AutoOpsRuleStorage interface { orders []*mysql.Order, limit, offset int, ) ([]*proto.AutoOpsRule, int, error) + ListAutoOpsRulesV2( + ctx context.Context, + options *mysql.ListOptions, + ) ([]*proto.AutoOpsRule, int, error) } type autoOpsRuleStorage struct { @@ -193,3 +197,55 @@ func (s *autoOpsRuleStorage) ListAutoOpsRules( nextOffset := offset + len(autoOpsRules) return autoOpsRules, nextOffset, nil } + +func (s *autoOpsRuleStorage) ListAutoOpsRulesV2( + ctx context.Context, + options *mysql.ListOptions, +) ([]*proto.AutoOpsRule, int, error) { + println("kaki ListAutoOpsRulesV2") + var whereParts []mysql.WherePart = []mysql.WherePart{} + var orderBySQL string = "" + var limitOffsetSQL string = "" + var limit int = 0 + var offset int = 0 + if options != nil { + whereParts = options.CreateWhereParts() + orderBySQL = mysql.ConstructOrderBySQLString(options.Orders) + limitOffsetSQL = mysql.ConstructLimitOffsetSQLString(options.Limit, options.Offset) + limit = options.Limit + offset = options.Offset + } + + whereSQL, whereArgs := mysql.ConstructWhereSQLString(whereParts) + query := fmt.Sprintf(selectAutoOpsRulesSQL, whereSQL, orderBySQL, limitOffsetSQL) + rows, err := s.qe.QueryContext(ctx, query, whereArgs...) + if err != nil { + return nil, 0, err + } + defer rows.Close() + autoOpsRules := make([]*proto.AutoOpsRule, 0, limit) + for rows.Next() { + autoOpsRule := proto.AutoOpsRule{} + var opsType int32 + err := rows.Scan( + &autoOpsRule.Id, + &autoOpsRule.FeatureId, + &opsType, + &mysql.JSONObject{Val: &autoOpsRule.Clauses}, + &autoOpsRule.CreatedAt, + &autoOpsRule.UpdatedAt, + &autoOpsRule.Deleted, + &autoOpsRule.AutoOpsStatus, + ) + if err != nil { + return nil, 0, err + } + autoOpsRule.OpsType = proto.OpsType(opsType) + autoOpsRules = append(autoOpsRules, &autoOpsRule) + } + if rows.Err() != nil { + return nil, 0, err + } + nextOffset := offset + len(autoOpsRules) + return autoOpsRules, nextOffset, nil +} diff --git a/pkg/autoops/storage/v2/auto_ops_rule_test.go b/pkg/autoops/storage/v2/auto_ops_rule_test.go index 939c808a41..7919721b47 100644 --- a/pkg/autoops/storage/v2/auto_ops_rule_test.go +++ b/pkg/autoops/storage/v2/auto_ops_rule_test.go @@ -252,6 +252,79 @@ func TestListAutoOpsRules(t *testing.T) { } } +func TestListAutoOpsRulesV2(t *testing.T) { + t.Parallel() + mockController := gomock.NewController(t) + defer mockController.Finish() + patterns := []struct { + setup func(*autoOpsRuleStorage) + listOpts *mysql.ListOptions + expected []*proto.AutoOpsRule + expectedCursor int + expectedErr error + }{ + { + setup: func(s *autoOpsRuleStorage) { + s.qe.(*mock.MockQueryExecer).EXPECT().QueryContext( + gomock.Any(), gomock.Any(), gomock.Any(), + ).Return(nil, errors.New("error")) + }, + listOpts: nil, + expected: nil, + expectedCursor: 0, + expectedErr: errors.New("error"), + }, + { + setup: func(s *autoOpsRuleStorage) { + rows := mock.NewMockRows(mockController) + rows.EXPECT().Close().Return(nil) + rows.EXPECT().Next().Return(false) + rows.EXPECT().Err().Return(nil) + s.qe.(*mock.MockQueryExecer).EXPECT().QueryContext( + gomock.Any(), gomock.Regex(`^SELECT\s+id,\s+feature_id,\s+ops_type,\s+clauses,\s+created_at,\s+updated_at,\s+deleted,\s+status\s+FROM\s+auto_ops_rule\s+WHERE\s+num\s+>=\s+\?\s+ORDER\s+BY\s+id\s+ASC\s+LIMIT\s+10\s+OFFSET\s+5`), gomock.Any(), + ).Return(rows, nil) + }, + listOpts: &mysql.ListOptions{ + Limit: 10, + Offset: 5, + Filters: []*mysql.FilterV2{ + { + Column: "num", + Operator: mysql.OperatorGreaterThanOrEqual, + Value: 5, + }, + }, + InFilter: nil, + NullFilters: nil, + JSONFilters: nil, + SearchQuery: nil, + Orders: []*mysql.Order{ + { + Column: "id", + Direction: mysql.OrderDirectionAsc, + }, + }, + }, + expected: []*proto.AutoOpsRule{}, + expectedCursor: 5, + expectedErr: nil, + }, + } + for _, p := range patterns { + storage := newAutoOpsRuleStorageWithMock(t, mockController) + if p.setup != nil { + p.setup(storage) + } + autoOpsRules, cursor, err := storage.ListAutoOpsRulesV2( + context.Background(), + p.listOpts, + ) + assert.Equal(t, p.expected, autoOpsRules) + assert.Equal(t, p.expectedCursor, cursor) + assert.Equal(t, p.expectedErr, err) + } +} + func newAutoOpsRuleStorageWithMock(t *testing.T, mockController *gomock.Controller) *autoOpsRuleStorage { t.Helper() return &autoOpsRuleStorage{mock.NewMockQueryExecer(mockController)} diff --git a/pkg/autoops/storage/v2/progressive_rollout.go b/pkg/autoops/storage/v2/progressive_rollout.go index 7864c2b57c..cd3e567958 100644 --- a/pkg/autoops/storage/v2/progressive_rollout.go +++ b/pkg/autoops/storage/v2/progressive_rollout.go @@ -65,6 +65,10 @@ type ProgressiveRolloutStorage interface { orders []*mysql.Order, limit, offset int, ) ([]*autoopsproto.ProgressiveRollout, int64, int, error) + ListProgressiveRolloutsV2( + ctx context.Context, + options *mysql.ListOptions, + ) ([]*autoopsproto.ProgressiveRollout, int64, int, error) UpdateProgressiveRollout(ctx context.Context, progressiveRollout *domain.ProgressiveRollout, environmentId string, @@ -203,6 +207,63 @@ func (s *progressiveRolloutStorage) ListProgressiveRollouts( return progressiveRollouts, totalCount, nextOffset, nil } +func (s *progressiveRolloutStorage) ListProgressiveRolloutsV2( + ctx context.Context, + options *mysql.ListOptions, +) ([]*autoopsproto.ProgressiveRollout, int64, int, error) { + println("kaki ListProgressiveRolloutsV2") + var whereParts []mysql.WherePart = []mysql.WherePart{} + var orderBySQL string = "" + var limitOffsetSQL string = "" + var limit int = 0 + var offset int = 0 + if options != nil { + whereParts = options.CreateWhereParts() + orderBySQL = mysql.ConstructOrderBySQLString(options.Orders) + limitOffsetSQL = mysql.ConstructLimitOffsetSQLString(options.Limit, options.Offset) + limit = options.Limit + offset = options.Offset + } + + whereSQL, whereArgs := mysql.ConstructWhereSQLString(whereParts) + query := fmt.Sprintf(selectOpsProgressiveRolloutsSQL, whereSQL, orderBySQL, limitOffsetSQL) + rows, err := s.qe.QueryContext(ctx, query, whereArgs...) + if err != nil { + return nil, 0, 0, err + } + defer rows.Close() + progressiveRollouts := make([]*autoopsproto.ProgressiveRollout, 0, limit) + for rows.Next() { + progressiveRollout := autoopsproto.ProgressiveRollout{} + err := rows.Scan( + &progressiveRollout.Id, + &progressiveRollout.FeatureId, + &mysql.JSONObject{Val: &progressiveRollout.Clause}, + &progressiveRollout.Status, + &progressiveRollout.StoppedBy, + &progressiveRollout.Type, + &progressiveRollout.StoppedAt, + &progressiveRollout.CreatedAt, + &progressiveRollout.UpdatedAt, + ) + if err != nil { + return nil, 0, 0, err + } + progressiveRollouts = append(progressiveRollouts, &progressiveRollout) + } + if rows.Err() != nil { + return nil, 0, 0, err + } + nextOffset := offset + len(progressiveRollouts) + var totalCount int64 + countQuery := fmt.Sprintf(countOpsProgressiveRolloutsSQL, whereSQL) + err = s.qe.QueryRowContext(ctx, countQuery, whereArgs...).Scan(&totalCount) + if err != nil { + return nil, 0, 0, err + } + return progressiveRollouts, totalCount, nextOffset, nil +} + func (s *progressiveRolloutStorage) UpdateProgressiveRollout( ctx context.Context, progressiveRollout *domain.ProgressiveRollout, diff --git a/pkg/autoops/storage/v2/progressive_rollout_test.go b/pkg/autoops/storage/v2/progressive_rollout_test.go index 17a461b73e..b7722bb432 100644 --- a/pkg/autoops/storage/v2/progressive_rollout_test.go +++ b/pkg/autoops/storage/v2/progressive_rollout_test.go @@ -16,6 +16,7 @@ package v2 import ( "context" + "errors" "testing" "github.com/stretchr/testify/assert" @@ -83,6 +84,159 @@ func TestCreateProgressiveRollout(t *testing.T) { } } +func TestListProgressiveRollouts(t *testing.T) { + t.Parallel() + mockController := gomock.NewController(t) + defer mockController.Finish() + + patterns := []struct { + setup func(*progressiveRolloutStorage) + whereParts []mysql.WherePart + orders []*mysql.Order + limit int + offset int + expected []*proto.ProgressiveRollout + expectedCursor int + expectedTotalCount int64 + expectedErr error + }{ + { + setup: func(s *progressiveRolloutStorage) { + s.qe.(*mock.MockQueryExecer).EXPECT().QueryContext( + gomock.Any(), gomock.Any(), gomock.Any(), + ).Return(nil, errors.New("error")) + }, + whereParts: nil, + orders: nil, + limit: 0, + offset: 0, + expected: nil, + expectedCursor: 0, + expectedTotalCount: 0, + expectedErr: errors.New("error"), + }, + { + setup: func(s *progressiveRolloutStorage) { + rows := mock.NewMockRows(mockController) + rows.EXPECT().Close().Return(nil) + rows.EXPECT().Next().Return(false) + rows.EXPECT().Err().Return(nil) + s.qe.(*mock.MockQueryExecer).EXPECT().QueryContext( + gomock.Any(), gomock.Any(), gomock.Any(), + ).Return(rows, nil) + row := mock.NewMockRow(mockController) + s.qe.(*mock.MockQueryExecer).EXPECT().QueryRowContext( + gomock.Any(), gomock.Regex(`^SELECT\s+COUNT\(1\)\s+FROM\s+ops_progressive_rollout\s+WHERE\s+num\s+>=\s+\?`), gomock.Any(), + ).Return(row) + row.EXPECT().Scan(gomock.Any()).Return(nil) + }, + whereParts: []mysql.WherePart{ + mysql.NewFilter("num", ">=", 5), + }, + orders: []*mysql.Order{ + mysql.NewOrder("id", mysql.OrderDirectionAsc), + }, + limit: 10, + offset: 5, + expected: []*proto.ProgressiveRollout{}, + expectedCursor: 5, + expectedTotalCount: 0, + expectedErr: nil, + }, + } + for _, p := range patterns { + storage := newProgressiveRolloutStorageWithMock(t, mockController) + if p.setup != nil { + p.setup(storage) + } + pr, totalCount, cursor, err := storage.ListProgressiveRollouts(context.Background(), p.whereParts, p.orders, p.limit, p.offset) + assert.Equal(t, p.expected, pr) + assert.Equal(t, p.expectedCursor, cursor) + assert.Equal(t, p.expectedTotalCount, totalCount) + assert.Equal(t, p.expectedErr, err) + } +} + +func TestListProgressiveRolloutsV2(t *testing.T) { + t.Parallel() + mockController := gomock.NewController(t) + defer mockController.Finish() + + patterns := []struct { + setup func(*progressiveRolloutStorage) + listOpts *mysql.ListOptions + expected []*proto.ProgressiveRollout + expectedCursor int + expectedTotalCount int64 + expectedErr error + }{ + { + setup: func(s *progressiveRolloutStorage) { + s.qe.(*mock.MockQueryExecer).EXPECT().QueryContext( + gomock.Any(), gomock.Any(), gomock.Any(), + ).Return(nil, errors.New("error")) + }, + listOpts: nil, + expected: nil, + expectedCursor: 0, + expectedTotalCount: 0, + expectedErr: errors.New("error"), + }, + { + setup: func(s *progressiveRolloutStorage) { + rows := mock.NewMockRows(mockController) + rows.EXPECT().Close().Return(nil) + rows.EXPECT().Next().Return(false) + rows.EXPECT().Err().Return(nil) + s.qe.(*mock.MockQueryExecer).EXPECT().QueryContext( + gomock.Any(), gomock.Any(), gomock.Any(), + ).Return(rows, nil) + row := mock.NewMockRow(mockController) + s.qe.(*mock.MockQueryExecer).EXPECT().QueryRowContext( + gomock.Any(), gomock.Regex(`^SELECT\s+COUNT\(1\)\s+FROM\s+ops_progressive_rollout\s+WHERE\s+num\s+>=\s+\?`), gomock.Any(), + ).Return(row) + row.EXPECT().Scan(gomock.Any()).Return(nil) + }, + listOpts: &mysql.ListOptions{ + Limit: 10, + Offset: 5, + Filters: []*mysql.FilterV2{ + { + Column: "num", + Operator: mysql.OperatorGreaterThanOrEqual, + Value: 5, + }, + }, + InFilter: nil, + NullFilters: nil, + JSONFilters: nil, + SearchQuery: nil, + Orders: []*mysql.Order{ + { + Column: "id", + Direction: mysql.OrderDirectionAsc, + }, + }, + }, + expected: []*proto.ProgressiveRollout{}, + expectedCursor: 5, + expectedTotalCount: 0, + expectedErr: nil, + }, + } + for _, p := range patterns { + storage := newProgressiveRolloutStorageWithMock(t, mockController) + if p.setup != nil { + p.setup(storage) + } + pr, totalCount, cursor, err := storage.ListProgressiveRolloutsV2(context.Background(), p.listOpts) + assert.Equal(t, p.expected, pr) + assert.Equal(t, p.expectedCursor, cursor) + assert.Equal(t, p.expectedTotalCount, totalCount) + assert.Equal(t, p.expectedErr, err) + } +} + func newProgressiveRolloutStorageWithMock(t *testing.T, mockController *gomock.Controller) *progressiveRolloutStorage { t.Helper() return &progressiveRolloutStorage{mock.NewMockQueryExecer(mockController)} diff --git a/pkg/storage/v2/mysql/query.go b/pkg/storage/v2/mysql/query.go index 1c62e4c45c..f59d554361 100644 --- a/pkg/storage/v2/mysql/query.go +++ b/pkg/storage/v2/mysql/query.go @@ -23,6 +23,41 @@ import ( const placeHolder = "?" +type Operator int + +const ( + // Operation to find the field is equal to the specified value. + OperatorEqual = iota + 1 + // Operation to find the field isn't equal to the specified value. + OperatorNotEqual + // Operation to find ones that contain any one of the multiple values. + OperatorIn + // Operation to find ones that do not contain any of the specified multiple values. + OperatorNotIn + // Operation to find ones the field is greater than the specified value. + OperatorGreaterThan + // Operation to find ones the field is greater or equal than the specified value. + OperatorGreaterThanOrEqual + // Operation to find ones the field is less than the specified value. + OperatorLessThan + // Operation to find ones the field is less or equal than the specified value. + OperatorLessThanOrEqual + // Operation to find ones that have a specified value in its array. + OperatorContains +) + +var operatorMap = map[Operator]string{ + OperatorEqual: "=", + OperatorNotEqual: "!=", + OperatorIn: "IN", + OperatorNotIn: "NOT IN", + OperatorGreaterThan: ">", + OperatorGreaterThanOrEqual: ">=", + OperatorLessThan: "<", + OperatorLessThanOrEqual: "<=", + OperatorContains: "MEMBER OF", +} + type WherePart interface { SQLString() (sql string, args []interface{}) } @@ -50,6 +85,21 @@ func (f *Filter) SQLString() (sql string, args []interface{}) { return } +type FilterV2 struct { + Column string + Operator Operator + Value interface{} +} + +func (f *FilterV2) SQLString() (sql string, args []interface{}) { + if f.Column == "" || f.Operator < OperatorEqual || f.Operator > OperatorContains { + return "", nil + } + sql = fmt.Sprintf("%s %s %s", f.Column, operatorMap[f.Operator], placeHolder) + args = append(args, f.Value) + return +} + type InFilter struct { Column string Values []interface{} @@ -63,7 +113,7 @@ func NewInFilter(column string, values []interface{}) WherePart { } func (f *InFilter) SQLString() (sql string, args []interface{}) { - if f.Column == "" { + if f.Column == "" || len(f.Values) == 0 { return "", nil } var sb strings.Builder @@ -288,6 +338,27 @@ func ConstructOrderBySQLString(orders []*Order) string { return sb.String() } +type Orders struct { + Orders []*Order +} + +func (o *Orders) SQLString() (sql string, args []interface{}) { + if len(o.Orders) == 0 { + return "", nil + } + var sb strings.Builder + sb.WriteString("ORDER BY ") + for i, o := range o.Orders { + if i != 0 { + sb.WriteString(", ") + } + sb.WriteString(o.Column) + sb.WriteString(" ") + sb.WriteString(o.Direction.String()) + } + return sb.String(), nil +} + const ( QueryNoLimit = 0 QueryNoOffset = 0 @@ -309,3 +380,40 @@ func ConstructLimitOffsetSQLString(limit, offset int) string { } return fmt.Sprintf("LIMIT %d OFFSET %d", limit, offset) } + +type ListOptions struct { + Limit int + Filters []*FilterV2 + InFilter *InFilter + NullFilters []*NullFilter + JSONFilters []*JSONFilter + SearchQuery *SearchQuery + Orders []*Order + Offset int +} + +func (lo *ListOptions) CreateWhereParts() []WherePart { + var whereParts []WherePart + if lo.Filters != nil { + for _, f := range lo.Filters { + whereParts = append(whereParts, f) + } + } + if lo.InFilter != nil { + whereParts = append(whereParts, lo.InFilter) + } + if lo.NullFilters != nil { + for _, f := range lo.NullFilters { + whereParts = append(whereParts, f) + } + } + if lo.JSONFilters != nil { + for _, f := range lo.JSONFilters { + whereParts = append(whereParts, f) + } + } + if lo.SearchQuery != nil { + whereParts = append(whereParts, lo.SearchQuery) + } + return whereParts +} From 17435666e60c96c17b686da4d0d797ce4e8aedd5 Mon Sep 17 00:00:00 2001 From: kakcy Date: Fri, 13 Dec 2024 16:35:49 +0900 Subject: [PATCH 2/5] refactor: change all ListProgressiveRollouts to use ListOptions --- pkg/autoops/api/api.go | 2 +- pkg/autoops/api/progressive_rollout.go | 81 +------------------ .../api/stop_progressive_rollout_operation.go | 25 +++++- pkg/autoops/storage/v2/auto_ops_rule.go | 49 ----------- pkg/autoops/storage/v2/auto_ops_rule_test.go | 71 +--------------- pkg/autoops/storage/v2/progressive_rollout.go | 53 ------------ .../storage/v2/progressive_rollout_test.go | 75 +---------------- pkg/feature/api/feature.go | 26 +++++- 8 files changed, 48 insertions(+), 334 deletions(-) diff --git a/pkg/autoops/api/api.go b/pkg/autoops/api/api.go index c1e6fe2647..4445aba4eb 100644 --- a/pkg/autoops/api/api.go +++ b/pkg/autoops/api/api.go @@ -1162,7 +1162,7 @@ func (s *AutoOpsService) listAutoOpsRules( Orders: nil, } - autoOpsRules, nextCursor, err := storage.ListAutoOpsRulesV2(ctx, listOptions) + autoOpsRules, nextCursor, err := storage.ListAutoOpsRules(ctx, listOptions) if err != nil { s.logger.Error( "Failed to list autoOpsRules", diff --git a/pkg/autoops/api/progressive_rollout.go b/pkg/autoops/api/progressive_rollout.go index a3d0b1fc92..98c7c8339b 100644 --- a/pkg/autoops/api/progressive_rollout.go +++ b/pkg/autoops/api/progressive_rollout.go @@ -373,7 +373,7 @@ func (s *AutoOpsService) ListProgressiveRollouts( if err != nil { return nil, err } - progressiveRollout, totalCount, nextOffset, err := s.listProgressiveRolloutsV2( + progressiveRollout, totalCount, nextOffset, err := s.listProgressiveRollouts( ctx, req, localizer, @@ -608,83 +608,6 @@ func (s *AutoOpsService) listProgressiveRollouts( ctx context.Context, req *autoopsproto.ListProgressiveRolloutsRequest, localizer locale.Localizer, -) ([]*autoopsproto.ProgressiveRollout, int64, int, error) { - whereParts := []mysql.WherePart{ - mysql.NewFilter("environment_id", "=", req.EnvironmentId), - } - limit := int(req.PageSize) - cursor := req.Cursor - if cursor == "" { - cursor = "0" - } - offset, err := strconv.Atoi(cursor) - if err != nil { - dt, err := statusProgressiveRolloutInvalidCursor.WithDetails(&errdetails.LocalizedMessage{ - Locale: localizer.GetLocale(), - Message: localizer.MustLocalizeWithTemplate(locale.InvalidArgumentError, "cursor"), - }) - if err != nil { - return nil, 0, 0, statusProgressiveRolloutInternal.Err() - } - return nil, 0, 0, dt.Err() - } - if len(req.FeatureIds) > 0 { - fIDs := s.convToInterfaceSlice(req.FeatureIds) - whereParts = append(whereParts, mysql.NewInFilter("feature_id", fIDs)) - } - orders, err := s.newListProgressiveRolloutsOrdersMySQL( - req.OrderBy, - req.OrderDirection, - localizer, - ) - if err != nil { - s.logger.Error( - "Invalid argument", - log.FieldsFromImcomingContext(ctx).AddFields( - zap.Error(err), - zap.String("environmentId", req.EnvironmentId), - )..., - ) - return nil, 0, 0, err - } - if req.Type != nil { - whereParts = append(whereParts, mysql.NewFilter("type", "=", req.Type)) - } - if req.Status != nil { - whereParts = append(whereParts, mysql.NewFilter("status", "=", req.Status)) - } - storage := v2as.NewProgressiveRolloutStorage(s.mysqlClient) - progressiveRollouts, totalCount, nextOffset, err := storage.ListProgressiveRollouts( - ctx, - whereParts, - orders, - limit, - offset, - ) - if err != nil { - s.logger.Error( - "Failed to list progressive rollouts", - log.FieldsFromImcomingContext(ctx).AddFields( - zap.Error(err), - zap.String("environmentId", req.EnvironmentId), - )..., - ) - dt, err := statusProgressiveRolloutInternal.WithDetails(&errdetails.LocalizedMessage{ - Locale: localizer.GetLocale(), - Message: localizer.MustLocalize(locale.InternalServerError), - }) - if err != nil { - return nil, 0, 0, statusProgressiveRolloutInternal.Err() - } - return nil, 0, 0, dt.Err() - } - return progressiveRollouts, totalCount, nextOffset, nil -} - -func (s *AutoOpsService) listProgressiveRolloutsV2( - ctx context.Context, - req *autoopsproto.ListProgressiveRolloutsRequest, - localizer locale.Localizer, ) ([]*autoopsproto.ProgressiveRollout, int64, int, error) { filters := []*mysql.FilterV2{ { @@ -750,7 +673,7 @@ func (s *AutoOpsService) listProgressiveRolloutsV2( } storage := v2as.NewProgressiveRolloutStorage(s.mysqlClient) - progressiveRollouts, totalCount, nextOffset, err := storage.ListProgressiveRolloutsV2(ctx, listOptions) + progressiveRollouts, totalCount, nextOffset, err := storage.ListProgressiveRollouts(ctx, listOptions) if err != nil { s.logger.Error( "Failed to list progressive rollouts", diff --git a/pkg/autoops/api/stop_progressive_rollout_operation.go b/pkg/autoops/api/stop_progressive_rollout_operation.go index 49de85af38..58e4231edd 100644 --- a/pkg/autoops/api/stop_progressive_rollout_operation.go +++ b/pkg/autoops/api/stop_progressive_rollout_operation.go @@ -30,11 +30,28 @@ func executeStopProgressiveRolloutOperation( environmentId string, operation autoopsproto.ProgressiveRollout_StoppedBy, ) error { - whereParts := []mysql.WherePart{ - mysql.NewFilter("environment_id", "=", environmentId), - mysql.NewInFilter("feature_id", featureIDs), + filters := []*mysql.FilterV2{ + { + Column: "environment_id", + Operator: mysql.OperatorEqual, + Value: environmentId, + }, } - list, _, _, err := storage.ListProgressiveRollouts(ctx, whereParts, nil, 0, 0) + inFilter := &mysql.InFilter{ + Column: "feature_id", + Values: featureIDs, + } + listOptions := &mysql.ListOptions{ + Filters: filters, + Orders: nil, + InFilter: inFilter, + NullFilters: nil, + JSONFilters: nil, + SearchQuery: nil, + Limit: 0, + Offset: 0, + } + list, _, _, err := storage.ListProgressiveRollouts(ctx, listOptions) if err != nil { return err } diff --git a/pkg/autoops/storage/v2/auto_ops_rule.go b/pkg/autoops/storage/v2/auto_ops_rule.go index 24dc7c1290..047df2c3fd 100644 --- a/pkg/autoops/storage/v2/auto_ops_rule.go +++ b/pkg/autoops/storage/v2/auto_ops_rule.go @@ -48,12 +48,6 @@ type AutoOpsRuleStorage interface { UpdateAutoOpsRule(ctx context.Context, e *domain.AutoOpsRule, environmentId string) error GetAutoOpsRule(ctx context.Context, id, environmentId string) (*domain.AutoOpsRule, error) ListAutoOpsRules( - ctx context.Context, - whereParts []mysql.WherePart, - orders []*mysql.Order, - limit, offset int, - ) ([]*proto.AutoOpsRule, int, error) - ListAutoOpsRulesV2( ctx context.Context, options *mysql.ListOptions, ) ([]*proto.AutoOpsRule, int, error) @@ -157,52 +151,9 @@ func (s *autoOpsRuleStorage) GetAutoOpsRule( } func (s *autoOpsRuleStorage) ListAutoOpsRules( - ctx context.Context, - whereParts []mysql.WherePart, - orders []*mysql.Order, - limit, offset int, -) ([]*proto.AutoOpsRule, int, error) { - whereSQL, whereArgs := mysql.ConstructWhereSQLString(whereParts) - orderBySQL := mysql.ConstructOrderBySQLString(orders) - limitOffsetSQL := mysql.ConstructLimitOffsetSQLString(limit, offset) - query := fmt.Sprintf(selectAutoOpsRulesSQL, whereSQL, orderBySQL, limitOffsetSQL) - rows, err := s.qe.QueryContext(ctx, query, whereArgs...) - if err != nil { - return nil, 0, err - } - defer rows.Close() - autoOpsRules := make([]*proto.AutoOpsRule, 0, limit) - for rows.Next() { - autoOpsRule := proto.AutoOpsRule{} - var opsType int32 - err := rows.Scan( - &autoOpsRule.Id, - &autoOpsRule.FeatureId, - &opsType, - &mysql.JSONObject{Val: &autoOpsRule.Clauses}, - &autoOpsRule.CreatedAt, - &autoOpsRule.UpdatedAt, - &autoOpsRule.Deleted, - &autoOpsRule.AutoOpsStatus, - ) - if err != nil { - return nil, 0, err - } - autoOpsRule.OpsType = proto.OpsType(opsType) - autoOpsRules = append(autoOpsRules, &autoOpsRule) - } - if rows.Err() != nil { - return nil, 0, err - } - nextOffset := offset + len(autoOpsRules) - return autoOpsRules, nextOffset, nil -} - -func (s *autoOpsRuleStorage) ListAutoOpsRulesV2( ctx context.Context, options *mysql.ListOptions, ) ([]*proto.AutoOpsRule, int, error) { - println("kaki ListAutoOpsRulesV2") var whereParts []mysql.WherePart = []mysql.WherePart{} var orderBySQL string = "" var limitOffsetSQL string = "" diff --git a/pkg/autoops/storage/v2/auto_ops_rule_test.go b/pkg/autoops/storage/v2/auto_ops_rule_test.go index 7919721b47..5f979c2fc4 100644 --- a/pkg/autoops/storage/v2/auto_ops_rule_test.go +++ b/pkg/autoops/storage/v2/auto_ops_rule_test.go @@ -184,75 +184,6 @@ func TestGetAutoOpsRule(t *testing.T) { } func TestListAutoOpsRules(t *testing.T) { - t.Parallel() - mockController := gomock.NewController(t) - defer mockController.Finish() - patterns := []struct { - setup func(*autoOpsRuleStorage) - whereParts []mysql.WherePart - orders []*mysql.Order - limit int - offset int - expected []*proto.AutoOpsRule - expectedCursor int - expectedErr error - }{ - { - setup: func(s *autoOpsRuleStorage) { - s.qe.(*mock.MockQueryExecer).EXPECT().QueryContext( - gomock.Any(), gomock.Any(), gomock.Any(), - ).Return(nil, errors.New("error")) - }, - whereParts: nil, - orders: nil, - limit: 0, - offset: 0, - expected: nil, - expectedCursor: 0, - expectedErr: errors.New("error"), - }, - { - setup: func(s *autoOpsRuleStorage) { - rows := mock.NewMockRows(mockController) - rows.EXPECT().Close().Return(nil) - rows.EXPECT().Next().Return(false) - rows.EXPECT().Err().Return(nil) - s.qe.(*mock.MockQueryExecer).EXPECT().QueryContext( - gomock.Any(), gomock.Any(), gomock.Any(), - ).Return(rows, nil) - }, - whereParts: []mysql.WherePart{ - mysql.NewFilter("num", ">=", 5), - }, - orders: []*mysql.Order{ - mysql.NewOrder("id", mysql.OrderDirectionAsc), - }, - limit: 10, - offset: 5, - expected: []*proto.AutoOpsRule{}, - expectedCursor: 5, - expectedErr: nil, - }, - } - for _, p := range patterns { - storage := newAutoOpsRuleStorageWithMock(t, mockController) - if p.setup != nil { - p.setup(storage) - } - autoOpsRules, cursor, err := storage.ListAutoOpsRules( - context.Background(), - p.whereParts, - p.orders, - p.limit, - p.offset, - ) - assert.Equal(t, p.expected, autoOpsRules) - assert.Equal(t, p.expectedCursor, cursor) - assert.Equal(t, p.expectedErr, err) - } -} - -func TestListAutoOpsRulesV2(t *testing.T) { t.Parallel() mockController := gomock.NewController(t) defer mockController.Finish() @@ -315,7 +246,7 @@ func TestListAutoOpsRulesV2(t *testing.T) { if p.setup != nil { p.setup(storage) } - autoOpsRules, cursor, err := storage.ListAutoOpsRulesV2( + autoOpsRules, cursor, err := storage.ListAutoOpsRules( context.Background(), p.listOpts, ) diff --git a/pkg/autoops/storage/v2/progressive_rollout.go b/pkg/autoops/storage/v2/progressive_rollout.go index cd3e567958..ffe3f076ab 100644 --- a/pkg/autoops/storage/v2/progressive_rollout.go +++ b/pkg/autoops/storage/v2/progressive_rollout.go @@ -60,12 +60,6 @@ type ProgressiveRolloutStorage interface { GetProgressiveRollout(ctx context.Context, id, environmentId string) (*domain.ProgressiveRollout, error) DeleteProgressiveRollout(ctx context.Context, id, environmentId string) error ListProgressiveRollouts( - ctx context.Context, - whereParts []mysql.WherePart, - orders []*mysql.Order, - limit, offset int, - ) ([]*autoopsproto.ProgressiveRollout, int64, int, error) - ListProgressiveRolloutsV2( ctx context.Context, options *mysql.ListOptions, ) ([]*autoopsproto.ProgressiveRollout, int64, int, error) @@ -161,53 +155,6 @@ func (s *progressiveRolloutStorage) DeleteProgressiveRollout( } func (s *progressiveRolloutStorage) ListProgressiveRollouts( - ctx context.Context, - whereParts []mysql.WherePart, - orders []*mysql.Order, - limit, offset int, -) ([]*autoopsproto.ProgressiveRollout, int64, int, error) { - whereSQL, whereArgs := mysql.ConstructWhereSQLString(whereParts) - orderBySQL := mysql.ConstructOrderBySQLString(orders) - limitOffsetSQL := mysql.ConstructLimitOffsetSQLString(limit, offset) - query := fmt.Sprintf(selectOpsProgressiveRolloutsSQL, whereSQL, orderBySQL, limitOffsetSQL) - rows, err := s.qe.QueryContext(ctx, query, whereArgs...) - if err != nil { - return nil, 0, 0, err - } - defer rows.Close() - progressiveRollouts := make([]*autoopsproto.ProgressiveRollout, 0, limit) - for rows.Next() { - progressiveRollout := autoopsproto.ProgressiveRollout{} - err := rows.Scan( - &progressiveRollout.Id, - &progressiveRollout.FeatureId, - &mysql.JSONObject{Val: &progressiveRollout.Clause}, - &progressiveRollout.Status, - &progressiveRollout.StoppedBy, - &progressiveRollout.Type, - &progressiveRollout.StoppedAt, - &progressiveRollout.CreatedAt, - &progressiveRollout.UpdatedAt, - ) - if err != nil { - return nil, 0, 0, err - } - progressiveRollouts = append(progressiveRollouts, &progressiveRollout) - } - if rows.Err() != nil { - return nil, 0, 0, err - } - nextOffset := offset + len(progressiveRollouts) - var totalCount int64 - countQuery := fmt.Sprintf(countOpsProgressiveRolloutsSQL, whereSQL) - err = s.qe.QueryRowContext(ctx, countQuery, whereArgs...).Scan(&totalCount) - if err != nil { - return nil, 0, 0, err - } - return progressiveRollouts, totalCount, nextOffset, nil -} - -func (s *progressiveRolloutStorage) ListProgressiveRolloutsV2( ctx context.Context, options *mysql.ListOptions, ) ([]*autoopsproto.ProgressiveRollout, int64, int, error) { diff --git a/pkg/autoops/storage/v2/progressive_rollout_test.go b/pkg/autoops/storage/v2/progressive_rollout_test.go index b7722bb432..5c43c99398 100644 --- a/pkg/autoops/storage/v2/progressive_rollout_test.go +++ b/pkg/autoops/storage/v2/progressive_rollout_test.go @@ -89,79 +89,6 @@ func TestListProgressiveRollouts(t *testing.T) { mockController := gomock.NewController(t) defer mockController.Finish() - patterns := []struct { - setup func(*progressiveRolloutStorage) - whereParts []mysql.WherePart - orders []*mysql.Order - limit int - offset int - expected []*proto.ProgressiveRollout - expectedCursor int - expectedTotalCount int64 - expectedErr error - }{ - { - setup: func(s *progressiveRolloutStorage) { - s.qe.(*mock.MockQueryExecer).EXPECT().QueryContext( - gomock.Any(), gomock.Any(), gomock.Any(), - ).Return(nil, errors.New("error")) - }, - whereParts: nil, - orders: nil, - limit: 0, - offset: 0, - expected: nil, - expectedCursor: 0, - expectedTotalCount: 0, - expectedErr: errors.New("error"), - }, - { - setup: func(s *progressiveRolloutStorage) { - rows := mock.NewMockRows(mockController) - rows.EXPECT().Close().Return(nil) - rows.EXPECT().Next().Return(false) - rows.EXPECT().Err().Return(nil) - s.qe.(*mock.MockQueryExecer).EXPECT().QueryContext( - gomock.Any(), gomock.Any(), gomock.Any(), - ).Return(rows, nil) - row := mock.NewMockRow(mockController) - s.qe.(*mock.MockQueryExecer).EXPECT().QueryRowContext( - gomock.Any(), gomock.Regex(`^SELECT\s+COUNT\(1\)\s+FROM\s+ops_progressive_rollout\s+WHERE\s+num\s+>=\s+\?`), gomock.Any(), - ).Return(row) - row.EXPECT().Scan(gomock.Any()).Return(nil) - }, - whereParts: []mysql.WherePart{ - mysql.NewFilter("num", ">=", 5), - }, - orders: []*mysql.Order{ - mysql.NewOrder("id", mysql.OrderDirectionAsc), - }, - limit: 10, - offset: 5, - expected: []*proto.ProgressiveRollout{}, - expectedCursor: 5, - expectedTotalCount: 0, - expectedErr: nil, - }, - } - for _, p := range patterns { - storage := newProgressiveRolloutStorageWithMock(t, mockController) - if p.setup != nil { - p.setup(storage) - } - pr, totalCount, cursor, err := storage.ListProgressiveRollouts(context.Background(), p.whereParts, p.orders, p.limit, p.offset) - assert.Equal(t, p.expected, pr) - assert.Equal(t, p.expectedCursor, cursor) - assert.Equal(t, p.expectedTotalCount, totalCount) - assert.Equal(t, p.expectedErr, err) - } -} - -func TestListProgressiveRolloutsV2(t *testing.T) { - t.Parallel() - mockController := gomock.NewController(t) - defer mockController.Finish() - patterns := []struct { setup func(*progressiveRolloutStorage) listOpts *mysql.ListOptions @@ -229,7 +156,7 @@ func TestListProgressiveRolloutsV2(t *testing.T) { if p.setup != nil { p.setup(storage) } - pr, totalCount, cursor, err := storage.ListProgressiveRolloutsV2(context.Background(), p.listOpts) + pr, totalCount, cursor, err := storage.ListProgressiveRollouts(context.Background(), p.listOpts) assert.Equal(t, p.expected, pr) assert.Equal(t, p.expectedCursor, cursor) assert.Equal(t, p.expectedTotalCount, totalCount) diff --git a/pkg/feature/api/feature.go b/pkg/feature/api/feature.go index d788f1eee3..97b1be1515 100644 --- a/pkg/feature/api/feature.go +++ b/pkg/feature/api/feature.go @@ -1923,11 +1923,29 @@ func (s *FeatureService) stopProgressiveRollout( EnvironmentId, featureID string) error { storage := v2ao.NewProgressiveRolloutStorage(tx) ids := convToInterfaceSlice([]string{featureID}) - whereParts := []mysql.WherePart{ - mysql.NewFilter("environment_id", "=", EnvironmentId), - mysql.NewInFilter("feature_id", ids), + filters := []*mysql.FilterV2{ + { + Column: "environment_id", + Operator: mysql.OperatorEqual, + Value: EnvironmentId, + }, + } + inFilter := &mysql.InFilter{ + Column: "feature_id", + Values: ids, } - list, _, _, err := storage.ListProgressiveRollouts(ctx, whereParts, nil, 0, 0) + listOptions := &mysql.ListOptions{ + Filters: filters, + Orders: nil, + InFilter: inFilter, + NullFilters: nil, + JSONFilters: nil, + SearchQuery: nil, + Limit: 0, + Offset: 0, + } + + list, _, _, err := storage.ListProgressiveRollouts(ctx, listOptions) if err != nil { return err } From 512224425f4b26785a8b9150d70f1d077e6b76af Mon Sep 17 00:00:00 2001 From: kakcy Date: Mon, 16 Dec 2024 17:45:10 +0900 Subject: [PATCH 3/5] reformat --- pkg/autoops/storage/v2/auto_ops_rule_test.go | 2 +- pkg/autoops/storage/v2/progressive_rollout.go | 1 - pkg/autoops/storage/v2/progressive_rollout_test.go | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/autoops/storage/v2/auto_ops_rule_test.go b/pkg/autoops/storage/v2/auto_ops_rule_test.go index 5f979c2fc4..e39a07eb6b 100644 --- a/pkg/autoops/storage/v2/auto_ops_rule_test.go +++ b/pkg/autoops/storage/v2/auto_ops_rule_test.go @@ -212,7 +212,7 @@ func TestListAutoOpsRules(t *testing.T) { rows.EXPECT().Next().Return(false) rows.EXPECT().Err().Return(nil) s.qe.(*mock.MockQueryExecer).EXPECT().QueryContext( - gomock.Any(), gomock.Regex(`^SELECT\s+id,\s+feature_id,\s+ops_type,\s+clauses,\s+created_at,\s+updated_at,\s+deleted,\s+status\s+FROM\s+auto_ops_rule\s+WHERE\s+num\s+>=\s+\?\s+ORDER\s+BY\s+id\s+ASC\s+LIMIT\s+10\s+OFFSET\s+5`), gomock.Any(), + gomock.Any(), gomock.Any(), gomock.Any(), ).Return(rows, nil) }, listOpts: &mysql.ListOptions{ diff --git a/pkg/autoops/storage/v2/progressive_rollout.go b/pkg/autoops/storage/v2/progressive_rollout.go index ffe3f076ab..39aefb7154 100644 --- a/pkg/autoops/storage/v2/progressive_rollout.go +++ b/pkg/autoops/storage/v2/progressive_rollout.go @@ -158,7 +158,6 @@ func (s *progressiveRolloutStorage) ListProgressiveRollouts( ctx context.Context, options *mysql.ListOptions, ) ([]*autoopsproto.ProgressiveRollout, int64, int, error) { - println("kaki ListProgressiveRolloutsV2") var whereParts []mysql.WherePart = []mysql.WherePart{} var orderBySQL string = "" var limitOffsetSQL string = "" diff --git a/pkg/autoops/storage/v2/progressive_rollout_test.go b/pkg/autoops/storage/v2/progressive_rollout_test.go index 5c43c99398..bfb3a004b9 100644 --- a/pkg/autoops/storage/v2/progressive_rollout_test.go +++ b/pkg/autoops/storage/v2/progressive_rollout_test.go @@ -120,7 +120,7 @@ func TestListProgressiveRollouts(t *testing.T) { ).Return(rows, nil) row := mock.NewMockRow(mockController) s.qe.(*mock.MockQueryExecer).EXPECT().QueryRowContext( - gomock.Any(), gomock.Regex(`^SELECT\s+COUNT\(1\)\s+FROM\s+ops_progressive_rollout\s+WHERE\s+num\s+>=\s+\?`), gomock.Any(), + gomock.Any(), gomock.Any(), gomock.Any(), ).Return(row) row.EXPECT().Scan(gomock.Any()).Return(nil) }, From 37d8009c8edecce4bb9e8d4c5a31dd057ca5e37f Mon Sep 17 00:00:00 2001 From: kakcy Date: Mon, 16 Dec 2024 18:02:58 +0900 Subject: [PATCH 4/5] reformat --- pkg/autoops/storage/v2/mock/auto_ops_rule.go | 8 ++++---- pkg/autoops/storage/v2/mock/progressive_rollout.go | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/autoops/storage/v2/mock/auto_ops_rule.go b/pkg/autoops/storage/v2/mock/auto_ops_rule.go index d2c237ca9b..72892dd6da 100644 --- a/pkg/autoops/storage/v2/mock/auto_ops_rule.go +++ b/pkg/autoops/storage/v2/mock/auto_ops_rule.go @@ -73,9 +73,9 @@ func (mr *MockAutoOpsRuleStorageMockRecorder) GetAutoOpsRule(ctx, id, environmen } // ListAutoOpsRules mocks base method. -func (m *MockAutoOpsRuleStorage) ListAutoOpsRules(ctx context.Context, whereParts []mysql.WherePart, orders []*mysql.Order, limit, offset int) ([]*autoops.AutoOpsRule, int, error) { +func (m *MockAutoOpsRuleStorage) ListAutoOpsRules(ctx context.Context, options *mysql.ListOptions) ([]*autoops.AutoOpsRule, int, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ListAutoOpsRules", ctx, whereParts, orders, limit, offset) + ret := m.ctrl.Call(m, "ListAutoOpsRules", ctx, options) ret0, _ := ret[0].([]*autoops.AutoOpsRule) ret1, _ := ret[1].(int) ret2, _ := ret[2].(error) @@ -83,9 +83,9 @@ func (m *MockAutoOpsRuleStorage) ListAutoOpsRules(ctx context.Context, wherePart } // ListAutoOpsRules indicates an expected call of ListAutoOpsRules. -func (mr *MockAutoOpsRuleStorageMockRecorder) ListAutoOpsRules(ctx, whereParts, orders, limit, offset any) *gomock.Call { +func (mr *MockAutoOpsRuleStorageMockRecorder) ListAutoOpsRules(ctx, options any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListAutoOpsRules", reflect.TypeOf((*MockAutoOpsRuleStorage)(nil).ListAutoOpsRules), ctx, whereParts, orders, limit, offset) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListAutoOpsRules", reflect.TypeOf((*MockAutoOpsRuleStorage)(nil).ListAutoOpsRules), ctx, options) } // UpdateAutoOpsRule mocks base method. diff --git a/pkg/autoops/storage/v2/mock/progressive_rollout.go b/pkg/autoops/storage/v2/mock/progressive_rollout.go index 19a71ba78f..213a825718 100644 --- a/pkg/autoops/storage/v2/mock/progressive_rollout.go +++ b/pkg/autoops/storage/v2/mock/progressive_rollout.go @@ -87,9 +87,9 @@ func (mr *MockProgressiveRolloutStorageMockRecorder) GetProgressiveRollout(ctx, } // ListProgressiveRollouts mocks base method. -func (m *MockProgressiveRolloutStorage) ListProgressiveRollouts(ctx context.Context, whereParts []mysql.WherePart, orders []*mysql.Order, limit, offset int) ([]*autoops.ProgressiveRollout, int64, int, error) { +func (m *MockProgressiveRolloutStorage) ListProgressiveRollouts(ctx context.Context, options *mysql.ListOptions) ([]*autoops.ProgressiveRollout, int64, int, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ListProgressiveRollouts", ctx, whereParts, orders, limit, offset) + ret := m.ctrl.Call(m, "ListProgressiveRollouts", ctx, options) ret0, _ := ret[0].([]*autoops.ProgressiveRollout) ret1, _ := ret[1].(int64) ret2, _ := ret[2].(int) @@ -98,9 +98,9 @@ func (m *MockProgressiveRolloutStorage) ListProgressiveRollouts(ctx context.Cont } // ListProgressiveRollouts indicates an expected call of ListProgressiveRollouts. -func (mr *MockProgressiveRolloutStorageMockRecorder) ListProgressiveRollouts(ctx, whereParts, orders, limit, offset any) *gomock.Call { +func (mr *MockProgressiveRolloutStorageMockRecorder) ListProgressiveRollouts(ctx, options any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListProgressiveRollouts", reflect.TypeOf((*MockProgressiveRolloutStorage)(nil).ListProgressiveRollouts), ctx, whereParts, orders, limit, offset) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListProgressiveRollouts", reflect.TypeOf((*MockProgressiveRolloutStorage)(nil).ListProgressiveRollouts), ctx, options) } // UpdateProgressiveRollout mocks base method. From fc410e7528493234ef972635a0854d4671ef7fbd Mon Sep 17 00:00:00 2001 From: kakcy Date: Wed, 15 Jan 2025 18:59:10 +0900 Subject: [PATCH 5/5] refactor: add ConstructQueryAndWhereArgs --- pkg/autoops/storage/v2/auto_ops_rule.go | 23 +++++------------ pkg/autoops/storage/v2/progressive_rollout.go | 25 ++++++------------- .../auto_ops_rule/select_auto_ops_rules.sql | 1 - .../count_ops_progressive_rollouts.sql | 1 - .../select_ops_progressive_rollouts.sql | 1 - pkg/storage/v2/mysql/query.go | 17 ++++++++++++- 6 files changed, 29 insertions(+), 39 deletions(-) diff --git a/pkg/autoops/storage/v2/auto_ops_rule.go b/pkg/autoops/storage/v2/auto_ops_rule.go index 047df2c3fd..da22cd5ffb 100644 --- a/pkg/autoops/storage/v2/auto_ops_rule.go +++ b/pkg/autoops/storage/v2/auto_ops_rule.go @@ -19,7 +19,6 @@ import ( "context" _ "embed" "errors" - "fmt" "github.com/bucketeer-io/bucketeer/pkg/autoops/domain" "github.com/bucketeer-io/bucketeer/pkg/storage/v2/mysql" @@ -154,27 +153,13 @@ func (s *autoOpsRuleStorage) ListAutoOpsRules( ctx context.Context, options *mysql.ListOptions, ) ([]*proto.AutoOpsRule, int, error) { - var whereParts []mysql.WherePart = []mysql.WherePart{} - var orderBySQL string = "" - var limitOffsetSQL string = "" - var limit int = 0 - var offset int = 0 - if options != nil { - whereParts = options.CreateWhereParts() - orderBySQL = mysql.ConstructOrderBySQLString(options.Orders) - limitOffsetSQL = mysql.ConstructLimitOffsetSQLString(options.Limit, options.Offset) - limit = options.Limit - offset = options.Offset - } - - whereSQL, whereArgs := mysql.ConstructWhereSQLString(whereParts) - query := fmt.Sprintf(selectAutoOpsRulesSQL, whereSQL, orderBySQL, limitOffsetSQL) + query, whereArgs := mysql.ConstructQueryAndWhereArgs(selectAutoOpsRulesSQL, options) rows, err := s.qe.QueryContext(ctx, query, whereArgs...) if err != nil { return nil, 0, err } defer rows.Close() - autoOpsRules := make([]*proto.AutoOpsRule, 0, limit) + autoOpsRules := make([]*proto.AutoOpsRule, 0) for rows.Next() { autoOpsRule := proto.AutoOpsRule{} var opsType int32 @@ -197,6 +182,10 @@ func (s *autoOpsRuleStorage) ListAutoOpsRules( if rows.Err() != nil { return nil, 0, err } + var offset int + if options != nil { + offset = options.Offset + } nextOffset := offset + len(autoOpsRules) return autoOpsRules, nextOffset, nil } diff --git a/pkg/autoops/storage/v2/progressive_rollout.go b/pkg/autoops/storage/v2/progressive_rollout.go index 39aefb7154..987fa5368c 100644 --- a/pkg/autoops/storage/v2/progressive_rollout.go +++ b/pkg/autoops/storage/v2/progressive_rollout.go @@ -19,7 +19,6 @@ import ( "context" _ "embed" "errors" - "fmt" "github.com/bucketeer-io/bucketeer/pkg/autoops/domain" "github.com/bucketeer-io/bucketeer/pkg/storage/v2/mysql" @@ -158,27 +157,13 @@ func (s *progressiveRolloutStorage) ListProgressiveRollouts( ctx context.Context, options *mysql.ListOptions, ) ([]*autoopsproto.ProgressiveRollout, int64, int, error) { - var whereParts []mysql.WherePart = []mysql.WherePart{} - var orderBySQL string = "" - var limitOffsetSQL string = "" - var limit int = 0 - var offset int = 0 - if options != nil { - whereParts = options.CreateWhereParts() - orderBySQL = mysql.ConstructOrderBySQLString(options.Orders) - limitOffsetSQL = mysql.ConstructLimitOffsetSQLString(options.Limit, options.Offset) - limit = options.Limit - offset = options.Offset - } - - whereSQL, whereArgs := mysql.ConstructWhereSQLString(whereParts) - query := fmt.Sprintf(selectOpsProgressiveRolloutsSQL, whereSQL, orderBySQL, limitOffsetSQL) + query, whereArgs := mysql.ConstructQueryAndWhereArgs(selectOpsProgressiveRolloutsSQL, options) rows, err := s.qe.QueryContext(ctx, query, whereArgs...) if err != nil { return nil, 0, 0, err } defer rows.Close() - progressiveRollouts := make([]*autoopsproto.ProgressiveRollout, 0, limit) + progressiveRollouts := make([]*autoopsproto.ProgressiveRollout, 0) for rows.Next() { progressiveRollout := autoopsproto.ProgressiveRollout{} err := rows.Scan( @@ -200,9 +185,13 @@ func (s *progressiveRolloutStorage) ListProgressiveRollouts( if rows.Err() != nil { return nil, 0, 0, err } + var offset int + if options != nil { + offset = options.Offset + } nextOffset := offset + len(progressiveRollouts) var totalCount int64 - countQuery := fmt.Sprintf(countOpsProgressiveRolloutsSQL, whereSQL) + countQuery, whereArgs := mysql.ConstructQueryAndWhereArgs(countOpsProgressiveRolloutsSQL, options) err = s.qe.QueryRowContext(ctx, countQuery, whereArgs...).Scan(&totalCount) if err != nil { return nil, 0, 0, err diff --git a/pkg/autoops/storage/v2/sql/auto_ops_rule/select_auto_ops_rules.sql b/pkg/autoops/storage/v2/sql/auto_ops_rule/select_auto_ops_rules.sql index 7bdeb94c4d..257aef8023 100644 --- a/pkg/autoops/storage/v2/sql/auto_ops_rule/select_auto_ops_rules.sql +++ b/pkg/autoops/storage/v2/sql/auto_ops_rule/select_auto_ops_rules.sql @@ -9,4 +9,3 @@ SELECT status FROM auto_ops_rule -%s %s %s diff --git a/pkg/autoops/storage/v2/sql/ops_progressive_rollout/count_ops_progressive_rollouts.sql b/pkg/autoops/storage/v2/sql/ops_progressive_rollout/count_ops_progressive_rollouts.sql index cf17c6551f..efc9c70cef 100644 --- a/pkg/autoops/storage/v2/sql/ops_progressive_rollout/count_ops_progressive_rollouts.sql +++ b/pkg/autoops/storage/v2/sql/ops_progressive_rollout/count_ops_progressive_rollouts.sql @@ -2,4 +2,3 @@ SELECT COUNT(1) FROM ops_progressive_rollout -%s diff --git a/pkg/autoops/storage/v2/sql/ops_progressive_rollout/select_ops_progressive_rollouts.sql b/pkg/autoops/storage/v2/sql/ops_progressive_rollout/select_ops_progressive_rollouts.sql index 8987b405fe..e1923084ea 100644 --- a/pkg/autoops/storage/v2/sql/ops_progressive_rollout/select_ops_progressive_rollouts.sql +++ b/pkg/autoops/storage/v2/sql/ops_progressive_rollout/select_ops_progressive_rollouts.sql @@ -10,4 +10,3 @@ SELECT updated_at FROM ops_progressive_rollout -%s %s %s diff --git a/pkg/storage/v2/mysql/query.go b/pkg/storage/v2/mysql/query.go index f59d554361..27bc5fef92 100644 --- a/pkg/storage/v2/mysql/query.go +++ b/pkg/storage/v2/mysql/query.go @@ -286,7 +286,7 @@ func ConstructWhereSQLString(wps []WherePart) (sql string, args []interface{}) { sb.WriteString(wpSQL) args = append(args, wpArgs...) } - sql = sb.String() + sql = sb.String() + " " return } @@ -338,6 +338,21 @@ func ConstructOrderBySQLString(orders []*Order) string { return sb.String() } +func ConstructQueryAndWhereArgs(baseQuery string, options *ListOptions) (query string, whereArgs []interface{}) { + if options != nil { + var whereQuery string + whereParts := options.CreateWhereParts() + whereQuery, whereArgs = ConstructWhereSQLString(whereParts) + orderByQuery := ConstructOrderBySQLString(options.Orders) + limitOffsetQuery := ConstructLimitOffsetSQLString(options.Limit, options.Offset) + query = baseQuery + whereQuery + orderByQuery + limitOffsetQuery + } else { + query = baseQuery + whereArgs = []interface{}{} + } + return +} + type Orders struct { Orders []*Order }