handler: collapse NewStruct into Check (#84)

Remove the top-level Struct and NewStruct constructors, and instead fold
the special case for struct arguments into Check. Update test cases to
exercise both input formats.

This is a breaking change to the handler package API.

handler/handler.go

@@ -110,8 +110,9 @@ type FuncInfo struct {
 	Argument     reflect.Type // the non-context argument type, or nil
 	Result       reflect.Type // the non-error result type, or nil
 	ReportsError bool         // true if the function reports an error
-	strictFields bool         // enforce strict field checking
-	posNames     []string     // positional field names (requires strictFields)
+
+	strictFields bool     // enforce strict field checking
+	posNames     []string // positional field names
 
 	fn interface{} // the original function value
 }
@@ -121,7 +122,7 @@ type FuncInfo struct {
 // report an error when unmarshaling an object into a struct if the object
 // contains fields unknown by the struct. Strict field checking has no effect
 // for non-struct arguments.
-func (fi *FuncInfo) SetStrict(strict bool) { fi.strictFields = strict }
+func (fi *FuncInfo) SetStrict(strict bool) *FuncInfo { fi.strictFields = strict; return fi }
 
 // Wrap adapts the function represented by fi in a Func that satisfies the
 // jrpc2.Handler interface.  The wrapped function can obtain the *jrpc2.Request
@@ -253,10 +254,35 @@ func (fi *FuncInfo) Wrap() Func {
 //
 // If fn does not have one of these forms, Check reports an error.
 //
-// Note that the JSON-RPC standard restricts encoded parameter values to arrays
-// and objects.  Check will accept argument types that do not encode to arrays
-// or objects, but the wrapper will report an error when decoding the request.
-// The recommended solution is to define a struct type for your parameters.
+// If the type of X is a struct or a pointer to a struct, the generated wrapper
+// accepts JSON parameters as either an object or an array.  The names used to
+// map array elements to struct fields are chosen by examining the fields of X
+// in order of their declaration.  Unexported fields are skipped, and the
+// parameter name for each exported field is chosen by following these rules,
+// in order:
+//
+// If the field has a `json:"-"` tag, the field is skipped.
+//
+// Otherwise, if the field has a `json:"name"` tag and the name is not empty,
+// "name" is used.
+//
+// Otherwise, if the field nas a `jrpc:"name"` tag, "name" is used.  Note: This
+// case is meant to support types with custom implementations of UnmarshalJSON.
+// Assigning a name that does not match the field name can cause json.Unmarshal
+// to report an error.
+//
+// Otherwise, if the field is anonymous (embedded) it is skipped. To include an
+// anonymous field, ensure it is tagged for one of the previous rules.
+//
+// Otherwise the name of the field is used with its first character converted
+// to lowercase.
+//
+// For other (non-struct) argument types, the accepted format is whatever the
+// json.Unmarshal function can decode into the value.  Note, however, that the
+// JSON-RPC standard restricts encoded parameter values to arrays and objects.
+// Check will accept argument types that cannot accept arrays or objects, but
+// the wrapper will report an error when decoding the request.  The recommended
+// solution is to define a struct type for your parameters.
 //
 // For a single arbitrary type, another approach is to use a 1-element array:
 //
@@ -266,7 +292,6 @@ func (fi *FuncInfo) Wrap() Func {
 //   }
 //
 // For more complex positional signatures, see also handler.Positional.
-//
 func Check(fn interface{}) (*FuncInfo, error) {
 	if fn == nil {
 		return nil, errors.New("nil function")
@@ -288,6 +313,11 @@ func Check(fn interface{}) (*FuncInfo, error) {
 		info.Argument = info.Type.In(1)
 	}
 
+	// Check for struct field names on the argument type.
+	if ok, names := structFieldNames(info.Argument); ok {
+		info.posNames = names
+	}
+
 	// Check return values.
 	no := info.Type.NumOut()
 	if no < 1 || no > 2 {

handler/handler_test.go

@@ -28,7 +28,7 @@ type argStruct struct {
 	B int    `json:"bravo"`
 }
 
-// Verify that the CHeck function correctly handles the various type signatures
+// Verify that the Check function correctly handles the various type signatures
 // it's advertised to support, and not others.
 func TestCheck(t *testing.T) {
 	tests := []struct {
@@ -160,48 +160,78 @@ func TestPositional(t *testing.T) {
 	}
 }
 
-// Verify that the STruct function correctly handles its cases.
-func TestStruct(t *testing.T) {
+// Verify that the Check function correctly handles struct names.
+func TestCheck_structArg(t *testing.T) {
 	type args struct {
-		A string `json:"alpha"`
-		B int    `json:"-"`
-		C bool   `json:",omitempty"`
-		D byte
-		E int `jrpc:"evil"`
+		A    string `json:"apple"`
+		B    int    `json:"-"`
+		C    bool   `json:",omitempty"`
+		D    byte   // unspecified, use default name
+		Evil int    `jrpc:"evil"`
 	}
+
+	const base = `{"jsonrpc":"2.0","id":1,"method":"M","params":%s}`
+	const inputObj = `{"apple":"1","c":true,"d":25,"evil":666}`
+	const inputArray = `["1", true, 25, 666]`
+	fail := errors.New("fail")
+
+	// Each of these cases has a valid struct argument type.  Call each wrapper
+	// with the same arguments in object an array format, and verify that the
+	// expected result or error are reported.
 	tests := []struct {
-		v   interface{}
-		bad bool
+		name string
+		v    interface{}
+		want interface{}
+		err  error
 	}{
-		{v: nil, bad: true},              // nil value
-		{v: "not a function", bad: true}, // not a function
-
 		// Things that should work.
-		{v: func(context.Context, args) error { return nil }},
-		{v: func(context.Context, *args) error { return nil }},
-		{v: func(context.Context, args) int { return 0 }},
-		{v: func(context.Context, *args) (bool, error) { return true, nil }},
-
-		// Things that should not work.
-
-		// No non-context argument.
-		{v: func(context.Context) error { return nil }, bad: true},
-		{v: func(context.Context) int { return 1 }, bad: true},
-		// Argument is not a struct.
-		{v: func(context.Context, bool) bool { return false }, bad: true},
-		// Too many arguemnts.
-		{v: func(context.Context, args, *args) int { return 0 }, bad: true},
+		{name: "non-pointer returns string",
+			v: func(_ context.Context, x args) string { return x.A }, want: "1"},
+		{name: "pointer returns bool",
+			v: func(_ context.Context, x *args) bool { return x.C }, want: true},
+		{name: "non-pointer returns int",
+			v: func(_ context.Context, x args) int { return x.Evil }, want: 666},
+		{name: "pointer returns bool",
+			v: func(_ context.Context, x *args) (bool, error) { return true, nil }, want: true},
+		{name: "non-pointer reports error",
+			v: func(context.Context, args) (int, error) { return 0, fail }, err: fail},
+		{name: "pointer reports error",
+			v: func(context.Context, *args) error { return fail }, err: fail},
 
 		// N.B. Other cases are covered by TestCheck. The cases here are only
 		// those that Struct checks for explicitly.
 	}
+
 	for _, test := range tests {
-		got, err := handler.Struct(test.v)
-		if !test.bad && err != nil {
-			t.Errorf("Struct(%T: unexpected error: %v", test.v, err)
-		} else if test.bad && err == nil {
-			t.Errorf("Struct(%T: got %+v, want error", test.v, got)
-		}
+		t.Run(test.name, func(t *testing.T) {
+			fi, err := handler.Check(test.v)
+			if err != nil {
+				t.Fatalf("Check failed for %T: %v", test.v, err)
+			}
+			fn := fi.Wrap()
+
+			for _, sub := range []struct {
+				name string
+				req  *jrpc2.Request
+			}{
+				{"Object", testutil.MustParseRequest(t, fmt.Sprintf(base, inputObj))},
+				{"Array", testutil.MustParseRequest(t, fmt.Sprintf(base, inputArray))},
+			} {
+				t.Run(sub.name, func(t *testing.T) {
+
+					rsp, err := fn(context.Background(), sub.req)
+					if err != test.err {
+						t.Errorf("Got error %v, want %v", err, test.err)
+					}
+					if rsp != test.want {
+						t.Errorf("Got value %v, want %v", rsp, test.want)
+					}
+					if t.Failed() {
+						t.Logf("Parameters: %#q", sub.req.ParamString())
+					}
+				})
+			}
+		})
 	}
 }
 
@@ -213,8 +243,7 @@ func TestFuncInfo_SetStrict(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Check failed: %v", err)
 	}
-	fi.SetStrict(true)
-	fn := fi.Wrap()
+	fn := fi.SetStrict(true).Wrap()
 
 	req := testutil.MustParseRequest(t, `{
    "jsonrpc": "2.0",

handler/positional.go

@@ -25,72 +25,18 @@ func NewPos(fn interface{}, names ...string) Func {
 	return fi.Wrap()
 }
 
-// NewStruct adapts a function to a jrpc2.Handler. The concrete value of fn
-// must be a function accepted by Struct. The resulting Func will handle JSON
-// encoding and decoding, call fn, and report appropriate aerrors.
-//
-// NewStruct is intended for use during program initialization, and will panic
-// if the t ype of fn does not have one of the accepted forms. Programs that
-// need to check for possible errors should call handler.Struct directly, and
-// use the Wrap method of the resulting FuncInfo to obtain the wrapper.
-func NewStruct(fn interface{}) Func {
-	fi, err := Struct(fn)
-	if err != nil {
-		panic(err)
-	}
-	return fi.Wrap()
-}
-
-// Struct checks whether fn can serve as a jrpc2.Handler. The concrete value of
-// fn must be a function with one of the following type signature schemes:
-//
-//   func(context.Context, X) (Y, error)
-//   func(context.Context, X) Y
-//   func(context.Context, X) error
-//
-// The type of X must be a struct or a pointer to a struct, Y may be any type
-// that can be marshaled to JSON.
-//
-// The generated wrapper accepts JSON parameters as either object or array.
-// The names used to map array elements to fields are chosen by examining the
-// fields of X in order of their declaration. Unexported fields are skipped,
-// and the parameter name for each exported field is chosen by following these
-// rules, in order:
-//
-// If the field has a `json:"-"` tag, the field is skipped.
-//
-// Otherwise, if the field has a `json:"name"` tag and the name is not empty,
-// "name" is used.
-//
-// Otherwise, if the field nas a `jrpc:"name"` tag, "name" is used.  Note: This
-// case is meant to support types with custom implementations of UnmarshalJSON.
-// Assigning a name that does not match the field name can cause json.Unmarshal
-// to report an error.
-//
-// Otherwise, if the field is anonymous (embedded) it is skipped.
-//
-// Otherwise the name of the field is used with its first character converted
-// to lowercase.
-func Struct(fn interface{}) (*FuncInfo, error) {
-	if fn == nil {
-		return nil, errors.New("nil function")
-	}
-
-	ftype := reflect.TypeOf(fn)
-	if ftype.Kind() != reflect.Func {
-		return nil, errors.New("not a function")
-	} else if np := ftype.NumIn(); np != 2 {
-		return nil, errors.New("wrong number of parameters")
+// structFieldNames reports whether atype is a struct or pointer to struct, and
+// if so returns a slice of the eligible field names in order of declaration.
+// If atype == nil or is not a (pointer to) struct, it returns false, nil.
+func structFieldNames(atype reflect.Type) (bool, []string) {
+	if atype == nil {
+		return false, nil
 	}
-
-	// Check will verify the rest of the signature; for now we just need to
-	// extract the argument names from the fields.
-	atype := ftype.In(1)
 	if atype.Kind() == reflect.Ptr {
 		atype = atype.Elem()
 	}
 	if atype.Kind() != reflect.Struct {
-		return nil, errors.New("second parameter is not a struct")
+		return false, nil
 	}
 
 	var names []string
@@ -115,21 +61,14 @@ func Struct(fn interface{}) (*FuncInfo, error) {
 			continue
 		}
 		if fi.Anonymous {
+			// This is an untagged anonymous field. Tagged anonymous fields are
+			// handled by the cases above.
 			continue
 		}
 		name := strings.ToLower(fi.Name[:1]) + fi.Name[1:]
 		names = append(names, name)
 	}
-
-	if len(names) == 0 {
-		return nil, errors.New("no matching fields")
-	}
-	fi, err := Check(fn)
-	if err == nil {
-		fi.strictFields = true
-		fi.posNames = names
-	}
-	return fi, err
+	return true, names
 }
 
 // Positional checks whether fn can serve as a jrpc2.Handler. The concrete

jrpc2_test.go

@@ -41,7 +41,7 @@ var testService = handler.Map{
 	"Nil":  handler.New(methodNil),
 	"Ctx":  handler.New(methodCtx),
 	"Ping": handler.New(methodPing),
-	"Echo": handler.NewStruct(methodStruct),
+	"Echo": handler.New(methodStruct),
 }
 
 type dummy struct{}