Change RawDatastore to do less reflection.

go/src/infra/gae/libs/gae/helper/datastore_impl.go

Index: go/src/infra/gae/libs/gae/helper/datastore_impl.go
diff --git a/go/src/infra/gae/libs/gae/helper/datastore_impl.go b/go/src/infra/gae/libs/gae/helper/datastore_impl.go
index bfe066ea4871b9713e6ade287f3a284fe316d5e7..01a032e1530ab7e034c1b27bf0605b8c428abbf9 100644
--- a/go/src/infra/gae/libs/gae/helper/datastore_impl.go
+++ b/go/src/infra/gae/libs/gae/helper/datastore_impl.go
@@ -11,6 +11,7 @@ import (
 	"fmt"
 	"infra/gae/libs/gae"

[Vadim Sh., 2015/07/14 18:55:43]

nit: move into separate import block

[iannucci, 2015/07/14 22:45:48]

Done.

 	"reflect"
+	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -25,6 +26,8 @@ var (
 	typeOfDSPropertyConverter = reflect.TypeOf((*gae.DSPropertyConverter)(nil)).Elem()
 	typeOfGeoPoint            = reflect.TypeOf(gae.DSGeoPoint{})
 	typeOfTime                = reflect.TypeOf(time.Time{})
+	typeOfString              = reflect.TypeOf("")
+	typeOfInt64               = reflect.TypeOf(int64(0))
 
 	valueOfnilDSKey = reflect.Zero(typeOfDSKey)
 )
@@ -35,15 +38,16 @@ type structTag struct {
 	isSlice        bool
 	substructCodec *structCodec
 	convert        bool
-	specialVal     string
+	metaVal        interface{}
+	metaIsString   bool

[iannucci, 2015/07/14 18:15:44]

This metaIsString used to be useful... I'll take it out.

 }
 
 type structCodec struct {
-	bySpecial map[string]int
-	byName    map[string]int
-	byIndex   []structTag
-	hasSlice  bool
-	problem   error
+	byMeta   map[string]int
+	byName   map[string]int
+	byIndex  []structTag
+	hasSlice bool
+	problem  error
 }
 
 type structPLS struct {
@@ -51,7 +55,7 @@ type structPLS struct {
 	c *structCodec
 }
 
-var _ gae.DSStructPLS = (*structPLS)(nil)
+var _ gae.DSPropertyLoadSaver = (*structPLS)(nil)
 
 // typeMismatchReason returns a string explaining why the property p could not
 // be stored in an entity field of type v.Type().
@@ -60,22 +64,35 @@ func typeMismatchReason(val interface{}, v reflect.Value) string {
 	return fmt.Sprintf("type mismatch: %s versus %v", entityType, v.Type())
 }
 
-func (p *structPLS) Load(propMap gae.DSPropertyMap) (convFailures []string, fatal error) {
-	if fatal = p.Problem(); fatal != nil {
-		return
+func (p *structPLS) Load(propMap gae.DSPropertyMap) error {

[iannucci, 2015/07/14 18:15:44]

I changed this interface to only return one error, since Get must pass this
error through. The caller can now determine the type of the error by seeing if
it's a gae.MultiError (which contains *ErrDSFieldMismatch objects). Practically
speaking, this sort of thing only happens on a poorly-planned schema change
(e.g. rename or removal of a struct field and then the calling code doesn't Get
into DSPropertyMap for backwards compatibility).

+	if err := p.Problem(); err != nil {
+		return err
 	}
 
-	t := p.o.Type()
+	convFailures := gae.MultiError(nil)
+
+	t := reflect.Type(nil)
 	for name, props := range propMap {
 		multiple := len(props) > 1
 		for i, prop := range props {
 			if reason := loadInner(p.c, p.o, i, name, prop, multiple); reason != "" {
-				convFailures = append(convFailures, fmt.Sprintf(
-					"cannot load field %q into a %q: %s", name, t, reason))
+				if t == nil {
+					t = p.o.Type()
+				}
+				convFailures = append(convFailures, &gae.ErrDSFieldMismatch{
+					StructType: t,
+					FieldName:  name,
+					Reason:     reason,
+				})
 			}
 		}
 	}
-	return
+
+	if convFailures != nil {
+		return convFailures
+	}
+
+	return nil
 }
 
 func loadInner(codec *structCodec, structValue reflect.Value, index int, name string, p gae.DSProperty, requireSlice bool) string {
@@ -222,12 +239,29 @@ func loadInner(codec *structCodec, structValue reflect.Value, index int, name st
 	return ""
 }
 
-func (p *structPLS) Save() (gae.DSPropertyMap, error) {
-	ret := gae.DSPropertyMap{}
+func (p *structPLS) Save(withMeta bool) (gae.DSPropertyMap, error) {

[iannucci, 2015/07/14 18:15:44]

withMeta was added so that the caller can ask for a DSPropertyMap which does or
does not preserve metadata ($-keyed fields). Filters will typically only ever
pass true to this (if they need to call it at all. the mcache filter is the only
one that will really need to call it, since it needs a DSPropertyMap for
serialization purposes). Preserving metadata means that a filter can convert a
structPLS -> DSPropertyMap, and pass the DSPropertyMap to the filter below it,
ensuring that a struct will only ever be converted to a DSPropertyMap once per
Get/GetAll call (since DSPropertyMap.Save(true) just returns the map itself).

+	size := len(p.c.byName)
+	if withMeta {
+		size += len(p.c.byMeta)
+	}
+	ret := make(gae.DSPropertyMap, size)
 	idxCount := 0
 	if err := p.save(ret, &idxCount, "", false); err != nil {

[Vadim Sh., 2015/07/14 18:55:43]

are you using pointers to return values from a function (&idxCount)? Ugh.. I
see. I was confused why it isn't referenced in Save(...) body itself.

[iannucci, 2015/07/14 22:45:47]

Yeah, I'm sorry... this is C+P from the SDK implementation. I've fixed it though
:)

 		return nil, err
 	}
+	if withMeta {
+		for k := range p.c.byMeta {
+			val, err := p.GetMeta(k)
+			if err != nil {
+				return nil, err // TODO(riannucci): should these be ignored?
+			}
+			p := gae.DSProperty{}
+			if err = p.SetValue(val, true); err != nil {
+				return nil, err
+			}
+			ret["$"+k] = []gae.DSProperty{p}
+		}
+	}
 	return ret, nil
 }
 
@@ -285,35 +319,37 @@ func (p *structPLS) save(propMap gae.DSPropertyMap, idxCount *int, prefix string
 	return nil
 }
 
-func (p *structPLS) GetSpecial(key string) (val string, current interface{}, err error) {
-	if err = p.Problem(); err != nil {
-		return
+func (p *structPLS) GetMeta(key string) (interface{}, error) {

[iannucci, 2015/07/14 18:15:43]

"special" was renamed to "meta" to more clearly denote what it was

Additionally, metadata now only can have one value. Consider:

type S struct {
  F int64 `$meta,100`
}

If F has the zero value (in this case ""), and there's a default value (i.e.
",100" in the above example), then GetMeta("meta") will return (int64(100),
nil). If F doesn't have the zero value, it's passed through unscathed.  Only
string and int64, and bool are supported meta values at this time. The code for
converting the default value is in the getStructCodecLocked function below.

+	if err := p.Problem(); err != nil {
+		return nil, err
 	}
-	idx, ok := p.c.bySpecial[key]
+	idx, ok := p.c.byMeta[key]
 	if !ok {
-		err = gae.ErrDSSpecialFieldUnset
-		return
+		return nil, gae.ErrDSMetaFieldUnset
 	}
-	val = p.c.byIndex[idx].specialVal
+	st := p.c.byIndex[idx]
+	val := st.metaVal
 	f := p.o.Field(idx)
 	if f.CanSet() {
-		current = f.Interface()
+		if !reflect.DeepEqual(reflect.Zero(f.Type()).Interface(), f.Interface()) {
+			val = f.Interface()
+		}
 	}
-	return
+	return val, nil
 }
 
-func (p *structPLS) SetSpecial(key string, val interface{}) (err error) {
+func (p *structPLS) SetMeta(key string, val interface{}) (err error) {
 	if err = p.Problem(); err != nil {
 		return
 	}
-	idx, ok := p.c.bySpecial[key]
+	idx, ok := p.c.byMeta[key]
 	if !ok {
-		return gae.ErrDSSpecialFieldUnset
+		return gae.ErrDSMetaFieldUnset
 	}
 	defer func() {
 		pv := recover()

[Vadim Sh., 2015/07/14 18:55:43]

nooooo!.. Is this the only way to catch errors here?

My concern with recover() (in general): it looks like "catch (...) { }" in C++,
except it also catches memory access errors etc. Though arguably
p.o.Field(idx).Set(reflect.ValueOf(val)) doesn't have a chance to produce any
really unexpected exception.

[dnj, 2015/07/14 19:34:57]

+1. IMO if some app is concerned with a panic in its AppEngine layer, let it do
the recover.

[iannucci, 2015/07/14 22:45:48]

Fixed!

 		if pv != nil && err == nil {
-			err = fmt.Errorf("gae/helper: cannot set special %q: %s", key, pv)
+			err = fmt.Errorf("gae/helper: cannot set meta %q: %s", key, pv)
 		}
 	}()
 	p.o.Field(idx).Set(reflect.ValueOf(val))
@@ -372,17 +408,17 @@ func getStructCodecLocked(t reflect.Type) (c *structCodec) {
 	}
 
 	c = &structCodec{
-		byIndex:   make([]structTag, t.NumField()),
-		byName:    make(map[string]int, t.NumField()),
-		bySpecial: make(map[string]int, t.NumField()),
-		problem:   errRecursiveStruct, // we'll clear this later if it's not recursive
+		byIndex: make([]structTag, t.NumField()),
+		byName:  make(map[string]int, t.NumField()),
+		byMeta:  make(map[string]int, t.NumField()),
+		problem: errRecursiveStruct, // we'll clear this later if it's not recursive
 	}
 	defer func() {
 		// If the codec has a problem, free up the indexes
 		if c.problem != nil {
 			c.byIndex = nil
 			c.byName = nil
-			c.bySpecial = nil
+			c.byMeta = nil
 		}
 	}()
 	structCodecs[t] = c
@@ -402,12 +438,18 @@ func getStructCodecLocked(t reflect.Type) (c *structCodec) {
 			}
 		case name[0] == '$':
 			name = name[1:]
-			if _, ok := c.bySpecial[name]; ok {
-				c.problem = me("special field %q set multiple times", "$"+name)
+			if _, ok := c.byMeta[name]; ok {
+				c.problem = me("meta field %q set multiple times", "$"+name)
 				return
 			}
-			c.bySpecial[name] = i
-			st.specialVal = opts
+			c.byMeta[name] = i
+			mv, err := convertMeta(opts, f.Type)
+			if err != nil {
+				c.problem = me("meta field %q has bad type: %s", "$"+name, err)
+				return
+			}
+			st.metaVal = mv
+			st.metaIsString = f.Type.Kind() == reflect.String
 			fallthrough
 		case name == "-":
 			st.name = "-"
@@ -507,3 +549,16 @@ func getStructCodecLocked(t reflect.Type) (c *structCodec) {
 	}
 	return
 }
+
+func convertMeta(val string, t reflect.Type) (interface{}, error) {
+	switch t {

[Vadim Sh., 2015/07/14 18:55:43]

what about boolean?

[iannucci, 2015/07/14 22:45:48]

oh, right. I said supported that, didn't I? :D I'm going to just not support
that for now, and add support when there's actually a use for it.

+	case typeOfString:
+		return val, nil
+	case typeOfInt64:
+		if val == "" {
+			return int64(0), nil
+		}
+		return strconv.ParseInt(val, 10, 64)
+	}
+	return nil, fmt.Errorf("helper: meta field with bad type %s", t)
+}