Get rid of awkward proto argument to Interface.Run

service/datastore/datastore.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 datastore
 
 import (
         "fmt"
         "reflect"
 
         "github.com/luci/luci-go/common/errors"
 )
 
 type datastoreImpl struct{ RawInterface }
 
 var _ Interface = (*datastoreImpl)(nil)
 
 func (d *datastoreImpl) KeyForObj(src interface{}) Key {
         ret, err := d.KeyForObjErr(src)
         if err != nil {
                 panic(err)
         }
         return ret
 }
 
 func (d *datastoreImpl) KeyForObjErr(src interface{}) (Key, error) {
         return newKeyObjErr(d.NewKey, src)
 }
 
-func (d *datastoreImpl) Run(q Query, proto interface{}, cb RunCB) error {
-        if _, ok := proto.(*Key); ok {
-                return d.RawInterface.Run(q.KeysOnly(), func(k Key, _ PropertyMap, gc func() (Cursor, error)) bool {
+func (d *datastoreImpl) Run(q Query, cbIface interface{}) error {
+        cbTyp := reflect.TypeOf(cbIface)
+        if cbTyp.NumIn() != 2 || cbTyp.NumOut() != 1 {

[Vadim Sh., 2015/08/07 20:23:05]

check that cbTyp is a function?

[iannucci, 2015/08/07 20:37:50]

Done.

+                return fmt.Errorf("cb has the wrong number of args: %T", cbIface)
+        }
+
+        if cbTyp.Out(0) != typeOfBool {
+                return fmt.Errorf("cb has the wrong return type (expected bool): %s", cbTyp.Out(0))
+        }
+
+        if cbTyp.In(1) != typeOfCursorCB {
+                return fmt.Errorf("cb has a bad second argument (expected CursorCB): %s", cbTyp.In(1))
+        }
+
+        if cbTyp.In(0) == typeOfKey {

[Vadim Sh., 2015/08/07 20:23:05]

nit: you can probably cache all this checks (using cbTyp as a cache key).
Probably an overkill?

[iannucci, 2015/08/07 20:37:50]

Yeah, probably can. I suspect it's overkill though. I'll add a todo in case we
ever profile stuff to look here.

[iannucci, 2015/08/07 20:37:50]

Yeah, probably can. I suspect it's overkill though. I'll add a todo in case we
ever profile stuff to look here.

+                cb := cbIface.(func(Key, CursorCB) bool)
+                return d.RawInterface.Run(q.KeysOnly(), func(k Key, _ PropertyMap, gc CursorCB) bool {
                         return cb(k, gc)
                 })
         }
 
-        mat := parseArg(reflect.TypeOf(proto))
+        mat := parseArg(cbTyp.In(0))
         if !mat.valid || mat.newElem == nil {
-                return fmt.Errorf("invalid Run proto type: %T", proto)
+                return fmt.Errorf("cb has a bad first argument: %s", cbTyp.In(0))
         }
+        cbVal := reflect.ValueOf(cbIface)
 
         innerErr := error(nil)
-        err := d.RawInterface.Run(q, func(k Key, pm PropertyMap, gc func() (Cursor, error)) bool {
+        err := d.RawInterface.Run(q, func(k Key, pm PropertyMap, gc CursorCB) bool {
                 itm := mat.newElem()
                 if innerErr = mat.setPM(itm, pm); innerErr != nil {
                         return false
                 }
                 mat.setKey(itm, k)
-                return cb(itm.Interface(), gc)
+                return cbVal.Call([]reflect.Value{itm, reflect.ValueOf(gc)})[0].Bool()
         })
         if err == nil {
                 err = innerErr
         }
         return err
 }
 
 func (d *datastoreImpl) GetAll(q Query, dst interface{}) error {
         v := reflect.ValueOf(dst)
         if v.Kind() != reflect.Ptr {
                 return fmt.Errorf("invalid GetAll dst: must have a ptr-to-slice: %T", dst)
         }
         if !v.IsValid() || v.IsNil() {
                 return errors.New("invalid GetAll dst: <nil>")
         }
 
         if keys, ok := dst.(*[]Key); ok {
-                return d.RawInterface.Run(q.KeysOnly(), func(k Key, _ PropertyMap, _ func() (Cursor, error)) bool {
+                return d.RawInterface.Run(q.KeysOnly(), func(k Key, _ PropertyMap, _ CursorCB) bool {
                         *keys = append(*keys, k)
                         return true
                 })
         }
 
         slice := v.Elem()
         mat := parseMultiArg(slice.Type())
         if !mat.valid || mat.newElem == nil {
                 return fmt.Errorf("invalid GetAll input type: %T", dst)
         }
 
         lme := errors.LazyMultiError{Size: slice.Len()}
         i := 0
-        err := d.RawInterface.Run(q, func(k Key, pm PropertyMap, _ func() (Cursor, error)) bool {
+        err := d.RawInterface.Run(q, func(k Key, pm PropertyMap, _ CursorCB) bool {
                 slice.Set(reflect.Append(slice, mat.newElem()))
                 itm := slice.Index(i)
                 mat.setKey(itm, k)
                 lme.Assign(i, mat.setPM(itm, pm))
                 i++
                 return true
         })
         if err == nil {
                 err = lme.Get()
         }
@@ skipping 94 matching lines @@
         err = lme.Get()
         if err == nil {
                 err = extErr
         }
         return
 }
 
 func (d *datastoreImpl) Raw() RawInterface {
         return d.RawInterface
 }