slog: add Level.MarshalJSON

Add a MarshalJSON method to Level so that it
marshals as a string.

This lets us remove special cases for Level in the handlers.

It also ensures that others who write handlers that output JSON will
get strings instead of numbers; the numbers are obscure,
uninformative, and could be confused with the values of other
level-numbering schemes.

Also, improve handler tests to check replacement.

Change-Id: Iedc7aa797469922fb9cfcc3b5d804806f3deb5d8
Reviewed-on: https://go-review.googlesource.com/c/exp/+/432183
Run-TryBot: Jonathan Amsterdam <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: jba

slog/handler.go

@@ -138,7 +138,9 @@ func (h *commonHandler) handle(r Record) error {
 		a = rep(a)
 		if a.Key() != "" {
 			app.appendKey(a.Key())
-			app.appendAttrValue(a)
+			if err := app.appendAttrValue(a); err != nil {
+				app.appendString(fmt.Sprintf("!ERROR:%v", err))
+			}
 		}
 	}
 
@@ -163,17 +165,7 @@ func (h *commonHandler) handle(r Record) error {
 			app.appendKey(key)
 			app.appendString(val.String())
 		} else {
-			// Don't use replace: we want to output Levels as strings
-			// to match the above.
-			a := rep(Any(key, val))
-			if a.Key() != "" {
-				app.appendKey(a.Key())
-				if l, ok := a.any.(Level); ok {
-					app.appendString(l.String())
-				} else {
-					app.appendAttrValue(a)
-				}
-			}
+			replace(Any(key, val))
 		}
 		app.appendSep()
 	}

slog/handler_test.go

@@ -51,11 +51,7 @@ func TestCommonHandle(t *testing.T) {
 		{
 			name: "cap keys",
 			h: &commonHandler{
-				opts: HandlerOptions{
-					ReplaceAttr: func(a Attr) Attr {
-						return a.WithKey(strings.ToUpper(a.Key()))
-					},
-				},
+				opts: HandlerOptions{ReplaceAttr: upperCaseKey},
 			},
 			want: "(TIME=2022-09-18T08:26:33.000Z;LEVEL=INFO;MSG=message;A=one;B=2)",
 		},
@@ -87,6 +83,10 @@ func TestCommonHandle(t *testing.T) {
 	}
 }
 
+func upperCaseKey(a Attr) Attr {
+	return a.WithKey(strings.ToUpper(a.Key()))
+}
+
 type testAppender struct {
 	buf *buffer.Buffer
 }

slog/json_handler.go

@@ -149,10 +149,7 @@ func (ap *jsonAppender) appendAttrValue(a Attr) error {
 			return err
 		}
 	case AnyKind:
-		v := a.Value()
-		if l, ok := v.(Level); ok {
-			ap.appendString(l.String())
-		} else if err := ap.appendJSONMarshal(v); err != nil {
+		if err := ap.appendJSONMarshal(a.Value()); err != nil {
 			return err
 		}
 	default:

slog/json_handler_test.go

@@ -12,25 +12,43 @@ import (
 	"io"
 	"math"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
 	"golang.org/x/exp/slog/internal/buffer"
 )
 
 func TestJSONHandler(t *testing.T) {
-	var buf bytes.Buffer
-	h := NewJSONHandler(&buf)
-	r := NewRecord(testTime, InfoLevel, "m", 0)
-	r.AddAttrs(Int("a", 1))
-	if err := h.Handle(r); err != nil {
-		t.Fatal(err)
-	}
-	got := buf.String()
-	got = got[:len(got)-1] // Remove final newline.
-	want := `{"time":"2000-01-02T03:04:05Z","level":"INFO","msg":"m","a":1}`
-	if got != want {
-		t.Errorf("\ngot  %s\nwant %s", got, want)
+	for _, test := range []struct {
+		name string
+		opts HandlerOptions
+		want string
+	}{
+		{
+			"none",
+			HandlerOptions{},
+			`{"time":"2000-01-02T03:04:05Z","level":"INFO","msg":"m","a":1,"m":{"b":2}}`,
+		},
+		{
+			"replace",
+			HandlerOptions{ReplaceAttr: upperCaseKey},
+			`{"TIME":"2000-01-02T03:04:05Z","LEVEL":"INFO","MSG":"m","A":1,"M":{"b":2}}`,
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			var buf bytes.Buffer
+			h := test.opts.NewJSONHandler(&buf)
+			r := NewRecord(testTime, InfoLevel, "m", 0)
+			r.AddAttrs(Int("a", 1), Any("m", map[string]int{"b": 2}))
+			if err := h.Handle(r); err != nil {
+				t.Fatal(err)
+			}
+			got := strings.TrimSuffix(buf.String(), "\n")
+			if got != test.want {
+				t.Errorf("\ngot  %s\nwant %s", got, test.want)
+			}
+		})
 	}
 }
 

slog/level.go

@@ -7,6 +7,7 @@ package slog
 import (
 	"fmt"
 	"math"
+	"strconv"
 	"sync/atomic"
 )
 
@@ -78,6 +79,13 @@ func (l Level) String() string {
 	}
 }
 
