funcr: Escape strings when they are not known-safe. #113

thockin · 2021-10-13T16:26:26Z

before:

$ go test -benchtime=5s -bench='Funcr.*LogInfo' ./benchmark/
goos: linux
goarch: amd64
pkg: github.com/go-logr/logr/benchmark
cpu: Intel(R) Xeon(R) W-2135 CPU @ 3.70GHz
BenchmarkFuncrLogInfoOneArg-6            	 6273384	       977.0 ns/op
BenchmarkFuncrJSONLogInfoOneArg-6        	 5880464	      1027 ns/op
BenchmarkFuncrLogInfoSeveralArgs-6       	 3902252	      1570 ns/op
BenchmarkFuncrJSONLogInfoSeveralArgs-6   	 3601423	      1728 ns/op
BenchmarkFuncrLogInfoWithValues-6        	 3778357	      1578 ns/op
BenchmarkFuncrJSONLogInfoWithValues-6    	 3506097	      1664 ns/op
PASS
ok  	github.com/go-logr/logr/benchmark	44.903s

after:

$ go test -benchtime=5s -bench='Funcr.*LogInfo' ./benchmark/
goos: linux
goarch: amd64
pkg: github.com/go-logr/logr/benchmark
cpu: Intel(R) Xeon(R) W-2135 CPU @ 3.70GHz
BenchmarkFuncrLogInfoOneArg-6            	 7800596	       747.6 ns/op
BenchmarkFuncrJSONLogInfoOneArg-6        	 7395650	       821.1 ns/op
BenchmarkFuncrLogInfoSeveralArgs-6       	 3894525	      1539 ns/op
BenchmarkFuncrJSONLogInfoSeveralArgs-6   	 3534934	      1687 ns/op
BenchmarkFuncrLogInfoWithValues-6        	 3886315	      1539 ns/op
BenchmarkFuncrJSONLogInfoWithValues-6    	 3516794	      1751 ns/op
PASS
ok  	github.com/go-logr/logr/benchmark	44.172s

Fixes #112

pohly · 2021-10-13T17:30:55Z

funcr/funcr.go

 				buf.WriteByte(',')
 			}
-			// JSON only does string keys.
+			// prettyWithFlags will produce already-escaped values


Does that mean that

logr/funcr/funcr.go

Lines 410 to 415 in 3608a4b

case reflect.String:

if flags&flagRawString > 0 {

return v.String()

}

// This is empirically faster than strings.Builder.

return `"` + v.String() + `"`

should quote the value?

I'm also unsure about

logr/funcr/funcr.go

Lines 504 to 509 in 3608a4b

// prettyWithFlags will produce already-escaped values

buf.WriteByte('"')

buf.WriteString(f.prettyWithFlags(it.Key().Interface(), flagRawString))

buf.WriteByte('"')

buf.WriteByte(':')

buf.WriteString(f.pretty(it.Value().Interface()))

String members work, probably because case reflect.String: is never reached due to the case string: above. Should we remove the dead code?

Map values are also fine, but map keys are not. Here's a test case:

