From 06738ea3ff90a66996c95541072d292bac80a930 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 2 Aug 2022 04:29:16 +0200 Subject: [PATCH 1/3] Fix SecToTime edge-cases (#20610) - Backport of #20610 - Currently there are edge-cases where the code fails to produce the correct results, this is mainly due to the misusage of the modulo operator. Well this could be fixed, it's better to use a more performant way and simply distract the previous number's cost to calculate the variables. - Resolves #20589 --- modules/util/sec_to_time.go | 20 ++++++++++++++++---- modules/util/sec_to_time_test.go | 17 +++++++++++------ 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/modules/util/sec_to_time.go b/modules/util/sec_to_time.go index 9ce6fe1a13eb..3904ca162fdd 100644 --- a/modules/util/sec_to_time.go +++ b/modules/util/sec_to_time.go @@ -16,12 +16,24 @@ import ( // 1563418 -> 2 weeks 4 days // 3937125s -> 1 month 2 weeks // 45677465s -> 1 year 6 months +// +// Magic numbers: +// 3600 = 60 * 60 (amount of seconds in a hour) +// 86400 = 60 * 60 * 24 (amount of seconds in a day) func SecToTime(duration int64) string { formattedTime := "" - years := duration / (3600 * 24 * 7 * 4 * 12) - months := (duration / (3600 * 24 * 30)) % 12 - weeks := (duration / (3600 * 24 * 7)) % 4 - days := (duration / (3600 * 24)) % 7 + years := (duration / 86400) / 365 + + // The following three variables are calculated with taking + // into account the previous calculated variables, this avoids + // pitfalls when using remainders. As that could lead to incorrect + // results when the calculated number equals the quotient number. + months := (duration/86400 - years*365) / 30 + weeks := (duration/86400 - years*365 - months*30) / 7 + days := duration/86400 - years*365 - months*30 - weeks*7 + + // The following three variables are calculated without depending + // on the previous calculated variables. hours := (duration / 3600) % 24 minutes := (duration / 60) % 60 seconds := duration % 60 diff --git a/modules/util/sec_to_time_test.go b/modules/util/sec_to_time_test.go index 854190462bc3..c67d7ea9debb 100644 --- a/modules/util/sec_to_time_test.go +++ b/modules/util/sec_to_time_test.go @@ -11,10 +11,15 @@ import ( ) func TestSecToTime(t *testing.T) { - assert.Equal(t, SecToTime(66), "1 minute 6 seconds") - assert.Equal(t, SecToTime(52410), "14 hours 33 minutes") - assert.Equal(t, SecToTime(563418), "6 days 12 hours") - assert.Equal(t, SecToTime(1563418), "2 weeks 4 days") - assert.Equal(t, SecToTime(3937125), "1 month 2 weeks") - assert.Equal(t, SecToTime(45677465), "1 year 5 months") + assert.Equal(t, "1 minute 6 seconds", SecToTime(66)) + assert.Equal(t, "1 hour", SecToTime(3600)) + assert.Equal(t, "1 hour", SecToTime(3601)) + assert.Equal(t, "14 hours 33 minutes", SecToTime(52410)) + assert.Equal(t, "6 days 12 hours", SecToTime(563418)) + assert.Equal(t, "2 weeks 4 days", SecToTime(1563418)) + assert.Equal(t, "4 weeks", SecToTime(2419200)) + assert.Equal(t, "4 weeks 1 day", SecToTime(2505600)) + assert.Equal(t, "1 month 2 weeks", SecToTime(3937125)) + assert.Equal(t, "11 months 1 week", SecToTime(29376000)) + assert.Equal(t, "1 year 5 months", SecToTime(45677465)) } From f76545f22f2930745e446a9b0ce90906261b6dd3 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 7 Aug 2022 14:27:53 +0200 Subject: [PATCH 2/3] Backport changes --- modules/util/sec_to_time.go | 16 ++++++++++------ modules/util/sec_to_time_test.go | 28 +++++++++++++++++----------- 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/modules/util/sec_to_time.go b/modules/util/sec_to_time.go index 3904ca162fdd..cdefd5073d2e 100644 --- a/modules/util/sec_to_time.go +++ b/modules/util/sec_to_time.go @@ -22,15 +22,19 @@ import ( // 86400 = 60 * 60 * 24 (amount of seconds in a day) func SecToTime(duration int64) string { formattedTime := "" - years := (duration / 86400) / 365 - // The following three variables are calculated with taking - // into account the previous calculated variables, this avoids + // The following four variables are calculated by taking + // into account the previously calculated variables, this avoids // pitfalls when using remainders. As that could lead to incorrect // results when the calculated number equals the quotient number. - months := (duration/86400 - years*365) / 30 - weeks := (duration/86400 - years*365 - months*30) / 7 - days := duration/86400 - years*365 - months*30 - weeks*7 + remainingDays := duration / (60 * 60 * 24) + years := remainingDays / 365 + remainingDays -= years * 365 + months := remainingDays * 12 / 365 + remainingDays -= months * 365 / 12 + weeks := remainingDays / 7 + remainingDays -= weeks * 7 + days := remainingDays // The following three variables are calculated without depending // on the previous calculated variables. diff --git a/modules/util/sec_to_time_test.go b/modules/util/sec_to_time_test.go index c67d7ea9debb..c9ad8e9209a2 100644 --- a/modules/util/sec_to_time_test.go +++ b/modules/util/sec_to_time_test.go @@ -11,15 +11,21 @@ import ( ) func TestSecToTime(t *testing.T) { - assert.Equal(t, "1 minute 6 seconds", SecToTime(66)) - assert.Equal(t, "1 hour", SecToTime(3600)) - assert.Equal(t, "1 hour", SecToTime(3601)) - assert.Equal(t, "14 hours 33 minutes", SecToTime(52410)) - assert.Equal(t, "6 days 12 hours", SecToTime(563418)) - assert.Equal(t, "2 weeks 4 days", SecToTime(1563418)) - assert.Equal(t, "4 weeks", SecToTime(2419200)) - assert.Equal(t, "4 weeks 1 day", SecToTime(2505600)) - assert.Equal(t, "1 month 2 weeks", SecToTime(3937125)) - assert.Equal(t, "11 months 1 week", SecToTime(29376000)) - assert.Equal(t, "1 year 5 months", SecToTime(45677465)) + second := int64(1) + minute := 60 * second + hour := 60 * minute + day := 24 * hour + year := 365 * day + + assert.Equal(t, "1 minute 6 seconds", SecToTime(minute+6*second)) + assert.Equal(t, "1 hour", SecToTime(hour)) + assert.Equal(t, "1 hour", SecToTime(hour+second)) + assert.Equal(t, "14 hours 33 minutes", SecToTime(14*hour+33*minute+30*second)) + assert.Equal(t, "6 days 12 hours", SecToTime(6*day+12*hour+30*minute+18*second)) + assert.Equal(t, "2 weeks 4 days", SecToTime((2*7+4)*day+2*hour+16*minute+58*second)) + assert.Equal(t, "4 weeks", SecToTime(4*7*day)) + assert.Equal(t, "4 weeks 1 day", SecToTime((4*7+1)*day)) + assert.Equal(t, "1 month 2 weeks", SecToTime((6*7+3)*day+13*hour+38*minute+45*second)) + assert.Equal(t, "11 months 1 week", SecToTime(year-25*day)) + assert.Equal(t, "1 year 5 months", SecToTime(year+163*day+10*hour+11*minute+5*second)) } From 67e6166ea05b9b38257f9e7bb630cbc5cca4fc31 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 7 Aug 2022 23:33:12 +0200 Subject: [PATCH 3/3] Fix tests --- modules/util/sec_to_time_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/util/sec_to_time_test.go b/modules/util/sec_to_time_test.go index c9ad8e9209a2..1e256aa8650c 100644 --- a/modules/util/sec_to_time_test.go +++ b/modules/util/sec_to_time_test.go @@ -26,6 +26,6 @@ func TestSecToTime(t *testing.T) { assert.Equal(t, "4 weeks", SecToTime(4*7*day)) assert.Equal(t, "4 weeks 1 day", SecToTime((4*7+1)*day)) assert.Equal(t, "1 month 2 weeks", SecToTime((6*7+3)*day+13*hour+38*minute+45*second)) - assert.Equal(t, "11 months 1 week", SecToTime(year-25*day)) + assert.Equal(t, "11 months", SecToTime(year-25*day)) assert.Equal(t, "1 year 5 months", SecToTime(year+163*day+10*hour+11*minute+5*second)) }