diff --git a/apiserver/internal/migrations/008_unique_label_names.go b/apiserver/internal/migrations/008_unique_label_names.go new file mode 100644 index 00000000..65467776 --- /dev/null +++ b/apiserver/internal/migrations/008_unique_label_names.go @@ -0,0 +1,108 @@ +package migrations + +import ( + "context" + "fmt" + + "gorm.io/gorm" +) + +func init() { + Register(&UniqueLabelNamesMigration{}) +} + +type UniqueLabelNamesMigration struct{} + +func (m *UniqueLabelNamesMigration) Version() int { + return 8 +} + +func (m *UniqueLabelNamesMigration) Name() string { + return "unique_label_names" +} + +func (m *UniqueLabelNamesMigration) Up(ctx context.Context, db *gorm.DB) error { + dbCtx := db.WithContext(ctx) + + dedup := []string{ + // Reassign task_labels from duplicate labels to the kept (lowest ID) label, + // skipping rows that would collide with an existing (task_id, keep_id) pair + `INSERT OR IGNORE INTO task_labels (task_id, label_id) + SELECT tl.task_id, keep.id + FROM task_labels tl + JOIN labels dup ON tl.label_id = dup.id + JOIN ( + SELECT MIN(id) AS id, name, created_by + FROM labels + GROUP BY created_by, name + ) keep ON dup.created_by = keep.created_by AND dup.name = keep.name + WHERE dup.id != keep.id`, + + // Remove task_labels pointing to duplicate labels + `DELETE FROM task_labels WHERE label_id IN ( + SELECT l.id FROM labels l + JOIN ( + SELECT MIN(id) AS keep_id, name, created_by + FROM labels + GROUP BY created_by, name + ) k ON l.created_by = k.created_by AND l.name = k.name + WHERE l.id != k.keep_id + )`, + + // Delete the duplicate labels themselves + `DELETE FROM labels WHERE id NOT IN ( + SELECT MIN(id) FROM labels GROUP BY created_by, name + )`, + } + + dialect := db.Dialector.Name() + if dialect == "mysql" { + dedup = []string{ + `INSERT IGNORE INTO task_labels (task_id, label_id) + SELECT tl.task_id, keep_tbl.keep_id + FROM task_labels tl + JOIN labels dup ON tl.label_id = dup.id + JOIN ( + SELECT MIN(id) AS keep_id, name, created_by + FROM labels + GROUP BY created_by, name + ) keep_tbl ON dup.created_by = keep_tbl.created_by AND dup.name = keep_tbl.name + WHERE dup.id != keep_tbl.keep_id`, + + `DELETE tl FROM task_labels tl + JOIN labels l ON tl.label_id = l.id + JOIN ( + SELECT MIN(id) AS keep_id, name, created_by + FROM labels + GROUP BY created_by, name + ) k ON l.created_by = k.created_by AND l.name = k.name + WHERE l.id != k.keep_id`, + + `DELETE l FROM labels l + JOIN ( + SELECT MIN(id) AS keep_id, name, created_by + FROM labels + GROUP BY created_by, name + ) k ON l.created_by = k.created_by AND l.name = k.name + WHERE l.id != k.keep_id`, + } + } + + for _, stmt := range dedup { + if err := dbCtx.Exec(stmt).Error; err != nil { + return fmt.Errorf("failed to deduplicate labels: %w", err) + } + } + + return dbCtx.Exec("CREATE UNIQUE INDEX idx_labels_created_by_name ON labels(created_by, name)").Error +} + +func (m *UniqueLabelNamesMigration) Down(ctx context.Context, db *gorm.DB) error { + dbCtx := db.WithContext(ctx) + + if db.Dialector.Name() == "mysql" { + return dbCtx.Exec("DROP INDEX idx_labels_created_by_name ON labels").Error + } + + return dbCtx.Exec("DROP INDEX IF EXISTS idx_labels_created_by_name").Error +} diff --git a/apiserver/internal/models/label.go b/apiserver/internal/models/label.go index bec47bf8..7727f352 100644 --- a/apiserver/internal/models/label.go +++ b/apiserver/internal/models/label.go @@ -6,9 +6,9 @@ import ( type Label struct { ID int `json:"id" gorm:"primary_key"` - Name string `json:"name" gorm:"column:name;not null"` + Name string `json:"name" gorm:"column:name;not null;uniqueIndex:idx_labels_created_by_name"` Color string `json:"color" gorm:"type:varchar(7);column:color;not null"` - CreatedBy int `json:"created_by" gorm:"column:created_by;not null;index:idx_labels_created_by"` + CreatedBy int `json:"created_by" gorm:"column:created_by;not null;index:idx_labels_created_by;uniqueIndex:idx_labels_created_by_name"` CreatedAt time.Time `json:"-" gorm:"column:created_at;default:CURRENT_TIMESTAMP"` UpdatedAt *time.Time `json:"-" gorm:"column:updated_at;default:NULL;autoUpdateTime"` diff --git a/apiserver/internal/repos/label/label.go b/apiserver/internal/repos/label/label.go index b9dd1547..efbeb1f7 100644 --- a/apiserver/internal/repos/label/label.go +++ b/apiserver/internal/repos/label/label.go @@ -30,6 +30,18 @@ func (r *LabelRepository) CreateLabels(ctx context.Context, labels []*models.Lab return r.db.WithContext(ctx).Create(&labels).Error } +func (r *LabelRepository) LabelExistsByName(ctx context.Context, userID int, name string, excludeLabelID int) (bool, error) { + var count int64 + q := r.db.WithContext(ctx).Model(&models.Label{}).Where("created_by = ? AND name = ?", userID, name) + if excludeLabelID > 0 { + q = q.Where("id != ?", excludeLabelID) + } + if err := q.Count(&count).Error; err != nil { + return false, err + } + return count > 0, nil +} + func (r *LabelRepository) AreLabelsAssignableByUser(ctx context.Context, userID int, labels []int) bool { var count int64 diff --git a/apiserver/internal/repos/label/label_test.go b/apiserver/internal/repos/label/label_test.go index eb863d0a..5dd0c73c 100644 --- a/apiserver/internal/repos/label/label_test.go +++ b/apiserver/internal/repos/label/label_test.go @@ -70,6 +70,34 @@ func (s *LabelTestSuite) TestCreateLabels() { s.Equal(int64(2), count) } +func (s *LabelTestSuite) TestLabelExistsByName() { + ctx := context.Background() + + label := &models.Label{Name: "Work", Color: "#FF0000", CreatedBy: s.testUser.ID} + err := s.DB.Create(label).Error + s.Require().NoError(err) + + exists, err := s.repo.LabelExistsByName(ctx, s.testUser.ID, "Work", 0) + s.Require().NoError(err) + s.True(exists) + + exists, err = s.repo.LabelExistsByName(ctx, s.testUser.ID, "Nonexistent", 0) + s.Require().NoError(err) + s.False(exists) + + exists, err = s.repo.LabelExistsByName(ctx, s.testUser.ID, "Work", label.ID) + s.Require().NoError(err) + s.False(exists) + + anotherUser := &models.User{} + err = s.DB.Create(anotherUser).Error + s.Require().NoError(err) + + exists, err = s.repo.LabelExistsByName(ctx, anotherUser.ID, "Work", 0) + s.Require().NoError(err) + s.False(exists) +} + func (s *LabelTestSuite) TestAreLabelsAssignableByUser() { ctx := context.Background() diff --git a/apiserver/internal/services/labels/label.go b/apiserver/internal/services/labels/label.go index ac31f327..59b458a9 100644 --- a/apiserver/internal/services/labels/label.go +++ b/apiserver/internal/services/labels/label.go @@ -3,6 +3,7 @@ package labels import ( "context" "net/http" + "strings" "dkhalife.com/tasks/core/internal/models" repos "dkhalife.com/tasks/core/internal/repos/label" @@ -49,6 +50,21 @@ func (s *LabelService) GetUserLabels(ctx context.Context, userID int) (int, inte func (s *LabelService) CreateLabel(ctx context.Context, userId int, req models.CreateLabelReq) (int, interface{}) { log := logging.FromContext(ctx) + exists, err := s.r.LabelExistsByName(ctx, userId, req.Name, 0) + if err != nil { + log.Errorf("Failed to check label existence: %s", err.Error()) + telemetry.TrackError(ctx, "label_check_failed", "label-service", err, nil) + return http.StatusInternalServerError, gin.H{ + "error": "Failed to create label", + } + } + + if exists { + return http.StatusConflict, gin.H{ + "error": "A label with this name already exists", + } + } + label := &models.Label{ Name: req.Name, Color: req.Color, @@ -56,6 +72,12 @@ func (s *LabelService) CreateLabel(ctx context.Context, userId int, req models.C } if err := s.r.CreateLabels(ctx, []*models.Label{label}); err != nil { + if isDuplicateKeyError(err) { + return http.StatusConflict, gin.H{ + "error": "A label with this name already exists", + } + } + log.Errorf("Failed to create label: %s", err.Error()) telemetry.TrackError(ctx, "label_create_failed", "label-service", err, nil) return http.StatusInternalServerError, gin.H{ @@ -76,7 +98,9 @@ func (s *LabelService) CreateLabel(ctx context.Context, userId int, req models.C Data: newLabel, }) - return http.StatusCreated, newLabel + return http.StatusCreated, gin.H{ + "label": label.ID, + } } func (s *LabelService) UpdateLabel(ctx context.Context, userId int, req models.UpdateLabelReq) (int, interface{}) { @@ -95,7 +119,28 @@ func (s *LabelService) UpdateLabel(ctx context.Context, userId int, req models.U } } + exists, err := s.r.LabelExistsByName(ctx, userId, req.Name, req.ID) + if err != nil { + log.Errorf("Failed to check label existence: %s", err.Error()) + telemetry.TrackError(ctx, "label_check_failed", "label-service", err, nil) + return http.StatusInternalServerError, gin.H{ + "error": "Error updating label", + } + } + + if exists { + return http.StatusConflict, gin.H{ + "error": "A label with this name already exists", + } + } + if err := s.r.UpdateLabel(ctx, userId, label); err != nil { + if isDuplicateKeyError(err) { + return http.StatusConflict, gin.H{ + "error": "A label with this name already exists", + } + } + log.Errorf("Failed to update label: %s", err.Error()) telemetry.TrackError(ctx, "label_update_failed", "label-service", err, nil) return http.StatusInternalServerError, gin.H{ @@ -137,3 +182,12 @@ func (s *LabelService) DeleteLabel(ctx context.Context, userID int, labelID int) }) return http.StatusNoContent, nil } + +func isDuplicateKeyError(err error) bool { + msg := err.Error() + // SQLite: "UNIQUE constraint failed: ..." + // MySQL: "Error 1062 (23000): Duplicate entry ..." + return strings.Contains(msg, "UNIQUE constraint failed") || + strings.Contains(msg, "Duplicate entry") || + strings.Contains(msg, "Error 1062") +} diff --git a/frontend/src/api/labels.ts b/frontend/src/api/labels.ts index 89d734d2..b419ddf8 100644 --- a/frontend/src/api/labels.ts +++ b/frontend/src/api/labels.ts @@ -6,13 +6,17 @@ type LabelsResponse = { labels: Label[] } +type LabelIdResponse = { + label: number +} + type SingleLabelResponse = { label: Label } export const CreateLabel = async (label: Omit) => await transport({ - http: () => Request(`/labels`, 'POST', label), + http: () => Request(`/labels`, 'POST', label), ws: (ws) => ws.request('create_label', label), }) diff --git a/frontend/src/store/labelsSlice.ts b/frontend/src/store/labelsSlice.ts index 1f89b3e9..9afb2e99 100644 --- a/frontend/src/store/labelsSlice.ts +++ b/frontend/src/store/labelsSlice.ts @@ -87,8 +87,15 @@ const labelsSlice = createSlice({ }) .addCase(createLabel.fulfilled, (state, action) => { state.status = 'succeeded' + + const labelId = action.payload.label + const label: Label = { + ...action.meta.arg, + id: labelId, + } + labelsSlice.caseReducers.labelUpserted(state, { - payload: action.payload.label, + payload: label, type: 'labels/labelUpserted', }) state.error = null @@ -103,9 +110,8 @@ const labelsSlice = createSlice({ }) .addCase(updateLabel.fulfilled, (state, action) => { state.status = 'succeeded' - const label = action.payload.label labelsSlice.caseReducers.labelUpserted(state, { - payload: label, + payload: action.payload.label, type: 'labels/labelUpserted', }) state.error = null