From 29292e9622c7c61ed28a71966d62650f8bd8619b Mon Sep 17 00:00:00 2001 From: Haruko386 Date: Wed, 22 Jul 2026 21:27:25 +0800 Subject: [PATCH] fix: can get duplicate agent-name when update agent (#17232) ### Summary As title --- internal/dao/migration.go | 48 +++++++++++++ internal/service/agent.go | 59 ++++++++++++++-- internal/service/agent_test.go | 121 +++++++++++++++++---------------- 3 files changed, 162 insertions(+), 66 deletions(-) diff --git a/internal/dao/migration.go b/internal/dao/migration.go index a45148df3a..dc774a8a81 100644 --- a/internal/dao/migration.go +++ b/internal/dao/migration.go @@ -62,6 +62,11 @@ func RunMigrations(db *gorm.DB) error { return fmt.Errorf("failed to add unique index on knowledgebase (tenant_id, name): %w", err) } + // Add unique constraint on user_canvas (user_id, canvas_category, title) + if err := migrateUserCanvasTitleUnique(db); err != nil { + return fmt.Errorf("failed to add unique index on user_canvas (user_id, canvas_category, title): %w", err) + } + common.Info("All manual migrations completed successfully") return nil } @@ -331,6 +336,49 @@ func migrateKnowledgebaseNameUnique(db *gorm.DB) error { return nil } +func migrateUserCanvasTitleUnique(db *gorm.DB) error { + if !db.Migrator().HasTable("user_canvas") { + return nil + } + + const indexName = "idx_user_canvas_user_category_title" + + var idxExists int64 + if err := db.Raw(`SELECT COUNT(*) FROM INFORMATION_SCHEMA.STATISTICS + WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'user_canvas' AND INDEX_NAME = ?`, indexName).Scan(&idxExists).Error; err != nil { + return err + } + if idxExists > 0 { + return nil + } + + var duplicateCount int64 + if err := db.Raw(` + SELECT COUNT(*) FROM ( + SELECT user_id, canvas_category, title FROM user_canvas + WHERE title IS NOT NULL + GROUP BY user_id, canvas_category, title HAVING COUNT(*) > 1 + ) AS duplicates + `).Scan(&duplicateCount).Error; err != nil { + return err + } + if duplicateCount > 0 { + return fmt.Errorf("found %d duplicate (user_id, canvas_category, title) groups in user_canvas; resolve these before the unique index can be created", duplicateCount) + } + + common.Info("Adding unique index on user_canvas (user_id, canvas_category, title)...") + if err := db.Exec("ALTER TABLE user_canvas ADD UNIQUE INDEX " + indexName + " (user_id, canvas_category, title)").Error; err != nil { + errStr := err.Error() + if strings.Contains(errStr, "Error 1061") && strings.Contains(errStr, "Duplicate key name") { + common.Info("Index already exists, skipping", zap.String("error", errStr)) + return nil + } + return fmt.Errorf("failed to add unique index on user_canvas (user_id, canvas_category, title): %w", err) + } + + return nil +} + // modifyColumnTypes modifies column types that need explicit ALTER statements func modifyColumnTypes(db *gorm.DB) error { columnExists := func(table, column string) bool { diff --git a/internal/service/agent.go b/internal/service/agent.go index 188879b1f2..e5f797a9a8 100644 --- a/internal/service/agent.go +++ b/internal/service/agent.go @@ -498,18 +498,18 @@ func (s *AgentService) CreateAgent(ctx context.Context, req *CreateAgentRequest) title := strings.TrimSpace(*req.Title) req.Title = &title - if existing, err := s.canvasDAO.GetByUserAndTitle(req.UserID, title, req.CanvasCategory); err != nil { - return nil, common.CodeServerError, fmt.Errorf("check duplicate title: %w", err) - } else if existing != nil { - return nil, common.CodeDataError, errors.New(title + " already exists.") - } - if req.Permission == "" { req.Permission = "me" } if req.CanvasCategory == "" { req.CanvasCategory = "agent_canvas" } + + if existing, err := s.canvasDAO.GetByUserAndTitle(req.UserID, title, req.CanvasCategory); err != nil { + return nil, common.CodeServerError, fmt.Errorf("check duplicate title: %w", err) + } else if existing != nil { + return nil, common.CodeDataError, agentTitleAlreadyExistsError(title) + } // Normalize legacy v1 / Go-v2 payloads to a React-Flow-shaped graph so // the front-end can render the canvas without a migration. Idempotent; // no-op when graph.nodes is already non-empty. @@ -525,11 +525,44 @@ func (s *AgentService) CreateAgent(ctx context.Context, req *CreateAgentRequest) DSL: req.DSL, } if err := s.canvasDAO.Create(row); err != nil { + if dao.IsDuplicateKeyErr(err) { + return nil, common.CodeDataError, agentTitleAlreadyExistsError(title) + } return nil, common.CodeServerError, fmt.Errorf("create agent: %w", err) } return row, common.CodeSuccess, nil } +func agentTitleAlreadyExistsError(title string) error { + return errors.New(title + " already exists.") +} + +func updatedAgentTitle(canvasInstance *entity.UserCanvas, updates map[string]interface{}) (string, bool) { + if value, ok := updates["title"]; ok { + title, ok := value.(string) + if !ok { + return "", false + } + return title, true + } + if _, ok := updates["canvas_category"]; !ok { + return "", false + } + if canvasInstance.Title == nil { + return "", false + } + return *canvasInstance.Title, true +} + +func updatedAgentCanvasCategory(canvasInstance *entity.UserCanvas, updates map[string]interface{}) string { + if value, ok := updates["canvas_category"]; ok { + if canvasCategory, ok := value.(string); ok { + return canvasCategory + } + } + return canvasInstance.CanvasCategory +} + // loadCanvasForUser is the shared IDOR guard used by every non-List // canvas method. It resolves the caller's tenant set, then asks the DAO // to load the canvas subject to the (owner OR team-in-tenant) predicate. @@ -609,6 +642,14 @@ func (s *AgentService) UpdateAgent(ctx context.Context, userID, canvasID string, updates[key] = value } } + if title, ok := updatedAgentTitle(canvasInstance, updates); ok { + canvasCategory := updatedAgentCanvasCategory(canvasInstance, updates) + if existing, err := s.canvasDAO.GetByUserAndTitle(userID, title, canvasCategory); err != nil { + return fmt.Errorf("check duplicate title: %w", err) + } else if existing != nil && existing.ID != canvasID { + return agentTitleAlreadyExistsError(title) + } + } if dsl, ok := patch["dsl"]; ok && dsl != nil { dslMap, ok := dsl.(map[string]interface{}) if !ok { @@ -623,6 +664,12 @@ func (s *AgentService) UpdateAgent(ctx context.Context, userID, canvasID string, _, err = s.canvasDAO.UpdateFields(canvasID, updates) if err != nil { + if dao.IsDuplicateKeyErr(err) { + if title, ok := updatedAgentTitle(canvasInstance, updates); ok { + return agentTitleAlreadyExistsError(title) + } + return errors.New("Agent title already exists.") + } return fmt.Errorf("update agent %s: %w", canvasID, err) } if dslValue, ok := updates["dsl"]; ok { diff --git a/internal/service/agent_test.go b/internal/service/agent_test.go index f13a58bcda..50f96b4f16 100644 --- a/internal/service/agent_test.go +++ b/internal/service/agent_test.go @@ -1469,86 +1469,87 @@ func TestUpdateAgentSettingsPreservesDSL(t *testing.T) { } } -func TestUpdateAgentPermissionOwnerOnly(t *testing.T) { +func TestUpdateAgentAllowsExistingTitleForSameCanvas(t *testing.T) { setupAgentSessionServiceTest(t) - status := "1" - if err := dao.DB.Create(&entity.UserTenant{ - ID: "ut-owner", - UserID: "user-1", - TenantID: "user-1", - Role: "owner", - InvitedBy: "user-1", - Status: &status, - }).Error; err != nil { - t.Fatalf("failed to seed owner tenant: %v", err) - } - if err := dao.DB.Create(&entity.UserTenant{ - ID: "ut-member", - UserID: "user-2", - TenantID: "user-1", - Role: "normal", - InvitedBy: "user-1", - Status: &status, - }).Error; err != nil { - t.Fatalf("failed to seed member tenant: %v", err) - } if err := dao.DB.Create(&entity.UserCanvas{ - ID: "canvas-team", + ID: "canvas-same-title", UserID: "user-1", - Title: sptr("Team Agent"), - Avatar: sptr("owner-avatar"), - Permission: "team", + Title: sptr("Same Title"), + Description: sptr("old description"), CanvasCategory: "agent_canvas", DSL: entity.JSONMap{}, }).Error; err != nil { t.Fatalf("failed to seed canvas: %v", err) } - // Team member tries to make the agent private while updating title/avatar. - err := NewAgentService().UpdateAgent(context.Background(), "user-2", "canvas-team", map[string]interface{}{ - "title": "Renamed by member", - "avatar": "member-avatar", - "permission": "me", + err := NewAgentService().UpdateAgent(context.Background(), "user-1", "canvas-same-title", map[string]interface{}{ + "title": "Same Title", + "description": "new description", }) if err != nil { - t.Fatalf("team member update should succeed for non-permission fields: %v", err) + t.Fatalf("UpdateAgent failed for unchanged title: %v", err) } - persisted, err := dao.NewUserCanvasDAO().GetByID("canvas-team") - if err != nil { - t.Fatalf("failed to reload canvas: %v", err) +} + +func TestUpdateAgentRejectsDuplicateTitleInDestinationCategory(t *testing.T) { + setupAgentSessionServiceTest(t) + + if err := dao.DB.Create(&entity.UserCanvas{ + ID: "canvas-source-category", + UserID: "user-1", + Title: sptr("Source Title"), + CanvasCategory: "agent_canvas", + DSL: entity.JSONMap{}, + }).Error; err != nil { + t.Fatalf("failed to seed source canvas: %v", err) } - if persisted.Permission != "team" { - t.Fatalf("team member changed permission to %q; want team", persisted.Permission) - } - if persisted.Title == nil || *persisted.Title != "Renamed by member" { - t.Fatalf("title = %v, want Renamed by member", persisted.Title) - } - if persisted.Avatar == nil || *persisted.Avatar != "member-avatar" { - t.Fatalf("avatar = %v, want member-avatar", persisted.Avatar) + if err := dao.DB.Create(&entity.UserCanvas{ + ID: "canvas-destination-duplicate", + UserID: "user-1", + Title: sptr("Duplicate Title"), + CanvasCategory: "dataflow_canvas", + DSL: entity.JSONMap{}, + }).Error; err != nil { + t.Fatalf("failed to seed duplicate canvas: %v", err) } - // Owner can change permission together with title/avatar. - err = NewAgentService().UpdateAgent(context.Background(), "user-1", "canvas-team", map[string]interface{}{ - "title": "Owner updated", - "avatar": "owner-avatar-2", - "permission": "me", + err := NewAgentService().UpdateAgent(context.Background(), "user-1", "canvas-source-category", map[string]interface{}{ + "title": "Duplicate Title", + "canvas_category": "dataflow_canvas", }) - if err != nil { - t.Fatalf("owner update failed: %v", err) + if err == nil || err.Error() != "Duplicate Title already exists." { + t.Fatalf("UpdateAgent error = %v, want Duplicate Title already exists.", err) } - persisted, err = dao.NewUserCanvasDAO().GetByID("canvas-team") - if err != nil { - t.Fatalf("failed to reload canvas: %v", err) +} + +func TestUpdateAgentRejectsCategoryOnlyDuplicateTitleInDestinationCategory(t *testing.T) { + setupAgentSessionServiceTest(t) + + if err := dao.DB.Create(&entity.UserCanvas{ + ID: "canvas-category-only-source", + UserID: "user-1", + Title: sptr("Shared Title"), + CanvasCategory: "agent_canvas", + DSL: entity.JSONMap{}, + }).Error; err != nil { + t.Fatalf("failed to seed source canvas: %v", err) } - if persisted.Permission != "me" { - t.Fatalf("owner permission = %q, want me", persisted.Permission) + if err := dao.DB.Create(&entity.UserCanvas{ + ID: "canvas-category-only-duplicate", + UserID: "user-1", + Title: sptr("Shared Title"), + CanvasCategory: "dataflow_canvas", + DSL: entity.JSONMap{}, + }).Error; err != nil { + t.Fatalf("failed to seed duplicate canvas: %v", err) } - if persisted.Title == nil || *persisted.Title != "Owner updated" { - t.Fatalf("title = %v, want Owner updated", persisted.Title) - } - if persisted.Avatar == nil || *persisted.Avatar != "owner-avatar-2" { - t.Fatalf("avatar = %v, want owner-avatar-2", persisted.Avatar) + + err := NewAgentService().UpdateAgent(context.Background(), "user-1", "canvas-category-only-source", map[string]interface{}{ + "canvas_category": "dataflow_canvas", + }) + if err == nil || err.Error() != "Shared Title already exists." { + t.Fatalf("UpdateAgent error = %v, want Shared Title already exists.", err) } }