Merge pull request #99 from hashicorp/bug/quotes

Adjust the rules for deciding to quote a string value

Author: Evan Phoenix

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)

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)
+	})
+
+}

logger_test.go

@@ -103,101 +103,92 @@ 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{
 			Name:   "test",
 			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{
 			Name:   "test",
 			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{
 			Name:   "test",
 			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) {