feat(api): allow template_name when creating project tasks via API - #4128
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTask creation now accepts ChangesTask template resolution and uniqueness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AddTask
participant resolveTaskTemplate
participant TemplateManager
participant SqlDb
AddTask->>resolveTaskTemplate: Resolve template_id or template_name
resolveTaskTemplate->>TemplateManager: Request project-scoped template
TemplateManager->>SqlDb: Get template by ID or name
SqlDb-->>TemplateManager: Return template or error
TemplateManager-->>resolveTaskTemplate: Return resolved template
resolveTaskTemplate-->>AddTask: Set TemplateID and continue task creation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…mplate_name-when-creating-project-tasks-via-api
There was a problem hiding this comment.
Pull request overview
This PR extends the Semaphore API task-creation flow to allow referencing a template by template_name in addition to template_id, with template_id taking precedence when both are provided. It adds database lookup support for resolving templates by name, updates API documentation accordingly, and introduces tests covering the new resolution behavior and edge cases.
Changes:
- Add
template_namesupport for task creation, resolving totemplate_idbefore the task enters the existing execution pipeline. - Add
GetTemplateByName(projectID, name)to the store interface with SQL implementation that rejects ambiguous names. - Update Swagger docs (both locations) and add API tests validating precedence and failure cases.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/public/swagger/api-docs.yml | Documents template_name support and precedence rules for task creation (web copy). |
| api-docs.yml | Documents template_name support and precedence rules for task creation (root copy). |
| db/Task.go | Adds TemplateName as a non-persisted field on db.Task and relaxes TemplateID struct-tag requirement. |
| db/Store.go | Extends TemplateManager with GetTemplateByName. |
| db/sql/template.go | Implements name-based template lookup with ambiguity detection. |
| api/projects/tasks.go | Adds template resolution helper used by the task-create API handler. |
| api/projects/tasks_test.go | Adds unit tests for template resolution by id/name, precedence, and error cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@befika I think we also need to add new template name uniqueness validation. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
api/projects/tasks.go:60
- A successful resolution leaves the request-only
TemplateNameon the task.TaskPool.AddTaskpasses that value throughCreateTask, which copies the struct, so the 201 response echoestemplate_name; when both fields were supplied it can even echo the ignored or invalid name. Clear it after resolving so task responses remain canonical and match subsequent reads.
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
api/projects/tasks.go:42
- The existing
AddTasksentence is now attached toresolveTaskTemplate's Go documentation, incorrectly claiming that this helper inserts a task and returns a header. Keep the resolver description here and move theAddTaskdescription back aboveAddTask.
// 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.
…-via-api' of github.com:befika/semaphore into sem-207-allow-template_name-when-creating-project-tasks-via-api
…uplicates during migration
Template names are now unique per project. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@db/sql/migration_2_20_2.go`:
- Around line 39-73: The duplicate tracking in migration 2.20.2 uses byte-exact
Go keys instead of the database’s case- and space-insensitive collation rules.
Update the reservation and generated-suffix checks in the duplicate-renaming
logic of migration_2_20_2.go to compare names using equivalent MySQL/MariaDB
collation semantics, then add a MySQL/MariaDB migration test in
migration_2_20_2_test.go covering collisions such as “Build”/“build” and
“Build”/“Build ” and verifying the generated names avoid unique-index conflicts.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d2fd0b8b-ede1-449b-8491-4d418466d145
📒 Files selected for processing (8)
api/projects/tasks_test.godb/Migration.godb/sql/migration.godb/sql/migration_2_20_2.godb/sql/migration_2_20_2_test.godb/sql/migrations/v2.20.2.sqldb/sql/template.godb/sql/template_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/projects/tasks_test.go
| 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 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- migration ---'
sed -n '1,130p' db/sql/migration_2_20_2.go
printf '%s\n' '--- migration test ---'
sed -n '1,130p' db/sql/migration_2_20_2_test.go
printf '%s\n' '--- related schema/index definitions ---'
rg -n -C 3 'project__template__project_id_name|project__template|collat|utf8|case.?insens|trim' db --glob '*.go' --glob '*.sql' --glob '*.yaml' --glob '*.yml'
printf '%s\n' '--- database drivers and test setup ---'
rg -n -C 3 'sqlite|postgres|mysql|mariadb|CREATE TABLE|create unique index|PrepareQuery|InitConfigCreateTestStore' db --glob '*.go' | head -n 500Repository: semaphoreui/semaphore
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- template column definitions ---'
sed -n '65,90p' db/sql/migrations/v0.0.0.sql
sed -n '175,205p' db/sql/migrations/v2.15.1.sqlite.sql
sed -n '1,20p' db/sql/migrations/v2.20.2.sql
printf '%s\n' '--- database configuration and dialect selection ---'
rg -n -C 4 'DB_(DIALECT|HOST|NAME|USER|PASSWORD)|dialect|SqliteDialect|MySQLDialect|PostgresDialect|mysql|postgres|sqlite' db config .github docker-compose.yml docker-compose.yaml 2>/dev/null | head -n 400
printf '%s\n' '--- migration runner order ---'
rg -n -C 5 'PreApply|migrations|v2\.20\.2|migration_2_20_2' db --glob '*.go' | head -n 300
printf '%s\n' '--- collation-related repository configuration ---'
rg -n -i -C 3 'collation|character set|charset|utf8mb4|utf8_general|case.insensitive|mysql' . --glob '!vendor/**' --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' | head -n 400Repository: semaphoreui/semaphore
Length of output: 50379
🌐 Web query:
MySQL 8.4 documentation nonbinary string comparisons trailing spaces case insensitive default collation utf8mb4_0900_ai_ci unique index
💡 Result:
In MySQL 8.4, the behavior of trailing space handling and case sensitivity for nonbinary strings is determined by the collation assigned to the column or the connection [1][2]. Trailing Spaces: The utf8mb4_0900_ai_ci collation has a PAD_ATTRIBUTE of NO PAD [3][4]. This means that trailing spaces are treated as significant characters in comparisons [1][5]. Consequently, two strings that differ only by the number of trailing spaces (e.g., 'a' and 'a ') are considered unequal [5][4]. This differs from older "PAD SPACE" collations (like utf8mb4_general_ci), where trailing spaces are ignored during comparison [1][3]. Unique Indexes: Because unique indexes rely on the collation's comparison rules, a unique index on a column using the utf8mb4_0900_ai_ci collation will treat strings with different trailing spaces as distinct values [3]. Therefore, you can insert both 'a' and 'a ' into a column with a unique index, and they will not trigger a duplicate entry error [3]. This behavior contrasts with "PAD SPACE" collations, where such an attempt would result in a duplicate key error [3][6]. Case Sensitivity: The utf8mb4_0900_ai_ci collation is case-insensitive (indicated by the _ai_ci suffix) [2]. As the default collation in MySQL 8.4, it ensures that nonbinary string comparisons—including those used by unique indexes—ignore character case [2]. Summary: - Trailing Spaces: Significant (NO PAD) [5][3]. - Case Sensitivity: Insensitive (_ai_ci) [2]. - Unique Index Effect: Allows values differing only by trailing spaces because they are compared as distinct [3].
Citations:
- 1: https://dev.mysql.com/doc/refman/8.4/en/charset-binary-collations.html
- 2: https://dev.mysql.com/doc/refman/8.4/en/case-sensitivity.html
- 3: https://saveriomiroddi.github.io/Summary-of-trailing-spaces-handling-in-MySQL-with-version-8.0-upgrade-considerations/
- 4: https://dba.stackexchange.com/questions/312585/mysql-8-trailing-spaces-being-evaluated-in-equals-comparison
- 5: https://dev.mysql.com/doc/refman/8.4/en/charset-unicode-sets.html
- 6: https://stackoverflow.com/questions/11714534/mysql-database-with-unique-fields-ignored-ending-spaces
🌐 Web query:
MariaDB 10.11 documentation default collation case insensitive trailing spaces VARCHAR unique index
💡 Result:
In MariaDB 10.11, standard VARCHAR columns use PADSPACE collations by default [1]. Under these collations, trailing spaces are ignored during comparisons (including UNIQUE index enforcement), even though the trailing spaces themselves are stored [2][3]. Key technical implications: 1. UNIQUE Index Behavior: Because trailing spaces are ignored during comparisons, a UNIQUE index will treat strings that differ only by the number of trailing spaces as duplicates [2][4][5]. For example, attempting to insert both 'value' and 'value ' into a column with a UNIQUE index will result in a "Duplicate entry" error [6][5][7]. 2. Case Insensitivity: Most standard MariaDB collations (e.g., utf8mb4_general_ci, latin1_swedish_ci) are case-insensitive [1][8]. When using these collations, a UNIQUE index will also treat strings that differ only by case (e.g., 'Maria' and 'maria') as duplicates. 3. Avoiding This Behavior: To ensure that trailing spaces are treated as significant in comparisons and index enforcement, you must use a NO PAD collation (e.g., utf8mb4_nopad_bin or other collations specifically designated with "nopad") [2][1]. Similarly, to make a column case-sensitive, you should use a binary (_bin) or case-sensitive collation [8]. While MariaDB does not remove trailing spaces from VARCHAR values upon storage [2][9], the reliance on PADSPACE collations means they are effectively ignored for equality checks, sorting, and constraint enforcement unless a specific NO PAD collation is applied [1].
Citations:
- 1: https://jira.mariadb.org/browse/MDEV-9711?stepId=6&workflowName=MariaDB+v4
- 2: https://mariadb.com/docs/server/reference/data-types/string-data-types/varchar
- 3: https://mariadb.com/kb/en/varchar/
- 4: https://mariadb.com/docs/server/mariadb-quickstart-guides/mariadb-indexes-guide
- 5: https://stackoverflow.com/questions/44535380/mariadb-unique-constraint-error-duplicate-entry-for-two-records-with-one-spac
- 6: https://jira.mariadb.org/browse/MDEV-33812
- 7: https://jira.mariadb.org/browse/MDEV-33812?stepId=6&workflowName=MariaDB+v4
- 8: https://mariadb.com/docs/server/reference/data-types/string-data-types/character-sets/character-set-and-collation-overview
- 9: https://github.com/mariadb-corporation/mariadb-docs/blob/main/server/reference/data-types/string-data-types/varchar.md
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
def raw_key(project_id, name):
return f"{project_id}\x00{name}"
def migration_reservation(names):
taken = set()
duplicates = []
for name in names:
key = raw_key(1, name)
if key in taken:
duplicates.append(name)
else:
taken.add(key)
renamed = list(names)
for pos, original in enumerate(names):
if original not in duplicates:
continue
name = original
i = 2
while raw_key(1, name) in taken:
name = f"{original} ({i})"
i += 1
renamed[pos] = name
taken.add(raw_key(1, name))
return renamed
def mariadb_default_equivalence(name):
return name.rstrip().casefold()
def mysql_84_default_equivalence(name):
return name.casefold()
cases = {
"case-only duplicate": ["Build", "build"],
"MariaDB PAD SPACE duplicate": ["Build", "Build "],
"case-equivalent generated suffix": ["Build", "Build", "build (2)"],
}
for label, names in cases.items():
result = migration_reservation(names)
print(label)
print(" input: ", names)
print(" result:", result)
print(" MariaDB keys:", [mariadb_default_equivalence(n) for n in result])
print(" MySQL 8.4 keys:", [mysql_84_default_equivalence(n) for n in result])
assert migration_reservation(cases["case-only duplicate"]) == ["Build", "build"]
assert len({mariadb_default_equivalence(n) for n in migration_reservation(cases["case-only duplicate"])}) < 2
assert len({mariadb_default_equivalence(n) for n in migration_reservation(cases["MariaDB PAD SPACE duplicate"])}) < 2
assert migration_reservation(cases["case-equivalent generated suffix"]) == [
"Build", "Build (3)", "build (2)"
]
assert len({
mariadb_default_equivalence(n)
for n in migration_reservation(cases["case-equivalent generated suffix"])
}) < 3
print("behavioral checks: collision remains under the database equivalence rules")
PYRepository: semaphoreui/semaphore
Length of output: 798
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
def raw_key(project_id, name):
return f"{project_id}\x00{name}"
def migration_reservation(names):
taken = set()
duplicates = []
for name in names:
key = raw_key(1, name)
if key in taken:
duplicates.append(name)
else:
taken.add(key)
renamed = list(names)
duplicate_positions = []
for pos, original in enumerate(names):
if raw_key(1, original) in taken and original in duplicates:
duplicate_positions.append(pos)
# Re-run the same reservation/rename flow with row positions preserved.
taken = set()
duplicates = []
for name in names:
if raw_key(1, name) in taken:
duplicates.append(name)
else:
taken.add(raw_key(1, name))
for pos, original in enumerate(names):
if original not in duplicates:
continue
name = original
for i in range(2, 100):
if raw_key(1, name) not in taken:
break
name = f"{original} ({i})"
renamed[pos] = name
taken.add(raw_key(1, name))
return renamed
def mariadb_equivalence(name):
return name.rstrip().casefold()
def mysql_84_equivalence(name):
return name.casefold()
def has_collision(names, equivalence):
keys = [equivalence(name) for name in names]
return len(keys) != len(set(keys))
cases = {
"case-only names": ["Build", "build"],
"MariaDB PAD SPACE names": ["Build", "Build "],
"case-equivalent generated suffix": ["Build", "Build", "build (2)"],
}
for label, names in cases.items():
result = migration_reservation(names)
print(label)
print(" result:", result)
print(" MariaDB collision:", has_collision(result, mariadb_equivalence))
print(" MySQL 8.4 collision:", has_collision(result, mysql_84_equivalence))
assert migration_reservation(cases["case-only names"]) == ["Build", "build"]
assert has_collision(
migration_reservation(cases["case-only names"]), mariadb_equivalence
)
assert migration_reservation(cases["MariaDB PAD SPACE names"]) == ["Build", "Build "]
assert has_collision(
migration_reservation(cases["MariaDB PAD SPACE names"]), mariadb_equivalence
)
assert has_collision(
migration_reservation(cases["case-equivalent generated suffix"]),
mariadb_equivalence,
)
assert has_collision(
migration_reservation(cases["case-equivalent generated suffix"]),
mysql_84_equivalence,
)
print("behavioral checks: raw Go keys leave database-equivalent collisions")
PYRepository: semaphoreui/semaphore
Length of output: 572
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
def raw_key(project_id, name):
return f"{project_id}\x00{name}"
def migration_reservation(names):
taken = set()
duplicates = []
for name in names:
if raw_key(1, name) in taken:
duplicates.append(name)
else:
taken.add(raw_key(1, name))
renamed = list(names)
for pos, original in enumerate(names):
if original not in duplicates:
continue
name = original
for i in range(2, 100):
if raw_key(1, name) not in taken:
break
name = f"{original} ({i})"
renamed[pos] = name
taken.add(raw_key(1, name))
return renamed
def mariadb_equivalence(name):
return name.rstrip().casefold()
def mysql_84_equivalence(name):
return name.casefold()
def has_collision(names, equivalence):
keys = [equivalence(name) for name in names]
return len(keys) != len(set(keys))
cases = {
"case-only names": ["Build", "build"],
"MariaDB PAD SPACE names": ["Build", "Build "],
"case-equivalent generated suffix": ["Build", "Build", "build (2)"],
}
for label, names in cases.items():
result = migration_reservation(names)
print(label)
print(" result:", result)
print(" MariaDB collision:", has_collision(result, mariadb_equivalence))
print(" MySQL 8.4 collision:", has_collision(result, mysql_84_equivalence))
assert migration_reservation(cases["case-only names"]) == ["Build", "build"]
assert has_collision(
migration_reservation(cases["case-only names"]), mariadb_equivalence
)
assert migration_reservation(cases["MariaDB PAD SPACE names"]) == ["Build", "Build "]
assert has_collision(
migration_reservation(cases["MariaDB PAD SPACE names"]), mariadb_equivalence
)
assert has_collision(
migration_reservation(cases["case-equivalent generated suffix"]),
mariadb_equivalence,
)
assert has_collision(
migration_reservation(cases["case-equivalent generated suffix"]),
mysql_84_equivalence,
)
print("behavioral checks: raw Go keys leave database-equivalent collisions")
PYRepository: semaphoreui/semaphore
Length of output: 572
Use database collation rules during duplicate detection.
PreApply uses byte-exact Go keys. On supported MySQL/MariaDB collations, names such as "Build" and "build" or "Build" and "Build " can map to the same unique-index key. The migration can leave these collisions unchanged, so v2.20.2.sql fails and blocks the upgrade. Apply equivalent comparison rules to initial reservations and generated suffixes. Add a MySQL/MariaDB migration test.
📍 Affects 2 files
db/sql/migration_2_20_2.go#L39-L73(this comment)db/sql/migration_2_20_2_test.go#L35-L76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@db/sql/migration_2_20_2.go` around lines 39 - 73, The duplicate tracking in
migration 2.20.2 uses byte-exact Go keys instead of the database’s case- and
space-insensitive collation rules. Update the reservation and generated-suffix
checks in the duplicate-renaming logic of migration_2_20_2.go to compare names
using equivalent MySQL/MariaDB collation semantics, then add a MySQL/MariaDB
migration test in migration_2_20_2_test.go covering collisions such as
“Build”/“build” and “Build”/“Build ” and verifying the generated names avoid
unique-index conflicts.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.dredd/hooks/capabilities.go (1)
297-299: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPreserve an opt-out for name-sensitive Dredd transactions.
This rewrites every existing
namefield for matching template PUT requests. The API contract exposesnamein the template PUT body, so Dredd cannot send the documented value unchanged. (raw.githubusercontent.com)Restrict this rewrite to fixture setup transactions, or add an explicit opt-out. Keep a negative case that sends a duplicate name unchanged so the new uniqueness validation remains covered.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.dredd/hooks/capabilities.go around lines 297 - 299, The template PUT handling in the Dredd hook currently rewrites every name field, preventing name-sensitive transactions from sending documented values unchanged. Update the logic around bodyFieldProcessor to apply the rewrite only to fixture-setup transactions or honor an explicit opt-out, while preserving a negative duplicate-name transaction that bypasses rewriting and exercises uniqueness validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.dredd/hooks/capabilities.go:
- Around line 297-299: The template PUT handling in the Dredd hook currently
rewrites every name field, preventing name-sensitive transactions from sending
documented values unchanged. Update the logic around bodyFieldProcessor to apply
the rewrite only to fixture-setup transactions or honor an explicit opt-out,
while preserving a negative duplicate-name transaction that bypasses rewriting
and exercises uniqueness validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 44d010ea-0d1f-4157-b809-5e4f7abda4b4
📒 Files selected for processing (1)
.dredd/hooks/capabilities.go
Summary by CodeRabbit
New Features
Migration
Documentation