From 64b2f2102b09bed6ad61fdfaf9dac442f90ff60b Mon Sep 17 00:00:00 2001 From: Denise Li Date: Thu, 25 Apr 2024 21:15:46 -0400 Subject: [PATCH] fix: prevent duplicate configs, secrets, databases within module (#1336) Fixes https://github.com/TBD54566975/ftl/issues/1121 This adds backend schema validation preventing duplicate configs, secrets, and databases within a module. This also adds go-runtime validation preventing duplicate databases. Go-runtime validation for duplicate configs/secrets was added in PR https://github.com/TBD54566975/ftl/pull/1328 --- backend/schema/validate.go | 64 +++++++++++++++ backend/schema/validate_test.go | 80 ++++++++++++++++++- go-runtime/compile/schema.go | 14 +++- go-runtime/compile/schema_test.go | 59 +++++++------- .../compile/testdata/failing/failing.go | 3 + 5 files changed, 188 insertions(+), 32 deletions(-) diff --git a/backend/schema/validate.go b/backend/schema/validate.go index fbc0f17bde..e8b2fa4e1a 100644 --- a/backend/schema/validate.go +++ b/backend/schema/validate.go @@ -15,6 +15,7 @@ import ( "github.com/TBD54566975/ftl/internal/cron" "github.com/TBD54566975/ftl/internal/errors" dc "github.com/TBD54566975/ftl/internal/reflect" + ftlslices "github.com/TBD54566975/ftl/internal/slices" ) var ( @@ -312,6 +313,10 @@ func ValidateModule(module *Module) error { } return next() }) + + // Scan declarations for duplicates + merr = append(merr, findDuplicateDecls(module.Decls)...) + merr = cleanErrors(merr) sort.SliceStable(module.Decls, func(i, j int) bool { iDecl := module.Decls[i] @@ -326,6 +331,65 @@ func ValidateModule(module *Module) error { return errors.Join(merr...) } +func findDuplicateDecls(decls []Decl) []error { + merr := []error{} + + configs := make(map[string][]int) + secrets := make(map[string][]int) + dbs := make(map[string]int) + + for i, d := range decls { + c, ok := d.(*Config) + if ok { // found a config + if _, ok := configs[c.Name]; !ok { // no matches possible + configs[c.Name] = []int{i} + continue + } + matchIdx, foundMatch := ftlslices.Find(configs[c.Name], func(j int) bool { + maybeMatch, _ := decls[j].(*Config) + return maybeMatch.Type.String() == c.Type.String() + }) + if foundMatch { + match, _ := decls[matchIdx].(*Config) + merr = append(merr, errorf(d, "duplicate config declaration at %d:%d", match.Pos.Line, match.Pos.Column)) + } else { + configs[c.Name] = append(configs[c.Name], i) + } + continue + } + + s, ok := d.(*Secret) + if ok { // found a secret + if _, ok := secrets[s.Name]; !ok { // no matches possible + secrets[s.Name] = []int{i} + continue + } + matchIdx, foundMatch := ftlslices.Find(secrets[s.Name], func(j int) bool { + maybeMatch, _ := decls[j].(*Secret) + return maybeMatch.Type.String() == s.Type.String() + }) + if foundMatch { + match, _ := decls[matchIdx].(*Secret) + merr = append(merr, errorf(d, "duplicate secret declaration at %d:%d", match.Pos.Line, match.Pos.Column)) + } else { + secrets[s.Name] = append(secrets[s.Name], i) + } + continue + } + + db, ok := d.(*Database) + if ok { // found a database + if _, ok := dbs[db.Name]; !ok { // no matches + dbs[db.Name] = i + continue + } + match, _ := decls[dbs[db.Name]].(*Database) + merr = append(merr, errorf(d, "duplicate database declaration at %d:%d", match.Pos.Line, match.Pos.Column)) + } + } + return merr +} + // getDeclSortingPriority (used for schema sorting) is pulled out into it's own switch so the Go sumtype check will fail // if a new Decl is added but not explicitly prioritized func getDeclSortingPriority(decl Decl) int { diff --git a/backend/schema/validate_test.go b/backend/schema/validate_test.go index a1c8bc27c8..51ee382953 100644 --- a/backend/schema/validate_test.go +++ b/backend/schema/validate_test.go @@ -194,7 +194,85 @@ func TestValidate(t *testing.T) { verb a(HttpRequest) HttpResponse +ingress http GET /a } - `}, + `, + }, + {name: "DuplicateConfigsSimple", + schema: ` + module one { + config FTL_ENDPOINT String + config FTL_ENDPOINT String + } + `, + errs: []string{ + "4:41-41: duplicate config declaration at 3:41", + }, + }, + {name: "DuplicateConfigsDiffTypes", + schema: ` + module one { + config FTL_ENDPOINT String + config FTL_ENDPOINT Any + } + `, + }, + {name: "DuplicateConfigsMultiple", + schema: ` + module one { + config FTL_ENDPOINT String + config FTL_ENDPOINT Any + config FTL_ENDPOINT String + config FTL_ENDPOINT String + } + `, + errs: []string{ + "5:41-41: duplicate config declaration at 3:41", + "6:41-41: duplicate config declaration at 3:41", + }, + }, + {name: "DuplicateSecretsSimple", + schema: ` + module one { + secret MY_SECRET String + secret MY_SECRET String + } + `, + errs: []string{ + "4:41-41: duplicate secret declaration at 3:41", + }, + }, + {name: "DuplicateSecretsDiffTypes", + schema: ` + module one { + secret MY_SECRET String + secret MY_SECRET Any + } + `, + }, + {name: "DuplicateSecretsMultiple", + schema: ` + module one { + secret MY_SECRET String + secret MY_SECRET Any + secret MY_SECRET String + secret MY_SECRET String + } + `, + errs: []string{ + "5:41-41: duplicate secret declaration at 3:41", + "6:41-41: duplicate secret declaration at 3:41", + }, + }, + {name: "DuplicateDatabasesSimple", + schema: ` + module one { + database MY_DB + database MY_DB + } + `, + errs: []string{ + "4:41-41: duplicate database declaration at 3:41", + }, + }, } for _, test := range tests { diff --git a/go-runtime/compile/schema.go b/go-runtime/compile/schema.go index b9d1fe8e7f..99c79b163d 100644 --- a/go-runtime/compile/schema.go +++ b/go-runtime/compile/schema.go @@ -259,16 +259,15 @@ func parseConfigDecl(pctx *parseContext, node *ast.CallExpr, fn *types.Func) { } // Check for duplicates + _, endCol := goNodePosToSchemaPos(node) for _, d := range pctx.module.Decls { c, ok := d.(*schema.Config) if ok && c.Name == name && c.Type.String() == st.String() { - _, endCol := goNodePosToSchemaPos(node) pctx.errors.add(errorf(node, "duplicate config declaration at %d:%d-%d", c.Pos.Line, c.Pos.Column, endCol)) return } s, ok := d.(*schema.Secret) if ok && s.Name == name && s.Type.String() == st.String() { - _, endCol := goNodePosToSchemaPos(node) pctx.errors.add(errorf(node, "duplicate secret declaration at %d:%d-%d", s.Pos.Line, s.Pos.Column, endCol)) return } @@ -293,6 +292,17 @@ func parseDatabaseDecl(pctx *parseContext, node *ast.CallExpr) { pctx.errors.add(errorf(node, "config and secret declarations must have a single string literal argument")) return } + + // Check for duplicates + _, endCol := goNodePosToSchemaPos(node) + for _, d := range pctx.module.Decls { + db, ok := d.(*schema.Database) + if ok && db.Name == name { + pctx.errors.add(errorf(node, "duplicate database declaration at %d:%d-%d", db.Pos.Line, db.Pos.Column, endCol)) + return + } + } + decl := &schema.Database{ Pos: goPosToSchemaPos(node.Pos()), Name: name, diff --git a/go-runtime/compile/schema_test.go b/go-runtime/compile/schema_test.go index ec5ff71c5f..4c833535d3 100644 --- a/go-runtime/compile/schema_test.go +++ b/go-runtime/compile/schema_test.go @@ -249,35 +249,36 @@ func TestErrorReporting(t *testing.T) { filename+":10:13-35: config and secret declarations must have a single string literal argument\n"+ filename+":13:18-52: duplicate config declaration at 12:18-52\n"+ filename+":16:18-49: duplicate secret declaration at 15:18-49\n"+ - filename+":19:2-10: unsupported type \"error\" for field \"BadParam\"\n"+ - filename+":22:2-17: unsupported type \"uint64\" for field \"AnotherBadParam\"\n"+ - filename+":25:3-3: unexpected token \"verb\" (expected Directive)\n"+ - filename+":31:36-39: unsupported request type \"ftl/failing.Request\"\n"+ - filename+":31:50-50: unsupported response type \"ftl/failing.Response\"\n"+ - filename+":32:16-29: call first argument must be a function in an ftl module\n"+ - filename+":33:2-46: call must have exactly three arguments\n"+ - filename+":34:16-25: call first argument must be a function\n"+ - filename+":39:1-2: must have at most two parameters (context.Context, struct)\n"+ - filename+":39:69-69: unsupported response type \"ftl/failing.Response\"\n"+ - filename+":44:22-27: first parameter must be of type context.Context but is ftl/failing.Request\n"+ - filename+":44:37-43: second parameter must be a struct but is string\n"+ - filename+":44:53-53: unsupported response type \"ftl/failing.Response\"\n"+ - filename+":49:43-47: second parameter must not be ftl.Unit\n"+ - filename+":49:59-59: unsupported response type \"ftl/failing.Response\"\n"+ - filename+":54:1-2: first parameter must be context.Context\n"+ - filename+":54:18-18: unsupported response type \"ftl/failing.Response\"\n"+ - filename+":59:1-2: must have at most two results (struct, error)\n"+ - filename+":59:41-44: unsupported request type \"ftl/failing.Request\"\n"+ - filename+":64:1-2: must at least return an error\n"+ - filename+":64:36-39: unsupported request type \"ftl/failing.Request\"\n"+ - filename+":68:35-38: unsupported request type \"ftl/failing.Request\"\n"+ - filename+":68:48-48: must return an error but is ftl/failing.Response\n"+ - filename+":73:41-44: unsupported request type \"ftl/failing.Request\"\n"+ - filename+":73:55-55: first result must be a struct but is string\n"+ - filename+":73:63-63: must return an error but is string\n"+ - filename+":73:63-63: second result must not be ftl.Unit\n"+ - filename+":80:1-1: verb \"WrongResponse\" already exported\n"+ - filename+":86:2-12: struct field unexported must be exported by starting with an uppercase letter", + filename+":19:14-44: duplicate database declaration at 18:14-44\n"+ + filename+":22:2-10: unsupported type \"error\" for field \"BadParam\"\n"+ + filename+":25:2-17: unsupported type \"uint64\" for field \"AnotherBadParam\"\n"+ + filename+":28:3-3: unexpected token \"verb\" (expected Directive)\n"+ + filename+":34:36-39: unsupported request type \"ftl/failing.Request\"\n"+ + filename+":34:50-50: unsupported response type \"ftl/failing.Response\"\n"+ + filename+":35:16-29: call first argument must be a function in an ftl module\n"+ + filename+":36:2-46: call must have exactly three arguments\n"+ + filename+":37:16-25: call first argument must be a function\n"+ + filename+":42:1-2: must have at most two parameters (context.Context, struct)\n"+ + filename+":42:69-69: unsupported response type \"ftl/failing.Response\"\n"+ + filename+":47:22-27: first parameter must be of type context.Context but is ftl/failing.Request\n"+ + filename+":47:37-43: second parameter must be a struct but is string\n"+ + filename+":47:53-53: unsupported response type \"ftl/failing.Response\"\n"+ + filename+":52:43-47: second parameter must not be ftl.Unit\n"+ + filename+":52:59-59: unsupported response type \"ftl/failing.Response\"\n"+ + filename+":57:1-2: first parameter must be context.Context\n"+ + filename+":57:18-18: unsupported response type \"ftl/failing.Response\"\n"+ + filename+":62:1-2: must have at most two results (struct, error)\n"+ + filename+":62:41-44: unsupported request type \"ftl/failing.Request\"\n"+ + filename+":67:1-2: must at least return an error\n"+ + filename+":67:36-39: unsupported request type \"ftl/failing.Request\"\n"+ + filename+":71:35-38: unsupported request type \"ftl/failing.Request\"\n"+ + filename+":71:48-48: must return an error but is ftl/failing.Response\n"+ + filename+":76:41-44: unsupported request type \"ftl/failing.Request\"\n"+ + filename+":76:55-55: first result must be a struct but is string\n"+ + filename+":76:63-63: must return an error but is string\n"+ + filename+":76:63-63: second result must not be ftl.Unit\n"+ + filename+":83:1-1: verb \"WrongResponse\" already exported\n"+ + filename+":89:2-12: struct field unexported must be exported by starting with an uppercase letter", ) } diff --git a/go-runtime/compile/testdata/failing/failing.go b/go-runtime/compile/testdata/failing/failing.go index c10acfe290..988116da85 100644 --- a/go-runtime/compile/testdata/failing/failing.go +++ b/go-runtime/compile/testdata/failing/failing.go @@ -15,6 +15,9 @@ var duplConfig = ftl.Config[string]("FTL_ENDPOINT") var goodSecret = ftl.Secret[string]("MY_SECRET") var duplSecret = ftl.Secret[string]("MY_SECRET") +var goodDB = ftl.PostgresDatabase("testDb") +var duplDB = ftl.PostgresDatabase("testDb") + type Request struct { BadParam error }