feat(api): allow template_name when creating project tasks via API

api-docs.yml

@@ -3419,11 +3419,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:

api/projects/tasks.go

@@ -37,12 +37,40 @@ 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) {
+	// 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
+	}
+
+	if task.TemplateID != 0 {
+		tpl, err = c.store.GetTemplate(projectID, task.TemplateID)
+		return
+	}
+
+	tpl, err = c.store.GetTemplateByName(projectID, name)
+	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

api/projects/tasks_test.go

@@ -5,7 +5,9 @@ import (
 	"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) {
@@ -52,3 +54,127 @@ func TestParseTasksPageParams_PreservesBase(t *testing.T) {
 	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 {
+	t.Helper()
+
+	tpl, err := store.CreateTemplate(db.Template{
+		Name:         name,
+		Playbook:     "test.yml",
+		ProjectID:    projectID,
+		RepositoryID: repositoryID,
+	})
+	require.NoError(t, err)
+
+	return tpl
+}
+
+func TestResolveTaskTemplate(t *testing.T) {
+	store := sql.InitConfigCreateTestStore()
+
+	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")
+		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) {
+		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)
+		assert.Empty(t, task.TemplateName)
+	})
+
+	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) {
+		// 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")
+		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)
+
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "more than one template")
+	})
+}

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...)

db/Store.go

@@ -268,6 +268,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)

db/Task.go

@@ -41,9 +41,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

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 {

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
+}

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)
+}

db/sql/migrations/v2.20.2.sql

@@ -0,0 +1 @@
+create unique index `project__template__project_id_name` on `project__template` (`project_id`, `name`);