diff --git a/server/router/api/v1/activity_service.go b/server/router/api/v1/activity_service.go index bcd0c9716..a58837dc4 100644 --- a/server/router/api/v1/activity_service.go +++ b/server/router/api/v1/activity_service.go @@ -34,9 +34,12 @@ func (s *APIV1Service) ListActivities(ctx context.Context, request *v1pb.ListAct for _, activity := range activities { activityMessage, err := s.convertActivityFromStore(ctx, activity) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to convert activity from store: %v", err) + // Skip activities that reference deleted memos instead of failing the entire list + continue + } + if activityMessage != nil { + activityMessages = append(activityMessages, activityMessage) } - activityMessages = append(activityMessages, activityMessage) } return &v1pb.ListActivitiesResponse{ @@ -61,16 +64,24 @@ func (s *APIV1Service) GetActivity(ctx context.Context, request *v1pb.GetActivit if err != nil { return nil, status.Errorf(codes.Internal, "failed to convert activity from store: %v", err) } + if activityMessage == nil { + return nil, status.Errorf(codes.NotFound, "activity references deleted content") + } return activityMessage, nil } // convertActivityFromStore converts a storage-layer activity to an API activity. // This handles the mapping between internal activity representation and the public API, // including proper type and level conversions. +// Returns nil if the activity references deleted content (to allow graceful skipping). func (s *APIV1Service) convertActivityFromStore(ctx context.Context, activity *store.Activity) (*v1pb.Activity, error) { payload, err := s.convertActivityPayloadFromStore(ctx, activity.Payload) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to convert activity payload from store: %v", err) + return nil, err + } + // Skip activities that reference deleted memos + if payload == nil { + return nil, nil } // Convert store activity type to proto enum @@ -103,6 +114,7 @@ func (s *APIV1Service) convertActivityFromStore(ctx context.Context, activity *s // convertActivityPayloadFromStore converts a storage-layer activity payload to an API payload. // This resolves references (e.g., memo IDs) to resource names for the API. +// Returns nil if the activity references deleted content (to allow graceful skipping). func (s *APIV1Service) convertActivityPayloadFromStore(ctx context.Context, payload *storepb.ActivityPayload) (*v1pb.ActivityPayload, error) { v2Payload := &v1pb.ActivityPayload{} if payload.MemoComment != nil { @@ -114,8 +126,9 @@ func (s *APIV1Service) convertActivityPayloadFromStore(ctx context.Context, payl if err != nil { return nil, status.Errorf(codes.Internal, "failed to get memo: %v", err) } + // If the comment memo was deleted, skip this activity gracefully if memo == nil { - return nil, status.Errorf(codes.NotFound, "memo does not exist") + return nil, nil } // Fetch the related memo (the one being commented on) @@ -126,6 +139,10 @@ func (s *APIV1Service) convertActivityPayloadFromStore(ctx context.Context, payl if err != nil { return nil, status.Errorf(codes.Internal, "failed to get related memo: %v", err) } + // If the related memo was deleted, skip this activity gracefully + if relatedMemo == nil { + return nil, nil + } v2Payload.Payload = &v1pb.ActivityPayload_MemoComment{ MemoComment: &v1pb.ActivityMemoCommentPayload{ diff --git a/server/router/api/v1/test/activity_deleted_memo_test.go b/server/router/api/v1/test/activity_deleted_memo_test.go new file mode 100644 index 000000000..c7b3b6614 --- /dev/null +++ b/server/router/api/v1/test/activity_deleted_memo_test.go @@ -0,0 +1,263 @@ +package test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + + apiv1 "github.com/usememos/memos/proto/gen/api/v1" + v1 "github.com/usememos/memos/server/router/api/v1" + "github.com/usememos/memos/store" +) + +// TestListActivitiesWithDeletedMemos verifies that ListActivities gracefully handles +// activities that reference deleted memos instead of crashing the entire request. +func TestListActivitiesWithDeletedMemos(t *testing.T) { + ctx := context.Background() + ts := NewTestService(t) + defer ts.Cleanup() + + // Create two users - one to create memo, one to comment + userOne, err := ts.CreateRegularUser(ctx, "test-user-1") + require.NoError(t, err) + userOneCtx := ts.CreateUserContext(ctx, userOne.ID) + + userTwo, err := ts.CreateRegularUser(ctx, "test-user-2") + require.NoError(t, err) + userTwoCtx := ts.CreateUserContext(ctx, userTwo.ID) + + // Create a memo by userOne + memo1, err := ts.Service.CreateMemo(userOneCtx, &apiv1.CreateMemoRequest{ + Memo: &apiv1.Memo{ + Content: "Original memo", + Visibility: apiv1.Visibility_PUBLIC, + }, + }) + require.NoError(t, err) + require.NotNil(t, memo1) + + // Create a comment on the memo by userTwo (this will create an activity for userOne) + comment, err := ts.Service.CreateMemoComment(userTwoCtx, &apiv1.CreateMemoCommentRequest{ + Name: memo1.Name, + Comment: &apiv1.Memo{ + Content: "This is a comment", + Visibility: apiv1.Visibility_PUBLIC, + }, + }) + require.NoError(t, err) + require.NotNil(t, comment) + + // Verify activity was created for the comment (check from userOne's perspective - they receive the notification) + activities, err := ts.Service.ListActivities(userOneCtx, &apiv1.ListActivitiesRequest{}) + require.NoError(t, err) + initialActivityCount := len(activities.Activities) + require.Greater(t, initialActivityCount, 0, "Should have at least one activity") + + // Delete the original memo (this deletes the comment too) + _, err = ts.Service.DeleteMemo(userOneCtx, &apiv1.DeleteMemoRequest{ + Name: memo1.Name, + }) + require.NoError(t, err) + + // List activities again - should succeed even though the memo is deleted + activities, err = ts.Service.ListActivities(userOneCtx, &apiv1.ListActivitiesRequest{}) + require.NoError(t, err) + // Activities list should be empty or not contain the deleted memo activity + for _, activity := range activities.Activities { + if activity.Payload != nil && activity.Payload.GetMemoComment() != nil { + require.NotEqual(t, memo1.Name, activity.Payload.GetMemoComment().Memo, + "Activity should not reference deleted memo") + } + } + // After deletion, there should be fewer activities + require.LessOrEqual(t, len(activities.Activities), initialActivityCount-1, + "Should have filtered out the activity for the deleted memo") +} + +// TestGetActivityWithDeletedMemo verifies that GetActivity returns a proper error +// when trying to fetch an activity that references a deleted memo. +func TestGetActivityWithDeletedMemo(t *testing.T) { + ctx := context.Background() + ts := NewTestService(t) + defer ts.Cleanup() + + // Create two users + userOne, err := ts.CreateRegularUser(ctx, "test-user-1") + require.NoError(t, err) + userOneCtx := ts.CreateUserContext(ctx, userOne.ID) + + userTwo, err := ts.CreateRegularUser(ctx, "test-user-2") + require.NoError(t, err) + userTwoCtx := ts.CreateUserContext(ctx, userTwo.ID) + + // Create a memo by userOne + memo1, err := ts.Service.CreateMemo(userOneCtx, &apiv1.CreateMemoRequest{ + Memo: &apiv1.Memo{ + Content: "Original memo", + Visibility: apiv1.Visibility_PUBLIC, + }, + }) + require.NoError(t, err) + require.NotNil(t, memo1) + + // Create a comment to trigger activity creation by userTwo + comment, err := ts.Service.CreateMemoComment(userTwoCtx, &apiv1.CreateMemoCommentRequest{ + Name: memo1.Name, + Comment: &apiv1.Memo{ + Content: "Comment", + Visibility: apiv1.Visibility_PUBLIC, + }, + }) + require.NoError(t, err) + require.NotNil(t, comment) + + // Get the activity ID by listing activities from userOne's perspective + activities, err := ts.Service.ListActivities(userOneCtx, &apiv1.ListActivitiesRequest{}) + require.NoError(t, err) + require.Greater(t, len(activities.Activities), 0) + + activityName := activities.Activities[0].Name + + // Delete the memo + _, err = ts.Service.DeleteMemo(userOneCtx, &apiv1.DeleteMemoRequest{ + Name: memo1.Name, + }) + require.NoError(t, err) + + // Try to get the specific activity - should return NotFound error + _, err = ts.Service.GetActivity(userOneCtx, &apiv1.GetActivityRequest{ + Name: activityName, + }) + require.Error(t, err) + require.Contains(t, err.Error(), "activity references deleted content") +} + +// TestActivitiesWithPartiallyDeletedMemos verifies that when some memos are deleted, +// other valid activities are still returned. +func TestActivitiesWithPartiallyDeletedMemos(t *testing.T) { + ctx := context.Background() + ts := NewTestService(t) + defer ts.Cleanup() + + // Create two users + userOne, err := ts.CreateRegularUser(ctx, "test-user-1") + require.NoError(t, err) + userOneCtx := ts.CreateUserContext(ctx, userOne.ID) + + userTwo, err := ts.CreateRegularUser(ctx, "test-user-2") + require.NoError(t, err) + userTwoCtx := ts.CreateUserContext(ctx, userTwo.ID) + + // Create two memos by userOne + memo1, err := ts.Service.CreateMemo(userOneCtx, &apiv1.CreateMemoRequest{ + Memo: &apiv1.Memo{ + Content: "First memo", + Visibility: apiv1.Visibility_PUBLIC, + }, + }) + require.NoError(t, err) + + memo2, err := ts.Service.CreateMemo(userOneCtx, &apiv1.CreateMemoRequest{ + Memo: &apiv1.Memo{ + Content: "Second memo", + Visibility: apiv1.Visibility_PUBLIC, + }, + }) + require.NoError(t, err) + + // Create comments on both by userTwo (creates activities for userOne) + _, err = ts.Service.CreateMemoComment(userTwoCtx, &apiv1.CreateMemoCommentRequest{ + Name: memo1.Name, + Comment: &apiv1.Memo{ + Content: "Comment on first", + Visibility: apiv1.Visibility_PUBLIC, + }, + }) + require.NoError(t, err) + + _, err = ts.Service.CreateMemoComment(userTwoCtx, &apiv1.CreateMemoCommentRequest{ + Name: memo2.Name, + Comment: &apiv1.Memo{ + Content: "Comment on second", + Visibility: apiv1.Visibility_PUBLIC, + }, + }) + require.NoError(t, err) + + // Should have 2 activities from userOne's perspective + activities, err := ts.Service.ListActivities(userOneCtx, &apiv1.ListActivitiesRequest{}) + require.NoError(t, err) + require.Equal(t, 2, len(activities.Activities)) + + // Delete first memo + _, err = ts.Service.DeleteMemo(userOneCtx, &apiv1.DeleteMemoRequest{ + Name: memo1.Name, + }) + require.NoError(t, err) + + // List activities - should still work and return only the second memo's activity + activities, err = ts.Service.ListActivities(userOneCtx, &apiv1.ListActivitiesRequest{}) + require.NoError(t, err) + require.Equal(t, 1, len(activities.Activities), "Should have 1 activity remaining") + + // Verify the remaining activity relates to a valid memo + require.NotNil(t, activities.Activities[0].Payload.GetMemoComment()) + require.Contains(t, activities.Activities[0].Payload.GetMemoComment().RelatedMemo, "memos/") +} + +// TestActivityStoreDirectDeletion tests the scenario where a memo is deleted directly +// from the store (simulating database-level deletion or migration). +func TestActivityStoreDirectDeletion(t *testing.T) { + ctx := context.Background() + ts := NewTestService(t) + defer ts.Cleanup() + + user, err := ts.CreateRegularUser(ctx, "test-user") + require.NoError(t, err) + userCtx := ts.CreateUserContext(ctx, user.ID) + + // Create a memo + memo1, err := ts.Service.CreateMemo(userCtx, &apiv1.CreateMemoRequest{ + Memo: &apiv1.Memo{ + Content: "Test memo", + Visibility: apiv1.Visibility_PUBLIC, + }, + }) + require.NoError(t, err) + + // Create a comment + comment, err := ts.Service.CreateMemoComment(userCtx, &apiv1.CreateMemoCommentRequest{ + Name: memo1.Name, + Comment: &apiv1.Memo{ + Content: "Test comment", + Visibility: apiv1.Visibility_PUBLIC, + }, + }) + require.NoError(t, err) + + // Extract memo UID from the comment name + commentMemoUID, err := v1.ExtractMemoUIDFromName(comment.Name) + require.NoError(t, err) + + commentMemo, err := ts.Store.GetMemo(ctx, &store.FindMemo{ + UID: &commentMemoUID, + }) + require.NoError(t, err) + require.NotNil(t, commentMemo) + + // Delete the comment memo directly from store (simulating orphaned activity) + err = ts.Store.DeleteMemo(ctx, &store.DeleteMemo{ID: commentMemo.ID}) + require.NoError(t, err) + + // List activities should still succeed even with orphaned activity + activities, err := ts.Service.ListActivities(userCtx, &apiv1.ListActivitiesRequest{}) + require.NoError(t, err) + // Activities should be empty or not include the orphaned one + for _, activity := range activities.Activities { + if activity.Payload != nil && activity.Payload.GetMemoComment() != nil { + require.NotEqual(t, comment.Name, activity.Payload.GetMemoComment().Memo, + "Should not return activity with deleted memo") + } + } +}