Fix error message from KeyForObj when passing an invalid struct.

service/datastore/pls.go

Index: service/datastore/pls.go
diff --git a/service/datastore/pls.go b/service/datastore/pls.go
index f1f54e683fc975bd293f31d8550c27708cec67d8..d84d32d774980c5d04ada8499da0772bce0c5970 100644
--- a/service/datastore/pls.go
+++ b/service/datastore/pls.go
@@ -5,6 +5,7 @@
 package datastore
 
 import (
+	"fmt"
 	"reflect"
 )
 
@@ -17,11 +18,10 @@ import (
 // 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
@@ -160,11 +160,14 @@ func GetPLS(obj interface{}) interface {
 	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())
@@ -182,11 +185,13 @@ 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
 }