impl/memory: Fix time serialization encoding.

service/datastore/properties.go

Index: service/datastore/properties.go
diff --git a/service/datastore/properties.go b/service/datastore/properties.go
index d82118dbe77eee6c75db7ddb90f0fd227f635e8f..6c3f5a62b3b4b149ba94a263101ca890f55cc11d 100644
--- a/service/datastore/properties.go
+++ b/service/datastore/properties.go
@@ -5,6 +5,7 @@
 package datastore
 
 import (
+	"bytes"
 	"encoding/base64"
 	"errors"
 	"fmt"
@@ -174,7 +175,11 @@ func init() {
 // Property is a value plus an indicator of whether the value should be
 // indexed. Name and Multiple are stored in the PropertyMap object.
 type Property struct {
-	value        interface{}
+	// value is the Property's value. It is stored in an internal, opaque type and
+	// should not be directly exported to consumers. Rather, it can be accessed
+	// in its original original value via Value().
+	value interface{}

[iannucci, 2015/12/31 23:22:34]

I would note particularly:

  * all byte sequence types are stored as byteSequence implementations
  * time is stored as int64

[dnj, 2016/01/01 17:43:30]

Done.

+
 	indexSetting IndexSetting
 	propType     PropertyType
 }
@@ -245,6 +250,12 @@ func PropertyTypeOf(v interface{}, checkValid bool) (PropertyType, error) {
 	}
 }
 
+// RoundTime rounds a time.Time to microseconds, which is the (undocumented)
+// way that the AppEngine SDK stores it.
+func RoundTime(t time.Time) time.Time {
+	return t.Round(time.Microsecond)
+}
+
 // timeLocationIsUTC tests if two time.Location are equal.
 //
 // This is tricky using the standard time API, as time is implicitly normalized
@@ -282,10 +293,9 @@ func UpconvertUnderlyingType(o interface{}) interface{} {
 		}
 	case reflect.Struct:
 		if t == typeOfTime {
-			// time in a Property can only hold microseconds
 			tim := v.Interface().(time.Time)
 			if !tim.IsZero() {
-				o = v.Interface().(time.Time).Round(time.Microsecond)
+				o = RoundTime(v.Interface().(time.Time))
 			}
 		}
 	}
@@ -295,7 +305,23 @@ func UpconvertUnderlyingType(o interface{}) interface{} {
 // Value returns the current value held by this property. It's guaranteed to
 // be a valid value type (i.e. `p.SetValue(p.Value(), true)` will never return
 // an error).
-func (p *Property) Value() interface{} { return p.value }
+func (p *Property) Value() (v interface{}) {

[iannucci, 2015/12/31 23:22:34]

you should now note that Value may return a copy of the data that's in this
Property. Previously the application could continue to 'own' the data in the
Property. E.g. if this returns []byte, and you modify it, it doesn't
(necessarily) change the underlying value.

[dnj, 2016/01/01 17:43:30]

I don't think that's the case. For example, PTByte was installed via []byte
-(SetValue)> bytesByteSequence -(Value)> []byte. It should still be the same
underlying byte array. PTString/PTBlobKey same deal. PTTime was always copied,
since it's passed by-value.

+	v = p.value
+
+	switch p.propType {
+	case PTBytes:
+		v = v.(byteSequence).Bytes()
+	case PTString:
+		v = v.(byteSequence).String()
+	case PTBlobKey:
+		v = blobstore.Key(v.(byteSequence).String())
+
+	case PTTime:
+		v = IntToTime(v.(int64))
+	}
+
+	return
+}
 
 // IndexSetting says whether or not the datastore should create indicies for
 // this value.
@@ -342,6 +368,24 @@ func (p *Property) SetValue(value interface{}, is IndexSetting) (err error) {
 			return
 		}
 	}
+
+	// Convert value to underlying datastore type.
+	switch t := value.(type) {
+	case string:
+		value = stringByteSequence(t)
+	case blobstore.Key:
+		value = stringByteSequence(t)
+	case []byte:
+		value = bytesByteSequence(t)
+	case time.Time:
+		t = RoundTime(t)
+		if t.IsZero() {
+			value = int64(0)
+		} else {
+			value = t.Unix()*1e6 + int64(t.Nanosecond()/1e3)
+		}
+	}
+
 	p.propType = pt
 	p.value = value
 	p.indexSetting = is
