firestore: Set method zeros out serverTimestamp fields unexpectedly

[mickeyreiss]

I'm running into an issue with the go firestore client, where Set is not doing what I'd expect.

When we call set with a struct that has a serverTimestamp field, that field is zeroed out, rather than ignored. I'd expect that field to update to the value I set, or if I set the value to zero, for that field to be server updated.

(It looks like the issue lies in the docref.go code which adds a write operation without checking if the current value of the serverTimestamp field is time zero.)

Here's a test to demonstrate:

package main

import (
	"testing"
	"context"
	"time"
	"cloud.google.com/go/firestore"
)

type Thing struct {
	ServerTime time.Time `firestore:"updatedAt,serverTimestamp"`
}

func Test(t *testing.T) {
	var firestoreClient *firestore.Client = m.Client // TODO: Set up your firestore client here!

	thing := &Thing{}

	ref, writeResult, err := firestoreClient.Collection("testing").Add(context.TODO(), thing)
	if err != nil {
		t.Fatal(err)
	}
	if writeResult == nil {
		t.Fatal()
	}

	readResult, err := ref.Get(context.TODO())
	if err != nil {
		t.Fatal(err)
	}
	var readThing Thing
	if err := readResult.DataTo(&readThing); err != nil {
		t.Fatal(err)
	}

	if readThing.ServerTime.IsZero() {
		t.Fatal("updated at wasn't automatically set")
	}

	newTime := time.Date(2020, 0, 0, 0, 0, 0, 0, time.UTC)

	updateThing := readThing
	updateThing.ServerTime = newTime
	_, err = ref.Set(context.TODO(), &updateThing)
	if err != nil {
		t.Fatal(err)
	}

	resultReadAfterUpdate, err := ref.Get(context.TODO())
	if err != nil {
		t.Fatal(err)
	}
	var thingReadAfterUpdate Thing
	if err := resultReadAfterUpdate.DataTo(&thingReadAfterUpdate); err != nil {
		t.Fatal(err)
	}
	if thingReadAfterUpdate.ServerTime != newTime {
		t.Errorf("server time was updated to incorrect value: expected %v, got %v", newTime, thingReadAfterUpdate.ServerTime)
	}
	if thingReadAfterUpdate.ServerTime.IsZero() {
		t.Fatal("weird, updated at was zeroed out", thingReadAfterUpdate)
	}
}

Output on my end:

--- FAIL: Test (1.51s)
	firestore_test.go:66: server time was updated to incorrect value: expected 2019-11-30 00:00:00 +0000 UTC, got 0001-01-01 00:00:00 +0000 UTC
	firestore_test.go:69: weird, updated at was zeroed out {0001-01-01 00:00:00 +0000 UTC}
F

[jeanbza]

I think this is WAI. See https://godoc.org/cloud.google.com/go/firestore:

- serverTimestamp: The field must be of type time.Time. When writing, if
  the field has the zero value, the server will populate the stored document with
  the time that the request is processed.

Note that ServerTimestamp is not a field that you should set. It is only intended to be used as a sentinel value.

When it is set, though, its behavior seems to be to unset the value in the backend. I'm not entirely sure if this is WAI as a way to unset the field (since the zero value sets it)... @jba do you know?

[mickeyreiss]

@jadekler I don't think firestore should unset the value in this case. The documentation says if the field has the zero value, so I'd assume if it is non-zero, no special behavior occurs.

My understanding of firestore's lower level API, has no concept of a "server timestamp field". Rather, server timestamp is a type of transform that can be applied to any timestamp field as part of the write transaction.

My theory is that this behavior is due to a possible bug in newSetWrites, where non-zero serverTimestamp fields are (correctly) omitted from the set of serverTimestampPaths but also (incorrectly) included in the set of paths to be written. Does that seem possible?

Note that ServerTimestamp is not a field that you should set. It is only intended to be used as a sentinel value.

I'm not sure what you mean by this. I'm not referencing ServerTimestamp directly in my test above.

[jeanbza]

Note that ServerTimestamp is not a field that you should set. It is only intended to be used as a sentinel value.

I'm not sure what you mean by this. I'm not referencing ServerTimestamp directly in my test above.

