Differentiate between single- and multi- props.

service/datastore/properties.go

Index: service/datastore/properties.go
diff --git a/service/datastore/properties.go b/service/datastore/properties.go
index 2808c3804702d400198792664ac025441fe2a3da..f00eb51a8459580c8f426c4300c0bf5cfb24cfaf 100644
--- a/service/datastore/properties.go
+++ b/service/datastore/properties.go
@@ -336,6 +336,13 @@ func UpconvertUnderlyingType(o interface{}) interface{} {
 	return o
 }
 
+func (Property) isAPropertyData() {}
+
+func (p Property) estimateSize() int64 { return p.EstimateSize() }
+
+// Clone implements the PropertyData interface.
+func (p Property) Clone() PropertyData { return p }
+
 func (p Property) String() string {
 	switch p.propType {
 	case PTString, PTBlobKey:
@@ -628,6 +635,33 @@ func (p *Property) Compare(other *Property) int {
 	}
 }
 
+// EstimateSize estimates the amount of space that this Property would consume
+// if it were committed as part of an entity in the real production datastore.
+//
+// It uses https://cloud.google.com/appengine/articles/storage_breakdown?csw=1
+// as a guide for these values.
+func (p *Property) EstimateSize() int64 {
+	switch p.Type() {
+	case PTNull:
+		return 1
+	case PTBool:
+		return 1 + 4
+	case PTInt, PTTime, PTFloat:
+		return 1 + 8
+	case PTGeoPoint:
+		return 1 + (8 * 2)
+	case PTString:
+		return 1 + int64(len(p.Value().(string)))
+	case PTBlobKey:
+		return 1 + int64(len(p.Value().(blobstore.Key)))
+	case PTBytes:
+		return 1 + int64(len(p.Value().([]byte)))
+	case PTKey:
+		return 1 + p.Value().(*Key).EstimateSize()
+	}
+	panic(fmt.Errorf("Unknown property type: %s", p.Type().String()))
+}
+
 // GQL returns a correctly formatted Cloud Datastore GQL literal which
 // is valid for a comparison value in the `WHERE` clause.
 //
@@ -672,39 +706,36 @@ func (p *Property) GQL() string {
 }
 
 // PropertySlice is a slice of Properties. It implements sort.Interface.
+//
+// PropertySlice holds multiple Properties. Writing a PropertySlice to datastore
+// implicitly marks the property as "multiple", even if it only has one element.
 type PropertySlice []Property
 
-func (s PropertySlice) Len() int           { return len(s) }
-func (s PropertySlice) Swap(i, j int)      { s[i], s[j] = s[j], s[i] }
-func (s PropertySlice) Less(i, j int) bool { return s[i].Less(&s[j]) }
+func (PropertySlice) isAPropertyData() {}
 
-// EstimateSize estimates the amount of space that this Property would consume
-// if it were committed as part of an entity in the real production datastore.
-//
-// It uses https://cloud.google.com/appengine/articles/storage_breakdown?csw=1
-// as a guide for these values.
-func (p *Property) EstimateSize() int64 {
-	switch p.Type() {
-	case PTNull:
-		return 1
-	case PTBool:
-		return 1 + 4
-	case PTInt, PTTime, PTFloat:
-		return 1 + 8
-	case PTGeoPoint:
-		return 1 + (8 * 2)
-	case PTString:
-		return 1 + int64(len(p.Value().(string)))
-	case PTBlobKey:
-		return 1 + int64(len(p.Value().(blobstore.Key)))
-	case PTBytes:
-		return 1 + int64(len(p.Value().([]byte)))
-	case PTKey:
-		return 1 + p.Value().(*Key).EstimateSize()
+// Clone implements the PropertyData interface.
+func (s PropertySlice) Clone() PropertyData {
+	if len(s) == 0 {
+		return PropertySlice(nil)
 	}
-	panic(fmt.Errorf("Unknown property type: %s", p.Type().String()))
+	return append(make(PropertySlice, 0, len(s)), s...)
 }
 
+// EstimateSize estimates the amount of space that this PropertySlice would
+// consume if it were committed as part of an entity in the real production
+// datastore.
+func (s PropertySlice) estimateSize() (v int64) {
+	for _, prop := range s {
+		// Use the public one so we don't have to copy.
+		v += prop.estimateSize()
+	}
+	return
+}
+
+func (s PropertySlice) Len() int           { return len(s) }
+func (s PropertySlice) Swap(i, j int)      { s[i], s[j] = s[j], s[i] }
+func (s PropertySlice) Less(i, j int) bool { return s[i].Less(&s[j]) }
+
 // MetaGetter is a subinterface of PropertyLoadSaver, but is also used to
 // abstract the meta argument for RawInterface.GetMulti.
 type MetaGetter interface {
@@ -785,6 +816,20 @@ type MetaGetterSetter interface {
 	SetMeta(key string, val interface{}) bool
 }
 
+// PropertyData is an interface implemented by Property and PropertySlice to
+// identify themselves as valid PropertyMap values.
+type PropertyData interface {
+	// isAPropertyData is a tag that forces PropertyData implementations to only
+	// be supplied by this package.
+	isAPropertyData()
+
+	// estimateSize estimates the aggregate size of the property data.
+	estimateSize() int64
+
+	// Clone creates a duplicate copy of this PropertyData.
+	Clone() PropertyData
+}
+
 // PropertyMap represents the contents of a datastore entity in a generic way.
 // 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
@@ -808,14 +853,14 @@ type MetaGetterSetter interface {
 //
 // 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 PropertyMap map[string][]Property
+type PropertyMap map[string]PropertyData
 
 var _ PropertyLoadSaver = PropertyMap(nil)
 
 // Load implements PropertyLoadSaver.Load
 func (pm PropertyMap) Load(props PropertyMap) error {
 	for k, v := range props {
-		pm[k] = append(pm[k], v...)
+		pm[k] = v.Clone()

[dnj, 2016/09/16 19:07:22]

I'm going to write some code to clear out "pm" here so it doesn't have residual
properties. This seems like the right thing to do on Load.

[dnj, 2016/09/16 19:21:33]

Actually nm on this.

 	}
 	return nil
 }
@@ -829,7 +874,9 @@ func (pm PropertyMap) Save(withMeta bool) (PropertyMap, error) {
 	ret := make(PropertyMap, len(pm))
 	for k, v := range pm {
 		if withMeta || !isMetaKey(k) {
-			ret[k] = append(ret[k], v...)
+			if err := ret.appendPropertyData(k, v); err != nil {

[iannucci, 2016/09/16 07:11:03]

I think this just needs to be

  ret[k] = v.Clone()

now? The append was just a convenient way to copy the slice before. There's no
real 'append' happening.

[dnj, 2016/09/16 19:07:22]

Oh good catch. Yes, correct.

+				return nil, fmt.Errorf("gae: failed to save property %q: %v", k, err)
+			}
 		}
 	}
 	return ret, nil
@@ -838,11 +885,11 @@ func (pm PropertyMap) Save(withMeta bool) (PropertyMap, error) {
 // GetMeta implements PropertyLoadSaver.GetMeta, and returns the current value
 // associated with the metadata key.
 func (pm PropertyMap) GetMeta(key string) (interface{}, bool) {
-	v, ok := pm["$"+key]
-	if !ok || len(v) == 0 {
-		return nil, false
+	pslice := pm.Slice("$" + key)
+	if len(pslice) > 0 {
+		return pslice[0].Value(), true
 	}
-	return v[0].Value(), true
+	return nil, false
 }
 
 // GetAllMeta implements PropertyLoadSaver.GetAllMeta.
@@ -850,9 +897,7 @@ func (pm PropertyMap) GetAllMeta() PropertyMap {
 	ret := make(PropertyMap, 8)
 	for k, v := range pm {
 		if isMetaKey(k) {
-			newV := make([]Property, len(v))
-			copy(newV, v)
-			ret[k] = newV
+			ret[k] = v.Clone()
 		}
 	}
 	return ret
@@ -865,7 +910,7 @@ func (pm PropertyMap) SetMeta(key string, val interface{}) bool {
 	if err := prop.SetValue(val, NoIndex); err != nil {
 		return false
 	}
-	pm["$"+key] = []Property{prop}
+	pm["$"+key] = prop
 	return true
 }
 
@@ -874,6 +919,26 @@ func (pm PropertyMap) Problem() error {
 	return nil
 }
 
+// Slice returns a PropertySlice for the given key
+//
+// If the value associated with that key is nil, an empty slice will be
+// returned. If the value is single Property, a slice of size 1 with that
+// Property in it will be returned.
+func (pm PropertyMap) Slice(key string) PropertySlice {

[iannucci, 2016/09/16 07:11:03]

should we also have a `First(key string)`? Or Single?

[dnj, 2016/09/16 19:07:22]

I thought about this. TBH I am of the opinion that "Slice" is for when you just
want values and you don't care. It doesn't lose any data.

Something like "Single" would actually potentially lose some data, unless it
panics if len != 1. Seems not worthwhile given that Slice is fairly easy to work
with.

+	pdata := pm[key]
+	if pdata == nil {
+		return nil
+	}
+	switch t := pdata.(type) {
+	case Property:
+		return PropertySlice{t}
+	case PropertySlice:
+		return t
+	default:
+		panic(fmt.Errorf("unknown PropertyData type %T", t))
+	}
+}
+
 // EstimateSize estimates the size that it would take to encode this PropertyMap
 // in the production Appengine datastore. The calculation excludes metadata
 // fields in the map.
@@ -885,14 +950,40 @@ func (pm PropertyMap) EstimateSize() int64 {
 	for k, vals := range pm {
 		if !isMetaKey(k) {
 			ret += int64(len(k))
-			for i := range vals {
-				ret += vals[i].EstimateSize()
-			}
+			ret += vals.estimateSize()
 		}
 	}
 	return ret
 }
 
+func (pm PropertyMap) appendPropertyData(key string, pdata PropertyData) error {
+	cur := pm[key]
+	if cur == nil {
+		pm[key] = pdata.Clone()
+		return nil
+	}
+
+	// If there's already a value here, we can only append a new value if the
+	// current data is a slice.
+	pslice, ok := cur.(PropertySlice)
+	if !ok {
+		return errors.New("cannot add additional value to single-value property")
+	}
+
+	switch add := pdata.(type) {
+	case Property:
+		// Don't need to clone, since Property is by-value.
+		pm[key] = append(pslice, add)
+
+	case PropertySlice:
+		// Don't need to clone, since we're appending pdata, not retaining it.
+		if len(add) > 0 {
+			pm[key] = append(pslice, add...)
+		}
+	}
+	return nil
+}
+
 func isMetaKey(k string) bool {
 	// empty counts as a metakey since it's not a valid data key, but it's
 	// not really a valid metakey either.