Merge pull request #341 from bookingcom/carbonapi/from-until-times-ve…

…rification

Carbonapi/from until times verification

Makefile

@@ -41,7 +41,7 @@ lint:
 check: test vet
 
 test:
-	$(PKGCONF) $(GO) test ./... -race -coverprofile=coverage.txt -covermode=atomic
+	$(PKGCONF) $(GO) test ./... -v -race -coverprofile=coverage.txt -covermode=atomic
 
 clean:
 	rm -f carbonapi carbonzipper

app/carbonapi/http_handlers.go

@@ -188,8 +188,18 @@ func (app *App) renderHandler(w http.ResponseWriter, r *http.Request) {
 		logAsError = true
 		return
 	}
-	if form.from32 == form.until32 {
-		writeError(uuid, r, w, http.StatusBadRequest, "Invalid empty time range", form.format, &toLog, span)
+
+	if form.from32 >= form.until32 {
+		var clientErrMsgFmt string
+		if form.from32 == form.until32 {
+			clientErrMsgFmt = "parameter from=%s has the same value as parameter until=%s. Result time range is empty"
+		} else {
+			clientErrMsgFmt = "parameter from=%s greater than parameter until=%s. Result time range is empty"
+		}
+		clientErrMsg := fmt.Sprintf(clientErrMsgFmt, form.from, form.until)
+		writeError(uuid, r, w, http.StatusBadRequest, clientErrMsg, form.format, &toLog, span)
+		toLog.HttpCode = http.StatusBadRequest
+		toLog.Reason = "invalid empty time range"
 		logAsError = true
 		return
 	}
@@ -607,8 +617,9 @@ func (app *App) renderHandlerProcessForm(r *http.Request, accessLogDetails *carb
 
 	// normalize from and until values
 	res.qtz = r.FormValue("tz")
-	res.from32 = date.DateParamToEpoch(res.from, res.qtz, timeNow().Add(-24*time.Hour).Unix(), app.defaultTimeZone)
-	res.until32 = date.DateParamToEpoch(res.until, res.qtz, timeNow().Unix(), app.defaultTimeZone)
+	var errFrom, errUntil error
+	res.from32, errFrom = date.DateParamToEpoch(res.from, res.qtz, timeNow().Add(-24*time.Hour).Unix(), app.defaultTimeZone)
+	res.until32, errUntil = date.DateParamToEpoch(res.until, res.qtz, timeNow().Unix(), app.defaultTimeZone)
 
 	accessLogDetails.UseCache = res.useCache
 	accessLogDetails.FromRaw = res.from
@@ -632,6 +643,14 @@ func (app *App) renderHandlerProcessForm(r *http.Request, accessLogDetails *carb
 		kv.String("graphite.format", res.format),
 	)
 
+	if errFrom != nil || errUntil != nil {
+		errFmt := "%s, invalid parameter %s=%s"
+		if errFrom != nil {
+			return res, fmt.Errorf(errFmt, errFrom.Error(), "from", res.from)
+		}
+		return res, fmt.Errorf(errFmt, errUntil.Error(), "until", res.until)
+	}
+
 	return res, nil
 }
 

date/date.go

@@ -9,7 +9,10 @@ import (
 	"github.com/bookingcom/carbonapi/pkg/parser"
 )
 
-var errBadTime = errors.New("bad time")
+var errBadTime = errors.New("time has incorrect format")
+var errBadRelativeTime = errors.New("invalid relative timestamp")
+var errTsPartsCount = errors.New("timestamp has too many parts")
+var errDateFormat = errors.New("invalid date format")
 var timeNow = time.Now
 
 // parseTime parses a time and returns hours and minutes
@@ -46,37 +49,37 @@ func parseTime(s string) (hour, minute int, err error) {
 var TimeFormats = []string{"20060102", "01/02/06"}
 
 // DateParamToEpoch turns a passed string parameter into a unix epoch
-func DateParamToEpoch(s string, qtz string, d int64, defaultTimeZone *time.Location) int32 {
+func DateParamToEpoch(s string, qtz string, d int64, defaultTimeZone *time.Location) (int32, error) {
 
 	if s == "" {
 		// return the default if nothing was passed
-		return int32(d)
+		return int32(d), nil
 	}
 
 	// relative timestamp
 	if s[0] == '-' {
 		offset, err := parser.IntervalString(s, -1)
 		if err != nil {
-			return int32(d)
+			return 0, errBadRelativeTime
 		}
 
-		return int32(timeNow().Add(time.Duration(offset) * time.Second).Unix())
+		return int32(timeNow().Add(time.Duration(offset) * time.Second).Unix()), nil
 	}
 
 	switch s {
 	case "now":
-		return int32(timeNow().Unix())
+		return int32(timeNow().Unix()), nil
 	case "midnight", "noon", "teatime":
 		yy, mm, dd := timeNow().Date()
 		hh, min, _ := parseTime(s) // error ignored, we know it's valid
 		dt := time.Date(yy, mm, dd, hh, min, 0, 0, defaultTimeZone)
-		return int32(dt.Unix())
+		return int32(dt.Unix()), nil
 	}
 
 	sint, err := strconv.ParseInt(s, 10, 64)
 	// need to check that len(s) > 8 to avoid turning 20060102 into seconds
 	if err == nil && len(s) > 8 {
-		return int32(sint) // We got a timestamp so returning it
+		return int32(sint), nil // We got a timestamp so returning it
 	}
 
 	s = strings.Replace(s, "_", " ", 1) // Go can't parse _ in date strings
@@ -90,7 +93,7 @@ func DateParamToEpoch(s string, qtz string, d int64, defaultTimeZone *time.Locat
 	case len(split) == 2:
 		ts, ds = split[0], split[1]
 	case len(split) > 2:
-		return int32(d)
+		return 0, errTsPartsCount
 	}
 
 	var tz = defaultTimeZone
@@ -118,17 +121,20 @@ dateStringSwitch:
 			}
 		}
 
-		return int32(d)
+		return 0, errDateFormat
 	}
 
 	var hour, minute int
+	var parseErr error
 	if ts != "" {
-		hour, minute, _ = parseTime(ts)
-		// defaults to hour=0, minute=0 on error, which is midnight, which is fine for now
+		hour, minute, parseErr = parseTime(ts)
+		if parseErr != nil {
+			return 0, parseErr
+		}
 	}
 
 	yy, mm, dd := t.Date()
 	t = time.Date(yy, mm, dd, hour, minute, 0, 0, defaultTimeZone)
 
-	return int32(t.Unix())
+	return int32(t.Unix()), nil
 }

