Refactor keys and queries in datastore service and implementation.

service/datastore/properties.go

Index: service/datastore/properties.go
diff --git a/service/datastore/properties.go b/service/datastore/properties.go
index ab0dfc1714f9dbb0dc703c6b0584e4dbf65dabf7..11339da18948babeb7005213f479189689432581 100644
--- a/service/datastore/properties.go
+++ b/service/datastore/properties.go
@@ -5,6 +5,8 @@
 package datastore
 
 import (
+	"bytes"
+	"encoding/base64"
 	"errors"
 	"fmt"
 	"math"
@@ -39,38 +41,6 @@ func (i IndexSetting) String() string {
 	return "ShouldIndex"
 }
 
-// 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{}
-	indexSetting IndexSetting
-	propType     PropertyType
-}
-
-// MkProperty makes a new indexed* Property 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(..., ShouldIndex) instead.
-//
-// *indexed if val is not an unindexable type like []byte.
-func MkProperty(val interface{}) Property {
-	ret := Property{}
-	if err := ret.SetValue(val, ShouldIndex); err != nil {
-		panic(err)
-	}
-	return ret
-}
-
-// MkPropertyNI makes a new Property (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(..., NoIndex) instead.
-func MkPropertyNI(val interface{}) Property {
-	ret := Property{}
-	if err := ret.SetValue(val, NoIndex); err != nil {
-		panic(err)
-	}
-	return ret
-}
-
 // PropertyConverter may be implemented by the pointer-to a struct field which
 // is serialized by the struct PropertyLoadSaver from GetPLS. Its ToProperty
 // will be called on save, and it's FromProperty will be called on load (from
@@ -176,7 +146,7 @@ const (
 	// PTGeoPoint is a Projection-query type.
 	PTGeoPoint
 
-	// PTKey represents a Key object.
+	// PTKey represents a *Key object.
 	//
 	// PTKey is a Projection-query type.
 	PTKey
@@ -225,6 +195,38 @@ func (t PropertyType) String() string {
 	}
 }
 
+// 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{}
+	indexSetting IndexSetting
+	propType     PropertyType
+}
+
+// MkProperty makes a new indexed* Property 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(..., ShouldIndex) instead.
+//
+// *indexed if val is not an unindexable type like []byte.
+func MkProperty(val interface{}) Property {

[dnj, 2015/09/18 16:47:58]

Now that Property is a struct, this should probably return a *Property (same for
the rest).

[iannucci, 2015/09/18 22:25:48]

It has always been a struct. I chose not to pass *Property because in the
current model you can never have an invalid property (by construction). If I
change that, I'd rather do it in a different CL as it would involve putting a
lot of `if p == nil { return error}` crap all over the place. I'm inclined not
to do it at all.

+	ret := Property{}
+	if err := ret.SetValue(val, ShouldIndex); err != nil {
+		panic(err)
+	}
+	return ret
+}
+
+// MkPropertyNI makes a new Property (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(..., NoIndex) instead.
+func MkPropertyNI(val interface{}) Property {
+	ret := Property{}
+	if err := ret.SetValue(val, NoIndex); err != nil {
+		panic(err)
+	}
+	return ret
+}
+
 // PropertyTypeOf returns the PT* type of the given Property-compatible
 // value v. If checkValid is true, this method will also ensure that time.Time
 // and GeoPoint have valid values.
@@ -244,7 +246,7 @@ func PropertyTypeOf(v interface{}, checkValid bool) (PropertyType, error) {
 		return PTBlobKey, nil
 	case string:
 		return PTString, nil
-	case Key:
+	case *Key:
 		// TODO(riannucci): Check key for validity in its own namespace?
 		return PTKey, nil
 	case time.Time:
@@ -338,7 +340,7 @@ func (p *Property) Type() PropertyType { return p.propType }
 //	- blobstore.Key
 //    (only the first 1500 bytes is indexable)
 //	- float64
-//	- Key
+//	- *Key
 //	- GeoPoint
 // This set is smaller than the set of valid struct field types that the
 // datastore can load and save. A Property Value cannot be a slice (apart
@@ -450,6 +452,153 @@ func (p *Property) Project(to PropertyType) (interface{}, error) {
 	}
 }
 
