[go] Add support for marshal/unmarshal to time.Duration. #16039

[Seth Shelnutt (Gerrit)]

Seth Shelnutt uploaded a change:
https://go-review.googlesource.com/24473

Add support for marshal/unmarshal to time.Duration. #16039

This add support for MarshalJSON, MarshalText and MarshalBinary
to the time.Duration type.

Change-Id: I1086754df43790323150b9a75c9321c58333233e
---
M src/time/time.go
M src/time/time_test.go
2 files changed, 239 insertions(+), 6 deletions(-)



diff --git a/src/time/time.go b/src/time/time.go
index c31de35..3b27ebf 100644
--- a/src/time/time.go
+++ b/src/time/time.go
@@ -602,6 +602,118 @@
return float64(hour) + float64(nsec)*(1e-9/60/60)
}

+const durationBinaryVersion byte = 1
+
+// MarshalBinary implements the encoding.BinaryMarshaler interface.
+func (d Duration) MarshalBinary() ([]byte, error) {
+ /*
+ // Create a byte buffer to store int64 into
+ var buf = make([]byte, binary.MaxVarintLen64)
+ digitsStored := binary.PutVarint(buf, int64(d))
+
+ // Validate at least one digit was stored
+ if digitsStored < 1 {
+ return nil, errors.New("Duration.MarshalBinary: could not stored digits
in binary")
+ }
+
+ enc := []byte{
+ durationBinaryVersion, // byte 0 : version
+ }
+
+ enc = append(enc, buf...)
+ */
+
+ enc := []byte{
+ durationBinaryVersion, // byte 0 : version
+ byte(d >> 56), // bytes 1-8: nanoseconds
+ byte(d >> 48),
+ byte(d >> 40),
+ byte(d >> 32),
+ byte(d >> 24),
+ byte(d >> 16),
+ byte(d >> 8),
+ byte(d),
+ }
+
+ return enc, nil
+}
+
+// UnmarshalBinary implements the encoding.BinaryUnmarshaler interface.
+func (d *Duration) UnmarshalBinary(data []byte) error {
+ buf := data
+ if len(buf) == 0 {
+ return errors.New("Duration.UnmarshalBinary: no data")
+ }
+
+ if buf[0] != durationBinaryVersion {
+ return errors.New("Duration.UnmarshalBinary: unsupported version")
+ }
+
+ if len(buf) != /*version*/ 1+ /*nanoseconds*/ 8 {
+ return errors.New("Duration.UnmarshalBinary: invalid length")
+ }
+
+ buf = buf[1:]
+ *d = Duration(int64(buf[7]) | int64(buf[6])<<8 | int64(buf[5])<<16 |
int64(buf[4])<<24 |
+ int64(buf[3])<<32 | int64(buf[2])<<40 | int64(buf[1])<<48 |
int64(buf[0])<<56)
+ /*
+ // Parse the bytes to an int64
+ convertedInt64, _ := binary.Varint(buf)
+ // Convert the int64 to Duration type
+ *d = Duration(convertedInt64)
+ */
+ return nil
+}
+
+// TODO(rsc): Remove GobEncoder, GobDecoder, MarshalJSON, UnmarshalJSON in
Go 2.
+// The same semantics will be provided by the generic MarshalBinary,
MarshalText,
+// UnmarshalBinary, UnmarshalText.
+
+// GobEncode implements the gob.GobEncoder interface.
+func (d Duration) GobEncode() ([]byte, error) {
+ return d.MarshalBinary()
+}
+
+// GobDecode implements the gob.GobDecoder interface.
+func (d *Duration) GobDecode(data []byte) error {
+ return d.UnmarshalBinary(data)
+}
+
+// MarshalJSON implements the json.Marshaler interface.
+// The duration is a quoted string provided by String()
+func (d Duration) MarshalJSON() ([]byte, error) {
+
+ b := make([]byte, 0, len(RFC3339Nano)+2)
+ b = append(b, '"')
+ b = append(b, []byte(d.String())...)
+ b = append(b, '"')
+ return b, nil
+}
+
+// UnmarshalJSON implements the json.Unmarshaler interface.
+// The duration is expected to be a quoted string in ParseDuration syntax.
+func (d *Duration) UnmarshalJSON(data []byte) error {
+ // Fractional seconds are handled implicitly by Parse.
+ var err error
+ *d, err = ParseDuration(string(data)[1 : len(string(data))-1])
+ return err
+}
+
+// MarshalText implements the encoding.TextMarshaler interface.
+// The duration is a quoted string provided by String()
+func (d Duration) MarshalText() ([]byte, error) {
+ return []byte(d.String()), nil
+}
+
+// UnmarshalText implements the encoding.TextUnmarshaler interface.
+// The duration is expected to be a quoted string in ParseDuration syntax.
+func (d *Duration) UnmarshalText(data []byte) error {
+ // Fractional seconds are handled implicitly by Parse.
+ var err error
+ *d, err = ParseDuration(string(data))
+ return err
+}
+
// Add returns the time t+d.
func (t Time) Add(d Duration) Time {
t.sec += int64(d / 1e9)
diff --git a/src/time/time_test.go b/src/time/time_test.go
index b7ebb37..11782da 100644
--- a/src/time/time_test.go
+++ b/src/time/time_test.go
@@ -665,7 +665,7 @@
return a.Equal(b) && aoffset == boffset && aname == bname
}

