From 76cea3af760de7b5293aa87b26287cdbe25f17d5 Mon Sep 17 00:00:00 2001 From: Shengwen Yu Date: Mon, 7 Aug 2023 15:14:52 +0800 Subject: [PATCH] fix: refactored ValidateCronString (update function signature; validate the 1st to be 0 when there are 6 fields) Signed-off-by: Shengwen Yu --- src/common/utils/utils.go | 15 ++++++--------- src/common/utils/utils_test.go | 16 ++++------------ src/pkg/p2p/preheat/models/policy/policy.go | 3 +-- src/pkg/retention/policy/models.go | 3 +-- src/server/v2.0/handler/gc.go | 3 +-- src/server/v2.0/handler/purge.go | 3 +-- src/server/v2.0/handler/scan_all.go | 3 +-- 7 files changed, 15 insertions(+), 31 deletions(-) diff --git a/src/common/utils/utils.go b/src/common/utils/utils.go index 0fb9637bdde2..22875014c078 100644 --- a/src/common/utils/utils.go +++ b/src/common/utils/utils.go @@ -302,22 +302,19 @@ func CronParser() cronlib.Parser { } // ValidateCronString check whether it is a valid cron string and whether the 1st field (indicating Seconds of time) of the cron string is a fixed value of 0 or not -func ValidateCronString(cron string) (bool, error) { +func ValidateCronString(cron string) error { if len(cron) == 0 { - return false, fmt.Errorf("empty cron string is invalid") + return fmt.Errorf("empty cron string is invalid") } _, err := CronParser().Parse(cron) if err != nil { - return false, err + return err } cronParts := strings.Split(cron, " ") - if len(cronParts) == 0 { - return false, fmt.Errorf("invalid cron string") - } - if cronParts[0] != "0" { - return false, fmt.Errorf("the 1st field (indicating Seconds of time) of the cron setting must be 0") + if len(cronParts) == 6 && cronParts[0] != "0" { + return fmt.Errorf("the 1st field (indicating Seconds of time) of the cron setting must be 0") } - return true, nil + return nil } // MostMatchSorter is a sorter for the most match, usually invoked in sort Less function diff --git a/src/common/utils/utils_test.go b/src/common/utils/utils_test.go index fc34ff6d48d9..73d9fb28b68a 100644 --- a/src/common/utils/utils_test.go +++ b/src/common/utils/utils_test.go @@ -441,22 +441,19 @@ func TestValidateCronString(t *testing.T) { testCases := []struct { description string input string - isValid bool hasErr bool }{ // empty cron string { description: "test case 1", input: "", - isValid: false, hasErr: true, }, // invalid cron format { description: "test case 2", - input: "1 2 3", - isValid: false, + input: "0 2 3", hasErr: true, }, @@ -464,24 +461,19 @@ func TestValidateCronString(t *testing.T) { { description: "test case 3", input: "1 0 0 1 1 0", - isValid: false, hasErr: true, }, // valid cron string { - description: "test case 2", - input: "0 0 0 1 1 0", - isValid: true, + description: "test case 4", + input: "0 1 2 1 1 *", hasErr: false, }, } for _, tc := range testCases { - gotIsValid, err := ValidateCronString(tc.input) - if tc.isValid != gotIsValid { - t.Errorf("%s, expected isValid to be %v, but the actual isValid is %v", tc.description, tc.isValid, gotIsValid) - } + err := ValidateCronString(tc.input) if tc.hasErr { if err == nil { t.Errorf("%s, expect having error, while actual error returned is nil", tc.description) diff --git a/src/pkg/p2p/preheat/models/policy/policy.go b/src/pkg/p2p/preheat/models/policy/policy.go index 279eb91a5e17..1a59bdddf404 100644 --- a/src/pkg/p2p/preheat/models/policy/policy.go +++ b/src/pkg/p2p/preheat/models/policy/policy.go @@ -122,8 +122,7 @@ type Trigger struct { func (s *Schema) ValidatePreheatPolicy() error { // currently only validate cron string of preheat policy if s.Trigger != nil && s.Trigger.Type == TriggerTypeScheduled && len(s.Trigger.Settings.Cron) > 0 { - isValid, err := utils.ValidateCronString(s.Trigger.Settings.Cron) - if !isValid || err != nil { + if err := utils.ValidateCronString(s.Trigger.Settings.Cron); err != nil { return errors.New(nil).WithCode(errors.BadRequestCode). WithMessage("invalid cron string for scheduled preheat: %s, error: %v", s.Trigger.Settings.Cron, err) } diff --git a/src/pkg/retention/policy/models.go b/src/pkg/retention/policy/models.go index 64970fc7ab47..14e7630f446f 100644 --- a/src/pkg/retention/policy/models.go +++ b/src/pkg/retention/policy/models.go @@ -67,8 +67,7 @@ func (m *Metadata) ValidateRetentionPolicy() error { if m.Trigger.Kind == TriggerKindSchedule && m.Trigger.Settings != nil { cronItem, ok := m.Trigger.Settings[TriggerSettingsCron] if ok && len(cronItem.(string)) > 0 { - isValid, err := utils.ValidateCronString(cronItem.(string)) - if !isValid || err != nil { + if err := utils.ValidateCronString(cronItem.(string)); err != nil { return errors.New(nil).WithCode(errors.BadRequestCode). WithMessage("invalid cron string for scheduled tag retention: %s, error: %v", cronItem.(string), err) } diff --git a/src/server/v2.0/handler/gc.go b/src/server/v2.0/handler/gc.go index ef14bb3dbcda..88cf97cb1bcc 100644 --- a/src/server/v2.0/handler/gc.go +++ b/src/server/v2.0/handler/gc.go @@ -148,8 +148,7 @@ func (g *gcAPI) createSchedule(ctx context.Context, cronType, cron string, polic } func (g *gcAPI) updateSchedule(ctx context.Context, cronType, cron string, policy gc.Policy) error { - isValid, err := utils.ValidateCronString(cron) - if !isValid || err != nil { + if err := utils.ValidateCronString(cron); err != nil { return errors.New(nil).WithCode(errors.BadRequestCode). WithMessage("invalid cron string for scheduled gc: %s, error: %v", cron, err) } diff --git a/src/server/v2.0/handler/purge.go b/src/server/v2.0/handler/purge.go index 773db541c997..564d94c6c3bd 100644 --- a/src/server/v2.0/handler/purge.go +++ b/src/server/v2.0/handler/purge.go @@ -141,8 +141,7 @@ func (p *purgeAPI) kick(ctx context.Context, vendorType string, scheType string, } func (p *purgeAPI) updateSchedule(ctx context.Context, vendorType, cronType, cron string, policy pg.JobPolicy, extraParams map[string]interface{}) error { - isValid, err := utils.ValidateCronString(cron) - if !isValid || err != nil { + if err := utils.ValidateCronString(cron); err != nil { return errors.New(nil).WithCode(errors.BadRequestCode). WithMessage("invalid cron string for scheduled log rotation purge: %s, error: %v", cron, err) } diff --git a/src/server/v2.0/handler/scan_all.go b/src/server/v2.0/handler/scan_all.go index 0ebe012a24b4..1ae867ac83f1 100644 --- a/src/server/v2.0/handler/scan_all.go +++ b/src/server/v2.0/handler/scan_all.go @@ -195,8 +195,7 @@ func (s *scanAllAPI) GetLatestScheduledScanAllMetrics(ctx context.Context, param } func (s *scanAllAPI) createOrUpdateScanAllSchedule(ctx context.Context, cronType, cron string, previous *scheduler.Schedule) (int64, error) { - isValid, err := utils.ValidateCronString(cron) - if !isValid || err != nil { + if err := utils.ValidateCronString(cron); err != nil { return 0, errors.New(nil).WithCode(errors.BadRequestCode). WithMessage("invalid cron string for scheduled scan all: %s, error: %v", cron, err) }