fields: tag parser can return error

Allow the tag parser to return an error, which
is propagated out of the Cache.Fields method.

Change-Id: I7330f7ec29704fd283d8bfcdd854da9e6e30d446
Reviewed-on: https://code-review.googlesource.com/9723
Reviewed-by: Jun Mukai <mukai@google.com>

bigquery/params.go

@@ -137,7 +137,11 @@ func paramType(t reflect.Type) (*bq.QueryParameterType, error) {
 
 	case reflect.Struct:
 		var fts []*bq.QueryParameterTypeStructTypes
-		for _, f := range fieldCache.Fields(t) {
+		fields, err := fieldCache.Fields(t)
+		if err != nil {
+			return nil, err
+		}
+		for _, f := range fields {
 			pt, err := paramType(f.Type)
 			if err != nil {
 				return nil, err
@@ -209,7 +213,10 @@ func paramValue(v reflect.Value) (bq.QueryParameterValue, error) {
 		fallthrough
 
 	case reflect.Struct:
-		fields := fieldCache.Fields(t)
+		fields, err := fieldCache.Fields(t)
+		if err != nil {
+			return bq.QueryParameterValue{}, err
+		}
 		res.StructValues = map[string]bq.QueryParameterValue{}
 		for _, f := range fields {
 			fv := v.FieldByIndex(f.Index)

bigquery/schema.go

@@ -196,7 +196,11 @@ func inferFieldSchema(rt reflect.Type) (*FieldSchema, error) {
 // inferFields extracts all exported field types from struct type.
 func inferFields(rt reflect.Type) (Schema, error) {
 	var s Schema
-	for _, field := range fieldCache.Fields(rt) {
+	fields, err := fieldCache.Fields(rt)
+	if err != nil {
+		return nil, err
+	}
+	for _, field := range fields {
 		f, err := inferFieldSchema(field.Type)
 		if err != nil {
 			return nil, err

bigquery/value.go

@@ -146,7 +146,10 @@ func (sl *structLoader) set(structp interface{}, schema Schema) error {
 // value of structType to the contents of a row with schema.
 func compileToOps(structType reflect.Type, schema Schema) ([]structLoaderOp, error) {
 	var ops []structLoaderOp
-	fields := fieldCache.Fields(structType)
+	fields, err := fieldCache.Fields(structType)
+	if err != nil {
+		return nil, err
+	}
 	for i, schemaField := range schema {
 		// Look for an exported struct field with the same name as the schema
 		// field, ignoring case (BigQuery column names are case-insensitive,
@@ -388,7 +391,10 @@ func (s structSaver) Save() (row map[string]Value, insertID string, err error) {
 		v = v.Elem()
 		t = t.Elem()
 	}
-	fields := fieldCache.Fields(t)
+	fields, err := fieldCache.Fields(t)
+	if err != nil {
+		return nil, "", err
+	}
 	for _, f := range fields {
 		row[f.Name] = v.FieldByIndex(f.Index).Interface()
 	}

internal/fields/fields.go

@@ -19,7 +19,7 @@
 //
 // First define a function that interprets tags:
 //
-//   func parseTag(st reflect.StructTag) (name string, keep bool, other interface{}) { ... }
+//   func parseTag(st reflect.StructTag) (name string, keep bool, other interface{}, err error) { ... }
 //
 // The function's return values describe whether to ignore the field
 // completely or provide an alternate name, as well as other data from the
@@ -68,24 +68,26 @@ type Field struct {
 	equalFold func(s, t []byte) bool
 }
 
+type ParseTagFunc func(reflect.StructTag) (name string, keep bool, other interface{}, err error)
+
 // A Cache records information about the fields of struct types.
 //
 // A Cache is safe for use by multiple goroutines.
 type Cache struct {
-	parseTag func(reflect.StructTag) (name string, keep bool, other interface{})
+	parseTag ParseTagFunc
 	cache    atomic.Value // map[reflect.Type][]Field
 	mu       sync.Mutex   // used only by writers of cache
 }
 
 // NewCache constructs a Cache. Its argument should be a function that accepts
-// a struct tag and returns three values: an alternative name for the field
+// a struct tag and returns four values: an alternative name for the field
 // extracted from the tag, a boolean saying whether to keep the field or ignore
-// it, and additional data that is stored with the field information to avoid
-// having to parse the tag again.
-func NewCache(parseTag func(reflect.StructTag) (name string, keep bool, other interface{})) *Cache {
+// it, additional data that is stored with the field information to avoid
+// having to parse the tag again, and an error.
+func NewCache(parseTag ParseTagFunc) *Cache {
 	if parseTag == nil {
-		parseTag = func(reflect.StructTag) (string, bool, interface{}) {
-			return "", true, nil
+		parseTag = func(reflect.StructTag) (string, bool, interface{}, error) {
+			return "", true, nil, nil
 		}
 	}
 	return &Cache{parseTag: parseTag}
@@ -120,11 +122,11 @@ type fieldScan struct {
 // If more than one field with the same name exists at the same level of embedding,
 // but exactly one of them is tagged, then the tagged field is reported and the others
 // are ignored.
-func (c *Cache) Fields(t reflect.Type) List {
+func (c *Cache) Fields(t reflect.Type) (List, error) {
 	if t.Kind() != reflect.Struct {
 		panic("fields: Fields of non-struct type")
 	}
-	return List(c.cachedTypeFields(t))
+	return c.cachedTypeFields(t)
 }
 
 // A List is a list of Fields.
@@ -153,37 +155,42 @@ func (l List) MatchBytes(name []byte) *Field {
 	return f
 }
 
+type cacheValue struct {
+	fields List
+	err    error
+}
+
 // cachedTypeFields is like typeFields but uses a cache to avoid repeated work.
 // This code has been copied and modified from
 // https://go.googlesource.com/go/+/go1.7.3/src/encoding/json/encode.go.
-func (c *Cache) cachedTypeFields(t reflect.Type) []Field {
-	mp, _ := c.cache.Load().(map[reflect.Type][]Field)
-	f := mp[t]
-	if f != nil {
-		return f
+func (c *Cache) cachedTypeFields(t reflect.Type) (List, error) {
+	mp, _ := c.cache.Load().(map[reflect.Type]cacheValue)
+	if cv, ok := mp[t]; ok {
+		return cv.fields, cv.err
 	}
 
 	// Compute fields without lock.
 	// Might duplicate effort but won't hold other computations back.
-	f = c.typeFields(t)
-	if f == nil {
-		f = []Field{}
-	}
+	f, err := c.typeFields(t)
+	list := List(f)
 
 	c.mu.Lock()
-	mp, _ = c.cache.Load().(map[reflect.Type][]Field)
-	newM := make(map[reflect.Type][]Field, len(mp)+1)
+	mp, _ = c.cache.Load().(map[reflect.Type]cacheValue)
+	newM := make(map[reflect.Type]cacheValue, len(mp)+1)
 	for k, v := range mp {
 		newM[k] = v
 	}
-	newM[t] = f
+	newM[t] = cacheValue{list, err}
 	c.cache.Store(newM)
 	c.mu.Unlock()
-	return f
+	return list, err
 }
 
-func (c *Cache) typeFields(t reflect.Type) []Field {
-	fields := c.listFields(t)
+func (c *Cache) typeFields(t reflect.Type) ([]Field, error) {
+	fields, err := c.listFields(t)
+	if err != nil {
+		return nil, err
+	}
 	sort.Sort(byName(fields))
 	// Delete all fields that are hidden by the Go rules for embedded fields.
 
@@ -208,10 +215,10 @@ func (c *Cache) typeFields(t reflect.Type) []Field {
 		}
 	}
 	sort.Sort(byIndex(out))
-	return out
+	return out, nil
 }
 
-func (c *Cache) listFields(t reflect.Type) []Field {
+func (c *Cache) listFields(t reflect.Type) ([]Field, error) {
 	// This uses the same condition that the Go language does: there must be a unique instance
 	// of the match at a given depth level. If there are multiple instances of a match at the
 	// same depth, they annihilate each other and inhibit any possible match at a lower level.
@@ -270,7 +277,10 @@ func (c *Cache) listFields(t reflect.Type) []Field {
 				}
 
 				// Examine the tag.
-				tagName, keep, other := c.parseTag(f.Tag)
+				tagName, keep, other, err := c.parseTag(f.Tag)
+				if err != nil {
+					return nil, err
+				}
 				if !keep {
 					continue
 				}
@@ -319,7 +329,7 @@ func (c *Cache) listFields(t reflect.Type) []Field {
 			}
 		}
 	}
-	return fields
+	return fields, nil
 }
 
 func newField(f reflect.StructField, tagName string, other interface{}, index []int, i int) Field {

internal/fields/fields_test.go

@@ -16,6 +16,7 @@ package fields
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"reflect"
 	"strings"
@@ -82,7 +83,10 @@ func tfield(name string, tval interface{}, index ...int) *Field {
 
 func TestFieldsNoTags(t *testing.T) {
 	c := NewCache(nil)
-	got := c.Fields(reflect.TypeOf(S1{}))
+	got, err := c.Fields(reflect.TypeOf(S1{}))
+	if err != nil {
+		t.Fatal(err)
+	}
 	want := []*Field{
 		field("Exported", int(0), 0),
 		field("Shadow", int(0), 2),
@@ -128,7 +132,10 @@ func TestAgainstJSONEncodingNoTags(t *testing.T) {
 	jsonRoundTrip(t, s1, &want)
 	var got S1
 	got.embed2 = &embed2{} // need this because reflection won't create it
-	fields := NewCache(nil).Fields(reflect.TypeOf(got))
+	fields, err := NewCache(nil).Fields(reflect.TypeOf(got))
+	if err != nil {
+		t.Fatal(err)
+	}
 	setFields(fields, &got, s1)
 	if !reflect.DeepEqual(got, want) {
 		t.Errorf("got\n%+v\nwant\n%+v", got, want)
@@ -161,20 +168,23 @@ type tEmbed2 struct {
 	Z int `json:"Dup2"` // same name as tEmbed1.X and both tagged, so ignored
 }
 
-func jsonTagParser(t reflect.StructTag) (name string, keep bool, other interface{}) {
+func jsonTagParser(t reflect.StructTag) (name string, keep bool, other interface{}, err error) {
 	s := t.Get("json")
 	parts := strings.Split(s, ",")
 	if parts[0] == "-" {
-		return "", false, nil
+		return "", false, nil, nil
 	}
 	if len(parts) > 1 {
 		other = parts[1:]
 	}
-	return parts[0], true, other
+	return parts[0], true, other, nil
 }
 
 func TestFieldsWithTags(t *testing.T) {
-	got := NewCache(jsonTagParser).Fields(reflect.TypeOf(S2{}))
+	got, err := NewCache(jsonTagParser).Fields(reflect.TypeOf(S2{}))
+	if err != nil {
+		t.Fatal(err)
+	}
 	want := []*Field{
 		field("NoTag", int(0), 0),
 		tfield("tag", int(0), 1),
@@ -209,7 +219,10 @@ func TestAgainstJSONEncodingWithTags(t *testing.T) {
 	var want S2
 	jsonRoundTrip(t, s2, &want)
 	var got S2
-	fields := NewCache(jsonTagParser).Fields(reflect.TypeOf(got))
+	fields, err := NewCache(jsonTagParser).Fields(reflect.TypeOf(got))
+	if err != nil {
+		t.Fatal(err)
+	}
 	setFields(fields, &got, s2)
 	if !reflect.DeepEqual(got, want) {
 		t.Errorf("got\n%+v\nwant\n%+v", got, want)
@@ -230,7 +243,10 @@ func TestUnexportedAnonymousNonStruct(t *testing.T) {
 		}
 	)
 
-	got := NewCache(jsonTagParser).Fields(reflect.TypeOf(S{}))
+	got, err := NewCache(jsonTagParser).Fields(reflect.TypeOf(S{}))
+	if err != nil {
+		t.Fatal(err)
+	}
 	if len(got) != 0 {
 		t.Errorf("got %d fields, want 0", len(got))
 	}
@@ -246,7 +262,10 @@ func TestUnexportedAnonymousStruct(t *testing.T) {
 			s1 `json:"Y"`
 		}
 	)
-	got := NewCache(jsonTagParser).Fields(reflect.TypeOf(S2{}))
+	got, err := NewCache(jsonTagParser).Fields(reflect.TypeOf(S2{}))
+	if err != nil {
+		t.Fatal(err)
+	}
 	if len(got) != 0 {
 		t.Errorf("got %d fields, want 0", len(got))
 	}
@@ -285,7 +304,10 @@ func TestIgnore(t *testing.T) {
 	type S struct {
 		X int `json:"-"`
 	}
-	got := NewCache(jsonTagParser).Fields(reflect.TypeOf(S{}))
+	got, err := NewCache(jsonTagParser).Fields(reflect.TypeOf(S{}))
+	if err != nil {
+		t.Fatal(err)
+	}
 	if len(got) != 0 {
 		t.Errorf("got %d fields, want 0", len(got))
 	}
@@ -295,7 +317,10 @@ func TestParsedTag(t *testing.T) {
 	type S struct {
 		X int `json:"name,omitempty"`
 	}
-	got := NewCache(jsonTagParser).Fields(reflect.TypeOf(S{}))
+	got, err := NewCache(jsonTagParser).Fields(reflect.TypeOf(S{}))
+	if err != nil {
+		t.Fatal(err)
+	}
 	want := []*Field{
 		{Name: "name", NameFromTag: true, Type: intType,
 			Index: []int{0}, ParsedTag: []string{"omitempty"}},
@@ -368,7 +393,10 @@ type S4 struct {
 }
 
 func TestMatchingField(t *testing.T) {
-	fields := NewCache(jsonTagParser).Fields(reflect.TypeOf(S3{}))
+	fields, err := NewCache(jsonTagParser).Fields(reflect.TypeOf(S3{}))
+	if err != nil {
+		t.Fatal(err)
+	}
 	for _, test := range []struct {
 		name string
 		want *Field
@@ -407,7 +435,10 @@ func TestAgainstJSONMatchingField(t *testing.T) {
 	var want S3
 	jsonRoundTrip(t, s3, &want)
 	v := reflect.ValueOf(want)
-	fields := NewCache(jsonTagParser).Fields(reflect.TypeOf(S3{}))
+	fields, err := NewCache(jsonTagParser).Fields(reflect.TypeOf(S3{}))
+	if err != nil {
+		t.Fatal(err)
+	}
 	for _, test := range []struct {
 		name string
 		got  int
@@ -428,3 +459,37 @@ func TestAgainstJSONMatchingField(t *testing.T) {
 		}
 	}
 }
+
+func TestTagErrors(t *testing.T) {
+	called := false
+	c := NewCache(func(t reflect.StructTag) (string, bool, interface{}, error) {
+		called = true
+		s := t.Get("f")
+		if s == "bad" {
+			return "", false, nil, errors.New("error")
+		}
+		return s, true, nil, nil
+	})
+
+	type T struct {
+		X int `f:"ok"`
+		Y int `f:"bad"`
+	}
+
+	_, err := c.Fields(reflect.TypeOf(T{}))
+	if !called {
+		t.Fatal("tag parser not called")
+	}
+	if err == nil {
+		t.Error("want error, got nil")
+	}
+	// Second time, we should cache the error.
+	called = false
+	_, err = c.Fields(reflect.TypeOf(T{}))
+	if called {
+		t.Fatal("tag parser called on second time")
+	}
+	if err == nil {
+		t.Error("want error, got nil")
+	}
+}