+func (l Level) MarshalJSON() ([]byte, error) {
+	// AppendQuote is sufficient for JSON-encoding all Level strings.
+	// They don't contain any runes that would produce invalid JSON
+	// when escaped.
+	return strconv.AppendQuote(nil, l.String()), nil
+}
+
 // An AtomicLevel is Level that can be read and written safely by multiple
 // goroutines.
 // Use NewAtomicLevel to create one.

slog/text_handler_test.go

@@ -21,50 +21,72 @@ var testTime = time.Date(2000, 1, 2, 3, 4, 5, 0, time.UTC)
 
 func TestTextHandler(t *testing.T) {
 	for _, test := range []struct {
-		name string
-		attr Attr
-		want string
+		name             string
+		attr             Attr
+		wantKey, wantVal string
 	}{
 		{
 			"unquoted",
 			Int("a", 1),
-			"a=1",
+			"a", "1",
 		},
 		{
 			"quoted",
 			String("x = y", `qu"o`),
-			`"x = y"="qu\"o"`,
+			`"x = y"`, `"qu\"o"`,
 		},
 		{
 			"Sprint",
 			Any("name", name{"Ren", "Hoek"}),
-			`name="Hoek, Ren"`,
+			`name`, `"Hoek, Ren"`,
 		},
 		{
 			"TextMarshaler",
 			Any("t", text{"abc"}),
-			`t="text{\"abc\"}"`,
+			`t`, `"text{\"abc\"}"`,
 		},
 		{
 			"TextMarshaler error",
 			Any("t", text{""}),
-			`t="!ERROR:text: empty string"`,
+			`t`, `"!ERROR:text: empty string"`,
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
-			var buf bytes.Buffer
-			h := NewTextHandler(&buf)
-			r := NewRecord(testTime, InfoLevel, "a message", 0)
-			r.AddAttrs(test.attr)
-			if err := h.Handle(r); err != nil {
-				t.Fatal(err)
-			}
-			got := buf.String()
-			// Remove final newline.
-			got = got[:len(got)-1]
-			want := `time=2000-01-02T03:04:05.000Z level=INFO msg="a message" ` + test.want
-			if got != want {
-				t.Errorf("\ngot  %s\nwant %s", got, want)
+			for _, opts := range []struct {
+				name       string
+				opts       HandlerOptions
+				wantPrefix string
+				modKey     func(string) string
+			}{
+				{
+					"none",
+					HandlerOptions{},
+					`time=2000-01-02T03:04:05.000Z level=INFO msg="a message"`,
+					func(s string) string { return s },
+				},
+				{
+					"replace",
+					HandlerOptions{ReplaceAttr: upperCaseKey},
+					`TIME=2000-01-02T03:04:05.000Z LEVEL=INFO MSG="a message"`,
+					strings.ToUpper,
+				},
+			} {
+				t.Run(opts.name, func(t *testing.T) {
+					var buf bytes.Buffer
+					h := opts.opts.NewTextHandler(&buf)
+					r := NewRecord(testTime, InfoLevel, "a message", 0)
+					r.AddAttrs(test.attr)
+					if err := h.Handle(r); err != nil {
+						t.Fatal(err)
+					}
+					got := buf.String()
+					// Remove final newline.
+					got = got[:len(got)-1]
+					want := opts.wantPrefix + " " + opts.modKey(test.wantKey) + "=" + test.wantVal
+					if got != want {
+						t.Errorf("\ngot  %s\nwant %s", got, want)
+					}
+				})
 			}
 		})
 	}