From 653e8e85d7791a35ff00c03c58909680e5f67399 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Wed, 13 Oct 2021 09:30:45 -0700 Subject: [PATCH 1/3] Fix lint warning --- funcr/funcr.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/funcr/funcr.go b/funcr/funcr.go index 3ff7ff6..0391dcd 100644 --- a/funcr/funcr.go +++ b/funcr/funcr.go @@ -675,7 +675,8 @@ func (f *Formatter) AddName(name string) { func (f *Formatter) AddValues(kvList []interface{}) { // Three slice args forces a copy. n := len(f.values) - vals := append(f.values[:n:n], kvList...) + vals := f.values[:n:n] + vals = append(vals, kvList...) if hook := f.opts.RenderValuesHook; hook != nil { vals = hook(f.sanitize(vals)) } From 058c4cac916cf0bbaee9cf675d2feefe4e3b508f Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Wed, 13 Oct 2021 09:28:57 -0700 Subject: [PATCH 2/3] funcr: Escape strings when they are not known-safe --- funcr/funcr.go | 80 ++++++++++++++++++++++++++++++--------------- funcr/funcr_test.go | 78 ++++++++++++++++++++++++++++++++++++------- 2 files changed, 119 insertions(+), 39 deletions(-) diff --git a/funcr/funcr.go b/funcr/funcr.go index 0391dcd..14cd2e9 100644 --- a/funcr/funcr.go +++ b/funcr/funcr.go @@ -236,7 +236,7 @@ func (f Formatter) render(builtins, args []interface{}) string { if hook := f.opts.RenderBuiltinsHook; hook != nil { vals = hook(f.sanitize(vals)) } - f.flatten(buf, vals, false) + f.flatten(buf, vals, false, false) // keys are ours, no need to escape continuing := len(builtins) > 0 if len(f.valuesStr) > 0 { if continuing { @@ -253,7 +253,7 @@ func (f Formatter) render(builtins, args []interface{}) string { if hook := f.opts.RenderArgsHook; hook != nil { vals = hook(f.sanitize(vals)) } - f.flatten(buf, vals, continuing) + f.flatten(buf, vals, continuing, true) // escape user-provided keys if f.outputFormat == outputJSON { buf.WriteByte('}') } @@ -263,10 +263,13 @@ func (f Formatter) render(builtins, args []interface{}) string { // flatten renders a list of key-value pairs into a buffer. If continuing is // true, it assumes that the buffer has previous values and will emit a // separator (which depends on the output format) before the first pair it -// writes. This also returns a potentially modified version of kvList, which +// writes. If escapeKeys is true, the keys are assumed to have +// non-JSON-compatible characters in them and must be evaluated for escapes. +// +// This function returns a potentially modified version of kvList, which // ensures that there is a value for every key (adding a value if needed) and // that each key is a string (substituting a key if needed). -func (f Formatter) flatten(buf *bytes.Buffer, kvList []interface{}, continuing bool) []interface{} { +func (f Formatter) flatten(buf *bytes.Buffer, kvList []interface{}, continuing bool, escapeKeys bool) []interface{} { // This logic overlaps with sanitize() but saves one type-cast per key, // which can be measurable. if len(kvList)%2 != 0 { @@ -290,9 +293,14 @@ func (f Formatter) flatten(buf *bytes.Buffer, kvList []interface{}, continuing b } } - buf.WriteByte('"') - buf.WriteString(k) - buf.WriteByte('"') + if escapeKeys { + buf.WriteString(prettyString(k)) + } else { + // this is faster + buf.WriteByte('"') + buf.WriteString(k) + buf.WriteByte('"') + } if f.outputFormat == outputJSON { buf.WriteByte(':') } else { @@ -308,8 +316,7 @@ func (f Formatter) pretty(value interface{}) string { } const ( - flagRawString = 0x1 // do not print quotes on strings - flagRawStruct = 0x2 // do not print braces on structs + flagRawStruct = 0x1 // do not print braces on structs ) // TODO: This is not fast. Most of the overhead goes here. @@ -334,11 +341,7 @@ func (f Formatter) prettyWithFlags(value interface{}, flags uint32) string { case bool: return strconv.FormatBool(v) case string: - if flags&flagRawString > 0 { - return v - } - // This is empirically faster than strings.Builder. - return strconv.Quote(v) + return prettyString(v) case int: return strconv.FormatInt(int64(v), 10) case int8: @@ -379,9 +382,8 @@ func (f Formatter) prettyWithFlags(value interface{}, flags uint32) string { if i > 0 { buf.WriteByte(',') } - buf.WriteByte('"') - buf.WriteString(v[i].(string)) - buf.WriteByte('"') + // arbitrary keys might need escaping + buf.WriteString(prettyString(v[i].(string))) buf.WriteByte(':') buf.WriteString(f.pretty(v[i+1])) } @@ -401,11 +403,7 @@ func (f Formatter) prettyWithFlags(value interface{}, flags uint32) string { case reflect.Bool: return strconv.FormatBool(v.Bool()) case reflect.String: - if flags&flagRawString > 0 { - return v.String() - } - // This is empirically faster than strings.Builder. - return `"` + v.String() + `"` + return prettyString(v.String()) case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: return strconv.FormatInt(int64(v.Int()), 10) case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr: @@ -463,6 +461,7 @@ func (f Formatter) prettyWithFlags(value interface{}, flags uint32) string { if name == "" { name = fld.Name } + // field names can't contain characters which need escaping buf.WriteByte('"') buf.WriteString(name) buf.WriteByte('"') @@ -493,10 +492,14 @@ func (f Formatter) prettyWithFlags(value interface{}, flags uint32) string { if i > 0 { buf.WriteByte(',') } - // JSON only does string keys. - buf.WriteByte('"') - buf.WriteString(f.prettyWithFlags(it.Key().Interface(), flagRawString)) - buf.WriteByte('"') + // prettyWithFlags will produce already-escaped values + keystr := f.prettyWithFlags(it.Key().Interface(), 0) + if t.Key().Kind() != reflect.String { + // JSON only does string keys. Unlike Go's standard JSON, we'll + // convert just about anything to a string. + keystr = prettyString(keystr) + } + buf.WriteString(keystr) buf.WriteByte(':') buf.WriteString(f.pretty(it.Value().Interface())) i++ @@ -512,6 +515,29 @@ func (f Formatter) prettyWithFlags(value interface{}, flags uint32) string { return fmt.Sprintf(`""`, t.Kind().String()) } +func prettyString(s string) string { + // Avoid escaping (which does allocations) if we can. + if needsEscape(s) { + return strconv.Quote(s) + } + b := bytes.NewBuffer(make([]byte, 0, 1024)) + b.WriteByte('"') + b.WriteString(s) + b.WriteByte('"') + return b.String() +} + +// needsEscape determines whether the input string needs to be escaped or not, +// without doing any allocations. +func needsEscape(s string) bool { + for _, r := range s { + if !strconv.IsPrint(r) || r == '\\' || r == '"' { + return true + } + } + return false +} + func isEmpty(v reflect.Value) bool { switch v.Kind() { case reflect.Array, reflect.Map, reflect.Slice, reflect.String: @@ -683,7 +709,7 @@ func (f *Formatter) AddValues(kvList []interface{}) { // Pre-render values, so we don't have to do it on each Info/Error call. buf := bytes.NewBuffer(make([]byte, 0, 1024)) - f.values = f.flatten(buf, vals, false) + f.values = f.flatten(buf, vals, false, true) // escape user-provided keys f.valuesStr = buf.String() } diff --git a/funcr/funcr_test.go b/funcr/funcr_test.go index 9f99ab4..7816fcd 100644 --- a/funcr/funcr_test.go +++ b/funcr/funcr_test.go @@ -27,6 +27,7 @@ import ( "github.com/go-logr/logr" ) +// Will be handled via reflection instead of type assertions. type substr string func ptrint(i int) *int { @@ -198,7 +199,9 @@ func TestPretty(t *testing.T) { exp string // used in cases where JSON can't handle it }{ {val: "strval"}, + {val: "strval\nwith\t\"escapes\""}, {val: substr("substrval")}, + {val: substr("substrval\nwith\t\"escapes\"")}, {val: true}, {val: false}, {val: int(93)}, @@ -235,7 +238,11 @@ func TestPretty(t *testing.T) { exp: `[]`, }, {val: []int{9, 3, 7, 6}}, + {val: []string{"str", "with\tescape"}}, + {val: []substr{"substr", "with\tescape"}}, {val: [4]int{9, 3, 7, 6}}, + {val: [2]string{"str", "with\tescape"}}, + {val: [2]substr{"substr", "with\tescape"}}, { val: struct { Int int @@ -255,11 +262,32 @@ func TestPretty(t *testing.T) { "nine": 3, }, }, + { + val: map[string]int{ + "with\tescape": 76, + }, + }, { val: map[substr]int{ "nine": 3, }, }, + { + val: map[substr]int{ + "with\tescape": 76, + }, + }, + { + val: map[int]int{ + 9: 3, + }, + }, + { + val: map[float64]int{ + 9.5: 3, + }, + exp: `{"9.5":3}`, + }, { val: struct { X int `json:"x"` @@ -283,6 +311,7 @@ func TestPretty(t *testing.T) { val: []struct{ X, Y string }{ {"nine", "three"}, {"seven", "six"}, + {"with\t", "\tescapes"}, }, }, { @@ -438,6 +467,24 @@ func TestPretty(t *testing.T) { val: PseudoStruct(makeKV("f1", 1, "f2", true, "f3", []int{})), exp: `{"f1":1,"f2":true,"f3":[]}`, }, + { + val: map[TjsontagsString]int{ + {String1: `"quoted"`, String4: `unquoted`}: 1, + }, + exp: `{"{\"string1\":\"\\\"quoted\\\"\",\"-\":\"\",\"string4\":\"unquoted\",\"String5\":\"\"}":1}`, + }, + { + val: map[TjsontagsInt]int{ + {Int1: 1, Int2: 2}: 3, + }, + exp: `{"{\"int1\":1,\"-\":0,\"Int5\":0}":3}`, + }, + { + val: map[[2]struct{ S string }]int{ + {{S: `"quoted"`}, {S: "unquoted"}}: 1, + }, + exp: `{"[{\"S\":\"\\\"quoted\\\"\"},{\"S\":\"unquoted\"}]":1}`, + }, } f := NewFormatter(Options{}) @@ -449,7 +496,7 @@ func TestPretty(t *testing.T) { } else { jb, err := json.Marshal(tc.val) if err != nil { - t.Errorf("[%d]: unexpected error: %v", i, err) + t.Fatalf("[%d]: unexpected error: %v", i, err) } want = string(jb) } @@ -496,6 +543,13 @@ func TestRender(t *testing.T) { args: makeKV("bool", PseudoStruct(makeKV("boolsub", true))), expectKV: `"int"={"intsub":1} "str"={"strsub":"2"} "bool"={"boolsub":true}`, expectJSON: `{"int":{"intsub":1},"str":{"strsub":"2"},"bool":{"boolsub":true}}`, + }, { + name: "escapes", + builtins: makeKV("\"1\"", 1), // will not be escaped, but should never happen + values: makeKV("\tstr", "ABC"), // escaped + args: makeKV("bool\n", true), // escaped + expectKV: `""1""=1 "\tstr"="ABC" "bool\n"=true`, + expectJSON: `{""1"":1,"\tstr":"ABC","bool\n":true}`, }, { name: "missing value", builtins: makeKV("builtin"), @@ -505,27 +559,27 @@ func TestRender(t *testing.T) { expectJSON: `{"builtin":"","value":"","arg":""}`, }, { name: "non-string key int", - args: makeKV(123, "val"), + builtins: makeKV(123, "val"), // should never happen values: makeKV(456, "val"), - builtins: makeKV(789, "val"), - expectKV: `""="val" ""="val" ""="val"`, - expectJSON: `{"":"val","":"val","":"val"}`, + args: makeKV(789, "val"), + expectKV: `""="val" ""="val" ""="val"`, + expectJSON: `{"":"val","":"val","":"val"}`, }, { name: "non-string key struct", - args: makeKV(struct { + builtins: makeKV(struct { // will not be escaped, but should never happen F1 string F2 int - }{"arg", 123}, "val"), + }{"builtin", 123}, "val"), values: makeKV(struct { F1 string F2 int }{"value", 456}, "val"), - builtins: makeKV(struct { + args: makeKV(struct { F1 string F2 int - }{"builtin", 789}, "val"), - expectKV: `""="val" ""="val" ""="val"`, - expectJSON: `{"":"val","":"val","":"val"}`, + }{"arg", 789}, "val"), + expectKV: `""="val" ""="val" ""="val"`, + expectJSON: `{"":"val","":"val","":"val"}`, }} for _, tc := range testCases { @@ -534,7 +588,7 @@ func TestRender(t *testing.T) { formatter.AddValues(tc.values) r := formatter.render(tc.builtins, tc.args) if r != expect { - t.Errorf("wrong output:\nexpected %q\n got %q", expect, r) + t.Errorf("wrong output:\nexpected %v\n got %v", expect, r) } } t.Run("KV", func(t *testing.T) { From c9b68e6ce73891fb4d29cfaf9f34c5b1c8ab0ebf Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 14 Oct 2021 12:14:37 -0700 Subject: [PATCH 3/3] func: Implement TextMarshaler for map keys --- funcr/funcr.go | 23 ++++++++++++++++++----- funcr/funcr_test.go | 27 ++++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/funcr/funcr.go b/funcr/funcr.go index 14cd2e9..121bf4d 100644 --- a/funcr/funcr.go +++ b/funcr/funcr.go @@ -36,6 +36,7 @@ package funcr import ( "bytes" + "encoding" "fmt" "path/filepath" "reflect" @@ -492,12 +493,24 @@ func (f Formatter) prettyWithFlags(value interface{}, flags uint32) string { if i > 0 { buf.WriteByte(',') } - // prettyWithFlags will produce already-escaped values - keystr := f.prettyWithFlags(it.Key().Interface(), 0) - if t.Key().Kind() != reflect.String { - // JSON only does string keys. Unlike Go's standard JSON, we'll - // convert just about anything to a string. + // If a map key supports TextMarshaler, use it. + keystr := "" + if m, ok := it.Key().Interface().(encoding.TextMarshaler); ok { + txt, err := m.MarshalText() + if err != nil { + keystr = fmt.Sprintf("", err.Error()) + } else { + keystr = string(txt) + } keystr = prettyString(keystr) + } else { + // prettyWithFlags will produce already-escaped values + keystr = f.prettyWithFlags(it.Key().Interface(), 0) + if t.Key().Kind() != reflect.String { + // JSON only does string keys. Unlike Go's standard JSON, we'll + // convert just about anything to a string. + keystr = prettyString(keystr) + } } buf.WriteString(keystr) buf.WriteByte(':') diff --git a/funcr/funcr_test.go b/funcr/funcr_test.go index 7816fcd..a33a998 100644 --- a/funcr/funcr_test.go +++ b/funcr/funcr_test.go @@ -37,6 +37,20 @@ func ptrstr(s string) *string { return &s } +// point implements encoding.TextMarshaler and can be used as a map key. +type point struct{ x, y int } + +func (p point) MarshalText() ([]byte, error) { + return []byte(fmt.Sprintf("(%d, %d)", p.x, p.y)), nil +} + +// pointErr implements encoding.TextMarshaler but returns an error. +type pointErr struct{ x, y int } + +func (p pointErr) MarshalText() ([]byte, error) { + return nil, fmt.Errorf("uh oh: %d, %d", p.x, p.y) +} + // Logging this should result in the MarshalLog() value. type Tmarshaler string @@ -288,6 +302,17 @@ func TestPretty(t *testing.T) { }, exp: `{"9.5":3}`, }, + { + val: map[point]int{ + {x: 1, y: 2}: 3, + }, + }, + { + val: map[pointErr]int{ + {x: 1, y: 2}: 3, + }, + exp: `{"":3}`, + }, { val: struct { X int `json:"x"` @@ -496,7 +521,7 @@ func TestPretty(t *testing.T) { } else { jb, err := json.Marshal(tc.val) if err != nil { - t.Fatalf("[%d]: unexpected error: %v", i, err) + t.Fatalf("[%d]: unexpected error: %v\ngot: %q", i, err, ours) } want = string(jb) }