date/date_test.go

@@ -7,43 +7,67 @@ import (
 )
 
 func TestDateParamToEpoch(t *testing.T) {
-
 	defaultTimeZone := time.Local
 	timeNow = func() time.Time {
 		//16 Aug 1994 15:30
 		return time.Date(1994, time.August, 16, 15, 30, 0, 100, defaultTimeZone)
 	}
 
 	const shortForm = "15:04 2006-Jan-02"
+	const defaultTsStr = "17:30 2019-Apr-25"
 
 	var tests = []struct {
 		input  string
 		output string
+		error  bool
 	}{
-		{"midnight", "00:00 1994-Aug-16"},
-		{"noon", "12:00 1994-Aug-16"},
-		{"teatime", "16:00 1994-Aug-16"},
-		{"tomorrow", "00:00 1994-Aug-17"},
-
-		{"noon 08/12/94", "12:00 1994-Aug-12"},
-		{"midnight 20060812", "00:00 2006-Aug-12"},
-		{"noon tomorrow", "12:00 1994-Aug-17"},
-
-		{"17:04 19940812", "17:04 1994-Aug-12"},
-		{"-1day", "15:30 1994-Aug-15"},
-		{"19940812", "00:00 1994-Aug-12"},
+		{"midnight", "00:00 1994-Aug-16", false},
+		{"noon", "12:00 1994-Aug-16", false},
+		{"teatime", "16:00 1994-Aug-16", false},
+		{"tomorrow", "00:00 1994-Aug-17", false},
+
+		{"noon 08/12/94", "12:00 1994-Aug-12", false},
+		{"midnight 20060812", "00:00 2006-Aug-12", false},
+		{"noon tomorrow", "12:00 1994-Aug-17", false},
+
+		{"17:04 19940812", "17:04 1994-Aug-12", false},
+		{"-1day", "15:30 1994-Aug-15", false},
+		{"19940812", "00:00 1994-Aug-12", false},
+		{"now", "15:30 1994-Aug-16", false},
+		{"hh:mm 19940812", "00:00 1994-Aug-12", true},
+		{"12:30:00 19940812", "00:00 1994-Aug-12", true},
+		{"12:mm 19940812", "00:00 1994-Aug-12", true},
+		{"today", "00:00 1994-Aug-16", false},
+		{"yesterday", "00:00 1994-Aug-15", false},
+		{"1556201160", time.Unix(1556201160, 0).Format(shortForm), false}, // time.Unix returns a local time
+		{"", defaultTsStr, false},
+		{"-something", defaultTsStr, true},
+		{"17:04 19940812 1001", defaultTsStr, true},
+		{"12:30 08/15/06", "12:30 2006-Aug-15", false},
+		{"12:30 08-15-06", defaultTsStr, true},
+		{"08/15/06 12:30", defaultTsStr, true},
+		{"+5m", defaultTsStr, true},
 	}
 
+	defaultTime, _ := time.ParseInLocation(shortForm, defaultTsStr, defaultTimeZone)
+	defaultTs := defaultTime.Unix()
+
 	for _, tt := range tests {
-		got := DateParamToEpoch(tt.input, "Local", 0, defaultTimeZone)
+		got, parseErr := DateParamToEpoch(tt.input, "Local", defaultTs, defaultTimeZone)
 		ts, err := time.ParseInLocation(shortForm, tt.output, defaultTimeZone)
 		if err != nil {
 			panic(fmt.Sprintf("error parsing time: %q: %v", tt.output, err))
 		}
+		if (tt.error && parseErr == nil) || (!tt.error && parseErr != nil) {
+			t.Errorf("dateParamToEpoch(%q, 0)=%v, want error: %v", tt.input, got, tt.error)
+			continue
+		}
 
-		want := int32(ts.Unix())
-		if got != want {
-			t.Errorf("dateParamToEpoch(%q, 0)=%v, want %v", tt.input, got, want)
+		if !tt.error {
+			want := int32(ts.Unix())
+			if got != want {
+				t.Errorf("dateParamToEpoch(%q, 0)=%v, want %v", tt.input, got, want)
+			}
 		}
 	}
 }