follow-up for 6849858

It is expected that the DurationExpr.s contains valid duration.
That's why DurationExpr.Duration() shouldn't return an error.
Instead, it must return the maximum possible value (or the minimum possible value)
if the provided `step` arg is too big for the stored duration ending with `i`
such as `1234567i`.

Returning the error from DurationExpr.Duration() may require too many changes at the caller side,
which may introduce new errors if they weren't verified carefully.
See, for example, VictoriaMetrics/VictoriaMetrics#8487 .
That's why it is better to do not return an error from DurationExpr.Duration(),
and returning the clamped duration instead.

Updates VictoriaMetrics/VictoriaMetrics#8447

Author: valyala

lexer.go

@@ -598,8 +598,13 @@ func DurationValue(s string, step int64) (int64, error) {
 			isMinus = true
 		}
 	}
-	if math.Abs(d) > 1<<63-1 {
-		return 0, fmt.Errorf("too big duration %.0fms", d)
+	if d > math.MaxInt64 {
+		// Truncate too big durations. See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/8447
+		return math.MaxInt64, nil
+	}
+	if d < math.MinInt64 {
+		// Truncate too small durations. See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/8447
+		return math.MinInt64, nil
 	}
 	return int64(d), nil
 }

lexer_test.go

@@ -504,14 +504,14 @@ func testLexerError(t *testing.T, s string) {
 }
 
 func TestPositiveDurationSuccess(t *testing.T) {
-	f := func(s string, step, expectedD int64) {
+	f := func(s string, step, dExpected int64) {
 		t.Helper()
 		d, err := PositiveDurationValue(s, step)
 		if err != nil {
 			t.Fatalf("unexpected error: %s", err)
 		}
-		if d != expectedD {
-			t.Fatalf("unexpected duration; got %d; want %d", d, expectedD)
+		if d != dExpected {
+			t.Fatalf("unexpected duration; got %d; want %d", d, dExpected)
 		}
 	}
 
@@ -549,6 +549,10 @@ func TestPositiveDurationSuccess(t *testing.T) {
 	f("1H", 45, 1*60*60*1000)
 	f("1D", 45, 1*24*60*60*1000)
 	f("1Y", 45, 1*365*24*60*60*1000)
+
+	// Too big duration
+	f("10000000000y", 0, math.MaxInt64)
+	f("922335359011637780i", 5*3600*1000, math.MaxInt64)
 }
 
 func TestPositiveDurationError(t *testing.T) {
@@ -572,23 +576,20 @@ func TestPositiveDurationError(t *testing.T) {
 	f("1mi")
 	f("1mb")
 
-	// Too big duration
-	f("10000000000y")
-
 	// Uppercase M isn't a duration, but a 1e6 multiplier.
 	// See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3664
 	f("1M")
 }
 
 func TestDurationSuccess(t *testing.T) {
-	f := func(s string, step, expectedD int64) {
+	f := func(s string, step, dExpected int64) {
 		t.Helper()
 		d, err := DurationValue(s, step)
 		if err != nil {
 			t.Fatalf("unexpected error: %s", err)
 		}
-		if d != expectedD {
-			t.Fatalf("unexpected duration; got %d; want %d", d, expectedD)
+		if d != dExpected {
+			t.Fatalf("unexpected duration; got %d; want %d", d, dExpected)
 		}
 	}
 
@@ -641,6 +642,14 @@ func TestDurationSuccess(t *testing.T) {
 	f("-3.H", 10, -3*60*60*1000)
 	f("1D", 10, 1*24*60*60*1000)
 	f("-.1Y", 10, -0.1*365*24*60*60*1000)
+
+	// Too big duration
+	f("10000000000y", 0, math.MaxInt64)
+	f("922335359011637780i", 5*3600*1000, math.MaxInt64)
+
+	// Too small duration
+	f("-10000000000y", 0, math.MinInt64)
+	f("-922335359011637780i", 5*3600*1000, math.MinInt64)
 }
 
 func TestDurationError(t *testing.T) {

parser.go

@@ -1030,10 +1030,7 @@ func expandDuration(was []*withArgExpr, d *DurationExpr) (*DurationExpr, error)
 		return t, nil
 	case *NumberExpr:
 		// Convert number of seconds to DurationExpr
-		de := &DurationExpr{
-			s: t.s,
-		}
-		return de, nil
+		return newDurationExpr(t.s)
 	default:
 		return nil, fmt.Errorf("unexpected value for WITH template %q; got %s; want duration", d.s, e.AppendString(nil))
 	}
@@ -1631,26 +1628,33 @@ func (p *parser) parsePositiveDuration() (*DurationExpr, error) {
 		}
 	}
 	// Verify duration value.
-	if _, err := DurationValue(s, 0); err != nil {
-		return nil, fmt.Errorf(`duration: parse value error: %q: %w`, s, err)
-	}
 	if s == "$__interval" {
 		s = "1i"
 	}
-	de := &DurationExpr{
-		s: s,
-	}
-	return de, nil
+	return newDurationExpr(s)
 }
 
 // DurationExpr contains the duration
 type DurationExpr struct {
+	// s is a string representation of the duration.
+	//
+	// it must contain valid duration if needsParsing is set to false.
 	s string
 
 	// needsParsing is set to true if s isn't parsed yet with expandWithExpr()
 	needsParsing bool
 }
 
+func newDurationExpr(s string) (*DurationExpr, error) {
+	if _, err := DurationValue(s, 0); err != nil {
+		return nil, fmt.Errorf(`cannot parse duration %q: %w`, s, err)
+	}
+	de := &DurationExpr{
+		s: s,
+	}
+	return de, nil
+}
+
 // AppendString appends string representation of de to dst and returns the result.
 func (de *DurationExpr) AppendString(dst []byte) []byte {
 	if de == nil {
@@ -1663,29 +1667,26 @@ func (de *DurationExpr) AppendString(dst []byte) []byte {
 //
 // Error is returned if the duration is negative.
 func (de *DurationExpr) NonNegativeDuration(step int64) (int64, error) {
-	d, err := de.Duration(step)
-	if err != nil {
-		return 0, err
-	}
+	d := de.Duration(step)
 	if d < 0 {
 		return 0, fmt.Errorf("unexpected negative duration %dms", d)
 	}
 	return d, nil
 }
 
 // Duration returns the duration from de in milliseconds.
-func (de *DurationExpr) Duration(step int64) (int64, error) {
+func (de *DurationExpr) Duration(step int64) int64 {
 	if de == nil {
-		return 0, nil
+		return 0
 	}
 	if de.needsParsing {
 		panic(fmt.Errorf("BUG: duration %q must be already parsed", de.s))
 	}
 	d, err := DurationValue(de.s, step)
 	if err != nil {
-		return 0, fmt.Errorf("cannot parse duration %q: %w", de.s, err)
+		panic(fmt.Errorf("BUG: cannot parse duration %q: %s", de.s, err))
 	}
-	return d, nil
+	return d
 }
 
 // parseIdentExpr parses expressions starting with `ident` token.

parser_test.go

@@ -1008,30 +1008,3 @@ func TestParseError(t *testing.T) {
 	f(`with (x={a="b" or c="d"}) {x,d="e"}`)
 	f(`with (x={a="b" or c="d"}) {x,d="e" or z="c"}`)
 }
-
-func TestParseDuration(t *testing.T) {
-	s := func(v string, step, expected int64) {
-		de := DurationExpr{s: v}
-		result, err := de.NonNegativeDuration(step)
-		if err != nil {
-			t.Fatalf("unexpected error: %s", err)
-		}
-		if result != expected {
-			t.Fatalf("unexpected result; got %d, expected %d", result, expected)
-		}
-	}
-
-	f := func(v string, step int64) {
-		de := DurationExpr{s: v}
-		_, err := de.NonNegativeDuration(step)
-		if err == nil {
-			t.Fatalf("expecting error, got nil")
-		}
-	}
-
-	s("1i", 10, 10)
-	s("10s", 300, 10*1000)
-
-	// too large resulting value
-	f("99999999999999999999i", 10)
-}