@@ -350,65 +394,81 @@ func (p *Property) SetValue(value interface{}, is IndexSetting) (err error) {
 
 // ForIndex gets a new Property with its value and type converted as if it were
 // being stored in a datastore index. See the doc on PropertyType for more info.
-func (p Property) ForIndex() Property {
-	switch p.propType {
-	case PTNull, PTInt, PTBool, PTString, PTFloat, PTGeoPoint, PTKey:
-		return p
+func (p Property) ForIndex() (PropertyType, interface{}) {

[iannucci, 2015/12/31 23:22:34]

why change the signature?

[dnj, 2016/01/01 17:43:30]

I was reluctant to export PTString types with []byte contents because it would
create an "always-copy" Property instance. Value(), when called by serialize,
would copy to a string. To avoid that, I'd need a "IndexValue" method that
doesn't do the conversion, and since index generation is internal-ish anyway
this seemed like a less publicly-visible approach.

+	switch t := p.propType; t {
+	case PTNull, PTInt, PTBool, PTFloat, PTGeoPoint, PTKey:
+		return t, p.Value()
 
 	case PTTime:
-		v, _ := p.Project(PTInt)
-		return Property{v, p.indexSetting, PTInt}
+		return PTInt, p.value
+
+	case PTString, PTBytes, PTBlobKey:
+		return PTString, p.value.(byteSequence).Value()

[iannucci, 2015/12/31 23:22:34]

why is .Value() necessary? Shouldn't it be .String() anyway?

Note that if you don't change the `ForIndex()` signature, you can just return a
new Property that's a copy of the old one, but with the propType changed (to
avoid copying the actual bytes data).

[dnj, 2016/01/01 17:43:30]

Don't want to return .String() b/c that would induce a copy if the underlying
type is []byte. By maintaining the underlying type, we get zero-copy return
value here. Instead, we return "PTString, []byte".

Another option is to export "byteSequence" and have users handle it, but I think
that's a lot of pain for little gain. ATM we have "ForIndex" for "internal-ish"
index generation, used basically by "serialize", and we have "Value" for public
stuff, which will remain zero-copy and native. Neither is really confusing or
abusable in its current state.
That was actually the original formulation. Looked good in the code, but see
above for thoughts.

+
+	default:
+		panic(fmt.Errorf("unknown PropertyType: %s", t))
+	}
+}
+
+// TimeToInt converts a time value to a datastore-appropraite integer value.
+//
+// This method truncates the time to microseconds and drops the timezone,
+// because that's the (undocumented) way that the appengine SDK does it.
+func TimeToInt(t time.Time) int64 {
+	t = RoundTime(t)
+	return t.Unix()*1e6 + int64(t.Nanosecond()/1e3)
+}
 
-	case PTBytes, PTBlobKey:
-		v, _ := p.Project(PTString)
-		return Property{v, p.indexSetting, PTString}
+// IntToTime converts a datastore time integer into its time.Time value.
+func IntToTime(v int64) time.Time {
+	if v == 0 {
+		return time.Time{}
 	}
-	panic(fmt.Errorf("unknown PropertyType: %s", p.propType))
+	return RoundTime(time.Unix(int64(v/1e6), int64((v%1e6)*1e3))).UTC()
 }
 
 // Project can be used to project a Property retrieved from a Projection query
 // into a different datatype. For example, if you have a PTInt property, you
 // could Project(PTTime) to convert it to a time.Time. The following conversions
 // are supported:
-//   PTXXX <-> PTXXX (i.e. identity)
-//   PTInt <-> PTTime
 //   PTString <-> PTBlobKey
 //   PTString <-> PTBytes
+//   PTXXX <-> PTXXX (i.e. identity)
+//   PTInt <-> PTTime
 //   PTNull <-> Anything
 func (p *Property) Project(to PropertyType) (interface{}, error) {
+	pt, v := p.propType, p.value
 	switch {
-	case to == p.propType:
-		return p.value, nil
-
-	case to == PTInt && p.propType == PTTime:
-		t := p.value.(time.Time)
-		v := uint64(t.Unix())*1e6 + uint64(t.Nanosecond()/1e3)
-		return int64(v), nil
-
-	case to == PTTime && p.propType == PTInt:
-		v := p.value.(int64)
-		t := time.Unix(int64(v/1e6), int64((v%1e6)*1e3))
-		if t.IsZero() {
-			return time.Time{}, nil
+	case pt == PTBytes || pt == PTString || pt == PTBlobKey:

[iannucci, 2015/12/31 23:22:34]

if you switch pt, this could be `case PTBytes, PTString, PTBlobKey`

[dnj, 2016/01/01 17:43:30]

Done.

+		v := v.(byteSequence)
+		switch to {
+		case PTBytes:
+			return v.Bytes(), nil
+		case PTString:
+			return v.String(), nil
+		case PTBlobKey:
+			return blobstore.Key(v.String()), nil
 		}
-		return t.UTC(), nil
 
-	case to == PTString && p.propType == PTBytes:
-		return string(p.value.([]byte)), nil
-
-	case to == PTString && p.propType == PTBlobKey:
-		return string(p.value.(blobstore.Key)), nil
+	case to == pt:
+		// Convert between internal types and external representations.
+		switch pt {
+		case PTTime:
+			return IntToTime(v.(int64)), nil
 
-	case to == PTBytes && p.propType == PTString:
-		return []byte(p.value.(string)), nil
+		default:
+			return v, nil
+		}
 
-	case to == PTBlobKey && p.propType == PTString:
-		return blobstore.Key(p.value.(string)), nil
+	case to == PTInt && pt == PTTime:
+		return v, nil
+	case to == PTTime && pt == PTInt:
+		return IntToTime(v.(int64)), nil
 
 	case to == PTNull:
 		return nil, nil
 
-	case p.propType == PTNull:
+	case pt == PTNull:
 		switch to {
 		case PTInt:
 			return int64(0), nil
@@ -429,29 +489,62 @@ func (p *Property) Project(to PropertyType) (interface{}, error) {
 		case PTBlobKey:
 			return blobstore.Key(""), nil
 		}
-		fallthrough
-	default:
-		return nil, fmt.Errorf("unable to project %s to %s", p.propType, to)
 	}
+	return nil, fmt.Errorf("unable to project %s to %s", pt, to)
 }
 
-func cmpVals(a, b interface{}, t PropertyType) int {
-	cmpFloat := func(a, b float64) int {
-		if a == b {
-			return 0
-		}
-		if a > b {
-			return 1
-		}
+func cmpFloat(a, b float64) int {
+	if a == b {
+		return 0
+	}
+	if a > b {
+		return 1
+	}
+	return -1
+}
+
+// Less returns true iff p would sort before other.
+//
+// This uses datastore's index rules for sorting (e.g.
+// []byte("hello") == "hello")
+func (p *Property) Less(other *Property) bool {
+	return p.Compare(other) < 0
+}
+
+// Equal returns true iff p and other have identical index representations.
+//
+// This uses datastore's index rules for sorting (e.g.
+// []byte("hello") == "hello")
+func (p *Property) Equal(other *Property) bool {
+	return p.Compare(other) == 0
+}
+
+// Compare compares this Property to another, returning a trinary value
+// indicating where it would sort relative to the other in datastore.
+//
+// It returns:
+//	<0 if the Property would sort before `other`.
+//	>0 if the Property would after before `other`.
+//	0 if the Property equals `other`.
+func (p *Property) Compare(other *Property) int {
+	if p.indexSetting && !other.indexSetting {
+		return 1
+	} else if !p.indexSetting && other.indexSetting {
 		return -1
 	}
 
-	switch t {
+	at, av := p.ForIndex()
+	bt, bv := other.ForIndex()
+	if cmp := int(at) - int(bt); cmp != 0 {
+		return cmp
+	}
+
+	switch t := at; t {
 	case PTNull:
 		return 0
 
 	case PTBool:
-		a, b := a.(bool), b.(bool)
+		a, b := av.(bool), bv.(bool)
 		if a == b {
 			return 0
 		}
@@ -461,7 +554,7 @@ func cmpVals(a, b interface{}, t PropertyType) int {
 		return -1
 
 	case PTInt:
-		a, b := a.(int64), b.(int64)
+		a, b := av.(int64), bv.(int64)
 		if a == b {
 			return 0
 		}
@@ -471,20 +564,13 @@ func cmpVals(a, b interface{}, t PropertyType) int {
 		return -1
 
 	case PTString:
-		a, b := a.(string), b.(string)
-		if a == b {
-			return 0
-		}
-		if a > b {
-			return 1
-		}
-		return -1
+		return cmpByteSequence(p.value.(byteSequence), other.value.(byteSequence))
 
 	case PTFloat:
-		return cmpFloat(a.(float64), b.(float64))
+		return cmpFloat(av.(float64), bv.(float64))
 
 	case PTGeoPoint:
-		a, b := a.(GeoPoint), b.(GeoPoint)
+		a, b := av.(GeoPoint), bv.(GeoPoint)
 		cmp := cmpFloat(a.Lat, b.Lat)
 		if cmp != 0 {
 			return cmp
@@ -492,7 +578,7 @@ func cmpVals(a, b interface{}, t PropertyType) int {
 		return cmpFloat(a.Lng, b.Lng)
 
 	case PTKey:
-		a, b := a.(*Key), b.(*Key)
+		a, b := av.(*Key), bv.(*Key)
 		if a.Equal(b) {
 			return 0
 		}
@@ -506,39 +592,6 @@ func cmpVals(a, b interface{}, t PropertyType) int {
 	}
 }
 
-// Less returns true iff p would sort before other.
-//
-// This uses datastore's index rules for sorting (e.g.
-// []byte("hello") == "hello")
-func (p *Property) Less(other *Property) bool {
-	if p.indexSetting && !other.indexSetting {
-		return true
-	} else if !p.indexSetting && other.indexSetting {
-		return false
-	}
-	a, b := p.ForIndex(), other.ForIndex()
-	cmp := int(a.propType) - int(b.propType)
-	if cmp < 0 {
-		return true
-	} else if cmp > 0 {
-		return false
-	}
-	return cmpVals(a.value, b.value, a.propType) < 0
-}
-
-// Equal returns true iff p and other have identical index representations.
-//
-// This uses datastore's index rules for sorting (e.g.
-// []byte("hello") == "hello")
-func (p *Property) Equal(other *Property) bool {
-	ret := p.indexSetting == other.indexSetting
-	if ret {
-		a, b := p.ForIndex(), other.ForIndex()
-		ret = a.propType == b.propType && cmpVals(a.value, b.value, a.propType) == 0
-	}
-	return ret
-}
-
 // GQL returns a correctly formatted Cloud Datastore GQL literal which
 // is valid for a comparison value in the `WHERE` clause.
 //
@@ -548,34 +601,35 @@ func (p *Property) Equal(other *Property) bool {
 // 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()
 	switch p.propType {
 	case PTNull:
 		return "NULL"
 
 	case PTInt, PTFloat, PTBool:
-		return fmt.Sprint(p.value)
+		return fmt.Sprint(v)
 
 	case PTString:
-		return gqlQuoteString(p.value.(string))
+		return gqlQuoteString(v.(string))
 
 	case PTBytes:
 		return fmt.Sprintf("BLOB(%q)",
-			base64.URLEncoding.EncodeToString(p.value.([]byte)))
+			base64.URLEncoding.EncodeToString(v.([]byte)))
 
 	case PTBlobKey:
 		return fmt.Sprintf("BLOBKEY(%s)", gqlQuoteString(
-			string(p.value.(blobstore.Key))))
+			string(v.(blobstore.Key))))
 
 	case PTKey:
-		return p.value.(*Key).GQL()
+		return v.(*Key).GQL()
 
 	case PTTime:
-		return fmt.Sprintf("DATETIME(%s)", p.value.(time.Time).Format(time.RFC3339Nano))
+		return fmt.Sprintf("DATETIME(%s)", v.(time.Time).Format(time.RFC3339Nano))
 
 	case PTGeoPoint:
 		// note that cloud SQL doesn't support this yet, but take a good guess at
 		// it.
-		v := p.value.(GeoPoint)
+		v := v.(GeoPoint)
 		return fmt.Sprintf("GEOPOINT(%v, %v)", v.Lat, v.Lng)
 	}
 	panic(fmt.Errorf("bad type: %s", p.propType))
@@ -604,13 +658,13 @@ func (p *Property) EstimateSize() int64 {
 	case PTGeoPoint:
 		return 1 + (8 * 2)
 	case PTString:
-		return 1 + int64(len(p.value.(string)))
+		return 1 + int64(len(p.Value().(string)))
 	case PTBlobKey:
-		return 1 + int64(len(p.value.(blobstore.Key)))
+		return 1 + int64(len(p.Value().(blobstore.Key)))
 	case PTBytes:
-		return 1 + int64(len(p.value.([]byte)))
+		return 1 + int64(len(p.Value().([]byte)))
 	case PTKey:
-		return 1 + p.value.(*Key).EstimateSize()
+		return 1 + p.Value().(*Key).EstimateSize()
 	}
 	panic(fmt.Errorf("Unknown property type: %s", p.Type().String()))
 }
@@ -828,3 +882,73 @@ func GetMetaDefault(getter MetaGetter, key string, dflt interface{}) interface{}
 	}
 	return cur
 }
+
+// byteSequence is a generic interface for an object that can be represented as
+// a sequence of bytes. Its implementations are used internally by Property to
+// enable zero-copy conversion and comparisons between byte sequence types.
+type byteSequence interface {
+	Len() int
+	Get(int) byte
+	Value() interface{}
+	String() string
+	Bytes() []byte
+	FastCmp(o byteSequence) (int, bool)
+}
+
+func cmpByteSequence(a, b byteSequence) int {
+	if v, ok := a.FastCmp(b); ok {
+		return v
+	}
+
+	ld := a.Len() - b.Len()
+	if ld < 0 {
+		ld = -ld
+	}
+	for i := 0; i < ld; i++ {
+		av, bv := a.Get(i), b.Get(i)
+		switch {
+		case av < bv:
+			return -1
+		case av > bv:
+			return 1
+		}
+	}
+
+	return ld
+}
+
+// bytesByteSequence is a byteSequence implementation for a byte slice.
+type bytesByteSequence []byte
+
+func (s bytesByteSequence) Len() int           { return len(s) }
+func (s bytesByteSequence) Get(i int) byte     { return s[i] }
+func (s bytesByteSequence) Value() interface{} { return []byte(s) }
+func (s bytesByteSequence) String() string     { return string(s) }
+func (s bytesByteSequence) Bytes() []byte      { return []byte(s) }
+func (s bytesByteSequence) FastCmp(o byteSequence) (int, bool) {
+	if t, ok := o.(bytesByteSequence); ok {
+		return bytes.Compare([]byte(s), []byte(t)), true
+	}
+	return 0, false
+}
+
+// stringByteSequence is a byteSequence implementation for a string.
+type stringByteSequence string
+
+func (s stringByteSequence) Len() int           { return len(s) }
+func (s stringByteSequence) Get(i int) byte     { return s[i] }
+func (s stringByteSequence) Value() interface{} { return string(s) }
+func (s stringByteSequence) String() string     { return string(s) }
+func (s stringByteSequence) Bytes() []byte      { return []byte(s) }
+func (s stringByteSequence) FastCmp(o byteSequence) (int, bool) {
+	if t, ok := o.(stringByteSequence); ok {
+		if string(s) == string(t) {
+			return 0, true
+		}
+		if string(s) < string(t) {
+			return -1, true
+		}
+		return 0, true
+	}
+	return 0, false
+}