Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of Time values out of UnixNano range #804

Merged
merged 3 commits into from
Mar 27, 2020
Merged

Fix handling of Time values out of UnixNano range #804

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e48bc3e
Fix handling of Time values out of UnixNano rnage
prashantv Mar 24, 2020
06a1362
_minTime should use math.MinInt64, and test for it
prashantv Mar 26, 2020
0182292
Use `time.Equal` to ensure times are not the same
prashantv Mar 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions field.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ import (
// improves the navigability of this package's API documentation.
type Field = zapcore.Field

var (
_minTimeInt64 = time.Unix(0, 0)
prashantv marked this conversation as resolved.
Show resolved Hide resolved
_maxTimeInt64 = time.Unix(0, math.MaxInt64)
)

// Skip constructs a no-op field, which is often useful when handling invalid
// inputs in other Field constructors.
func Skip() Field {
Expand Down Expand Up @@ -339,6 +344,9 @@ func Stringer(key string, val fmt.Stringer) Field {
// Time constructs a Field with the given key and value. The encoder
// controls how the time is serialized.
func Time(key string, val time.Time) Field {
if val.Before(_minTimeInt64) || val.After(_maxTimeInt64) {
return Field{Key: key, Type: zapcore.TimeFullType, Interface: val}
}
return Field{Key: key, Type: zapcore.TimeType, Integer: val.UnixNano(), Interface: val.Location()}
}

Expand Down
4 changes: 4 additions & 0 deletions field_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
package zap

import (
"math"
"net"
"sync"
"testing"
Expand Down Expand Up @@ -107,6 +108,9 @@ func TestFieldConstructors(t *testing.T) {
{"String", Field{Key: "k", Type: zapcore.StringType, String: "foo"}, String("k", "foo")},
{"Time", Field{Key: "k", Type: zapcore.TimeType, Integer: 0, Interface: time.UTC}, Time("k", time.Unix(0, 0).In(time.UTC))},
{"Time", Field{Key: "k", Type: zapcore.TimeType, Integer: 1000, Interface: time.UTC}, Time("k", time.Unix(0, 1000).In(time.UTC))},
{"Time", Field{Key: "k", Type: zapcore.TimeType, Integer: math.MaxInt64, Interface: time.UTC}, Time("k", time.Unix(0, math.MaxInt64).In(time.UTC))},
{"Time", Field{Key: "k", Type: zapcore.TimeFullType, Interface: time.Time{}}, Time("k", time.Time{})},
{"Time", Field{Key: "k", Type: zapcore.TimeFullType, Interface: time.Unix(math.MaxInt64, 0)}, Time("k", time.Unix(math.MaxInt64, 0))},
{"Uint", Field{Key: "k", Type: zapcore.Uint64Type, Integer: 1}, Uint("k", 1)},
{"Uint64", Field{Key: "k", Type: zapcore.Uint64Type, Integer: 1}, Uint64("k", 1)},
{"Uint32", Field{Key: "k", Type: zapcore.Uint32Type, Integer: 1}, Uint32("k", 1)},
Expand Down
7 changes: 6 additions & 1 deletion zapcore/field.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,11 @@ const (
Int8Type
// StringType indicates that the field carries a string.
StringType
// TimeType indicates that the field carries a time.Time.
// TimeType indicates that the field carries a time.Time that is
// representable by a UnixNano() stored as an int64.
TimeType
// TimeFullType indicates that the field carries a time.Time stored as-is.
TimeFullType
abhinav marked this conversation as resolved.
Show resolved Hide resolved
// Uint64Type indicates that the field carries a uint64.
Uint64Type
// Uint32Type indicates that the field carries a uint32.
Expand Down Expand Up @@ -145,6 +148,8 @@ func (f Field) AddTo(enc ObjectEncoder) {
// Fall back to UTC if location is nil.
enc.AddTime(f.Key, time.Unix(0, f.Integer))
}
case TimeFullType:
enc.AddTime(f.Key, f.Interface.(time.Time))
case Uint64Type:
enc.AddUint64(f.Key, uint64(f.Integer))
case Uint32Type:
Expand Down
11 changes: 11 additions & 0 deletions zapcore/field_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/zap"
. "go.uber.org/zap/zapcore"
)
Expand Down Expand Up @@ -164,6 +165,10 @@ func TestFields(t *testing.T) {
}

func TestEquals(t *testing.T) {
timeOutOfRange := time.Unix(0, math.MaxInt64).Add(time.Nanosecond)
timeOutOfRangeNano := time.Unix(0, timeOutOfRange.UnixNano())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should try out of range in both directions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 done

require.NotEqual(t, timeOutOfRange, timeOutOfRangeNano, "should be different as value is out of UnixNano range")

tests := []struct {
a, b Field
want bool
Expand Down Expand Up @@ -198,6 +203,12 @@ func TestEquals(t *testing.T) {
b: zap.Time("k", time.Unix(1000, 1000).In(time.FixedZone("TEST", -8))),
want: false,
},
{
// Values outside the UnixNano range were encoded incorrectly (#737, #803).
a: zap.Time("k", timeOutOfRange),
b: zap.Time("k", timeOutOfRangeNano),
want: false,
},
{
a: zap.Time("k", time.Unix(1000, 1000)),
b: zap.Time("k", time.Unix(1000, 2000)),
Expand Down