From 217da681f7b8191f5ab398be720f7d4adb2974fd Mon Sep 17 00:00:00 2001 From: "vladimir.pestov" Date: Tue, 10 Sep 2019 12:16:56 +0200 Subject: [PATCH 1/6] specify empty time range more precisely --- app/carbonapi/http_handlers.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/carbonapi/http_handlers.go b/app/carbonapi/http_handlers.go index e266a638..711ac735 100644 --- a/app/carbonapi/http_handlers.go +++ b/app/carbonapi/http_handlers.go @@ -188,8 +188,11 @@ 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 { + writeError(ctx, r, w, http.StatusBadRequest, "Invalid empty time range", form) + toLog.HttpCode = http.StatusBadRequest + toLog.Reason = "invalid empty time range" logAsError = true return } From f57ce8d08f08f6d5b6d7c04e33fd0e7f3c949986 Mon Sep 17 00:00:00 2001 From: "vladimir.pestov" Date: Tue, 10 Sep 2019 14:00:01 +0200 Subject: [PATCH 2/6] added more tests for date converter --- date/date_test.go | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/date/date_test.go b/date/date_test.go index 1c719f15..233197d1 100644 --- a/date/date_test.go +++ b/date/date_test.go @@ -15,6 +15,7 @@ func TestDateParamToEpoch(t *testing.T) { } const shortForm = "15:04 2006-Jan-02" + const defaultTsStr = "17:30 2019-Apr-25" var tests = []struct { input string @@ -32,10 +33,27 @@ func TestDateParamToEpoch(t *testing.T) { {"17:04 19940812", "17:04 1994-Aug-12"}, {"-1day", "15:30 1994-Aug-15"}, {"19940812", "00:00 1994-Aug-12"}, + {"now", "15:30 1994-Aug-16"}, + {"hh:mm 19940812", "00:00 1994-Aug-12"}, + {"12:30:00 19940812", "00:00 1994-Aug-12"}, + {"12:mm 19940812", "00:00 1994-Aug-12"}, + {"today", "00:00 1994-Aug-16"}, + {"yesterday", "00:00 1994-Aug-15"}, + {"1556201160", "16:06 2019-Apr-25"}, + {"", defaultTsStr}, + {"-something", defaultTsStr}, + {"17:04 19940812 1001", defaultTsStr}, + {"12:30 08/15/06", "12:30 2006-Aug-15"}, + {"12:30 08-15-06", defaultTsStr}, + {"08/15/06 12:30", defaultTsStr}, + {"+5m", defaultTsStr}, } + defaultTime, _ := time.ParseInLocation(shortForm, defaultTsStr, defaultTimeZone) + defaultTs := defaultTime.Unix() + for _, tt := range tests { - got := DateParamToEpoch(tt.input, "Local", 0, defaultTimeZone) + got := 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)) From 8431907c8daf8d5a162de9a5e583a3ba1e2a704e Mon Sep 17 00:00:00 2001 From: "vladimir.pestov" Date: Tue, 10 Sep 2019 15:34:50 +0200 Subject: [PATCH 3/6] initial implementation of time restriction --- date/date.go | 27 +++++++++++--------- date/date_test.go | 63 ++++++++++++++++++++++++++--------------------- 2 files changed, 50 insertions(+), 40 deletions(-) diff --git a/date/date.go b/date/date.go index 5bf93ff6..8d68c466 100644 --- a/date/date.go +++ b/date/date.go @@ -46,37 +46,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, errBadTime } - 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 +90,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, errBadTime } var tz = defaultTimeZone @@ -118,17 +118,20 @@ dateStringSwitch: } } - return int32(d) + return 0, errBadTime } 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 } diff --git a/date/date_test.go b/date/date_test.go index 233197d1..cc7c291e 100644 --- a/date/date_test.go +++ b/date/date_test.go @@ -20,48 +20,55 @@ func TestDateParamToEpoch(t *testing.T) { 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"}, + {"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"}, - {"midnight 20060812", "00:00 2006-Aug-12"}, - {"noon tomorrow", "12:00 1994-Aug-17"}, + {"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"}, - {"-1day", "15:30 1994-Aug-15"}, - {"19940812", "00:00 1994-Aug-12"}, - {"now", "15:30 1994-Aug-16"}, - {"hh:mm 19940812", "00:00 1994-Aug-12"}, - {"12:30:00 19940812", "00:00 1994-Aug-12"}, - {"12:mm 19940812", "00:00 1994-Aug-12"}, - {"today", "00:00 1994-Aug-16"}, - {"yesterday", "00:00 1994-Aug-15"}, - {"1556201160", "16:06 2019-Apr-25"}, - {"", defaultTsStr}, - {"-something", defaultTsStr}, - {"17:04 19940812 1001", defaultTsStr}, - {"12:30 08/15/06", "12:30 2006-Aug-15"}, - {"12:30 08-15-06", defaultTsStr}, - {"08/15/06 12:30", defaultTsStr}, - {"+5m", defaultTsStr}, + {"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", "16:06 2019-Apr-25", false}, + {"", 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", defaultTs, 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) + } } } } From f230e5d6b3b4c21da21f2643c3c4ea1386e71b0b Mon Sep 17 00:00:00 2001 From: "vladimir.pestov" Date: Wed, 11 Sep 2019 10:57:21 +0200 Subject: [PATCH 4/6] added detailed error messages --- app/carbonapi/http_handlers.go | 22 +++++++++++++++++++--- date/date.go | 11 +++++++---- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/app/carbonapi/http_handlers.go b/app/carbonapi/http_handlers.go index 711ac735..8a832b6a 100644 --- a/app/carbonapi/http_handlers.go +++ b/app/carbonapi/http_handlers.go @@ -190,7 +190,14 @@ func (app *App) renderHandler(w http.ResponseWriter, r *http.Request) { } if form.from32 >= form.until32 { - writeError(ctx, r, w, http.StatusBadRequest, "Invalid empty time range", form) + 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 @@ -610,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 @@ -635,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, errors.New(fmt.Sprintf(errFmt, errFrom.Error(), "from", res.from)) + } + return res, errors.New(fmt.Sprintf(errFmt, errUntil.Error(), "until", res.until)) + } + return res, nil } diff --git a/date/date.go b/date/date.go index 8d68c466..bf08be06 100644 --- a/date/date.go +++ b/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 @@ -57,7 +60,7 @@ func DateParamToEpoch(s string, qtz string, d int64, defaultTimeZone *time.Locat if s[0] == '-' { offset, err := parser.IntervalString(s, -1) if err != nil { - return 0, errBadTime + return 0, errBadRelativeTime } return int32(timeNow().Add(time.Duration(offset) * time.Second).Unix()), nil @@ -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 0, errBadTime + return 0, errTsPartsCount } var tz = defaultTimeZone @@ -118,7 +121,7 @@ dateStringSwitch: } } - return 0, errBadTime + return 0, errDateFormat } var hour, minute int From 1dd0b2ddebe0d8b47eb11e9ae73c964f6e381e02 Mon Sep 17 00:00:00 2001 From: "vladimir.pestov" Date: Thu, 12 Sep 2019 22:21:21 +0200 Subject: [PATCH 5/6] codestyle violation fix --- app/carbonapi/http_handlers.go | 4 ++-- date/date.go | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/carbonapi/http_handlers.go b/app/carbonapi/http_handlers.go index 8a832b6a..adc0aed2 100644 --- a/app/carbonapi/http_handlers.go +++ b/app/carbonapi/http_handlers.go @@ -192,9 +192,9 @@ func (app *App) renderHandler(w http.ResponseWriter, r *http.Request) { 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" + 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" + 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) diff --git a/date/date.go b/date/date.go index bf08be06..662bf90e 100644 --- a/date/date.go +++ b/date/date.go @@ -9,10 +9,10 @@ import ( "github.com/bookingcom/carbonapi/pkg/parser" ) -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 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 From e6349179172140488aa83b9e2e0ced60e08122c6 Mon Sep 17 00:00:00 2001 From: Xiaofan Hu Date: Fri, 5 Nov 2021 16:48:52 +0100 Subject: [PATCH 6/6] carbonapi: linter and test case fixes * Fix a date test case of unix timestamp * Fix some linter errors of fmt.Errorf * Make test output verbose --- Makefile | 2 +- app/carbonapi/http_handlers.go | 4 ++-- date/date_test.go | 3 +-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index 58f58898..d5bfbda3 100644 --- a/Makefile +++ b/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 diff --git a/app/carbonapi/http_handlers.go b/app/carbonapi/http_handlers.go index adc0aed2..db1bf90f 100644 --- a/app/carbonapi/http_handlers.go +++ b/app/carbonapi/http_handlers.go @@ -646,9 +646,9 @@ func (app *App) renderHandlerProcessForm(r *http.Request, accessLogDetails *carb if errFrom != nil || errUntil != nil { errFmt := "%s, invalid parameter %s=%s" if errFrom != nil { - return res, errors.New(fmt.Sprintf(errFmt, errFrom.Error(), "from", res.from)) + return res, fmt.Errorf(errFmt, errFrom.Error(), "from", res.from) } - return res, errors.New(fmt.Sprintf(errFmt, errUntil.Error(), "until", res.until)) + return res, fmt.Errorf(errFmt, errUntil.Error(), "until", res.until) } return res, nil diff --git a/date/date_test.go b/date/date_test.go index cc7c291e..18017f4a 100644 --- a/date/date_test.go +++ b/date/date_test.go @@ -7,7 +7,6 @@ import ( ) func TestDateParamToEpoch(t *testing.T) { - defaultTimeZone := time.Local timeNow = func() time.Time { //16 Aug 1994 15:30 @@ -40,7 +39,7 @@ func TestDateParamToEpoch(t *testing.T) { {"12:mm 19940812", "00:00 1994-Aug-12", true}, {"today", "00:00 1994-Aug-16", false}, {"yesterday", "00:00 1994-Aug-15", false}, - {"1556201160", "16:06 2019-Apr-25", 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},