Change RawDatastore to do less reflection.

go/src/infra/gae/libs/gae/properties.go

Index: go/src/infra/gae/libs/gae/properties.go
diff --git a/go/src/infra/gae/libs/gae/properties.go b/go/src/infra/gae/libs/gae/properties.go
index c7aa9e063e9c992a7b0eeee48186ff171199ac3d..bd719b12e62dcb9b0a4acc21bf5d9a276a5ce917 100644
--- a/go/src/infra/gae/libs/gae/properties.go
+++ b/go/src/infra/gae/libs/gae/properties.go
@@ -13,10 +13,10 @@ import (
 )
 
 var (
-	// ErrDSSpecialFieldUnset is returned from DSStructPLS.{Get,Set}Special
-	// implementations when the specified special key isn't set on the struct at
+	// ErrDSMetaFieldUnset is returned from DSPropertyLoadSaver.{Get,Set}Meta
+	// implementations when the specified meta key isn't set on the struct at
 	// all.
-	ErrDSSpecialFieldUnset = fmt.Errorf("gae: special field unset")
+	ErrDSMetaFieldUnset = fmt.Errorf("gae: meta field unset")
 )
 
 var (
@@ -45,6 +45,28 @@ type DSProperty struct {
 	propType DSPropertyType
 }
 
+// MkDSProperty makes a new DSProperty and returns it. If val is an invalid

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

nit: a new indexed DSProperty

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

Done.

+// value, this panics (so don't do it). If you want to handle the error
+// normally, use SetValue instead.

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

nit: use SetValue(..., false) instead.

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

Done.

+func MkDSProperty(val interface{}) DSProperty {
+	ret := DSProperty{}
+	if err := ret.SetValue(val, false); err != nil {
+		panic(err)
+	}
+	return ret
+}
+
+// MkDSPropertyNI makes a new DSProperty (with noindex set to true), and returns
+// it. If val is an invalid value, this panics (so don't do it). If you want to
+// handle the error normally, use SetValue instead.

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

nit: use SetValue(..., true) instead

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

Done.

+func MkDSPropertyNI(val interface{}) DSProperty {
+	ret := DSProperty{}
+	if err := ret.SetValue(val, true); err != nil {

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

nit: 'true' to indicate "noindex" is a bit confusing. Maybe use named constants
instead of bool?

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

Yeah, I think you're right (it was more copy pasta :(). Fixed everywhere.

+		panic(err)
+	}
+	return ret
+}
+
 // DSPropertyConverter may be implemented by the pointer-to a struct field which
 // is serialized by RawDatastore. Its ToDSProperty will be called on save, and
 // it's FromDSProperty will be called on load (from datastore). The method may
@@ -292,34 +314,41 @@ func (p *DSProperty) SetValue(value interface{}, noIndex bool) (err error) {
 
 // DSPropertyLoadSaver may be implemented by a user type, and RawDatastore will
 // use this interface to serialize the type instead of trying to automatically
-// create a serialization codec for it with helper.GetStructPLS.
+// create a serialization codec for it with helper.GetPLS.
 type DSPropertyLoadSaver interface {
-	Load(DSPropertyMap) (convFailures []string, fatal error)
-	Save() (DSPropertyMap, error)
-}
+	// Load takes the values from the given map and attempts to save them into
+	// the underlying object (usually a struct or a DSPropertyMap). If a fatal
+	// error occurs, it's returned via error. If non-fatal conversion errors
+	// occur, error will be a MultiError containing one or moer ErrDSFieldMismatch
+	// objects.
+	Load(DSPropertyMap) error
+	Save(withMeta bool) (DSPropertyMap, error)

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

document Save?

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

Done.

 
-// DSStructPLS is a DSPropertyLoadSaver, but with some bonus features which only
-// apply to user structs (instead of raw DSPropertyMap's).
-type DSStructPLS interface {
-	DSPropertyLoadSaver
-
-	// GetSpecial will get information about the struct field which has the
-	// struct tag in the form of `gae:"$<key>"`.
+	// GetMeta will get information about the field which has the struct tag in
+	// the form of `gae:"$<key>"`.
+	//
+	// string and {,u}int fields will return the value in the struct tag,
+	// converted to the appropriate type, if the field has the zero value. If the
+	// struct tag is improperly formatted, an error will be returned (e.g. having
+	// an int field with a non-integer value in the tag).
 	//
 	// Example:
 	//   type MyStruct struct {
-	//     CoolField int `gae:"$id,cool"`
+	//     CoolField int `gae:"$id,1"`
 	//   }
-	//   val, current, err := helper.GetStructPLS(&MyStruct{10}).GetSpecial("id")
-	//   // val == "cool"
-	//   // current == 10
+	//   val, err := helper.GetPLS(&MyStruct{}).GetMeta("id")
+	//   // val == 1
 	//   // err == nil
-	GetSpecial(key string) (val string, current interface{}, err error)
+	//
+	//   val, err := helper.GetPLS(&MyStruct{10}).GetMeta("id")
+	//   // val == 10
+	//   // err == nil
+	GetMeta(key string) (interface{}, error)
 
-	// SetSpecial allows you to set the current value of the special-keyed field.
-	SetSpecial(key string, val interface{}) error
+	// SetMeta allows you to set the current value of the meta-keyed field.
+	SetMeta(key string, val interface{}) error
 
-	// Problem indicates that this StructPLS has a fatal problem. Usually this is
+	// Problem indicates that this PLS has a fatal problem. Usually this is
 	// set when the underlying struct has recursion, invalid field types, nested
 	// slices, etc.
 	Problem() error
@@ -329,28 +358,87 @@ type DSStructPLS interface {
 // It maps from property name to a list of property values which correspond to
 // that property name. It is the spiritual successor to PropertyList from the

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

spiritual successor, lol

 // original SDK.
+//
+// DSPropertyMap may contain "meta" values, which are keyed with a '$' prefix.
+// Technically the datastore allows property names, but all of the SDKs go out

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

nit: "allows arbitrary property names"? I think you forgot a word there...

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

yes! good catch! Done.

+// of their way to try to make all property names valid programming language
+// tokens. Special values must correspond to a single DSProperty...
+// corresponding to 0 is equivalent to unset, and corresponding to >1 is an
+// error. So:
+//
+//   {
+//     "$id": {MkDSProperty(1)}, // GetProperty("id") -> 1, nil
+//     "$foo": {}, // GetProperty("foo") -> nil, ErrDSMetaFieldUnset
+//     // GetProperty("bar") -> nil, ErrDSMetaFieldUnset
+//     "$meep": {
+//       MkDSProperty("hi"),
+//       MkDSProperty("there")}, // GetProperty("meep") -> nil, error!
+//   }
+//
+// Additionally, Save returns a copy of the map with the meta keys omitted (e.g.
+// these keys are not going to be serialized to the datastore).
 type DSPropertyMap map[string][]DSProperty
 
-var _ DSPropertyLoadSaver = (*DSPropertyMap)(nil)
+var _ DSPropertyLoadSaver = DSPropertyMap(nil)
 
 // Load implements DSPropertyLoadSaver.Load
-func (pm *DSPropertyMap) Load(props DSPropertyMap) (convErr []string, err error) {
+func (pm DSPropertyMap) Load(props DSPropertyMap) error {
 	if pm == nil {
-		return nil, errors.New("gae: nil DSPropertyMap")
-	}
-	if *pm == nil {
-		*pm = make(DSPropertyMap, len(props))
+		return errors.New("gae: nil DSPropertyMap")

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

let it panic? It's caller responsibility to not pass nil.

Actually, it's receiver. So definitely let it panic...

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

sgtm, done.

 	}
 	for k, v := range props {
-		(*pm)[k] = append((*pm)[k], v...)
+		pm[k] = append(pm[k], v...)
 	}
-	return nil, nil
+	return nil
 }
 
 // Save implements DSPropertyLoadSaver.Save
-func (pm *DSPropertyMap) Save() (DSPropertyMap, error) {
+func (pm DSPropertyMap) Save(withMeta bool) (DSPropertyMap, error) {
+	if pm == nil {
+		return nil, errors.New("gae: nil DSPropertyMap")
+	}
+	if withMeta {
+		return pm, nil

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

copy pm before returning?

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

okay :/ I hate copying stuff... GO, Y U NO CONST :(

+	}
+	ret := make(DSPropertyMap, len(pm))
+	for k, v := range pm {
+		if k[0] == '$' { // remove meta keys

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

are you 100% sure k != ""?

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

good thought, done.

+			continue
+		}
+		ret[k] = append(ret[k], v...)
+	}
+	return ret, nil
+}
+
+func (pm DSPropertyMap) GetMeta(key string) (interface{}, error) {
 	if pm == nil {
 		return nil, errors.New("gae: nil DSPropertyMap")
 	}
-	return *pm, nil
+	v, ok := pm["$"+key]
+	if !ok || len(v) == 0 {
+		return nil, ErrDSMetaFieldUnset
+	}
+	if len(v) > 1 {
+		return nil, errors.New("gae: too many values for Meta key")
+	}
+	return v[0].Value(), nil
+}
+
+func (pm DSPropertyMap) SetMeta(key string, val interface{}) error {
+	if pm == nil {
+		return errors.New("gae: nil DSPropertyMap")
+	}
+	prop := DSProperty{}
+	if err := prop.SetValue(val, true); err != nil {
+		return err
+	}
+	pm["$"+key] = []DSProperty{prop}
+	return nil
+}
+
+func (pm DSPropertyMap) Problem() error {
+	if pm == nil {
+		return errors.New("gae: nil DSPropertyMap")
+	}
+	return nil
 }