+func cmpVals(a, b interface{}, t PropertyType) int {

[iannucci, 2015/09/18 04:31:53]

does anyone know how to write this thing better? ugh...

[dnj, 2015/09/18 16:47:58]

*shrug* inner switch statements could make it a little nicer, I suppose.

+	switch t {
+	case PTNull:
+		return 0
+
+	case PTBool:
+		a, b := a.(bool), b.(bool)
+		if a == b {
+			return 0
+		}
+		if a && !b {
+			return 1
+		}
+		return -1
+
+	case PTInt:
+		a, b := a.(int64), b.(int64)
+		if a == b {
+			return 0
+		}
+		if a > b {
+			return 1
+		}
+		return -1
+
+	case PTString:
+		a, b := a.(string), b.(string)
+		if a == b {

[dnj, 2015/09/18 16:47:58]

Use strings.Compare here.

+			return 0
+		}
+		if a > b {
+			return 1
+		}
+		return -1
+
+	case PTFloat:
+		a, b := a.(float64), b.(float64)
+		if a == b {
+			return 0
+		}
+		if a > b {
+			return 1
+		}
+		return -1
+
+	case PTGeoPoint:
+		a, b := a.(GeoPoint), b.(GeoPoint)
+		cmp := cmpVals(a.Lat, b.Lat, PTFloat)

[dnj, 2015/09/18 16:47:58]

This seems unnecessarily reflective.

[iannucci, 2015/09/18 22:25:48]

functionized

+		if cmp != 0 {
+			return cmp
+		}
+		return cmpVals(a.Lng, b.Lng, PTFloat)
+
+	case PTKey:
+		a, b := a.(*Key), b.(*Key)
+		if a.Equal(b) {
+			return 0
+		}
+		if b.Less(a) {
+			return 1
+		}
+		return -1
+
+	default:
+		panic(fmt.Errorf("uncomparable type: %s", t))
+	}
+}
+
+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
+}
+
+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
+}
+
+func (p *Property) GQL() string {
+	switch p.propType {
+	case PTNull:
+		return "NULL"
+
+	case PTInt, PTFloat, PTBool:
+		return fmt.Sprint(p.value)
+
+	case PTString:
+		return gqlQuoteString(p.value.(string))
+
+	case PTBytes:
+		return fmt.Sprintf("BLOB(%q)",
+			base64.URLEncoding.EncodeToString(p.value.([]byte)))
+
+	case PTBlobKey:
+		return fmt.Sprintf("BLOBKEY(%s)", gqlQuoteString(
+			string(p.value.(blobstore.Key))))
+
+	case PTKey:
+		aid, ns, toks := p.value.(*Key).Split()
+		ret := &bytes.Buffer{}
+		fmt.Fprintf(ret, "KEY(DATASET(%s)", gqlQuoteString(aid))
+		if ns != "" {
+			fmt.Fprintf(ret, ", NAMESPACE(%s)", gqlQuoteString(ns))
+		}
+		for _, t := range toks {
+			if t.IntID != 0 {
+				fmt.Fprintf(ret, ", %s, %d", gqlQuoteString(t.Kind), t.IntID)
+			} else {
+				fmt.Fprintf(ret, ", %s, %s", gqlQuoteString(t.Kind), gqlQuoteString(t.StringID))
+			}
+		}
+		ret.WriteString(")")
+		return ret.String()
+
+	case PTTime:
+		return fmt.Sprintf("DATETIME(%s)", p.value.(time.Time).Format("2006-01-02T15:04:05.000000Z"))

[dnj, 2015/09/18 16:47:58]

time.RFC3339?

[iannucci, 2015/09/18 22:25:48]

Nano, and thanks

+
+	case PTGeoPoint:
+		// note that cloud SQL doesn't support this yet, but take a good guess at
+		// it.
+		v := p.value.(GeoPoint)
+		return fmt.Sprintf("GEOPOINT(%v, %v)", v.Lat, v.Lng)
+	}
+	panic(fmt.Errorf("bad type: %s", p.propType))
+}
+
+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]) }
+
 // MetaGetter is a subinterface of PropertyLoadSaver, but is also used to
 // abstract the meta argument for RawInterface.GetMulti.
 type MetaGetter interface {
@@ -460,7 +609,7 @@ type MetaGetter interface {
 	//   int64  - may have default (ascii encoded base-10)
 	//   string - may have default
 	//   Toggle - MUST have default ("true" or "false")
-	//   Key    - NO default allowed
+	//   *Key    - NO default allowed
 	//
 	// Struct fields of type Toggle (which is an Auto/On/Off) require you to
 	// specify a value of 'true' or 'false' for the default value of the struct