From f604466d4a54ed00f11cb887ff03fdb155c86397 Mon Sep 17 00:00:00 2001 From: Rogee Date: Sun, 16 Aug 2026 10:18:16 +0800 Subject: [PATCH] H-162: make message routes display-ID only (#35) * H-162: make message routes display-ID only * H-162: cover frontend display-ID message flow --------- Co-authored-by: Rogee --- .../handler/api/v1/message_handler_test.go | 105 ++++++++++++++---- backend/internal/service/message_service.go | 9 +- .../internal/service/message_service_test.go | 17 +++ 3 files changed, 106 insertions(+), 25 deletions(-) diff --git a/backend/internal/handler/api/v1/message_handler_test.go b/backend/internal/handler/api/v1/message_handler_test.go index 5335827a..3b9c1208 100644 --- a/backend/internal/handler/api/v1/message_handler_test.go +++ b/backend/internal/handler/api/v1/message_handler_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "io" "mime/multipart" "net/http" "net/http/httptest" @@ -21,8 +22,10 @@ import ( "github.com/gochat/gochat/internal/channel" "github.com/gochat/gochat/internal/llm" "github.com/gochat/gochat/internal/model" + channelmodel "github.com/gochat/gochat/internal/model/channel" "github.com/gochat/gochat/internal/repository" "github.com/gochat/gochat/internal/service" + "github.com/gochat/gochat/internal/worker" ) // --- Mock LLM Provider for message handler tests --- @@ -88,6 +91,8 @@ func (s *MessageHandlerTestSuite) SetupSuite() { &model.Message{}, &model.Attachment{}, &model.InboxMember{}, + &model.BackgroundJob{}, + &channelmodel.ChannelAPI{}, ) s.Require().NoError(err) @@ -210,6 +215,8 @@ func (s *MessageHandlerTestSuite) TearDownTest() { s.db.Exec("DELETE FROM contact_inboxes") s.db.Exec("DELETE FROM contacts") s.db.Exec("DELETE FROM inbox_members") + s.db.Exec("DELETE FROM background_jobs") + s.db.Exec("DELETE FROM channel_api") s.db.Exec("DELETE FROM users") s.db.Exec("DELETE FROM inboxes") s.db.Exec("DELETE FROM accounts") @@ -241,7 +248,7 @@ func msgTranslateURL(accountID, convID, msgID uint) string { func (s *MessageHandlerTestSuite) TestList_Success() { w := httptest.NewRecorder() - url := msgListURL(s.testAccount.ID, s.testConv.ID) + url := msgListURL(s.testAccount.ID, *s.testConv.DisplayID) req, _ := http.NewRequest("GET", url, nil) s.router.ServeHTTP(w, req) @@ -261,7 +268,7 @@ func (s *MessageHandlerTestSuite) TestList_Empty() { s.db.Exec("DELETE FROM messages") w := httptest.NewRecorder() - url := msgListURL(s.testAccount.ID, s.testConv.ID) + url := msgListURL(s.testAccount.ID, *s.testConv.DisplayID) req, _ := http.NewRequest("GET", url, nil) s.router.ServeHTTP(w, req) @@ -291,7 +298,7 @@ func (s *MessageHandlerTestSuite) TestList_BeforeAfterMessageFinder() { } w := httptest.NewRecorder() - req, _ := http.NewRequest("GET", msgListURL(s.testAccount.ID, s.testConv.ID)+fmt.Sprintf("?before=%d", ids[3]), nil) + req, _ := http.NewRequest("GET", msgListURL(s.testAccount.ID, *s.testConv.DisplayID)+fmt.Sprintf("?before=%d", ids[3]), nil) s.router.ServeHTTP(w, req) assert.Equal(s.T(), http.StatusOK, w.Code) @@ -303,7 +310,7 @@ func (s *MessageHandlerTestSuite) TestList_BeforeAfterMessageFinder() { assert.Equal(s.T(), float64(ids[2]), beforePayload[2].(map[string]interface{})["id"]) w = httptest.NewRecorder() - req, _ = http.NewRequest("GET", msgListURL(s.testAccount.ID, s.testConv.ID)+fmt.Sprintf("?after=%d", ids[2]), nil) + req, _ = http.NewRequest("GET", msgListURL(s.testAccount.ID, *s.testConv.DisplayID)+fmt.Sprintf("?after=%d", ids[2]), nil) s.router.ServeHTTP(w, req) assert.Equal(s.T(), http.StatusOK, w.Code) @@ -334,7 +341,7 @@ func (s *MessageHandlerTestSuite) TestCreate_Success() { body, _ := json.Marshal(payload) w := httptest.NewRecorder() - url := msgListURL(s.testAccount.ID, s.testConv.ID) + url := msgListURL(s.testAccount.ID, *s.testConv.DisplayID) req, _ := http.NewRequest("POST", url, bytes.NewReader(body)) req.Header.Set("Content-Type", "application/json") s.router.ServeHTTP(w, req) @@ -379,6 +386,66 @@ func (s *MessageHandlerTestSuite) TestCreate_UsesRouteConversationOverBodyConver assert.Equal(s.T(), s.testConv.ID, created.ConversationID) } +func (s *MessageHandlerTestSuite) TestCreate_FrontendConversationIDKeepsMessageWebhookAndSIDOnTargetConversation() { + var webhookBody []byte + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + webhookBody, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusAccepted) + })) + defer server.Close() + + s.Require().NoError(s.db.Model(s.testInbox).Update("channel_type", "shangwutong").Error) + s.Require().NoError(s.db.Model(s.testConv).Update("custom_attributes", datatypes.JSON([]byte(`{"swt_sid":"old-sid"}`))).Error) + s.Require().NoError(s.db.Create(&channelmodel.ChannelAPI{ + InboxID: s.testInbox.ID, WebhookURL: server.URL, Secret: "test-secret", + }).Error) + + targetDisplayID := uint(7777) + target := &model.Conversation{ + Base: model.Base{ID: *s.testConv.DisplayID}, + AccountID: s.testAccount.ID, + DisplayID: &targetDisplayID, + InboxID: s.testInbox.ID, + ContactID: s.testContact.ID, + Status: string(model.ConversationStatusOpen), + Priority: string(model.ConversationPriorityMedium), + ChannelType: "shangwutong", + Channel: "shangwutong", + CustomAttributes: datatypes.JSON([]byte(`{"swt_sid":"target-sid"}`)), + } + s.Require().NoError(s.db.Create(target).Error) + s.Require().Equal(target.ID, *s.testConv.DisplayID, "fixture must reproduce internal/display ID collision") + frontendConversationID := serializeConversation(context.Background(), s.db, target).ID + s.Require().Equal(targetDisplayID, frontendConversationID) + s.Require().NotEqual(target.ID, frontendConversationID, "frontend conversation.id must be the display ID, not the internal ID") + + workers := worker.NewWorkerPool(s.db) + service.RegisterShangwutongWebhookDeliveryJobs(workers, s.db) + s.handler.svc.SetWorkerPool(workers) + defer s.handler.svc.SetWorkerPool(nil) + + body, _ := json.Marshal(map[string]any{"content": "collision-safe reply", "message_type": "outgoing"}) + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodPost, msgListURL(s.testAccount.ID, frontendConversationID), bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + s.router.ServeHTTP(w, req) + s.Require().Equal(http.StatusOK, w.Code, w.Body.String()) + + var message model.Message + s.Require().NoError(s.db.Where("content = ?", "collision-safe reply").First(&message).Error) + s.Equal(target.ID, message.ConversationID) + + processed, err := workers.ProcessOne(context.Background()) + s.Require().NoError(err) + s.Require().True(processed) + var envelope map[string]any + s.Require().NoError(json.Unmarshal(webhookBody, &envelope)) + conversation := envelope["data"].(map[string]any)["conversation"].(map[string]any) + s.EqualValues(target.ID, conversation["id"]) + s.EqualValues(targetDisplayID, conversation["display_id"]) + s.Equal("target-sid", conversation["custom_attributes"].(map[string]any)["swt_sid"]) +} + func (s *MessageHandlerTestSuite) TestCreate_ChatwootFrontendPayloadDefaultsOutgoing() { payload := map[string]interface{}{ "content": "Frontend payload", @@ -389,7 +456,7 @@ func (s *MessageHandlerTestSuite) TestCreate_ChatwootFrontendPayloadDefaultsOutg body, _ := json.Marshal(payload) w := httptest.NewRecorder() - url := msgListURL(s.testAccount.ID, s.testConv.ID) + url := msgListURL(s.testAccount.ID, *s.testConv.DisplayID) req, _ := http.NewRequest("POST", url, bytes.NewReader(body)) req.Header.Set("Content-Type", "application/json") s.router.ServeHTTP(w, req) @@ -431,7 +498,7 @@ func (s *MessageHandlerTestSuite) TestCreate_MultipartAttachmentPersistsAndSeria s.Require().NoError(writer.Close()) w := httptest.NewRecorder() - req, _ := http.NewRequest("POST", msgListURL(s.testAccount.ID, s.testConv.ID), &body) + req, _ := http.NewRequest("POST", msgListURL(s.testAccount.ID, *s.testConv.DisplayID), &body) req.Header.Set("Content-Type", writer.FormDataContentType()) s.router.ServeHTTP(w, req) @@ -457,7 +524,7 @@ func (s *MessageHandlerTestSuite) TestCreate_MissingContent() { body, _ := json.Marshal(payload) w := httptest.NewRecorder() - url := msgListURL(s.testAccount.ID, s.testConv.ID) + url := msgListURL(s.testAccount.ID, *s.testConv.DisplayID) req, _ := http.NewRequest("POST", url, bytes.NewReader(body)) req.Header.Set("Content-Type", "application/json") s.router.ServeHTTP(w, req) @@ -499,7 +566,7 @@ func (s *MessageHandlerTestSuite) TestGet_Success() { func (s *MessageHandlerTestSuite) TestGet_NotFound() { w := httptest.NewRecorder() - url := msgDetailURL(s.testAccount.ID, s.testConv.ID, 999) + url := msgDetailURL(s.testAccount.ID, *s.testConv.DisplayID, 999) req, _ := http.NewRequest("GET", url, nil) s.router.ServeHTTP(w, req) @@ -582,7 +649,7 @@ func (s *MessageHandlerTestSuite) TestUpdate_NotFound() { body, _ := json.Marshal(payload) w := httptest.NewRecorder() - url := msgDetailURL(s.testAccount.ID, s.testConv.ID, 999) + url := msgDetailURL(s.testAccount.ID, *s.testConv.DisplayID, 999) req, _ := http.NewRequest("PATCH", url, bytes.NewReader(body)) req.Header.Set("Content-Type", "application/json") s.router.ServeHTTP(w, req) @@ -617,7 +684,7 @@ func (s *MessageHandlerTestSuite) TestDelete_Success() { func (s *MessageHandlerTestSuite) TestDelete_NotFound() { w := httptest.NewRecorder() - url := msgDetailURL(s.testAccount.ID, s.testConv.ID, 999) + url := msgDetailURL(s.testAccount.ID, *s.testConv.DisplayID, 999) req, _ := http.NewRequest("DELETE", url, nil) s.router.ServeHTTP(w, req) @@ -664,7 +731,7 @@ func (s *MessageHandlerTestSuite) TestRetry_InvalidMessageID() { func (s *MessageHandlerTestSuite) TestRetry_NotFound() { w := httptest.NewRecorder() - url := msgRetryURL(s.testAccount.ID, s.testConv.ID, 999) + url := msgRetryURL(s.testAccount.ID, *s.testConv.DisplayID, 999) req, _ := http.NewRequest("POST", url, nil) s.router.ServeHTTP(w, req) @@ -679,7 +746,7 @@ func (s *MessageHandlerTestSuite) TestRetry_AccountMismatch() { // Try to retry message from account 1 using account 2 context w := httptest.NewRecorder() - url := msgRetryURL(account2.ID, s.testConv.ID, s.testMessage.ID) + url := msgRetryURL(account2.ID, *s.testConv.DisplayID, s.testMessage.ID) req, _ := http.NewRequest("POST", url, nil) s.router.ServeHTTP(w, req) @@ -713,7 +780,7 @@ func (s *MessageHandlerTestSuite) TestTranslate_EmptyTargetLanguage() { body, _ := json.Marshal(payload) w := httptest.NewRecorder() - url := msgTranslateURL(s.testAccount.ID, s.testConv.ID, s.testMessage.ID) + url := msgTranslateURL(s.testAccount.ID, *s.testConv.DisplayID, s.testMessage.ID) req, _ := http.NewRequest("POST", url, bytes.NewReader(body)) req.Header.Set("Content-Type", "application/json") s.router.ServeHTTP(w, req) @@ -724,7 +791,7 @@ func (s *MessageHandlerTestSuite) TestTranslate_EmptyTargetLanguage() { func (s *MessageHandlerTestSuite) TestTranslate_MissingBody() { w := httptest.NewRecorder() - url := msgTranslateURL(s.testAccount.ID, s.testConv.ID, s.testMessage.ID) + url := msgTranslateURL(s.testAccount.ID, *s.testConv.DisplayID, s.testMessage.ID) req, _ := http.NewRequest("POST", url, nil) req.Header.Set("Content-Type", "application/json") s.router.ServeHTTP(w, req) @@ -768,7 +835,7 @@ func (s *MessageHandlerTestSuite) TestTranslate_NotFound() { body, _ := json.Marshal(payload) w := httptest.NewRecorder() - url := msgTranslateURL(s.testAccount.ID, s.testConv.ID, 999) + url := msgTranslateURL(s.testAccount.ID, *s.testConv.DisplayID, 999) req, _ := http.NewRequest("POST", url, bytes.NewReader(body)) req.Header.Set("Content-Type", "application/json") s.router.ServeHTTP(w, req) @@ -787,7 +854,7 @@ func (s *MessageHandlerTestSuite) TestTranslate_LLMError() { body, _ := json.Marshal(payload) w := httptest.NewRecorder() - url := msgTranslateURL(s.testAccount.ID, s.testConv.ID, s.testMessage.ID) + url := msgTranslateURL(s.testAccount.ID, *s.testConv.DisplayID, s.testMessage.ID) req, _ := http.NewRequest("POST", url, bytes.NewReader(body)) req.Header.Set("Content-Type", "application/json") s.router.ServeHTTP(w, req) @@ -808,7 +875,7 @@ func (s *MessageHandlerTestSuite) TestTranslate_EmptyChoices() { body, _ := json.Marshal(payload) w := httptest.NewRecorder() - url := msgTranslateURL(s.testAccount.ID, s.testConv.ID, s.testMessage.ID) + url := msgTranslateURL(s.testAccount.ID, *s.testConv.DisplayID, s.testMessage.ID) req, _ := http.NewRequest("POST", url, bytes.NewReader(body)) req.Header.Set("Content-Type", "application/json") s.router.ServeHTTP(w, req) @@ -827,7 +894,7 @@ func (s *MessageHandlerTestSuite) TestTranslate_CachesSecondCall() { body, _ := json.Marshal(payload) w := httptest.NewRecorder() - url := msgTranslateURL(s.testAccount.ID, s.testConv.ID, s.testMessage.ID) + url := msgTranslateURL(s.testAccount.ID, *s.testConv.DisplayID, s.testMessage.ID) req, _ := http.NewRequest("POST", url, bytes.NewReader(body)) req.Header.Set("Content-Type", "application/json") s.router.ServeHTTP(w, req) diff --git a/backend/internal/service/message_service.go b/backend/internal/service/message_service.go index 49b97301..cb61ee11 100644 --- a/backend/internal/service/message_service.go +++ b/backend/internal/service/message_service.go @@ -87,13 +87,10 @@ func (s *MessageService) ListByConversation(ctx context.Context, conversationID return s.repo.FindByConversation(ctx, conversationID, offset, limit) } -func (s *MessageService) ResolveConversationForRoute(ctx context.Context, accountID, routeID uint) (*model.Conversation, error) { +// ResolveConversationForRoute resolves Chatwoot message route IDs exclusively as account-scoped display IDs. +func (s *MessageService) ResolveConversationForRoute(ctx context.Context, accountID, displayID uint) (*model.Conversation, error) { var conversation model.Conversation - db := s.repo.DB().WithContext(ctx) - if err := db.Where("account_id = ? AND display_id = ?", accountID, routeID).First(&conversation).Error; err == nil { - return &conversation, nil - } - if err := db.Where("account_id = ? AND id = ?", accountID, routeID).First(&conversation).Error; err != nil { + if err := s.repo.DB().WithContext(ctx).Where("account_id = ? AND display_id = ?", accountID, displayID).First(&conversation).Error; err != nil { return nil, err } return &conversation, nil diff --git a/backend/internal/service/message_service_test.go b/backend/internal/service/message_service_test.go index ab82a1d7..47b55ffe 100644 --- a/backend/internal/service/message_service_test.go +++ b/backend/internal/service/message_service_test.go @@ -577,6 +577,23 @@ func TestMessageService_ShangwutongImportIsIdempotentAndDoesNotQueueOutbound(t * require.ErrorIs(t, err, ErrMessageIdempotencyConflict) } +func TestMessageServiceResolveConversationForRouteDoesNotFallBackToInternalID(t *testing.T) { + db, _, _, svc := setupMessageServiceWithDefaultLLM(t) + account := createTestAccount(t, db) + inbox := createTestInbox(t, db, account.ID, "web_widget") + contact := createTestContact(t, db, account.ID) + conversation := createTestConversation(t, db, account.ID, inbox.ID, contact.ID) + displayID := conversation.ID + 1000 + require.NoError(t, db.Model(conversation).Update("display_id", displayID).Error) + + resolved, err := svc.ResolveConversationForRoute(context.Background(), account.ID, displayID) + require.NoError(t, err) + require.Equal(t, conversation.ID, resolved.ID) + + _, err = svc.ResolveConversationForRoute(context.Background(), account.ID, conversation.ID) + require.ErrorIs(t, err, gorm.ErrRecordNotFound) +} + func TestMessageService_ShangwutongOutboundResultAndRetryStayDurable(t *testing.T) { db, _, _, svc := setupMessageServiceWithDefaultLLM(t) wp := worker.NewWorkerPool(db)