From e9f4bda47a9063599df43477df69ddad9250097c Mon Sep 17 00:00:00 2001
From: Abhinav Gupta <mail@abhinavg.net>
Date: Fri, 1 Sep 2023 10:34:56 -0700
Subject: [PATCH] zapslog: Handler should ignore an empty Attr (#1344)

Per the slog.Handler contract,
handlers should not log attributes that are equal to the zero value.
This is equivalent to Zap's `zap.Skip()`.

Discovered by #1335
Refs #1334
---
 exp/zapslog/handler.go      | 13 ++++++++++---
 exp/zapslog/handler_test.go | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/exp/zapslog/handler.go b/exp/zapslog/handler.go
index a24449390..e8ad81c43 100644
--- a/exp/zapslog/handler.go
+++ b/exp/zapslog/handler.go
@@ -159,6 +159,10 @@ func (h *Handler) Handle(ctx context.Context, record slog.Record) error {
 
 	fields := make([]zapcore.Field, 0, record.NumAttrs())
 	record.Attrs(func(attr slog.Attr) bool {
+		if attr.Equal(slog.Attr{}) {
+			return true // ignore empty attributes
+		}
+
 		fields = append(fields, convertAttrToField(attr))
 		return true
 	})
@@ -169,9 +173,12 @@ func (h *Handler) Handle(ctx context.Context, record slog.Record) error {
 // WithAttrs returns a new Handler whose attributes consist of
 // both the receiver's attributes and the arguments.
 func (h *Handler) WithAttrs(attrs []slog.Attr) slog.Handler {
-	fields := make([]zapcore.Field, len(attrs))
-	for i, attr := range attrs {
-		fields[i] = convertAttrToField(attr)
+	fields := make([]zapcore.Field, 0, len(attrs))
+	for _, attr := range attrs {
+		if attr.Equal(slog.Attr{}) {
+			continue // ignore empty attributes
+		}
+		fields = append(fields, convertAttrToField(attr))
 	}
 	return h.withFields(fields...)
 }
diff --git a/exp/zapslog/handler_test.go b/exp/zapslog/handler_test.go
index c5c75d991..2bee658fb 100644
--- a/exp/zapslog/handler_test.go
+++ b/exp/zapslog/handler_test.go
@@ -33,6 +33,8 @@ import (
 )
 
 func TestAddSource(t *testing.T) {
+	t.Parallel()
+
 	fac, logs := observer.New(zapcore.DebugLevel)
 	sl := slog.New(NewHandler(fac, &HandlerOptions{
 		AddSource: true,
@@ -48,3 +50,34 @@ func TestAddSource(t *testing.T) {
 		"Unexpected caller annotation.",
 	)
 }
+
+func TestEmptyAttr(t *testing.T) {
+	t.Parallel()
+
+	fac, observedLogs := observer.New(zapcore.DebugLevel)
+	sl := slog.New(NewHandler(fac, nil))
+
+	t.Run("Handle", func(t *testing.T) {
+		sl.Info(
+			"msg",
+			slog.String("foo", "bar"),
+			slog.Attr{},
+		)
+
+		logs := observedLogs.TakeAll()
+		require.Len(t, logs, 1, "Expected exactly one entry to be logged")
+		assert.Equal(t, map[string]any{
+			"foo": "bar",
+		}, logs[0].ContextMap(), "Unexpected context")
+	})
+
+	t.Run("WithAttrs", func(t *testing.T) {
+		sl.With(slog.String("foo", "bar"), slog.Attr{}).Info("msg")
+
+		logs := observedLogs.TakeAll()
+		require.Len(t, logs, 1, "Expected exactly one entry to be logged")
+		assert.Equal(t, map[string]any{
+			"foo": "bar",
+		}, logs[0].ContextMap(), "Unexpected context")
+	})
+}