datastore: Commit.Key doesn't actually validate the PendingKey is from the right commit

[benjaminjkraft]

I was looking through the code for handling pending keys, in search of a bug that turned out to be unrelated (and in our code), and I noticed the following seemingly-incorrect behavior.

Code

// (some unrelated commit, here an empty one for simplicity)
c1, err := client.RunInTransaction(ctx, func(_ *datastore.Transaction) error { return nil })
if err != nil { panic(err) }


var m models.MyModel // (any struct will do)
k := datastore.IncompleteKey("MyModel", nil)
var pk *datastore.PendingKey
c2, err := ctx.Datastore().RunInTransaction(ctx, func(txn *datastore.Transaction) error {
	var err error
	pk, err = txn.Put(k, &ud)
	return err
})
if err != nil { panic(err) }

fmt.Println(c1.Key(pk), c2.Key(pk)) // prints "/MyModel,<id>" twice

Expected behavior

Since pk is a PendingKey from the commit c2, it seems the intention is that c1.Key(pk) is invalid and should panic.

Actual behavior

Instead, c1.Key(pk) is valid, and returns the same value as c2.Key(pk). This isn't a huge deal since the answer is right, but it's confusing; for example one could accidentally call c1.Key(pk) before c2 exists, and it would just return nil.

Cause

The problem is pretty clear in transaction.go: in Transaction.Commit (line 226) the code assigns p.commit = c, and then in Commit.Key (line 400) it errs if c != p.commit. But nothing ever assigns c in Commit, so it's nil! Meaning that all such commits compare equal. I think the intention is for commit to be effectively a sentinel, in which case the fix would be to at some point in Transaction.Commit assign c = new(Commit) or similar.

[crwilcox]

Thanks for raising this. While I am not sure this is truly a bug, I don't think the use case you stumbled upon serves a purpose and likely shouldn't be allowed.

As part of a docs change ~3 years ago this looks like it would have changed from exactly what you think it should be now ;)
01dbcbb#diff-079c68b9cecfc52927852b5c69dca2af9d71a8140b618164c70b5d323c794c66L209