fix(firestore): Handle nil in String() (#11383)

googleapis · Jan 8, 2025 · d3f81aa · d3f81aa
1 parent a6eeac7
commit d3f81aa
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 20 deletions.
diff --git a/firestore/docref.go b/firestore/docref.go
@@ -511,9 +511,9 @@ type transform struct {
 
 func (t transform) String() string {
 	if t.t == nil {
-		return ""
+		return "{t:nil}"
 	}
-	return t.t.String()
+	return fmt.Sprintf("{t:%v}", t.t.String())
 }
 
 // FieldTransformIncrement returns a special value that can be used with Set, Create, or
@@ -675,9 +675,9 @@ type Update struct {
 func (u Update) String() string {
 	t, ok := u.Value.(transform)
 	if !ok {
-		return fmt.Sprintf("Path: %s FieldPath: %s Value: %s", u.Path, u.FieldPath, u.Value)
+		return fmt.Sprintf("{Path:%s FieldPath:%s Value:%s}", u.Path, u.FieldPath, u.Value)
 	}
-	return fmt.Sprintf("Path: %s FieldPath: %s Value: %s", u.Path, u.FieldPath, t.String())
+	return fmt.Sprintf("{Path:%s FieldPath:%s Value:%s}", u.Path, u.FieldPath, t.String())
 }
 
 // An fpv is a pair of validated FieldPath and value.

diff --git a/firestore/docref_test.go b/firestore/docref_test.go
@@ -319,49 +319,87 @@ func commitRequestForSet() *pb.CommitRequest {
 
 func TestUpdateProcess(t *testing.T) {
 	for _, test := range []struct {
+		desc    string
 		in      Update
 		want    fpv
 		wantErr bool
+		wantStr string
 	}{
 		{
-			in:   Update{Path: "a", Value: 1},
-			want: fpv{fieldPath: []string{"a"}, value: 1},
+			desc:    "Integer value",
+			in:      Update{Path: "a", Value: 1},
+			want:    fpv{fieldPath: []string{"a"}, value: 1},
+			wantStr: "{Path:a FieldPath:[] Value:%!s(int=1)}",
 		},
 		{
-			in:   Update{Path: "c.d", Value: Delete},
-			want: fpv{fieldPath: []string{"c", "d"}, value: Delete},
+			desc:    "Delete transform",
+			in:      Update{Path: "c.d", Value: Delete},
+			want:    fpv{fieldPath: []string{"c", "d"}, value: Delete},
+			wantStr: "{Path:c.d FieldPath:[] Value:Delete}",
 		},
 		{
-			in:   Update{FieldPath: []string{"*", "~"}, Value: ServerTimestamp},
-			want: fpv{fieldPath: []string{"*", "~"}, value: ServerTimestamp},
+			desc: "Increment transform",
+			in:   Update{Path: "c.d", Value: Increment(8)},
+			want: fpv{fieldPath: []string{"c", "d"}, value: transform{
+				t: &pb.DocumentTransform_FieldTransform{
+					TransformType: &pb.DocumentTransform_FieldTransform_Increment{
+						Increment: &pb.Value{
+							ValueType: &pb.Value_IntegerValue{IntegerValue: 8},
+						},
+					},
+				},
+				err: nil,
+			}},
+			wantStr: "{Path:c.d FieldPath:[] Value:{t:increment:{integer_value:8}}}",
+		},
+		{
+			desc:    "ServerTimestamp transform",
+			in:      Update{FieldPath: []string{"*", "~"}, Value: ServerTimestamp},
+			want:    fpv{fieldPath: []string{"*", "~"}, value: ServerTimestamp},
+			wantStr: "{Path: FieldPath:[* ~] Value:ServerTimestamp}",
 		},
 		{
+			desc:    "bad rune in path",
 			in:      Update{Path: "*"},
 			wantErr: true, // bad rune in path
+			wantStr: "{Path:* FieldPath:[] Value:%!s(<nil>)}",
 		},
 		{
+			desc:    "both Path and FieldPath",
 			in:      Update{Path: "a", FieldPath: []string{"b"}},
 			wantErr: true, // both Path and FieldPath
+			wantStr: "{Path:a FieldPath:[b] Value:%!s(<nil>)}",
 		},
 		{
+			desc:    "neither Path nor FieldPath",
 			in:      Update{Value: 1},
 			wantErr: true, // neither Path nor FieldPath
+			wantStr: "{Path: FieldPath:[] Value:%!s(int=1)}",
 		},
 		{
+			desc:    "empty FieldPath component",
 			in:      Update{FieldPath: []string{"", "a"}},
 			wantErr: true, // empty FieldPath component
+			wantStr: "{Path: FieldPath:[ a] Value:%!s(<nil>)}",
 		},
 	} {
-		got, err := test.in.process()
-		if test.wantErr {
-			if err == nil {
-				t.Errorf("%+v: got nil, want error", test.in)
+		t.Run(test.desc, func(t *testing.T) {
+			got, err := test.in.process()
+			if test.wantErr {
+				if err == nil {
+					t.Errorf("%+v: got nil, want error", test.in)
+				}
+			} else if err != nil {
+				t.Errorf("%+v: got error %v, want nil", test.in, err)
+			} else if !testEqual(got, test.want) {
+				t.Errorf("%+v: got %+v, want %+v", test.in, got, test.want)
 			}
-		} else if err != nil {
-			t.Errorf("%+v: got error %v, want nil", test.in, err)
-		} else if !testEqual(got, test.want) {
-			t.Errorf("%+v: got %+v, want %+v", test.in, got, test.want)
-		}
+
+			gotStr := test.in.String()
+			if gotStr != test.wantStr {
+				t.Errorf("%+v: got %q, want %q", test.in, gotStr, test.wantStr)
+			}
+		})
 	}
 }
 

diff --git a/firestore/util_test.go b/firestore/util_test.go
@@ -44,7 +44,7 @@ func mustTimestampProto(t time.Time) *timestamppb.Timestamp {
 
 var cmpOpts = []cmp.Option{
 	cmp.AllowUnexported(DocumentSnapshot{},
-		Query{}, OrFilter{}, AndFilter{}, PropertyPathFilter{}, PropertyFilter{}, order{}, fpv{}, DocumentRef{}, CollectionRef{}, Query{}),
+		Query{}, OrFilter{}, AndFilter{}, PropertyPathFilter{}, PropertyFilter{}, order{}, fpv{}, DocumentRef{}, CollectionRef{}, Query{}, transform{}),
 	cmpopts.IgnoreTypes(Client{}, &Client{}),
 	cmp.Comparer(func(*readSettings, *readSettings) bool {
 		return true // Don't try to compare two readSettings pointer types