Added more strict validation on from/until parameters in api

[KozzyKoder]

Hello, this PR relates to this issue:
#171

Idea is to find to extend test coverage and find permissive validation rules and restrict them by returning an error instead of default value.

[KozzyKoder]

@azhiltsov also some things might be good to think about:

1. checked how wrong intervals processed at graphite python. Seems it accepts now only from=until, but also until < from. In latter case it looks like it swaps from <-> until. Not intuitive behaviour, is it fine that we restrict this behaviour?

2. added more restrictions on formats itself:

a) "hh:mm 19940812", "12:30:00 19940812", "12:mm 19940812" - similar inputs with incorrect time now returns an error instead 00:00 in current implementation

b) "-something" - completely incorrect relative times also returns error

c)"17:04 19940812 1001" - more components than required now also results in error

d)"12:30 08-15-06" - date with wrong delimiter returns error

e)"08/15/06 12:30" - swapped date and time also considered as error

f)"+5m" - relative dates in future not allowed also

---

previously cases described above were returning default value(from - 24hours back, until -now)

What do you think about these cases? Shall we relax some of these restrictions?

[azhiltsov]

Original goal of this project was to implement the same functionality as graphite-web.
We were pursuing this goal for a while by even reproducing some behaviors which might be perceived as buggy.
I do not have strong opinion about behavior we expect from time parsing. The only thing we should cut is ambiguous parameters. If you can easily distinguish 12:30 08-15-06 from 08-15-06 12:30 then I don't see a reason to block this. Grafana for instance performs conversion internally and uses epoch notation, so using extending parsing is mostly targeting human interaction. Most probably graphite-web is using some of human readable notations, and I would call @deniszh to let us know if we are about to brake it.
@gksinghjsr and @grzkv what is your take on this?
I would propose to use "keep what make sense" approach and what does not (but presented in graphite-web) can be opt_in with compatibility config flag.
My vision that carbonapi shouldn't lock-in on whisper as back-end and if so then in some other back-ends queries in the future for instance could make sense.

[grzkv]

I don't think we should closely follow python implementation in everything, only in parts where it makes sense. The strong criterion is: Grafana should always work well. As @azhiltsov has described, it uses Unix time so it should be ok.

I think we should drop the behaviour that is confusing or just wrong. Specific examples:

• until==from does not make any sense
• until < from is the clearly a user input mistake, treating this as an OK request is very misleading
• +5m is user input error
• -something_that_is_not_time is user error

@KozzyKoder To sum, I agree with the points a, b, c, e, f. I think, d is debatable, but I rather agree as well.

[ksurent]

It’s already there: https://github.com/golang/go/wiki/CodeReviewComments#error-strings.

…

On Thu, 12 Sep 2019 at 13:45, Alexey Zhiltsov ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In date/date.go <#200 (comment)>: > @@ -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") By convention, error strings start with a lowercase letter. Is there a good documentation about this? Can we mention it in https://github.com/bookingcom/carbonapi/blob/master/CONTRIBUTING.md ? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#200?email_source=notifications&email_token=AAALXX4AJBIY2JJFH7XLQY3QJITUVA5CNFSM4IVG2KT2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEQOPZY#discussion_r323695019>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAALXXZPZE643SQMCMIRBF3QJITUVANCNFSM4IVG2KTQ> .

[grzkv]

Hi all. There are a lot of valuable contributions in this PR. How about we move this completion?

[grzkv]

@azhiltsov @avereha @grlvrl @KozzyKoder Let's finalize this. We either move on or close.

Taking into account the age and controversy, I suggest we close this.