From 9b7c3bfda25cac156aa2d91ff2d93bbb07cd3fd7 Mon Sep 17 00:00:00 2001 From: Evan Phoenix Date: Fri, 10 Sep 2021 16:12:28 -0700 Subject: [PATCH] Adjust the rules for deciding to quote a string value --- intlogger.go | 24 ++++++++++++--- logger_loc_test.go | 59 +++++++++++++++++++++++++++++++++++ logger_test.go | 77 ++++++++++++++++++++-------------------------- 3 files changed, 113 insertions(+), 47 deletions(-) create mode 100644 logger_loc_test.go diff --git a/intlogger.go b/intlogger.go index d491ae8..fd51af1 100644 --- a/intlogger.go +++ b/intlogger.go @@ -199,6 +199,24 @@ func trimCallerPath(path string) string { return path[idx+1:] } +// isNormal indicates if the rune is one allowed to exist as an unquoted +// string value. This is a subset of ASCII, `-` through `~`. +func isNormal(r rune) bool { + return 0x2D <= r && r <= 0x7E // - through ~ +} + +// needsQuoting returns false if all the runes in string are normal, according +// to isNormal +func needsQuoting(str string) bool { + for _, r := range str { + if !isNormal(r) { + return true + } + } + + return false +} + // Non-JSON logging format function func (l *intLogger) logPlain(t time.Time, name string, level Level, msg string, args ...interface{}) { @@ -323,13 +341,11 @@ func (l *intLogger) logPlain(t time.Time, name string, level Level, msg string, l.writer.WriteString("=\n") writeIndent(l.writer, val, " | ") l.writer.WriteString(" ") - } else if !raw && strings.ContainsAny(val, " \t") { + } else if !raw && needsQuoting(val) { l.writer.WriteByte(' ') l.writer.WriteString(key) l.writer.WriteByte('=') - l.writer.WriteByte('"') - l.writer.WriteString(val) - l.writer.WriteByte('"') + l.writer.WriteString(strconv.Quote(val)) } else { l.writer.WriteByte(' ') l.writer.WriteString(key) diff --git a/logger_loc_test.go b/logger_loc_test.go new file mode 100644 index 0000000..c9cc991 --- /dev/null +++ b/logger_loc_test.go @@ -0,0 +1,59 @@ +package hclog + +import ( + "bytes" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +// This file contains tests that are sensitive to their location in the file, +// because they contain line numbers. They're basically "quarantined" from the +// other tests because they break all the time when new tests are added. + +func TestLoggerLoc(t *testing.T) { + t.Run("includes the caller location", func(t *testing.T) { + var buf bytes.Buffer + + logger := New(&LoggerOptions{ + Name: "test", + Output: &buf, + IncludeLocation: true, + }) + + logger.Info("this is test", "who", "programmer", "why", "testing is fun") + + str := buf.String() + dataIdx := strings.IndexByte(str, ' ') + rest := str[dataIdx+1:] + + // This test will break if you move this around, it's line dependent, just fyi + assert.Equal(t, "[INFO] go-hclog/logger_loc_test.go:25: test: this is test: who=programmer why=\"testing is fun\"\n", rest) + }) + + t.Run("includes the caller location excluding helper functions", func(t *testing.T) { + var buf bytes.Buffer + + logMe := func(l Logger) { + l.Info("this is test", "who", "programmer", "why", "testing is fun") + } + + logger := New(&LoggerOptions{ + Name: "test", + Output: &buf, + IncludeLocation: true, + AdditionalLocationOffset: 1, + }) + + logMe(logger) + + str := buf.String() + dataIdx := strings.IndexByte(str, ' ') + rest := str[dataIdx+1:] + + // This test will break if you move this around, it's line dependent, just fyi + assert.Equal(t, "[INFO] go-hclog/logger_loc_test.go:49: test: this is test: who=programmer why=\"testing is fun\"\n", rest) + }) + +} diff --git a/logger_test.go b/logger_test.go index 656cfb2..403c699 100644 --- a/logger_test.go +++ b/logger_test.go @@ -103,7 +103,7 @@ func TestLogger(t *testing.T) { assert.Equal(t, "[INFO] test: this is test: who=programmer why=[\"testing & qa\", \"dev\"]\n", rest) }) - t.Run("formats multiline values nicely", func(t *testing.T) { + t.Run("escapes quotes in values", func(t *testing.T) { var buf bytes.Buffer logger := New(&LoggerOptions{ @@ -111,21 +111,16 @@ func TestLogger(t *testing.T) { Output: &buf, }) - logger.Info("this is test", "who", "programmer", "why", "testing\nand other\npretty cool things") + logger.Info("this is test", "who", "programmer", "why", `this is "quoted"`) str := buf.String() dataIdx := strings.IndexByte(str, ' ') rest := str[dataIdx+1:] - expected := `[INFO] test: this is test: who=programmer - why= - | testing - | and other - | pretty cool things` + "\n \n" - assert.Equal(t, expected, rest) + assert.Equal(t, `[INFO] test: this is test: who=programmer why="this is \"quoted\""`+"\n", rest) }) - t.Run("outputs stack traces", func(t *testing.T) { + t.Run("quotes when there are nonprintable sequences in a value", func(t *testing.T) { var buf bytes.Buffer logger := New(&LoggerOptions{ @@ -133,15 +128,16 @@ func TestLogger(t *testing.T) { Output: &buf, }) - logger.Info("who", "programmer", "why", "testing", Stacktrace()) + logger.Info("this is test", "who", "programmer", "why", "\U0001F603") - lines := strings.Split(buf.String(), "\n") - require.True(t, len(lines) > 1) + str := buf.String() + dataIdx := strings.IndexByte(str, ' ') + rest := str[dataIdx+1:] - assert.Equal(t, "github.com/hashicorp/go-hclog.Stacktrace", lines[1]) + assert.Equal(t, "[INFO] test: this is test: who=programmer why=\"\U0001F603\"\n", rest) }) - t.Run("outputs stack traces with it's given a name", func(t *testing.T) { + t.Run("formats multiline values nicely", func(t *testing.T) { var buf bytes.Buffer logger := New(&LoggerOptions{ @@ -149,55 +145,50 @@ func TestLogger(t *testing.T) { Output: &buf, }) - logger.Info("who", "programmer", "why", "testing", "foo", Stacktrace()) + logger.Info("this is test", "who", "programmer", "why", "testing\nand other\npretty cool things") - lines := strings.Split(buf.String(), "\n") - require.True(t, len(lines) > 1) + str := buf.String() + dataIdx := strings.IndexByte(str, ' ') + rest := str[dataIdx+1:] - assert.Equal(t, "github.com/hashicorp/go-hclog.Stacktrace", lines[1]) + expected := `[INFO] test: this is test: who=programmer + why= + | testing + | and other + | pretty cool things` + "\n \n" + assert.Equal(t, expected, rest) }) - t.Run("includes the caller location", func(t *testing.T) { + t.Run("outputs stack traces", func(t *testing.T) { var buf bytes.Buffer logger := New(&LoggerOptions{ - Name: "test", - Output: &buf, - IncludeLocation: true, + Name: "test", + Output: &buf, }) - logger.Info("this is test", "who", "programmer", "why", "testing is fun") + logger.Info("who", "programmer", "why", "testing", Stacktrace()) - str := buf.String() - dataIdx := strings.IndexByte(str, ' ') - rest := str[dataIdx+1:] + lines := strings.Split(buf.String(), "\n") + require.True(t, len(lines) > 1) - // This test will break if you move this around, it's line dependent, just fyi - assert.Equal(t, "[INFO] go-hclog/logger_test.go:169: test: this is test: who=programmer why=\"testing is fun\"\n", rest) + assert.Equal(t, "github.com/hashicorp/go-hclog.Stacktrace", lines[1]) }) - t.Run("includes the caller location excluding helper functions", func(t *testing.T) { + t.Run("outputs stack traces with it's given a name", func(t *testing.T) { var buf bytes.Buffer - logMe := func(l Logger) { - l.Info("this is test", "who", "programmer", "why", "testing is fun") - } - logger := New(&LoggerOptions{ - Name: "test", - Output: &buf, - IncludeLocation: true, - AdditionalLocationOffset: 1, + Name: "test", + Output: &buf, }) - logMe(logger) + logger.Info("who", "programmer", "why", "testing", "foo", Stacktrace()) - str := buf.String() - dataIdx := strings.IndexByte(str, ' ') - rest := str[dataIdx+1:] + lines := strings.Split(buf.String(), "\n") + require.True(t, len(lines) > 1) - // This test will break if you move this around, it's line dependent, just fyi - assert.Equal(t, "[INFO] go-hclog/logger_test.go:193: test: this is test: who=programmer why=\"testing is fun\"\n", rest) + assert.Equal(t, "github.com/hashicorp/go-hclog.Stacktrace", lines[1]) }) t.Run("prefixes the name", func(t *testing.T) {