firestore: better treatment of ServerTimestamp and Delete
- More checking for incorrect use of these sentinels.
- A small optimization: a map with only ServerTimestamps as values
turns into nil, instead of an empty map.
Change-Id: Ia79348d6637eac845e77acbb4a622539a9147f44
Reviewed-on: https://code-review.googlesource.com/19050
Reviewed-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Michael Darakananda <pongad@google.com>
diff --git a/firestore/docref.go b/firestore/docref.go
index ca88136..abbdda8 100644
--- a/firestore/docref.go
+++ b/firestore/docref.go
@@ -148,10 +148,10 @@
return nil, errNilDocRef
}
origFieldPaths, allPaths, err := processSetOptions(opts)
- isMerge := len(origFieldPaths) > 0 || allPaths // was some Merge option specified?
if err != nil {
return nil, err
}
+ isMerge := len(origFieldPaths) > 0 || allPaths // was some Merge option specified?
doc, serverTimestampPaths, err := toProtoDocument(data)
if err != nil {
return nil, err
@@ -214,7 +214,7 @@
// There were field paths, but they all got removed.
// The write does nothing but enforce the precondition.
w = &pb.Write{CurrentDocument: pc}
- case !isMerge:
+ case !isMerge && (pc != nil || doc.Fields != nil):
// Set without merge, so no update mask.
w = &pb.Write{
Operation: &pb.Write_Update{doc},
diff --git a/firestore/docref_test.go b/firestore/docref_test.go
index f77106a..4a8961d 100644
--- a/firestore/docref_test.go
+++ b/firestore/docref_test.go
@@ -163,13 +163,25 @@
transform: []string{"b"},
},
{
+ desc: "a ServerTimestamp alone",
+ data: map[string]interface{}{"b": ServerTimestamp},
+ write: nil,
+ transform: []string{"b"},
+ },
+ {
+ desc: "a ServerTimestamp alone with a path",
+ data: map[string]interface{}{"b": ServerTimestamp},
+ opt: MergePaths([]string{"b"}),
+ write: nil,
+ transform: []string{"b"},
+ },
+ {
desc: "nested ServerTimestamp field",
data: map[string]interface{}{
"a": 1,
"b": map[string]interface{}{"c": ServerTimestamp},
},
- // TODO(jba): make this be map[string]*pb.Value{"a": intval(1)},
- write: map[string]*pb.Value{"a": intval(1), "b": mapval(map[string]*pb.Value{})},
+ write: map[string]*pb.Value{"a": intval(1)},
transform: []string{"b.c"},
},
{
@@ -179,9 +191,7 @@
"b": ServerTimestamp,
"c": map[string]interface{}{"d": ServerTimestamp},
},
- // TODO(jba): make this be map[string]*pb.Value{"a": intval(1)},
- write: map[string]*pb.Value{"a": intval(1),
- "c": mapval(map[string]*pb.Value{})},
+ write: map[string]*pb.Value{"a": intval(1)},
transform: []string{"b", "c.d"},
},
{
diff --git a/firestore/document.go b/firestore/document.go
index 538ef56..7efc14b 100644
--- a/firestore/document.go
+++ b/firestore/document.go
@@ -156,19 +156,26 @@
return nil, nil, errors.New("firestore: nil document contents")
}
v := reflect.ValueOf(x)
- pv, err := toProtoValue(v)
+ pv, sawTransform, err := toProtoValue(v)
if err != nil {
return nil, nil, err
}
- fieldPaths, err := extractTransformPaths(v, nil)
- if err != nil {
- return nil, nil, err
+ var fieldPaths []FieldPath
+ if sawTransform {
+ fieldPaths, err = extractTransformPaths(v, nil)
+ if err != nil {
+ return nil, nil, err
+ }
}
- m := pv.GetMapValue()
- if m == nil {
- return nil, nil, fmt.Errorf("firestore: cannot covert value of type %T into a map", x)
+ var fields map[string]*pb.Value
+ if pv != nil {
+ m := pv.GetMapValue()
+ if m == nil {
+ return nil, nil, fmt.Errorf("firestore: cannot covert value of type %T into a map", x)
+ }
+ fields = m.Fields
}
- return &pb.Document{Fields: m.Fields}, fieldPaths, nil
+ return &pb.Document{Fields: fields}, fieldPaths, nil
}
func extractTransformPaths(v reflect.Value, prefix FieldPath) ([]FieldPath, error) {
diff --git a/firestore/integration_test.go b/firestore/integration_test.go
index e282a2f..54b6e20 100644
--- a/firestore/integration_test.go
+++ b/firestore/integration_test.go
@@ -217,6 +217,9 @@
checkTimeBetween(t, wr.UpdateTime, start, end)
_, err := doc.Create(ctx, integrationTestMap)
codeEq(t, "Create on a present doc", codes.AlreadyExists, err)
+ // OK to create an empty document.
+ _, err = integrationColl(t).NewDoc().Create(ctx, map[string]interface{}{})
+ codeEq(t, "Create empty doc", codes.OK, err)
}
func TestIntegration_Get(t *testing.T) {
@@ -234,8 +237,6 @@
if want := wantIntegrationTestMap; !testEqual(got, want) {
t.Errorf("got\n%v\nwant\n%v", pretty.Value(got), pretty.Value(want))
}
-
- //
_, err = integrationColl(t).NewDoc().Get(ctx)
codeEq(t, "Get on a missing doc", codes.NotFound, err)
}
@@ -425,6 +426,8 @@
er(doc.UpdateMap(ctx, um, LastUpdateTime(wr.UpdateTime.Add(-time.Millisecond)))))
codeEq(t, "UpdateMap with right LastUpdateTime", codes.OK,
er(doc.UpdateMap(ctx, um, LastUpdateTime(wr.UpdateTime))))
+ codeEq(t, "just server transform", codes.OK,
+ er(doc.UpdateMap(ctx, map[string]interface{}{"a": ServerTimestamp})))
}
func TestIntegration_UpdateStruct(t *testing.T) {
diff --git a/firestore/mock_test.go b/firestore/mock_test.go
index 047d345..d0cdba9 100644
--- a/firestore/mock_test.go
+++ b/firestore/mock_test.go
@@ -83,8 +83,9 @@
ri.adjust(gotReq)
}
if !proto.Equal(gotReq, ri.wantReq) {
- return nil, fmt.Errorf("mockServer: bad request\ngot: %T\n%+v\nwant: %T\n%+v",
- gotReq, gotReq, ri.wantReq, ri.wantReq)
+ return nil, fmt.Errorf("mockServer: bad request\ngot: %T\n%s\nwant: %T\n%s",
+ gotReq, proto.MarshalTextString(gotReq),
+ ri.wantReq, proto.MarshalTextString(ri.wantReq))
}
}
resp := s.resps[0]
diff --git a/firestore/query.go b/firestore/query.go
index 13b5cfb..0574504 100644
--- a/firestore/query.go
+++ b/firestore/query.go
@@ -277,10 +277,14 @@
}
vals[i] = &pb.Value{&pb.Value_ReferenceValue{q.parentPath + "/documents/" + q.collectionID + "/" + docID}}
} else {
- vals[i], err = toProtoValue(reflect.ValueOf(fval))
+ var sawTransform bool
+ vals[i], sawTransform, err = toProtoValue(reflect.ValueOf(fval))
if err != nil {
return nil, err
}
+ if sawTransform {
+ return nil, errors.New("firestore: ServerTimestamp disallowed in query value")
+ }
}
}
return vals, nil
@@ -311,10 +315,13 @@
default:
return nil, fmt.Errorf("firestore: invalid operator %q", f.op)
}
- val, err := toProtoValue(reflect.ValueOf(f.value))
+ val, sawTransform, err := toProtoValue(reflect.ValueOf(f.value))
if err != nil {
return nil, err
}
+ if sawTransform {
+ return nil, errors.New("firestore: ServerTimestamp disallowed in query value")
+ }
return &pb.StructuredQuery_Filter{
FilterType: &pb.StructuredQuery_Filter_FieldFilter{
&pb.StructuredQuery_FieldFilter{
diff --git a/firestore/query_test.go b/firestore/query_test.go
index 20f901f..0fea114 100644
--- a/firestore/query_test.go
+++ b/firestore/query_test.go
@@ -228,6 +228,8 @@
}
func TestQueryToProtoErrors(t *testing.T) {
+ st := map[string]interface{}{"a": ServerTimestamp}
+ del := map[string]interface{}{"a": Delete}
q := (&Client{}).Collection("C").Query
for _, query := range []Query{
Query{}, // no collection ID
@@ -240,6 +242,12 @@
q.SelectPaths([]string{"/", "", "~"}), // invalid path
q.OrderBy("[", Asc), // invalid path
q.OrderByPath([]string{""}, Desc), // invalid path
+ q.Where("x", "==", st), // ServerTimestamp in filter
+ q.OrderBy("a", Asc).StartAt(st), // ServerTimestamp in Start
+ q.OrderBy("a", Asc).EndAt(st), // ServerTimestamp in End
+ q.Where("x", "==", del), // Delete in filter
+ q.OrderBy("a", Asc).StartAt(del), // Delete in Start
+ q.OrderBy("a", Asc).EndAt(del), // Delete in End
} {
_, err := query.toProto()
if err == nil {
diff --git a/firestore/to_value.go b/firestore/to_value.go
index ea106fc..c4a6820 100644
--- a/firestore/to_value.go
+++ b/firestore/to_value.go
@@ -41,40 +41,44 @@
// toProtoValue converts a Go value to a Firestore Value protobuf.
// Some corner cases:
// - All nils (nil interface, nil slice, nil map, nil pointer) are converted to
-// a NullValue (not a nil *pb.Value). toProtoValue never returns (nil, nil).
+// a NullValue (not a nil *pb.Value). toProtoValue never returns (nil, false, nil).
+// It returns (nil, true, nil) if everything in the value is ServerTimestamp.
// - An error is returned for uintptr, uint and uint64, because Firestore uses
// an int64 to represent integral values, and those types can't be properly
// represented in an int64.
// - An error is returned for the special Delete value.
-func toProtoValue(v reflect.Value) (*pb.Value, error) {
+func toProtoValue(v reflect.Value) (pbv *pb.Value, sawServerTimestamp bool, err error) {
if !v.IsValid() {
- return nullValue, nil
+ return nullValue, false, nil
}
vi := v.Interface()
if vi == Delete {
- return nil, errors.New("firestore: cannot use Delete in value")
+ return nil, false, errors.New("firestore: cannot use Delete in value")
+ }
+ if vi == ServerTimestamp {
+ return nil, false, errors.New("firestore: must use ServerTimestamp as a map value")
}
switch x := vi.(type) {
case []byte:
- return &pb.Value{&pb.Value_BytesValue{x}}, nil
+ return &pb.Value{&pb.Value_BytesValue{x}}, false, nil
case time.Time:
ts, err := ptypes.TimestampProto(x)
if err != nil {
- return nil, err
+ return nil, false, err
}
- return &pb.Value{&pb.Value_TimestampValue{ts}}, nil
+ return &pb.Value{&pb.Value_TimestampValue{ts}}, false, nil
case *latlng.LatLng:
if x == nil {
// gRPC doesn't like nil oneofs. Use NullValue.
- return nullValue, nil
+ return nullValue, false, nil
}
- return &pb.Value{&pb.Value_GeoPointValue{x}}, nil
+ return &pb.Value{&pb.Value_GeoPointValue{x}}, false, nil
case *DocumentRef:
if x == nil {
// gRPC doesn't like nil oneofs. Use NullValue.
- return nullValue, nil
+ return nullValue, false, nil
}
- return &pb.Value{&pb.Value_ReferenceValue{x.Path}}, nil
+ return &pb.Value{&pb.Value_ReferenceValue{x.Path}}, false, nil
// Do not add bool, string, int, etc. to this switch; leave them in the
// reflect-based switch below. Moving them here would drop support for
// types whose underlying types are those primitives.
@@ -83,15 +87,15 @@
}
switch v.Kind() {
case reflect.Bool:
- return &pb.Value{&pb.Value_BooleanValue{v.Bool()}}, nil
+ return &pb.Value{&pb.Value_BooleanValue{v.Bool()}}, false, nil
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
- return &pb.Value{&pb.Value_IntegerValue{v.Int()}}, nil
+ return &pb.Value{&pb.Value_IntegerValue{v.Int()}}, false, nil
case reflect.Uint8, reflect.Uint16, reflect.Uint32:
- return &pb.Value{&pb.Value_IntegerValue{int64(v.Uint())}}, nil
+ return &pb.Value{&pb.Value_IntegerValue{int64(v.Uint())}}, false, nil
case reflect.Float32, reflect.Float64:
- return &pb.Value{&pb.Value_DoubleValue{v.Float()}}, nil
+ return &pb.Value{&pb.Value_DoubleValue{v.Float()}}, false, nil
case reflect.String:
- return &pb.Value{&pb.Value_StringValue{v.String()}}, nil
+ return &pb.Value{&pb.Value_StringValue{v.String()}}, false, nil
case reflect.Slice:
return sliceToProtoValue(v)
case reflect.Map:
@@ -100,7 +104,7 @@
return structToProtoValue(v)
case reflect.Ptr:
if v.IsNil() {
- return nullValue, nil
+ return nullValue, false, nil
}
return toProtoValue(v.Elem())
case reflect.Interface:
@@ -110,71 +114,105 @@
fallthrough // any other interface value is an error
default:
- return nil, fmt.Errorf("firestore: cannot convert type %s to value", v.Type())
+ return nil, false, fmt.Errorf("firestore: cannot convert type %s to value", v.Type())
}
}
-func sliceToProtoValue(v reflect.Value) (*pb.Value, error) {
+func sliceToProtoValue(v reflect.Value) (*pb.Value, bool, error) {
// A nil slice is converted to a null value.
if v.IsNil() {
- return nullValue, nil
+ return nullValue, false, nil
}
vals := make([]*pb.Value, v.Len())
for i := 0; i < v.Len(); i++ {
- val, err := toProtoValue(v.Index(i))
+ val, sawServerTimestamp, err := toProtoValue(v.Index(i))
if err != nil {
- return nil, err
+ return nil, false, err
+ }
+ if sawServerTimestamp {
+ return nil, false, errors.New("firestore: ServerTimestamp cannot occur in an array")
}
vals[i] = val
}
- return &pb.Value{&pb.Value_ArrayValue{&pb.ArrayValue{vals}}}, nil
+ return &pb.Value{&pb.Value_ArrayValue{&pb.ArrayValue{vals}}}, false, nil
}
-func mapToProtoValue(v reflect.Value) (*pb.Value, error) {
+func mapToProtoValue(v reflect.Value) (*pb.Value, bool, error) {
if v.Type().Key().Kind() != reflect.String {
- return nil, errors.New("firestore: map key type must be string")
+ return nil, false, errors.New("firestore: map key type must be string")
}
// A nil map is converted to a null value.
if v.IsNil() {
- return nullValue, nil
+ return nullValue, false, nil
}
m := map[string]*pb.Value{}
+ sawServerTimestamp := false
for _, k := range v.MapKeys() {
mi := v.MapIndex(k)
if mi.Interface() == ServerTimestamp {
+ sawServerTimestamp = true
continue
}
- val, err := toProtoValue(mi)
+ val, sst, err := toProtoValue(mi)
if err != nil {
- return nil, err
+ return nil, false, err
+ }
+ if sst {
+ sawServerTimestamp = true
+ }
+ if val == nil { // value was a map with all ServerTimestamp values
+ continue
}
m[k.String()] = val
}
- return &pb.Value{&pb.Value_MapValue{&pb.MapValue{m}}}, nil
+ var pv *pb.Value
+ if len(m) == 0 && sawServerTimestamp {
+ // The entire map consisted of ServerTimestamp values.
+ pv = nil
+ } else {
+ pv = &pb.Value{&pb.Value_MapValue{&pb.MapValue{m}}}
+ }
+ return pv, sawServerTimestamp, nil
}
-func structToProtoValue(v reflect.Value) (*pb.Value, error) {
+func structToProtoValue(v reflect.Value) (*pb.Value, bool, error) {
m := map[string]*pb.Value{}
fields, err := fieldCache.Fields(v.Type())
if err != nil {
- return nil, err
+ return nil, false, err
}
+ sawServerTimestamp := false
for _, f := range fields {
fv := v.FieldByIndex(f.Index)
opts := f.ParsedTag.(tagOptions)
if opts.serverTimestamp {
+ // TODO(jba): should we return a non-zero time?
+ sawServerTimestamp = true
continue
}
if opts.omitEmpty && isEmptyValue(fv) {
continue
}
- val, err := toProtoValue(fv)
+ val, sst, err := toProtoValue(fv)
if err != nil {
- return nil, err
+ return nil, false, err
+ }
+ if sst {
+ sawServerTimestamp = true
+ }
+ if val == nil { // value was a map with all ServerTimestamp values
+ continue
}
m[f.Name] = val
}
- return &pb.Value{&pb.Value_MapValue{&pb.MapValue{m}}}, nil
+ var pv *pb.Value
+ if len(m) == 0 && sawServerTimestamp {
+ // The entire struct consisted of ServerTimestamp or omitempty values.
+ pv = nil
+ } else {
+ pv = &pb.Value{&pb.Value_MapValue{&pb.MapValue{m}}}
+ }
+ return pv, sawServerTimestamp, nil
}
type tagOptions struct {
diff --git a/firestore/to_value_test.go b/firestore/to_value_test.go
index 13ddd2c..ae58a13 100644
--- a/firestore/to_value_test.go
+++ b/firestore/to_value_test.go
@@ -117,8 +117,47 @@
},
refval("projects/P/databases/D/documents/c/d"),
},
+ // ServerTimestamps are removed, possibly leaving nil.
+ {map[string]interface{}{"a": ServerTimestamp}, nil},
+ {
+ map[string]interface{}{
+ "a": map[string]interface{}{
+ "b": map[string]interface{}{
+ "c": ServerTimestamp,
+ },
+ },
+ },
+ nil,
+ },
+ {
+ map[string]interface{}{
+ "a": map[string]interface{}{
+ "b": map[string]interface{}{
+ "c": ServerTimestamp,
+ "d": ServerTimestamp,
+ },
+ },
+ },
+ nil,
+ },
+ {
+ map[string]interface{}{
+ "a": map[string]interface{}{
+ "b": map[string]interface{}{
+ "c": ServerTimestamp,
+ "d": ServerTimestamp,
+ "e": 1,
+ },
+ },
+ },
+ mapval(map[string]*pb.Value{
+ "a": mapval(map[string]*pb.Value{
+ "b": mapval(map[string]*pb.Value{"e": intval(1)}),
+ }),
+ }),
+ },
} {
- got, err := toProtoValue(reflect.ValueOf(test.in))
+ got, _, err := toProtoValue(reflect.ValueOf(test.in))
if err != nil {
t.Errorf("%v (%T): %v", test.in, test.in, err)
continue
@@ -139,8 +178,18 @@
map[int]bool{}, // map key type is not string
make(chan int), // can't handle type
map[string]fmt.Stringer{"a": stringy{}}, // only empty interfaces
+ ServerTimestamp, // ServerTimestamp can only be a field value
+ []interface{}{ServerTimestamp},
+ map[string]interface{}{"a": []interface{}{ServerTimestamp}},
+ map[string]interface{}{"a": []interface{}{
+ map[string]interface{}{"b": ServerTimestamp},
+ }},
+ Delete, // Delete should never appear
+ []interface{}{Delete},
+ map[string]interface{}{"a": Delete},
+ map[string]interface{}{"a": []interface{}{Delete}},
} {
- _, err := toProtoValue(reflect.ValueOf(in))
+ _, _, err := toProtoValue(reflect.ValueOf(in))
if err == nil {
t.Errorf("%v: got nil, want error", in)
}
@@ -161,7 +210,7 @@
OmitEmpty: 3,
OmitEmptyTime: aTime,
}
- got, err := toProtoValue(reflect.ValueOf(in))
+ got, _, err := toProtoValue(reflect.ValueOf(in))
if err != nil {
t.Fatal(err)
}
@@ -174,7 +223,7 @@
t.Errorf("got %+v, want %+v", got, want)
}
- got, err = toProtoValue(reflect.ValueOf(testStruct2{}))
+ got, _, err = toProtoValue(reflect.ValueOf(testStruct2{}))
if err != nil {
t.Fatal(err)
}
@@ -191,7 +240,7 @@
*latlng.LatLng
}
- got, err := toProtoValue(reflect.ValueOf(embed{tm, ll}))
+ got, _, err := toProtoValue(reflect.ValueOf(embed{tm, ll}))
if err != nil {
t.Fatal(err)
}
