From de97eacdbcaf5c1abb94f54f9a5a59e584c0f9f4 Mon Sep 17 00:00:00 2001 From: Robin Bryce Date: Mon, 11 Nov 2024 15:00:20 +0000 Subject: [PATCH] fix: horizon commanline option parse issues * explicitly error for negative horizons * clamp to avoid over flowing idtimestamp arithmetic * force to largest supported value for range errors, this is fail safe. --- tests/watch/watch_test.go | 49 +++++++++++++++++++++++++++++++ watch.go | 62 +++++++++++++++++++++++++++++++++++---- watch_test.go | 48 +++++++++++++++--------------- 3 files changed, 131 insertions(+), 28 deletions(-) diff --git a/tests/watch/watch_test.go b/tests/watch/watch_test.go index 86f34e0..f086f6e 100644 --- a/tests/watch/watch_test.go +++ b/tests/watch/watch_test.go @@ -6,6 +6,55 @@ import ( "github.com/datatrails/veracity" ) +func (s *WatchCmdSuite) TestErrorForNegativeHorizon() { + + app := veracity.NewApp("version", false) + veracity.AddCommands(app, false) + + err := app.Run([]string{ + "veracity", + "--data-url", s.Env.VerifiableDataURL, + "watch", + "--horizon", "-1h", + }) + s.ErrorContains(err, "negative horizon") + // alternative approach which just makes the error more readable + // s.ErrorContains(err, "is to large or otherwise out of range") +} + +func (s *WatchCmdSuite) TestNoErrorVeryLargeHorizon() { + + app := veracity.NewApp("version", false) + veracity.AddCommands(app, false) + + err := app.Run([]string{ + "veracity", + "--data-url", s.Env.VerifiableDataURL, + "watch", + "--horizon", "1000000000h", + //"--horizon", "100000h", // 11 years, so we are sure we look back far enough to find an event + }) + s.NoError(err) + // alternative approach which just makes the error more readable + // s.ErrorContains(err, "is out of range or otherwise invalid") +} + +func (s *WatchCmdSuite) TestNoErrorLargeButParsableHorizon() { + + app := veracity.NewApp("version", false) + veracity.AddCommands(app, false) + + err := app.Run([]string{ + "veracity", + "--data-url", s.Env.VerifiableDataURL, + "watch", + "--horizon", "1000000h", // over flows the id timestamp epoch + }) + s.NoError(err) + // alternative approach which just makes the error more readable + // s.ErrorContains(err, "is out of range or otherwise invalid") +} + func (s *WatchCmdSuite) TestNoErrorOrNoChanges() { app := veracity.NewApp("version", false) diff --git a/watch.go b/watch.go index 0cedf8e..b8e6d29 100644 --- a/watch.go +++ b/watch.go @@ -29,6 +29,8 @@ const ( // maxPollCount is the maximum number of times to poll for *some* activity. // Polling always terminates as soon as the first activity is detected. maxPollCount = 15 + // More than this over flows the epoch which is half the length of the unix time epoch + maxHorizon = time.Hour * 100000 ) var ( @@ -78,11 +80,11 @@ func NewLogWatcherCmd() *cli.Command { Name: "idsince", Aliases: []string{"s"}, Usage: "Start time as an idtimestamp. Start time defaults to now. All results are >= this hex string. If provided, it is used exactly as is. Takes precedence over since", }, - &cli.DurationFlag{ + &cli.StringFlag{ Name: "horizon", Aliases: []string{"z"}, - Value: time.Hour * 24, - Usage: "Infer since as now - horizon, aka 1h to onl see things in the last hour. If watching (count=0), since is re-calculated every interval", + Value: "24s", + Usage: "Infer since as now - horizon. The format is {number}{units} eg 1h to only see things in the last hour. If watching (count=0), since is re-calculated every interval", }, &cli.DurationFlag{ Name: "interval", Aliases: []string{"d"}, @@ -124,18 +126,68 @@ type cliContext interface { Int(name string) int } +const ( + rangeDurationParseErrorSubString = "time: invalid duration " +) + +// parseHorizon parses a duration string from the command line In accordance +// with the most common reason for parse failure (specifying a large number), On +// an error that looks like a range to large issue, we coerce to the maximum +// hours and ignore the error. Errors that don't contain the marker substring +// are returned as is. +func parseHorizon(horizon string) (time.Duration, error) { + d, err := time.ParseDuration(horizon) + if err == nil { + + // clamp the horizon, otherwise it may overflow the idtimestamp epoch + if d > maxHorizon { + return maxHorizon, nil + } + if d < 0 { + return 0, fmt.Errorf("negative horizon value:%s", horizon) + } + + return d, nil + } + + // Note: it is a matter of opinion whether we should error here or not. + // Since finding many tenants is only a performance issue, we simply + // force the maximum range of hours on an error that indicates a range + // issue. By far the most common use for a large value is "just give me everything" + // The substring was obtained by code inspection of the time.ParseDuration implementation + if strings.HasPrefix(err.Error(), rangeDurationParseErrorSubString) { + return maxHorizon, nil + } + + // Alternative which accurately reports the error but is likely just as inconvenient + // if strings.HasPrefix(err.Error(), rangeDurationParseErrorSubString) { + // return WatchConfig{}, fmt.Errorf("the horizon '%s' is to large or otherwise out of range", horizon) + // } + + return d, fmt.Errorf("the horizon '%s' is out of range or otherwise invalid", horizon) +} + // NewWatchConfig derives a configuration from the options set on the command line context func NewWatchConfig(cCtx cliContext, cmd *CmdCtx) (WatchConfig, error) { + var err error + cfg := WatchConfig{} cfg.Interval = cCtx.Duration("interval") - cfg.Horizon = cCtx.Duration("horizon") + + horizon := cCtx.String("horizon") + if horizon != "" { + cfg.Horizon, err = parseHorizon(horizon) + if err != nil { + return WatchConfig{}, err + } + } if cCtx.Timestamp("since") != nil { cfg.Since = *cCtx.Timestamp("since") } cfg.IDSince = cCtx.String("idsince") - err := watcher.ConfigDefaults(&cfg.WatchConfig) + err = watcher.ConfigDefaults(&cfg.WatchConfig) if err != nil { return WatchConfig{}, err } diff --git a/watch_test.go b/watch_test.go index 11e40e4..5ef3e4f 100644 --- a/watch_test.go +++ b/watch_test.go @@ -68,6 +68,28 @@ func TestNewWatchConfig(t *testing.T) { check checkWatchConfig errPrefix string }{ + { + name: "interval too small", + args: args{ + cCtx: &mockContext{ + horizon: "1h", + // just under a second + interval: time.Millisecond * 999, + }, + cmd: new(CmdCtx), + }, + errPrefix: "polling more than once per second is not", + }, + + { + name: "horizon or since options are required", + args: args{ + cCtx: &mockContext{}, + cmd: new(CmdCtx), + }, + errPrefix: "provide horizon on its own or either of the since", + }, + { name: "poll count is at least one", args: args{ @@ -108,18 +130,6 @@ func TestNewWatchConfig(t *testing.T) { assert.NotEqual(t, "", cfg.IDSince) // should be set to IDTimeHex }, }, - { - name: "interval too small", - args: args{ - cCtx: &mockContext{ - horizon: time.Hour, - // just under a second - interval: time.Millisecond * 999, - }, - cmd: new(CmdCtx), - }, - errPrefix: "polling more than once per second is not", - }, { name: "bad hex string for idtimestamp errors", args: args{ @@ -130,14 +140,6 @@ func TestNewWatchConfig(t *testing.T) { }, errPrefix: "encoding/hex: invalid byte", }, - { - name: "horizon or since options are required", - args: args{ - cCtx: &mockContext{}, - cmd: new(CmdCtx), - }, - errPrefix: "provide horizon on its own or either of the since", - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -500,7 +502,7 @@ type mockContext struct { since *time.Time mode string idsince string - horizon time.Duration + horizon string interval time.Duration count int tenant string @@ -514,6 +516,8 @@ func (c mockContext) String(n string) string { return c.idsince case "tenant": return c.tenant + case "horizon": + return c.horizon default: return "" } @@ -530,8 +534,6 @@ func (c mockContext) Int(n string) int { func (c mockContext) Duration(n string) time.Duration { switch n { - case "horizon": - return c.horizon case "interval": return c.interval default: