From c250aa1e401dbc94b092a29b8c7f6127121c82c0 Mon Sep 17 00:00:00 2001
From: Abhinav Gupta <abg@uber.com>
Date: Sat, 14 Mar 2020 09:37:14 -0700
Subject: [PATCH] json: AppendTimeLayout should add field separators (#799)

In #786, we added support for improving the performance of encoding time
by allowing encoder implementations to implement AppendTimeLayout.

If the encoder implements, `AppendTimeLayout(time.Time, string)`, some
of the time encoders shipped with Zap will use that method, which in
turn can make use of `Buffer.AppendTime`.

That change inadvertently broke the JSON output for arrays of time that
make use of that functionality.

Compare the old version of AppendTimeLayout with AppendString (both
operations append a string to the array).

    func (enc *jsonEncoder) AppendString(val string) {
      enc.addElementSeparator()
      enc.buf.AppendByte('"')
      // ...
    }

    func (enc *jsonEncoder) AppendTimeLayout(time time.Time, layout string) {
      enc.buf.AppendByte('"')
      // ...
    }

Without the `enc.addElementSeparator` call, `AppendTimeLayout` does not
include the `,` separator between array elements, instead yielding the
following.

    ["2001-12-19T00:00:00Z""2002-12-18T00:00:00Z""2003-12-17T00:00:00Z"]

Fixes #798
---
 zapcore/json_encoder.go           |  1 +
 zapcore/json_encoder_impl_test.go | 52 +++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/zapcore/json_encoder.go b/zapcore/json_encoder.go
index 07f1897ed..7facc1b36 100644
--- a/zapcore/json_encoder.go
+++ b/zapcore/json_encoder.go
@@ -267,6 +267,7 @@ func (enc *jsonEncoder) AppendString(val string) {
 }
 
 func (enc *jsonEncoder) AppendTimeLayout(time time.Time, layout string) {
+	enc.addElementSeparator()
 	enc.buf.AppendByte('"')
 	enc.buf.AppendTime(time, layout)
 	enc.buf.AppendByte('"')
diff --git a/zapcore/json_encoder_impl_test.go b/zapcore/json_encoder_impl_test.go
index 81e3ed384..3f11a8cca 100644
--- a/zapcore/json_encoder_impl_test.go
+++ b/zapcore/json_encoder_impl_test.go
@@ -385,6 +385,58 @@ func TestJSONEncoderArrays(t *testing.T) {
 	}
 }
 
+func TestJSONEncoderTimeArrays(t *testing.T) {
+	times := []time.Time{
+		time.Unix(1008720000, 0).UTC(), // 2001-12-19
+		time.Unix(1040169600, 0).UTC(), // 2002-12-18
+		time.Unix(1071619200, 0).UTC(), // 2003-12-17
+	}
+
+	tests := []struct {
+		desc    string
+		encoder TimeEncoder
+		want    string
+	}{
+		{
+			desc:    "epoch",
+			encoder: EpochTimeEncoder,
+			want:    `[1008720000,1040169600,1071619200]`,
+		},
+		{
+			desc:    "epoch millis",
+			encoder: EpochMillisTimeEncoder,
+			want:    `[1008720000000,1040169600000,1071619200000]`,
+		},
+		{
+			desc:    "iso8601",
+			encoder: ISO8601TimeEncoder,
+			want:    `["2001-12-19T00:00:00.000Z","2002-12-18T00:00:00.000Z","2003-12-17T00:00:00.000Z"]`,
+		},
+		{
+			desc:    "rfc3339",
+			encoder: RFC3339TimeEncoder,
+			want:    `["2001-12-19T00:00:00Z","2002-12-18T00:00:00Z","2003-12-17T00:00:00Z"]`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			cfg := _defaultEncoderConfig
+			cfg.EncodeTime = tt.encoder
+
+			enc := &jsonEncoder{buf: bufferpool.Get(), EncoderConfig: &cfg}
+			err := enc.AddArray("array", ArrayMarshalerFunc(func(arr ArrayEncoder) error {
+				for _, time := range times {
+					arr.AppendTime(time)
+				}
+				return nil
+			}))
+			assert.NoError(t, err)
+			assert.Equal(t, `"array":`+tt.want, enc.buf.String())
+		})
+	}
+}
+
 func assertJSON(t *testing.T, expected string, enc *jsonEncoder) {
 	assert.Equal(t, expected, enc.buf.String(), "Encoded JSON didn't match expectations.")
 }