Sorry, I wasn't very clear. Above I had meant a field that has the ServerTimestamp declaration in the struct tags. So, in your example, updateThing.ServerTime = newTime seems like something that is unsupported - I expect that field to only to be set by the server, never the user. That said, I'll wait for @jba's word on the intention behind ServerTimestamp in struct tags.

[jba]

tl;dr This is working as intended. Admittedly, the behavior is subtle, and I think we should add some documentation. Let's go through your code piece by piece.

ref, ... := firestoreClient.Collection("testing").Add(context.TODO(), thing)
...
readResult, err := ref.Get(context.TODO())
...var readThing Thing
...readResult.DataTo(&readThing)

This behaves as you'd expect. You created a document with one field, which holds the time that you wrote the document.

Now it's important to keep in mind that Firestore documents are JSON-like sets of key-value pairs. The Go client maps between the actual documents and Go structs as a convenience.

Also, as the doc that @jadekler quoted above says, a struct field marked with serverTimestamp is used when writing only if zero.

Proceeding with your code:

newTime := time.Date(2020, 0, 0, 0, 0, 0, 0, time.UTC)
updateThing := readThing
updateThing.ServerTime = newTime
...ref.Set(context.TODO(), &updateThing)

Since you didn't specify a merge option to Set, it will completely overwrite the existing object with updateThing. Let's translate updateThing to a JSON-like document:

1. The ServerTime field is non-zero, so we ignore it on a write.
2. There are no other fields.

So the document you are writing is the empty document, {}. After you get the document again:

resultReadAfterUpdate, err := ref.Get(context.TODO())

you can observe that's it's empty by printing resultReadAfterUpdate.Data(). You'll see it's the empty map.

Now you do

var thingReadAfterUpdate Thing
... resultReadAfterUpdate.DataTo(&thingReadAfterUpdate)

Since you are populating a struct with an empty document, nothing in the struct gets changed. Here I think we should improve our documentation to make this clear. I've created https://code-review.googlesource.com/c/gocloud/+/34170. Let me know if that would have helped.

[mickeyreiss]

Thanks for the investigation and clarification, @jba. The documentation update in 34170 LGTM.

We are finding this behavior a bit unintuitive, so we are going to stop using serverTimestamp in our go structs for now. Instead, we'll manage the timestamps manually in our code.

For context, we are writing to firestore documents that are read by Pring in iOS. This library makes the strong assumption that createdAt and updatedAt are present and not null.

[CaptainCodeman]

I'm struggling to understand how this can reasonably be labelled as working as intended - this is a HUGE gotcha that needs to be in big red letters.

Given this type:

Test struct {
	Name    string    `firestore:"name"`
	Created time.Time `firestore:"created,serverTimestamp"`
}

If I write the data:

ref := client.Doc("test/thing")
data := Test{
	Name: "Thing",
}
ref.Set(ctx, data)

... the created timestamp is set as expected:

If I load the data, it's there and usable. But if I update and resave the struct:

doc, err := ref.Get(ctx)
if err != nil {
  t.Fatal(err)
}

if err := doc.DataTo(&data); err != nil {
  t.Fatal(err)
}

data.Name = "Updated Thing"

if _, err := ref.Set(ctx, data); err != nil {
  t.Fatal(err)
}

... then boom - the created field is deleted

I am really struggling to understand how that can be considered expected and intentional, to me it's the worst database behavior I can imagine.

I can get what I would consider correct / expected behavior by listing the field in the Merge option:

if _, err := ref.Set(ctx, data, firestore.Merge([]string{"created"})); err != nil {
	t.Fatal(err)
}

But couldn't the package do that based on the serverTimestamp tag and non-empty value? Wouldn't that behavior be more likely what people want and expect to happen? I can't think of a scenario where me telling a database to store a struct means "by 'save this' I mean silently delete that field that I'm giving you a value for". I prevents it working for any initial write (no existing data), so I effectively have to write my own serverTimestamp code which kind of defeats the purpose of the tag.

[jeanbza]

@CaptainCodeman It plainly states in the Set documentation that it overwrites the document. If you have a suggestion for how we can make it more clear, please file a new issue as this one is quite old.