Fix error message from KeyForObj when passing an invalid struct.

service/datastore/pls_impl.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.
 
 // HEAVILY adapted from github.com/golang/appengine/datastore
 
 package datastore
 
 import (
         "fmt"
@@ skipping 35 matching lines @@
 var _ PropertyLoadSaver = (*structPLS)(nil)
 
 // typeMismatchReason returns a string explaining why the property p could not
 // be stored in an entity field of type v.Type().
 func typeMismatchReason(val interface{}, v reflect.Value) string {
         entityType := reflect.TypeOf(val)
         return fmt.Sprintf("type mismatch: %s versus %v", entityType, v.Type())
 }
 
 func (p *structPLS) Load(propMap PropertyMap) error {
-        if err := p.Problem(); err != nil {
-                return err
+        if p.c.problem != nil {
+                panic(p.c.problem)
         }
 
         convFailures := errors.MultiError(nil)
 
         t := reflect.Type(nil)
         for name, props := range propMap {
                 multiple := len(props) > 1
                 for i, prop := range props {
                         if reason := loadInner(p.c, p.o, i, name, prop, multiple); reason != "" {
                                 if t == nil {
@@ skipping 130 matching lines @@
                 }
                 set(pVal)
         }
         if slice.IsValid() {
                 slice.Set(reflect.Append(slice, v))
         }
         return ""
 }
 
 func (p *structPLS) Save(withMeta bool) (PropertyMap, error) {
-        if err := p.Problem(); err != nil {
-                return nil, err
+        if p.c.problem != nil {
+                panic(p.c.problem)
         }
 
         ret := PropertyMap(nil)
         if withMeta {
                 ret = getMGS(p.o.Addr().Interface()).GetAllMeta()
         } else {
                 ret = make(PropertyMap, len(p.c.byName))
         }
         if _, err := p.save(ret, "", ShouldIndex); err != nil {
                 return nil, err
         }
         return ret, nil
 }
 
 func (p *structPLS) getDefaultKind() string {
         if !p.o.IsValid() {
                 return ""
         }
         return p.o.Type().Name()
 }
 
 func (p *structPLS) save(propMap PropertyMap, prefix string, is IndexSetting) (idxCount int, err error) {
-        if err = p.Problem(); err != nil {
-                return
+        if p.c.problem != nil {
+                panic(p.c.problem)
         }
 
         saveProp := func(name string, si IndexSetting, v reflect.Value, st *structTag) (err error) {
                 if st.substructCodec != nil {
                         count, err := (&structPLS{v, st.substructCodec}).save(propMap, name, si)
                         if err == nil {
                                 idxCount += count
                                 if idxCount > maxIndexedProperties {
                                         err = errors.New("gae: too many indexed properties")
                                 }
@@ skipping 41 matching lines @@
                         }
                 } else {
                         if err = saveProp(name, is1, v, &st); err != nil {
                                 return
                         }
                 }
         }
         return
 }
 
-func (p *structPLS) GetMeta(key string) (interface{}, error) {
-        if err := p.Problem(); err != nil {
-                return nil, err
+func (p *structPLS) GetMeta(key string) (interface{}, bool) {
+        if p.c.problem != nil {
+                panic(p.c.problem)
         }
 
         if idx, ok := p.c.byMeta[key]; ok {
                 if val, ok := p.getMetaFor(idx); ok {
-                        return val, nil
+                        return val, true
                 }
         } else if key == "kind" {
-                return p.getDefaultKind(), nil
+                return p.getDefaultKind(), true
         }
-        return nil, ErrMetaFieldUnset
+        return nil, false
 }
 
 func (p *structPLS) getMetaFor(idx int) (interface{}, bool) {
         st := p.c.byIndex[idx]
         val := st.metaVal
         if st.canSet {
                 f := p.o.Field(idx)
                 if st.convert {
                         prop, err := f.Addr().Interface().(PropertyConverter).ToProperty()
                         if err != nil {
@@ skipping 27 matching lines @@
                 }
         }
         if needKind {
                 if _, ok := p.c.byMeta["kind"]; !ok {
                         ret["$kind"] = []Property{MkPropertyNI(p.getDefaultKind())}
                 }
         }
         return ret
 }
 
-func (p *structPLS) GetMetaDefault(key string, def interface{}) interface{} {
-        return GetMetaDefaultImpl(p.GetMeta, key, def)
-}
+func (p *structPLS) SetMeta(key string, val interface{}) bool {
+        if p.c.problem != nil {
+                panic(p.c.problem)
+        }
 
-func (p *structPLS) SetMeta(key string, val interface{}) (err error) {
-        if err = p.Problem(); err != nil {
-                return
-        }
         idx, ok := p.c.byMeta[key]
         if !ok {
-                return ErrMetaFieldUnset
+                return false
         }
         st := p.c.byIndex[idx]
         if !st.canSet {
-                return fmt.Errorf("gae/helper: cannot set meta %q: unexported field", key)
+                return false
         }
         if st.convert {
-                return p.o.Field(idx).Addr().Interface().(PropertyConverter).FromProperty(
+                err := p.o.Field(idx).Addr().Interface().(PropertyConverter).FromProperty(
                         MkPropertyNI(val))
+                return err == nil
         }
 
         // setting a BoolField
         if b, ok := val.(bool); ok {
                 if b {
                         val = On
                 } else {
                         val = Off
                 }
         }
         f := p.o.Field(idx)
         if val == nil {
                 f.Set(reflect.Zero(f.Type()))
         } else {
                 value := reflect.ValueOf(val)
                 f.Set(value.Convert(f.Type()))
         }
-        return nil
+        return true
 }
 
-func (p *structPLS) Problem() error { return p.c.problem }
-
 var (
         // The RWMutex is chosen intentionally, as the majority of access to the
         // structCodecs map will be in parallel and will be to read an existing codec.
         // There's no reason to serialize goroutines on every
         // gae.Interface.{Get,Put}{,Multi} call.
         structCodecsMutex sync.RWMutex
         structCodecs      = map[reflect.Type]*structCodec{}
 )
 
 // validPropertyName returns whether name consists of one or more valid Go
@@ skipping 66 matching lines @@
                 st.convert = reflect.PtrTo(ft).Implements(typeOfPropertyConverter)
                 switch {
                 case name == "":
                         if !f.Anonymous {
                                 name = f.Name
                         }
                 case name[0] == '$':
                         name = name[1:]
                         if _, ok := c.byMeta[name]; ok {
                                 c.problem = me("meta field %q set multiple times", "$"+name)
                                 return

[dnj, 2015/12/12 02:47:50]

I think you should panic when the problem is identified, such that it is
impossible to create a structPLS from an invalid structure. It seems unintuitive
to me to have the panic be delayed until the structPLS is actually used.

[iannucci, 2015/12/12 03:52:22]

Ok, now we panic harder (in getCodec).

                         }
                         c.byMeta[name] = i
                         if !st.convert {
                                 mv, err := convertMeta(opts, ft)
                                 if err != nil {
                                         c.problem = me("meta field %q has bad type: %s", "$"+name, err)
                                         return
                                 }
                                 st.metaVal = mv
                         }
@@ skipping 113 matching lines @@
                 switch val {
                 case "on", "On", "true":
                         return true, nil
                 case "off", "Off", "false":
                         return false, nil
                 }
                 return nil, fmt.Errorf("Toggle field has bad/missing default, got %q", val)
         }
         return nil, fmt.Errorf("helper: meta field with bad type/value %s/%q", t, val)
 }