From e085fc2143c4774d7afb6807f834e9842636224e Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 20 Dec 2024 14:09:12 +1100 Subject: [PATCH] fix: all references without module get normalized to its module (#3829) - Visit each child before validating it's parent, so normalization of child nodes is done before validation of parent nodes. - A bunch of tests used `ref.Module == ""` to refer to builtins, but we want all schema to be fully qualified. --- common/schema/validate.go | 52 ++++++++++----------- common/schema/validate_test.go | 84 +++++++++++++++++----------------- 2 files changed, 65 insertions(+), 71 deletions(-) diff --git a/common/schema/validate.go b/common/schema/validate.go index 5a28a53d5..78594bb51 100644 --- a/common/schema/validate.go +++ b/common/schema/validate.go @@ -115,6 +115,11 @@ func ValidateModuleInSchema(schema *Schema, m optional.Option[*Module]) (*Schema } indent++ defer func() { indent-- }() + + // Validate all children before validating the current node. + if err := next(); err != nil { + return err + } switch n := n.(type) { case *Ref: mdecl := scopes.Resolve(*n) @@ -222,7 +227,7 @@ func ValidateModuleInSchema(schema *Schema, m optional.Option[*Module]) (*Schema *EnumVariant, *Config, *Secret, *Topic, *DatabaseRuntime, *DatabaseRuntimeConnections, *Data, *Field: } - return next() + return nil }) if err != nil { merr = append(merr, err) @@ -267,6 +272,12 @@ func ValidateModule(module *Module) error { scopes = scopes.PushScope(scoped.Scope()) defer func() { scopes = pop }() } + + // Validate all children before validating the current node. + if err := next(); err != nil { + return err + } + if n, ok := n.(Decl); ok { tname := typeName(n) duplKey := tname + ":" + n.GetName() @@ -284,35 +295,26 @@ func ValidateModule(module *Module) error { switch n := n.(type) { case *Ref: mdecl := scopes.Resolve(*n) - if mdecl != nil { - moduleName := "" - if m, ok := mdecl.Module.Get(); ok { - moduleName = m.Name + if mdecl == nil && n.Module == "" { + // Fully qualify refs with module == "" to the current module if it can be resolved. + if internalDecl := scopes.Resolve(Ref{Module: module.Name, Name: n.Name, TypeParameters: n.TypeParameters}); internalDecl != nil { + n.Module = module.Name + mdecl = scopes.Resolve(*n) } + } + if mdecl != nil { switch decl := mdecl.Symbol.(type) { case *Verb, *Enum, *Database, *Config, *Secret, *Topic: - if n.Module == "" { - n.Module = moduleName - } if len(n.TypeParameters) != 0 { merr = append(merr, errorf(n, "reference to %s %q cannot have type parameters", typeName(decl), n.Name)) } case *Data: - if n.Module == "" { - n.Module = moduleName - } if 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, errorf(n, "unqualified reference to invalid %s %q", typeName(decl), n)) - } } - } else if n.Module == "" || n.Module == module.Name { // Don't report errors for external modules. - merr = append(merr, errorf(n, "unknown reference %q, is the type annotated and exported?", n)) } case *Verb: @@ -351,17 +353,12 @@ func ValidateModule(module *Module) error { } } - case *MetadataRetry: - if n.Catch != nil && n.Catch.Module == "" { - n.Catch.Module = module.Name - } - case Type, Metadata, Value, IngressPathComponent, DatabaseConnector, *Module, *Schema, *Optional, *TypeParameter, *EnumVariant, *Config, *Secret, *DatabaseRuntime, *DatabaseRuntimeConnections, *Database, *Enum: } - return next() + return nil }) merr = cleanErrors(merr) @@ -790,6 +787,9 @@ func resolveValidIngressReqResp(scopes Scopes, reqOrResp string, moduleDecl opti switch t := n.(type) { case *Ref: m := scopes.Resolve(*t) + if m == nil { + return optional.None[*Data](), fmt.Errorf("unknown reference %v", t) + } sym := m.Symbol return resolveValidIngressReqResp(scopes, reqOrResp, optional.Some(m), sym, n) case *Data: @@ -908,9 +908,6 @@ func validateQueryParamsPayloadType(n Node, r Type, v *Verb, reqOrResp string) e func validateVerbSubscriptions(module *Module, v *Verb, md *MetadataSubscriber, scopes Scopes) (merr []error) { merr = []error{} - if md.Topic.Module == "" { - md.Topic.Module = module.Name - } topicDecl := scopes.Resolve(*md.Topic) if topicDecl == nil { // errors for invalid refs are handled elsewhere @@ -959,9 +956,6 @@ func validateRetries(module *Module, retry *MetadataRetry, requestType optional. merr = append(merr, errorf(retry, "catch can only be defined on verbs")) return } - if retry.Catch.Module == "" { - retry.Catch.Module = module.Name - } catchDecl := scopes.Resolve(*retry.Catch) if catchDecl == nil { if retry.Catch.Module != "" && retry.Catch.Module != module.Name && !schema.Ok() { diff --git a/common/schema/validate_test.go b/common/schema/validate_test.go index 000d71774..cb894c366 100644 --- a/common/schema/validate_test.go +++ b/common/schema/validate_test.go @@ -21,12 +21,12 @@ func TestValidate(t *testing.T) { {name: "TwoModuleCycle", schema: ` module one { - export verb one(Empty) Empty + export verb one(builtin.Empty) builtin.Empty +calls two.two } module two { - export verb two(Empty) Empty + export verb two(builtin.Empty) builtin.Empty +calls one.one } `, @@ -34,33 +34,33 @@ func TestValidate(t *testing.T) { {name: "ThreeModulesNoCycle", schema: ` module one { - verb one(Empty) Empty + verb one(builtin.Empty) builtin.Empty +calls two.two } module two { - export verb two(Empty) Empty + export verb two(builtin.Empty) builtin.Empty +calls three.three } module three { - export verb three(Empty) Empty + export verb three(builtin.Empty) builtin.Empty } `}, {name: "ThreeModulesCycle", schema: ` module one { - export verb one(Empty) Empty + export verb one(builtin.Empty) builtin.Empty +calls two.two } module two { - export verb two(Empty) Empty + export verb two(builtin.Empty) builtin.Empty +calls three.three } module three { - export verb three(Empty) Empty + export verb three(builtin.Empty) builtin.Empty +calls one.one } `, @@ -68,13 +68,13 @@ func TestValidate(t *testing.T) { {name: "TwoModuleCycleDiffVerbs", schema: ` module one { - verb a(Empty) Empty + verb a(builtin.Empty) builtin.Empty +calls two.a - export verb b(Empty) Empty + export verb b(builtin.Empty) builtin.Empty } module two { - export verb a(Empty) Empty + export verb a(builtin.Empty) builtin.Empty +calls one.b } `, @@ -82,51 +82,51 @@ func TestValidate(t *testing.T) { {name: "SelfReference", schema: ` module one { - verb a(Empty) Empty + verb a(builtin.Empty) builtin.Empty +calls one.b - verb b(Empty) Empty + verb b(builtin.Empty) builtin.Empty +calls one.a } `}, {name: "ValidIngressRequestType", schema: ` module one { - export verb a(HttpRequest) HttpResponse + export verb a(builtin.HttpRequest) builtin.HttpResponse +ingress http GET /a } `}, {name: "InvalidIngressRequestType", schema: ` module one { - export verb a(Empty) Empty + export verb a(builtin.Empty) builtin.Empty +ingress http GET /a } `, errs: []string{ - "3:20: ingress verb a: request type Empty must be builtin.HttpRequest", - "3:27: ingress verb a: response type Empty must be builtin.HttpResponse", + "3:20: ingress verb a: request type builtin.Empty must be builtin.HttpRequest", + "3:35: ingress verb a: response type builtin.Empty must be builtin.HttpResponse", }}, {name: "IngressBodyTypes", schema: ` module one { - export verb bytes(HttpRequest) HttpResponse + export verb bytes(builtin.HttpRequest) builtin.HttpResponse +ingress http POST /bytes - export verb string(HttpRequest) HttpResponse + export verb string(builtin.HttpRequest) builtin.HttpResponse +ingress http POST /string - export verb data(HttpRequest) HttpResponse + export verb data(builtin.HttpRequest) builtin.HttpResponse +ingress http POST /data // Invalid types. - export verb any(HttpRequest) HttpResponse + export verb any(builtin.HttpRequest) builtin.HttpResponse +ingress http GET /any - export verb path(HttpRequest) HttpResponse + export verb path(builtin.HttpRequest) builtin.HttpResponse +ingress http GET /path/{invalid} - export verb pathInvalid(HttpRequest) HttpResponse + export verb pathInvalid(builtin.HttpRequest) builtin.HttpResponse +ingress http GET /path/{invalid}/{extra} - export verb pathMissing(HttpRequest) HttpResponse + export verb pathMissing(builtin.HttpRequest) builtin.HttpResponse +ingress http GET /path/{missing} - export verb pathFound(HttpRequest) HttpResponse + export verb pathFound(builtin.HttpRequest) builtin.HttpResponse +ingress http GET /path/{parameter} // Data comment @@ -136,8 +136,8 @@ func TestValidate(t *testing.T) { } `, errs: []string{ - "11:22: ingress verb any: GET request type HttpRequest must have a body of unit not Any", - "11:52: ingress verb any: response type HttpResponse must have a body of bytes, string, data structure, unit, float, int, bool, map, or array not Any", + "11:22: ingress verb any: GET request type builtin.HttpRequest must have a body of unit not Any", + "11:60: ingress verb any: response type builtin.HttpResponse must have a body of bytes, string, data structure, unit, float, int, bool, map, or array not Any", "16:31: ingress verb pathInvalid: cannot use path parameter \"invalid\" with request type String as it has multiple path parameters, expected Data or Map type", "16:41: ingress verb pathInvalid: cannot use path parameter \"extra\" with request type String as it has multiple path parameters, expected Data or Map type", "18:31: ingress verb pathMissing: request pathParameter type one.Path does not contain a field corresponding to the parameter \"missing\"", @@ -147,18 +147,18 @@ func TestValidate(t *testing.T) { {name: "GetRequestWithBody", schema: ` module one { - export verb bytes(HttpRequest) HttpResponse + export verb bytes(builtin.HttpRequest) builtin.HttpResponse +ingress http GET /bytes } `, errs: []string{ - "3:24: ingress verb bytes: GET request type HttpRequest must have a body of unit not Bytes", + "3:24: ingress verb bytes: GET request type builtin.HttpRequest must have a body of unit not Bytes", }}, {name: "Array", schema: ` module one { data Data {} - export verb one(HttpRequest<[one.Data], Unit, Unit>) HttpResponse<[one.Data], Empty> + export verb one(builtin.HttpRequest<[one.Data], Unit, Unit>) builtin.HttpResponse<[one.Data], builtin.Empty> +ingress http POST /one } `, @@ -179,7 +179,7 @@ func TestValidate(t *testing.T) { schema: ` module one { data Data {} - export verb one(HttpRequest<[one.Data], Unit, Unit>) HttpResponse<[one.Data], Empty> + export verb one(builtin.HttpRequest<[one.Data], Unit, Unit>) builtin.HttpResponse<[one.Data], builtin.Empty> +ingress http POST /one +ingress http POST /two } @@ -191,9 +191,9 @@ func TestValidate(t *testing.T) { {name: "CronOnNonEmptyVerb", schema: ` module one { - verb verbWithWrongInput(Empty) Unit + verb verbWithWrongInput(builtin.Empty) Unit +cron * * * * * * * - verb verbWithWrongOutput(Unit) Empty + verb verbWithWrongOutput(Unit) builtin.Empty +cron * * * * * * * } `, @@ -208,7 +208,7 @@ func TestValidate(t *testing.T) { export data Data {} } module one { - export verb a(HttpRequest) HttpResponse + export verb a(builtin.HttpRequest) builtin.HttpResponse +ingress http GET /a } `, @@ -272,11 +272,11 @@ func TestValidate(t *testing.T) { {name: "NonSubscriberVerbsWithRetry", schema: ` module one { - verb A(Empty) Unit + verb A(builtin.Empty) Unit +retry 10 5s 20m - verb B(Empty) Unit + verb B(builtin.Empty) Unit +retry 1m5s 20m30s - verb C(Empty) Unit + verb C(builtin.Empty) Unit } `, errs: []string{ @@ -347,7 +347,7 @@ func TestValidate(t *testing.T) { {name: "InvalidRetryInvalidSpace", schema: ` module one { - verb A(Empty) Unit + verb A(builtin.Empty) Unit +retry 10 5 s } `, @@ -491,24 +491,24 @@ func TestValidateModuleWithSchema(t *testing.T) { schema: ` module one { export data Test {} - export verb one(Empty) Empty + export verb one(builtin.Empty) builtin.Empty } `, moduleSchema: ` module two { - export verb two(Empty) one.Test + export verb two(builtin.Empty) one.Test +calls one.one }`, }, {name: "NonExportedVerbCall", schema: ` module one { - verb one(Empty) Empty + verb one(builtin.Empty) builtin.Empty } `, moduleSchema: ` module two { - export verb two(Empty) Empty + export verb two(builtin.Empty) builtin.Empty +calls one.one }`, errs: []string{