Fix error message from KeyForObj when passing an invalid struct.

service/datastore/pls.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 (
+        "fmt"
         "reflect"
 )
 
 // GetPLS resolves obj into default struct PropertyLoadSaver and
 // MetaGetterSetter implementation.
 //
 // obj must be a non-nil pointer to a struct of some sort.
 //
 // By default, exported fields will be serialized to/from the datastore. If the
 // field is not exported, it will be skipped by the serialization routines.
 //
 // If a field is of a non-supported type (see Property for the list of supported
-// property types), the resulting PropertyLoadSaver will have a non-nil
-// Problem(). Other problems include duplicate field names (due to tagging),
-// recursively defined structs, nested structures with multiple slices (e.g.
-// slices of slices, either directly `[][]type` or indirectly `[]Embedded` where
-// Embedded contains a slice.)
+// property types), this function will panic. Other problems include duplicate
+// field names (due to tagging), recursively defined structs, nested structures
+// with multiple slices (e.g.  slices of slices, either directly `[][]type` or
+// indirectly `[]Embedded` where Embedded contains a slice.)
 //
 // GetPLS supports the following struct tag syntax:
 //   `gae:"fieldName[,noindex]"` -- an alternate fieldname for an exportable
 //      field.  When the struct is serialized or deserialized, fieldName will be
 //      associated with the struct field instead of the field's Go name. This is
 //      useful when writing Go code which interfaces with appengine code written
 //      in other languages (like python) which use lowercase as their default
 //      datastore field names.
 //
 //      A fieldName of "-" means that gae will ignore the field for all
@@ skipping 118 matching lines @@
 //   }
 //
 //   type Person struct {
 //     Name `gae:"$id"`
 //   }
 func GetPLS(obj interface{}) interface {
         PropertyLoadSaver
         MetaGetterSetter
 } {
         v := reflect.ValueOf(obj)
+        if !v.IsValid() {
+                panic(fmt.Errorf("cannot GetPLS(%T): failed to reflect", obj))
+        }
         if v.Kind() != reflect.Ptr || v.Elem().Kind() != reflect.Struct {
-                return &structPLS{c: &structCodec{problem: ErrInvalidEntityType}}
+                panic(fmt.Errorf("cannot GetPLS(%T): not a pointer-to-struct", obj))
         }
         if v.IsNil() {
-                return &structPLS{c: &structCodec{problem: ErrInvalidEntityType}}
+                panic(fmt.Errorf("cannot GetPLS(%T): pointer-to-struct is nil", obj))
         }
         v = v.Elem()
         c := getCodec(v.Type())
         return &structPLS{v, c}
 }
 
 func getMGS(obj interface{}) MetaGetterSetter {
         if mgs, ok := obj.(MetaGetterSetter); ok {
                 return mgs
         }
         return GetPLS(obj)
 }
 
 func getCodec(structType reflect.Type) *structCodec {
         structCodecsMutex.RLock()
         c, ok := structCodecs[structType]
         structCodecsMutex.RUnlock()
-        if ok {
-                return c
+        if !ok {
+                structCodecsMutex.Lock()
+                defer structCodecsMutex.Unlock()
+                c = getStructCodecLocked(structType)
         }
-
-        structCodecsMutex.Lock()
-        defer structCodecsMutex.Unlock()
-        return getStructCodecLocked(structType)
+        if c.problem != nil {

[dnj, 2015/12/12 04:04:53]

IMO: While I find the effort to avoid recomputing an invalid codec noble, I
think the complication of retaining "problem" is more complicated than
necessary. The only use case is when a user computes the struct in a
"panic/recover" loop, else they'd just be dead, and those folk have more
problems than some reflection inefficiency. I think you should remove the
"problem" member and just panic in "getStructCodecLocked" if the struct is bad.

I'm okay either way, just wanted to put this out there :)

+                panic(c.problem)
+        }
+        return c
 }