fix timezone name in cron string logic

temporalio · Sep 10, 2022 · 7804d0d · 7804d0d
1 parent 271b9cb
commit 7804d0d
Show file tree

Hide file tree

Showing 3 changed files with 186 additions and 35 deletions.
diff --git a/service/worker/scheduler/calendar_test.go b/service/worker/scheduler/calendar_test.go
@@ -102,6 +102,17 @@ func (s *calendarSuite) TestCalendarMatch() {
 	s.False(cc.matches(time.Date(2022, time.March, 16, 13, 55, 55, 0, time.UTC)))
 	// correct offset
 	s.True(cc.matches(time.Date(2022, time.March, 16, 12, 55, 55, 0, time.UTC)))
+
+	// different sunday representations
+	for _, dow := range []string{"0", "7", "sun", "*", "0-3", "5-7"} {
+		cc = s.mustCompileCalendarSpec(&schedpb.CalendarSpec{
+			Second:    "55",
+			Minute:    "55",
+			Hour:      "5",
+			DayOfWeek: dow,
+		}, pacific)
+		s.True(cc.matches(time.Date(2022, time.March, 6, 5, 55, 55, 0, pacific)))
+	}
 }
 
 func (s *calendarSuite) TestParseCronString() {

diff --git a/service/worker/scheduler/spec.go b/service/worker/scheduler/spec.go
@@ -45,6 +45,44 @@ type (
 )
 
 func NewCompiledSpec(spec *schedpb.ScheduleSpec) (*CompiledSpec, error) {
+	spec, err := canonicalizeSpec(spec)
+	if err != nil {
+		return nil, err
+	}
+
+	// load timezone
+	tz, err := loadTimezone(spec)
+	if err != nil {
+		return nil, err
+	}
+
+	// compile StructuredCalendarSpecs
+	ccs := make([]*compiledCalendar, len(spec.StructuredCalendar))
+	for i, structured := range spec.StructuredCalendar {
+		if ccs[i], err = newCompiledCalendar(structured, tz); err != nil {
+			return nil, err
+		}
+	}
+
+	// compile excludes
+	excludes := make([]*compiledCalendar, len(spec.ExcludeStructuredCalendar))
+	for i, excal := range spec.ExcludeStructuredCalendar {
+		if excludes[i], err = newCompiledCalendar(excal, tz); err != nil {
+			return nil, err
+		}
+	}
+
+	cspec := &CompiledSpec{
+		spec:     spec,
+		tz:       tz,
+		calendar: ccs,
+		excludes: excludes,
+	}
+
+	return cspec, nil
+}
+
+func canonicalizeSpec(spec *schedpb.ScheduleSpec) (*schedpb.ScheduleSpec, error) {
 	// make shallow copy so we can change some fields
 	specCopy := *spec
 	spec = &specCopy
@@ -70,17 +108,18 @@ func NewCompiledSpec(spec *schedpb.ScheduleSpec) (*CompiledSpec, error) {
 	spec.ExcludeCalendar = nil
 
 	// parse CronStrings
+	const unset = "__unset__"
+	cronTZ := unset
 	for _, cs := range spec.CronString {
 		structured, interval, tz, err := parseCronString(cs)
 		if err != nil {
 			return nil, err
 		}
-		if tz != "" {
-			if spec.TimezoneName != tz || spec.TimezoneData != nil {
-				return nil, errConflictingTimezoneNames
-			}
-			spec.TimezoneName = tz
+		if cronTZ != unset && tz != cronTZ {
+			// all cron strings must agree on timezone (whether present or not)
+			return nil, errConflictingTimezoneNames
 		}
+		cronTZ = tz
 		if structured != nil {
 			spec.StructuredCalendar = append(spec.StructuredCalendar, structured)
 		}
@@ -90,43 +129,24 @@ func NewCompiledSpec(spec *schedpb.ScheduleSpec) (*CompiledSpec, error) {
 	}
 	spec.CronString = nil
 
-	// validate intervals
-	for _, interval := range spec.Interval {
-		if err := validateInterval(interval); err != nil {
-			return nil, err
+	// if we have cron string(s), copy the timezone to spec, checking for conflict first.
+	// if cron string timezone is empty string, don't copy, let the one in spec be used.
+	if cronTZ != unset && cronTZ != "" {
+		if spec.TimezoneName != "" && spec.TimezoneName != cronTZ {
+			return nil, errConflictingTimezoneNames
+		} else if spec.TimezoneName == "" {
+			spec.TimezoneName = cronTZ
 		}
 	}
 
-	// load timezone
-	tz, err := loadTimezone(spec)
-	if err != nil {
-		return nil, err
-	}
-
-	// compile StructuredCalendarSpecs
-	ccs := make([]*compiledCalendar, len(spec.StructuredCalendar))
-	for i, structured := range spec.StructuredCalendar {
-		if ccs[i], err = newCompiledCalendar(structured, tz); err != nil {
-			return nil, err
-		}
-	}
-
-	// compile excludes
-	excludes := make([]*compiledCalendar, len(spec.ExcludeStructuredCalendar))
-	for i, excal := range spec.ExcludeStructuredCalendar {
-		if excludes[i], err = newCompiledCalendar(excal, tz); err != nil {
+	// validate intervals
+	for _, interval := range spec.Interval {
+		if err := validateInterval(interval); err != nil {
 			return nil, err
 		}
 	}
 
-	cspec := &CompiledSpec{
-		spec:     spec,
-		tz:       tz,
-		calendar: ccs,
-		excludes: excludes,
-	}
-
-	return cspec, nil
+	return spec, nil
 }
 
 func validateInterval(i *schedpb.IntervalSpec) error {

diff --git a/service/worker/scheduler/spec_test.go b/service/worker/scheduler/spec_test.go
@@ -69,6 +69,126 @@ func (s *specSuite) checkSequenceFull(spec *schedpb.ScheduleSpec, start time.Tim
 	}
 }
 
+func (s *specSuite) TestCanonicalize() {
+	canonical, err := canonicalizeSpec(&schedpb.ScheduleSpec{})
+	s.NoError(err)
+	s.Equal(&schedpb.ScheduleSpec{}, canonical)
+
+	canonical, err = canonicalizeSpec(&schedpb.ScheduleSpec{
+		CronString: []string{
+			"@every 43h",
+		},
+	})
+	s.NoError(err)
+	s.Equal(&schedpb.ScheduleSpec{
+		Interval: []*schedpb.IntervalSpec{{
+			Interval: timestamp.DurationPtr(43 * time.Hour),
+		}},
+	}, canonical)
+
+	// negative interval
+	canonical, err = canonicalizeSpec(&schedpb.ScheduleSpec{
+		Interval: []*schedpb.IntervalSpec{{
+			Interval: timestamp.DurationPtr(-43 * time.Hour),
+		}},
+	})
+	s.Error(err)
+
+	// phase exceeds interval
+	canonical, err = canonicalizeSpec(&schedpb.ScheduleSpec{
+		Interval: []*schedpb.IntervalSpec{{
+			Interval: timestamp.DurationPtr(3 * time.Hour),
+			Phase:    timestamp.DurationPtr(4 * time.Hour),
+		}},
+	})
+	s.Error(err)
+
+	canonical, err = canonicalizeSpec(&schedpb.ScheduleSpec{
+		Calendar: []*schedpb.CalendarSpec{
+			{Hour: "5,7", Minute: "23"},
+		},
+	})
+	s.NoError(err)
+	structured := []*schedpb.StructuredCalendarSpec{{
+		Second:     []*schedpb.Range{{Start: 0}},
+		Minute:     []*schedpb.Range{{Start: 23}},
+		Hour:       []*schedpb.Range{{Start: 5}, {Start: 7}},
+		DayOfMonth: []*schedpb.Range{{Start: 1, End: 31}},
+		Month:      []*schedpb.Range{{Start: 1, End: 12}},
+		DayOfWeek:  []*schedpb.Range{{Start: 0, End: 7}},
+		Year:       []*schedpb.Range{{Start: minCalendarYear, End: maxCalendarYear}},
+	}}
+	s.Equal(&schedpb.ScheduleSpec{
+		StructuredCalendar: structured,
+	}, canonical)
+
+	// no tz in cron string, leave spec alone
+	canonical, err = canonicalizeSpec(&schedpb.ScheduleSpec{
+		CronString: []string{
+			"23 5,7 * * *",
+		},
+		Jitter:       timestamp.DurationPtr(5 * time.Minute),
+		StartTime:    timestamp.TimePtr(time.Date(2022, 3, 23, 0, 0, 0, 0, time.UTC)),
+		TimezoneName: "Europe/London",
+	})
+	s.NoError(err)
+	s.Equal(&schedpb.ScheduleSpec{
+		StructuredCalendar: structured,
+		Jitter:             timestamp.DurationPtr(5 * time.Minute),
+		StartTime:          timestamp.TimePtr(time.Date(2022, 3, 23, 0, 0, 0, 0, time.UTC)),
+		TimezoneName:       "Europe/London",
+	}, canonical)
+
+	// tz matches, ok
+	canonical, err = canonicalizeSpec(&schedpb.ScheduleSpec{
+		CronString: []string{
+			"CRON_TZ=Europe/London 23 5,7 * * *",
+		},
+		TimezoneName: "Europe/London",
+	})
+	s.NoError(err)
+	s.Equal(&schedpb.ScheduleSpec{
+		StructuredCalendar: structured,
+		TimezoneName:       "Europe/London",
+	}, canonical)
+
+	// tz mismatch, error
+	canonical, err = canonicalizeSpec(&schedpb.ScheduleSpec{
+		CronString: []string{
+			"CRON_TZ=America/New_York 23 5,7 * * *",
+		},
+		TimezoneName: "Europe/London",
+	})
+	s.Error(err)
+
+	// tz mismatch between cron strings, error
+	canonical, err = canonicalizeSpec(&schedpb.ScheduleSpec{
+		CronString: []string{
+			"CRON_TZ=Europe/London 23 5,7 * * *",
+			"CRON_TZ=America/New_York 23 5,7 * * *",
+		},
+	})
+	s.Error(err)
+
+	// all cron strings don't agree, error
+	canonical, err = canonicalizeSpec(&schedpb.ScheduleSpec{
+		CronString: []string{
+			"CRON_TZ=Europe/London 23 5,7 * * *",
+			"23 5,7 * * *",
+		},
+	})
+	s.Error(err)
+
+	// all cron strings don't agree, error
+	canonical, err = canonicalizeSpec(&schedpb.ScheduleSpec{
+		CronString: []string{
+			"23 5,7 * * *",
+			"CRON_TZ=Europe/London 23 5,7 * * *",
+		},
+	})
+	s.Error(err)
+}
+
 func (s *specSuite) TestSpecIntervalBasic() {
 	s.checkSequenceRaw(
 		&schedpb.ScheduleSpec{