Skip to content

Commit

Permalink
fix: all references without module get normalized to its module (#3829)
Browse files Browse the repository at this point in the history 
- 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.
  • Loading branch information
@matt2e
matt2e authored Dec 20, 2024
1 parent d408f35 commit e085fc2
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 71 deletions.
52 changes: 23 additions & 29 deletions common/schema/validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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()
Expand All @@ -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:
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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() {
Expand Down
84 changes: 42 additions & 42 deletions common/schema/validate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,112 +21,112 @@ 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
}
`,
errs: []string{"found cycle in dependencies: two -> one -> two"}},
{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
}
`,
errs: []string{"found cycle in dependencies: two -> three -> one -> two"}},
{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
}
`,
errs: []string{"found cycle in dependencies: two -> one -> two"}},
{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<Unit, Unit, Unit>) HttpResponse<Empty, Empty>
export verb a(builtin.HttpRequest<Unit, Unit, Unit>) builtin.HttpResponse<builtin.Empty, builtin.Empty>
+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<Bytes, Unit, Unit>) HttpResponse<Bytes, Bytes>
export verb bytes(builtin.HttpRequest<Bytes, Unit, Unit>) builtin.HttpResponse<Bytes, Bytes>
+ingress http POST /bytes
export verb string(HttpRequest<String, Unit, Unit>) HttpResponse<String, String>
export verb string(builtin.HttpRequest<String, Unit, Unit>) builtin.HttpResponse<String, String>
+ingress http POST /string
export verb data(HttpRequest<Empty, Unit, Unit>) HttpResponse<Empty, Empty>
export verb data(builtin.HttpRequest<builtin.Empty, Unit, Unit>) builtin.HttpResponse<builtin.Empty, builtin.Empty>
+ingress http POST /data
// Invalid types.
export verb any(HttpRequest<Any, Unit, Unit>) HttpResponse<Any, Any>
export verb any(builtin.HttpRequest<Any, Unit, Unit>) builtin.HttpResponse<Any, Any>
+ingress http GET /any
export verb path(HttpRequest<Unit, String, Unit>) HttpResponse<String, String>
export verb path(builtin.HttpRequest<Unit, String, Unit>) builtin.HttpResponse<String, String>
+ingress http GET /path/{invalid}
export verb pathInvalid(HttpRequest<Unit, String, Unit>) HttpResponse<String, String>
export verb pathInvalid(builtin.HttpRequest<Unit, String, Unit>) builtin.HttpResponse<String, String>
+ingress http GET /path/{invalid}/{extra}
export verb pathMissing(HttpRequest<Unit, one.Path, Unit>) HttpResponse<String, String>
export verb pathMissing(builtin.HttpRequest<Unit, one.Path, Unit>) builtin.HttpResponse<String, String>
+ingress http GET /path/{missing}
export verb pathFound(HttpRequest<Unit, one.Path, Unit>) HttpResponse<String, String>
export verb pathFound(builtin.HttpRequest<Unit, one.Path, Unit>) builtin.HttpResponse<String, String>
+ingress http GET /path/{parameter}
// Data comment
Expand All @@ -136,8 +136,8 @@ func TestValidate(t *testing.T) {
}
`,
errs: []string{
"11:22: ingress verb any: GET request type HttpRequest<Any, Unit, Unit> must have a body of unit not Any",
"11:52: ingress verb any: response type HttpResponse<Any, Any> 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<Any, Unit, Unit> must have a body of unit not Any",
"11:60: ingress verb any: response type builtin.HttpResponse<Any, Any> 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\"",
Expand All @@ -147,18 +147,18 @@ func TestValidate(t *testing.T) {
{name: "GetRequestWithBody",
schema: `
module one {
export verb bytes(HttpRequest<Bytes, Unit, Unit>) HttpResponse<Bytes, Bytes>
export verb bytes(builtin.HttpRequest<Bytes, Unit, Unit>) builtin.HttpResponse<Bytes, Bytes>
+ingress http GET /bytes
}
`,
errs: []string{
"3:24: ingress verb bytes: GET request type HttpRequest<Bytes, Unit, Unit> must have a body of unit not Bytes",
"3:24: ingress verb bytes: GET request type builtin.HttpRequest<Bytes, Unit, Unit> 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
}
`,
Expand All @@ -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
}
Expand All @@ -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 * * * * * * *
}
`,
Expand All @@ -208,7 +208,7 @@ func TestValidate(t *testing.T) {
export data Data {}
}
module one {
export verb a(HttpRequest<two.Data, Unit, Unit>) HttpResponse<two.Data, Empty>
export verb a(builtin.HttpRequest<two.Data, Unit, Unit>) builtin.HttpResponse<two.Data, builtin.Empty>
+ingress http GET /a
}
`,
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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
}
`,
Expand Down Expand Up @@ -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{
Expand Down

0 comments on commit e085fc2

Please sign in to comment.