Refactor keys and queries in datastore service and implementation.

impl/prod/datastore_key.go

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 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"
 )
 
 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
+                return nil, nil
         }
-        if rkey, ok := k.(dsKeyImpl); ok {
-                return rkey.Key
+
+        // 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 {
+                return nil, err
         }
-        // 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))
-        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)
+
+        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 rkey
+
+        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
 }