Merge pull request #74 from soltysh/issue33

Refactor parsing to return error instead of using panic/recover

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)
 		}
 	}
 }