Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: setting secrets and configs error with aliased field names #2103

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 0 additions & 127 deletions backend/controller/ingress/alias.go

This file was deleted.

4 changes: 2 additions & 2 deletions backend/controller/ingress/ingress_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -401,9 +401,9 @@ func TestEnumValidation(t *testing.T) {
{&schema.Ref{Name: "TypeEnumRequest", Module: "test"}, obj{"message": obj{"name": "Invalid", "value": 0}},
"\"Invalid\" is not a valid variant of enum test.TypeEnum"},
{&schema.Ref{Name: "TypeEnumRequest", Module: "test"}, obj{"message": obj{"name": 0, "value": 0}},
`invalid type for enum "test.TypeEnum"; name field must be a string, was int`},
`failed to transform aliased fields: 0:0: expected 'name' field to be a string, got int`},
{&schema.Ref{Name: "TypeEnumRequest", Module: "test"}, obj{"message": "Hello"},
`malformed enum type test.TypeEnumRequest.message: expected structure is {"name": "<variant name>", "value": <variant value>}`},
`failed to transform aliased fields: 0:0: expected map, got string`},
Comment on lines +404 to +406
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@safeer we might want to come back and fix up these error messages if possible to make them more readable

}

for _, test := range tests {
Expand Down
2 changes: 1 addition & 1 deletion backend/controller/ingress/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func BuildRequestBody(route *dal.IngressRoute, r *http.Request, sch *schema.Sche
}
}

requestMap, err = transformFromAliasedFields(request, sch, requestMap)
requestMap, err = schema.TransformFromAliasedFields(request, sch, requestMap)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion backend/controller/ingress/response.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func bodyForType(typ schema.Type, sch *schema.Schema, data []byte) ([]byte, erro
return nil, fmt.Errorf("HTTP response body is not valid JSON: %w", err)
}

