From ffefa3288c8af9ec6cfdcebab0bfee0737b04e73 Mon Sep 17 00:00:00 2001
From: Patrick Ohly <patrick.ohly@intel.com>
Date: Wed, 9 Feb 2022 20:01:11 +0100
Subject: [PATCH] klogr: improve handling of missing value in WithValues

This is invalid:
   logger.WithValue("keyWithoutValue").WithValue("anotherKeyWithoutValue")

Because it concatenated key/values without checking, klogr treated this like:
   logger.WithValue("keyWithoutValue", "anotherKeyWithoutValue")

Now the parameters are checked and a "(MISSING)" is appended as in the other
cases where a missing value is detected.
---
 internal/serialize/keyvalues.go | 22 ++++++++++++++++++++++
 klogr/klogr.go                  |  4 +---
 klogr/klogr_test.go             |  9 +++++++++
 test/output_test.go             | 12 ------------
 4 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/internal/serialize/keyvalues.go b/internal/serialize/keyvalues.go
index c3211ec58..4f583912f 100644
--- a/internal/serialize/keyvalues.go
+++ b/internal/serialize/keyvalues.go
@@ -22,6 +22,28 @@ import (
 	"strconv"
 )
 
+// WithValues implements LogSink.WithValues. The old key/value pairs are
+// assumed to be well-formed, the new ones are checked and padded if
+// necessary. It returns a new slice.
+func WithValues(oldKV, newKV []interface{}) []interface{} {
+	if len(newKV) == 0 {
+		return oldKV
+	}
+	newLen := len(oldKV) + len(newKV)
+	hasMissingValue := newLen%2 != 0
+	if hasMissingValue {
+		newLen++
+	}
+	// The new LogSink must have its own slice.
+	kv := make([]interface{}, 0, newLen)
+	kv = append(kv, oldKV...)
+	kv = append(kv, newKV...)
+	if hasMissingValue {
+		kv = append(kv, missingValue)
+	}
+	return kv
+}
+
 // TrimDuplicates deduplicates elements provided in multiple key/value tuple
 // slices, whilst maintaining the distinction between where the items are
 // contained.
diff --git a/klogr/klogr.go b/klogr/klogr.go
index d8e660e45..02433e844 100644
--- a/klogr/klogr.go
+++ b/klogr/klogr.go
@@ -170,9 +170,7 @@ func (l klogger) WithName(name string) logr.LogSink {
 }
 
 func (l klogger) WithValues(kvList ...interface{}) logr.LogSink {
-	// Three slice args forces a copy.
-	n := len(l.values)
-	l.values = append(l.values[:n:n], kvList...)
+	l.values = serialize.WithValues(l.values, kvList)
 	return &l
 }
 
diff --git a/klogr/klogr_test.go b/klogr/klogr_test.go
index 45565b271..ab3a525a5 100644
--- a/klogr/klogr_test.go
+++ b/klogr/klogr_test.go
@@ -116,6 +116,15 @@ func testOutput(t *testing.T, format string) {
 			expectedOutput: ` "msg"="test"  "akey"="avalue" "akey2"="(MISSING)"
 `,
 			expectedKlogOutput: `"test" akey="avalue" akey2="(MISSING)"
+`,
+		},
+		"should correctly handle odd-numbers of KVs in WithValue": {
+			klogr:         new().WithValues("keyWithoutValue"),
+			text:          "test",
+			keysAndValues: []interface{}{"akey", "avalue", "akey2"},
+			expectedOutput: ` "msg"="test" "keyWithoutValue"="(MISSING)" "akey"="avalue" "akey2"="(MISSING)"
+`,
+			expectedKlogOutput: `"test" keyWithoutValue="(MISSING)" akey="avalue" akey2="(MISSING)"
 `,
 		},
 		"should correctly html characters": {
diff --git a/test/output_test.go b/test/output_test.go
index ddb47044b..8dad50b6e 100644
--- a/test/output_test.go
+++ b/test/output_test.go
@@ -36,21 +36,9 @@ func TestKlogOutput(t *testing.T) {
 
 // TestKlogrOutput tests klogr output via klog.
 func TestKlogrOutput(t *testing.T) {
-	// klogr currently doesn't produce exactly the same output as klog.
-	// TODO: fix that.
-	mapping := map[string]string{
-		`I output.go:<LINE>] "test" keyWithoutValue="(MISSING)"
-I output.go:<LINE>] "test" keyWithoutValue="(MISSING)" anotherKeyWithoutValue="(MISSING)"
-I output.go:<LINE>] "test" keyWithoutValue="(MISSING)"
-`: `I output.go:<LINE>] "test" keyWithoutValue="(MISSING)"
-I output.go:<LINE>] "test" keyWithoutValue="anotherKeyWithoutValue"
-I output.go:<LINE>] "test" keyWithoutValue="(MISSING)"
-`,
-	}
 	Output(t, OutputConfig{
 		NewLogger: func(out io.Writer, v int, vmodule string) logr.Logger {
 			return klogr.NewWithOptions(klogr.WithFormat(klogr.FormatKlog))
 		},
-		ExpectedOutputMapping: mapping,
 	})
 }