From d619b4aba48968580cfb797cc212d242da0475e1 Mon Sep 17 00:00:00 2001 From: Alec Thomas Date: Fri, 9 Feb 2024 16:47:16 +1100 Subject: [PATCH] feat: change HttpResponse to HttpResponse This allows for seperate HTTP response structures for valid responses and errors, codifying the best practice of having a distinct error response structure. Fixed a bug where sdk.Option was not using encoding.Marshal(). Refactored ingress response construction such that headers and payload are entirely handled by a single function, ResponseForVerb. --- Bitfile | 6 +- backend/controller/controller.go | 20 +--- backend/controller/ingress/ingress.go | 45 --------- backend/controller/ingress/ingress_test.go | 46 +++++---- backend/controller/ingress/response.go | 99 +++++++++++++++---- backend/schema/builtin.go | 6 +- backend/schema/data.go | 17 +++- backend/schema/data_test.go | 2 +- backend/schema/jsonschema.go | 2 +- backend/schema/schema.go | 2 +- backend/schema/schema_test.go | 21 ++-- .../src/main/kotlin/ftl/ad/Ad.kt | 2 +- go-runtime/encoding/encoding.go | 2 + go-runtime/encoding/encoding_test.go | 4 + go-runtime/sdk/option.go | 4 +- integration/integration_test.go | 23 ++--- integration/testdata/go/httpingress/echo.go | 56 +++++------ .../testdata/kotlin/httpingress/Echo.kt | 44 ++++----- .../ftl/schemaextractor/ExtractSchemaRule.kt | 2 +- .../schemaextractor/ExtractSchemaRuleTest.kt | 9 +- .../dependencies/BuiltinModuleClient.kt | 8 +- 21 files changed, 227 insertions(+), 193 deletions(-) diff --git a/Bitfile b/Bitfile index 15e0c5020a..f75b820639 100644 --- a/Bitfile +++ b/Bitfile @@ -94,7 +94,7 @@ go-runtime/compile/build-template.zip: go-runtime/compile/build-template/**/* cd go-runtime/compile/build-template build: zip -q --symlinks -r ../build-template.zip . -go-runtime/compile/external-module-template.zip: go-runtime/compile/external-module-template/**/* +go-runtime/compile/external-module-template.zip: go-runtime/compile/external-module-template/**/* cd go-runtime/compile/external-module-template build: zip -q --symlinks -r ../external-module-template.zip . @@ -102,7 +102,7 @@ kotlin-runtime/scaffolding.zip: kotlin-runtime/scaffolding/**/* cd kotlin-runtime/scaffolding build: zip -q --symlinks -r ../scaffolding.zip . -%{SCHEMA_OUT}: %{SCHEMA_IN} +%{SCHEMA_OUT}: %{SCHEMA_IN} build: ftl-schema > %{OUT} buf format -w %{OUT} @@ -114,7 +114,7 @@ kotlin-runtime/scaffolding.zip: kotlin-runtime/scaffolding/**/* (cd protos && buf generate) (cd backend/common/3rdparty/protos && buf generate) # There's a build cycle dependency here, so we can't clean: ftl-schema depends on generated .pb.go files - -clean + -clean %{KT_RUNTIME_OUT}: %{KT_RUNTIME_IN} %{PROTO_IN} build: diff --git a/backend/controller/controller.go b/backend/controller/controller.go index eeed7085b3..8d9ca20f61 100644 --- a/backend/controller/controller.go +++ b/backend/controller/controller.go @@ -169,12 +169,6 @@ func New(ctx context.Context, db *dal.DAL, config Config, runnerScaling scaling. return svc, nil } -type HTTPResponse struct { - Status int `json:"status"` - Headers map[string][]string `json:"headers"` - Body json.RawMessage `json:"body"` -} - // ServeHTTP handles ingress routes. func (s *Service) ServeHTTP(w http.ResponseWriter, r *http.Request) { logger := log.FromContext(r.Context()) @@ -242,26 +236,20 @@ func (s *Service) ServeHTTP(w http.ResponseWriter, r *http.Request) { var responseBody []byte if metadata, ok := verb.GetMetadataIngress().Get(); ok && metadata.Type == "http" { - var response HTTPResponse + var response ingress.HTTPResponse if err := json.Unmarshal(msg.Body, &response); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return } - verbResponse := verb.Response.(*schema.DataRef) //nolint:forcetypeassert - err = ingress.ValidateContentType(verbResponse, sch, response.Headers) - if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - - responseBody, err = ingress.ResponseBodyForVerb(sch, verb, response.Body, response.Headers) + var responseHeaders http.Header + responseBody, responseHeaders, err = ingress.ResponseForVerb(sch, verb, response) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return } - for k, v := range response.Headers { + for k, v := range responseHeaders { w.Header()[k] = v } diff --git a/backend/controller/ingress/ingress.go b/backend/controller/ingress/ingress.go index 5918b96e65..3267e5c433 100644 --- a/backend/controller/ingress/ingress.go +++ b/backend/controller/ingress/ingress.go @@ -58,51 +58,6 @@ func matchSegments(pattern, urlPath string, onMatch func(segment, value string)) return true } -func ValidateContentType(dataRef *schema.DataRef, sch *schema.Schema, headers map[string][]string) error { - bodyField, err := getBodyField(dataRef, sch) - if err != nil { - return err - } - - _, hasContentType := headers["Content-Type"] - if !hasContentType { - defaultType := getDefaultContentType(bodyField) - if defaultType == "" { - return nil // No content type is required - } - headers["Content-Type"] = []string{defaultType} - } - - contentType := headers["Content-Type"][0] - switch bodyField.Type.(type) { - case *schema.String, *schema.Int, *schema.Float, *schema.Bool: - if !strings.HasPrefix(contentType, "text/") { - return fmt.Errorf("expected text content type, got %s", contentType) - } - case *schema.DataRef, *schema.Map, *schema.Array: - if !strings.HasPrefix(contentType, "application/json") { - return fmt.Errorf("expected application/json content type, got %s", contentType) - } - default: - return nil - } - - return nil -} - -func getDefaultContentType(bodyField *schema.Field) string { - switch bodyField.Type.(type) { - case *schema.Bytes: - return "application/octet-stream" - case *schema.String, *schema.Int, *schema.Float, *schema.Bool: - return "text/plain; charset=utf-8" - case *schema.DataRef, *schema.Map, *schema.Array: - return "application/json; charset=utf-8" - default: - return "" - } -} - func ValidateCallBody(body []byte, verbRef *schema.VerbRef, sch *schema.Schema) error { verb := sch.ResolveVerbRef(verbRef) if verb == nil { diff --git a/backend/controller/ingress/ingress_test.go b/backend/controller/ingress/ingress_test.go index 3112bf7cd4..b850261654 100644 --- a/backend/controller/ingress/ingress_test.go +++ b/backend/controller/ingress/ingress_test.go @@ -1,6 +1,7 @@ package ingress import ( + "net/http" "net/url" "testing" @@ -146,16 +147,17 @@ func TestResponseBodyForVerb(t *testing.T) { Name: "Json", Response: &schema.DataRef{Module: "builtin", Name: "HttpResponse", TypeParameters: []schema.Type{ &schema.DataRef{ - Module: "test", - Name: "Test", - TypeParameters: []schema.Type{}, + Module: "test", + Name: "Test", }, + &schema.String{}, }}, } stringVerb := &schema.Verb{ Name: "String", Response: &schema.DataRef{Module: "builtin", Name: "HttpResponse", TypeParameters: []schema.Type{ &schema.String{}, + &schema.String{}, }}, } sch := &schema.Schema{ @@ -176,25 +178,28 @@ func TestResponseBodyForVerb(t *testing.T) { }, } tests := []struct { - name string - verb *schema.Verb - headers map[string][]string - body []byte - expectedBody []byte + name string + verb *schema.Verb + headers map[string][]string + body []byte + expectedBody []byte + expectedHeaders http.Header }{ { - name: "application/json", - verb: jsonVerb, - headers: map[string][]string{"Content-Type": {"application/json"}}, - body: []byte(`{"message": "Hello, World!"}`), - expectedBody: []byte(`{"msg":"Hello, World!"}`), + name: "application/json", + verb: jsonVerb, + headers: map[string][]string{"Content-Type": {"application/json"}}, + body: []byte(`{"message": "Hello, World!"}`), + expectedBody: []byte(`{"msg":"Hello, World!"}`), + expectedHeaders: http.Header{"Content-Type": []string{"application/json"}}, }, { - name: "Default to application/json", - verb: jsonVerb, - headers: map[string][]string{}, - body: []byte(`{"message": "Default to JSON"}`), - expectedBody: []byte(`{"msg":"Default to JSON"}`), + name: "Default to application/json", + verb: jsonVerb, + headers: map[string][]string{}, + body: []byte(`{"message": "Default to JSON"}`), + expectedBody: []byte(`{"msg":"Default to JSON"}`), + expectedHeaders: http.Header{"Content-Type": []string{"application/json; charset=utf-8"}}, }, { name: "text/html", @@ -207,9 +212,12 @@ func TestResponseBodyForVerb(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - result, err := ResponseBodyForVerb(sch, tc.verb, tc.body, tc.headers) + result, headers, err := ResponseForVerb(sch, tc.verb, HTTPResponse{Body: tc.body, Headers: tc.headers}) assert.NoError(t, err) assert.Equal(t, tc.expectedBody, result) + if tc.expectedHeaders != nil { + assert.Equal(t, tc.expectedHeaders, headers) + } }) } } diff --git a/backend/controller/ingress/response.go b/backend/controller/ingress/response.go index cf355e9ac7..0d1e7c41ef 100644 --- a/backend/controller/ingress/response.go +++ b/backend/controller/ingress/response.go @@ -1,82 +1,139 @@ package ingress import ( + "bytes" "encoding/base64" "encoding/json" "fmt" + "maps" + "net/http" "strconv" "github.com/TBD54566975/ftl/backend/schema" ) -func ResponseBodyForVerb(sch *schema.Schema, verb *schema.Verb, body []byte, headers map[string][]string) ([]byte, error) { +// HTTPResponse mirrors builtins.HttpResponse. +type HTTPResponse struct { + Status int `json:"status,omitempty"` + Headers map[string][]string `json:"headers,omitempty"` + Body json.RawMessage `json:"body,omitempty"` + Error json.RawMessage `json:"error,omitempty"` +} + +// ResponseForVerb returns the HTTP response for a given verb. +func ResponseForVerb(sch *schema.Schema, verb *schema.Verb, response HTTPResponse) ([]byte, http.Header, error) { responseRef, ok := verb.Response.(*schema.DataRef) if !ok { - return body, nil + return nil, nil, nil } - bodyField, err := getBodyField(responseRef, sch) + bodyData, err := sch.ResolveDataRefMonomorphised(responseRef) if err != nil { - return nil, err + return nil, nil, fmt.Errorf("failed to resolve response data type: %w", err) } - switch bodyType := bodyField.Type.(type) { + haveBody := response.Body != nil && !bytes.Equal(response.Body, []byte("null")) + haveError := response.Error != nil && !bytes.Equal(response.Error, []byte("null")) + + var fieldType schema.Type + var body []byte + + switch { + case haveBody == haveError: + return nil, nil, fmt.Errorf("response must have either a body or an error") + + case haveBody: + fieldType = bodyData.FieldByName("body").Type.(*schema.Optional).Type //nolint:forcetypeassert + body = response.Body + + case haveError: + fieldType = bodyData.FieldByName("error").Type.(*schema.Optional).Type //nolint:forcetypeassert + body = response.Error + } + + // Clone and canonicalise the headers. + headers := http.Header(maps.Clone(response.Headers)) + for k, v := range response.Headers { + headers[http.CanonicalHeaderKey(k)] = v + } + // If the Content-Type header is not set, set it to the default value for the response or error type. + if _, ok := headers["Content-Type"]; !ok { + if contentType := getDefaultContentType(fieldType); contentType != "" { + headers.Set("Content-Type", getDefaultContentType(fieldType)) + } + } + + switch bodyType := fieldType.(type) { case *schema.DataRef: var responseMap map[string]any err := json.Unmarshal(body, &responseMap) if err != nil { - return nil, fmt.Errorf("HTTP response body is not valid JSON: %w", err) + return nil, nil, fmt.Errorf("HTTP response body is not valid JSON: %w", err) } aliasedResponseMap, err := transformToAliasedFields(bodyType, sch, responseMap) if err != nil { - return nil, err + return nil, nil, err } - return json.Marshal(aliasedResponseMap) + outBody, err := json.Marshal(aliasedResponseMap) + return outBody, headers, err case *schema.Bytes: var base64String string if err := json.Unmarshal(body, &base64String); err != nil { - return nil, fmt.Errorf("HTTP response body is not valid base64: %w", err) + return nil, nil, fmt.Errorf("HTTP response body is not valid base64: %w", err) } decodedBody, err := base64.StdEncoding.DecodeString(base64String) if err != nil { - return nil, fmt.Errorf("failed to decode base64 response body: %w", err) + return nil, nil, fmt.Errorf("failed to decode base64 response body: %w", err) } - return decodedBody, nil + return decodedBody, headers, nil case *schema.String: var responseString string if err := json.Unmarshal(body, &responseString); err != nil { - return nil, fmt.Errorf("HTTP response body is not a valid string: %w", err) + return nil, nil, fmt.Errorf("HTTP response body is not a valid string: %w", err) } - return []byte(responseString), nil + return []byte(responseString), headers, nil case *schema.Int: var responseInt int if err := json.Unmarshal(body, &responseInt); err != nil { - return nil, fmt.Errorf("HTTP response body is not a valid int: %w", err) + return nil, nil, fmt.Errorf("HTTP response body is not a valid int: %w", err) } - return []byte(strconv.Itoa(responseInt)), nil + return []byte(strconv.Itoa(responseInt)), headers, nil case *schema.Float: var responseFloat float64 if err := json.Unmarshal(body, &responseFloat); err != nil { - return nil, fmt.Errorf("HTTP response body is not a valid float: %w", err) + return nil, nil, fmt.Errorf("HTTP response body is not a valid float: %w", err) } - return []byte(strconv.FormatFloat(responseFloat, 'f', -1, 64)), nil + return []byte(strconv.FormatFloat(responseFloat, 'f', -1, 64)), headers, nil case *schema.Bool: var responseBool bool if err := json.Unmarshal(body, &responseBool); err != nil { - return nil, fmt.Errorf("HTTP response body is not a valid bool: %w", err) + return nil, nil, fmt.Errorf("HTTP response body is not a valid bool: %w", err) } - return []byte(strconv.FormatBool(responseBool)), nil + return []byte(strconv.FormatBool(responseBool)), headers, nil case *schema.Unit: - return []byte{}, nil + return []byte{}, headers, nil + + default: + return body, headers, nil + } +} +func getDefaultContentType(typ schema.Type) string { + switch typ.(type) { + case *schema.Bytes: + return "application/octet-stream" + case *schema.String, *schema.Int, *schema.Float, *schema.Bool: + return "text/plain; charset=utf-8" + case *schema.DataRef, *schema.Map, *schema.Array: + return "application/json; charset=utf-8" default: - return body, nil + return "" } } diff --git a/backend/schema/builtin.go b/backend/schema/builtin.go index 38953ecb7b..803170ba53 100644 --- a/backend/schema/builtin.go +++ b/backend/schema/builtin.go @@ -17,10 +17,12 @@ builtin module builtin { } // HTTP response structure used for HTTP ingress verbs. - data HttpResponse { + data HttpResponse { status Int headers {String: [String]} - body Body + // Either "body" or "error" must be present, not both. + body Body? + error Error? } data Empty {} diff --git a/backend/schema/data.go b/backend/schema/data.go index fdae557c7e..3aacb5ea41 100644 --- a/backend/schema/data.go +++ b/backend/schema/data.go @@ -32,22 +32,31 @@ func (d *Data) Scope() Scope { return scope } +func (d *Data) FieldByName(name string) *Field { + for _, f := range d.Fields { + if f.Name == name { + return f + } + } + return nil +} + // Monomorphise this data type with the given type arguments. // // If this data type has no type parameters, it will be returned as-is. // // This will return a new Data structure with all type parameters replaced with // the given types. -func (d *Data) Monomorphise(types ...Type) (*Data, error) { - if len(d.TypeParameters) != len(types) { - return nil, fmt.Errorf("expected %d type arguments, got %d", len(d.TypeParameters), len(types)) +func (d *Data) Monomorphise(ref *DataRef) (*Data, error) { + if len(d.TypeParameters) != len(ref.TypeParameters) { + return nil, fmt.Errorf("%s: expected %d type arguments, got %d", ref.Pos, len(d.TypeParameters), len(ref.TypeParameters)) } if len(d.TypeParameters) == 0 { return d, nil } names := map[string]Type{} for i, t := range d.TypeParameters { - names[t.Name] = types[i] + names[t.Name] = ref.TypeParameters[i] } monomorphised := reflect.DeepCopy(d) monomorphised.TypeParameters = nil diff --git a/backend/schema/data_test.go b/backend/schema/data_test.go index 9d6fb38603..5d13e98152 100644 --- a/backend/schema/data_test.go +++ b/backend/schema/data_test.go @@ -14,7 +14,7 @@ func TestMonomorphisation(t *testing.T) { {Name: "a", Type: &DataRef{Name: "T"}}, }, } - actual, err := data.Monomorphise(&String{}) + actual, err := data.Monomorphise(&DataRef{TypeParameters: []Type{&String{}}}) assert.NoError(t, err) expected := &Data{ Comments: []string{}, diff --git a/backend/schema/jsonschema.go b/backend/schema/jsonschema.go index 1b9d199195..45ec96c1c5 100644 --- a/backend/schema/jsonschema.go +++ b/backend/schema/jsonschema.go @@ -38,7 +38,7 @@ func DataToJSONSchema(schema *Schema, dataRef DataRef) (*jsonschema.Schema, erro } if len(dataRef.TypeParameters) > 0 { - monomorphisedData, err := data.Monomorphise(dataRef.TypeParameters...) + monomorphisedData, err := data.Monomorphise(dataRef) if err != nil { return nil, err } diff --git a/backend/schema/schema.go b/backend/schema/schema.go index 479f7c3998..e7a6436850 100644 --- a/backend/schema/schema.go +++ b/backend/schema/schema.go @@ -63,7 +63,7 @@ func (s *Schema) ResolveDataRefMonomorphised(ref *DataRef) (*Data, error) { return nil, fmt.Errorf("unknown data %v", ref) } - return data.Monomorphise(ref.TypeParameters...) + return data.Monomorphise(ref) } func (s *Schema) ResolveVerbRef(ref *VerbRef) *Verb { diff --git a/backend/schema/schema_test.go b/backend/schema/schema_test.go index 10e9a631a8..f05b966e1c 100644 --- a/backend/schema/schema_test.go +++ b/backend/schema/schema_test.go @@ -41,7 +41,7 @@ module todo { calls todo.destroy - verb destroy(builtin.HttpRequest) builtin.HttpResponse + verb destroy(builtin.HttpRequest) builtin.HttpResponse ingress http GET /todo/destroy/{id} } ` @@ -103,6 +103,7 @@ Module DataRef DataRef DataRef + String MetadataIngress IngressPathLiteral IngressPathLiteral @@ -192,7 +193,7 @@ func TestParsing(t *testing.T) { input: `module int { data String { name String } verb verb(String) String }`, errors: []string{"1:14: data structure 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", @@ -200,7 +201,7 @@ func TestParsing(t *testing.T) { &Verb{ Name: "myIngress", Request: &DataRef{Module: "builtin", Name: "HttpRequest", TypeParameters: []Type{&String{}}}, - Response: &DataRef{Module: "builtin", Name: "HttpResponse", TypeParameters: []Type{&String{}}}, + Response: &DataRef{Module: "builtin", Name: "HttpResponse", TypeParameters: []Type{&String{}, &String{}}}, }, }, }}, @@ -217,7 +218,7 @@ func TestParsing(t *testing.T) { message String } - verb echo(builtin.HttpRequest) builtin.HttpResponse + verb echo(builtin.HttpRequest) builtin.HttpResponse ingress http GET /echo calls time.time @@ -231,7 +232,7 @@ func TestParsing(t *testing.T) { time Time } - verb time(builtin.HttpRequest) builtin.HttpResponse + verb time(builtin.HttpRequest) builtin.HttpResponse ingress http GET /time } `, @@ -244,7 +245,7 @@ func TestParsing(t *testing.T) { &Verb{ Name: "echo", Request: &DataRef{Module: "builtin", Name: "HttpRequest", TypeParameters: []Type{&DataRef{Module: "echo", Name: "EchoRequest"}}}, - Response: &DataRef{Module: "builtin", Name: "HttpResponse", TypeParameters: []Type{&DataRef{Module: "echo", Name: "EchoResponse"}}}, + Response: &DataRef{Module: "builtin", Name: "HttpResponse", TypeParameters: []Type{&DataRef{Module: "echo", Name: "EchoResponse"}, &String{}}}, Metadata: []Metadata{ &MetadataIngress{Type: "http", Method: "GET", Path: []IngressPathComponent{&IngressPathLiteral{Text: "echo"}}}, &MetadataCalls{Calls: []*VerbRef{{Module: "time", Name: "time"}}}, @@ -259,7 +260,7 @@ func TestParsing(t *testing.T) { &Verb{ Name: "time", Request: &DataRef{Module: "builtin", Name: "HttpRequest", TypeParameters: []Type{&DataRef{Module: "time", Name: "TimeRequest"}}}, - Response: &DataRef{Module: "builtin", Name: "HttpResponse", TypeParameters: []Type{&DataRef{Module: "time", Name: "TimeResponse"}}}, + Response: &DataRef{Module: "builtin", Name: "HttpResponse", TypeParameters: []Type{&DataRef{Module: "time", Name: "TimeResponse"}, &String{}}}, Metadata: []Metadata{ &MetadataIngress{Type: "http", Method: "GET", Path: []IngressPathComponent{&IngressPathLiteral{Text: "time"}}}, }, @@ -328,7 +329,7 @@ func TestParsing(t *testing.T) { assert.NotZero(t, test.expected, "test.expected is nil") assert.NotZero(t, test.expected.Modules, "test.expected.Modules is nil") test.expected.Modules = append([]*Module{Builtins()}, test.expected.Modules...) - assert.Equal(t, Normalise(test.expected), Normalise(actual), test.input, assert.OmitEmpty()) + assert.Equal(t, Normalise(test.expected), Normalise(actual), assert.OmitEmpty()) } }) } @@ -354,7 +355,7 @@ module todo { } verb create(todo.CreateRequest) todo.CreateResponse calls todo.destroy - verb destroy(builtin.HttpRequest) builtin.HttpResponse + verb destroy(builtin.HttpRequest) builtin.HttpResponse ingress http GET /todo/destroy/{id} } ` @@ -401,7 +402,7 @@ var testSchema = MustValidate(&Schema{ Metadata: []Metadata{&MetadataCalls{Calls: []*VerbRef{{Module: "todo", Name: "destroy"}}}}}, &Verb{Name: "destroy", Request: &DataRef{Module: "builtin", Name: "HttpRequest", TypeParameters: []Type{&DataRef{Module: "todo", Name: "DestroyRequest"}}}, - Response: &DataRef{Module: "builtin", Name: "HttpResponse", TypeParameters: []Type{&DataRef{Module: "todo", Name: "DestroyResponse"}}}, + Response: &DataRef{Module: "builtin", Name: "HttpResponse", TypeParameters: []Type{&DataRef{Module: "todo", Name: "DestroyResponse"}, &String{}}}, Metadata: []Metadata{ &MetadataIngress{ Type: "http", diff --git a/examples/kotlin/ftl-module-ad/src/main/kotlin/ftl/ad/Ad.kt b/examples/kotlin/ftl-module-ad/src/main/kotlin/ftl/ad/Ad.kt index eced970255..41c9ae0ce1 100644 --- a/examples/kotlin/ftl-module-ad/src/main/kotlin/ftl/ad/Ad.kt +++ b/examples/kotlin/ftl-module-ad/src/main/kotlin/ftl/ad/Ad.kt @@ -19,7 +19,7 @@ class AdModule { @Verb @HttpIngress(Method.GET, "/get") - fun get(context: Context, req: HttpRequest): HttpResponse { + fun get(context: Context, req: HttpRequest): HttpResponse { val ads: List = when { req.body.contextKeys != null -> contextualAds(req.body.contextKeys) else -> randomAds() diff --git a/go-runtime/encoding/encoding.go b/go-runtime/encoding/encoding.go index 99cd0aceef..917e20ef81 100644 --- a/go-runtime/encoding/encoding.go +++ b/go-runtime/encoding/encoding.go @@ -30,6 +30,7 @@ func encodeValue(v reflect.Value, w *bytes.Buffer) error { case t.Kind() == reflect.Ptr && t.Elem().Implements(jsonUnmarshaler): v = v.Elem() fallthrough + case t.Implements(jsonUnmarshaler): enc := v.Interface().(json.Marshaler) //nolint:forcetypeassert data, err := enc.MarshalJSON() @@ -42,6 +43,7 @@ func encodeValue(v reflect.Value, w *bytes.Buffer) error { case t.Kind() == reflect.Ptr && t.Elem().Implements(textUnarmshaler): v = v.Elem() fallthrough + case t.Implements(textUnarmshaler): enc := v.Interface().(encoding.TextMarshaler) //nolint:forcetypeassert data, err := enc.MarshalText() diff --git a/go-runtime/encoding/encoding_test.go b/go-runtime/encoding/encoding_test.go index ebfa3e05ac..1276dbd88a 100644 --- a/go-runtime/encoding/encoding_test.go +++ b/go-runtime/encoding/encoding_test.go @@ -10,6 +10,9 @@ import ( ) func TestMarshal(t *testing.T) { + type inner struct { + FooBar string + } somePtr := sdk.Some(42) tests := []struct { name string @@ -27,6 +30,7 @@ func TestMarshal(t *testing.T) { {name: "Map", input: struct{ Map map[string]int }{map[string]int{"foo": 42}}, expected: `{"map":{"foo":42}}`}, {name: "Option", input: struct{ Option sdk.Option[int] }{sdk.Some(42)}, expected: `{"option":42}`}, {name: "OptionPtr", input: struct{ Option *sdk.Option[int] }{&somePtr}, expected: `{"option":42}`}, + {name: "OptionStruct", input: struct{ Option sdk.Option[inner] }{sdk.Some(inner{"foo"})}, expected: `{"option":{"fooBar":"foo"}}`}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/go-runtime/sdk/option.go b/go-runtime/sdk/option.go index bdf3be673e..2d81a135b0 100644 --- a/go-runtime/sdk/option.go +++ b/go-runtime/sdk/option.go @@ -8,6 +8,8 @@ import ( "encoding/json" "fmt" "reflect" + + ftlencoding "github.com/TBD54566975/ftl/go-runtime/encoding" ) // Stdlib interfaces types implement. @@ -167,7 +169,7 @@ func (o Option[T]) Default(value T) T { func (o Option[T]) MarshalJSON() ([]byte, error) { if o.ok { - return json.Marshal(o.value) + return ftlencoding.Marshal(o.value) } return []byte("null"), nil } diff --git a/integration/integration_test.go b/integration/integration_test.go index f5eb70e4b5..c5d6da7cae 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -22,6 +22,7 @@ import ( "connectrpc.com/connect" "github.com/alecthomas/assert/v2" + "github.com/alecthomas/repr" _ "github.com/amacneil/dbmate/v2/pkg/driver/postgres" _ "github.com/jackc/pgx/v5/stdlib" // SQL driver @@ -72,21 +73,21 @@ func TestHttpIngress(t *testing.T) { assert.Equal(t, []string{"Header from FTL"}, resp.headers["Get"]) assert.Equal(t, []string{"application/json; charset=utf-8"}, resp.headers["Content-Type"]) - message, ok := resp.body["msg"].(string) - assert.True(t, ok, "msg is not a string") + 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.body["nested"].(map[string]any) - assert.True(t, ok, "nested is not a map") + 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") + assert.True(t, ok, "good_stuff is not a string: %s", repr.String(resp.jsonBody)) assert.Equal(t, "This is good stuff", goodStuff) }), httpCall(rd, http.MethodPost, "/users", jsonData(t, obj{"userId": 123, "postId": 345}), func(t testing.TB, resp *httpResponse) { assert.Equal(t, 201, resp.status) assert.Equal(t, []string{"Header from FTL"}, resp.headers["Post"]) - success, ok := resp.body["success"].(bool) - assert.True(t, ok, "success is not a bool") + success, ok := resp.jsonBody["success"].(bool) + assert.True(t, ok, "success is not a bool: %s", repr.String(resp.jsonBody)) assert.True(t, success) }), // contains aliased field @@ -96,12 +97,12 @@ func TestHttpIngress(t *testing.T) { httpCall(rd, http.MethodPut, "/users/123", jsonData(t, obj{"postId": "346"}), func(t testing.TB, resp *httpResponse) { assert.Equal(t, 200, resp.status) assert.Equal(t, []string{"Header from FTL"}, resp.headers["Put"]) - assert.Equal(t, map[string]any{}, resp.body) + assert.Equal(t, map[string]any{}, resp.jsonBody) }), httpCall(rd, http.MethodDelete, "/users/123", jsonData(t, obj{}), func(t testing.TB, resp *httpResponse) { assert.Equal(t, 200, resp.status) assert.Equal(t, []string{"Header from FTL"}, resp.headers["Delete"]) - assert.Equal(t, map[string]any{}, resp.body) + assert.Equal(t, map[string]any{}, resp.jsonBody) }), httpCall(rd, http.MethodGet, "/html", jsonData(t, obj{}), func(t testing.TB, resp *httpResponse) { @@ -397,7 +398,7 @@ func call[Resp any](module, verb string, req obj, onResponse func(t testing.TB, type httpResponse struct { status int headers map[string][]string - body map[string]any + jsonBody map[string]any bodyBytes []byte } @@ -432,7 +433,7 @@ func httpCall(rd runtimeData, method string, path string, body []byte, onRespons onResponse(t, &httpResponse{ status: resp.StatusCode, headers: resp.Header, - body: resBody, + jsonBody: resBody, bodyBytes: bodyBytes, }) return nil diff --git a/integration/testdata/go/httpingress/echo.go b/integration/testdata/go/httpingress/echo.go index c3e1f71000..4f9ba7efe8 100644 --- a/integration/testdata/go/httpingress/echo.go +++ b/integration/testdata/go/httpingress/echo.go @@ -26,15 +26,15 @@ type GetResponse struct { //ftl:verb //ftl:ingress http GET /users/{userId}/posts/{postId} -func Get(ctx context.Context, req builtin.HttpRequest[GetRequest]) (builtin.HttpResponse[GetResponse], error) { - return builtin.HttpResponse[GetResponse]{ +func Get(ctx context.Context, req builtin.HttpRequest[GetRequest]) (builtin.HttpResponse[GetResponse, string], error) { + return builtin.HttpResponse[GetResponse, string]{ Headers: map[string][]string{"Get": {"Header from FTL"}}, - Body: GetResponse{ + Body: ftl.Some(GetResponse{ Message: fmt.Sprintf("UserID: %s, PostID: %s", req.Body.UserID, req.Body.PostID), Nested: Nested{ GoodStuff: "This is good stuff", }, - }, + }), }, nil } @@ -49,11 +49,11 @@ type PostResponse struct { //ftl:verb //ftl:ingress http POST /users -func Post(ctx context.Context, req builtin.HttpRequest[PostRequest]) (builtin.HttpResponse[PostResponse], error) { - return builtin.HttpResponse[PostResponse]{ +func Post(ctx context.Context, req builtin.HttpRequest[PostRequest]) (builtin.HttpResponse[PostResponse, string], error) { + return builtin.HttpResponse[PostResponse, string]{ Status: 201, Headers: map[string][]string{"Post": {"Header from FTL"}}, - Body: PostResponse{Success: true}, + Body: ftl.Some(PostResponse{Success: true}), }, nil } @@ -66,10 +66,10 @@ type PutResponse struct{} //ftl:verb //ftl:ingress http PUT /users/{userId} -func Put(ctx context.Context, req builtin.HttpRequest[PutRequest]) (builtin.HttpResponse[builtin.Empty], error) { - return builtin.HttpResponse[builtin.Empty]{ +func Put(ctx context.Context, req builtin.HttpRequest[PutRequest]) (builtin.HttpResponse[builtin.Empty, string], error) { + return builtin.HttpResponse[builtin.Empty, string]{ Headers: map[string][]string{"Put": {"Header from FTL"}}, - Body: builtin.Empty{}, + Body: ftl.Some(builtin.Empty{}), }, nil } @@ -81,11 +81,11 @@ type DeleteResponse struct{} //ftl:verb //ftl:ingress http DELETE /users/{userId} -func Delete(ctx context.Context, req builtin.HttpRequest[DeleteRequest]) (builtin.HttpResponse[builtin.Empty], error) { - return builtin.HttpResponse[builtin.Empty]{ +func Delete(ctx context.Context, req builtin.HttpRequest[DeleteRequest]) (builtin.HttpResponse[builtin.Empty, string], error) { + return builtin.HttpResponse[builtin.Empty, string]{ Status: 200, Headers: map[string][]string{"Delete": {"Header from FTL"}}, - Body: builtin.Empty{}, + Body: ftl.Some(builtin.Empty{}), }, nil } @@ -93,45 +93,45 @@ type HtmlRequest struct{} //ftl:verb //ftl:ingress http GET /html -func Html(ctx context.Context, req builtin.HttpRequest[HtmlRequest]) (builtin.HttpResponse[string], error) { - return builtin.HttpResponse[string]{ +func Html(ctx context.Context, req builtin.HttpRequest[HtmlRequest]) (builtin.HttpResponse[string, string], error) { + return builtin.HttpResponse[string, string]{ Headers: map[string][]string{"Content-Type": {"text/html; charset=utf-8"}}, - Body: "

