From 9f800146020e41e05dc0d2411705d835daa7f8e2 Mon Sep 17 00:00:00 2001 From: Befikadu Date: Fri, 7 Aug 2026 13:45:52 +0300 Subject: [PATCH 1/7] feat(api): allow template_name when creating project tasks via API --- api-docs.yml | 6 ++ api/projects/tasks.go | 25 +++++- api/projects/tasks_test.go | 147 +++++++++++++++++++++++--------- db/Store.go | 1 + db/Task.go | 7 +- db/sql/template.go | 31 +++++++ web/public/swagger/api-docs.yml | 6 ++ 7 files changed, 181 insertions(+), 42 deletions(-) diff --git a/api-docs.yml b/api-docs.yml index 8de4a886e5..22e5ba78aa 100644 --- a/api-docs.yml +++ b/api-docs.yml @@ -3409,11 +3409,17 @@ paths: - name: task in: body required: true + description: > + Either template_id or template_name must be given. When both are + given, template_id is used. schema: type: object properties: template_id: type: integer + template_name: + type: string + example: Build website debug: type: boolean dry_run: diff --git a/api/projects/tasks.go b/api/projects/tasks.go index b35f43bd01..385e7d21f2 100644 --- a/api/projects/tasks.go +++ b/api/projects/tasks.go @@ -37,12 +37,35 @@ func taskPool(r *http.Request) *tasks.TaskPool { } // AddTask inserts a task into the database and returns a header or returns error +// resolveTaskTemplate returns the template of the task, which may be referenced +// either by id or by name. The resolved id is written back to the task so the +// rest of the pipeline only deals with ids. +func (c *TaskController) resolveTaskTemplate(projectID int, task *db.Task) (tpl db.Template, err error) { + if task.TemplateID == 0 && task.TemplateName == "" { + err = db.NewValidationError("template_id or template_name is required") + return + } + + if task.TemplateID != 0 { + tpl, err = c.store.GetTemplate(projectID, task.TemplateID) + return + } + + tpl, err = c.store.GetTemplateByName(projectID, task.TemplateName) + if err != nil { + return + } + + task.TemplateID = tpl.ID + return +} + func (c *TaskController) AddTask(w http.ResponseWriter, r *http.Request) { project := helpers.GetFromContext(r, "project").(db.Project) user := helpers.GetFromContext(r, "user").(*db.User) taskObj := helpers.GetFromContext(r, "task").(db.Task) - tpl, err := c.store.GetTemplate(project.ID, taskObj.TemplateID) + tpl, err := c.resolveTaskTemplate(project.ID, &taskObj) if err != nil { helpers.WriteError(w, err) return diff --git a/api/projects/tasks_test.go b/api/projects/tasks_test.go index 21d6993ea6..90bfe533c3 100644 --- a/api/projects/tasks_test.go +++ b/api/projects/tasks_test.go @@ -1,54 +1,121 @@ package projects import ( - "net/url" "testing" "github.com/semaphoreui/semaphore/db" + "github.com/semaphoreui/semaphore/db/sql" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func TestParseTasksPageParams(t *testing.T) { - tests := []struct { - name string - query string - expectedPageSize int - expectedCount int // params.Count == pageSize + 1 - expectedBeforeID int - }{ - {"defaults", "", maxTasksPageSize, maxTasksPageSize + 1, 0}, - {"count and before", "count=20&before=100", 20, 21, 100}, - {"legacy limit", "limit=50", 50, 51, 0}, - {"count overrides limit", "count=10&limit=50", 10, 11, 0}, - {"page size capped at max", "count=10000", maxTasksPageSize, maxTasksPageSize + 1, 0}, - {"negative count ignored", "count=-5", maxTasksPageSize, maxTasksPageSize + 1, 0}, - {"zero count ignored", "count=0", maxTasksPageSize, maxTasksPageSize + 1, 0}, - {"invalid count ignored", "count=abc", maxTasksPageSize, maxTasksPageSize + 1, 0}, - {"negative before ignored", "count=20&before=-1", 20, 21, 0}, - {"invalid before ignored", "count=20&before=xyz", 20, 21, 0}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - query, err := url.ParseQuery(tt.query) - assert.NoError(t, err) - - params, pageSize := parseTasksPageParams(query, db.RetrieveQueryParams{}) - - assert.Equal(t, tt.expectedPageSize, pageSize) - assert.Equal(t, tt.expectedCount, params.Count) - assert.Equal(t, tt.expectedBeforeID, params.BeforeID) - }) - } +// createTaskTestTemplate creates a template usable by the task tests. Templates +// are not unique by name, so the name is a parameter to cover ambiguity. +func createTaskTestTemplate(t *testing.T, store db.Store, projectID int, repositoryID int, name string) db.Template { + t.Helper() + + tpl, err := store.CreateTemplate(db.Template{ + Name: name, + Playbook: "test.yml", + ProjectID: projectID, + RepositoryID: repositoryID, + }) + require.NoError(t, err) + + return tpl } -func TestParseTasksPageParams_PreservesBase(t *testing.T) { - base := db.RetrieveQueryParams{SortBy: "id", SortInverted: true} +func TestResolveTaskTemplate(t *testing.T) { + store := sql.CreateTestStore() + + project, err := store.CreateProject(db.Project{Name: "task template resolution"}) + require.NoError(t, err) + + key, err := store.CreateAccessKey(db.AccessKey{ + ProjectID: &project.ID, + Name: "none", + Type: db.AccessKeyNone, + }) + require.NoError(t, err) + + repo, err := store.CreateRepository(db.Repository{ + ProjectID: project.ID, + SSHKeyID: key.ID, + Name: "repo", + GitURL: "git@example.com:test/test", + GitBranch: "master", + }) + require.NoError(t, err) + + build := createTaskTestTemplate(t, store, project.ID, repo.ID, "Build website") + + otherProject, err := store.CreateProject(db.Project{Name: "other"}) + require.NoError(t, err) + + c := &TaskController{store: store} + + t.Run("resolves by id", func(t *testing.T) { + task := db.Task{TemplateID: build.ID} + + tpl, err := c.resolveTaskTemplate(project.ID, &task) + + require.NoError(t, err) + assert.Equal(t, build.ID, tpl.ID) + }) + + t.Run("resolves by name and writes the id back", func(t *testing.T) { + task := db.Task{TemplateName: "Build website"} + + tpl, err := c.resolveTaskTemplate(project.ID, &task) + + require.NoError(t, err) + assert.Equal(t, build.ID, tpl.ID) + assert.Equal(t, build.ID, task.TemplateID, "the resolved id must be written back to the task") + }) + + t.Run("id wins when both are given", func(t *testing.T) { + task := db.Task{TemplateID: build.ID, TemplateName: "does not exist"} + + tpl, err := c.resolveTaskTemplate(project.ID, &task) + + require.NoError(t, err) + assert.Equal(t, build.ID, tpl.ID) + }) + + t.Run("neither id nor name is rejected", func(t *testing.T) { + task := db.Task{} + + _, err := c.resolveTaskTemplate(project.ID, &task) + + require.Error(t, err) + assert.Contains(t, err.Error(), "template_id or template_name is required") + }) + + t.Run("unknown name is not found", func(t *testing.T) { + task := db.Task{TemplateName: "no such template"} + + _, err := c.resolveTaskTemplate(project.ID, &task) + + assert.ErrorIs(t, err, db.ErrNotFound) + }) + + t.Run("a template of another project is not found", func(t *testing.T) { + task := db.Task{TemplateName: "Build website"} + + _, err := c.resolveTaskTemplate(otherProject.ID, &task) + + assert.ErrorIs(t, err, db.ErrNotFound) + }) + + t.Run("an ambiguous name is rejected", func(t *testing.T) { + createTaskTestTemplate(t, store, project.ID, repo.ID, "Duplicate") + createTaskTestTemplate(t, store, project.ID, repo.ID, "Duplicate") + + task := db.Task{TemplateName: "Duplicate"} - params, pageSize := parseTasksPageParams(url.Values{}, base) + _, err := c.resolveTaskTemplate(project.ID, &task) - assert.Equal(t, "id", params.SortBy) - assert.True(t, params.SortInverted) - assert.Equal(t, maxTasksPageSize, pageSize) - assert.Equal(t, maxTasksPageSize+1, params.Count) + require.Error(t, err) + assert.Contains(t, err.Error(), "more than one template") + }) } diff --git a/db/Store.go b/db/Store.go index 7ef78c274c..a795ded4c3 100644 --- a/db/Store.go +++ b/db/Store.go @@ -282,6 +282,7 @@ type TemplateManager interface { CreateTemplate(template Template) (Template, error) UpdateTemplate(template Template) error GetTemplate(projectID int, templateID int) (Template, error) + GetTemplateByName(projectID int, name string) (Template, error) DeleteTemplate(projectID int, templateID int) error SetTemplateDescription(projectID int, templateID int, description string) error GetTemplateVaults(projectID int, templateID int) ([]TemplateVault, error) diff --git a/db/Task.go b/db/Task.go index bb3cc1cae0..9ec857a8fd 100644 --- a/db/Task.go +++ b/db/Task.go @@ -40,9 +40,14 @@ type AnsibleTaskParams struct { // Task is a model of a task which will be executed by the runner type Task struct { ID int `db:"id" json:"id"` - TemplateID int `db:"template_id" json:"template_id" binding:"required"` + TemplateID int `db:"template_id" json:"template_id"` ProjectID int `db:"project_id" json:"project_id"` + // TemplateName allows a task to reference its template by name instead of + // by id when it is created through the API. It is resolved to TemplateID by + // the API and never stored. + TemplateName string `db:"-" json:"template_name,omitempty"` + Status task_logger.TaskStatus `db:"status" json:"status"` // override variables diff --git a/db/sql/template.go b/db/sql/template.go index cc457c9d00..a47145767d 100644 --- a/db/sql/template.go +++ b/db/sql/template.go @@ -444,6 +444,37 @@ func (d *SqlDb) GetTemplates(projectID int, filter db.TemplateFilter, params db. return } +// GetTemplateByName returns the template of the project with the given name. +// Template names are not unique per project, so an ambiguous name is rejected +// instead of silently running one of the matching templates. +func (d *SqlDb) GetTemplateByName(projectID int, name string) (template db.Template, err error) { + var templates []db.Template + + _, err = d.selectAll( + &templates, + "select * from project__template where project_id=? and name=?", + projectID, + name) + + if err != nil { + return + } + + switch len(templates) { + case 0: + err = db.ErrNotFound + return + case 1: + default: + err = db.NewValidationError("more than one template is named " + name + ", use template_id") + return + } + + template = templates[0] + err = db.FillTemplate(d, &template) + return +} + func (d *SqlDb) GetTemplate(projectID int, templateID int) (template db.Template, err error) { err = d.selectOne( &template, diff --git a/web/public/swagger/api-docs.yml b/web/public/swagger/api-docs.yml index 5aad742cc0..9a026b4cdf 100644 --- a/web/public/swagger/api-docs.yml +++ b/web/public/swagger/api-docs.yml @@ -2740,11 +2740,17 @@ paths: - name: task in: body required: true + description: > + Either template_id or template_name must be given. When both are + given, template_id is used. schema: type: object properties: template_id: type: integer + template_name: + type: string + example: Build website debug: type: boolean dry_run: From 7418a48eb43652cf90a5be92c4b65065bbabec2e Mon Sep 17 00:00:00 2001 From: Befikadu Date: Fri, 7 Aug 2026 14:00:56 +0300 Subject: [PATCH 2/7] feat(api): update validation errors to use common_errors package --- api/projects/tasks.go | 2 +- api/projects/tasks_test.go | 48 +++++++++++++++++++++++++++++++++++++- db/sql/template.go | 2 +- 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/api/projects/tasks.go b/api/projects/tasks.go index 385e7d21f2..def6e08769 100644 --- a/api/projects/tasks.go +++ b/api/projects/tasks.go @@ -42,7 +42,7 @@ func taskPool(r *http.Request) *tasks.TaskPool { // rest of the pipeline only deals with ids. func (c *TaskController) resolveTaskTemplate(projectID int, task *db.Task) (tpl db.Template, err error) { if task.TemplateID == 0 && task.TemplateName == "" { - err = db.NewValidationError("template_id or template_name is required") + err = common_errors.NewValidationError("template_id or template_name is required") return } diff --git a/api/projects/tasks_test.go b/api/projects/tasks_test.go index 90bfe533c3..434eef8125 100644 --- a/api/projects/tasks_test.go +++ b/api/projects/tasks_test.go @@ -1,6 +1,7 @@ package projects import ( + "net/url" "testing" "github.com/semaphoreui/semaphore/db" @@ -9,6 +10,51 @@ import ( "github.com/stretchr/testify/require" ) +func TestParseTasksPageParams(t *testing.T) { + tests := []struct { + name string + query string + expectedPageSize int + expectedCount int // params.Count == pageSize + 1 + expectedBeforeID int + }{ + {"defaults", "", maxTasksPageSize, maxTasksPageSize + 1, 0}, + {"count and before", "count=20&before=100", 20, 21, 100}, + {"legacy limit", "limit=50", 50, 51, 0}, + {"count overrides limit", "count=10&limit=50", 10, 11, 0}, + {"page size capped at max", "count=10000", maxTasksPageSize, maxTasksPageSize + 1, 0}, + {"negative count ignored", "count=-5", maxTasksPageSize, maxTasksPageSize + 1, 0}, + {"zero count ignored", "count=0", maxTasksPageSize, maxTasksPageSize + 1, 0}, + {"invalid count ignored", "count=abc", maxTasksPageSize, maxTasksPageSize + 1, 0}, + {"negative before ignored", "count=20&before=-1", 20, 21, 0}, + {"invalid before ignored", "count=20&before=xyz", 20, 21, 0}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + query, err := url.ParseQuery(tt.query) + assert.NoError(t, err) + + params, pageSize := parseTasksPageParams(query, db.RetrieveQueryParams{}) + + assert.Equal(t, tt.expectedPageSize, pageSize) + assert.Equal(t, tt.expectedCount, params.Count) + assert.Equal(t, tt.expectedBeforeID, params.BeforeID) + }) + } +} + +func TestParseTasksPageParams_PreservesBase(t *testing.T) { + base := db.RetrieveQueryParams{SortBy: "id", SortInverted: true} + + params, pageSize := parseTasksPageParams(url.Values{}, base) + + assert.Equal(t, "id", params.SortBy) + assert.True(t, params.SortInverted) + assert.Equal(t, maxTasksPageSize, pageSize) + assert.Equal(t, maxTasksPageSize+1, params.Count) +} + // createTaskTestTemplate creates a template usable by the task tests. Templates // are not unique by name, so the name is a parameter to cover ambiguity. func createTaskTestTemplate(t *testing.T, store db.Store, projectID int, repositoryID int, name string) db.Template { @@ -26,7 +72,7 @@ func createTaskTestTemplate(t *testing.T, store db.Store, projectID int, reposit } func TestResolveTaskTemplate(t *testing.T) { - store := sql.CreateTestStore() + store := sql.InitConfigCreateTestStore() project, err := store.CreateProject(db.Project{Name: "task template resolution"}) require.NoError(t, err) diff --git a/db/sql/template.go b/db/sql/template.go index 3012e63d75..0eac127a12 100644 --- a/db/sql/template.go +++ b/db/sql/template.go @@ -470,7 +470,7 @@ func (d *SqlDb) GetTemplateByName(projectID int, name string) (template db.Templ return case 1: default: - err = db.NewValidationError("more than one template is named " + name + ", use template_id") + err = common_errors.NewValidationError("more than one template is named " + name + ", use template_id") return } From ecac740d8f1a7b61f337e601b5ceb72974773300 Mon Sep 17 00:00:00 2001 From: Denis Gukov Date: Sat, 8 Aug 2026 18:24:03 +0500 Subject: [PATCH 3/7] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- db/sql/template.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/sql/template.go b/db/sql/template.go index 0eac127a12..9feeafbf8d 100644 --- a/db/sql/template.go +++ b/db/sql/template.go @@ -456,7 +456,7 @@ func (d *SqlDb) GetTemplateByName(projectID int, name string) (template db.Templ _, err = d.selectAll( &templates, - "select * from project__template where project_id=? and name=?", + "select * from project__template where project_id=? and name=? limit 2", projectID, name) From 07f95a89d9f69af58406f342fcd8963453db2ac6 Mon Sep 17 00:00:00 2001 From: Befikadu Date: Wed, 12 Aug 2026 09:47:23 +0300 Subject: [PATCH 4/7] feat(api): ensure template_name is cleared when resolving task template --- api/projects/tasks.go | 9 +++++++-- api/projects/tasks_test.go | 2 ++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/api/projects/tasks.go b/api/projects/tasks.go index def6e08769..df7e0c1ec4 100644 --- a/api/projects/tasks.go +++ b/api/projects/tasks.go @@ -41,7 +41,12 @@ func taskPool(r *http.Request) *tasks.TaskPool { // either by id or by name. The resolved id is written back to the task so the // rest of the pipeline only deals with ids. func (c *TaskController) resolveTaskTemplate(projectID int, task *db.Task) (tpl db.Template, err error) { - if task.TemplateID == 0 && task.TemplateName == "" { + // The name is resolved to an id here, so it is cleared to keep it out of the + // stored task and the response. + name := task.TemplateName + task.TemplateName = "" + + if task.TemplateID == 0 && name == "" { err = common_errors.NewValidationError("template_id or template_name is required") return } @@ -51,7 +56,7 @@ func (c *TaskController) resolveTaskTemplate(projectID int, task *db.Task) (tpl return } - tpl, err = c.store.GetTemplateByName(projectID, task.TemplateName) + tpl, err = c.store.GetTemplateByName(projectID, name) if err != nil { return } diff --git a/api/projects/tasks_test.go b/api/projects/tasks_test.go index 434eef8125..4d2ed553e8 100644 --- a/api/projects/tasks_test.go +++ b/api/projects/tasks_test.go @@ -117,6 +117,7 @@ func TestResolveTaskTemplate(t *testing.T) { require.NoError(t, err) assert.Equal(t, build.ID, tpl.ID) assert.Equal(t, build.ID, task.TemplateID, "the resolved id must be written back to the task") + assert.Empty(t, task.TemplateName, "the name must not survive into the stored task or the response") }) t.Run("id wins when both are given", func(t *testing.T) { @@ -126,6 +127,7 @@ func TestResolveTaskTemplate(t *testing.T) { require.NoError(t, err) assert.Equal(t, build.ID, tpl.ID) + assert.Empty(t, task.TemplateName) }) t.Run("neither id nor name is rejected", func(t *testing.T) { From af9c5fa9329313df033dcbf1bb16b30dfca8fbb4 Mon Sep 17 00:00:00 2001 From: Befikadu Date: Wed, 12 Aug 2026 10:35:46 +0300 Subject: [PATCH 5/7] feat(api): enforce unique template names within projects and handle duplicates during migration --- api/projects/tasks_test.go | 15 ++++++- db/Migration.go | 1 + db/sql/migration.go | 2 + db/sql/migration_2_20_2.go | 77 +++++++++++++++++++++++++++++++++ db/sql/migration_2_20_2_test.go | 77 +++++++++++++++++++++++++++++++++ db/sql/migrations/v2.20.2.sql | 1 + db/sql/template.go | 36 +++++++++++++++ db/sql/template_test.go | 62 ++++++++++++++++++++++++++ 8 files changed, 269 insertions(+), 2 deletions(-) create mode 100644 db/sql/migration_2_20_2.go create mode 100644 db/sql/migration_2_20_2_test.go create mode 100644 db/sql/migrations/v2.20.2.sql diff --git a/api/projects/tasks_test.go b/api/projects/tasks_test.go index 4d2ed553e8..61e195d4ff 100644 --- a/api/projects/tasks_test.go +++ b/api/projects/tasks_test.go @@ -156,12 +156,23 @@ func TestResolveTaskTemplate(t *testing.T) { }) t.Run("an ambiguous name is rejected", func(t *testing.T) { + // Both the store and the unique index reject a duplicate name, so the + // collision has to be made behind their backs. This is a database which + // lost the index, for example one restored from a schema-less dump: the + // task must still refuse to guess rather than run the wrong template. createTaskTestTemplate(t, store, project.ID, repo.ID, "Duplicate") - createTaskTestTemplate(t, store, project.ID, repo.ID, "Duplicate") + legacy := createTaskTestTemplate(t, store, project.ID, repo.ID, "Duplicate (2)") + + _, err = store.Sql().Exec("drop index project__template__project_id_name") + require.NoError(t, err) + + _, err = store.Sql().Exec( + "update project__template set name=? where id=?", "Duplicate", legacy.ID) + require.NoError(t, err) task := db.Task{TemplateName: "Duplicate"} - _, err := c.resolveTaskTemplate(project.ID, &task) + _, err = c.resolveTaskTemplate(project.ID, &task) require.Error(t, err) assert.Contains(t, err.Error(), "more than one template") diff --git a/db/Migration.go b/db/Migration.go index 7456ec1c82..4bfb2e2e95 100644 --- a/db/Migration.go +++ b/db/Migration.go @@ -134,6 +134,7 @@ func GetMigrations(dialect string) []Migration { {Version: "2.19.12"}, {Version: "2.20.0"}, {Version: "2.20.1"}, + {Version: "2.20.2"}, } return append(initScripts, commonScripts...) diff --git a/db/sql/migration.go b/db/sql/migration.go index 0f5e86ee0f..f1ab2685bf 100644 --- a/db/sql/migration.go +++ b/db/sql/migration.go @@ -225,6 +225,8 @@ func (d *SqlDb) ApplyMigration(migration db.Migration) error { err = migration_2_16_8{db: d}.PreApply(tx) case "2.18.4": err = migration_2_18_4{db: d}.PreApply(tx) + case "2.20.2": + err = migration_2_20_2{db: d}.PreApply(tx) } if err != nil { diff --git a/db/sql/migration_2_20_2.go b/db/sql/migration_2_20_2.go new file mode 100644 index 0000000000..7cb240e396 --- /dev/null +++ b/db/sql/migration_2_20_2.go @@ -0,0 +1,77 @@ +package sql + +import ( + "strconv" + + "github.com/go-gorp/gorp/v3" +) + +type migration_2_20_2 struct { + db *SqlDb +} + +// PreApply renames templates which share a name inside a project, so that +// v2.20.2.sql can put a unique index on (project_id, name). Template names were +// never unique before, so any installation may hold duplicates and the index +// would otherwise fail the upgrade. +// +// The rename is done here rather than in SQL because a generated name can clash +// with a name which is already taken ("Build" twice next to a real "Build (2)"), +// which needs a retry that portable SQL cannot express. +func (m migration_2_20_2) PreApply(tx *gorp.Transaction) error { + type templateName struct { + ID int `db:"id"` + ProjectID int `db:"project_id"` + Name string `db:"name"` + } + + var templates []templateName + + // Ordered by id so the oldest template of each name keeps it, and so the + // result does not depend on the order rows come back in. + _, err := tx.Select(&templates, + m.db.PrepareQuery("select `id`, `project_id`, `name` from `project__template` order by `id`")) + + if err != nil { + return err + } + + taken := make(map[string]bool, len(templates)) + key := func(projectID int, name string) string { + return strconv.Itoa(projectID) + "\x00" + name + } + + // Every name in use is reserved before anything is renamed, so that a + // generated name cannot take the name of a template which already has it: + // "Build" twice next to a real "Build (2)" must not turn the latter into + // "Build (2) (2)". + var duplicates []templateName + + for _, template := range templates { + if taken[key(template.ProjectID, template.Name)] { + duplicates = append(duplicates, template) + continue + } + + taken[key(template.ProjectID, template.Name)] = true + } + + for _, template := range duplicates { + name := template.Name + for i := 2; taken[key(template.ProjectID, name)]; i++ { + name = template.Name + " (" + strconv.Itoa(i) + ")" + } + + _, err = tx.Exec( + m.db.PrepareQuery("update `project__template` set `name`=? where `id`=?"), + name, template.ID) + + if err != nil { + return err + } + + taken[key(template.ProjectID, name)] = true + } + + return nil +} diff --git a/db/sql/migration_2_20_2_test.go b/db/sql/migration_2_20_2_test.go new file mode 100644 index 0000000000..aec8e6b47c --- /dev/null +++ b/db/sql/migration_2_20_2_test.go @@ -0,0 +1,77 @@ +package sql + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestMigration_2_20_2_EnforcesUniqueTemplateName checks the index exists after +// the migrations the test store runs, so that a name identifies one template. +func TestMigration_2_20_2_EnforcesUniqueTemplateName(t *testing.T) { + store := InitConfigCreateTestStore() + projectID, repositoryID := newTemplateTestProject(t, store) + + insert := func(name string) error { + _, err := store.exec( + "insert into project__template (project_id, repository_id, name, playbook, arguments, allow_override_args_in_task, app) values (?, ?, ?, 'site.yml', '[]', false, 'ansible')", + projectID, repositoryID, name) + return err + } + + require.NoError(t, insert("Build website")) + + // The store also rejects this, but here the insert goes straight to the + // database, so only the index can stop it. + assert.Error(t, insert("Build website")) + + assert.NoError(t, insert("Deploy website")) +} + +// TestMigration_2_20_2_RenamesExistingDuplicates covers PreApply: templates which +// already share a name must be renamed rather than failing the upgrade. +func TestMigration_2_20_2_RenamesExistingDuplicates(t *testing.T) { + store := InitConfigCreateTestStore() + projectID, repositoryID := newTemplateTestProject(t, store) + + // Recreate the pre-migration state: drop the index, then write the duplicates + // an old installation would be holding. + _, err := store.exec("drop index project__template__project_id_name") + require.NoError(t, err) + + names := []string{"Build", "Build", "Build", "Build (2)", "Deploy"} + for _, name := range names { + _, err = store.exec( + "insert into project__template (project_id, repository_id, name, playbook, arguments, allow_override_args_in_task, app) values (?, ?, ?, 'site.yml', '[]', false, 'ansible')", + projectID, repositoryID, name) + require.NoError(t, err) + } + + tx, err := store.Sql().Begin() + require.NoError(t, err) + + require.NoError(t, migration_2_20_2{db: store}.PreApply(tx)) + require.NoError(t, tx.Commit()) + + var renamed []struct { + ID int `db:"id"` + Name string `db:"name"` + } + _, err = store.Sql().Select(&renamed, + "select id, name from project__template order by id") + require.NoError(t, err) + + // The oldest template of each name keeps it; the rest get a suffix, skipping + // "Build (2)" because that name is already taken. + actual := make([]string, 0, len(renamed)) + for _, template := range renamed { + actual = append(actual, template.Name) + } + assert.Equal(t, []string{"Build", "Build (3)", "Build (4)", "Build (2)", "Deploy"}, actual) + + // The renames must leave the table indexable. + _, err = store.exec( + "create unique index project__template__project_id_name on project__template (project_id, name)") + assert.NoError(t, err) +} diff --git a/db/sql/migrations/v2.20.2.sql b/db/sql/migrations/v2.20.2.sql new file mode 100644 index 0000000000..ffc705c689 --- /dev/null +++ b/db/sql/migrations/v2.20.2.sql @@ -0,0 +1 @@ +create unique index `project__template__project_id_name` on `project__template` (`project_id`, `name`); diff --git a/db/sql/template.go b/db/sql/template.go index 9feeafbf8d..053ecc1b2c 100644 --- a/db/sql/template.go +++ b/db/sql/template.go @@ -10,6 +10,30 @@ import ( log "github.com/sirupsen/logrus" ) +// validateTemplateNameIsFree rejects a template name which is already used by +// another template of the same project, so that a template can be referred to by +// name. templateID is the template being updated, or 0 when creating one. +// +// Templates created before this check may still share a name, which is why +// GetTemplateByName rejects an ambiguous name rather than relying on this. +func (d *SqlDb) validateTemplateNameIsFree(projectID int, templateID int, name string) error { + var count int + + err := d.selectOne(&count, + "select count(*) from project__template where project_id=? and name=? and id<>?", + projectID, name, templateID) + + if err != nil { + return err + } + + if count > 0 { + return common_errors.NewValidationError("template with name " + name + " already exists") + } + + return nil +} + func (d *SqlDb) CreateTemplate(template db.Template) (newTemplate db.Template, err error) { err = template.Validate() @@ -17,6 +41,12 @@ func (d *SqlDb) CreateTemplate(template db.Template) (newTemplate db.Template, e return } + err = d.validateTemplateNameIsFree(template.ProjectID, 0, template.Name) + + if err != nil { + return + } + template.ApplyLegacyEnvironmentField() insertID, err := d.insert( @@ -95,6 +125,12 @@ func (d *SqlDb) UpdateTemplate(template db.Template) error { return err } + err = d.validateTemplateNameIsFree(template.ProjectID, template.ID, template.Name) + + if err != nil { + return err + } + _, err = d.exec("update project__template set "+ "inventory_id=?, "+ "repository_id=?, "+ diff --git a/db/sql/template_test.go b/db/sql/template_test.go index db12e07c83..3f3032a3d4 100644 --- a/db/sql/template_test.go +++ b/db/sql/template_test.go @@ -97,3 +97,65 @@ func TestTemplateWithoutExecutorImage(t *testing.T) { require.NoError(t, err) assert.Nil(t, loaded.ExecutorImage) } + +// TestTemplateNameUniqueness covers the check which lets a template be referred +// to by name: a name may be reused across projects, but not inside one. +func TestTemplateNameUniqueness(t *testing.T) { + store := InitConfigCreateTestStore() + projectID, repositoryID := newTemplateTestProject(t, store) + + newTemplate := func(name string) db.Template { + return db.Template{ + ProjectID: projectID, + RepositoryID: repositoryID, + Name: name, + Playbook: "site.yml", + } + } + + build, err := store.CreateTemplate(newTemplate("Build website")) + require.NoError(t, err) + + t.Run("a duplicate name is rejected", func(t *testing.T) { + _, err := store.CreateTemplate(newTemplate("Build website")) + + require.Error(t, err) + assert.ErrorContains(t, err, "already exists") + }) + + t.Run("a free name is accepted", func(t *testing.T) { + _, err := store.CreateTemplate(newTemplate("Deploy website")) + + assert.NoError(t, err) + }) + + t.Run("the same name in another project is accepted", func(t *testing.T) { + otherProjectID, otherRepositoryID := newTemplateTestProject(t, store) + + _, err := store.CreateTemplate(db.Template{ + ProjectID: otherProjectID, + RepositoryID: otherRepositoryID, + Name: "Build website", + Playbook: "site.yml", + }) + + assert.NoError(t, err) + }) + + t.Run("a template keeps its own name on update", func(t *testing.T) { + description := "edited" + build.Description = &description + + assert.NoError(t, store.UpdateTemplate(build)) + }) + + t.Run("renaming onto another template is rejected", func(t *testing.T) { + renamed := build + renamed.Name = "Deploy website" + + err := store.UpdateTemplate(renamed) + + require.Error(t, err) + assert.ErrorContains(t, err, "already exists") + }) +} From 9e4d55e80e547e6caef49bc6d7da9d959703347f Mon Sep 17 00:00:00 2001 From: Befikadu Date: Wed, 12 Aug 2026 12:23:10 +0300 Subject: [PATCH 6/7] feat(api): handle unique template names during project task creation via API --- .dredd/hooks/capabilities.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.dredd/hooks/capabilities.go b/.dredd/hooks/capabilities.go index 869dda4653..224916f1d4 100644 --- a/.dredd/hooks/capabilities.go +++ b/.dredd/hooks/capabilities.go @@ -294,6 +294,10 @@ func alterRequestBody(t *trans.Transaction) { if invite != nil { bodyFieldProcessor("invite_id", 4, &request) } + if t.Request.Method == "PUT" && strings.Contains(t.Request.URI, "/templates/") { + bodyFieldProcessor("name", "Test-"+getUUID(), &request) + } + bodyFieldProcessor("environment_id", environmentID, &request) bodyFieldProcessor("environment_ids", []int{environmentID}, &request) bodyFieldProcessor("inventory_id", inventoryID, &request) From 339f15752e1bd6c10201face74c87199b84dc0af Mon Sep 17 00:00:00 2001 From: Befikadu Date: Wed, 12 Aug 2026 14:49:35 +0300 Subject: [PATCH 7/7] feat(api): improve template name handling during migration to ensure uniqueness --- db/sql/migration_2_20_2.go | 74 +++++++++++++++++++++----------------- 1 file changed, 41 insertions(+), 33 deletions(-) diff --git a/db/sql/migration_2_20_2.go b/db/sql/migration_2_20_2.go index 7cb240e396..d218b5576f 100644 --- a/db/sql/migration_2_20_2.go +++ b/db/sql/migration_2_20_2.go @@ -15,9 +15,14 @@ type migration_2_20_2 struct { // never unique before, so any installation may hold duplicates and the index // would otherwise fail the upgrade. // -// The rename is done here rather than in SQL because a generated name can clash -// with a name which is already taken ("Build" twice next to a real "Build (2)"), -// which needs a retry that portable SQL cannot express. +// Which names collide is decided by the database rather than by comparing them +// here: the unique index uses the collation of the column, and on the default +// MySQL and MariaDB collations "Build", "build" and "Build " are all the same +// key while Go sees three different strings. Comparing in Go would leave those +// rows in place and the index would still fail. +// +// The rename is a loop rather than a single statement because a generated name +// can itself be taken ("Build" twice next to a real "Build (2)"). func (m migration_2_20_2) PreApply(tx *gorp.Transaction) error { type templateName struct { ID int `db:"id"` @@ -25,41 +30,25 @@ func (m migration_2_20_2) PreApply(tx *gorp.Transaction) error { Name string `db:"name"` } - var templates []templateName + var duplicates []templateName - // Ordered by id so the oldest template of each name keeps it, and so the - // result does not depend on the order rows come back in. - _, err := tx.Select(&templates, - m.db.PrepareQuery("select `id`, `project_id`, `name` from `project__template` order by `id`")) + // Every template which an older one of the project already shadows. Ordered + // by id so the oldest keeps its name and the result does not depend on the + // order rows come back in. + _, err := tx.Select(&duplicates, m.db.PrepareQuery( + "select `id`, `project_id`, `name` from `project__template` t "+ + "where exists (select 1 from (select `id`, `project_id`, `name` from `project__template`) o "+ + "where o.`project_id` = t.`project_id` and o.`name` = t.`name` and o.`id` < t.`id`) "+ + "order by t.`id`")) if err != nil { return err } - taken := make(map[string]bool, len(templates)) - key := func(projectID int, name string) string { - return strconv.Itoa(projectID) + "\x00" + name - } - - // Every name in use is reserved before anything is renamed, so that a - // generated name cannot take the name of a template which already has it: - // "Build" twice next to a real "Build (2)" must not turn the latter into - // "Build (2) (2)". - var duplicates []templateName - - for _, template := range templates { - if taken[key(template.ProjectID, template.Name)] { - duplicates = append(duplicates, template) - continue - } - - taken[key(template.ProjectID, template.Name)] = true - } - for _, template := range duplicates { - name := template.Name - for i := 2; taken[key(template.ProjectID, name)]; i++ { - name = template.Name + " (" + strconv.Itoa(i) + ")" + name, err := m.freeTemplateName(tx, template.ProjectID, template.Name) + if err != nil { + return err } _, err = tx.Exec( @@ -69,9 +58,28 @@ func (m migration_2_20_2) PreApply(tx *gorp.Transaction) error { if err != nil { return err } - - taken[key(template.ProjectID, name)] = true } return nil } + +// freeTemplateName returns a name based on base which no template of the project +// uses. Availability is asked of the database so that the answer follows the +// same collation as the unique index. +func (m migration_2_20_2) freeTemplateName(tx *gorp.Transaction, projectID int, base string) (string, error) { + for i := 2; ; i++ { + name := base + " (" + strconv.Itoa(i) + ")" + + count, err := tx.SelectInt(m.db.PrepareQuery( + "select count(*) from `project__template` where `project_id`=? and `name`=?"), + projectID, name) + + if err != nil { + return "", err + } + + if count == 0 { + return name, nil + } + } +}