From b97affd52d514bcd0ff45501e0af2ddc928acf05 Mon Sep 17 00:00:00 2001 From: Stuart Douglas Date: Sat, 17 Aug 2024 15:46:24 +1000 Subject: [PATCH] feat: HttpRequest path and query mapping fixes: #2230 --- backend/controller/console/console_test.go | 15 ++- backend/controller/ingress/handler_test.go | 8 +- backend/controller/ingress/ingress.go | 6 +- backend/controller/ingress/request.go | 135 ++++++++++++++------ backend/controller/ingress/request_test.go | 89 +++++++++---- backend/schema/builtin.go | 6 +- backend/schema/schema_test.go | 20 +-- backend/schema/validate.go | 141 ++++++++++++++++++--- backend/schema/validate_test.go | 39 +++--- 9 files changed, 333 insertions(+), 126 deletions(-) diff --git a/backend/controller/console/console_test.go b/backend/controller/console/console_test.go index 344320dc9c..4485f3c982 100644 --- a/backend/controller/console/console_test.go +++ b/backend/controller/console/console_test.go @@ -3,8 +3,9 @@ package console import ( "testing" - "github.com/TBD54566975/ftl/backend/schema" "github.com/alecthomas/assert/v2" + + "github.com/TBD54566975/ftl/backend/schema" ) func TestVerbSchemaString(t *testing.T) { @@ -15,7 +16,7 @@ func TestVerbSchemaString(t *testing.T) { } ingressVerb := &schema.Verb{ Name: "Ingress", - Request: &schema.Ref{Module: "builtin", Name: "HttpRequest", TypeParameters: []schema.Type{&schema.String{}}}, + Request: &schema.Ref{Module: "builtin", Name: "HttpRequest", TypeParameters: []schema.Type{&schema.String{}, &schema.Unit{}, &schema.Unit{}}}, Response: &schema.Ref{Module: "builtin", Name: "HttpResponse", TypeParameters: []schema.Type{&schema.String{}, &schema.String{}}}, Metadata: []schema.Metadata{ &schema.MetadataIngress{Type: "http", Method: "GET", Path: []schema.IngressPathComponent{&schema.IngressPathLiteral{Text: "test"}}}, @@ -107,7 +108,7 @@ verb Echo(foo.EchoRequest) foo.EchoResponse` func TestVerbSchemaStringIngress(t *testing.T) { verb := &schema.Verb{ Name: "Ingress", - Request: &schema.Ref{Module: "builtin", Name: "HttpRequest", TypeParameters: []schema.Type{&schema.Ref{Module: "foo", Name: "FooRequest"}}}, + Request: &schema.Ref{Module: "builtin", Name: "HttpRequest", TypeParameters: []schema.Type{&schema.Ref{Module: "foo", Name: "FooRequest"}, &schema.Unit{}, &schema.Unit{}}}, Response: &schema.Ref{Module: "builtin", Name: "HttpResponse", TypeParameters: []schema.Type{&schema.Ref{Module: "foo", Name: "FooResponse"}, &schema.String{}}}, Metadata: []schema.Metadata{ &schema.MetadataIngress{Type: "http", Method: "GET", Path: []schema.IngressPathComponent{&schema.IngressPathLiteral{Text: "foo"}}}, @@ -135,11 +136,11 @@ func TestVerbSchemaStringIngress(t *testing.T) { } expected := `// HTTP request structure used for HTTP ingress verbs. -export data HttpRequest { +export data HttpRequest { method String path String - pathParameters {String: String} - query {String: [String]} + pathParameters Path + query Query headers {String: [String]} body Body } @@ -161,7 +162,7 @@ data FooResponse { Message String } -verb Ingress(builtin.HttpRequest) builtin.HttpResponse +verb Ingress(builtin.HttpRequest) builtin.HttpResponse +ingress http GET /foo` schemaString, err := verbSchemaString(sch, verb) diff --git a/backend/controller/ingress/handler_test.go b/backend/controller/ingress/handler_test.go index 7251e88860..4c21fd4c6d 100644 --- a/backend/controller/ingress/handler_test.go +++ b/backend/controller/ingress/handler_test.go @@ -45,16 +45,16 @@ func TestIngress(t *testing.T) { foo String } - export verb getAlias(HttpRequest) HttpResponse + export verb getAlias(HttpRequest) HttpResponse +ingress http GET /getAlias - export verb getPath(HttpRequest) HttpResponse + export verb getPath(HttpRequest) HttpResponse +ingress http GET /getPath/{username} - export verb postMissingTypes(HttpRequest) HttpResponse + export verb postMissingTypes(HttpRequest) HttpResponse +ingress http POST /postMissingTypes - export verb postJsonPayload(HttpRequest) HttpResponse + export verb postJsonPayload(HttpRequest) HttpResponse +ingress http POST /postJsonPayload } `) diff --git a/backend/controller/ingress/ingress.go b/backend/controller/ingress/ingress.go index bfc7e6dc0e..8c761fa93d 100644 --- a/backend/controller/ingress/ingress.go +++ b/backend/controller/ingress/ingress.go @@ -67,21 +67,21 @@ func ValidateCallBody(body []byte, verb *schema.Verb, sch *schema.Schema) error return nil } -func getBodyField(ref *schema.Ref, sch *schema.Schema) (*schema.Field, error) { +func getField(name string, ref *schema.Ref, sch *schema.Schema) (*schema.Field, error) { data, err := sch.ResolveMonomorphised(ref) if err != nil { return nil, err } var bodyField *schema.Field for _, field := range data.Fields { - if field.Name == "body" { + if field.Name == name { bodyField = field break } } if bodyField == nil { - return nil, fmt.Errorf("verb %s must have a 'body' field", ref.Name) + return nil, fmt.Errorf("verb %s must have a '%s' field", ref.Name, name) } return bodyField, nil diff --git a/backend/controller/ingress/request.go b/backend/controller/ingress/request.go index 0885c35c7b..aa316039b4 100644 --- a/backend/controller/ingress/request.go +++ b/backend/controller/ingress/request.go @@ -34,12 +34,16 @@ func BuildRequestBody(route *dal.IngressRoute, r *http.Request, sch *schema.Sche var requestMap map[string]any if metadata, ok := verb.GetMetadataIngress().Get(); ok && metadata.Type == "http" { - pathParameters := map[string]any{} + pathParametersMap := map[string]string{} matchSegments(route.Path, r.URL.Path, func(segment, value string) { - pathParameters[segment] = value + pathParametersMap[segment] = value }) + pathParameters, err := manglePathParameters(pathParametersMap, request, sch) + if err != nil { + return nil, err + } - httpRequestBody, err := extractHTTPRequestBody(route, r, request, sch) + httpRequestBody, err := extractHTTPRequestBody(r, request, sch) if err != nil { return nil, err } @@ -56,6 +60,7 @@ func BuildRequestBody(route *dal.IngressRoute, r *http.Request, sch *schema.Sche queryMap[key] = valuesAny } + finalQueryParams, err := mangleQueryParameters(queryMap, r.URL.Query(), request, sch) headerMap := make(map[string]any) for key, values := range r.Header { valuesAny := make([]any, len(values)) @@ -69,15 +74,16 @@ func BuildRequestBody(route *dal.IngressRoute, r *http.Request, sch *schema.Sche requestMap["method"] = r.Method requestMap["path"] = r.URL.Path requestMap["pathParameters"] = pathParameters - requestMap["query"] = queryMap + requestMap["query"] = finalQueryParams requestMap["headers"] = headerMap requestMap["body"] = httpRequestBody } else { - var err error - requestMap, err = buildRequestMap(route, r, request, sch) - if err != nil { - return nil, err - } + return nil, fmt.Errorf("no HTTP ingress metadata for verb %s", verb.Name) + //var err error + //requestMap, err = buildRequestMap(route, r, request, sch) + //if err != nil { + // return nil, err + //} } requestMap, err = schema.TransformFromAliasedFields(request, sch, requestMap) @@ -102,15 +108,15 @@ func BuildRequestBody(route *dal.IngressRoute, r *http.Request, sch *schema.Sche return body, nil } -func extractHTTPRequestBody(route *dal.IngressRoute, r *http.Request, ref *schema.Ref, sch *schema.Schema) (any, error) { - bodyField, err := getBodyField(ref, sch) +func extractHTTPRequestBody(r *http.Request, ref *schema.Ref, sch *schema.Schema) (any, error) { + bodyField, err := getField("body", ref, sch) if err != nil { return nil, err } if ref, ok := bodyField.Type.(*schema.Ref); ok { if err := sch.ResolveToType(ref, &schema.Data{}); err == nil { - return buildRequestMap(route, r, ref, sch) + return buildRequestMap(r) } } @@ -122,6 +128,81 @@ func extractHTTPRequestBody(route *dal.IngressRoute, r *http.Request, ref *schem return valueForData(bodyField.Type, bodyData) } +// Takes the map of path parameters and transforms them into the appropriate type +func manglePathParameters(params map[string]string, ref *schema.Ref, sch *schema.Schema) (any, error) { + + paramsField, err := getField("pathParameters", ref, sch) + if err != nil { + return nil, err + } + + switch paramsField.Type.(type) { + case *schema.Ref, *schema.Map: + ret := map[string]any{} + for k, v := range params { + ret[k] = v + } + return ret, nil + + } + // 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, err + 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, err + 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) + } + } + // Empty map + return map[string]any{}, nil +} + +// Takes the map of path 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) { + + paramsField, err := getField("query", ref, sch) + if err != nil { + return nil, err + } + + switch paramsField.Type.(type) { + + // If the type is Map it might be a map of lists for multi values params + case *schema.Map: + m := paramsField.Type.(*schema.Map) + switch m.Value.(type) { + case *schema.Array: + return params, 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] + } + } + return newParams, nil +} + func valueForData(typ schema.Type, data []byte) (any, error) { switch typ.(type) { case *schema.Ref: @@ -203,12 +284,7 @@ func readRequestBody(r *http.Request) ([]byte, error) { return bodyData, nil } -func buildRequestMap(route *dal.IngressRoute, r *http.Request, ref *schema.Ref, sch *schema.Schema) (map[string]any, error) { - requestMap := map[string]any{} - matchSegments(route.Path, r.URL.Path, func(segment, value string) { - requestMap[segment] = value - }) - +func buildRequestMap(r *http.Request) (map[string]any, error) { switch r.Method { case http.MethodPost, http.MethodPut: var bodyMap map[string]any @@ -217,29 +293,10 @@ func buildRequestMap(route *dal.IngressRoute, r *http.Request, ref *schema.Ref, return nil, fmt.Errorf("HTTP request body is not valid JSON: %w", err) } - // Merge bodyMap into params - for k, v := range bodyMap { - requestMap[k] = v - } + return bodyMap, nil default: - symbol, err := sch.ResolveRequestResponseType(ref) - if err != nil { - return nil, err - } - - if data, ok := symbol.(*schema.Data); ok { - queryMap, err := parseQueryParams(r.URL.Query(), data) - if err != nil { - return nil, fmt.Errorf("HTTP query params are not valid: %w", err) - } - - for key, value := range queryMap { - requestMap[key] = value - } - } + return nil, nil } - - return requestMap, nil } func parseQueryParams(values url.Values, data *schema.Data) (map[string]any, error) { diff --git a/backend/controller/ingress/request_test.go b/backend/controller/ingress/request_test.go index dec12964ef..af3363f275 100644 --- a/backend/controller/ingress/request_test.go +++ b/backend/controller/ingress/request_test.go @@ -41,13 +41,13 @@ type PostJSONPayload struct { } // HTTPRequest mirrors builtin.HttpRequest. -type HTTPRequest[Body any] struct { +type HTTPRequest[Body any, Path any, Query any] struct { Body Body Headers map[string][]string `json:"headers,omitempty"` Method string Path string - PathParameters map[string]string `json:"pathParameters,omitempty"` - Query map[string][]string `json:"query,omitempty"` + PathParameters Path `json:"pathParameters,omitempty"` + Query Query `json:"query,omitempty"` } func TestBuildRequestBody(t *testing.T) { @@ -77,20 +77,29 @@ func TestBuildRequestBody(t *testing.T) { foo String } - export verb getAlias(HttpRequest) HttpResponse + export verb getAlias(HttpRequest) HttpResponse +ingress http GET /getAlias - export verb getPath(HttpRequest) HttpResponse + export verb getPath(HttpRequest) HttpResponse +ingress http GET /getPath/{username} - export verb optionalQuery(HttpRequest) HttpResponse + export verb optionalQuery(HttpRequest) HttpResponse +ingress http GET /optionalQuery - export verb postMissingTypes(HttpRequest) HttpResponse + export verb postMissingTypes(HttpRequest) HttpResponse +ingress http POST /postMissingTypes - export verb postJsonPayload(HttpRequest) HttpResponse + export verb postJsonPayload(HttpRequest) HttpResponse +ingress http POST /postJsonPayload + + export verb getById(HttpRequest) HttpResponse + +ingress http GET /getbyid/{id} + + export verb mapQuery(HttpRequest) HttpResponse + +ingress http GET /mapQuery + + export verb multiMapQuery(HttpRequest) HttpResponse + +ingress http GET /multiMapQuery } `) assert.NoError(t, err) @@ -119,13 +128,10 @@ func TestBuildRequestBody(t *testing.T) { query: map[string][]string{ "alias": {"value"}, }, - expected: HTTPRequest[AliasRequest]{ + expected: HTTPRequest[ftl.Unit, map[string]string, AliasRequest]{ Method: "GET", Path: "/getAlias", - Query: map[string][]string{ - "alias": {"value"}, - }, - Body: AliasRequest{ + Query: AliasRequest{ Aliased: "value", }, }, @@ -135,7 +141,7 @@ func TestBuildRequestBody(t *testing.T) { method: "POST", path: "/postMissingTypes", routePath: "/postMissingTypes", - expected: HTTPRequest[MissingTypes]{ + expected: HTTPRequest[MissingTypes, map[string]string, map[string][]string]{ Method: "POST", Path: "/postMissingTypes", Body: MissingTypes{}, @@ -147,7 +153,7 @@ func TestBuildRequestBody(t *testing.T) { path: "/postJsonPayload", routePath: "/postJsonPayload", body: obj{"foo": "bar"}, - expected: HTTPRequest[PostJSONPayload]{ + expected: HTTPRequest[PostJSONPayload, map[string]string, map[string][]string]{ Method: "POST", Path: "/postJsonPayload", Body: PostJSONPayload{Foo: "bar"}, @@ -161,13 +167,10 @@ func TestBuildRequestBody(t *testing.T) { query: map[string][]string{ "foo": {"bar"}, }, - expected: HTTPRequest[QueryParameterRequest]{ + expected: HTTPRequest[map[string]string, map[string][]string, QueryParameterRequest]{ Method: "GET", Path: "/optionalQuery", - Query: map[string][]string{ - "foo": {"bar"}, - }, - Body: QueryParameterRequest{ + Query: QueryParameterRequest{ Foo: ftl.Some("bar"), }, }, @@ -177,17 +180,53 @@ func TestBuildRequestBody(t *testing.T) { method: "GET", path: "/getPath/bob", routePath: "/getPath/{username}", - expected: HTTPRequest[PathParameterRequest]{ + expected: HTTPRequest[ftl.Unit, PathParameterRequest, map[string][]string]{ Method: "GET", Path: "/getPath/bob", - PathParameters: map[string]string{ - "username": "bob", - }, - Body: PathParameterRequest{ + PathParameters: PathParameterRequest{ Username: "bob", }, }, }, + {name: "GetById", + verb: "getById", + method: "GET", + path: "/getbyid/100", + routePath: "/getbyid/{id}", + expected: HTTPRequest[ftl.Unit, int, ftl.Unit]{ + Method: "GET", + Path: "/getbyid/100", + PathParameters: 100, + }, + }, + {name: "MapQuery", + verb: "mapQuery", + method: "GET", + path: "/mapQuery", + routePath: "/mapQuery", + query: map[string][]string{ + "alias": {"value"}, + }, + expected: HTTPRequest[ftl.Unit, ftl.Unit, map[string]string]{ + Method: "GET", + Path: "/mapQuery", + Query: map[string]string{"alias": "value"}, + }, + }, + {name: "MultiMapQuery", + verb: "multiMapQuery", + method: "GET", + path: "/multiMapQuery", + routePath: "/multiMapQuery", + query: map[string][]string{ + "alias": {"value"}, + }, + expected: HTTPRequest[ftl.Unit, ftl.Unit, map[string][]string]{ + Method: "GET", + Path: "/multiMapQuery", + Query: map[string][]string{"alias": []string{"value"}}, + }, + }, } { t.Run(test.name, func(t *testing.T) { if test.body == nil { diff --git a/backend/schema/builtin.go b/backend/schema/builtin.go index 956a6f79f4..132cae0d69 100644 --- a/backend/schema/builtin.go +++ b/backend/schema/builtin.go @@ -7,11 +7,11 @@ const BuiltinsSource = ` // Built-in types for FTL. builtin module builtin { // HTTP request structure used for HTTP ingress verbs. - export data HttpRequest { + export data HttpRequest { method String path String - pathParameters {String: String} - query {String: [String]} + pathParameters Path + query Query headers {String: [String]} body Body } diff --git a/backend/schema/schema_test.go b/backend/schema/schema_test.go index 26cf338129..672fc1a215 100644 --- a/backend/schema/schema_test.go +++ b/backend/schema/schema_test.go @@ -50,7 +50,7 @@ module todo { +calls todo.destroy +database calls todo.testdb - export verb destroy(builtin.HttpRequest) builtin.HttpResponse + export verb destroy(builtin.HttpRequest) builtin.HttpResponse +ingress http GET /todo/destroy/{name} verb mondays(Unit) Unit @@ -192,7 +192,9 @@ Module Ref Verb Ref + Unit Ref + Unit Ref Ref String @@ -299,14 +301,14 @@ func TestParsing(t *testing.T) { input: `module int { data String { name String } verb verb(String) String }`, errors: []string{"1:14-14: data name \"String\" is a reserved word"}}, {name: "BuiltinRef", - input: `module test { verb myIngress(HttpRequest) HttpResponse }`, + input: `module test { verb myIngress(HttpRequest) HttpResponse }`, expected: &Schema{ Modules: []*Module{{ Name: "test", Decls: []Decl{ &Verb{ Name: "myIngress", - Request: &Ref{Module: "builtin", Name: "HttpRequest", TypeParameters: []Type{&String{}}}, + Request: &Ref{Module: "builtin", Name: "HttpRequest", TypeParameters: []Type{&String{}, &Unit{}, &Unit{}}}, Response: &Ref{Module: "builtin", Name: "HttpResponse", TypeParameters: []Type{&String{}, &String{}}}, }, }, @@ -324,7 +326,7 @@ func TestParsing(t *testing.T) { message String } - export verb echo(builtin.HttpRequest) builtin.HttpResponse + export verb echo(builtin.HttpRequest) builtin.HttpResponse +ingress http GET /echo +calls time.time @@ -338,7 +340,7 @@ func TestParsing(t *testing.T) { time Time } - export verb time(builtin.HttpRequest) builtin.HttpResponse + export verb time(builtin.HttpRequest) builtin.HttpResponse +ingress http GET /time } `, @@ -351,7 +353,7 @@ func TestParsing(t *testing.T) { &Verb{ Name: "echo", Export: true, - Request: &Ref{Module: "builtin", Name: "HttpRequest", TypeParameters: []Type{&Ref{Module: "echo", Name: "EchoRequest"}}}, + Request: &Ref{Module: "builtin", Name: "HttpRequest", TypeParameters: []Type{&Ref{Module: "echo", Name: "EchoRequest"}, &Unit{}, &Unit{}}}, Response: &Ref{Module: "builtin", Name: "HttpResponse", TypeParameters: []Type{&Ref{Module: "echo", Name: "EchoResponse"}, &String{}}}, Metadata: []Metadata{ &MetadataIngress{Type: "http", Method: "GET", Path: []IngressPathComponent{&IngressPathLiteral{Text: "echo"}}}, @@ -367,7 +369,7 @@ func TestParsing(t *testing.T) { &Verb{ Name: "time", Export: true, - Request: &Ref{Module: "builtin", Name: "HttpRequest", TypeParameters: []Type{&Ref{Module: "time", Name: "TimeRequest"}}}, + Request: &Ref{Module: "builtin", Name: "HttpRequest", TypeParameters: []Type{&Ref{Module: "time", Name: "TimeRequest"}, &Unit{}, &Unit{}}}, Response: &Ref{Module: "builtin", Name: "HttpResponse", TypeParameters: []Type{&Ref{Module: "time", Name: "TimeResponse"}, &String{}}}, Metadata: []Metadata{ &MetadataIngress{Type: "http", Method: "GET", Path: []IngressPathComponent{&IngressPathLiteral{Text: "time"}}}, @@ -833,7 +835,7 @@ module todo { } export verb create(todo.CreateRequest) todo.CreateResponse +calls todo.destroy +database calls todo.testdb - export verb destroy(builtin.HttpRequest) builtin.HttpResponse + export verb destroy(builtin.HttpRequest) builtin.HttpResponse +ingress http GET /todo/destroy/{name} verb scheduled(Unit) Unit +cron */10 * * 1-10,11-31 * * * @@ -944,7 +946,7 @@ var testSchema = MustValidate(&Schema{ }}, &Verb{Name: "destroy", Export: true, - Request: &Ref{Module: "builtin", Name: "HttpRequest", TypeParameters: []Type{&Ref{Module: "todo", Name: "DestroyRequest"}}}, + Request: &Ref{Module: "builtin", Name: "HttpRequest", TypeParameters: []Type{&Unit{}, &Ref{Module: "todo", Name: "DestroyRequest"}, &Unit{}}}, Response: &Ref{Module: "builtin", Name: "HttpResponse", TypeParameters: []Type{&Ref{Module: "todo", Name: "DestroyResponse"}, &String{}}}, Metadata: []Metadata{ &MetadataIngress{ diff --git a/backend/schema/validate.go b/backend/schema/validate.go index 3d22e978e7..a8d7636a40 100644 --- a/backend/schema/validate.go +++ b/backend/schema/validate.go @@ -547,24 +547,52 @@ func validateVerbMetadata(scopes Scopes, module *Module, n *Verb) (merr []error) switch md := md.(type) { case *MetadataIngress: - reqBodyType, reqBody, errs := validateIngressRequestOrResponse(scopes, module, n, "request", n.Request) + reqInfo, errs := validateIngressRequest(scopes, module, n, "request", n.Request) merr = append(merr, errs...) - _, _, errs = validateIngressRequestOrResponse(scopes, module, n, "response", n.Response) + errs = validateIngressResponse(scopes, module, n, "response", n.Response) merr = append(merr, errs...) - // Validate path - for _, path := range md.Path { - switch path := path.(type) { - case *IngressPathParameter: - reqBodyData, ok := reqBody.(*Data) + if reqInfo.pathParamSymbol != nil { + // If this is nil it has already failed validation + + hasParameters := false + // Validate path + for _, path := range md.Path { + switch path := path.(type) { + case *IngressPathParameter: + hasParameters = true + switch reqInfo.pathParamSymbol.(type) { + case *Data: + if reqInfo.pathParamSymbol.(*Data).FieldByName(path.Name) == nil { + merr = append(merr, errorf(path, "ingress verb %s: request pathParameter type %s does not contain a field corresponding to the parameter %q", n.Name, reqInfo.pathParamType, path.Name)) + } + case *Map: + // No validation for map, it is always fine + case *String, *Int, *Bool, *Float: + // Only valid for a single path parameter + count := 0 + for _, p := range md.Path { + switch p.(type) { + case *IngressPathParameter: + count++ + } + } + if count != 1 { + merr = append(merr, errorf(path, "ingress verb %s: cannot use path parameter %q with request type %s as it has multiple path parameters, expected Data or Map type", n.Name, path.Name, reqInfo.pathParamType)) + } + default: + merr = append(merr, errorf(path, "ingress verb %s: cannot use path parameter %q with request type %s, expected Data or Map type", n.Name, path.Name, reqInfo.pathParamType)) + } + case *IngressPathLiteral: + } + } + if !hasParameters { + _, ok := reqInfo.pathParamSymbol.(*Unit) if !ok { - merr = append(merr, errorf(path, "ingress verb %s: cannot use path parameter %q with request type %s, expected Data type", n.Name, path.Name, reqBodyType)) - } else if reqBodyData.FieldByName(path.Name) == nil { - merr = append(merr, errorf(path, "ingress verb %s: request type %s does not contain a field corresponding to the parameter %q", n.Name, reqBodyType, path.Name)) + merr = append(merr, errorf(reqInfo.pathParamSymbol, "ingress verb %s: cannot use path parameter type %s, expected Unit as ingress has no path parameters", n.Name, reqInfo.pathParamType)) } - - case *IngressPathLiteral: } + } case *MetadataCronJob: _, err := cron.Parse(md.Cron) @@ -613,7 +641,16 @@ func validateVerbMetadata(scopes Scopes, module *Module, n *Verb) (merr []error) return } -func validateIngressRequestOrResponse(scopes Scopes, module *Module, n *Verb, reqOrResp string, r Type) (fieldType Type, body Symbol, merr []error) { +type HttpRequestExtractedTypes struct { + fieldType Type + body Symbol + pathParamType Type + pathParamSymbol Symbol + queryParamType Type + queryParamSymbol Symbol +} + +func validateIngressResponse(scopes Scopes, module *Module, n *Verb, reqOrResp string, r Type) (merr []error) { data, err := resolveValidIngressReqResp(scopes, reqOrResp, optional.None[*ModuleDecl](), r, nil) if err != nil { merr = append(merr, errorf(r, "ingress verb %s: %s type %s: %v", n.Name, reqOrResp, r, err)) @@ -621,12 +658,50 @@ func validateIngressRequestOrResponse(scopes Scopes, module *Module, n *Verb, re } resp, ok := data.Get() if !ok { - merr = append(merr, errorf(r, "ingress verb %s: %s type %s must be builtin.HttpRequest", n.Name, reqOrResp, r)) + merr = append(merr, errorf(r, "ingress verb %s: %s type %s must be builtin.HttpResponse", n.Name, reqOrResp, r)) return } scopes = scopes.PushScope(resp.Scope()) - fieldType = resp.FieldByName("body").Type + + _, _, merr = validateParam(resp, "body", scopes, module, n, reqOrResp, r, validateBodyPayloadType) + return +} + +func validateIngressRequest(scopes Scopes, module *Module, n *Verb, reqOrResp string, r Type) (result HttpRequestExtractedTypes, merr []error) { + data, err := resolveValidIngressReqResp(scopes, reqOrResp, optional.None[*ModuleDecl](), r, nil) + if err != nil { + merr = append(merr, errorf(r, "ingress verb %s: %s type %s: %v", n.Name, reqOrResp, r, err)) + return + } + resp, ok := data.Get() + isRequest := reqOrResp == "request" + if !ok { + if isRequest { + merr = append(merr, errorf(r, "ingress verb %s: %s type %s must be builtin.HttpRequest", n.Name, reqOrResp, r)) + } else { + merr = append(merr, errorf(r, "ingress verb %s: %s type %s must be builtin.HttpResponse", n.Name, reqOrResp, r)) + } + return + } + + scopes = scopes.PushScope(resp.Scope()) + + var errs []error + result.fieldType, result.body, errs = validateParam(resp, "body", scopes, module, n, reqOrResp, r, validateBodyPayloadType) + merr = append(merr, errs...) + if isRequest { + result.pathParamType, result.pathParamSymbol, errs = validateParam(resp, "pathParameters", scopes, module, n, reqOrResp, r, validatePathParamsPayloadType) + merr = append(merr, errs...) + + result.queryParamType, result.queryParamSymbol, errs = validateParam(resp, "query", scopes, module, n, reqOrResp, r, validateQueryParamsPayloadType) + merr = append(merr, errs...) + } + return +} + +func validateParam(resp *Data, paramName string, scopes Scopes, module *Module, n *Verb, reqOrResp string, r Type, validationFunc func(Node, Type, *Verb, string) error) (fieldType Type, body Symbol, merr []error) { + fieldType = resp.FieldByName(paramName).Type if opt, ok := fieldType.(*Optional); ok { fieldType = opt.Type } @@ -643,7 +718,7 @@ func validateIngressRequestOrResponse(scopes Scopes, module *Module, n *Verb, re return } body = bodySym.Symbol - err = validatePayloadType(bodySym.Symbol, r, n, reqOrResp) + err := validationFunc(bodySym.Symbol, r, n, reqOrResp) if err != nil { merr = append(merr, err) } @@ -689,7 +764,7 @@ func resolveValidIngressReqResp(scopes Scopes, reqOrResp string, moduleDecl opti } } -func validatePayloadType(n Node, r Type, v *Verb, reqOrResp string) error { +func validateBodyPayloadType(n Node, r Type, v *Verb, reqOrResp string) error { switch t := n.(type) { case *Bytes, *String, *Data, *Unit, *Float, *Int, *Bool, *Map, *Array: // Valid HTTP response payload types. case *TypeAlias: @@ -697,7 +772,7 @@ func validatePayloadType(n Node, r Type, v *Verb, reqOrResp string) error { if len(t.Metadata) > 0 { return nil } - return validatePayloadType(t.Type, r, v, reqOrResp) + return validateBodyPayloadType(t.Type, r, v, reqOrResp) case *Enum: // Type enums are valid but value enums are not. if t.IsValueEnum() { @@ -709,6 +784,36 @@ func validatePayloadType(n Node, r Type, v *Verb, reqOrResp string) error { return nil } +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 *TypeAlias: + // allow aliases of external types + if len(t.Metadata) > 0 { + return nil + } + return validatePathParamsPayloadType(t.Type, r, v, reqOrResp) + 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 +} + +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 *TypeAlias: + // allow aliases of external types + if len(t.Metadata) > 0 { + return nil + } + return validateQueryParamsPayloadType(t.Type, r, v, reqOrResp) + 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 +} + func validateVerbSubscriptions(module *Module, v *Verb, md *MetadataSubscriber, scopes Scopes, schema optional.Option[*Schema]) (merr []error) { merr = []error{} var subscription *Subscription diff --git a/backend/schema/validate_test.go b/backend/schema/validate_test.go index fd8215bfda..5110ad8029 100644 --- a/backend/schema/validate_test.go +++ b/backend/schema/validate_test.go @@ -92,7 +92,7 @@ func TestValidate(t *testing.T) { {name: "ValidIngressRequestType", schema: ` module one { - export verb a(HttpRequest) HttpResponse + export verb a(HttpRequest) HttpResponse +ingress http GET /a } `}, @@ -105,26 +105,28 @@ func TestValidate(t *testing.T) { `, errs: []string{ "3:20-20: ingress verb a: request type Empty must be builtin.HttpRequest", - "3:27-27: ingress verb a: response type Empty must be builtin.HttpRequest", + "3:27-27: ingress verb a: response type Empty must be builtin.HttpResponse", }}, {name: "IngressBodyTypes", schema: ` module one { - export verb bytes(HttpRequest) HttpResponse + export verb bytes(HttpRequest) HttpResponse +ingress http GET /bytes - export verb string(HttpRequest) HttpResponse + export verb string(HttpRequest) HttpResponse +ingress http GET /string - export verb data(HttpRequest) HttpResponse + export verb data(HttpRequest) HttpResponse +ingress http GET /data // Invalid types. - export verb any(HttpRequest) HttpResponse + export verb any(HttpRequest) HttpResponse +ingress http GET /any - export verb path(HttpRequest) HttpResponse + export verb path(HttpRequest) HttpResponse +ingress http GET /path/{invalid} - export verb pathMissing(HttpRequest) HttpResponse + export verb pathInvalid(HttpRequest) HttpResponse + +ingress http GET /path/{invalid}/{extra} + export verb pathMissing(HttpRequest) HttpResponse +ingress http GET /path/{missing} - export verb pathFound(HttpRequest) HttpResponse + export verb pathFound(HttpRequest) HttpResponse +ingress http GET /path/{parameter} // Data comment @@ -134,18 +136,19 @@ func TestValidate(t *testing.T) { } `, errs: []string{ - "11:22-22: ingress verb any: request type HttpRequest must have a body of bytes, string, data structure, unit, float, int, bool, map, or array not Any", - "11:40-40: ingress verb any: response type HttpResponse must have a body of bytes, string, data structure, unit, float, int, bool, map, or array not Any", - "14:31-31: ingress verb path: cannot use path parameter \"invalid\" with request type String, expected Data type", - "16:31-31: ingress verb pathMissing: request type one.Path does not contain a field corresponding to the parameter \"missing\"", - "16:7-7: duplicate http ingress GET /path/{} for 17:6:\"pathFound\" and 15:6:\"pathMissing\"", - "18:7-7: duplicate http ingress GET /path/{} for 13:6:\"path\" and 17:6:\"pathFound\"", + "11:22-22: ingress verb any: request type HttpRequest must have a body of bytes, string, data structure, unit, float, int, bool, map, or array not Any", + "11:52-52: ingress verb any: response type HttpResponse must have a body of bytes, string, data structure, unit, float, int, bool, map, or array not Any", + "16:31-31: ingress verb pathInvalid: cannot use path parameter \"invalid\" with request type String as it has multiple path parameters, expected Data or Map type", + "16:41-41: ingress verb pathInvalid: cannot use path parameter \"extra\" with request type String as it has multiple path parameters, expected Data or Map type", + "18:31-31: ingress verb pathMissing: request pathParameter type one.Path does not contain a field corresponding to the parameter \"missing\"", + "18:7-7: duplicate http ingress GET /path/{} for 19:6:\"pathFound\" and 17:6:\"pathMissing\"", + "20:7-7: duplicate http ingress GET /path/{} for 13:6:\"path\" and 19:6:\"pathFound\"", }}, {name: "Array", schema: ` module one { data Data {} - export verb one(HttpRequest<[one.Data]>) HttpResponse<[one.Data], Empty> + export verb one(HttpRequest<[one.Data],Unit, Unit>) HttpResponse<[one.Data], Empty> +ingress http GET /one } `, @@ -166,7 +169,7 @@ func TestValidate(t *testing.T) { schema: ` module one { data Data {} - export verb one(HttpRequest<[one.Data]>) HttpResponse<[one.Data], Empty> + export verb one(HttpRequest<[one.Data], Unit, Unit>) HttpResponse<[one.Data], Empty> +ingress http GET /one +ingress http GET /two } @@ -195,7 +198,7 @@ func TestValidate(t *testing.T) { export data Data {} } module one { - export verb a(HttpRequest) HttpResponse + export verb a(HttpRequest) HttpResponse +ingress http GET /a } `,