From 986b48caeeee04ced5b6bd9907781b2ee9ea95dd Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Thu, 30 Jul 2020 10:59:46 -0700 Subject: [PATCH] duration: fix negative duration math In Go, negative numbers mod positive numbers returns negative numbers (e.g. -5 % 12 is -7), which is the root of the bug. Release note (bug fix): Previously, subtracting months from a timestamp/date/timestamptz whose date value is greater than 28 could subtract a further year off. This has now be rectified. --- pkg/util/duration/duration.go | 7 +- pkg/util/duration/duration_test.go | 101 +++++++++++++++++++++++++++-- 2 files changed, 103 insertions(+), 5 deletions(-) diff --git a/pkg/util/duration/duration.go b/pkg/util/duration/duration.go index 8ddbf94a08dc..d8e11657553e 100644 --- a/pkg/util/duration/duration.go +++ b/pkg/util/duration/duration.go @@ -421,7 +421,12 @@ func Add(ctx Context, t time.Time, d Duration) time.Time { } // Adjustments for 1-based math. - expectedMonth := time.Month((int(t.Month())-1+int(d.Months))%12 + 1) + expectedMonth := time.Month((int(t.Month())-1+int(d.Months))%12) + 1 + // If we have a negative duration, we have a negative modulus. + // Push it back up to the positive expectedMonth. + if expectedMonth <= 0 { + expectedMonth += 12 + } // Use AddDate() to get a rough value. This might overshoot the // end of the expected month by multiple days. We could iteratively diff --git a/pkg/util/duration/duration_test.go b/pkg/util/duration/duration_test.go index 755e66238b0a..14c80800c3de 100644 --- a/pkg/util/duration/duration_test.go +++ b/pkg/util/duration/duration_test.go @@ -11,6 +11,7 @@ package duration import ( + "fmt" "math" "testing" "time" @@ -212,6 +213,56 @@ func TestAdd(t *testing.T) { d: Duration{Months: 15}, exp: time.Date(1994, 1, 01, 0, 0, 0, 0, time.UTC), }, + { + t: time.Date(2020, time.July, 28, 0, 0, 0, 0, time.UTC), + d: Duration{Months: 12}, + exp: time.Date(2021, time.July, 28, 0, 0, 0, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 29, 0, 0, 0, 0, time.UTC), + d: Duration{Months: 18}, + exp: time.Date(2022, time.January, 29, 0, 0, 0, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 29, 0, 0, 0, 0, time.UTC), + d: Duration{Months: 30}, + exp: time.Date(2023, time.January, 29, 0, 0, 0, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 29, 0, 0, 0, 0, time.UTC), + d: Duration{Months: 12}, + exp: time.Date(2021, time.July, 29, 0, 0, 0, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 29, 0, 0, 0, 0, time.UTC), + d: Duration{Months: 12, nanos: int64(10 * time.Second)}, + exp: time.Date(2021, time.July, 29, 0, 0, 10, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 29, 0, 0, 0, 0, time.UTC), + d: Duration{Months: 17, nanos: int64(10 * time.Second)}, + exp: time.Date(2021, time.December, 29, 0, 0, 10, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 29, 0, 0, 0, 0, time.UTC), + d: Duration{Months: 18, nanos: int64(10 * time.Second)}, + exp: time.Date(2022, time.January, 29, 0, 0, 10, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 29, 0, 0, 0, 0, time.UTC), + d: Duration{Months: 19, nanos: int64(10 * time.Second)}, + exp: time.Date(2022, time.February, 28, 0, 0, 10, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 29, 0, 0, 0, 0, time.UTC), + d: Duration{Months: 20, nanos: int64(10 * time.Second)}, + exp: time.Date(2022, time.March, 29, 0, 0, 10, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 30, 0, 0, 0, 0, time.UTC), + d: Duration{Months: 12}, + exp: time.Date(2021, time.July, 30, 0, 0, 0, 0, time.UTC), + }, // Check leap behaviors { @@ -270,12 +321,54 @@ func TestAdd(t *testing.T) { d: Duration{Months: -1, Days: -1}, exp: time.Date(2015, 12, 31, 0, 0, 0, 0, time.UTC), }, + { + t: time.Date(2020, time.July, 28, 0, 0, 0, 0, time.UTC), + d: Duration{Months: -12}, + exp: time.Date(2019, time.July, 28, 0, 0, 0, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 29, 0, 0, 0, 0, time.UTC), + d: Duration{Months: -18}, + exp: time.Date(2019, time.January, 29, 0, 0, 0, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 29, 0, 0, 0, 0, time.UTC), + d: Duration{Months: -30}, + exp: time.Date(2018, time.January, 29, 0, 0, 0, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 29, 0, 0, 0, 0, time.UTC), + d: Duration{Months: -12}, + exp: time.Date(2019, time.July, 29, 0, 0, 0, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 29, 0, 0, 0, 0, time.UTC), + d: Duration{Months: -12, nanos: int64(-10 * time.Second)}, + exp: time.Date(2019, time.July, 28, 23, 59, 50, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 29, 0, 0, 0, 0, time.UTC), + d: Duration{Months: -19, nanos: int64(-10 * time.Second)}, + exp: time.Date(2018, time.December, 28, 23, 59, 50, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 29, 0, 0, 0, 0, time.UTC), + d: Duration{Months: -20, nanos: int64(-10 * time.Second)}, + exp: time.Date(2018, time.November, 28, 23, 59, 50, 0, time.UTC), + }, + { + t: time.Date(2020, time.July, 30, 0, 0, 0, 0, time.UTC), + d: Duration{Months: -12}, + exp: time.Date(2019, time.July, 30, 0, 0, 0, 0, time.UTC), + }, } for i, test := range tests { - if res := Add(nil, test.t, test.d); !test.exp.Equal(res) { - t.Errorf("%d: expected Add(%v, %d) = %v, found %v", - i, test.t, test.d, test.exp, res) - } + t.Run(fmt.Sprintf("%s_%s", test.t, test.d), func(t *testing.T) { + if res := Add(nil, test.t, test.d); !test.exp.Equal(res) { + t.Errorf("%d: expected Add(%s, %s) = %s, found %s", + i, test.t, test.d, test.exp, res) + } + }) } }