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: handle refs without module #3817

Closed
wants to merge 2 commits into from
Closed
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
21 changes: 17 additions & 4 deletions common/schema/typeresolver.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package schema

import (
"errors"
"fmt"
"strings"

Expand Down Expand Up @@ -77,10 +78,8 @@ func NewScopes() Scopes {
if err := scopes.Add(optional.None[*Module](), builtins.Name, builtins); err != nil {
panic(err)
}
for _, decl := range builtins.Decls {
if err := scopes.Add(optional.Some(builtins), decl.GetName(), decl); err != nil {
panic(err)
}
if err := scopes.AddModuleDecls(builtins); err != nil {
panic(err)
}
// Push an empty scope for other modules to be added to.
scopes = scopes.Push()
Expand Down Expand Up @@ -128,6 +127,20 @@ func (s *Scopes) Add(owner optional.Option[*Module], name string, symbol Symbol)
return nil
}

// AddModuleDecls adds all decls in a module to the current scope.
func (s *Scopes) AddModuleDecls(module *Module) error {
errs := []error{}
for _, decl := range module.Decls {
if err := s.Add(optional.Some(module), decl.GetName(), decl); err != nil {
errs = append(errs, err)
}
}
if len(errs) > 0 {
return errors.Join(errs...)
}
return nil
}

func (s Scopes) ResolveType(t Type) *ModuleDecl {
switch t := t.(type) {
case Named:
Expand Down
29 changes: 29 additions & 0 deletions common/schema/typeresolver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,41 @@ func TestTypeResolver(t *testing.T) {
t T
}
verb test(test.Request<String>) Empty

// This module has it's own definition of HttpRequest
data HttpRequest {
}
}
`)
assert.NoError(t, err)
otherModule, err := ParseModuleString("", `
module other {
data External {
}
}
`)
assert.NoError(t, err)
scopes := NewScopes()
err = scopes.Add(optional.None[*Module](), module.Name, module)
assert.NoError(t, err)
err = scopes.Add(optional.None[*Module](), otherModule.Name, otherModule)
assert.NoError(t, err)

// Resolving "HttpRequest" should return builtin.HttpRequest
httpRequest := scopes.Resolve(Ref{Name: "HttpRequest"})
assert.Equal(t, httpRequest.Module.MustGet().Name, "builtin")

// Push a new scope for "test" module's decls
scopes = scopes.Push()
assert.NoError(t, scopes.AddModuleDecls(module))

// Resolving "HttpRequest" should return test.HttpRequest now that we've pushed the new scope
httpRequest = scopes.Resolve(Ref{Name: "HttpRequest"})
assert.Equal(t, httpRequest.Module.MustGet().Name, "test")

// Resolving "External" should fail
external := scopes.Resolve(Ref{Name: "External"})
assert.Equal(t, external, nil)

// Resolve a builtin.
actualInt, _ := ResolveAs[*Int](scopes, Ref{Name: "Int"})
Expand Down
47 changes: 24 additions & 23 deletions common/schema/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func ValidateModuleInSchema(schema *Schema, m optional.Option[*Module]) (*Schema
schema.Modules = slices.DeleteFunc(schema.Modules, func(m *Module) bool { return m.Name == builtins.Name })
schema.Modules = append([]*Module{builtins}, schema.Modules...)

scopes := NewScopes()
generalScopes := NewScopes()

// Validate dependencies
if err := validateDependencies(schema); err != nil {
Expand All @@ -82,7 +82,7 @@ func ValidateModuleInSchema(schema *Schema, m optional.Option[*Module]) (*Schema
if module == builtins {
continue
}
if err := scopes.Add(optional.None[*Module](), module.Name, module); err != nil {
if err := generalScopes.Add(optional.None[*Module](), module.Name, module); err != nil {
merr = append(merr, err)
}
}
Expand All @@ -106,6 +106,12 @@ func ValidateModuleInSchema(schema *Schema, m optional.Option[*Module]) (*Schema
merr = append(merr, err)
}

// Push current decls
scopes := generalScopes.Push()
if err := scopes.AddModuleDecls(module); err != nil {
merr = append(merr, err)
}

indent := 0
err := Visit(module, func(n Node, next func() error) error {
save := scopes
Expand All @@ -115,6 +121,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 +233,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 @@ -258,23 +269,24 @@ func ValidateModule(module *Module) error {
merr = append(merr, err)
}
scopes = scopes.Push()
// Key is <type>:<name>
duplicateDecls := map[string]Decl{}
if err := scopes.AddModuleDecls(module); err != nil {
merr = append(merr, err)
}

_ = Visit(module, func(n Node, next func() error) error { //nolint:errcheck
if scoped, ok := n.(Scoped); ok {
pop := scopes
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()
if dupl, ok := duplicateDecls[duplKey]; ok {
merr = append(merr, errorf(n, "duplicate %s %q, first defined at %s", tname, n.GetName(), dupl.Position()))
} else {
duplicateDecls[duplKey] = n
}
if !ValidateName(n.GetName()) {
merr = append(merr, errorf(n, "%s name %q is invalid", tname, n.GetName()))
} else if _, ok := primitivesScope[n.GetName()]; ok {
Expand Down Expand Up @@ -351,17 +363,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 @@ -908,9 +915,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 +963,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
28 changes: 10 additions & 18 deletions common/schema/validate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,8 @@ func TestValidate(t *testing.T) {
}
`,
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:27: ingress verb a: response type builtin.Empty must be builtin.HttpResponse",
}},
{name: "IngressBodyTypes",
schema: `
Expand Down Expand Up @@ -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:52: 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 @@ -152,7 +152,7 @@ func TestValidate(t *testing.T) {
}
`,
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: `
Expand Down Expand Up @@ -222,8 +222,8 @@ func TestValidate(t *testing.T) {
}
`,
errs: []string{
`4:21: duplicate config "FTL_ENDPOINT", first defined at 3:20`,
`5:21: duplicate config "FTL_ENDPOINT", first defined at 3:20`,
`4:21: duplicate declaration of "FTL_ENDPOINT" at 3:20`,
`5:21: duplicate declaration of "FTL_ENDPOINT" at 3:20`,
},
},
{name: "DuplicateSecrets",
Expand All @@ -235,18 +235,10 @@ func TestValidate(t *testing.T) {
}
`,
errs: []string{
`4:6: duplicate secret "MY_SECRET", first defined at 3:6`,
`5:6: duplicate secret "MY_SECRET", first defined at 3:6`,
`4:6: duplicate declaration of "MY_SECRET" at 3:6`,
`5:6: duplicate declaration of "MY_SECRET" at 3:6`,
},
},
{name: "ConfigAndSecretsWithSameName",
schema: `
module one {
config FTL_ENDPOINT String
secret FTL_ENDPOINT String
}
`,
},
{name: "DuplicateDatabases",
schema: `
module one {
Expand All @@ -255,7 +247,7 @@ func TestValidate(t *testing.T) {
}
`,
errs: []string{
`4:6: duplicate database "MY_DB", first defined at 3:6`,
`4:6: duplicate declaration of "MY_DB" at 3:6`,
},
},
{name: "ValueEnumMismatchedVariantTypes",
Expand Down
Loading