err = transformAliasedFields(sch, t, response, func(obj map[string]any, field *schema.Field) string {
err = schema.TransformAliasedFields(sch, t, response, func(obj map[string]any, field *schema.Field) string {
if jsonAlias, ok := field.Alias(schema.AliasKindJSON).Get(); ok && field.Name != jsonAlias {
obj[jsonAlias] = obj[field.Name]
delete(obj, field.Name)
Expand Down
128 changes: 127 additions & 1 deletion backend/schema/jsonvalidate.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,12 @@ func ValidateJSONValue(fieldType Type, path path, value any, sch *Schema) error
switch d := decl.(type) {
case *Data:
if valueMap, ok := value.(map[string]any); ok {
if err := ValidateRequestMap(fieldType, path, valueMap, sch); err != nil {
transformedMap, err := TransformFromAliasedFields(fieldType, sch, valueMap)
if err != nil {
return fmt.Errorf("failed to transform aliased fields: %w", err)
}

if err := ValidateRequestMap(fieldType, path, transformedMap, sch); err != nil {
return err
}
typeMatches = true
Expand Down Expand Up @@ -211,6 +216,7 @@ func ValidateJSONValue(fieldType Type, path path, value any, sch *Schema) error
return nil
}

// ValidateRequestMap validates a given JSON map against the provided schema.
func ValidateRequestMap(ref *Ref, path path, request map[string]any, sch *Schema) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for future reference:
Since this function is only called by ingress.BuildRequestBody and ValidateJSONValue above, we could have this function call TransformFromAliasedFields directly. Then, the caller doesn't need to know to do it, so we can prevent issues like this in the future.

Discussed on Slack - we'll leave this as is for now to keep everything as stable as possible

data, err := sch.ResolveMonomorphised(ref)
if err != nil {
Expand Down Expand Up @@ -249,3 +255,123 @@ func allowMissingField(field *Field) bool {
}
return false
}

func TransformAliasedFields(sch *Schema, t Type, obj any, aliaser func(obj map[string]any, field *Field) string) error {
if obj == nil {
return nil
}
switch t := t.(type) {
case *Ref:
decl, ok := sch.Resolve(t).Get()
if !ok {
return fmt.Errorf("%s: failed to resolve ref %s", t.Pos, t)
}
switch decl := decl.(type) {
case *Data:
data, err := sch.ResolveMonomorphised(t)
if err != nil {
return fmt.Errorf("%s: failed to resolve data type: %w", t.Pos, err)
}
m, ok := obj.(map[string]any)
if !ok {
return fmt.Errorf("%s: expected map, got %T", t.Pos, obj)
}
for _, field := range data.Fields {
name := aliaser(m, field)
if err := TransformAliasedFields(sch, field.Type, m[name], aliaser); err != nil {
return err
}
}
case *Enum:
if decl.IsValueEnum() {
return nil
}

// type enum
m, ok := obj.(map[string]any)
if !ok {
return fmt.Errorf("%s: expected map, got %T", t.Pos, obj)
}
name, ok := m["name"]
if !ok {
return fmt.Errorf("%s: expected type enum request to have 'name' field", t.Pos)
}
nameStr, ok := name.(string)
if !ok {
return fmt.Errorf("%s: expected 'name' field to be a string, got %T", t.Pos, name)
}

value, ok := m["value"]
if !ok {
return fmt.Errorf("%s: expected type enum request to have 'value' field", t.Pos)
}

for _, v := range decl.Variants {
if v.Name == nameStr {
if err := TransformAliasedFields(sch, v.Value.(*TypeValue).Value, value, aliaser); err != nil { //nolint:forcetypeassert
return err
}
}
}
case *TypeAlias:
return TransformAliasedFields(sch, decl.Type, obj, aliaser)
case *Config, *Database, *FSM, *Secret, *Verb, *Topic, *Subscription:
return fmt.Errorf("%s: unsupported ref type %T", t.Pos, decl)
}

case *Array:
a, ok := obj.([]any)
if !ok {
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 {
return err
}
}

case *Map:
m, ok := obj.(map[string]any)
if !ok {
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 {
return err
}
if err := TransformAliasedFields(sch, t.Value, value, aliaser); err != nil {
return err
}
}

case *Optional:
return TransformAliasedFields(sch, t.Type, obj, aliaser)

case *Any, *Bool, *Bytes, *Float, *Int,
*String, *Time, *Unit:
}
return nil
}

func TransformFromAliasedFields(ref *Ref, sch *Schema, request map[string]any) (map[string]any, error) {
return request, TransformAliasedFields(sch, ref, request, func(obj map[string]any, field *Field) string {
if jsonAlias, ok := field.Alias(AliasKindJSON).Get(); ok {
if _, ok := obj[field.Name]; !ok && obj[jsonAlias] != nil {
obj[field.Name] = obj[jsonAlias]
delete(obj, jsonAlias)
}
}
return field.Name
})
}

func TransformToAliasedFields(ref *Ref, sch *Schema, request map[string]any) (map[string]any, error) {
return request, TransformAliasedFields(sch, ref, request, func(obj map[string]any, field *Field) string {
if jsonAlias, ok := field.Alias(AliasKindJSON).Get(); ok && field.Name != jsonAlias {
obj[jsonAlias] = obj[field.Name]
delete(obj, field.Name)
return jsonAlias
}
return field.Name
})
}
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
package ingress
package schema

import (
"testing"

"github.com/alecthomas/assert/v2"

"github.com/TBD54566975/ftl/backend/schema"
)

func TestTransformFromAliasedFields(t *testing.T) {
Expand All @@ -31,9 +29,9 @@ func TestTransformFromAliasedFields(t *testing.T) {
}
`

sch, err := schema.ParseString("test", schemaText)
sch, err := ParseString("test", schemaText)
assert.NoError(t, err)
actual, err := transformFromAliasedFields(&schema.Ref{Module: "test", Name: "Test"}, sch, map[string]any{
actual, err := TransformFromAliasedFields(&Ref{Module: "test", Name: "Test"}, sch, map[string]any{
"bar": "value",
"inner": map[string]any{
"foo": "value",
Expand Down Expand Up @@ -106,9 +104,9 @@ func TestTransformToAliasedFields(t *testing.T) {
}
`

sch, err := schema.ParseString("test", schemaText)
sch, err := ParseString("test", schemaText)
assert.NoError(t, err)
actual, err := transformToAliasedFields(&schema.Ref{Module: "test", Name: "Test"}, sch, map[string]any{
actual, err := TransformToAliasedFields(&Ref{Module: "test", Name: "Test"}, sch, map[string]any{
"scalar": "value",
"inner": map[string]any{
"waz": "value",
Expand Down
Loading