From 0022bd37a7f2483312950bcafbb3916ab76635ec Mon Sep 17 00:00:00 2001 From: Jonathan Amsterdam Date: Wed, 19 Apr 2023 16:51:05 -0400 Subject: [PATCH] log/slog: remove special float handling from JSONHandler Remove the special-case handling of NaN and infinities from appendJSONValue, making JSONHandler behave almost exactly like a json.Encoder without HTML escaping. The only differences are: - Encoding errors are turned into strings, instead of causing the Handle method to fail. - Values of type `error` are displayed as strings by calling their `Error` method. Fixes #59345. Change-Id: Id06bd952bbfef596e864bd5fd3f9f4f178f738c9 Reviewed-on: https://go-review.googlesource.com/c/go/+/486855 TryBot-Result: Gopher Robot Run-TryBot: Jonathan Amsterdam Reviewed-by: Alan Donovan --- src/log/slog/json_handler.go | 42 ++++++++++++------------------- src/log/slog/json_handler_test.go | 20 +++++++-------- 2 files changed, 26 insertions(+), 36 deletions(-) diff --git a/src/log/slog/json_handler.go b/src/log/slog/json_handler.go index a99a99f1c1..ec25771245 100644 --- a/src/log/slog/json_handler.go +++ b/src/log/slog/json_handler.go @@ -12,7 +12,6 @@ import ( "fmt" "io" "log/slog/internal/buffer" - "math" "strconv" "time" "unicode/utf8" @@ -75,12 +74,16 @@ func (h *JSONHandler) WithGroup(name string) Handler { // To modify these or other attributes, or remove them from the output, use // [HandlerOptions.ReplaceAttr]. // -// Values are formatted as with encoding/json.Marshal, with the following -// exceptions: -// - Floating-point NaNs and infinities are formatted as one of the strings -// "NaN", "Infinity" or "-Infinity". -// - Levels are formatted as with Level.String. -// - HTML characters are not escaped. +// Values are formatted as with an [encoding/json.Encoder] with SetEscapeHTML(false), +// with two exceptions. +// +// First, an Attr whose Value is of type error is formatted as a string, by +// calling its Error method. Only errors in Attrs receive this special treatment, +// not errors embedded in structs, slices, maps or other data structures that +// are processed by the encoding/json package. +// +// Second, an encoding failure does not cause Handle to return an error. +// Instead, the error message is formatted as a string. // // Each call to Handle results in a single serialized call to io.Writer.Write. func (h *JSONHandler) Handle(_ context.Context, r Record) error { @@ -108,22 +111,11 @@ func appendJSONValue(s *handleState, v Value) error { case KindUint64: *s.buf = strconv.AppendUint(*s.buf, v.Uint64(), 10) case KindFloat64: - f := v.Float64() - // json.Marshal fails on special floats, so handle them here. - switch { - case math.IsInf(f, 1): - s.buf.WriteString(`"Infinity"`) - case math.IsInf(f, -1): - s.buf.WriteString(`"-Infinity"`) - case math.IsNaN(f): - s.buf.WriteString(`"NaN"`) - default: - // json.Marshal is funny about floats; it doesn't - // always match strconv.AppendFloat. So just call it. - // That's expensive, but floats are rare. - if err := appendJSONMarshal(s.buf, f); err != nil { - return err - } + // json.Marshal is funny about floats; it doesn't + // always match strconv.AppendFloat. So just call it. + // That's expensive, but floats are rare. + if err := appendJSONMarshal(s.buf, v.Float64()); err != nil { + return err } case KindBool: *s.buf = strconv.AppendBool(*s.buf, v.Bool()) @@ -163,9 +155,7 @@ func appendJSONMarshal(buf *buffer.Buffer, v any) error { // It does not surround the string in quotation marks. // // Modified from encoding/json/encode.go:encodeState.string, -// with escapeHTML set to true. -// -// TODO: review whether HTML escaping is necessary. +// with escapeHTML set to false. func appendEscapedJSONString(buf []byte, s string) []byte { char := func(b byte) { buf = append(buf, b) } str := func(s string) { buf = append(buf, s...) } diff --git a/src/log/slog/json_handler_test.go b/src/log/slog/json_handler_test.go index d8457cb9ee..61078caec8 100644 --- a/src/log/slog/json_handler_test.go +++ b/src/log/slog/json_handler_test.go @@ -74,7 +74,7 @@ type jsonMarshalerError struct { func (jsonMarshalerError) Error() string { return "oops" } func TestAppendJSONValue(t *testing.T) { - // On most values, jsonAppendAttrValue should agree with json.Marshal. + // jsonAppendAttrValue should always agree with json.Marshal. for _, value := range []any{ "hello", `"[{escape}]"`, @@ -89,8 +89,9 @@ func TestAppendJSONValue(t *testing.T) { testTime, jsonMarshaler{"xyz"}, jsonMarshalerError{jsonMarshaler{"pqr"}}, + LevelWarn, } { - got := jsonValueString(t, AnyValue(value)) + got := jsonValueString(AnyValue(value)) want, err := marshalJSON(value) if err != nil { t.Fatal(err) @@ -117,24 +118,23 @@ func TestJSONAppendAttrValueSpecial(t *testing.T) { value any want string }{ - {math.NaN(), `"NaN"`}, - {math.Inf(+1), `"Infinity"`}, - {math.Inf(-1), `"-Infinity"`}, - {LevelWarn, `"WARN"`}, + {math.NaN(), `"!ERROR:json: unsupported value: NaN"`}, + {math.Inf(+1), `"!ERROR:json: unsupported value: +Inf"`}, + {math.Inf(-1), `"!ERROR:json: unsupported value: -Inf"`}, + {io.EOF, `"EOF"`}, } { - got := jsonValueString(t, AnyValue(test.value)) + got := jsonValueString(AnyValue(test.value)) if got != test.want { t.Errorf("%v: got %s, want %s", test.value, got, test.want) } } } -func jsonValueString(t *testing.T, v Value) string { - t.Helper() +func jsonValueString(v Value) string { var buf []byte s := &handleState{h: &commonHandler{json: true}, buf: (*buffer.Buffer)(&buf)} if err := appendJSONValue(s, v); err != nil { - t.Fatal(err) + s.appendError(err) } return string(buf) } -- 2.48.1