From dc94db6b3d40b1f7d118b44f5f47fc1aec9cd760 Mon Sep 17 00:00:00 2001 From: Evan Jones Date: Thu, 29 Jun 2023 12:01:57 -0400 Subject: [PATCH] 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. --- pgtype/hstore.go | 13 ++++++-- pgtype/hstore_test.go | 69 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 67 insertions(+), 15 deletions(-) diff --git a/pgtype/hstore.go b/pgtype/hstore.go index 9befabd05..2f34f4c9e 100644 --- a/pgtype/hstore.go +++ b/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 diff --git a/pgtype/hstore_test.go b/pgtype/hstore_test.go index 34bc454ce..ddc3de577 100644 --- a/pgtype/hstore_test.go +++ b/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")}