Skip to content

Commit aeb604d

Browse files
dkhalifeCopilot
dkhalife
and
Copilot
authored
fix: prevent duplicate label creation from Android sync engine (#310)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ac77f31 commit aeb604d

7 files changed

Lines changed: 219 additions & 7 deletions

File tree

‎apiserver/internal/migrations/008_unique_label_names.go‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
package migrations
2+
3+
import (
4+
"context"
5+
"fmt"
6+
7+
"gorm.io/gorm"
8+
)
9+
10+
func init() {
11+
Register(&UniqueLabelNamesMigration{})
12+
}
13+
14+
type UniqueLabelNamesMigration struct{}
15+
16+
func (m *UniqueLabelNamesMigration) Version() int {
17+
return 8
18+
}
19+
20+
func (m *UniqueLabelNamesMigration) Name() string {
21+
return "unique_label_names"
22+
}
23+
24+
func (m *UniqueLabelNamesMigration) Up(ctx context.Context, db *gorm.DB) error {
25+
dbCtx := db.WithContext(ctx)
26+
27+
dedup := []string{
28+
// Reassign task_labels from duplicate labels to the kept (lowest ID) label,
29+
// skipping rows that would collide with an existing (task_id, keep_id) pair
30+
`INSERT OR IGNORE INTO task_labels (task_id, label_id)
31+
SELECT tl.task_id, keep.id
32+
FROM task_labels tl
33+
JOIN labels dup ON tl.label_id = dup.id
34+
JOIN (
35+
SELECT MIN(id) AS id, name, created_by
36+
FROM labels
37+
GROUP BY created_by, name
38+
) keep ON dup.created_by = keep.created_by AND dup.name = keep.name
39+
WHERE dup.id != keep.id`,
40+
41+
// Remove task_labels pointing to duplicate labels
42+
`DELETE FROM task_labels WHERE label_id IN (
43+
SELECT l.id FROM labels l
44+
JOIN (
45+
SELECT MIN(id) AS keep_id, name, created_by
46+
FROM labels
47+
GROUP BY created_by, name
48+
) k ON l.created_by = k.created_by AND l.name = k.name
49+
WHERE l.id != k.keep_id
50+
)`,
51+
52+
// Delete the duplicate labels themselves
53+
`DELETE FROM labels WHERE id NOT IN (
54+
SELECT MIN(id) FROM labels GROUP BY created_by, name
55+
)`,
56+
}
57+
58+
dialect := db.Dialector.Name()
59+
if dialect == "mysql" {
60+
dedup = []string{
61+
`INSERT IGNORE INTO task_labels (task_id, label_id)
62+
SELECT tl.task_id, keep_tbl.keep_id
63+
FROM task_labels tl
64+
JOIN labels dup ON tl.label_id = dup.id
65+
JOIN (
66+
SELECT MIN(id) AS keep_id, name, created_by
67+
FROM labels
68+
GROUP BY created_by, name
69+
) keep_tbl ON dup.created_by = keep_tbl.created_by AND dup.name = keep_tbl.name
70+
WHERE dup.id != keep_tbl.keep_id`,
71+
72+
`DELETE tl FROM task_labels tl
73+
JOIN labels l ON tl.label_id = l.id
74+
JOIN (
75+
SELECT MIN(id) AS keep_id, name, created_by
76+
FROM labels
77+
GROUP BY created_by, name
78+
) k ON l.created_by = k.created_by AND l.name = k.name
79+
WHERE l.id != k.keep_id`,
80+
81+
`DELETE l FROM labels l
82+
JOIN (
83+
SELECT MIN(id) AS keep_id, name, created_by
84+
FROM labels
85+
GROUP BY created_by, name
86+
) k ON l.created_by = k.created_by AND l.name = k.name
87+
WHERE l.id != k.keep_id`,
88+
}
89+
}
90+
91+
for _, stmt := range dedup {
92+
if err := dbCtx.Exec(stmt).Error; err != nil {
93+
return fmt.Errorf("failed to deduplicate labels: %w", err)
94+
}
95+
}
96+
97+
return dbCtx.Exec("CREATE UNIQUE INDEX idx_labels_created_by_name ON labels(created_by, name)").Error
98+
}
99+
100+
func (m *UniqueLabelNamesMigration) Down(ctx context.Context, db *gorm.DB) error {
101+
dbCtx := db.WithContext(ctx)
102+
103+
if db.Dialector.Name() == "mysql" {
104+
return dbCtx.Exec("DROP INDEX idx_labels_created_by_name ON labels").Error
105+
}
106+
107+
return dbCtx.Exec("DROP INDEX IF EXISTS idx_labels_created_by_name").Error
108+
}

‎apiserver/internal/models/label.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,9 @@ import (
66

77
type Label struct {
88
ID int `json:"id" gorm:"primary_key"`
9-
Name string `json:"name" gorm:"column:name;not null"`
9+
Name string `json:"name" gorm:"column:name;not null;uniqueIndex:idx_labels_created_by_name"`
1010
Color string `json:"color" gorm:"type:varchar(7);column:color;not null"`
11-
CreatedBy int `json:"created_by" gorm:"column:created_by;not null;index:idx_labels_created_by"`
11+
CreatedBy int `json:"created_by" gorm:"column:created_by;not null;index:idx_labels_created_by;uniqueIndex:idx_labels_created_by_name"`
1212
CreatedAt time.Time `json:"-" gorm:"column:created_at;default:CURRENT_TIMESTAMP"`
1313
UpdatedAt *time.Time `json:"-" gorm:"column:updated_at;default:NULL;autoUpdateTime"`
1414

‎apiserver/internal/repos/label/label.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,18 @@ func (r *LabelRepository) CreateLabels(ctx context.Context, labels []*models.Lab
3030
return r.db.WithContext(ctx).Create(&labels).Error
3131
}
3232

33+
func (r *LabelRepository) LabelExistsByName(ctx context.Context, userID int, name string, excludeLabelID int) (bool, error) {
34+
var count int64
35+
q := r.db.WithContext(ctx).Model(&models.Label{}).Where("created_by = ? AND name = ?", userID, name)
36+
if excludeLabelID > 0 {
37+
q = q.Where("id != ?", excludeLabelID)
38+
}
39+
if err := q.Count(&count).Error; err != nil {
40+
return false, err
41+
}
42+
return count > 0, nil
43+
}
44+
3345
func (r *LabelRepository) AreLabelsAssignableByUser(ctx context.Context, userID int, labels []int) bool {
3446
var count int64
3547

‎apiserver/internal/repos/label/label_test.go‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,34 @@ func (s *LabelTestSuite) TestCreateLabels() {
7070
s.Equal(int64(2), count)
7171
}
7272

73+
func (s *LabelTestSuite) TestLabelExistsByName() {
74+
ctx := context.Background()
75+
76+
label := &models.Label{Name: "Work", Color: "#FF0000", CreatedBy: s.testUser.ID}
77+
err := s.DB.Create(label).Error
78+
s.Require().NoError(err)
79+
80+
exists, err := s.repo.LabelExistsByName(ctx, s.testUser.ID, "Work", 0)
81+
s.Require().NoError(err)
82+
s.True(exists)
83+
84+
exists, err = s.repo.LabelExistsByName(ctx, s.testUser.ID, "Nonexistent", 0)
85+
s.Require().NoError(err)
86+
s.False(exists)
87+
88+
exists, err = s.repo.LabelExistsByName(ctx, s.testUser.ID, "Work", label.ID)
89+
s.Require().NoError(err)
90+
s.False(exists)
91+
92+
anotherUser := &models.User{}
93+
err = s.DB.Create(anotherUser).Error
94+
s.Require().NoError(err)
95+
96+
exists, err = s.repo.LabelExistsByName(ctx, anotherUser.ID, "Work", 0)
97+
s.Require().NoError(err)
98+
s.False(exists)
99+
}
100+
73101
func (s *LabelTestSuite) TestAreLabelsAssignableByUser() {
74102
ctx := context.Background()
75103

‎apiserver/internal/services/labels/label.go‎

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package labels
33
import (
44
"context"
55
"net/http"
6+
"strings"
67

78
"dkhalife.com/tasks/core/internal/models"
89
repos "dkhalife.com/tasks/core/internal/repos/label"
@@ -49,13 +50,34 @@ func (s *LabelService) GetUserLabels(ctx context.Context, userID int) (int, inte
4950
func (s *LabelService) CreateLabel(ctx context.Context, userId int, req models.CreateLabelReq) (int, interface{}) {
5051
log := logging.FromContext(ctx)
5152

53+
exists, err := s.r.LabelExistsByName(ctx, userId, req.Name, 0)
54+
if err != nil {
55+
log.Errorf("Failed to check label existence: %s", err.Error())
56+
telemetry.TrackError(ctx, "label_check_failed", "label-service", err, nil)
57+
return http.StatusInternalServerError, gin.H{
58+
"error": "Failed to create label",
59+
}
60+
}
61+
62+
if exists {
63+
return http.StatusConflict, gin.H{
64+
"error": "A label with this name already exists",
65+
}
66+
}
67+
5268
label := &models.Label{
5369
Name: req.Name,
5470
Color: req.Color,
5571
CreatedBy: userId,
5672
}
5773

5874
if err := s.r.CreateLabels(ctx, []*models.Label{label}); err != nil {
75+
if isDuplicateKeyError(err) {
76+
return http.StatusConflict, gin.H{
77+
"error": "A label with this name already exists",
78+
}
79+
}
80+
5981
log.Errorf("Failed to create label: %s", err.Error())
6082
telemetry.TrackError(ctx, "label_create_failed", "label-service", err, nil)
6183
return http.StatusInternalServerError, gin.H{
@@ -76,7 +98,9 @@ func (s *LabelService) CreateLabel(ctx context.Context, userId int, req models.C
7698
Data: newLabel,
7799
})
78100

79-
return http.StatusCreated, newLabel
101+
return http.StatusCreated, gin.H{
102+
"label": label.ID,
103+
}
80104
}
81105

82106
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
95119
}
96120
}
97121

122+
exists, err := s.r.LabelExistsByName(ctx, userId, req.Name, req.ID)
123+
if err != nil {
124+
log.Errorf("Failed to check label existence: %s", err.Error())
125+
telemetry.TrackError(ctx, "label_check_failed", "label-service", err, nil)
126+
return http.StatusInternalServerError, gin.H{
127+
"error": "Error updating label",
128+
}
129+
}
130+
131+
if exists {
132+
return http.StatusConflict, gin.H{
133+
"error": "A label with this name already exists",
134+
}
135+
}
136+
98137
if err := s.r.UpdateLabel(ctx, userId, label); err != nil {
138+
if isDuplicateKeyError(err) {
139+
return http.StatusConflict, gin.H{
140+
"error": "A label with this name already exists",
141+
}
142+
}
143+
99144
log.Errorf("Failed to update label: %s", err.Error())
100145
telemetry.TrackError(ctx, "label_update_failed", "label-service", err, nil)
101146
return http.StatusInternalServerError, gin.H{
@@ -137,3 +182,12 @@ func (s *LabelService) DeleteLabel(ctx context.Context, userID int, labelID int)
137182
})
138183
return http.StatusNoContent, nil
139184
}
185+
186+
func isDuplicateKeyError(err error) bool {
187+
msg := err.Error()
188+
// SQLite: "UNIQUE constraint failed: ..."
189+
// MySQL: "Error 1062 (23000): Duplicate entry ..."
190+
return strings.Contains(msg, "UNIQUE constraint failed") ||
191+
strings.Contains(msg, "Duplicate entry") ||
192+
strings.Contains(msg, "Error 1062")
193+
}

‎frontend/src/api/labels.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,17 @@ type LabelsResponse = {
66
labels: Label[]
77
}
88

9+
type LabelIdResponse = {
10+
label: number
11+
}
12+
913
type SingleLabelResponse = {
1014
label: Label
1115
}
1216

1317
export const CreateLabel = async (label: Omit<Label, 'id'>) =>
1418
await transport({
15-
http: () => Request<SingleLabelResponse>(`/labels`, 'POST', label),
19+
http: () => Request<LabelIdResponse>(`/labels`, 'POST', label),
1620
ws: (ws) => ws.request('create_label', label),
1721
})
1822

‎frontend/src/store/labelsSlice.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,15 @@ const labelsSlice = createSlice({
8787
})
8888
.addCase(createLabel.fulfilled, (state, action) => {
8989
state.status = 'succeeded'
90+
91+
const labelId = action.payload.label
92+
const label: Label = {
93+
...action.meta.arg,
94+
id: labelId,
95+
}
96+
9097
labelsSlice.caseReducers.labelUpserted(state, {
91-
payload: action.payload.label,
98+
payload: label,
9299
type: 'labels/labelUpserted',
93100
})
94101
state.error = null
@@ -103,9 +110,8 @@ const labelsSlice = createSlice({
103110
})
104111
.addCase(updateLabel.fulfilled, (state, action) => {
105112
state.status = 'succeeded'
106-
const label = action.payload.label
107113
labelsSlice.caseReducers.labelUpserted(state, {
108-
payload: label,
114+
payload: action.payload.label,
109115
type: 'labels/labelUpserted',
110116
})
111117
state.error = null

0 commit comments

Comments
 (0)