HTML Page From FTL 🚀!

", + Body: ftl.Some("

HTML Page From FTL 🚀!

"), }, nil } //ftl:verb //ftl:ingress http POST /bytes -func Bytes(ctx context.Context, req builtin.HttpRequest[[]byte]) (builtin.HttpResponse[[]byte], error) { - return builtin.HttpResponse[[]byte]{Body: req.Body}, nil +func Bytes(ctx context.Context, req builtin.HttpRequest[[]byte]) (builtin.HttpResponse[[]byte, string], error) { + return builtin.HttpResponse[[]byte, string]{Body: ftl.Some(req.Body)}, nil } //ftl:verb //ftl:ingress http GET /empty -func Empty(ctx context.Context, req builtin.HttpRequest[ftl.Unit]) (builtin.HttpResponse[ftl.Unit], error) { - return builtin.HttpResponse[ftl.Unit]{Body: ftl.Unit{}}, nil +func Empty(ctx context.Context, req builtin.HttpRequest[ftl.Unit]) (builtin.HttpResponse[ftl.Unit, string], error) { + return builtin.HttpResponse[ftl.Unit, string]{Body: ftl.Some(ftl.Unit{})}, nil } //ftl:verb //ftl:ingress http GET /string -func String(ctx context.Context, req builtin.HttpRequest[string]) (builtin.HttpResponse[string], error) { - return builtin.HttpResponse[string]{Body: req.Body}, nil +func String(ctx context.Context, req builtin.HttpRequest[string]) (builtin.HttpResponse[string, string], error) { + return builtin.HttpResponse[string, string]{Body: ftl.Some(req.Body)}, nil } //ftl:verb //ftl:ingress http GET /int -func Int(ctx context.Context, req builtin.HttpRequest[int]) (builtin.HttpResponse[int], error) { - return builtin.HttpResponse[int]{Body: req.Body}, nil +func Int(ctx context.Context, req builtin.HttpRequest[int]) (builtin.HttpResponse[int, string], error) { + return builtin.HttpResponse[int, string]{Body: ftl.Some(req.Body)}, nil } //ftl:verb //ftl:ingress http GET /float -func Float(ctx context.Context, req builtin.HttpRequest[float64]) (builtin.HttpResponse[float64], error) { - return builtin.HttpResponse[float64]{Body: req.Body}, nil +func Float(ctx context.Context, req builtin.HttpRequest[float64]) (builtin.HttpResponse[float64, string], error) { + return builtin.HttpResponse[float64, string]{Body: ftl.Some(req.Body)}, nil } //ftl:verb //ftl:ingress http GET /bool -func Bool(ctx context.Context, req builtin.HttpRequest[bool]) (builtin.HttpResponse[bool], error) { - return builtin.HttpResponse[bool]{Body: req.Body}, nil +func Bool(ctx context.Context, req builtin.HttpRequest[bool]) (builtin.HttpResponse[bool, string], error) { + return builtin.HttpResponse[bool, string]{Body: ftl.Some(req.Body)}, nil } diff --git a/integration/testdata/kotlin/httpingress/Echo.kt b/integration/testdata/kotlin/httpingress/Echo.kt index 0eb3c65cbd..043a42f22d 100644 --- a/integration/testdata/kotlin/httpingress/Echo.kt +++ b/integration/testdata/kotlin/httpingress/Echo.kt @@ -47,8 +47,8 @@ class Echo { @Verb @HttpIngress( Method.GET, "/users/{userID}/posts/{postID}") - fun `get`(context: Context, req: HttpRequest): HttpResponse { - return HttpResponse( + fun `get`(context: Context, req: HttpRequest): HttpResponse { + return HttpResponse( status = 200, headers = mapOf("Get" to arrayListOf("Header from FTL")), body = GetResponse( @@ -60,8 +60,8 @@ class Echo { @Verb @HttpIngress(Method.POST, "/users") - fun post(context: Context, req: HttpRequest): HttpResponse { - return HttpResponse( + fun post(context: Context, req: HttpRequest): HttpResponse { + return HttpResponse( status = 201, headers = mapOf("Post" to arrayListOf("Header from FTL")), body = PostResponse(success = true) @@ -70,8 +70,8 @@ class Echo { @Verb @HttpIngress(Method.PUT, "/users/{userId}") - fun put(context: Context, req: HttpRequest): HttpResponse { - return HttpResponse( + fun put(context: Context, req: HttpRequest): HttpResponse { + return HttpResponse( status = 200, headers = mapOf("Put" to arrayListOf("Header from FTL")), body = Empty() @@ -80,8 +80,8 @@ class Echo { @Verb @HttpIngress(Method.DELETE, "/users/{userId}") - fun delete(context: Context, req: HttpRequest): HttpResponse { - return HttpResponse( + fun delete(context: Context, req: HttpRequest): HttpResponse { + return HttpResponse( status = 200, headers = mapOf("Delete" to arrayListOf("Header from FTL")), body = Empty() @@ -90,8 +90,8 @@ class Echo { @Verb @HttpIngress(Method.GET, "/html") - fun html(context: Context, req: HttpRequest): HttpResponse { - return HttpResponse( + fun html(context: Context, req: HttpRequest): HttpResponse { + return HttpResponse( status = 200, headers = mapOf("Content-Type" to arrayListOf("text/html; charset=utf-8")), body = "

