From d20338695a99435e29b891a2b82dbc9a8b31e3ec Mon Sep 17 00:00:00 2001 From: Stuart Douglas Date: Wed, 9 Oct 2024 13:38:15 +1100 Subject: [PATCH] fix: query/path params only allow string fixes: #3037 --- .../ingress/ingress_integration_test.go | 15 ++ backend/controller/ingress/ingress_test.go | 12 +- backend/controller/ingress/request.go | 141 +++++++++++++----- .../testdata/go/httpingress/httpingress.go | 19 ++- .../block/ftl/java/test/http/TestHTTP.java | 12 +- internal/schema/validate.go | 23 ++- 6 files changed, 170 insertions(+), 52 deletions(-) diff --git a/backend/controller/ingress/ingress_integration_test.go b/backend/controller/ingress/ingress_integration_test.go index 2d6831e096..5bd9f994c2 100644 --- 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 index d0f8e596a6..122e9d8ff7 100644 --- 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 index 99379dd475..59f9b013dd 100644 --- 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,7 +403,12 @@ 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 { @@ -363,7 +416,17 @@ func valueForField(typ schema.Type, value []string) (optional.Option[any], error 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 { @@ -371,7 +434,7 @@ func valueForField(typ schema.Type, value []string) (optional.Option[any], error } 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 index e5711c9909..08cd340ced 100644 --- 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/backend/controller/ingress/testdata/java/httpingress/src/main/java/xyz/block/ftl/java/test/http/TestHTTP.java b/backend/controller/ingress/testdata/java/httpingress/src/main/java/xyz/block/ftl/java/test/http/TestHTTP.java index bb4da42d26..53e5c40d7e 100644 --- a/backend/controller/ingress/testdata/java/httpingress/src/main/java/xyz/block/ftl/java/test/http/TestHTTP.java +++ b/backend/controller/ingress/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 index 3cf1dcb91a..5354b39d32 100644 --- 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) {