Skip to content

Commit

Permalink
Return an error rather than panicking on invalid cron specs
Browse files Browse the repository at this point in the history
  • Loading branch information
robfig committed Dec 7, 2013
1 parent 4832237 commit f2c3314
Show file tree
Hide file tree
Showing 6 changed files with 57 additions and 14 deletions.
5 changes: 2 additions & 3 deletions constantdelay.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,11 @@ type ConstantDelaySchedule struct {
}

// Every returns a crontab Schedule that activates once every duration.
// Delays of less than a second are not supported (will panic).
// Delays of less than a second are not supported (will round up to 1 second).
// Any fields less than a Second are truncated.
func Every(duration time.Duration) ConstantDelaySchedule {
if duration < time.Second {
panic("cron/constantdelay: delays of less than a second are not supported: " +
duration.String())
duration = time.Second
}
return ConstantDelaySchedule{
Delay: duration - time.Duration(duration.Nanoseconds())%time.Second,
Expand Down
3 changes: 3 additions & 0 deletions constantdelay_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ func TestConstantDelayNext(t *testing.T) {
// Round to nearest second on the delay
{"Mon Jul 9 14:45 2012", 15*time.Minute + 50*time.Nanosecond, "Mon Jul 9 15:00 2012"},

// Round up to 1 second if the duration is less.
{"Mon Jul 9 14:45:00 2012", 15 * time.Millisecond, "Mon Jul 9 14:45:01 2012"},

// Round to nearest second when calculating the next time.
{"Mon Jul 9 14:45:00.005 2012", 15 * time.Minute, "Mon Jul 9 15:00 2012"},

Expand Down
13 changes: 9 additions & 4 deletions cron.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,18 @@ type FuncJob func()
func (f FuncJob) Run() { f() }

// AddFunc adds a func to the Cron to be run on the given schedule.
func (c *Cron) AddFunc(spec string, cmd func()) {
c.AddJob(spec, FuncJob(cmd))
func (c *Cron) AddFunc(spec string, cmd func()) error {
return c.AddJob(spec, FuncJob(cmd))
}

// AddFunc adds a Job to the Cron to be run on the given schedule.
func (c *Cron) AddJob(spec string, cmd Job) {
c.Schedule(Parse(spec), cmd)
func (c *Cron) AddJob(spec string, cmd Job) error {
schedule, err := Parse(spec)
if err != nil {
return err
}
c.Schedule(schedule, cmd)
return nil
}

// Schedule adds a Job to the Cron to be run on the given schedule.
Expand Down
16 changes: 12 additions & 4 deletions parser.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cron

import (
"fmt"
"log"
"math"
"strconv"
Expand All @@ -9,14 +10,21 @@ import (
)

// Parse returns a new crontab schedule representing the given spec.
// It panics with a descriptive error if the spec is not valid.
// It returns a descriptive error if the spec is not valid.
//
// It accepts
// - Full crontab specs, e.g. "* * * * * ?"
// - Descriptors, e.g. "@midnight", "@every 1h30m"
func Parse(spec string) Schedule {
func Parse(spec string) (_ Schedule, err error) {
// Convert panics into errors
defer func() {
if recovered := recover(); recovered != nil {
err = fmt.Errorf("%v", recovered)
}
}()

if spec[0] == '@' {
return parseDescriptor(spec)
return parseDescriptor(spec), nil
}

// Split on whitespace. We require 5 or 6 fields.
Expand All @@ -40,7 +48,7 @@ func Parse(spec string) Schedule {
Dow: getField(fields[5], dow),
}

return schedule
return schedule, nil
}

// getField returns an Int with the bits set representing all of the times that
Expand Down
5 changes: 4 additions & 1 deletion parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,10 @@ func TestSpecSchedule(t *testing.T) {
}

for _, c := range entries {
actual := Parse(c.expr)
actual, err := Parse(c.expr)
if err != nil {
t.Error(err)
}
if !reflect.DeepEqual(actual, c.expected) {
t.Errorf("%s => (expected) %b != %b (actual)", c.expr, c.expected, actual)
}
Expand Down
29 changes: 27 additions & 2 deletions spec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,12 @@ func TestActivation(t *testing.T) {
}

for _, test := range tests {
actual := Parse(test.spec).Next(getTime(test.time).Add(-1 * time.Second))
sched, err := Parse(test.spec)
if err != nil {
t.Error(err)
continue
}
actual := sched.Next(getTime(test.time).Add(-1 * time.Second))
expected := getTime(test.time)
if test.expected && expected != actual || !test.expected && expected == actual {
t.Errorf("Fail evaluating %s on %s: (expected) %s != %s (actual)",
Expand Down Expand Up @@ -117,14 +122,34 @@ func TestNext(t *testing.T) {
}

for _, c := range runs {
actual := Parse(c.spec).Next(getTime(c.time))
sched, err := Parse(c.spec)
if err != nil {
t.Error(err)
continue
}
actual := sched.Next(getTime(c.time))
expected := getTime(c.expected)
if !actual.Equal(expected) {
t.Errorf("%s, \"%s\": (expected) %v != %v (actual)", c.time, c.spec, expected, actual)
}
}
}

func TestErrors(t *testing.T) {
invalidSpecs := []string{
"xyz",
"60 0 * * *",
"0 60 * * *",
"0 0 * * XYZ",
}
for _, spec := range invalidSpecs {
_, err := Parse(spec)
if err == nil {
t.Error("expected an error parsing: ", spec)
}
}
}

func getTime(value string) time.Time {
if value == "" {
return time.Time{}
Expand Down

0 comments on commit f2c3314

Please sign in to comment.