Skip to content

Commit

Permalink
feat: ingress and encoder will omit some empty fields (#957)
Browse files Browse the repository at this point in the history
Specifically Any, Unit, Option, maps, and arrays.

Also added some tests for BuildRequestBody().
  • Loading branch information
alecthomas authored Feb 19, 2024
1 parent ebfca82 commit 66699ed
Show file tree
Hide file tree
Showing 15 changed files with 366 additions and 80 deletions.
11 changes: 7 additions & 4 deletions backend/controller/ingress/alias.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,18 @@ import (
)

func transformAliasedFields(sch *schema.Schema, t schema.Type, obj any, aliaser func(obj map[string]any, field *schema.Field) string) error {
if obj == nil {
return nil
}
switch t := t.(type) {
case *schema.DataRef:
data, err := sch.ResolveDataRefMonomorphised(t)
if err != nil {
return fmt.Errorf("failed to resolve data type: %w", err)
return fmt.Errorf("%s: failed to resolve data type: %w", t.Pos, err)
}
m, ok := obj.(map[string]any)
if !ok {
return fmt.Errorf("expected map, got %T", obj)
return fmt.Errorf("%s: expected map, got %T", t.Pos, obj)
}
for _, field := range data.Fields {
name := aliaser(m, field)
Expand All @@ -27,7 +30,7 @@ func transformAliasedFields(sch *schema.Schema, t schema.Type, obj any, aliaser
case *schema.Array:
a, ok := obj.([]any)
if !ok {
return fmt.Errorf("expected array, got %T", obj)
return fmt.Errorf("%s: expected array, got %T", t.Pos, obj)
}
for _, elem := range a {
if err := transformAliasedFields(sch, t.Element, elem, aliaser); err != nil {
Expand All @@ -38,7 +41,7 @@ func transformAliasedFields(sch *schema.Schema, t schema.Type, obj any, aliaser
case *schema.Map:
m, ok := obj.(map[string]any)
if !ok {
return fmt.Errorf("expected map, got %T", obj)
return fmt.Errorf("%s: expected map, got %T", t.Pos, obj)
}
for key, value := range m {
if err := transformAliasedFields(sch, t.Key, key, aliaser); err != nil {
Expand Down
85 changes: 69 additions & 16 deletions backend/controller/ingress/ingress_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,30 +49,83 @@ func TestValidation(t *testing.T) {
name string
schema string
request obj
err string
}{
{name: "int", schema: `module test { data Test { intValue Int } }`, request: obj{"intValue": 10.0}},
{name: "float", schema: `module test { data Test { floatValue Float } }`, request: obj{"floatValue": 10.0}},
{name: "string", schema: `module test { data Test { stringValue String } }`, request: obj{"stringValue": "test"}},
{name: "bool", schema: `module test { data Test { boolValue Bool } }`, request: obj{"boolValue": true}},
{name: "intString", schema: `module test { data Test { intValue Int } }`, request: obj{"intValue": "10"}},
{name: "floatString", schema: `module test { data Test { floatValue Float } }`, request: obj{"floatValue": "10.0"}},
{name: "boolString", schema: `module test { data Test { boolValue Bool } }`, request: obj{"boolValue": "true"}},
{name: "array", schema: `module test { data Test { arrayValue [String] } }`, request: obj{"arrayValue": []any{"test1", "test2"}}},
{name: "map", schema: `module test { data Test { mapValue {String: String} } }`, request: obj{"mapValue": obj{"key1": "value1", "key2": "value2"}}},
{name: "dataRef", schema: `module test { data Nested { intValue Int } data Test { dataRef Nested } }`, request: obj{"dataRef": obj{"intValue": 10.0}}},
{name: "optional", schema: `module test { data Test { intValue Int? } }`, request: obj{}},
{name: "optionalProvided", schema: `module test { data Test { intValue Int? } }`, request: obj{"intValue": 10.0}},
{name: "arrayDataRef", schema: `module test { data Nested { intValue Int } data Test { arrayValue [Nested] } }`, request: obj{"arrayValue": []any{obj{"intValue": 10.0}, obj{"intValue": 20.0}}}},
{name: "mapDataRef", schema: `module test { data Nested { intValue Int } data Test { mapValue {String: Nested} } }`, request: obj{"mapValue": obj{"key1": obj{"intValue": 10.0}, "key2": obj{"intValue": 20.0}}}},
{name: "otherModuleRef", schema: `module other { data Other { intValue Int } } module test { data Test { otherRef other.Other } }`, request: obj{"otherRef": obj{"intValue": 10.0}}},
{name: "Int",
schema: `module test { data Test { intValue Int } }`,
request: obj{"intValue": 10.0}},
{name: "Float",
schema: `module test { data Test { floatValue Float } }`,
request: obj{"floatValue": 10.0}},
{name: "String",
schema: `module test { data Test { stringValue String } }`,
request: obj{"stringValue": "test"}},
{name: "Bool",
schema: `module test { data Test { boolValue Bool } }`,
request: obj{"boolValue": true}},
{name: "IntString",
schema: `module test { data Test { intValue Int } }`,
request: obj{"intValue": "10"}},
{name: "FloatString",
schema: `module test { data Test { floatValue Float } }`,
request: obj{"floatValue": "10.0"}},
{name: "BoolString",
schema: `module test { data Test { boolValue Bool } }`,
request: obj{"boolValue": "true"}},
{name: "Array",
schema: `module test { data Test { arrayValue [String] } }`,
request: obj{"arrayValue": []any{"test1", "test2"}}},
{name: "Map",
schema: `module test { data Test { mapValue {String: String} } }`,
request: obj{"mapValue": obj{"key1": "value1", "key2": "value2"}}},
{name: "DataRef",
schema: `module test { data Nested { intValue Int } data Test { dataRef Nested } }`,
request: obj{"dataRef": obj{"intValue": 10.0}}},
{name: "Optional",
schema: `module test { data Test { intValue Int? } }`,
request: obj{}},
{name: "OptionalProvided",
schema: `module test { data Test { intValue Int? } }`,
request: obj{"intValue": 10.0}},
{name: "ArrayDataRef",
schema: `module test { data Nested { intValue Int } data Test { arrayValue [Nested] } }`,
request: obj{"arrayValue": []any{obj{"intValue": 10.0}, obj{"intValue": 20.0}}}},
{name: "MapDataRef",
schema: `module test { data Nested { intValue Int } data Test { mapValue {String: Nested} } }`,
request: obj{"mapValue": obj{"key1": obj{"intValue": 10.0}, "key2": obj{"intValue": 20.0}}}},
{name: "OtherModuleRef",
schema: `module other { data Other { intValue Int } } module test { data Test { otherRef other.Other } }`,
request: obj{"otherRef": obj{"intValue": 10.0}}},
{name: "AllowedMissingFieldTypes",
schema: `
module test {
data Test {
array [Int]
map {String: Int}
any Any
bytes Bytes
unit Unit
}
}`,
request: obj{}},
{name: "RequiredFields",
schema: `module test { data Test { int Int } }`,
request: obj{},
err: "int is required",
},
// TODO: More tests for invalid data.
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
sch, err := schema.ParseString("", test.schema)
assert.NoError(t, err)

err = validateRequestMap(&schema.DataRef{Module: "test", Name: "Test"}, nil, test.request, sch)
assert.NoError(t, err, "%v", test.name)
if test.err != "" {
assert.EqualError(t, err, test.err)
} else {
assert.NoError(t, err)
}
})
}
}
Expand Down
16 changes: 13 additions & 3 deletions backend/controller/ingress/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (
func BuildRequestBody(route *dal.IngressRoute, r *http.Request, sch *schema.Schema) ([]byte, error) {
verb := sch.ResolveVerbRef(&schema.VerbRef{Name: route.Verb, Module: route.Module})
if verb == nil {
return nil, fmt.Errorf("unknown verb %s", route.Verb)
return nil, fmt.Errorf("unknown verb %q", route.Verb)
}

request, ok := verb.Request.(*schema.DataRef)
Expand Down Expand Up @@ -241,9 +241,8 @@ func validateRequestMap(dataRef *schema.DataRef, path path, request map[string]a
for _, field := range data.Fields {
fieldPath := append(path, "."+field.Name) //nolint:gocritic

_, isOptional := field.Type.(*schema.Optional)
value, haveValue := request[field.Name]
if !isOptional && !haveValue {
if !haveValue && !allowMissingField(field) {
errs = append(errs, fmt.Errorf("%s is required", fieldPath))
continue
}
Expand All @@ -260,6 +259,17 @@ func validateRequestMap(dataRef *schema.DataRef, path path, request map[string]a
return errors.Join(errs...)
}

// Fields of these types can be omitted from the JSON representation.
func allowMissingField(field *schema.Field) bool {
switch field.Type.(type) {
case *schema.Optional, *schema.Any, *schema.Array, *schema.Map, *schema.Bytes, *schema.Unit:
return true

case *schema.Bool, *schema.DataRef, *schema.Float, *schema.Int, *schema.String, *schema.Time:
}
return false
}

func parseQueryParams(values url.Values, data *schema.Data) (map[string]any, error) {
if jsonStr, ok := values["@json"]; ok {
if len(values) > 1 {
Expand Down
190 changes: 190 additions & 0 deletions backend/controller/ingress/request_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
package ingress

import (
"bytes"
"encoding/json"
"net/http"
"net/url"
"reflect"
"testing"

"github.com/alecthomas/assert/v2"

"github.com/TBD54566975/ftl/backend/controller/dal"
"github.com/TBD54566975/ftl/backend/schema"
"github.com/TBD54566975/ftl/go-runtime/encoding"
"github.com/TBD54566975/ftl/go-runtime/ftl"
)

type AliasRequest struct {
Aliased string `json:"alias"`
}

type PathParameterRequest struct {
Username string
}

type MissingTypes struct {
Optional ftl.Option[string] `json:"optional,omitempty"`
Array []string `json:"array,omitempty"`
Map map[string]string `json:"map,omitempty"`
Any any `json:"any,omitempty"`
Unit ftl.Unit `json:"unit,omitempty"`
}

type PostJSONPayload struct {
Foo string
}

// HTTPRequest mirrors builtin.HttpRequest.
type HTTPRequest[Body 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"`
}

func TestBuildRequestBody(t *testing.T) {
sch, err := schema.ParseString("test", `
module test {
data AliasRequest {
aliased String alias json "alias"
}
data PathParameterRequest {
username String
}
data MissingTypes {
optional String?
array [String]
map {String: String}
any Any
unit Unit
}
data JsonPayload {
foo String
}
verb getAlias(HttpRequest<AliasRequest>) HttpResponse<Empty, Empty>
ingress http GET /getAlias
verb getPath(HttpRequest<PathParameterRequest>) HttpResponse<Empty, Empty>
ingress http GET /getPath/{username}
verb postMissingTypes(HttpRequest<MissingTypes>) HttpResponse<Empty, Empty>
ingress http POST /postMissingTypes
verb postJsonPayload(HttpRequest<JsonPayload>) HttpResponse<Empty, Empty>
ingress http POST /postJsonPayload
}
`)
assert.NoError(t, err)
for _, test := range []struct {
name string
verb string
method string
path string
query url.Values
body obj
expected any
err string
}{
{name: "UnknownVerb",
verb: "unknown",
err: `unknown verb "unknown"`},
{name: "UnknownModule",
verb: "unknown",
err: `unknown verb "unknown"`},
//FIXME: Query parameter decoding doesn't work?
//
// {name: "QueryParameterDecoding",
// verb: "getAlias",
// method: "GET",
// path: "/getAlias",
// query: map[string][]string{
// "alias": {"value"},
// },
// expected: HTTPRequest[AliasRequest]{
// Method: "GET",
// Path: "/getAlias",
// Query: map[string][]string{
// "alias": {"value"},
// },
// Body: AliasRequest{
// Aliased: "value",
// },
// },
// },
{name: "AllowMissingFieldTypes",
verb: "postMissingTypes",
method: "POST",
path: "/postMissingTypes",
expected: HTTPRequest[MissingTypes]{
Method: "POST",
Path: "/postMissingTypes",
Body: MissingTypes{},
},
},
{name: "JSONPayload",
verb: "postJsonPayload",
method: "POST",
path: "/postJsonPayload",
body: obj{"foo": "bar"},
expected: HTTPRequest[PostJSONPayload]{
Method: "POST",
Path: "/postJsonPayload",
Body: PostJSONPayload{Foo: "bar"},
},
},
// FIXME: Path parameters don't seem to be interpolated into body fields?
//
// {name: "PathParameterDecoding",
// verb: "getPath",
// method: "GET",
// path: "/getPath/bob",
// expected: HTTPRequest[PathParameterRequest]{
// Method: "GET",
// Path: "/getPath/bob",
// PathParameters: map[string]string{
// "username": "bob",
// },
// Body: PathParameterRequest{
// Username: "bob",
// },
// },
// },
} {
t.Run(test.name, func(t *testing.T) {
if test.body == nil {
test.body = obj{}
}
body, err := encoding.Marshal(test.body)
assert.NoError(t, err)
requestURL := "http://127.0.0.1" + test.path
if test.query != nil {
requestURL += "?" + test.query.Encode()
}
r, err := http.NewRequest(test.method, requestURL, bytes.NewReader(body)) //nolint:noctx
assert.NoError(t, err)
requestBody, err := BuildRequestBody(&dal.IngressRoute{
Path: test.path,
Module: "test",
Verb: test.verb,
}, r, sch)
if test.err != "" {
assert.EqualError(t, err, test.err)
return
}
assert.NoError(t, err)
actualrv := reflect.New(reflect.TypeOf(test.expected))
actual := actualrv.Interface()
err = json.Unmarshal(requestBody, actual)
assert.NoError(t, err)
assert.Equal(t, test.expected, actualrv.Elem().Interface(), assert.OmitEmpty())
})
}
}
Loading

0 comments on commit 66699ed

Please sign in to comment.