Google-cloud-go: firestore: Document IDs changed

[CaptainCodeman]

It looks like the generated document IDs have changed, previously they were consistently 20 character strings, the last one I got was 43 (as they are also base64 encoded)

I didn't see it documented anywhere and it appears to now differ from other firestore clients. Is this intentional? There's a chance it might impact things if people are expecting the 20 characters (e.g. an ID reference being written to a SQL table etc...)

Old:

func uniqueID() string {
    var b [20]byte
    rngMu.Lock()
    for i := 0; i < len(b); i++ {
        b[i] = alphanum[rng.Intn(len(alphanum))]
    }
    rngMu.Unlock()
    return string(b[:])
}

https://github.com/googleapis/google-cloud-go/blob/6e5e2ac4e57e83f4b56609a4cad822eb6a497921/firestore/collref.go#L128

New:

func uniqueID() string {
    b := make([]byte, 32)
    if _, err := rand.Read(b); err != nil {
        panic(fmt.Sprintf("firestore: crypto/rand.Read error: %v", err))
    }
    return base64.RawURLEncoding.EncodeToString(b)
}

https://github.com/googleapis/google-cloud-go/blob/6e5e2ac4e57e83f4b56609a4cad822eb6a497921/firestore/collref.go#L128

[codyoss]

cc @schmidt-sebastian

[schmidt-sebastian]

That change surprises me. @tbpg do you remember why we changed the auto-generated document IDs in the Go client?

[tbpg]

I changed the ID generation in https://code-review.googlesource.com/c/gocloud/+/44570 as the random number generation was not actually random. Sorry for the surprise!

Should we switch it back to always 20 bytes?

[schmidt-sebastian]

I would vote in favor of it as all other clients use 20 ASCII characters.

[CaptainCodeman]

I would prefer it to behave the same as other clients - that was one of the guarantees of the official client libs when they launched and being able to translate between languages and have similar results is quite useful. Obviously not identical for the generated keys (wouldn't _that_ be something!) but a wildly different format seems like it would be a surprise. You might also be OK using a 20 character alphanumeric string in a URL for instance but not a 43+ one.

As for how random the pseudo-random used before was, I believe it was random _enough_ for creating a unique identifier if seeded. It shouldn't be the firestore clients job to generate cryptographically secure keys if that's what someone happens to want to use them for and personally, I wouldn't want to be pulling in more code (I'm guessing the crypto package is more involved) and burning more CPU cycles without realizing it.

[tbpg]

Sounds good. I'll send a CL to switch back to 20 characters. As for using crypto/rand or math/rand, I'd prefer to continue to use crypto/rand. CPU usage and complexity are not dramatically worse, and any performance degradation would be overshadowed by any other network activity.

Quoting from @FiloSottile on https://code-review.googlesource.com/c/gocloud/+/44570:

For example, if there's always an HTTPS request associated with that operation, there's no chance this will matter.

[tripflex]

When will this be available via cloud.google.com/go/firestore ? The latest version shows as 1.1.1 which does not appear to have this fixed, and this has caused huge issues for me, including creating auto generated IDs that start with a hyphens - and i don't know enough about Go to build from source (i spent the past 2 hours trying to figure out how to fix this in fuego library with no luck)

[CaptainCodeman]

You can always include the old code to create IDs as they were previously until it's reverted, e.g.

var (
    rngMu sync.Mutex
    rng   = rand.New(rand.NewSource(time.Now().UnixNano() ^ int64(os.Getpid())))
)

const alphanum = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"

func uniqueID() string {
    var b [20]byte
    rngMu.Lock()
    for i := 0; i < len(b); i++ {
        b[i] = alphanum[rng.Intn(len(alphanum))]
    }
    rngMu.Unlock()
    return string(b[:])
}

Use the uniqueID function when you want an ID.

You may need to make some changes to how you write docs though (not just letting it create the ref for you).

[tbpg]

Sorry about that -- we'll try to get a release out today or possibly next week.

[tbpg]

You can always depend on a particular commit using a pseudo-version:

go get cloud.google.com/go/firestore@master

[codyoss]

@tripflex https://github.com/googleapis/google-cloud-go/releases/tag/firestore%2Fv1.2.0

[tripflex]

Thank you guys for getting this out so quickly, i do appreciate it!