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

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

api-docs.yml

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

@@ -137,6 +137,7 @@ func GetMigrations(dialect string) []Migration {
 		{Version: "2.20.1"},
 		{Version: "2.20.2"},
 		{Version: "2.20.3"},
+		{Version: "2.20.4"},
 	}
 
 	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

@@ -244,6 +244,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.4":
+		err = migration_2_20_4{db: d}.PreApply(tx)
 	}
 
 	if err != nil {

db/sql/migration_2_20_4.go

@@ -0,0 +1,85 @@
+package sql
+
+import (
+	"strconv"
+
+	"github.com/go-gorp/gorp/v3"
+)
+
+type migration_2_20_4 struct {
+	db *SqlDb
+}
+
+// PreApply renames templates which share a name inside a project, so that
+// v2.20.4.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.
+//
+// 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_4) PreApply(tx *gorp.Transaction) error {
+	type templateName struct {
+		ID        int    `db:"id"`
+		ProjectID int    `db:"project_id"`
+		Name      string `db:"name"`
+	}
+
+	var duplicates []templateName
+
+	// 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
+	}
+
+	for _, template := range duplicates {
+		name, err := m.freeTemplateName(tx, template.ProjectID, template.Name)
+		if err != nil {
+			return err
+		}
+
+		_, err = tx.Exec(
+			m.db.PrepareQuery("update `project__template` set `name`=? where `id`=?"),
+			name, template.ID)
+
+		if err != nil {
+			return err
+		}
+	}
+
+	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_4) 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
+		}
+	}
+}