pgtype.Hstore: add a round-trip test for binary and text codecs

This ensures the output of Encode can pass through Scan and produce
the same input. This found two two minor problems with the text
codec. These are not bugs: These situations do not happen when using
pgx with Postgres. However, I think it is worth fixing to ensure the
code is internally consistent.

The problems with the text codec are:

* It did not correctly distinguish between nil and empty. This is not
  a problem with Postgres, since NULL values are marked separately,
  but the binary codec distinguishes between them, so it seems like
  the text codec should as well.
* It did not output spaces between keys. Postgres produces output in
  this format, and the parser now only strictly parses the Postgres
  format. This is not a bug, but seems like a good idea.

Author: evanj

pgtype/hstore.go

@@ -121,8 +121,15 @@ func (encodePlanHstoreCodecText) Encode(value any, buf []byte) (newBuf []byte, e
 		return nil, err
 	}
 
-	if hstore == nil {
-		return nil, nil
+	if len(hstore) == 0 {
+		// distinguish between empty and nil: Not strictly required by Postgres, since its protocol
+		// explicitly marks NULL column values separately. However, the Binary codec does this, and
+		// this means we can "round trip" Encode and Scan without data loss.
+		// nil: []byte(nil); empty: []byte{}
+		if hstore == nil {
+			return nil, nil
+		}
+		return []byte{}, nil
 	}
 
 	firstPair := true
@@ -131,7 +138,7 @@ func (encodePlanHstoreCodecText) Encode(value any, buf []byte) (newBuf []byte, e
 		if firstPair {
 			firstPair = false
 		} else {
-			buf = append(buf, ',')
+			buf = append(buf, ',', ' ')
 		}
 
 		// unconditionally quote hstore keys/values like Postgres does

pgtype/hstore_test.go

@@ -2,6 +2,7 @@ package pgtype_test
 
 import (
 	"context"
+	"fmt"
 	"reflect"
 	"testing"
 	"time"
@@ -53,6 +54,11 @@ func isExpectedEqMapStringPointerString(a any) func(any) bool {
 	}
 }
 
+// stringPtr returns a pointer to s.
+func stringPtr(s string) *string {
+	return &s
+}
+
 func TestHstoreCodec(t *testing.T) {
 	ctr := defaultConnTestRunner
 	ctr.AfterConnect = func(ctx context.Context, t testing.TB, conn *pgx.Conn) {
@@ -65,10 +71,6 @@ func TestHstoreCodec(t *testing.T) {
 		conn.TypeMap().RegisterType(&pgtype.Type{Name: "hstore", OID: hstoreOID, Codec: pgtype.HstoreCodec{}})
 	}
 
-	fs := func(s string) *string {
-		return &s
-	}
-
 	tests := []pgxtest.ValueRoundTripTest{
 		{
 			map[string]string{},
@@ -101,14 +103,14 @@ func TestHstoreCodec(t *testing.T) {
 			isExpectedEqMapStringPointerString(map[string]*string{}),
 		},
 		{
-			map[string]*string{"foo": fs("bar"), "baq": fs("quz")},
+			map[string]*string{"foo": stringPtr("bar"), "baq": stringPtr("quz")},
 			new(map[string]*string),
-			isExpectedEqMapStringPointerString(map[string]*string{"foo": fs("bar"), "baq": fs("quz")}),
+			isExpectedEqMapStringPointerString(map[string]*string{"foo": stringPtr("bar"), "baq": stringPtr("quz")}),
 		},
 		{
-			map[string]*string{"foo": nil, "baq": fs("quz")},
+			map[string]*string{"foo": nil, "baq": stringPtr("quz")},
 			new(map[string]*string),
-			isExpectedEqMapStringPointerString(map[string]*string{"foo": nil, "baq": fs("quz")}),
+			isExpectedEqMapStringPointerString(map[string]*string{"foo": nil, "baq": stringPtr("quz")}),
 		},
 		{nil, new(*map[string]string), isExpectedEq((*map[string]string)(nil))},
 		{nil, new(*map[string]*string), isExpectedEq((*map[string]*string)(nil))},
@@ -201,7 +203,7 @@ func TestHstoreCodec(t *testing.T) {
 			if typedParam != nil {
 				h = pgtype.Hstore{}
 				for k, v := range typedParam {
-					h[k] = fs(v)
+					h[k] = stringPtr(v)
 				}
 			}
 		}
@@ -261,10 +263,53 @@ func TestParseInvalidInputs(t *testing.T) {
 	}
 }
 
-func BenchmarkHstoreEncode(b *testing.B) {
-	stringPtr := func(s string) *string {
-		return &s
+func TestRoundTrip(t *testing.T) {
+	codecs := []struct {
+		name       string
+		encodePlan pgtype.EncodePlan
+		scanPlan   pgtype.ScanPlan
+	}{
+		{
+			"text",
+			pgtype.HstoreCodec{}.PlanEncode(nil, 0, pgtype.TextFormatCode, pgtype.Hstore(nil)),
+			pgtype.HstoreCodec{}.PlanScan(nil, 0, pgtype.TextFormatCode, (*pgtype.Hstore)(nil)),
+		},
+		{
+			"binary",
+			pgtype.HstoreCodec{}.PlanEncode(nil, 0, pgtype.BinaryFormatCode, pgtype.Hstore(nil)),
+			pgtype.HstoreCodec{}.PlanScan(nil, 0, pgtype.BinaryFormatCode, (*pgtype.Hstore)(nil)),
+		},
 	}
+
+	inputs := []pgtype.Hstore{
+		nil,
+		{},
+		{"": stringPtr("")},
+		{"k1": stringPtr("v1")},
+		{"k1": stringPtr("v1"), "k2": stringPtr("v2")},
+	}
+	for _, codec := range codecs {
+		for i, input := range inputs {
+			t.Run(fmt.Sprintf("%s/%d", codec.name, i), func(t *testing.T) {
+				serialized, err := codec.encodePlan.Encode(input, nil)
+				if err != nil {
+					t.Fatal(err)
+				}
+				var output pgtype.Hstore
+				err = codec.scanPlan.Scan(serialized, &output)
+				if err != nil {
+					t.Fatal(err)
+				}
+				if !reflect.DeepEqual(output, input) {
+					t.Errorf("output=%#v does not match input=%#v", output, input)
+				}
+			})
+		}
+	}
+
+}
+
+func BenchmarkHstoreEncode(b *testing.B) {
 	h := pgtype.Hstore{"a x": stringPtr("100"), "b": stringPtr("200"), "c": stringPtr("300"),
 		"d": stringPtr("400"), "e": stringPtr("500")}