fix: gracefully handle deleted memos in activity service to prevent inbox crashes
This commit is contained in:
parent
a69056a1c2
commit
984d9b461b
2 changed files with 284 additions and 4 deletions
|
|
@ -34,9 +34,12 @@ func (s *APIV1Service) ListActivities(ctx context.Context, request *v1pb.ListAct
|
||||||
for _, activity := range activities {
|
for _, activity := range activities {
|
||||||
activityMessage, err := s.convertActivityFromStore(ctx, activity)
|
activityMessage, err := s.convertActivityFromStore(ctx, activity)
|
||||||
if err != nil {
|
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{
|
return &v1pb.ListActivitiesResponse{
|
||||||
|
|
@ -61,16 +64,24 @@ func (s *APIV1Service) GetActivity(ctx context.Context, request *v1pb.GetActivit
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, status.Errorf(codes.Internal, "failed to convert activity from store: %v", err)
|
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
|
return activityMessage, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// convertActivityFromStore converts a storage-layer activity to an API activity.
|
// convertActivityFromStore converts a storage-layer activity to an API activity.
|
||||||
// This handles the mapping between internal activity representation and the public API,
|
// This handles the mapping between internal activity representation and the public API,
|
||||||
// including proper type and level conversions.
|
// 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) {
|
func (s *APIV1Service) convertActivityFromStore(ctx context.Context, activity *store.Activity) (*v1pb.Activity, error) {
|
||||||
payload, err := s.convertActivityPayloadFromStore(ctx, activity.Payload)
|
payload, err := s.convertActivityPayloadFromStore(ctx, activity.Payload)
|
||||||
if err != nil {
|
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
|
// 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.
|
// convertActivityPayloadFromStore converts a storage-layer activity payload to an API payload.
|
||||||
// This resolves references (e.g., memo IDs) to resource names for the API.
|
// 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) {
|
func (s *APIV1Service) convertActivityPayloadFromStore(ctx context.Context, payload *storepb.ActivityPayload) (*v1pb.ActivityPayload, error) {
|
||||||
v2Payload := &v1pb.ActivityPayload{}
|
v2Payload := &v1pb.ActivityPayload{}
|
||||||
if payload.MemoComment != nil {
|
if payload.MemoComment != nil {
|
||||||
|
|
@ -114,8 +126,9 @@ func (s *APIV1Service) convertActivityPayloadFromStore(ctx context.Context, payl
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, status.Errorf(codes.Internal, "failed to get memo: %v", err)
|
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 {
|
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)
|
// Fetch the related memo (the one being commented on)
|
||||||
|
|
@ -126,6 +139,10 @@ func (s *APIV1Service) convertActivityPayloadFromStore(ctx context.Context, payl
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, status.Errorf(codes.Internal, "failed to get related memo: %v", err)
|
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{
|
v2Payload.Payload = &v1pb.ActivityPayload_MemoComment{
|
||||||
MemoComment: &v1pb.ActivityMemoCommentPayload{
|
MemoComment: &v1pb.ActivityMemoCommentPayload{
|
||||||
|
|
|
||||||
263
server/router/api/v1/test/activity_deleted_memo_test.go
Normal file
263
server/router/api/v1/test/activity_deleted_memo_test.go
Normal file
|
|
@ -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")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
Loading…
Reference in a new issue