Refactor keys and queries in datastore service and implementation.

service/datastore/properties.go

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package datastore
 
 import (
+        "bytes"
+        "encoding/base64"
         "errors"
         "fmt"
         "math"
         "reflect"
         "time"
 
         "github.com/luci/gae/service/blobstore"
 )
 
 var (
@@ skipping 14 matching lines @@
         NoIndex     IndexSetting = true
 )
 
 func (i IndexSetting) String() string {
         if i {
                 return "NoIndex"
         }
         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
 // datastore). The method may do arbitrary computation, and if it encounters an
 // error, may return it.  This error will be a fatal error (as defined by
 // PropertyLoadSaver) for the struct conversion.
 //
 // Example:
 //   type Complex complex
 //   func (c *Complex) ToProperty() (ret Property, err error) {
@@ skipping 85 matching lines @@
         PTString
 
         // PTFloat is always a float64.
         //
         // This is a Projection-query type.
         PTFloat
 
         // PTGeoPoint is a Projection-query type.
         PTGeoPoint
 
-        // PTKey represents a Key object.
+        // PTKey represents a *Key object.
         //
         // PTKey is a Projection-query type.
         PTKey
 
         // PTBlobKey represents a blobstore.Key
         PTBlobKey
 
         // NOTE: THIS MUST BE LAST VALUE FOR THE init() ASSERTION BELOW TO WORK.
         PTUnknown
 )
@@ skipping 28 matching lines @@
                 return "PTGeoPoint"
         case PTKey:
                 return "PTKey"
         case PTBlobKey:
                 return "PTBlobKey"
         default:
                 return fmt.Sprintf("PTUnknown(%02x)", byte(t))
         }
 }
 
+// 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.
 func PropertyTypeOf(v interface{}, checkValid bool) (PropertyType, error) {
         switch x := v.(type) {
         case nil:
                 return PTNull, nil
         case int64:
                 return PTInt, nil
         case float64:
                 return PTFloat, nil
         case bool:
                 return PTBool, nil
         case []byte:
                 return PTBytes, nil
         case blobstore.Key:
                 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:
                 err := error(nil)
                 if checkValid && (x.Before(minTime) || x.After(maxTime)) {
                         err = errors.New("time value out of range")
                 }
                 if checkValid && !timeLocationIsUTC(x.Location()) {
                         err = fmt.Errorf("time value has wrong Location: %v", x.Location())
                 }
@@ skipping 73 matching lines @@
 //      - int64
 //      - time.Time
 //      - bool
 //      - string
 //    (only the first 1500 bytes is indexable)
 //      - []byte
 //    (only the first 1500 bytes is indexable)
 //      - 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
 // from []byte); use multiple Properties instead. Also, a Value's type
 // must be explicitly on the list above; it is not sufficient for the
 // underlying type to be on that list. For example, a Value of "type
 // myInt64 int64" is invalid. Smaller-width integers and floats are also
 // invalid. Again, this is more restrictive than the set of valid struct
 // field types.
 //
@@ skipping 91 matching lines @@
                         return nil, nil
                 case PTBlobKey:
                         return blobstore.Key(""), nil
                 }
                 fallthrough
         default:
                 return nil, fmt.Errorf("unable to project %s to %s", p.propType, to)
         }
 }
 
+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 {
         // GetMeta will get information about the field which has the struct tag in
         // the form of `gae:"$<key>[,<default>]?"`.
         //
         // Supported metadata types are:
         //   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
         // tag, and GetMeta will return the combined value as a regular boolean true
         // or false value.
         // Example:
         //   type MyStruct struct {
         //     CoolField int64 `gae:"$id,1"`
         //   }
         //   val, err := helper.GetPLS(&MyStruct{}).GetMeta("id")
@@ skipping 150 matching lines @@
         dflt = UpconvertUnderlyingType(dflt)
         cur, err := gm(key)
         if err != nil {
                 return dflt
         }
         if dflt != nil && reflect.TypeOf(cur) != reflect.TypeOf(dflt) {
                 return dflt
         }
         return cur
 }