From 07d2631b91c5764107ccfd25bd55ff810c8c1376 Mon Sep 17 00:00:00 2001 From: Chris Hines Date: Thu, 6 Aug 2015 20:54:08 -0400 Subject: [PATCH] Append ErrMissingValue to odd length keyvals rather than panic. --- json_logger.go | 13 ++++++++----- json_logger_test.go | 15 ++++++++++++++- log.go | 42 ++++++++++++++++++++++++++---------------- log_test.go | 30 ++++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 22 deletions(-) diff --git a/json_logger.go b/json_logger.go index 8907c8f..9f98247 100644 --- a/json_logger.go +++ b/json_logger.go @@ -17,12 +17,15 @@ func NewJSONLogger(w io.Writer) Logger { } func (l *jsonLogger) Log(keyvals ...interface{}) error { - if len(keyvals)%2 == 1 { - panic("odd number of keyvals") - } - m := make(map[string]interface{}, len(keyvals)/2) + n := (len(keyvals) + 1) / 2 // +1 to handle case when len is odd + m := make(map[string]interface{}, n) for i := 0; i < len(keyvals); i += 2 { - merge(m, keyvals[i], keyvals[i+1]) + k := keyvals[i] + v := ErrMissingValue + if i+1 < len(keyvals) { + v = keyvals[i+1] + } + merge(m, k, v) } return json.NewEncoder(l.Writer).Encode(m) } diff --git a/json_logger_test.go b/json_logger_test.go index b668da4..f724783 100644 --- a/json_logger_test.go +++ b/json_logger_test.go @@ -10,13 +10,26 @@ import ( ) func TestJSONLogger(t *testing.T) { + t.Parallel() buf := &bytes.Buffer{} logger := log.NewJSONLogger(buf) if err := logger.Log("err", errors.New("err"), "m", map[string]int{"0": 0}, "a", []int{1, 2, 3}); err != nil { t.Fatal(err) } if want, have := `{"a":[1,2,3],"err":"err","m":{"0":0}}`+"\n", buf.String(); want != have { - t.Errorf("want %#v, have %#v", want, have) + t.Errorf("\nwant %#v\nhave %#v", want, have) + } +} + +func TestJSONLoggerMissingValue(t *testing.T) { + t.Parallel() + buf := &bytes.Buffer{} + logger := log.NewJSONLogger(buf) + if err := logger.Log("k"); err != nil { + t.Fatal(err) + } + if want, have := `{"k":"(MISSING)"}`+"\n", buf.String(); want != have { + t.Errorf("\nwant %#v\nhave %#v", want, have) } } diff --git a/log.go b/log.go index fe7fa5a..adde236 100644 --- a/log.go +++ b/log.go @@ -4,7 +4,10 @@ // key/value data. package log -import "sync/atomic" +import ( + "errors" + "sync/atomic" +) // Logger is the fundamental interface for all log operations. Log creates a // log event from keyvals, a variadic sequence of alternating keys and values. @@ -15,6 +18,10 @@ type Logger interface { Log(keyvals ...interface{}) error } +// ErrMissingValue may be appended to keyval slices with an odd length to +// populate the missing value. +var ErrMissingValue interface{} = errors.New("(MISSING)") + // NewContext returns a new Context that logs to logger. func NewContext(logger Logger) Context { if c, ok := logger.(Context); ok { @@ -37,10 +44,10 @@ type Context struct { // stored context with their generated value, appends keyvals, and passes the // result to the wrapped Logger. func (l Context) Log(keyvals ...interface{}) error { - if len(keyvals)%2 != 0 { - panic("bad keyvals") - } kvs := append(l.keyvals, keyvals...) + if len(kvs)%2 != 0 { + kvs = append(kvs, ErrMissingValue) + } if l.hasValuer { // If no keyvals were appended above then we must copy l.keyvals so // that future log events will reevaluate the stored Valuers. @@ -57,17 +64,17 @@ func (l Context) With(keyvals ...interface{}) Context { if len(keyvals) == 0 { return l } - if len(keyvals)%2 != 0 { - panic("bad keyvals") + kvs := append(l.keyvals, keyvals...) + if len(kvs)%2 != 0 { + kvs = append(kvs, ErrMissingValue) } - // Limiting the capacity of the stored keyvals ensures that a new - // backing array is created if the slice must grow in Log or With. - // Using the extra capacity without copying risks a data race that - // would violate the Logger interface contract. - n := len(l.keyvals) + len(keyvals) return Context{ - logger: l.logger, - keyvals: append(l.keyvals, keyvals...)[:n:n], + logger: l.logger, + // Limiting the capacity of the stored keyvals ensures that a new + // backing array is created if the slice must grow in Log or With. + // Using the extra capacity without copying risks a data race that + // would violate the Logger interface contract. + keyvals: kvs[:len(kvs):len(kvs)], hasValuer: l.hasValuer || containsValuer(keyvals), } } @@ -78,16 +85,19 @@ func (l Context) WithPrefix(keyvals ...interface{}) Context { if len(keyvals) == 0 { return l } - if len(keyvals)%2 != 0 { - panic("bad keyvals") - } // Limiting the capacity of the stored keyvals ensures that a new // backing array is created if the slice must grow in Log or With. // Using the extra capacity without copying risks a data race that // would violate the Logger interface contract. n := len(l.keyvals) + len(keyvals) + if len(keyvals)%2 != 0 { + n++ + } kvs := make([]interface{}, 0, n) kvs = append(kvs, keyvals...) + if len(kvs)%2 != 0 { + kvs = append(kvs, ErrMissingValue) + } kvs = append(kvs, l.keyvals...) return Context{ logger: l.logger, diff --git a/log_test.go b/log_test.go index c13b078..01173d0 100644 --- a/log_test.go +++ b/log_test.go @@ -11,6 +11,7 @@ import ( var discard = log.Logger(log.LoggerFunc(func(...interface{}) error { return nil })) func TestContext(t *testing.T) { + t.Parallel() buf := &bytes.Buffer{} logger := log.NewLogfmtLogger(buf) @@ -36,6 +37,35 @@ func TestContext(t *testing.T) { } } +func TestContextMissingValue(t *testing.T) { + t.Parallel() + var output []interface{} + logger := log.Logger(log.LoggerFunc(func(keyvals ...interface{}) error { + output = keyvals + return nil + })) + + lc := log.NewContext(logger) + + lc.Log("k") + if want, have := 2, len(output); want != have { + t.Errorf("want len(output) == %v, have %v", want, have) + } + if want, have := log.ErrMissingValue, output[1]; want != have { + t.Errorf("want %#v, have %#v", want, have) + } + + lc.With("k1").WithPrefix("k0").Log("k2") + if want, have := 6, len(output); want != have { + t.Errorf("want len(output) == %v, have %v", want, have) + } + for i := 1; i < 6; i += 2 { + if want, have := log.ErrMissingValue, output[i]; want != have { + t.Errorf("want output[%d] == %#v, have %#v", i, want, have) + } + } +} + // Test that With returns a Logger safe for concurrent use. This test // validates that the stored logging context does not get corrupted when // multiple clients concurrently log additional keyvals.