-var gobTests = []Time{
+var gobTimeTests = []Time{
Date(0, 1, 2, 3, 4, 5, 6, UTC),
Date(7, 8, 9, 10, 11, 12, 13, FixedZone("", 0)),
Unix(81985467080890095, 0x76543210), // Time.sec: 0x0123456789ABCDEF
@@ -678,7 +678,7 @@
var b bytes.Buffer
enc := gob.NewEncoder(&b)
dec := gob.NewDecoder(&b)
- for _, tt := range gobTests {
+ for _, tt := range gobTimeTests {
var gobtt Time
if err := enc.Encode(&tt); err != nil {
t.Errorf("%v gob Encode error = %q, want nil", tt, err)
@@ -691,7 +691,7 @@
}
}

-var invalidEncodingTests = []struct {
+var invalidTimeEncodingTests = []struct {
bytes []byte
want string
}{
@@ -701,7 +701,7 @@
}

func TestInvalidTimeGob(t *testing.T) {
- for _, tt := range invalidEncodingTests {
+ for _, tt := range invalidTimeEncodingTests {
var ignored Time
err := ignored.GobDecode(tt.bytes)
if err == nil || err.Error() != tt.want {
@@ -737,7 +737,7 @@
}
}

-var jsonTests = []struct {
+var jsonTimeTests = []struct {
time Time
json string
}{
@@ -747,7 +747,7 @@
}

func TestTimeJSON(t *testing.T) {
- for _, tt := range jsonTests {
+ for _, tt := range jsonTimeTests {
var jsonTime Time

if jsonBytes, err := json.Marshal(tt.time); err != nil {
@@ -883,6 +883,127 @@
}
}

+var gobDurationTests = []Duration{
+ Duration(123456789),
+ Duration(rand.Int31()) * Millisecond,
+ 16 * Hour,
+ 4*Minute + 10*Second + 500*Millisecond,
+ 0,
+ -1<<63 + 1*Nanosecond,
+}
+
+func TestDurationGob(t *testing.T) {
+ var b bytes.Buffer
+ enc := gob.NewEncoder(&b)
+ dec := gob.NewDecoder(&b)
+ for _, dt := range gobDurationTests {
+ var gobdt Duration
+ if err := enc.Encode(&dt); err != nil {
+ t.Errorf("%v gob Encode error = %q, want nil", dt, err)
+ } else if err := dec.Decode(&gobdt); err != nil {
+ t.Errorf("%v gob Decode error = %q, want nil", dt, err)
+ } else if gobdt != dt {
+ t.Errorf("Decoded duration = %v, want %v", gobdt, dt)
+ }
+ b.Reset()
+ }
+}
+
+var invalidDurationEncodingTests = []struct {
+ bytes []byte
+ want string
+}{
+ {[]byte{}, "Duration.UnmarshalBinary: no data"},
+ {[]byte{0, 2, 3}, "Duration.UnmarshalBinary: unsupported version"},
+ {[]byte{1, 2, 3}, "Duration.UnmarshalBinary: invalid length"},
+}
+
+func TestInvalidDurationGob(t *testing.T) {
+ for _, tt := range invalidDurationEncodingTests {
+ var ignored Duration
+ err := ignored.GobDecode(tt.bytes)
+ if err == nil || err.Error() != tt.want {
+ t.Errorf("time.GobDecode(%#v) error = %v, want %v", tt.bytes, err,
tt.want)
+ }
+ err = ignored.UnmarshalBinary(tt.bytes)
+ if err == nil || err.Error() != tt.want {
+ t.Errorf("time.UnmarshalBinary(%#v) error = %v, want %v", tt.bytes,
err, tt.want)
+ }
+ }
+}
+
+var jsonDurationTests = []struct {
+ json string
+ ok bool
+ duration Duration
+}{
+ // simple
+ {`"0s"`, true, 0},
+ {`"5s"`, true, 5 * Second},
+ {`"30s"`, true, 30 * Second},
+ {`"24m38s"`, true, 1478 * Second},
+ // sign
+ {`"-5s"`, true, -5 * Second},
+ {`"5s"`, true, 5 * Second},
+ // decimal
+ {`"5s"`, true, 5 * Second},
+ {`"5.6s"`, true, 5*Second + 600*Millisecond},
+ {`"500ms"`, true, 500 * Millisecond},
+ {`"1s"`, true, 1 * Second},
+ {`"1s"`, true, 1 * Second},
+ {`"1.004s"`, true, 1*Second + 4*Millisecond},
+ {`"1.004s"`, true, 1*Second + 4*Millisecond},
+ {`"1m40.001s"`, true, 100*Second + 1*Millisecond},
+ // different units
+ {`"10ns"`, true, 10 * Nanosecond},
+ {`"11µs"`, true, 11 * Microsecond},
+ {`"12µs"`, true, 12 * Microsecond}, // U+00B5
+ {`"13ms"`, true, 13 * Millisecond},
+ {`"14s"`, true, 14 * Second},
+ {`"15m0s"`, true, 15 * Minute},
+ {`"16h0m0s"`, true, 16 * Hour},
+ // composite durations
+ {`"3h30m0s"`, true, 3*Hour + 30*Minute},
+ {`"4m10.5s"`, true, 4*Minute + 10*Second + 500*Millisecond},
+ {`"-2m3.4s"`, true, -(2*Minute + 3*Second + 400*Millisecond)},
+ {`"1h2m3.004005006s"`, true, 1*Hour + 2*Minute + 3*Second + 4*Millisecond
+ 5*Microsecond + 6*Nanosecond},
+ {`"39h9m14.425s"`, true, 39*Hour + 9*Minute + 14*Second +
425*Millisecond},
+ // large value
+ {`"52.763797s"`, true, 52763797000 * Nanosecond},
+ // more than 9 digits after decimal point, see
https://golang.org/issue/6617
+ {`"20m0s"`, true, 20 * Minute},
+ // 9007199254740993 = 1<<53+1 cannot be stored precisely in a float64
+ {`"2501h59m59.254740993s"`, true, (1<<53 + 1) * Nanosecond},
+ // largest duration that can be represented by int64 in nanoseconds
+ {`"2562047h47m16.854775807s"`, true, (1<<63 - 1) * Nanosecond},
+ // large negative value
+ {`"-2562047h47m16.854775807s"`, true, -1<<63 + 1*Nanosecond},
+}
+
+func TestDurationJSON(t *testing.T) {
+ for _, dt := range jsonDurationTests {
+ var jsonDuration Duration
+
+ if jsonBytes, err := json.Marshal(dt.duration); err != nil {
+ t.Errorf("%v json.Marshal error = %v, want nil", dt.duration, err)
+ } else if string(jsonBytes) != dt.json {
+ t.Errorf("%v JSON = %#q, want %#q", dt.duration, string(jsonBytes),
dt.json)
+ } else if err = json.Unmarshal(jsonBytes, &jsonDuration); err != nil {
+ t.Errorf("%v json.Unmarshal error = %v, want nil", dt.duration, err)
+ } else if jsonDuration != dt.duration {
+ t.Errorf("Unmarshaled duration = %v, want %v", jsonDuration,
dt.duration)
+ }
+ }
+}
+
+func TestInvalidDurationJSON(t *testing.T) {
+ var dt Duration
+ err := json.Unmarshal([]byte(`{"now is the duration":"buddy"}`), &dt)
+ if err == nil {
+ t.Errorf("expected *Error unmarshaling JSON, got %v", err)
+ }
+}
+
// golang.org/issue/4622
func TestLocationRace(t *testing.T) {
ResetLocalOnceForTest() // reset the Once to trigger the race

--
https://go-review.googlesource.com/24473

[Ralph Corderoy (Gerrit)]

Ralph Corderoy has posted comments on this change.

Add support for marshal/unmarshal to time.Duration. #16039

Patch Set 1:

(5 comments)

https://go-review.googlesource.com/#/c/24473/1//COMMIT_MSG
Commit Message:

PS1, Line 7: 16039
Is this the correct issue? Doesn't seem immediately related.


https://go-review.googlesource.com/#/c/24473/1/src/time/time.go
File src/time/time.go:

PS1, Line 609: /*
: // Create a byte buffer to store int64 into
: var buf = make([]byte, binary.MaxVarintLen64)
: digitsStored := binary.PutVarint(buf, int64(d))
Delete commented out alternative?


PS1, Line 652: if len(buf) != /*version*/ 1+ /*nanoseconds*/ 8 {
Just "1 + 8" as the following code is simple and shows what they are, and
the encode is close by too for help?


PS1, Line 698: *d, err = ParseDuration(string(data)[1 :
len(string(data))-1])
Discards first and last characters without checking they're double-quotes.
Check the string is long enough to be a quoted Duration?


https://go-review.googlesource.com/#/c/24473/1/src/time/time_test.go
File src/time/time_test.go:

PS1, Line 935: var jsonDurationTests = []struct {
Needs strings that: aren't double quoted, empty with just double quotes,
and just one double quote.


--
https://go-review.googlesource.com/24473
Gerrit-Reviewer: Ralph Corderoy <ra...@inputplus.co.uk>
Gerrit-HasComments: Yes

[Justin Nuß (Gerrit)]

Justin Nuß has posted comments on this change.

Add support for marshal/unmarshal to time.Duration. #16039

Patch Set 1:

(7 comments)

The new code calls String() twice only to convert the string to a []byte or
append it to one. Maybe you could create a new method bytes() that uses the
logic from String() to return a []byte and change String() to just return
string(d.bytes()).

Similiar to this, the new code calls ParseDuration() twice with strings
that get converted to byte slices only for the call. You could create a
parseDurationBytes() function that takes a []byte instead of a string and
change ParseDuration to use it.

This can (and probably should) be done in another CL, so that this CL can
focus on the implementation and not the optimizations. Also the
optimizations would need benchmarks.

https://go-review.googlesource.com/#/c/24473/1//COMMIT_MSG
Commit Message:

PS1, Line 7: 16039
> Is this the correct issue? Doesn't seem immediately related.

The first line should start with the package name. Something like

time: implement Marshaler/Unmarshaler interfaces for Duration


PS1, Line 9: This add support for MarshalJSON, MarshalText and MarshalBinary
: to the time.Duration type.
Missing GobEncoder


https://go-review.googlesource.com/#/c/24473/1/src/time/time.go
File src/time/time.go:

Line 657: *d = Duration(int64(buf[7]) | int64(buf[6])<<8 |

int64(buf[5])<<16 | int64(buf[4])<<24 |

Maybe split into multiple lines?


Line 660: // Parse the bytes to an int64
What's the reason behind using the above instead of this?


Line 685:
Remove empty line


Line 688: b = append(b, []byte(d.String())...)
No need to convert to a byte-slice. Just write append(b, d.String()...)


Line 698: *d, err = ParseDuration(string(data)[1 : len(string(data))-1])
No need to convert data to a string to take it's length. Also by slicing
before converting you can save 2 bytes.

*d, err = ParseDuration(string(data[1 : len(data)-1]))


--
https://go-review.googlesource.com/24473
Gerrit-Reviewer: Justin Nuß <nuss....@gmail.com>

[Justin Nuß (Gerrit)]

Justin Nuß has posted comments on this change.

Add support for marshal/unmarshal to time.Duration. #16039

Patch Set 1:

(1 comment)

https://go-review.googlesource.com/#/c/24473/1//COMMIT_MSG
Commit Message:

PS1, Line 9: This add support for MarshalJSON, MarshalText and MarshalBinary
: to the time.Duration type.
> Missing GobEncoder
Also this should mention the Unmarshal/Decode methods.

[Seth Shelnutt (Gerrit)]

Seth Shelnutt uploaded a new patch set:
https://go-review.googlesource.com/24473

time: implement Encoder/Decoder/Marshaler/Unmarshaler interfaces for
Duration. #16039

This add support for MarshalJSON, UnmarshalJSON, MarshalText, UnmarshalTex,
MarshalBinary, UnmarshalBinary, GobEncoder, GodDecoder to the time.Duration

type.

Change-Id: I1086754df43790323150b9a75c9321c58333233e
---
M src/time/time.go
M src/time/time_test.go

2 files changed, 243 insertions(+), 6 deletions(-)

[Seth Shelnutt (Gerrit)]

Seth Shelnutt has posted comments on this change.

time: implement Encoder/Decoder/Marshaler/Unmarshaler interfaces for
Duration. #16039

Patch Set 2:

> (7 comments)
>
> The new code calls String() twice only to convert the string to a
> []byte or append it to one. Maybe you could create a new method
> bytes() that uses the logic from String() to return a []byte and
> change String() to just return string(d.bytes()).
>
> Similiar to this, the new code calls ParseDuration() twice with
> strings that get converted to byte slices only for the call. You
> could create a parseDurationBytes() function that takes a []byte
> instead of a string and change ParseDuration to use it.
>
> This can (and probably should) be done in another CL, so that this
> CL can focus on the implementation and not the optimizations. Also
> the optimizations would need benchmarks.

I will look into a parseDurationBytes(), to see if it is effective and more
optimal solution. I'll upload that as a separate CL for review and analysis.

--
https://go-review.googlesource.com/24473
Gerrit-Reviewer: Justin Nuß <nuss....@gmail.com>
Gerrit-Reviewer: Ralph Corderoy <ra...@inputplus.co.uk>

Gerrit-Reviewer: Seth Shelnutt <shel...@gmail.com>
Gerrit-HasComments: No

[Seth Shelnutt (Gerrit)]

Seth Shelnutt has posted comments on this change.

time: implement Encoder/Decoder/Marshaler/Unmarshaler interfaces for
Duration. #16039

Patch Set 2:

I've uploaded a new patchset which addresses all the comments, except the
parseDurationBytes. I've also added additional test cases to handle the
invalidate JSON checks.

[Elias Naur (Gerrit)]

Elias Naur has posted comments on this change.

time: implement Encoder/Decoder/Marshaler/Unmarshaler interfaces for
Duration. #16039

Patch Set 2:

(1 comment)

https://go-review.googlesource.com/#/c/24473/2//COMMIT_MSG
Commit Message:

PS2, Line 7: #16039
By convention, issue numbers belong in the end of the commit message.
Please use "Fixes #16039" for the gopher bot to automatically close it on
submit.


--
https://go-review.googlesource.com/24473
Gerrit-Reviewer: Elias Naur <elias...@gmail.com>

Gerrit-Reviewer: Justin Nuß <nuss....@gmail.com>
Gerrit-Reviewer: Ralph Corderoy <ra...@inputplus.co.uk>
Gerrit-Reviewer: Seth Shelnutt <shel...@gmail.com>

Gerrit-HasComments: Yes

[Seth Shelnutt (Gerrit)]

Seth Shelnutt uploaded a new patch set:
https://go-review.googlesource.com/24473

time: implement Encoder/Decoder/Marshaler/Unmarshaler interfaces for

Duration. #16039

This add support for MarshalJSON, UnmarshalJSON, MarshalText, UnmarshalTex,

MarshalBinary, UnmarshalBinary, GobEncoder, GodDecoder to the time.Duration

type.

Change-Id: I1086754df43790323150b9a75c9321c58333233e
---
M src/time/time.go
M src/time/time_test.go

2 files changed, 241 insertions(+), 6 deletions(-)

[Seth Shelnutt (Gerrit)]

Seth Shelnutt uploaded a new patch set:
https://go-review.googlesource.com/24473

time: implement Encoder/Decoder/Marshaler/Unmarshaler interfaces for
Duration

This add support for MarshalJSON, UnmarshalJSON, MarshalText, UnmarshalTex,
MarshalBinary, UnmarshalBinary, GobEncoder, GodDecoder to the time.Duration
type.

Fixes #16039

Change-Id: I1086754df43790323150b9a75c9321c58333233e
---
M src/time/time.go
M src/time/time_test.go

[Ian Lance Taylor (Gerrit)]

Ian Lance Taylor has posted comments on this change.

time: implement Encoder/Decoder/Marshaler/Unmarshaler interfaces for
Duration

Patch Set 4:

R=go1.8

--
https://go-review.googlesource.com/24473
Gerrit-Reviewer: Elias Naur <elias...@gmail.com>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

Gerrit-Reviewer: Justin Nuß <nuss....@gmail.com>
Gerrit-Reviewer: Ralph Corderoy <ra...@inputplus.co.uk>
Gerrit-Reviewer: Seth Shelnutt <shel...@gmail.com>

Gerrit-HasComments: No

[Ralph Corderoy (Gerrit)]

Ralph Corderoy has posted comments on this change.

time: implement Encoder/Decoder/Marshaler/Unmarshaler interfaces for
Duration

Patch Set 4:

(2 comments)

https://go-review.googlesource.com/#/c/24473/4/src/time/time_test.go
File src/time/time_test.go:

PS4, Line 937: ok bool
Why have ok when they are all true?


PS4, Line 1001: ok bool
Why have ok when they are all false?

--
https://go-review.googlesource.com/24473
Gerrit-Reviewer: Elias Naur <elias...@gmail.com>
Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
Gerrit-Reviewer: Justin Nuß <nuss....@gmail.com>
Gerrit-Reviewer: Ralph Corderoy <ra...@inputplus.co.uk>
Gerrit-Reviewer: Seth Shelnutt <shel...@gmail.com>

Gerrit-HasComments: Yes