firestore: recognizable error for no such column from DataAt/DataAtPath

[tv42]

I have a DocumentSnapshot and I'm interested in one of the fields in it, decoded into the Go type that I expect it to be. (Other field types should cause an error.)

If the field does not exist, I wish to abort processing early, but I want to detect that case specifically and not just handle it like transient errors. Specifically, I want to return nil.

If I call err := doc.DataTo(&structWithFooField), I have no way of knowing whether the field existed or not.

If I call foo, err := doc.DataAt("foo"), I have no way of knowing whether the field existed or not.

Describe the solution you'd like In a typical Go project, I'd say define a new error type or sentinel, type NoSuchFieldError struct { FieldName string } or var ErrNoSuchField = errors.New("..."), and return that something satistfying errors.As/errors.Is from https://github.com/googleapis/google-cloud-go/blob/7d5ccde710b3daed6a913d6b763304a536c4c087/firestore/document.go#L156

Since everything here wants to use GRPC status codes, NotFound seems appropriate https://godoc.org/google.golang.org/grpc/codes#NotFound except that's not necessarily good enough because it's already used for document does not exist: "If the document does not exist, DataAt returns a NotFound error." https://godoc.org/cloud.google.com/go/firestore#DocumentSnapshot.DataAt

I guess a way around that would be to satisfy codes.NotFound, but also something else. Like the above new error type or sentinel.

Describe alternatives you've considered

foo, ok := doc.Data()["foo"].(string)

I try to stay away from interface{} and type assertions, whenever possible. Nothing in the problem itself demands their use. Of course, DataAt still forces that on me, and a future feature like DataFieldTo(name string, dst interface{}) error would be even better -- but it would want the same error reporting capability that this issue is asking for.

[tritone]

@benwhitehead thoughts on this? Would be curious how other firestore clients handle this kind of issue, if it comes up.

Regardless, I think we would probably want to avoid forcing gRPC error codes when they are not actually returned from the service, but some kind of sentinel error potentially sounds reasonable. We haven't gotten around to adding Go 1.13 error wrapping to this library yet though.

[BenWhitehead]

In the case of the Java Firestore client the data mapping (toObject(class)) doesn't really support this type of scenario -- when attempting to map a doc into an object it iterates over the fields present in the document and tries to map them to the object and there isn't a way to change or interrupt this process. If not using toObject(Class) and instead reading the fields from the DocumentSnapshot there is a contains method which can be called to test whether a field exists before calling one of the get* methods.

[tbpg]

A sentinel error value seems reasonable to me here. I don't think we should "fake" a gRPC error code.

I don't see similar error values defined in firestore, pubsub, storage, or spanner (I may have missed them), but we do have some in datastore: https://pkg.go.dev/cloud.google.com/go/datastore#pkg-variables.