Differentiate between single- and multi- props.

service/datastore/properties.go

 // Copyright 2015 The LUCI Authors. All rights reserved.
 // Use of this source code is governed under the Apache License, Version 2.0
 // that can be found in the LICENSE file.
 
 package datastore
 
 import (
         "bytes"
         "encoding/base64"
         "errors"
@@ skipping 318 matching lines @@
 
         switch t {
         case typeOfKey:
                 if v.IsNil() {
                         return nil
                 }
         }
         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:
                 return fmt.Sprintf("%s(%q)", p.propType, p.Value())
         case PTBytes:
                 return fmt.Sprintf("%s(%#x)", p.propType, p.Value())
         default:
                 return fmt.Sprintf("%s(%v)", p.propType, p.Value())
         }
 }
@@ skipping 272 matching lines @@
                 if b.Less(a) {
                         return 1
                 }
                 return -1
 
         default:
                 panic(fmt.Errorf("uncomparable type: %s", t))
         }
 }
 
+// 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.
 //
 // The flavor of GQL that this emits is defined here:
 //   https://cloud.google.com/datastore/docs/apis/gql/gql_reference
 //
 // NOTE: GeoPoint values are emitted with speculated future syntax. There is
 // currently no syntax for literal GeoPoint values.
 func (p *Property) GQL() string {
         v := p.Value()
@@ skipping 24 matching lines @@
         case PTGeoPoint:
                 // note that cloud SQL doesn't support this yet, but take a good guess at
                 // it.
                 v := v.(GeoPoint)
                 return fmt.Sprintf("GEOPOINT(%v, %v)", v.Lat, v.Lng)
         }
         panic(fmt.Errorf("bad type: %s", p.propType))
 }
 
 // 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 (PropertySlice) isAPropertyData() {}
+
+// Clone implements the PropertyData interface.
+func (s PropertySlice) Clone() PropertyData {
+        if len(s) == 0 {
+                return PropertySlice(nil)
+        }
+        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]) }
 
-// 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()))
-}
-
 // MetaGetter is a subinterface of PropertyLoadSaver, but is also used to
 // abstract the meta argument for RawInterface.GetMulti.
 type MetaGetter interface {
         // GetMeta will get information about the field which has the struct tag in
         // the form of `gae:"$<key>[,<default>]?"`.
         //
         // It returns the value, if any, and true iff the value was retrieved.
         //
         // Supported metadata types are:
         //   int64  - may have default (ascii encoded base-10)
@@ skipping 60 matching lines @@
         // the `GetPLS` implementation will add any statically-defined metadata
         // fields. So if GetMeta provides $id, but there's a simple tagged field for
         // $kind, this method is only expected to return a PropertyMap with "$id".
         GetAllMeta() PropertyMap
 
         // SetMeta allows you to set the current value of the meta-keyed field.
         // It returns true iff the field was set.
         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
 // original SDK.
 //
 // PropertyMap may contain "meta" values, which are keyed with a '$' prefix.
 // Technically the datastore allows arbitrary property names, but all of the
 // SDKs go out of their way to try to make all property names valid programming
 // language tokens. Special values must correspond to a single Property...
 // corresponding to 0 is equivalent to unset, and corresponding to >1 is an
 // error. So:
 //
 //   {
 //     "$id": {MkProperty(1)}, // GetProperty("id") -> 1, nil
 //     "$foo": {}, // GetProperty("foo") -> nil, ErrMetaFieldUnset
 //     // GetProperty("bar") -> nil, ErrMetaFieldUnset
 //     "$meep": {
 //       MkProperty("hi"),
 //       MkProperty("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 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
 }
 
 // Save implements PropertyLoadSaver.Save by returning a copy of the
 // current map data.
 func (pm PropertyMap) Save(withMeta bool) (PropertyMap, error) {
         if len(pm) == 0 {
                 return PropertyMap{}, nil
         }
         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
 }
 
 // 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.
 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
 }
 
 // SetMeta implements PropertyLoadSaver.SetMeta. It will only return an error
 // if `val` has an invalid type (e.g. not one supported by Property).
 func (pm PropertyMap) SetMeta(key string, val interface{}) bool {
         prop := Property{}
         if err := prop.SetValue(val, NoIndex); err != nil {
                 return false
         }
-        pm["$"+key] = []Property{prop}
+        pm["$"+key] = prop
         return true
 }
 
 // Problem implements PropertyLoadSaver.Problem. It ALWAYS returns nil.
 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.
 //
 // It uses https://cloud.google.com/appengine/articles/storage_breakdown?csw=1
 // as a guide for sizes.
 func (pm PropertyMap) EstimateSize() int64 {
         ret := int64(0)
         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.
         return k == "" || k[0] == '$'
 }
 
 // GetMetaDefault is a helper for GetMeta, allowing a default value.
 //
 // If the metadata key is not available, or its type doesn't equal the
 // homogenized type of dflt, then dflt will be returned.
@@ skipping 87 matching lines @@
                 if string(s) == string(t) {
                         return 0, true
                 }
                 if string(s) < string(t) {
                         return -1, true
                 }
                 return 1, true
         }
         return 0, false
 }