fix: query/path params only allow string

fixes: #3037
block · Oct 9, 2024 · d203386 · d203386
1 parent f4fab8c
commit d203386
Show file tree

Hide file tree

Showing 6 changed files with 170 additions and 52 deletions.
diff --git a/backend/controller/ingress/ingress_integration_test.go b/backend/controller/ingress/ingress_integration_test.go
@@ -35,6 +35,21 @@ func TestHttpIngress(t *testing.T) {
 				assert.True(t, ok, "good_stuff is not a string: %s", repr.String(resp.JsonBody))
 				assert.Equal(t, "This is good stuff", goodStuff)
 			})},
+			in.SubTest{Name: "GetWithNumericQueryParams", Action: in.HttpCall(http.MethodGet, "/getquery?userId=123&postId=456", nil, nil, func(t testing.TB, resp *in.HTTPResponse) {
+				assert.Equal(t, 200, resp.Status)
+				assert.Equal(t, []string{"Header from FTL"}, resp.Headers["Get"])
+				expectContentType(t, resp, "application/json;charset=utf-8")
+
+				message, ok := resp.JsonBody["msg"].(string)
+				assert.True(t, ok, "msg is not a string: %s", repr.String(resp.JsonBody))
+				assert.Equal(t, "UserID: 123, PostID: 456", message)
+
+				nested, ok := resp.JsonBody["nested"].(map[string]any)
+				assert.True(t, ok, "nested is not a map: %s", repr.String(resp.JsonBody))
+				goodStuff, ok := nested["good_stuff"].(string)
+				assert.True(t, ok, "good_stuff is not a string: %s", repr.String(resp.JsonBody))
+				assert.Equal(t, "This is good stuff", goodStuff)
+			})},
 			in.SubTest{Name: "PostUsers", Action: in.HttpCall(http.MethodPost, "/users", nil, in.JsonData(t, in.Obj{"userId": 123, "postId": 345}), func(t testing.TB, resp *in.HTTPResponse) {
 				assert.Equal(t, 201, resp.Status)
 				assert.Equal(t, []string{"Header from FTL"}, resp.Headers["Post"])

diff --git a/backend/controller/ingress/ingress_test.go b/backend/controller/ingress/ingress_test.go
@@ -169,13 +169,13 @@ func TestParseQueryParams(t *testing.T) {
 		err     string
 	}{
 		{query: "", request: obj{}},
-		{query: "int=1", request: obj{"int": "1"}},
-		{query: "float=2.2", request: obj{"float": "2.2"}},
+		{query: "int=1", request: obj{"int": int64(1)}},
+		{query: "float=2.2", request: obj{"float": float64(2.2)}},
 		{query: "string=test", request: obj{"string": "test"}},
-		{query: "bool=true", request: obj{"bool": "true"}},
-		{query: "array=2", request: obj{"array": []string{"2"}}},
-		{query: "array=10&array=11", request: obj{"array": []string{"10", "11"}}},
-		{query: "int=10&array=11&array=12", request: obj{"int": "10", "array": []string{"11", "12"}}},
+		{query: "bool=true", request: obj{"bool": true}},
+		{query: "array=2", request: obj{"array": []any{int64(2)}}},
+		{query: "array=10&array=11", request: obj{"array": []any{int64(10), int64(11)}}},
+		{query: "int=10&array=11&array=12", request: obj{"int": int64(10), "array": []any{int64(11), int64(12)}}},
 		{query: "int=1&int=2", request: nil, err: `failed to parse query parameter "int": multiple values are not supported`},
 		{query: "[a,b]=c", request: nil, err: "complex key \"[a,b]\" is not supported, use '@json=' instead"},
 		{query: "array=[1,2]", request: nil, err: `failed to parse query parameter "array": complex value "[1,2]" is not supported, use '@json=' instead`},

diff --git a/backend/controller/ingress/request.go b/backend/controller/ingress/request.go
@@ -134,45 +134,55 @@ func manglePathParameters(params map[string]string, ref *schema.Ref, sch *schema
 		return nil, err
 	}
 
-	switch paramsField.Type.(type) {
-	case *schema.Ref, *schema.Map:
+	switch m := paramsField.Type.(type) {
+	case *schema.Map:
 		ret := map[string]any{}
 		for k, v := range params {
 			ret[k] = v
 		}
 		return ret, nil
+	case *schema.Ref:
+		data := &schema.Data{}
+		err := sch.ResolveToType(m, data)
+		if err != nil {
+			return nil, fmt.Errorf("failed to resolve path parameter type: %w", err)
+		}
+		return parsePathParams(params, data)
 	default:
 	}
 	// This is a scalar, there should only be a single param
 	// This is validated by the schema, we don't need extra validation here
 	for _, val := range params {
-
-		switch paramsField.Type.(type) {
-		case *schema.String:
-			return val, nil
-		case *schema.Int:
-			parsed, err := strconv.ParseInt(val, 10, 64)
-			if err != nil {
-				return nil, fmt.Errorf("failed to parse int from path parameter: %w", err)
-			}
-			return parsed, nil
-		case *schema.Float:
-			float, err := strconv.ParseFloat(val, 64)
-			if err != nil {
-				return nil, fmt.Errorf("failed to parse float from path parameter: %w", err)
-			}
-			return float, nil
-		case *schema.Bool:
-			// TODO: is anything else considered truthy?
-			return val == "true", nil
-		default:
-			return nil, fmt.Errorf("unsupported path parameter type %T", paramsField.Type)
-		}
+		return parseScalar(paramsField.Type, val)
 	}
 	// Empty map
 	return map[string]any{}, nil
 }
 
+func parseScalar(paramsField schema.Type, val string) (any, error) {
+	switch paramsField.(type) {
+	case *schema.String:
+		return val, nil
+	case *schema.Int:
+		parsed, err := strconv.ParseInt(val, 10, 64)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse int from path parameter: %w", err)
+		}
+		return parsed, nil
+	case *schema.Float:
+		float, err := strconv.ParseFloat(val, 64)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse float from path parameter: %w", err)
+		}
+		return float, nil
+	case *schema.Bool:
+		// TODO: is anything else considered truthy?
+		return val == "true", nil
+	default:
+		return nil, fmt.Errorf("unsupported path parameter type %T", paramsField)
+	}
+}
+
 // Takes the map of query parameters and transforms them into the appropriate type
 func mangleQueryParameters(params map[string]any, underlying map[string][]string, ref *schema.Ref, sch *schema.Schema) (any, error) {
 
@@ -181,19 +191,33 @@ func mangleQueryParameters(params map[string]any, underlying map[string][]string
 		return nil, err
 	}
 
-	if m, ok := paramsField.Type.(*schema.Map); ok {
+	switch m := paramsField.Type.(type) {
+	case *schema.Map:
 		if _, ok := m.Value.(*schema.Array); ok {
 			return params, nil
+		} else if _, ok := m.Value.(*schema.String); ok {
+			// We need to turn them into straight strings
+			newParams := map[string]any{}
+			for k, v := range underlying {
+				if len(v) > 0 {
+					newParams[k] = v[0]
+				}
+			}
+			return newParams, nil
 		}
-	}
-	// We need to turn them into straight strings
-	newParams := map[string]any{}
-	for k, v := range underlying {
-		if len(v) > 0 {
-			newParams[k] = v[0]
+	case *schema.Ref:
+		data := &schema.Data{}
+		err := sch.ResolveToType(m, data)
+		if err != nil {
+			return nil, fmt.Errorf("failed to resolve query parameter type: %w", err)
 		}
+		return parseQueryParams(underlying, data)
+	case *schema.Unit:
+		return params, nil
+	default:
 	}
-	return newParams, nil
+
+	return nil, fmt.Errorf("unsupported query parameter type %v", paramsField.Type)
 }
 
 func valueForData(typ schema.Type, data []byte) (any, error) {
@@ -292,7 +316,7 @@ func buildRequestMap(r *http.Request) (map[string]any, error) {
 	}
 }
 
-func parseQueryParams(values url.Values, data *schema.Data) (map[string]any, error) {
+func parseQueryParams(values map[string][]string, data *schema.Data) (map[string]any, error) {
 	if jsonStr, ok := values["@json"]; ok {
 		if len(values) > 1 {
 			return nil, fmt.Errorf("only '@json' parameter is allowed, but other parameters were found")
@@ -309,7 +333,6 @@ func parseQueryParams(values url.Values, data *schema.Data) (map[string]any, err
 		if hasInvalidQueryChars(key) {
 			return nil, fmt.Errorf("complex key %q is not supported, use '@json=' instead", key)
 		}
-
 		var field *schema.Field
 		for _, f := range data.Fields {
 			if jsonAlias, ok := f.Alias(schema.AliasKindJson).Get(); (ok && jsonAlias == key) || f.Name == key {
@@ -334,15 +357,40 @@ func parseQueryParams(values url.Values, data *schema.Data) (map[string]any, err
 		if err != nil {
 			return nil, fmt.Errorf("failed to parse query parameter %q: %w", key, err)
 		}
-
 		if v, ok := val.Get(); ok {
 			queryMap[key] = v
 		}
 	}
-
 	return queryMap, nil
 }
 
+func parsePathParams(values map[string]string, data *schema.Data) (map[string]any, error) {
+	// This is annoyingly close to the query params logic, but just disimilar enough to not be easily refactored
+	pathMap := make(map[string]any)
+	for key, value := range values {
+		var field *schema.Field
+		for _, f := range data.Fields {
+			if jsonAlias, ok := f.Alias(schema.AliasKindJson).Get(); (ok && jsonAlias == key) || f.Name == key {
+				field = f
+			}
+		}
+
+		if field == nil {
+			pathMap[key] = value
+			continue
+		}
+
+		val, err := valueForField(field.Type, []string{value})
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse path parameter %q: %w", key, err)
+		}
+		if v, ok := val.Get(); ok {
+			pathMap[key] = v
+		}
+	}
+	return pathMap, nil
+}
+
 func valueForField(typ schema.Type, value []string) (optional.Option[any], error) {
 	switch t := typ.(type) {
 	case *schema.Bytes, *schema.Map, *schema.Time,
@@ -355,23 +403,38 @@ func valueForField(typ schema.Type, value []string) (optional.Option[any], error
 		if hasInvalidQueryChars(value[0]) {
 			return optional.None[any](), fmt.Errorf("complex value %q is not supported, use '@json=' instead", value[0])
 		}
-		return optional.Some[any](value[0]), nil
+
+		parsed, err := parseScalar(typ, value[0])
+		if err != nil {
+			return optional.None[any](), fmt.Errorf("failed to parse int from path parameter: %w", err)
+		}
+		return optional.Some[any](parsed), nil
 
 	case *schema.Array:
 		for _, v := range value {
 			if hasInvalidQueryChars(v) {
 				return optional.None[any](), fmt.Errorf("complex value %q is not supported, use '@json=' instead", v)
 			}
 		}
-		return optional.Some[any](value), nil
+		ret := []any{}
+		for _, v := range value {
+			field, err := valueForField(t.Element, []string{v})
+			if err != nil {
+				return optional.None[any](), fmt.Errorf("failed to parse array element: %w", err)
+			}
+			if unwrapped, ok := field.Get(); ok {
+				ret = append(ret, unwrapped)
+			}
+		}
+		return optional.Some[any](ret), nil
 
 	case *schema.Optional:
 		if len(value) > 0 {
 			return valueForField(t.Type, value)
 		}
 
 	default:
-		panic(fmt.Sprintf("unsupported type %T", typ))
+		return optional.None[any](), fmt.Errorf("unsupported type %T", typ)
 	}
 
 	return optional.Some[any](value), nil

diff --git a/backend/controller/ingress/testdata/go/httpingress/httpingress.go b/backend/controller/ingress/testdata/go/httpingress/httpingress.go
@@ -11,8 +11,8 @@ import (
 )
 
 type GetRequest struct {
-	UserID string `json:"userId,omitempty"`
-	PostID string `json:"postId,something,else"`
+	UserID int `json:"userId,omitempty"`
+	PostID int `json:"postId,something,else"`
 }
 
 type Nested struct {
@@ -42,7 +42,20 @@ func Get(ctx context.Context, req builtin.HttpRequest[ftl.Unit, GetRequest, ftl.
 	return builtin.HttpResponse[GetResponse, string]{
 		Headers: map[string][]string{"Get": {"Header from FTL"}},
 		Body: ftl.Some(GetResponse{
-			Message: fmt.Sprintf("UserID: %s, PostID: %s", req.PathParameters.UserID, req.PathParameters.PostID),
+			Message: fmt.Sprintf("UserID: %d, PostID: %d", req.PathParameters.UserID, req.PathParameters.PostID),
+			Nested: Nested{
+				GoodStuff: "This is good stuff",
+			},
+		}),
+	}, nil
+}
+
+//ftl:ingress http GET /getquery
+func GetQuery(ctx context.Context, req builtin.HttpRequest[ftl.Unit, ftl.Unit, GetRequest]) (builtin.HttpResponse[GetResponse, string], error) {
+	return builtin.HttpResponse[GetResponse, string]{
+		Headers: map[string][]string{"Get": {"Header from FTL"}},
+		Body: ftl.Some(GetResponse{
+			Message: fmt.Sprintf("UserID: %d, PostID: %d", req.Query.UserID, req.Query.PostID),
 			Nested: Nested{
 				GoodStuff: "This is good stuff",
 			},

diff --git a/...ngress/testdata/java/httpingress/src/main/java/xyz/block/ftl/java/test/http/TestHTTP.java b/...ngress/testdata/java/httpingress/src/main/java/xyz/block/ftl/java/test/http/TestHTTP.java
@@ -21,7 +21,17 @@ public class TestHTTP {
     @GET
     @Path("/users/{userId}/posts/{postId}")
     @ResponseHeader(name = "Get", value = "Header from FTL")
-    public GetResponse get(@RestPath String userId, @RestPath String postId) {
+    public GetResponse get(@RestPath int userId, @RestPath int postId) {
+        return new GetResponse()
+                .setMsg(String.format("UserID: %s, PostID: %s", userId, postId))
+                .setNested(new Nested().setGoodStuff("This is good stuff"));
+    }
+
+
+    @GET
+    @Path("/getquery")
+    @ResponseHeader(name = "Get", value = "Header from FTL")
+    public GetResponse getquery(@RestQuery int userId, @RestQuery int postId) {
         return new GetResponse()
                 .setMsg(String.format("UserID: %s, PostID: %s", userId, postId))
                 .setNested(new Nested().setGoodStuff("This is good stuff"));

diff --git a/internal/schema/validate.go b/internal/schema/validate.go
@@ -816,7 +816,11 @@ func requireUnitPayloadType(n Node, r Type, v *Verb, reqOrResp string) error {
 
 func validatePathParamsPayloadType(n Node, r Type, v *Verb, reqOrResp string) error {
 	switch t := n.(type) {
-	case *String, *Data, *Unit, *Float, *Int, *Bool, *Map: // Valid HTTP param payload types.
+	case *String, *Data, *Unit, *Float, *Int, *Bool: // Valid HTTP param payload types.
+	case *Map:
+		if _, ok := t.Value.(*String); !ok {
+			return errorf(r, "ingress verb %s: %s type %s path params can only support maps of strings, not %v", v.Name, reqOrResp, r, n)
+		}
 	case *TypeAlias:
 		// allow aliases of external types
 		for _, m := range t.Metadata {
@@ -833,7 +837,20 @@ func validatePathParamsPayloadType(n Node, r Type, v *Verb, reqOrResp string) er
 
 func validateQueryParamsPayloadType(n Node, r Type, v *Verb, reqOrResp string) error {
 	switch t := n.(type) {
-	case *Data, *Unit, *Map: // Valid HTTP query payload types.
+	case *Data, *Unit: // Valid HTTP query payload types.
+		return nil
+	case *Map:
+		switch val := t.Value.(type) {
+		case *String:
+			// Valid HTTP query payload type
+			return nil
+		case *Array:
+			if _, ok := val.Element.(*String); ok {
+				return nil
+			}
+		default:
+			return errorf(r, "ingress verb %s: %s type %s query params can only support maps of strings or string arrays, not %v", v.Name, reqOrResp, r, n)
+		}
 	case *TypeAlias:
 		// allow aliases of external types
 		for _, m := range t.Metadata {
@@ -845,7 +862,7 @@ func validateQueryParamsPayloadType(n Node, r Type, v *Verb, reqOrResp string) e
 	default:
 		return errorf(r, "ingress verb %s: %s type %s must have a param of data structure, unit or map not %s", v.Name, reqOrResp, r, n)
 	}
-	return nil
+	return errorf(r, "ingress verb %s: %s type %s query params can only support maps of strings or string arrays, or data types not %v", v.Name, reqOrResp, r, n)
 }
 
 func validateVerbSubscriptions(module *Module, v *Verb, md *MetadataSubscriber, scopes Scopes, schema optional.Option[*Schema]) (merr []error) {