Skip to content

Commit

Permalink
feat: change HttpResponse<R> to HttpResponse<R, E>
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
alecthomas committed Feb 9, 2024
1 parent 0013a43 commit d619b4a
Show file tree
Hide file tree
Showing 21 changed files with 227 additions and 193 deletions.
6 changes: 3 additions & 3 deletions Bitfile
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,15 @@ 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 .

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}
Expand All @@ -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:
Expand Down
20 changes: 4 additions & 16 deletions backend/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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
}

Expand Down
45 changes: 0 additions & 45 deletions backend/controller/ingress/ingress.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
46 changes: 27 additions & 19 deletions backend/controller/ingress/ingress_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package ingress

import (
"net/http"
"net/url"
"testing"

Expand Down Expand Up @@ -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{
Expand All @@ -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",
Expand All @@ -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)
}
})
}
}
99 changes: 78 additions & 21 deletions backend/controller/ingress/response.go
Original file line number Diff line number Diff line change
@@ -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 ""
}
}
6 changes: 4 additions & 2 deletions backend/schema/builtin.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@ builtin module builtin {
}
// HTTP response structure used for HTTP ingress verbs.
data HttpResponse<Body> {
data HttpResponse<Body, Error> {
status Int
headers {String: [String]}
body Body
// Either "body" or "error" must be present, not both.
body Body?
error Error?
}
data Empty {}
Expand Down
Loading

0 comments on commit d619b4a

Please sign in to comment.