diff --git a/funcr/funcr_test.go b/funcr/funcr_test.go index 18ccb31..e7dedde 100644 --- a/funcr/funcr_test.go +++ b/funcr/funcr_test.go @@ -504,6 +504,17 @@ func TestRender(t *testing.T) { args: makeKV("bool\u00a0", true), // escaped expectKV: `""1""=1 "\tstr\n"="ABC" "bool\u00a0"=true`, expectJSON: `{""1"":1,"\tstr\n":"ABC","bool\u00a0":true}`, + }, { + name: "escape reflection", + args: makeKV("struct", struct { + Map map[string]string + String string + }{ + Map: map[string]string{`"quoted map key"`: `"quoted map value"`}, + String: `"quoted string value"`, + }), + expectKV: `"struct"={"Map":{"\"quoted map key\"":"\"quoted map value\""},"String":"\"quoted string value\""}`, + expectJSON: `{"struct":{"Map":{"\"quoted map key\"":"\"quoted map value\""},"String":"\"quoted string value\""}}`, }, { name: "missing value", builtins: makeKV("builtin"),

Should the code emit non-string map keys in those cases where JSON supports that? Right now it always seems to render as string.

Here's a test case for integer keys:

diff --git a/funcr/funcr_test.go b/funcr/funcr_test.go index 18ccb31..122a72c 100644 --- a/funcr/funcr_test.go +++ b/funcr/funcr_test.go @@ -518,6 +518,11 @@ func TestRender(t *testing.T) { args: makeKV(789, "val"), expectKV: `"<non-string-key: 123>"="val" "<non-string-key: 456>"="val" "<non-string-key: 789>"="val"`, expectJSON: `{"<non-string-key: 123>":"val","<non-string-key: 456>":"val","<non-string-key: 789>":"val"}`, + }, { + name: "int map keys", + values: makeKV("map", map[int]string{1: "first"}), + expectKV: `"map"={1:"first"}`, + expectJSON: `{"map":{1:"first"}}`, }, { name: "non-string key struct", builtins: makeKV(struct { // will not be escaped, but should never happen

Hmm, I may have misremembered. Looks like JSON doesn't support non-string keys. So the current behavior is correct, except for quoting.

Indeed. In several places! PTAL

thockin · 2021-10-13T22:04:53Z

Will look at it again to see if there are any further micro-optimizations to be had - nothing obvious...

thockin · 2021-10-14T00:09:52Z

Oh, I found one. Updating the PR comment.

thockin · 2021-10-14T00:24:24Z

PTAL

pohly · 2021-10-14T07:07:50Z

Looks much better. reflect.String is covered via substr (which probably stands for substitute string, not sub-string...).

But I am still wondering how well arbitrary maps are resp. should be supported. My two cents:

We aim for valid JSON output, so emitting invalid JSON is a bug and needs to be fixed.
Compatibility with encoding.json is nice, but not required.

With that in mind, here are some additional test cases. They document the current behavior even if that is broken:

diff --git a/funcr/funcr_test.go b/funcr/funcr_test.go
index a2b0d4e..3cefcab 100644
--- a/funcr/funcr_test.go
+++ b/funcr/funcr_test.go
@@ -27,6 +27,8 @@ import (
 	"github.com/go-logr/logr"
 )
 
+// This is not the same as a string and will be handled via reflection instead
+// of type assertions.
 type substr string
 
 func ptrint(i int) *int {
@@ -62,6 +64,14 @@ func (t Tstringer) Error() string {
 	return "Error(): you should not see this"
 }
 
+// point implements the encoding.TextMarshaler interface and thus can be used
+// as key in a map when encoding with encoding.json.
+type point struct{ x, y int }
+
+func (p point) MarshalText() ([]byte, error) {
+	return []byte(fmt.Sprintf("(%d, %d)", p.x, p.y)), nil
+}
+
 type TjsontagsString struct {
 	String1 string `json:"string1"`           // renamed
 	String2 string `json:"-"`                 // ignored
@@ -276,6 +286,33 @@ func TestPretty(t *testing.T) {
 				"with\tescape": 76,
 			},
 		},
+		{
+			val: map[int]int{
+				1: 2,
+			},
+		},
+		{
+			val: map[point]int{
+				{x: 1, y: 2}: 3,
+			},
+			// encoding.json produces: `{"(1, 2)":3}`
+			// funcr doesn't support encoding.TextMarshaler, so we get instead:
+			exp: `{"{}":3}`,
+		},
+		{
+			val: map[TjsontagsString]int{
+				{String1: `"quoted"`, String4: `unquoted`}: 1,
+			},
+			// Quoting is messed up. Should the suppressed field be include ("-")?
+			exp: `{"{"string1":"\"quoted\"","-":"","string4":"unquoted","String5":""}":1}`,
+		},
+		{
+			val: map[TjsontagsInt]int{
+				{Int1: 1, Int2: 2}: 3,
+			},
+			// Quoting is messed up. Should the suppressed field be include ("-")?
+			exp: `{"{"int1":1,"-":0,"Int5":0}":3}`,
+		},
 		{
 			val: struct {
 				X int `json:"x"`

thockin · 2021-10-14T19:15:53Z

Good test cases! Cleaned up even more.

Also added TextMarshaler support.

PTAL :)

pohly

I'm running out of ideas for breaking this 😁 Good job, let's merge it.

thockin assigned pohly Oct 13, 2021

thockin force-pushed the funcr-qstr branch from a92086f to ff8e9f8 Compare October 13, 2021 16:29

Fix lint warning

653e8e8

thockin force-pushed the funcr-qstr branch from fce95e4 to 3608a4b Compare October 13, 2021 16:37

pohly reviewed Oct 13, 2021

View reviewed changes

thockin force-pushed the funcr-qstr branch from 3608a4b to 91eb453 Compare October 13, 2021 22:00

thockin force-pushed the funcr-qstr branch from 91eb453 to 378bd75 Compare October 14, 2021 00:09

thockin force-pushed the funcr-qstr branch from 378bd75 to 77bdfcd Compare October 14, 2021 00:24

thockin changed the title ~~funcr: Escape keys when they are not known-safe.~~ funcr: Escape strings when they are not known-safe. Oct 14, 2021

thockin force-pushed the funcr-qstr branch from 77bdfcd to d92b5d4 Compare October 14, 2021 18:56

funcr: Escape strings when they are not known-safe

058c4ca

thockin force-pushed the funcr-qstr branch from d92b5d4 to 16030fe Compare October 14, 2021 19:15

func: Implement TextMarshaler for map keys

c9b68e6

thockin force-pushed the funcr-qstr branch from 16030fe to c9b68e6 Compare October 14, 2021 19:18

pohly approved these changes Oct 14, 2021

View reviewed changes

pohly merged commit 1215c81 into go-logr:master Oct 14, 2021

	case reflect.String:
	if flags&flagRawString > 0 {
	return v.String()
	}
	// This is empirically faster than strings.Builder.
	return `"` + v.String() + `"`

	// prettyWithFlags will produce already-escaped values
	buf.WriteByte('"')
	buf.WriteString(f.prettyWithFlags(it.Key().Interface(), flagRawString))
	buf.WriteByte('"')
	buf.WriteByte(':')
	buf.WriteString(f.pretty(it.Value().Interface()))

funcr: Escape strings when they are not known-safe. #113

funcr: Escape strings when they are not known-safe. #113

Uh oh!

Conversation

thockin commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pohly Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

pohly Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

pohly Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

pohly Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

pohly Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thockin Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

thockin commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thockin commented Oct 14, 2021

Uh oh!

thockin commented Oct 14, 2021

Uh oh!

pohly commented Oct 14, 2021

Uh oh!

thockin commented Oct 14, 2021

Uh oh!

pohly left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thockin commented Oct 13, 2021 •

edited

Loading

pohly Oct 13, 2021 •

edited

Loading

thockin commented Oct 13, 2021 •

edited

Loading