From 1b1559298913b7db925034d4c1ce25e8e392578e Mon Sep 17 00:00:00 2001 From: Alec Thomas Date: Fri, 22 Mar 2024 13:48:59 +1100 Subject: [PATCH] chore: improve ingress schema validation (#1098) This vastly improves ingress schema validation, ensuring that the request/response body types are valid, and that if path parameters are used the request body is a data structure with a corresponding field. cc @lifeizhou-ap --- backend/schema/any.go | 2 +- backend/schema/array.go | 4 +- backend/schema/bool.go | 2 +- backend/schema/bytes.go | 2 +- backend/schema/data.go | 2 +- backend/schema/float.go | 2 +- backend/schema/int.go | 2 +- backend/schema/jsonschema.go | 2 +- backend/schema/map.go | 2 + backend/schema/normalise.go | 2 +- backend/schema/optional.go | 2 + backend/schema/parser.go | 7 +- backend/schema/schema_test.go | 116 +------------------- backend/schema/string.go | 2 +- backend/schema/time.go | 2 +- backend/schema/typeresolver.go | 50 +++++++++ backend/schema/typeresolver_test.go | 41 +++++++ backend/schema/unit.go | 2 +- backend/schema/validate.go | 148 ++++++++++++++++++------- backend/schema/validate_test.go | 162 ++++++++++++++++++++++++++++ 20 files changed, 391 insertions(+), 163 deletions(-) create mode 100644 backend/schema/typeresolver_test.go create mode 100644 backend/schema/validate_test.go diff --git a/backend/schema/any.go b/backend/schema/any.go index 0a3138b3a4..d8f52afd95 100644 --- a/backend/schema/any.go +++ b/backend/schema/any.go @@ -21,4 +21,4 @@ func (*Any) schemaType() {} func (*Any) schemaSymbol() {} func (*Any) String() string { return "Any" } func (a *Any) ToProto() proto.Message { return &schemapb.Any{Pos: posToProto(a.Pos)} } -func (*Any) GetName() string { return "" } +func (*Any) GetName() string { return "Any" } diff --git a/backend/schema/array.go b/backend/schema/array.go index cdb67c44b5..790275fde3 100644 --- a/backend/schema/array.go +++ b/backend/schema/array.go @@ -13,10 +13,12 @@ type Array struct { } var _ Type = (*Array)(nil) +var _ Symbol = (*Array)(nil) func (a *Array) Position() Position { return a.Pos } func (a *Array) schemaChildren() []Node { return []Node{a.Element} } -func (*Array) schemaType() {} +func (a *Array) schemaType() {} +func (a *Array) schemaSymbol() {} func (a *Array) String() string { return "[" + a.Element.String() + "]" } func (a *Array) ToProto() proto.Message { diff --git a/backend/schema/bool.go b/backend/schema/bool.go index a39784f421..d3cc7e45d5 100644 --- a/backend/schema/bool.go +++ b/backend/schema/bool.go @@ -21,4 +21,4 @@ func (*Bool) schemaType() {} func (*Bool) schemaSymbol() {} func (*Bool) String() string { return "Bool" } func (b *Bool) ToProto() proto.Message { return &schemapb.Bool{Pos: posToProto(b.Pos)} } -func (*Bool) GetName() string { return "" } +func (*Bool) GetName() string { return "Bool" } diff --git a/backend/schema/bytes.go b/backend/schema/bytes.go index 6a8c076ca5..faa3a7d863 100644 --- a/backend/schema/bytes.go +++ b/backend/schema/bytes.go @@ -21,4 +21,4 @@ func (*Bytes) schemaType() {} func (*Bytes) schemaSymbol() {} func (*Bytes) String() string { return "Bytes" } func (b *Bytes) ToProto() proto.Message { return &schemapb.Bytes{Pos: posToProto(b.Pos)} } -func (*Bytes) GetName() string { return "" } +func (*Bytes) GetName() string { return "Bytes" } diff --git a/backend/schema/data.go b/backend/schema/data.go index 864883ad63..906e67a869 100644 --- a/backend/schema/data.go +++ b/backend/schema/data.go @@ -121,7 +121,7 @@ func (d *Data) Monomorphise(ref *Ref) (*Data, error) { Metadata, *MetadataCalls, *MetadataDatabases, *MetadataIngress, *MetadataAlias, *Module, *Schema, *String, *Time, Type, *TypeParameter, *Unit, *Verb, *Enum, *EnumVariant, - Value, *IntValue, *StringValue, Symbol: + Value, *IntValue, *StringValue, Symbol, Named: } return next() }) diff --git a/backend/schema/float.go b/backend/schema/float.go index 8b31b052c2..cbe9942fb1 100644 --- a/backend/schema/float.go +++ b/backend/schema/float.go @@ -21,4 +21,4 @@ func (*Float) schemaType() {} func (*Float) schemaSymbol() {} func (*Float) String() string { return "Float" } func (f *Float) ToProto() proto.Message { return &schemapb.Float{Pos: posToProto(f.Pos)} } -func (*Float) GetName() string { return "" } +func (*Float) GetName() string { return "Float" } diff --git a/backend/schema/int.go b/backend/schema/int.go index d443ea84f1..717be3d82e 100644 --- a/backend/schema/int.go +++ b/backend/schema/int.go @@ -21,4 +21,4 @@ func (*Int) schemaChildren() []Node { return nil } func (*Int) schemaType() {} func (*Int) String() string { return "Int" } func (i *Int) ToProto() proto.Message { return &schemapb.Int{Pos: posToProto(i.Pos)} } -func (*Int) GetName() string { return "" } +func (*Int) GetName() string { return "Int" } diff --git a/backend/schema/jsonschema.go b/backend/schema/jsonschema.go index 3df4ebedc0..8d80752f22 100644 --- a/backend/schema/jsonschema.go +++ b/backend/schema/jsonschema.go @@ -164,7 +164,7 @@ func nodeToJSSchema(node Node, refs map[RefKey]*Ref) *jsonschema.Schema { case Decl, *Field, Metadata, *MetadataCalls, *MetadataDatabases, *MetadataIngress, *MetadataAlias, IngressPathComponent, *IngressPathLiteral, *IngressPathParameter, *Module, *Schema, Type, *Database, *Verb, *EnumVariant, - Value, *StringValue, *IntValue, *Config, *Secret, Symbol: + Value, *StringValue, *IntValue, *Config, *Secret, Symbol, Named: panic(fmt.Sprintf("unsupported node type %T", node)) default: diff --git a/backend/schema/map.go b/backend/schema/map.go index 1169c04e1e..b8d149dd5f 100644 --- a/backend/schema/map.go +++ b/backend/schema/map.go @@ -16,10 +16,12 @@ type Map struct { } var _ Type = (*Map)(nil) +var _ Symbol = (*Map)(nil) func (m *Map) Position() Position { return m.Pos } func (m *Map) schemaChildren() []Node { return []Node{m.Key, m.Value} } func (*Map) schemaType() {} +func (*Map) schemaSymbol() {} func (m *Map) String() string { return fmt.Sprintf("{%s: %s}", m.Key.String(), m.Value.String()) } func (m *Map) ToProto() proto.Message { diff --git a/backend/schema/normalise.go b/backend/schema/normalise.go index 9bd26a2eb1..fdd97c0266 100644 --- a/backend/schema/normalise.go +++ b/backend/schema/normalise.go @@ -130,7 +130,7 @@ func Normalise[T Node](n T) T { c.Pos = zero c.Type = Normalise(c.Type) - case Symbol, Decl, Metadata, IngressPathComponent, Type, Value: // Can never occur in reality, but here to satisfy the sum-type check. + case Named, Symbol, Decl, Metadata, IngressPathComponent, Type, Value: // Can never occur in reality, but here to satisfy the sum-type check. panic("??") } return ni.(T) //nolint:forcetypeassert diff --git a/backend/schema/optional.go b/backend/schema/optional.go index 0db43e4881..1ce142c362 100644 --- a/backend/schema/optional.go +++ b/backend/schema/optional.go @@ -14,10 +14,12 @@ type Optional struct { } var _ Type = (*Optional)(nil) +var _ Symbol = (*Optional)(nil) func (o *Optional) Position() Position { return o.Pos } func (o *Optional) String() string { return o.Type.String() + "?" } func (*Optional) schemaType() {} +func (*Optional) schemaSymbol() {} func (o *Optional) schemaChildren() []Node { return []Node{o.Type} } func (o *Optional) ToProto() proto.Message { return &schemapb.Optional{Pos: posToProto(o.Pos), Type: typeToProto(o.Type)} diff --git a/backend/schema/parser.go b/backend/schema/parser.go index ebbbfec901..3a97bade39 100644 --- a/backend/schema/parser.go +++ b/backend/schema/parser.go @@ -127,10 +127,15 @@ type Value interface { //sumtype:decl type Symbol interface { Node - GetName() string schemaSymbol() } +// A Named symbol in the grammar. +type Named interface { + Symbol + GetName() string +} + // Decl represents user-defined data types in the schema grammar. // //sumtype:decl diff --git a/backend/schema/schema_test.go b/backend/schema/schema_test.go index 7f11244d6a..c23d9c3301 100644 --- a/backend/schema/schema_test.go +++ b/backend/schema/schema_test.go @@ -47,7 +47,7 @@ module todo { +calls todo.destroy +database calls todo.testdb verb destroy(builtin.HttpRequest) builtin.HttpResponse - +ingress http GET /todo/destroy/{id} + +ingress http GET /todo/destroy/{name} } module foo { @@ -389,7 +389,7 @@ module todo { verb create(todo.CreateRequest) todo.CreateResponse +calls todo.destroy +database calls todo.testdb verb destroy(builtin.HttpRequest) builtin.HttpResponse - +ingress http GET /todo/destroy/{id} + +ingress http GET /todo/destroy/{name} } ` actual, err := ParseModuleString("", input) @@ -480,7 +480,7 @@ var testSchema = MustValidate(&Schema{ Path: []IngressPathComponent{ &IngressPathLiteral{Text: "todo"}, &IngressPathLiteral{Text: "destroy"}, - &IngressPathParameter{Name: "id"}, + &IngressPathParameter{Name: "name"}, }, }, }, @@ -513,113 +513,3 @@ var testSchema = MustValidate(&Schema{ }, }, }) - -func TestValidateDependencies(t *testing.T) { - tests := []struct { - name string - schema string - err string - }{ - { - // one <--> two, cyclical - name: "TwoModuleCycle", - schema: ` - module one { - verb one(builtin.Empty) builtin.Empty - +calls two.two - } - - module two { - verb two(builtin.Empty) builtin.Empty - +calls one.one - } - `, - err: "found cycle in dependencies: two -> one -> two", - }, - { - // one --> two --> three, noncyclical - name: "ThreeModulesNoCycle", - schema: ` - module one { - verb one(builtin.Empty) builtin.Empty - +calls two.two - } - - module two { - verb two(builtin.Empty) builtin.Empty - +calls three.three - } - - module three { - verb three(builtin.Empty) builtin.Empty - } - `, - err: "", - }, - { - // one --> two --> three -> one, cyclical - name: "ThreeModulesCycle", - schema: ` - module one { - verb one(builtin.Empty) builtin.Empty - +calls two.two - } - - module two { - verb two(builtin.Empty) builtin.Empty - +calls three.three - } - - module three { - verb three(builtin.Empty) builtin.Empty - +calls one.one - } - `, - err: "found cycle in dependencies: two -> three -> one -> two", - }, - { - // one.a --> two.a - // one.b <--- - // cyclical (does not depend on verbs used) - name: "TwoModuleCycleDiffVerbs", - schema: ` - module one { - verb a(builtin.Empty) builtin.Empty - +calls two.a - verb b(builtin.Empty) builtin.Empty - } - - module two { - verb a(builtin.Empty) builtin.Empty - +calls one.b - } - `, - err: "found cycle in dependencies: two -> one -> two", - }, - { - // one --> one, this is allowed - name: "SelfReference", - schema: ` - module one { - verb a(builtin.Empty) builtin.Empty - +calls one.b - - verb b(builtin.Empty) builtin.Empty - +calls one.a - } - `, - err: "", - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - _, err := ParseString("", test.schema) - if test.err == "" { - assert.NoError(t, err) - } else { - assert.EqualError(t, err, test.err) - } - }) - } -} diff --git a/backend/schema/string.go b/backend/schema/string.go index dcabff2ec2..d4bbbaf43c 100644 --- a/backend/schema/string.go +++ b/backend/schema/string.go @@ -21,4 +21,4 @@ func (*String) schemaType() {} func (*String) schemaSymbol() {} func (*String) String() string { return "String" } func (s *String) ToProto() proto.Message { return &schemapb.String{Pos: posToProto(s.Pos)} } -func (*String) GetName() string { return "" } +func (*String) GetName() string { return "String" } diff --git a/backend/schema/time.go b/backend/schema/time.go index 77649f05d6..044437e65b 100644 --- a/backend/schema/time.go +++ b/backend/schema/time.go @@ -21,4 +21,4 @@ func (*Time) schemaType() {} func (*Time) schemaSymbol() {} func (*Time) String() string { return "Time" } func (t *Time) ToProto() proto.Message { return &schemapb.Time{Pos: posToProto(t.Pos)} } -func (*Time) GetName() string { return "" } +func (*Time) GetName() string { return "Time" } diff --git a/backend/schema/typeresolver.go b/backend/schema/typeresolver.go index c14dcb3223..2db6863b5c 100644 --- a/backend/schema/typeresolver.go +++ b/backend/schema/typeresolver.go @@ -34,6 +34,34 @@ func (s Scope) String() string { return out.String() } +// ResolveTypeAs resolves a [Type] to a concrete symbol and declaration. +func ResolveTypeAs[S Symbol](scopes Scopes, t Type) (symbol S, decl *ModuleDecl) { + decl = scopes.ResolveType(t) + if decl == nil { + return + } + var ok bool + symbol, ok = decl.Symbol.(S) + if !ok { + return + } + return symbol, decl +} + +// ResolveAs resolves a [Ref] to a concrete symbol and declaration. +func ResolveAs[S Symbol](scopes Scopes, ref Ref) (symbol S, decl *ModuleDecl) { + decl = scopes.Resolve(ref) + if decl == nil { + return + } + var ok bool + symbol, ok = decl.Symbol.(S) + if !ok { + return + } + return symbol, decl +} + // Scopes to search during type resolution. type Scopes []Scope @@ -95,6 +123,22 @@ func (s *Scopes) Add(owner *Module, name string, symbol Symbol) error { return nil } +func (s Scopes) ResolveType(t Type) *ModuleDecl { + switch t := t.(type) { + case Named: + return s.Resolve(Ref{Name: t.GetName()}) + + case *Ref: + return s.Resolve(*t) + + case Symbol: + return &ModuleDecl{nil, t} + + default: + return nil + } +} + // Resolve a reference to a symbol declaration or nil. func (s Scopes) Resolve(ref Ref) *ModuleDecl { if ref.Module == "" { @@ -115,6 +159,12 @@ func (s Scopes) Resolve(ref Ref) *ModuleDecl { // Holy nested if statement Batman. return decl } + } else { + for _, d := range mdecl.Module.Decls { + if d.GetName() == ref.Name { + return &ModuleDecl{mdecl.Module, d} + } + } } } } diff --git a/backend/schema/typeresolver_test.go b/backend/schema/typeresolver_test.go new file mode 100644 index 0000000000..8724c5cff7 --- /dev/null +++ b/backend/schema/typeresolver_test.go @@ -0,0 +1,41 @@ +package schema + +import ( + "testing" + + "github.com/alecthomas/assert/v2" +) + +func TestTypeResolver(t *testing.T) { + module, err := ParseModuleString("", ` + module test { + data Request { + t T + } + verb test(Request) Empty + } + `) + assert.NoError(t, err) + scopes := NewScopes() + scopes = scopes.PushScope(module.Scope()) + + // Resolve a builtin. + actualInt, _ := ResolveAs[*Int](scopes, Ref{Name: "Int"}) + assert.NotZero(t, actualInt) + assert.Equal(t, &Int{}, actualInt, assert.Exclude[Position]()) + + // Resolve a generic data structure. + actualData, _ := ResolveAs[*Data](scopes, Ref{Module: "test", Name: "Request"}) + assert.NotZero(t, actualData) + expectedData := &Data{ + Name: "Request", + TypeParameters: []*TypeParameter{{Name: "T"}}, + Fields: []*Field{{Name: "t", Type: &Ref{Name: "T"}}}, + } + assert.Equal(t, expectedData, actualData, assert.Exclude[Position]()) + + // Resolve a type parameter. + scopes = scopes.PushScope(actualData.Scope()) + actualTP, _ := ResolveAs[*TypeParameter](scopes, Ref{Name: "T"}) + assert.Equal(t, actualTP, &TypeParameter{Name: "T"}, assert.Exclude[Position]()) +} diff --git a/backend/schema/unit.go b/backend/schema/unit.go index fc4f818024..2ffe92f58f 100644 --- a/backend/schema/unit.go +++ b/backend/schema/unit.go @@ -21,4 +21,4 @@ func (u *Unit) schemaSymbol() {} func (u *Unit) String() string { return "Unit" } func (u *Unit) ToProto() protoreflect.ProtoMessage { return &schemapb.Unit{Pos: posToProto(u.Pos)} } func (u *Unit) schemaChildren() []Node { return nil } -func (u *Unit) GetName() string { return "" } +func (u *Unit) GetName() string { return "Unit" } diff --git a/backend/schema/validate.go b/backend/schema/validate.go index 7992be128f..54a749fb3a 100644 --- a/backend/schema/validate.go +++ b/backend/schema/validate.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/alecthomas/participle/v2" + "github.com/alecthomas/participle/v2/lexer" xreflect "golang.design/x/reflect" "golang.org/x/exp/maps" @@ -81,7 +82,7 @@ func Validate(schema *Schema) (*Schema, error) { } if _, seen := modules[module.Name]; seen { - merr = append(merr, fmt.Errorf("%s: duplicate module %q", module.Pos, module.Name)) + merr = append(merr, errorf(module, "duplicate module %q", module.Name)) } modules[module.Name] = true if err := ValidateModule(module); err != nil { @@ -106,38 +107,44 @@ func Validate(schema *Schema) (*Schema, error) { n.Module = mdecl.Module.Name } if len(n.TypeParameters) != 0 { - merr = append(merr, fmt.Errorf("%s: reference to %s %q cannot have type parameters", - n.Pos, reflect.TypeOf(decl).Elem().Name(), n.Name)) + merr = append(merr, errorf(n, "reference to %s %q cannot have type parameters", + reflect.TypeOf(decl).Elem().Name(), n.Name)) } case *Data: if mdecl.Module != nil { n.Module = mdecl.Module.Name } if len(n.TypeParameters) != len(decl.TypeParameters) { - merr = append(merr, fmt.Errorf("%s: reference to data structure %s has %d type parameters, but %d were expected", - n.Pos, n.Name, len(n.TypeParameters), len(decl.TypeParameters))) + merr = append(merr, errorf(n, "reference to data structure %s has %d type parameters, but %d were expected", + n.Name, len(n.TypeParameters), len(decl.TypeParameters))) } case *TypeParameter: default: - merr = append(merr, fmt.Errorf("%s: invalid reference %q at %q", n.Pos, n, mdecl.Symbol.Position())) + merr = append(merr, errorf(n, "invalid reference %q at %q", n, mdecl.Symbol.Position())) } } else { - merr = append(merr, fmt.Errorf("%s: unknown reference %q", n.Pos, n)) + merr = append(merr, errorf(n, "unknown reference %q", n)) } case *Verb: for _, md := range n.Metadata { if md, ok := md.(*MetadataIngress); ok { - if existing, ok := ingress[md.String()]; ok { - merr = append(merr, fmt.Errorf("%s: duplicate %q for %s:%q and %s:%q", md.Pos, md.String(), existing.Pos, existing.Name, n.Pos, n.Name)) + // Check for duplicate ingress keys + key := md.Method + " " + for _, path := range md.Path { + switch path := path.(type) { + case *IngressPathLiteral: + key += "/" + path.Text + + case *IngressPathParameter: + key += "/{}" + } } - - if md.Type == "http" && (!strings.HasPrefix(n.Request.String(), "builtin.HttpRequest") || !strings.HasPrefix(n.Response.String(), "builtin.HttpResponse")) { - merr = append(merr, fmt.Errorf("%s: HTTP ingress verb %s(%s) %s must have the signature %s(builtin.HttpRequest) builtin.HttpResponse", - md.Pos, n.Name, n.Request, n.Response, n.Name)) + if existing, ok := ingress[key]; ok { + merr = append(merr, errorf(md, "duplicate %s ingress %s for %s:%q and %s:%q", md.Type, key, existing.Pos, existing.Name, n.Pos, n.Name)) } - ingress[md.String()] = n + ingress[key] = n } } @@ -146,13 +153,12 @@ func Validate(schema *Schema) (*Schema, error) { case *String, *Int: for _, v := range n.Variants { if reflect.TypeOf(v.Value.schemaValueType()) != reflect.TypeOf(t) { - merr = append(merr, fmt.Errorf("%s: enum variant %q of type %s cannot have a "+ - "value of type %s", v.Pos, v.Name, t, v.Value.schemaValueType())) + merr = append(merr, errorf(v, "enum variant %q of type %s cannot have a value of type %s", v.Name, t, v.Value.schemaValueType())) } } return next() default: - merr = append(merr, fmt.Errorf("%s: enum type must be String or Int, not %s", n.Pos, n.Type)) + merr = append(merr, errorf(n, "enum type must be String or Int, not %s", n.Type)) } case *Array, *Bool, *Bytes, *Data, *Database, Decl, *Field, *Float, @@ -160,7 +166,7 @@ func Validate(schema *Schema) (*Schema, error) { *Int, *Map, Metadata, *MetadataCalls, *MetadataDatabases, *MetadataIngress, *MetadataAlias, *Module, *Optional, *Schema, *String, *Time, Type, *Unit, *Any, *TypeParameter, *EnumVariant, - Value, *IntValue, *StringValue, *Config, *Secret, Symbol: + Value, *IntValue, *StringValue, *Config, *Secret, Symbol, Named: } return next() }) @@ -189,10 +195,10 @@ func ValidateModule(module *Module) error { scopes := NewScopes() if !ValidateName(module.Name) { - merr = append(merr, fmt.Errorf("%s: module name %q is invalid", module.Pos, module.Name)) + merr = append(merr, errorf(module, "module name %q is invalid", module.Name)) } if module.Builtin && module.Name != "builtin" { - merr = append(merr, fmt.Errorf("%s: only the \"ftl\" module can be marked as builtin", module.Pos)) + merr = append(merr, errorf(module, "only the \"ftl\" module can be marked as builtin")) } if err := scopes.Add(nil, module.Name, module); err != nil { merr = append(merr, err) @@ -213,69 +219,71 @@ func ValidateModule(module *Module) error { n.Module = mdecl.Module.Name } if len(n.TypeParameters) != 0 { - merr = append(merr, fmt.Errorf("%s: reference to %s %q cannot have type parameters", - n.Pos, reflect.TypeOf(decl).Elem().Name(), n.Name)) + merr = append(merr, errorf(n, "reference to %s %q cannot have type parameters", + reflect.TypeOf(decl).Elem().Name(), n.Name)) } case *Data: if n.Module == "" { n.Module = mdecl.Module.Name } if len(n.TypeParameters) != len(decl.TypeParameters) { - merr = append(merr, fmt.Errorf("%s: reference to data structure %s has %d type parameters, but %d were expected", - n.Pos, n.Name, len(n.TypeParameters), len(decl.TypeParameters))) + merr = append(merr, errorf(n, "reference to data structure %s has %d type parameters, but %d were expected", + n.Name, len(n.TypeParameters), len(decl.TypeParameters))) } case *TypeParameter: default: if n.Module == "" { - merr = append(merr, fmt.Errorf("%s: unqualified reference to invalid %s %q", n.Pos, reflect.TypeOf(decl).Elem().Name(), n)) + merr = append(merr, errorf(n, "unqualified reference to invalid %s %q", reflect.TypeOf(decl).Elem().Name(), n)) } n.Module = mdecl.Module.Name } } else if n.Module == "" || n.Module == module.Name { // Don't report errors for external modules. - merr = append(merr, fmt.Errorf("%s: unknown reference %q", n.Pos, n)) + merr = append(merr, errorf(n, "unknown reference %q", n)) } case *Verb: if !ValidateName(n.Name) { - merr = append(merr, fmt.Errorf("%s: Verb name %q is invalid", n.Pos, n.Name)) + merr = append(merr, errorf(n, "Verb name %q is invalid", n.Name)) } if _, ok := primitivesScope[n.Name]; ok { - merr = append(merr, fmt.Errorf("%s: Verb name %q is a reserved word", n.Pos, n.Name)) + merr = append(merr, errorf(n, "Verb name %q is a reserved word", n.Name)) } + merr = append(merr, validateVerbMetadata(scopes, n)...) + case *Data: if !ValidateName(n.Name) { - merr = append(merr, fmt.Errorf("%s: data structure name %q is invalid", n.Pos, n.Name)) + merr = append(merr, errorf(n, "data structure name %q is invalid", n.Name)) } if _, ok := primitivesScope[n.Name]; ok { - merr = append(merr, fmt.Errorf("%s: data structure name %q is a reserved word", n.Pos, n.Name)) + merr = append(merr, errorf(n, "data structure name %q is a reserved word", n.Name)) } for _, md := range n.Metadata { if md, ok := md.(*MetadataCalls); ok { - merr = append(merr, fmt.Errorf("%s: metadata %q is not valid on data structures", md.Pos, strings.TrimSpace(md.String()))) + merr = append(merr, errorf(md, "metadata %q is not valid on data structures", strings.TrimSpace(md.String()))) } } case *Config: if !ValidateName(n.Name) { - merr = append(merr, fmt.Errorf("%s: config name %q is invalid", n.Pos, n.Name)) + merr = append(merr, errorf(n, "config name %q is invalid", n.Name)) } if _, ok := primitivesScope[n.Name]; ok { - merr = append(merr, fmt.Errorf("%s: config name %q is a reserved word", n.Pos, n.Name)) + merr = append(merr, errorf(n, "config name %q is a reserved word", n.Name)) } case *Secret: if !ValidateName(n.Name) { - merr = append(merr, fmt.Errorf("%s: secret name %q is invalid", n.Pos, n.Name)) + merr = append(merr, errorf(n, "secret name %q is invalid", n.Name)) } if _, ok := primitivesScope[n.Name]; ok { - merr = append(merr, fmt.Errorf("%s: secret name %q is a reserved word", n.Pos, n.Name)) + merr = append(merr, errorf(n, "secret name %q is a reserved word", n.Name)) } case *Field: for _, md := range n.Metadata { if _, ok := md.(*MetadataAlias); !ok { - merr = append(merr, fmt.Errorf("%s: metadata %q is not valid on fields", md.Position(), strings.TrimSpace(md.String()))) + merr = append(merr, errorf(md, "metadata %q is not valid on fields", strings.TrimSpace(md.String()))) } } @@ -285,7 +293,7 @@ func ValidateModule(module *Module) error { IngressPathComponent, *IngressPathLiteral, *IngressPathParameter, *Optional, *Unit, *Any, *TypeParameter, *Enum, *EnumVariant, *IntValue, *StringValue: - case Symbol, Type, Metadata, Decl, Value: // Union types. + case Named, Symbol, Type, Metadata, Decl, Value: // Union types. } return next() }) @@ -422,3 +430,69 @@ func dfsForDependencyCycle(imports map[string][]string, vertexStates map[depende return nil } + +func errorf(pos interface{ Position() Position }, format string, args ...interface{}) error { + return participle.Errorf(lexer.Position(pos.Position()), format, args...) +} + +func validateVerbMetadata(scopes Scopes, n *Verb) (merr []error) { + // Validate metadata + for _, md := range n.Metadata { + switch md := md.(type) { + case *MetadataIngress: + reqBodyType, reqBody, errs := validateIngressRequestOrResponse(scopes, n, "request", n.Request) + merr = append(merr, errs...) + _, _, errs = validateIngressRequestOrResponse(scopes, 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 !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)) + } + + case *IngressPathLiteral: + } + } + + case *MetadataCalls, *MetadataDatabases, *MetadataAlias: + } + } + return +} + +func validateIngressRequestOrResponse(scopes Scopes, n *Verb, reqOrResp string, r Type) (fieldType Type, body Symbol, merr []error) { + rref, _ := r.(*Ref) + resp, sym := ResolveTypeAs[*Data](scopes, r) + if sym == nil || sym.Module == nil || sym.Module.Name != "builtin" || resp.Name != "Http"+strings.Title(reqOrResp) { + merr = append(merr, errorf(r, "ingress verb %s: %s type %s must be builtin.HttpRequest", n.Name, reqOrResp, r)) + } else { + resp, err := resp.Monomorphise(rref) //nolint:govet + if err != nil { + merr = append(merr, errorf(r, "ingress verb %s: %s type %s could not be monomorphised: %v", n.Name, reqOrResp, r, err)) + } else { + scopes = scopes.PushScope(resp.Scope()) + fieldType = resp.FieldByName("body").Type + if opt, ok := fieldType.(*Optional); ok { + fieldType = opt.Type + } + bodySym := scopes.ResolveType(fieldType) + if bodySym == nil { + merr = append(merr, errorf(resp, "ingress verb %s: couldn't resolve %s body type %s", n.Name, reqOrResp, fieldType)) + } else { + body = bodySym.Symbol + switch bodySym.Symbol.(type) { + case *Bytes, *String, *Data: // Valid HTTP response payload types. + default: + merr = append(merr, errorf(r, "ingress verb %s: %s type %s must have a body of type Bytes, String or Data, not %s", n.Name, reqOrResp, r, bodySym.Symbol)) + } + } + } + } + return +} diff --git a/backend/schema/validate_test.go b/backend/schema/validate_test.go new file mode 100644 index 0000000000..98a5ca963c --- /dev/null +++ b/backend/schema/validate_test.go @@ -0,0 +1,162 @@ +package schema + +import ( + "testing" + + "github.com/alecthomas/assert/v2" + + "github.com/TBD54566975/ftl/internal/errors" + "github.com/TBD54566975/ftl/internal/slices" +) + +func TestValidate(t *testing.T) { + tests := []struct { + name string + schema string + errs []string + }{ + {name: "TwoModuleCycle", + schema: ` + module one { + verb one(Empty) Empty + +calls two.two + } + + module two { + verb two(Empty) Empty + +calls one.one + } + `, + errs: []string{"found cycle in dependencies: two -> one -> two"}}, + {name: "ThreeModulesNoCycle", + schema: ` + module one { + verb one(Empty) Empty + +calls two.two + } + + module two { + verb two(Empty) Empty + +calls three.three + } + + module three { + verb three(Empty) Empty + } + `}, + {name: "ThreeModulesCycle", + schema: ` + module one { + verb one(Empty) Empty + +calls two.two + } + + module two { + verb two(Empty) Empty + +calls three.three + } + + module three { + verb three(Empty) Empty + +calls one.one + } + `, + errs: []string{"found cycle in dependencies: two -> three -> one -> two"}}, + {name: "TwoModuleCycleDiffVerbs", + schema: ` + module one { + verb a(Empty) Empty + +calls two.a + verb b(Empty) Empty + } + + module two { + verb a(Empty) Empty + +calls one.b + } + `, + errs: []string{"found cycle in dependencies: two -> one -> two"}}, + {name: "SelfReference", + schema: ` + module one { + verb a(Empty) Empty + +calls one.b + + verb b(Empty) Empty + +calls one.a + } + `}, + {name: "ValidIngressRequestType", + schema: ` + module one { + verb a(HttpRequest) HttpResponse + +ingress http GET /a + } + `}, + {name: "InvalidIngressRequestType", + schema: ` + module one { + verb a(Empty) Empty + +ingress http GET /a + } + `, + errs: []string{ + "3:13: ingress verb a: request type Empty must be builtin.HttpRequest", + "3:20: ingress verb a: response type Empty must be builtin.HttpRequest", + }}, + {name: "IngressBodyTypes", + schema: ` + module one { + verb bytes(HttpRequest) HttpResponse + +ingress http GET /bytes + verb string(HttpRequest) HttpResponse + +ingress http GET /string + verb data(HttpRequest) HttpResponse + +ingress http GET /data + + // Invalid types. + verb int(HttpRequest) HttpResponse + +ingress http GET /int + verb bool(HttpRequest) HttpResponse + +ingress http GET /bool + verb any(HttpRequest) HttpResponse + +ingress http GET /any + verb path(HttpRequest) HttpResponse + +ingress http GET /path/{invalid} + verb pathMissing(HttpRequest) HttpResponse + +ingress http GET /path/{missing} + verb pathFound(HttpRequest) HttpResponse + +ingress http GET /path/{parameter} + + data Path { + parameter String + } + } + `, + errs: []string{ + "11:15: ingress verb int: request type HttpRequest must have a body of type Bytes, String or Data, not Int", + "11:33: ingress verb int: response type HttpResponse must have a body of type Bytes, String or Data, not Int", + "13:16: ingress verb bool: request type HttpRequest must have a body of type Bytes, String or Data, not Bool", + "13:35: ingress verb bool: response type HttpResponse must have a body of type Bytes, String or Data, not Bool", + "15:15: ingress verb any: request type HttpRequest must have a body of type Bytes, String or Data, not Any", + "15:33: ingress verb any: response type HttpResponse must have a body of type Bytes, String or Data, not Any", + "18:31: ingress verb path: cannot use path parameter \"invalid\" with request type String, expected Data type", + "20:7: duplicate http ingress GET /path/{} for 21:6:\"pathFound\" and 19:6:\"pathMissing\"", + "20:31: ingress verb pathMissing: request type Path does not contain a field corresponding to the parameter \"missing\"", + "22:7: duplicate http ingress GET /path/{} for 17:6:\"path\" and 21:6:\"pathFound\"", + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + _, err := ParseString("", test.schema) + if test.errs == nil { + assert.NoError(t, err) + } else { + errs := slices.Map(errors.UnwrapAll(err), func(e error) string { return e.Error() }) + assert.Equal(t, test.errs, errs) + } + }) + } +}