Google-cloud-go: firestore client.GetAll panics when []*DocumentRef includes references to same document

[Morrowless]

Client

cloud.google.com/go v0.38.0

Describe Your Environment

GAE runtime go112 standard

Problem

This is a reproducible problem.
client.GetAll(ctx, userRefs)returns []*DocumentSnapshot without error, but accessing said docs causes a runtime panic. This only occurs when refs contain refs to identical docs. If all refs point to different docs, no issue.

Expected Behavior

Expect to get valid Docs from GetAll()

Actual Behavior

Accessing element of returned doc snapshots causes runtime error:

http: panic serving 127.0.0.1:38365: runtime error: invalid memory address or nil pointer dereference goroutine 172 [running]: net/http.(conn).serve.func1(0xc0003a5680) /usr/local/go/src/net/http/server.go:1769 +0x139 panic(0xab8200, 0x117c7e0) /usr/local/go/src/runtime/panic.go:522 +0x1b5 cloud.google.com/go/firestore.(DocumentSnapshot).Exists(...) /go/pkg/mod/cloud.google.com/[email protected]/firestore/document.go:58

[jadekler]

I'm not sure of your definition of "accessing": would you mind pasting a complete code sample that reproduces this issue?

[jadekler]

cc @benwhitehead

[Morrowless]

Sure.

func Foo(w http.ResponseWriter, r *http.Request) {
    ctx := context.Background()
    client, err := firestore.NewClient(ctx, global.GCPProjectID())
    if err != nil {
        log.Fatalf("Failed to create client: %v", err)
    }
    defer client.Close()
    userRef1 := client.Collection("Users").Doc("XYD7HZ2EN5VoSqYObCvtDp9pYxh1")
    userRef2 := client.Collection("Users").Doc("rUJYulWL45Q83X9lABVMhKGnSSF2")
    userRefs := []*firestore.DocumentRef{userRef1, userRef2}
    docs, err := client.GetAll(ctx, userRefs)
    if err != nil {
        log.Printf("error: %v", err)
    }
    for _, doc := range docs {
        var user user.User
        err := doc.DataTo(&user)
        if err != nil {
            log.Printf("error: %v", err)
        }
        log.Printf("user id: %s", user.ID)
    }
}

The above prints:

user id: XYD7HZ2EN5VoSqYObCvtDp9pYxh1
user id: rUJYulWL45Q83X9lABVMhKGnSSF2

But if both doc ids are the same:

userRef1 := client.Collection("Users").Doc("rUJYulWL45Q83X9lABVMhKGnSSF2")
userRef2 := client.Collection("Users").Doc("rUJYulWL45Q83X9lABVMhKGnSSF2")

I get a runtime error:

2019/05/29 00:23:54 http: panic serving 127.0.0.1:36847: runtime error: invalid memory address or nil pointer dereference goroutine 8 [running]: net/http.(conn).serve.func1(0xc00029ca00) /usr/local/go/src/net/http/server.go:1769 +0x139 panic(0xabd540, 0x11857e0) /usr/local/go/src/runtime/panic.go:522 +0x1b5 cloud.google.com/go/firestore.(DocumentSnapshot).Exists(...) /go/pkg/mod/cloud.google.com/[email protected]/firestore/document.go:58 cloud.google.com/go/firestore.(DocumentSnapshot).DataTo(0x0, 0xa50be0, 0xc00024a000, 0xc0000d5b58, 0x2) /go/pkg/mod/cloud.google.com/[email protected]/firestore/document.go:112 +0x27 platypus/handler.Foo(0xc9e2c0, 0xc0002b6000, 0xc0002b4200) /tmp/staging088131450/srv/handler/test.go:38 +0x373 net/http.HandlerFunc.ServeHTTP(0xbdb138, 0xc9e2c0, 0xc0002b6000, 0xc0002b4200) /usr/local/go/src/net/http/server.go:1995 +0x44 github.com/go-chi/chi.(Mux).routeHTTP(0xc00008ab40, 0xc9e2c0, 0xc0002b6000, 0xc0002b4200) /go/pkg/mod/github.com/go-chi/[email protected]+incompatible/mux.go:425 +0x27f net/http.HandlerFunc.ServeHTTP(0xc000248610, 0xc9e2c0, 0xc0002b6000, 0xc0002b4200) /usr/local/go/src/net/http/server.go:1995 +0x44 github.com/go-chi/chi.(Mux).ServeHTTP(0xc00008ab40, 0xc9e2c0, 0xc0002b6000, 0xc0002b4100) /go/pkg/mod/github.com/go-chi/[email protected]+incompatible/mux.go:82 +0x294 net/http.serverHandler.ServeHTTP(0xc0000b35f0, 0xc9e2c0, 0xc0002b6000, 0xc0002b4100) /usr/local/go/src/net/http/server.go:2774 +0xa8 net/http.(conn).serve(0xc00029ca00, 0xc9f940, 0xc0002a0400) /usr/local/go/src/net/http/server.go:1878 +0x851 created by net/http.(*Server).Serve /usr/local/go/src/net/http/server.go:2884 +0x2f4

[jadekler]

The GetAll documentation states:

GetAll retrieves multiple documents with a single call. The DocumentSnapshots are returned in the order of the given DocumentRefs.

What seems to be happening is that when we perform BatchGetDocuments with two of the same document, we only get one response. Since GetAll promises to put results in the order of the given DocumentRefs, that means either the first or second result will be nil (and the other result will be the single returned document from BatchGetDocuments).

It's not clear what we should do here. I could see:

• BatchGetDocuments returning duplicates. (service backend change)
• GetAll returning error when duplicates are specified.
• GetAll putting the same document in all the places it's used.

@benwhitehead @schmidt-sebastian @wilhuff @crwilcox any preferences here?

In the meantime, @Morrowless, dedupe your documents passed to GetAll to avoid this problem.

[Morrowless]

Thanks, that is exactly what I'm doing right now.

[wilhuff]

We should do as the other SDKs do:

1. create a set of unique resource names to request given the input document references
2. make the request for those resource names
3. associate response documents with the requested resource names
4. run through the array of requested document references to generate an array of responses such that each response positionally corresponds to the requested document

i.e. the third option.

[schmidt-sebastian]

@jadekler This seems like something we could already add to the conformance tests. If you look at the code sample linked to above, you can see that we only request one document per unique DocumentRef, but still provide duplicate results if requested.