rpc: remove dependency of URL (GET) requests on tmjson (#7590)

The parameters for RPC GET requests are parsed from query arguments in the
request URL. Rework this code to remove the need for tmjson. The structure of a
call still requires reflection, and still works the same way as before, but the
code structure has been simplified and cleaned up a bit.

Points of note:

- Consolidate handling of pointer types, so we only need to dereference once.
- Reduce the number of allocations of reflective types.
- Report errors for unsupported types rather than returning untyped nil.

Update the tests as well. There was one test case that checked for an error on
a behaviour the OpenAPI docs explicitly demonstrates as supported, so I fixed
that test case, and also added some new ones for cases that weren't checked.

Related:

* Update e2e base Go image to 1.17 (to match config).

rpc/jsonrpc/server/http_uri_handler.go

@@ -1,15 +1,16 @@
 package server
 
 import (
+	"context"
 	"encoding/hex"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"net/http"
 	"reflect"
-	"regexp"
+	"strconv"
 	"strings"
 
-	tmjson "github.com/tendermint/tendermint/libs/json"
 	"github.com/tendermint/tendermint/libs/log"
 	"github.com/tendermint/tendermint/rpc/coretypes"
 	rpctypes "github.com/tendermint/tendermint/rpc/jsonrpc/types"
@@ -17,8 +18,6 @@ import (
 
 // HTTP + URI handler
 
-var reInt = regexp.MustCompile(`^-?[0-9]+$`)
-
 // convert from a function name to the http handler
 func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWriter, *http.Request) {
 	// Always return -1 as there's no ID here.
@@ -35,22 +34,21 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit
 	}
 
 	// All other endpoints
-	return func(w http.ResponseWriter, r *http.Request) {
-		ctx := rpctypes.WithCallInfo(r.Context(), &rpctypes.CallInfo{HTTPRequest: r})
-		args := []reflect.Value{reflect.ValueOf(ctx)}
-
-		fnArgs, err := httpParamsToArgs(rpcFunc, r)
+	return func(w http.ResponseWriter, req *http.Request) {
+		ctx := rpctypes.WithCallInfo(req.Context(), &rpctypes.CallInfo{
+			HTTPRequest: req,
+		})
+		args, err := parseURLParams(ctx, rpcFunc, req)
 		if err != nil {
-			writeHTTPResponse(w, logger, rpctypes.RPCInvalidParamsError(
-				dummyID, fmt.Errorf("error converting http params to arguments: %w", err)))
+			w.Header().Set("Content-Type", "text/plain")
+			w.WriteHeader(http.StatusBadRequest)
+			fmt.Fprintln(w, err.Error())
 			return
 		}
-		args = append(args, fnArgs...)
+		outs := rpcFunc.f.Call(args)
 
-		returns := rpcFunc.f.Call(args)
-
-		logger.Debug("HTTPRestRPC", "method", r.URL.Path, "args", args, "returns", returns)
-		result, err := unreflectResult(returns)
+		logger.Debug("HTTPRestRPC", "method", req.URL.Path, "args", args, "returns", outs)
+		result, err := unreflectResult(outs)
 		switch e := err.(type) {
 		// if no error then return a success response
 		case nil:
@@ -74,142 +72,135 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit
 	}
 }
 
-// Covert an http query to a list of properly typed values.
-// To be properly decoded the arg must be a concrete type from tendermint (if its an interface).
-func httpParamsToArgs(rpcFunc *RPCFunc, r *http.Request) ([]reflect.Value, error) {
-	// skip types.Context
-	const argsOffset = 1
+func parseURLParams(ctx context.Context, rf *RPCFunc, req *http.Request) ([]reflect.Value, error) {
+	if err := req.ParseForm(); err != nil {
+		return nil, fmt.Errorf("invalid HTTP request: %w", err)
+	}
+	getArg := func(name string) (string, bool) {
+		if req.Form.Has(name) {
+			return req.Form.Get(name), true
+		}
+		return "", false
+	}
 
-	values := make([]reflect.Value, len(rpcFunc.argNames))
+	vals := make([]reflect.Value, len(rf.argNames)+1)
+	vals[0] = reflect.ValueOf(ctx)
+	for i, name := range rf.argNames {
+		atype := rf.args[i+1]
 
-	for i, name := range rpcFunc.argNames {
-		argType := rpcFunc.args[i+argsOffset]
-
-		values[i] = reflect.Zero(argType) // set default for that type
-
-		arg := getParam(r, name)
-		// log.Notice("param to arg", "argType", argType, "name", name, "arg", arg)
-
-		if arg == "" {
+		text, ok := getArg(name)
+		if !ok {
+			vals[i+1] = reflect.Zero(atype)
 			continue
 		}
 
-		v, ok, err := nonJSONStringToArg(argType, arg)
+		val, err := parseArgValue(atype, text)
 		if err != nil {
-			return nil, err
-		}
-		if ok {
-			values[i] = v
-			continue
-		}
-
-		values[i], err = jsonStringToArg(argType, arg)
-		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("decoding parameter %q: %w", name, err)
 		}
+		vals[i+1] = val
 	}
-
-	return values, nil
+	return vals, nil
 }
 
-func jsonStringToArg(rt reflect.Type, arg string) (reflect.Value, error) {
-	rv := reflect.New(rt)
-	err := tmjson.Unmarshal([]byte(arg), rv.Interface())
-	if err != nil {
-		return rv, err
-	}
-	rv = rv.Elem()
-	return rv, nil
-}
-
-func nonJSONStringToArg(rt reflect.Type, arg string) (reflect.Value, bool, error) {
-	if rt.Kind() == reflect.Ptr {
-		rv1, ok, err := nonJSONStringToArg(rt.Elem(), arg)
-		switch {
-		case err != nil:
-			return reflect.Value{}, false, err
-		case ok:
-			rv := reflect.New(rt.Elem())
-			rv.Elem().Set(rv1)
-			return rv, true, nil
-		default:
-			return reflect.Value{}, false, nil
-		}
+func parseArgValue(atype reflect.Type, text string) (reflect.Value, error) {
+	// Regardless whether the argument is a pointer type, allocate a pointer so
+	// we can set the computed value.
+	var out reflect.Value
+	isPtr := atype.Kind() == reflect.Ptr
+	if isPtr {
+		out = reflect.New(atype.Elem())
 	} else {
-		return _nonJSONStringToArg(rt, arg)
+		out = reflect.New(atype)
+	}
+
+	baseType := out.Type().Elem()
+	if isIntType(baseType) {
+		// Integral type: Require a base-10 digit string. For compatibility with
+		// existing use allow quotation marks.
+		v, err := decodeInteger(text)
+		if err != nil {
+			return reflect.Value{}, fmt.Errorf("invalid integer: %w", err)
+		}
+		out.Elem().Set(reflect.ValueOf(v).Convert(baseType))
+	} else if isStringOrBytes(baseType) {
+		// String or byte slice: Check for quotes, hex encoding.
+		dec, err := decodeString(text)
+		if err != nil {
+			return reflect.Value{}, err
+		}
+		out.Elem().Set(reflect.ValueOf(dec).Convert(baseType))
+
+	} else if baseType.Kind() == reflect.Bool {
+		b, err := strconv.ParseBool(text)
+		if err != nil {
+			return reflect.Value{}, fmt.Errorf("invalid boolean: %w", err)
+		}
+		out.Elem().Set(reflect.ValueOf(b))
+
+	} else {
+		// We don't know how to represent other types.
+		return reflect.Value{}, fmt.Errorf("unsupported argument type %v", baseType)
+	}
+
+	// If the argument wants a pointer, return the value as-is, otherwise
+	// indirect the pointer back off.
+	if isPtr {
+		return out, nil
+	}
+	return out.Elem(), nil
+}
+
+var uint64Type = reflect.TypeOf(uint64(0))
+
+// isIntType reports whether atype is an integer-shaped type.
+func isIntType(atype reflect.Type) bool {
+	switch atype.Kind() {
+	case reflect.Float32, reflect.Float64:
+		return false
+	default:
+		return atype.ConvertibleTo(uint64Type)
 	}
 }
 
-// NOTE: rt.Kind() isn't a pointer.
-func _nonJSONStringToArg(rt reflect.Type, arg string) (reflect.Value, bool, error) {
-	isIntString := reInt.Match([]byte(arg))
-	isQuotedString := strings.HasPrefix(arg, `"`) && strings.HasSuffix(arg, `"`)
-	isHexString := strings.HasPrefix(strings.ToLower(arg), "0x")
-
-	var expectingString, expectingByteSlice, expectingInt bool
-	switch rt.Kind() {
-	case reflect.Int,
-		reflect.Uint,
-		reflect.Int8,
-		reflect.Uint8,
-		reflect.Int16,
-		reflect.Uint16,
-		reflect.Int32,
-		reflect.Uint32,
-		reflect.Int64,
-		reflect.Uint64:
-		expectingInt = true
+// isStringOrBytes reports whether atype is a string or []byte.
+func isStringOrBytes(atype reflect.Type) bool {
+	switch atype.Kind() {
 	case reflect.String:
-		expectingString = true
+		return true
 	case reflect.Slice:
-		expectingByteSlice = rt.Elem().Kind() == reflect.Uint8
+		return atype.Elem().Kind() == reflect.Uint8
+	default:
+		return false
 	}
-
-	if isIntString && expectingInt {
-		qarg := `"` + arg + `"`
-		rv, err := jsonStringToArg(rt, qarg)
-		if err != nil {
-			return rv, false, err
-		}
-
-		return rv, true, nil
-	}
-
-	if isHexString {
-		if !expectingString && !expectingByteSlice {
-			err := fmt.Errorf("got a hex string arg, but expected '%s'",
-				rt.Kind().String())
-			return reflect.ValueOf(nil), false, err
-		}
-
-		var value []byte
-		value, err := hex.DecodeString(arg[2:])
-		if err != nil {
-			return reflect.ValueOf(nil), false, err
-		}
-		if rt.Kind() == reflect.String {
-			return reflect.ValueOf(string(value)), true, nil
-		}
-		return reflect.ValueOf(value), true, nil
-	}
-
-	if isQuotedString && expectingByteSlice {
-		v := reflect.New(reflect.TypeOf(""))
-		err := tmjson.Unmarshal([]byte(arg), v.Interface())
-		if err != nil {
-			return reflect.ValueOf(nil), false, err
-		}
-		v = v.Elem()
-		return reflect.ValueOf([]byte(v.String())), true, nil
-	}
-
-	return reflect.ValueOf(nil), false, nil
 }
 
-func getParam(r *http.Request, param string) string {
-	s := r.URL.Query().Get(param)
-	if s == "" {
-		s = r.FormValue(param)
-	}
-	return s
+// isQuotedString reports whether s is enclosed in double quotes.
+func isQuotedString(s string) bool {
+	return len(s) >= 2 && strings.HasPrefix(s, `"`) && strings.HasSuffix(s, `"`)
+}
+
+// decodeInteger decodes s into an int64. If s is "double quoted" the quotes
+// are removed; otherwise s must be a base-10 digit string.
+func decodeInteger(s string) (int64, error) {
+	if isQuotedString(s) {
+		s = s[1 : len(s)-1]
+	}
+	return strconv.ParseInt(s, 10, 64)
+}
+
+// decodeString decodes s into a byte slice. If s has an 0x prefix, it is
+// treated as a hex-encoded string. If it is "double quoted" it is treated as a
+// JSON string value. Otherwise, s is converted to bytes directly.
+func decodeString(s string) ([]byte, error) {
+	if lc := strings.ToLower(s); strings.HasPrefix(lc, "0x") {
+		return hex.DecodeString(lc[2:])
+	} else if isQuotedString(s) {
+		var dec string
+		if err := json.Unmarshal([]byte(s), &dec); err != nil {
+			return nil, fmt.Errorf("invalid quoted string: %w", err)
+		}
+		return []byte(dec), nil
+	}
+	return []byte(s), nil
 }

rpc/jsonrpc/server/parse_test.go

@@ -187,8 +187,15 @@ func TestParseURI(t *testing.T) {
 		// can parse numbers quoted, too
 		{[]string{`"7"`, `"flew"`}, 7, "flew", false},
 		{[]string{`"-10"`, `"bob"`}, -10, "bob", false},
-		// cant parse strings uquoted
-		{[]string{`"-10"`, `bob`}, -10, "bob", true},
+		// can parse strings hex-escaped, in either case
+		{[]string{`-9`, `0x626f62`}, -9, "bob", false},
+		{[]string{`-9`, `0X646F7567`}, -9, "doug", false},
+		// can parse strings unquoted (as per OpenAPI docs)
+		{[]string{`0`, `hey you`}, 0, "hey you", false},
+		// fail for invalid numbers, strings, hex
+		{[]string{`"-xx"`, `bob`}, 0, "", true},  // bad number
+		{[]string{`"95""`, `"bob`}, 0, "", true}, // bad string
+		{[]string{`15`, `0xa`}, 0, "", true},     // bad hex
 	}
 	for idx, tc := range cases {
 		i := strconv.Itoa(idx)
@@ -198,14 +205,14 @@ func TestParseURI(t *testing.T) {
 			tc.raw[0], tc.raw[1])
 		req, err := http.NewRequest("GET", url, nil)
 		assert.NoError(t, err)
-		vals, err := httpParamsToArgs(call, req)
+		vals, err := parseURLParams(context.Background(), call, req)
 		if tc.fail {
 			assert.Error(t, err, i)
 		} else {
 			assert.NoError(t, err, "%s: %+v", i, err)
-			if assert.Equal(t, 2, len(vals), i) {
-				assert.Equal(t, tc.height, vals[0].Int(), i)
-				assert.Equal(t, tc.name, vals[1].String(), i)
+			if assert.Equal(t, 3, len(vals), i) {
+				assert.Equal(t, tc.height, vals[1].Int(), i)
+				assert.Equal(t, tc.name, vals[2].String(), i)
 			}
 		}
 

test/e2e/docker/Dockerfile

@@ -1,7 +1,7 @@
 # We need to build in a Linux environment to support C libraries, e.g. RocksDB.
 # We use Debian instead of Alpine, so that we can use binary database packages
 # instead of spending time compiling them.
-FROM golang:1.16
+FROM golang:1.17
 
 RUN apt-get -qq update -y && apt-get -qq upgrade -y >/dev/null
 RUN apt-get -qq install -y libleveldb-dev librocksdb-dev >/dev/null