Refactor parsing to return error instead of using panic/recover

Currently the parser code relies on recover function to catch errors and
return them to the end user. The recommended go approach is to return
errors from functions and check them. This commit refactors all
log.Panicf calls to actually returning errors.

parser.go

@@ -2,7 +2,6 @@ package cron
 
 import (
 	"fmt"
-	"log"
 	"math"
 	"strconv"
 	"strings"
@@ -17,36 +16,46 @@ import (
 // It accepts
 //   - Standard crontab specs, e.g. "* * * * ?"
 //   - Descriptors, e.g. "@midnight", "@every 1h30m"
-func ParseStandard(standardSpec string) (_ Schedule, err error) {
-	// Convert panics into errors
-	defer func() {
-		if recovered := recover(); recovered != nil {
-			err = fmt.Errorf("%v", recovered)
-		}
-	}()
-
+func ParseStandard(standardSpec string) (Schedule, error) {
 	if standardSpec[0] == '@' {
-		return parseDescriptor(standardSpec), nil
+		return parseDescriptor(standardSpec)
 	}
 
 	// Split on whitespace.  We require exactly 5 fields.
 	// (minute) (hour) (day of month) (month) (day of week)
 	fields := strings.Fields(standardSpec)
 	if len(fields) != 5 {
-		log.Panicf("Expected exactly 5, found %d: %s", len(fields), standardSpec)
+		return nil, fmt.Errorf("Expected exactly 5 fields, found %d: %s", len(fields), standardSpec)
 	}
 
-	schedule := &SpecSchedule{
-		Second: 1 << seconds.min,
-		Minute: getField(fields[0], minutes),
-		Hour:   getField(fields[1], hours),
-		Dom:    getField(fields[2], dom),
-		Month:  getField(fields[3], months),
-		Dow:    getField(fields[4], dow),
+	var err error
+	field := func(field string, r bounds) uint64 {
+		if err != nil {
+			return uint64(0)
+		}
+		var bits uint64
+		bits, err = getField(field, r)
+		return bits
+	}
+	var (
+		minute     = field(fields[0], minutes)
+		hour       = field(fields[1], hours)
+		dayofmonth = field(fields[2], dom)
+		month      = field(fields[3], months)
+		dayofweek  = field(fields[4], dow)
+	)
+	if err != nil {
+		return nil, err
 	}
 
-	return schedule, nil
-
+	return &SpecSchedule{
+		Second: 1 << seconds.min,
+		Minute: minute,
+		Hour:   hour,
+		Dom:    dayofmonth,
+		Month:  month,
+		Dow:    dayofweek,
+	}, nil
 }
 
 // Parse returns a new crontab schedule representing the given spec.
@@ -55,23 +64,16 @@ func ParseStandard(standardSpec string) (_ Schedule, err error) {
 // It accepts
 //   - Full crontab specs, e.g. "* * * * * ?"
 //   - Descriptors, e.g. "@midnight", "@every 1h30m"
-func Parse(spec string) (_ Schedule, err error) {
-	// Convert panics into errors
-	defer func() {
-		if recovered := recover(); recovered != nil {
-			err = fmt.Errorf("%v", recovered)
-		}
-	}()
-
+func Parse(spec string) (Schedule, error) {
 	if spec[0] == '@' {
-		return parseDescriptor(spec), nil
+		return parseDescriptor(spec)
 	}
 
 	// Split on whitespace.  We require 5 or 6 fields.
 	// (second) (minute) (hour) (day of month) (month) (day of week, optional)
 	fields := strings.Fields(spec)
 	if len(fields) != 5 && len(fields) != 6 {
-		log.Panicf("Expected 5 or 6 fields, found %d: %s", len(fields), spec)
+		return nil, fmt.Errorf("Expected 5 or 6 fields, found %d: %s", len(fields), spec)
 	}
 
 	// If a sixth field is not provided (DayOfWeek), then it is equivalent to star.
@@ -79,39 +81,64 @@ func Parse(spec string) (_ Schedule, err error) {
 		fields = append(fields, "*")
 	}
 
-	schedule := &SpecSchedule{
-		Second: getField(fields[0], seconds),
-		Minute: getField(fields[1], minutes),
-		Hour:   getField(fields[2], hours),
-		Dom:    getField(fields[3], dom),
-		Month:  getField(fields[4], months),
-		Dow:    getField(fields[5], dow),
+	var err error
+	field := func(field string, r bounds) uint64 {
+		if err != nil {
+			return uint64(0)
+		}
+		var bits uint64
+		bits, err = getField(field, r)
+		return bits
+	}
+	var (
+		second     = field(fields[0], seconds)
+		minute     = field(fields[1], minutes)
+		hour       = field(fields[2], hours)
+		dayofmonth = field(fields[3], dom)
+		month      = field(fields[4], months)
+		dayofweek  = field(fields[5], dow)
+	)
+	if err != nil {
+		return nil, err
 	}
 
-	return schedule, nil
+	return &SpecSchedule{
+		Second: second,
+		Minute: minute,
+		Hour:   hour,
+		Dom:    dayofmonth,
+		Month:  month,
+		Dow:    dayofweek,
+	}, nil
 }
 
 // getField returns an Int with the bits set representing all of the times that
-// the field represents.  A "field" is a comma-separated list of "ranges".
-func getField(field string, r bounds) uint64 {
-	// list = range {"," range}
+// the field represents or error parsing field value.  A "field" is a comma-separated
+// list of "ranges".
+func getField(field string, r bounds) (uint64, error) {
 	var bits uint64
 	ranges := strings.FieldsFunc(field, func(r rune) bool { return r == ',' })
 	for _, expr := range ranges {
-		bits |= getRange(expr, r)
+		bit, err := getRange(expr, r)
+		if err != nil {
+			return bits, err
+		}
+		bits |= bit
 	}
-	return bits
+	return bits, nil
 }
 
 // getRange returns the bits indicated by the given expression:
 //   number | number "-" number [ "/" number ]
-func getRange(expr string, r bounds) uint64 {
-
+// or error parsing range.
+func getRange(expr string, r bounds) (uint64, error) {
 	var (
 		start, end, step uint
 		rangeAndStep     = strings.Split(expr, "/")
 		lowAndHigh       = strings.Split(rangeAndStep[0], "-")
 		singleDigit      = len(lowAndHigh) == 1
+		err              error
+		zero             = uint64(0)
 	)
 
 	var extra_star uint64
@@ -120,14 +147,20 @@ func getRange(expr string, r bounds) uint64 {
 		end = r.max
 		extra_star = starBit
 	} else {
-		start = parseIntOrName(lowAndHigh[0], r.names)
+		start, err = parseIntOrName(lowAndHigh[0], r.names)
+		if err != nil {
+			return zero, err
+		}
 		switch len(lowAndHigh) {
 		case 1:
 			end = start
 		case 2:
-			end = parseIntOrName(lowAndHigh[1], r.names)
+			end, err = parseIntOrName(lowAndHigh[1], r.names)
+			if err != nil {
+				return zero, err
+			}
 		default:
-			log.Panicf("Too many hyphens: %s", expr)
+			return zero, fmt.Errorf("Too many hyphens: %s", expr)
 		}
 	}
 
@@ -135,53 +168,56 @@ func getRange(expr string, r bounds) uint64 {
 	case 1:
 		step = 1
 	case 2:
-		step = mustParseInt(rangeAndStep[1])
+		step, err = mustParseInt(rangeAndStep[1])
+		if err != nil {
+			return zero, err
+		}
 
 		// Special handling: "N/step" means "N-max/step".
 		if singleDigit {
 			end = r.max
 		}
 	default:
-		log.Panicf("Too many slashes: %s", expr)
+		return zero, fmt.Errorf("Too many slashes: %s", expr)
 	}
 
 	if start < r.min {
-		log.Panicf("Beginning of range (%d) below minimum (%d): %s", start, r.min, expr)
+		return zero, fmt.Errorf("Beginning of range (%d) below minimum (%d): %s", start, r.min, expr)
 	}
 	if end > r.max {
-		log.Panicf("End of range (%d) above maximum (%d): %s", end, r.max, expr)
+		return zero, fmt.Errorf("End of range (%d) above maximum (%d): %s", end, r.max, expr)
 	}
 	if start > end {
-		log.Panicf("Beginning of range (%d) beyond end of range (%d): %s", start, end, expr)
+		return zero, fmt.Errorf("Beginning of range (%d) beyond end of range (%d): %s", start, end, expr)
 	}
 	if step == 0 {
-		log.Panicf("Step of range should be a positive number: %s", expr)
+		return zero, fmt.Errorf("Step of range should be a positive number: %s", expr)
 	}
 
-	return getBits(start, end, step) | extra_star
+	return getBits(start, end, step) | extra_star, nil
 }
 
 // parseIntOrName returns the (possibly-named) integer contained in expr.
-func parseIntOrName(expr string, names map[string]uint) uint {
+func parseIntOrName(expr string, names map[string]uint) (uint, error) {
 	if names != nil {
 		if namedInt, ok := names[strings.ToLower(expr)]; ok {
-			return namedInt
+			return namedInt, nil
 		}
 	}
 	return mustParseInt(expr)
 }
 
-// mustParseInt parses the given expression as an int or panics.
-func mustParseInt(expr string) uint {
+// mustParseInt parses the given expression as an int or returns an error.
+func mustParseInt(expr string) (uint, error) {
 	num, err := strconv.Atoi(expr)
 	if err != nil {
-		log.Panicf("Failed to parse int from %s: %s", expr, err)
+		return 0, fmt.Errorf("Failed to parse int from %s: %s", expr, err)
 	}
 	if num < 0 {
-		log.Panicf("Negative number (%d) not allowed: %s", num, expr)
+		return 0, fmt.Errorf("Negative number (%d) not allowed: %s", num, expr)
 	}
 
-	return uint(num)
+	return uint(num), nil
 }
 
 // getBits sets all bits in the range [min, max], modulo the given step size.
@@ -205,10 +241,9 @@ func all(r bounds) uint64 {
 	return getBits(r.min, r.max, 1) | starBit
 }
 
-// parseDescriptor returns a pre-defined schedule for the expression, or panics
-// if none matches.
-func parseDescriptor(spec string) Schedule {
-	switch spec {
+// parseDescriptor returns a predefined schedule for the expression, or error if none matches.
+func parseDescriptor(descriptor string) (Schedule, error) {
+	switch descriptor {
 	case "@yearly", "@annually":
 		return &SpecSchedule{
 			Second: 1 << seconds.min,
@@ -217,7 +252,7 @@ func parseDescriptor(spec string) Schedule {
 			Dom:    1 << dom.min,
 			Month:  1 << months.min,
 			Dow:    all(dow),
-		}
+		}, nil
 
 	case "@monthly":
 		return &SpecSchedule{
@@ -227,7 +262,7 @@ func parseDescriptor(spec string) Schedule {
 			Dom:    1 << dom.min,
 			Month:  all(months),
 			Dow:    all(dow),
-		}
+		}, nil
 
 	case "@weekly":
 		return &SpecSchedule{
@@ -237,7 +272,7 @@ func parseDescriptor(spec string) Schedule {
 			Dom:    all(dom),
 			Month:  all(months),
 			Dow:    1 << dow.min,
-		}
+		}, nil
 
 	case "@daily", "@midnight":
 		return &SpecSchedule{
@@ -247,7 +282,7 @@ func parseDescriptor(spec string) Schedule {
 			Dom:    all(dom),
 			Month:  all(months),
 			Dow:    all(dow),
-		}
+		}, nil
 
 	case "@hourly":
 		return &SpecSchedule{
@@ -257,18 +292,17 @@ func parseDescriptor(spec string) Schedule {
 			Dom:    all(dom),
 			Month:  all(months),
 			Dow:    all(dow),
-		}
+		}, nil
 	}
 
 	const every = "@every "
-	if strings.HasPrefix(spec, every) {
-		duration, err := time.ParseDuration(spec[len(every):])
+	if strings.HasPrefix(descriptor, every) {
+		duration, err := time.ParseDuration(descriptor[len(every):])
 		if err != nil {
-			log.Panicf("Failed to parse duration %s: %s", spec, err)
+			return nil, fmt.Errorf("Failed to parse duration %s: %s", descriptor, err)
 		}
-		return Every(duration)
+		return Every(duration), nil
 	}
 
-	log.Panicf("Unrecognized descriptor: %s", spec)
-	return nil
+	return nil, fmt.Errorf("Unrecognized descriptor: %s", descriptor)
 }

parser_test.go

@@ -2,36 +2,55 @@ package cron
 
 import (
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 )
 
 func TestRange(t *testing.T) {
+	zero := uint64(0)
 	ranges := []struct {
 		expr     string
 		min, max uint
 		expected uint64
+		err      string
 	}{
-		{"5", 0, 7, 1 << 5},
-		{"0", 0, 7, 1 << 0},
-		{"7", 0, 7, 1 << 7},
-
-		{"5-5", 0, 7, 1 << 5},
-		{"5-6", 0, 7, 1<<5 | 1<<6},
-		{"5-7", 0, 7, 1<<5 | 1<<6 | 1<<7},
-
-		{"5-6/2", 0, 7, 1 << 5},
-		{"5-7/2", 0, 7, 1<<5 | 1<<7},
-		{"5-7/1", 0, 7, 1<<5 | 1<<6 | 1<<7},
-
-		{"*", 1, 3, 1<<1 | 1<<2 | 1<<3 | starBit},
-		{"*/2", 1, 3, 1<<1 | 1<<3 | starBit},
+		{"5", 0, 7, 1 << 5, ""},
+		{"0", 0, 7, 1 << 0, ""},
+		{"7", 0, 7, 1 << 7, ""},
+
+		{"5-5", 0, 7, 1 << 5, ""},
+		{"5-6", 0, 7, 1<<5 | 1<<6, ""},
+		{"5-7", 0, 7, 1<<5 | 1<<6 | 1<<7, ""},
+
+		{"5-6/2", 0, 7, 1 << 5, ""},
+		{"5-7/2", 0, 7, 1<<5 | 1<<7, ""},
+		{"5-7/1", 0, 7, 1<<5 | 1<<6 | 1<<7, ""},
+
+		{"*", 1, 3, 1<<1 | 1<<2 | 1<<3 | starBit, ""},
+		{"*/2", 1, 3, 1<<1 | 1<<3 | starBit, ""},
+
+		{"5--5", 0, 0, zero, "Too many hyphens"},
+		{"jan-x", 0, 0, zero, "Failed to parse int from"},
+		{"2-x", 1, 5, zero, "Failed to parse int from"},
+		{"*/-12", 0, 0, zero, "Negative number"},
+		{"*//2", 0, 0, zero, "Too many slashes"},
+		{"1", 3, 5, zero, "below minimum"},
+		{"6", 3, 5, zero, "above maximum"},
+		{"5-3", 3, 5, zero, "beyond end of range"},
+		{"*/0", 0, 0, zero, "should be a positive number"},
 	}
 
 	for _, c := range ranges {
-		actual := getRange(c.expr, bounds{c.min, c.max, nil})
+		actual, err := getRange(c.expr, bounds{c.min, c.max, nil})
+		if len(c.err) != 0 && (err == nil || !strings.Contains(err.Error(), c.err)) {
+			t.Errorf("%s => expected %v, got %v", c.expr, c.err, err)
+		}
+		if len(c.err) == 0 && err != nil {
+			t.Error("%s => unexpected error %v", c.expr, err)
+		}
 		if actual != c.expected {
-			t.Errorf("%s => (expected) %d != %d (actual)", c.expr, c.expected, actual)
+			t.Errorf("%s => expected %d, got %d", c.expr, c.expected, actual)
 		}
 	}
 }
@@ -49,14 +68,14 @@ func TestField(t *testing.T) {
 	}
 
 	for _, c := range fields {
-		actual := getField(c.expr, bounds{c.min, c.max, nil})
+		actual, _ := getField(c.expr, bounds{c.min, c.max, nil})
 		if actual != c.expected {
-			t.Errorf("%s => (expected) %d != %d (actual)", c.expr, c.expected, actual)
+			t.Errorf("%s => expected %d, got %d", c.expr, c.expected, actual)
 		}
 	}
 }
 
-func TestBits(t *testing.T) {
+func TestAll(t *testing.T) {
 	allBits := []struct {
 		r        bounds
 		expected uint64
@@ -71,16 +90,17 @@ func TestBits(t *testing.T) {
 	for _, c := range allBits {
 		actual := all(c.r) // all() adds the starBit, so compensate for that..
 		if c.expected|starBit != actual {
-			t.Errorf("%d-%d/%d => (expected) %b != %b (actual)",
+			t.Errorf("%d-%d/%d => expected %b, got %b",
 				c.r.min, c.r.max, 1, c.expected|starBit, actual)
 		}
 	}
+}
 
+func TestBits(t *testing.T) {
 	bits := []struct {
 		min, max, step uint
 		expected       uint64
 	}{
-
 		{0, 0, 1, 0x1},
 		{1, 1, 1, 0x2},
 		{1, 5, 2, 0x2a}, // 101010
@@ -90,28 +110,83 @@ func TestBits(t *testing.T) {
 	for _, c := range bits {
 		actual := getBits(c.min, c.max, c.step)
 		if c.expected != actual {
-			t.Errorf("%d-%d/%d => (expected) %b != %b (actual)",
+			t.Errorf("%d-%d/%d => expected %b, got %b",
 				c.min, c.max, c.step, c.expected, actual)
 		}
 	}
 }
 
-func TestSpecSchedule(t *testing.T) {
+func TestParse(t *testing.T) {
 	entries := []struct {
 		expr     string
 		expected Schedule
+		err      string
 	}{
-		{"* 5 * * * *", &SpecSchedule{all(seconds), 1 << 5, all(hours), all(dom), all(months), all(dow)}},
-		{"@every 5m", ConstantDelaySchedule{time.Duration(5) * time.Minute}},
+		{
+			expr: "* 5 * * * *",
+			expected: &SpecSchedule{
+				Second: all(seconds),
+				Minute: 1 << 5,
+				Hour:   all(hours),
+				Dom:    all(dom),
+				Month:  all(months),
+				Dow:    all(dow),
+			},
+		},
+		{
+			expr: "* 5 j * * *",
+			err:  "Failed to parse int from",
+		},
+		{
+			expr:     "@every 5m",
+			expected: ConstantDelaySchedule{Delay: time.Duration(5) * time.Minute},
+		},
+		{
+			expr: "@every Xm",
+			err:  "Failed to parse duration",
+		},
+		{
+			expr: "@yearly",
+			expected: &SpecSchedule{
+				Second: 1 << seconds.min,
+				Minute: 1 << minutes.min,
+				Hour:   1 << hours.min,
+				Dom:    1 << dom.min,
+				Month:  1 << months.min,
+				Dow:    all(dow),
+			},
+		},
+		{
+			expr: "@annually",
+			expected: &SpecSchedule{
+				Second: 1 << seconds.min,
+				Minute: 1 << minutes.min,
+				Hour:   1 << hours.min,
+				Dom:    1 << dom.min,
+				Month:  1 << months.min,
+				Dow:    all(dow),
+			},
+		},
+		{
+			expr: "@unrecognized",
+			err:  "Unrecognized descriptor",
+		},
+		{
+			expr: "* * * *",
+			err:  "Expected 5 or 6 fields",
+		},
 	}
 
 	for _, c := range entries {
 		actual, err := Parse(c.expr)
-		if err != nil {
-			t.Error(err)
+		if len(c.err) != 0 && (err == nil || !strings.Contains(err.Error(), c.err)) {
+			t.Error("%s => expected %v, got %v", c.expr, c.err, err)
+		}
+		if len(c.err) == 0 && err != nil {
+			t.Error("%s => unexpected error %v", c.expr, err)
 		}
 		if !reflect.DeepEqual(actual, c.expected) {
-			t.Errorf("%s => (expected) %b != %b (actual)", c.expr, c.expected, actual)
+			t.Errorf("%s => expected %b, got %b", c.expr, c.expected, actual)
 		}
 	}
 }
@@ -120,18 +195,36 @@ func TestStandardSpecSchedule(t *testing.T) {
 	entries := []struct {
 		expr     string
 		expected Schedule
+		err      string
 	}{
-		{"5 * * * *", &SpecSchedule{1 << seconds.min, 1 << 5, all(hours), all(dom), all(months), all(dow)}},
-		{"@every 5m", ConstantDelaySchedule{time.Duration(5) * time.Minute}},
+		{
+			expr:     "5 * * * *",
+			expected: &SpecSchedule{1 << seconds.min, 1 << 5, all(hours), all(dom), all(months), all(dow)},
+		},
+		{
+			expr:     "@every 5m",
+			expected: ConstantDelaySchedule{time.Duration(5) * time.Minute},
+		},
+		{
+			expr: "5 j * * *",
+			err:  "Failed to parse int from",
+		},
+		{
+			expr: "* * * *",
+			err:  "Expected exactly 5 fields",
+		},
 	}
 
 	for _, c := range entries {
 		actual, err := ParseStandard(c.expr)
-		if err != nil {
-			t.Error(err)
+		if len(c.err) != 0 && (err == nil || !strings.Contains(err.Error(), c.err)) {
+			t.Error("%s => expected %v, got %v", c.expr, c.err, err)
+		}
+		if len(c.err) == 0 && err != nil {
+			t.Error("%s => unexpected error %v", c.expr, err)
 		}
 		if !reflect.DeepEqual(actual, c.expected) {
-			t.Errorf("%s => (expected) %b != %b (actual)", c.expr, c.expected, actual)
+			t.Errorf("%s => expected %b, got %b", c.expr, c.expected, actual)
 		}
 	}
 }