Refactor keys and queries in datastore service and implementation.

impl/prod/datastore_key.go

Index: impl/prod/datastore_key.go
diff --git a/impl/prod/datastore_key.go b/impl/prod/datastore_key.go
index bbc65eaaca2ce8aad0ff74bd881598440f22fc2a..0892ca20735dd6ca37310fb4a559f1e943c089a3 100644
--- a/impl/prod/datastore_key.go
+++ b/impl/prod/datastore_key.go
@@ -6,7 +6,8 @@ package prod
 
 import (
 	ds "github.com/luci/gae/service/datastore"
-	"github.com/luci/gae/service/datastore/dskey"
+	"golang.org/x/net/context"
+	"google.golang.org/appengine"
 	"google.golang.org/appengine/datastore"
 )
 
@@ -14,51 +15,55 @@ type dsKeyImpl struct {
 	*datastore.Key
 }
 
-var _ ds.Key = dsKeyImpl{}
-
-func (k dsKeyImpl) Parent() ds.Key   { return dsR2F(k.Key.Parent()) }
-func (k dsKeyImpl) Incomplete() bool { return k.Key.Incomplete() }
-func (k dsKeyImpl) Valid(allowSpecial bool, aid, ns string) bool {
-	return dskey.Valid(k, allowSpecial, aid, ns)
-}
-func (k dsKeyImpl) PartialValid(aid, ns string) bool {
-	return dskey.PartialValid(k, aid, ns)
-}
-
 // dsR2F (DS real-to-fake) converts an SDK Key to a ds.Key
-func dsR2F(k *datastore.Key) ds.Key {
+func dsR2F(k *datastore.Key) *ds.Key {
 	if k == nil {
 		return nil
 	}
-	return dsKeyImpl{k}
+	aid := k.AppID()
+	ns := k.Namespace()
+	toks := []ds.KeyTok(nil)

[dnj, 2015/09/18 16:47:57]

Since this will be executed frequently, it may be worth doing a counting
traversal first so you can allocate "toks" to the correct size.

[iannucci, 2015/09/18 22:25:48]

done

+	for k != nil {
+		toks = append(toks, ds.KeyTok{
+			Kind: k.Kind(), StringID: k.StringID(), IntID: k.IntID()})
+		k = k.Parent()
+	}
+	for i, j := 0, len(toks)-1; i < j; i, j = i+1, j-1 {
+		toks[i], toks[j] = toks[j], toks[i]
+	}
+	return ds.NewKeyToks(aid, ns, toks)
 }
 
 // dsF2R (DS fake-to-real) converts a DSKey back to an SDK *Key.
-func dsF2R(k ds.Key) *datastore.Key {
+func dsF2R(ctx context.Context, k *ds.Key) (*datastore.Key, error) {
 	if k == nil {
-		return nil
-	}
-	if rkey, ok := k.(dsKeyImpl); ok {
-		return rkey.Key
+		return nil, nil
 	}
-	// we should always hit the fast case above, but just in case, safely round
-	// trip through the proto encoding.
-	rkey, err := datastore.DecodeKey(dskey.Encode(k))
+
+	// drop aid.
+	_, ns, toks := k.Split()

[iannucci, 2015/09/18 04:31:52]

We drop the appid here... we can't actually control it, and the keys generated
here never leave this package and are just fed to the actual production
datastore.

[dnj, 2015/09/18 16:47:57]

Please document this in a formal comment.

Note that k.Split() can return a zero-length array if key is non-nil and the
KeyTok array is empty. You ensure that this cannot be the case by construction,
but I'd formally document the expectation that this is the case somewhere. 

[iannucci, 2015/09/18 22:25:48]

Noted in doc.go as a BUG.
Noted on service/datastore.Key

+	err := error(nil)
+	ctx, err = appengine.Namespace(ctx, ns)
 	if err != nil {
-		// should never happen in a good program, but it's not ignorable, and
-		// passing an error back makes this function too cumbersome (and it causes
-		// this `if err != nil { panic(err) }` logic to show up in a bunch of
-		// places. Realistically, everything should hit the early exit clause above.
-		panic(err)
+		return nil, err
 	}
-	return rkey
+
+	ret := datastore.NewKey(ctx, toks[0].Kind, toks[0].StringID, toks[0].IntID, nil)
+	for _, t := range toks[1:] {
+		ret = datastore.NewKey(ctx, t.Kind, t.StringID, t.IntID, ret)
+	}
+
+	return ret, nil
 }
 
 // dsMF2R (DS multi-fake-to-fake) converts a slice of wrapped keys to SDK keys.
-func dsMF2R(ks []ds.Key) []*datastore.Key {
+func dsMF2R(ctx context.Context, ks []*ds.Key) ([]*datastore.Key, error) {
+	err := error(nil)

[dnj, 2015/09/18 16:47:57]

Does it make sense to use a MultiError here?

[iannucci, 2015/09/18 22:25:47]

yuuuup

 	ret := make([]*datastore.Key, len(ks))
 	for i, k := range ks {
-		ret[i] = dsF2R(k)
+		if ret[i], err = dsF2R(ctx, k); err != nil {
+			return nil, err
+		}
 	}
-	return ret
+	return ret, nil
 }