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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions field.go
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, math.MinInt64)
_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
5 changes: 5 additions & 0 deletions field_test.go
Expand Up @@ -21,6 +21,7 @@
package zap

import (
"math"
"net"
"sync"
"testing"
Expand Down Expand Up @@ -107,6 +108,10 @@ 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.MinInt64, Interface: time.UTC}, Time("k", time.Unix(0, math.MinInt64).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
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
19 changes: 19 additions & 0 deletions zapcore/field_test.go
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,14 @@ func TestFields(t *testing.T) {
}

func TestEquals(t *testing.T) {
// Values outside the UnixNano range were encoded incorrectly (#737, #803).
timeOutOfRangeHigh := time.Unix(0, math.MaxInt64).Add(time.Nanosecond)
timeOutOfRangeLow := time.Unix(0, math.MinInt64).Add(-time.Nanosecond)
timeOutOfRangeHighNano := time.Unix(0, timeOutOfRangeHigh.UnixNano())
timeOutOfRangeLowNano := time.Unix(0, timeOutOfRangeLow.UnixNano())
require.False(t, timeOutOfRangeHigh.Equal(timeOutOfRangeHighNano), "should be different as value is > UnixNano range")
require.False(t, timeOutOfRangeHigh.Equal(timeOutOfRangeHighNano), "should be different as value is < UnixNano range")

tests := []struct {
a, b Field
want bool
Expand Down Expand Up @@ -198,6 +207,16 @@ func TestEquals(t *testing.T) {
b: zap.Time("k", time.Unix(1000, 1000).In(time.FixedZone("TEST", -8))),
want: false,
},
{
a: zap.Time("k", timeOutOfRangeLow),
b: zap.Time("k", timeOutOfRangeLowNano),
want: false,
},
{
a: zap.Time("k", timeOutOfRangeHigh),
b: zap.Time("k", timeOutOfRangeHighNano),
want: false,
},
{
a: zap.Time("k", time.Unix(1000, 1000)),
b: zap.Time("k", time.Unix(1000, 2000)),
Expand Down