HTML Page From FTL 🚀!

", @@ -100,8 +100,8 @@ class Echo { @Verb @HttpIngress(Method.POST, "/bytes") - fun bytes(context: Context, req: HttpRequest): HttpResponse { - return HttpResponse( + fun bytes(context: Context, req: HttpRequest): HttpResponse { + return HttpResponse( status = 200, headers = mapOf("Content-Type" to arrayListOf("application/octet-stream")), body = req.body, @@ -110,8 +110,8 @@ class Echo { @Verb @HttpIngress(Method.GET, "/empty") - fun empty(context: Context, req: HttpRequest): HttpResponse { - return HttpResponse( + fun empty(context: Context, req: HttpRequest): HttpResponse { + return HttpResponse( status = 200, headers = mapOf("Empty" to arrayListOf("Header from FTL")), body = Unit @@ -120,8 +120,8 @@ class Echo { @Verb @HttpIngress(Method.GET, "/string") - fun string(context: Context, req: HttpRequest): HttpResponse { - return HttpResponse( + fun string(context: Context, req: HttpRequest): HttpResponse { + return HttpResponse( status = 200, headers = mapOf("String" to arrayListOf("Header from FTL")), body = req.body @@ -130,8 +130,8 @@ class Echo { @Verb @HttpIngress(Method.GET, "/int") - fun int(context: Context, req: HttpRequest): HttpResponse { - return HttpResponse( + fun int(context: Context, req: HttpRequest): HttpResponse { + return HttpResponse( status = 200, headers = mapOf("Int" to arrayListOf("Header from FTL")), body = req.body @@ -140,8 +140,8 @@ class Echo { @Verb @HttpIngress(Method.GET, "/float") - fun float(context: Context, req: HttpRequest): HttpResponse { - return HttpResponse( + fun float(context: Context, req: HttpRequest): HttpResponse { + return HttpResponse( status = 200, headers = mapOf("Float" to arrayListOf("Header from FTL")), body = req.body @@ -150,8 +150,8 @@ class Echo { @Verb @HttpIngress(Method.GET, "/bool") - fun bool(context: Context, req: HttpRequest): HttpResponse { - return HttpResponse( + fun bool(context: Context, req: HttpRequest): HttpResponse { + return HttpResponse( status = 200, headers = mapOf("Bool" to arrayListOf("Header from FTL")), body = req.body diff --git a/kotlin-runtime/ftl-runtime/src/main/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRule.kt b/kotlin-runtime/ftl-runtime/src/main/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRule.kt index ccbe3ae0f3..4c01c98977 100644 --- a/kotlin-runtime/ftl-runtime/src/main/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRule.kt +++ b/kotlin-runtime/ftl-runtime/src/main/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRule.kt @@ -158,7 +158,7 @@ class SchemaExtractor( val respClass = verb.createTypeBindingForReturnType(bindingContext)?.type ?: throw IllegalStateException("$verbSourcePos Could not resolve ${verb.name} return type") require(respClass.toClassDescriptor().isData || respClass.isEmptyBuiltin()) { - "Return type of ${verb.name} must be a data class or builtin.Empty" + "${verbSourcePos}: return type of ${verb.name} must be a data class or builtin.Empty but is ${respClass.fqNameOrNull()?.asString()}" } } } diff --git a/kotlin-runtime/ftl-runtime/src/test/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRuleTest.kt b/kotlin-runtime/ftl-runtime/src/test/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRuleTest.kt index cf0eb86d7f..6878f533f6 100644 --- a/kotlin-runtime/ftl-runtime/src/test/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRuleTest.kt +++ b/kotlin-runtime/ftl-runtime/src/test/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRuleTest.kt @@ -63,10 +63,10 @@ internal class ExtractSchemaRuleTest(private val env: KotlinCoreEnvironment) { @Throws(InvalidInput::class) @Verb @HttpIngress(Method.GET, "/echo") - fun echo(context: Context, req: HttpRequest>): HttpResponse { + fun echo(context: Context, req: HttpRequest>): HttpResponse { callTime(context) - return HttpResponse( + return HttpResponse( status = 200, headers = mapOf("Get" to arrayListOf("Header from FTL")), body = EchoResponse(messages = listOf(EchoMessage(message = "Hello!"))) @@ -214,7 +214,10 @@ internal class ExtractSchemaRuleTest(private val env: KotlinCoreEnvironment) { name = "EchoResponse", module = "echo" ) - ) + ), + Type( + string = xyz.block.ftl.v1.schema.String() + ), ), ), ), diff --git a/kotlin-runtime/ftl-runtime/src/test/kotlin/xyz/block/ftl/schemaextractor/testdata/dependencies/BuiltinModuleClient.kt b/kotlin-runtime/ftl-runtime/src/test/kotlin/xyz/block/ftl/schemaextractor/testdata/dependencies/BuiltinModuleClient.kt index ebff34f08e..dd41b8dcea 100644 --- a/kotlin-runtime/ftl-runtime/src/test/kotlin/xyz/block/ftl/schemaextractor/testdata/dependencies/BuiltinModuleClient.kt +++ b/kotlin-runtime/ftl-runtime/src/test/kotlin/xyz/block/ftl/schemaextractor/testdata/dependencies/BuiltinModuleClient.kt @@ -1,5 +1,6 @@ -// Code generated by FTL-Generator, do not edit. // Built-in types for FTL. +// +// This is copied from the FTL runtime and is not meant to be edited. package ftl.builtin import kotlin.Long @@ -23,10 +24,11 @@ public data class HttpRequest( /** * HTTP response structure used for HTTP ingress verbs. */ -public data class HttpResponse( +public data class HttpResponse( public val status: Long, public val headers: Map>, - public val body: Body, + public val body: Body? = null, + public val error: Error? = null, ) public class Empty