From 675bf5daf3e65aafe5d84b319c1f19ef38c3f085 Mon Sep 17 00:00:00 2001 From: Leander Beernaert Date: Thu, 5 Jan 2023 10:20:55 +0100 Subject: [PATCH] feat(GODT-1817): Extend move to not expunge original messages Update connector interface to signal Gluon whether to expunge the original messages after a move command. This is required so that we can correctly support the concept of mail labels. --- connector/connector.go | 3 +- connector/dummy.go | 4 +- internal/backend/state_connector_impl.go | 2 +- internal/state/actions.go | 5 +- internal/state/connector.go | 2 +- internal/state/updates_mailbox.go | 9 +- tests/move_test.go | 129 +++++++++++++++++++++++ tests/recovery_mailbox_test.go | 2 +- 8 files changed, 145 insertions(+), 11 deletions(-) diff --git a/connector/connector.go b/connector/connector.go index db642a93..a858f3bd 100644 --- a/connector/connector.go +++ b/connector/connector.go @@ -34,7 +34,8 @@ type Connector interface { RemoveMessagesFromMailbox(ctx context.Context, messageIDs []imap.MessageID, mboxID imap.MailboxID) error // MoveMessages removes the given messages from one mailbox and adds them to the another mailbox. - MoveMessages(ctx context.Context, messageIDs []imap.MessageID, mboxFromID, mboxToID imap.MailboxID) error + // Returns true if the original messages should be removed from mboxFromID (e.g: Distinguishing between labels and folders). + MoveMessages(ctx context.Context, messageIDs []imap.MessageID, mboxFromID, mboxToID imap.MailboxID) (bool, error) // MarkMessagesSeen sets the seen value of the given messages. MarkMessagesSeen(ctx context.Context, messageIDs []imap.MessageID, seen bool) error diff --git a/connector/dummy.go b/connector/dummy.go index f280f2c9..23aeff71 100644 --- a/connector/dummy.go +++ b/connector/dummy.go @@ -206,7 +206,7 @@ func (conn *Dummy) RemoveMessagesFromMailbox(ctx context.Context, messageIDs []i return nil } -func (conn *Dummy) MoveMessages(ctx context.Context, messageIDs []imap.MessageID, mboxFromID, mboxToID imap.MailboxID) error { +func (conn *Dummy) MoveMessages(ctx context.Context, messageIDs []imap.MessageID, mboxFromID, mboxToID imap.MailboxID) (bool, error) { for _, messageID := range messageIDs { conn.state.removeMessageFromMailbox(messageID, mboxFromID) conn.state.addMessageToMailbox(messageID, mboxToID) @@ -219,7 +219,7 @@ func (conn *Dummy) MoveMessages(ctx context.Context, messageIDs []imap.MessageID )) } - return nil + return true, nil } func (conn *Dummy) MarkMessagesSeen(ctx context.Context, messageIDs []imap.MessageID, seen bool) error { diff --git a/internal/backend/state_connector_impl.go b/internal/backend/state_connector_impl.go index c49879dd..d854dbdf 100644 --- a/internal/backend/state_connector_impl.go +++ b/internal/backend/state_connector_impl.go @@ -100,7 +100,7 @@ func (sc *stateConnectorImpl) MoveMessagesFromMailbox( messageIDs []imap.MessageID, mboxFromID imap.MailboxID, mboxToID imap.MailboxID, -) error { +) (bool, error) { ctx = sc.newContextWithMetadata(ctx) return sc.connector.MoveMessages(ctx, messageIDs, mboxFromID, mboxToID) diff --git a/internal/state/actions.go b/internal/state/actions.go index 30139619..117a9313 100644 --- a/internal/state/actions.go +++ b/internal/state/actions.go @@ -532,11 +532,12 @@ func (state *State) actionMoveMessages( internalIDs, remoteIDs := ids.SplitMessageIDPairSlice(messagesIDsToMove) - if err := state.user.GetRemote().MoveMessagesFromMailbox(ctx, remoteIDs, mboxFromID.RemoteID, mboxToID.RemoteID); err != nil { + shouldRemoveOldMessages, err := state.user.GetRemote().MoveMessagesFromMailbox(ctx, remoteIDs, mboxFromID.RemoteID, mboxToID.RemoteID) + if err != nil { return nil, err } - messageUIDs, updates, err := MoveMessagesFromMailbox(ctx, tx, mboxFromID.InternalID, mboxToID.InternalID, internalIDs, state, state.imapLimits) + messageUIDs, updates, err := MoveMessagesFromMailbox(ctx, tx, mboxFromID.InternalID, mboxToID.InternalID, internalIDs, state, state.imapLimits, shouldRemoveOldMessages) if err != nil { return nil, err } diff --git a/internal/state/connector.go b/internal/state/connector.go index 228a0dd9..8c495398 100644 --- a/internal/state/connector.go +++ b/internal/state/connector.go @@ -61,7 +61,7 @@ type Connector interface { messageIDs []imap.MessageID, mboxFromID imap.MailboxID, mboxToID imap.MailboxID, - ) error + ) (bool, error) // SetMessagesSeen marks the message with the given ID as seen or unseen. SetMessagesSeen(ctx context.Context, messageIDs []imap.MessageID, seen bool) error diff --git a/internal/state/updates_mailbox.go b/internal/state/updates_mailbox.go index 19abff8c..f94a7818 100644 --- a/internal/state/updates_mailbox.go +++ b/internal/state/updates_mailbox.go @@ -21,6 +21,7 @@ func MoveMessagesFromMailbox( messageIDs []imap.InternalMessageID, s *State, imapLimits limits.IMAP, + removeOldMessages bool, ) ([]db.UIDWithFlags, []Update, error) { messageCount, uid, err := db.GetMailboxMessageCountAndUID(ctx, tx.Client(), mboxToID) if err != nil { @@ -35,7 +36,7 @@ func MoveMessagesFromMailbox( return nil, nil, err } - if mboxFromID != mboxToID { + if mboxFromID != mboxToID && removeOldMessages { if err := db.RemoveMessagesFromMailbox(ctx, tx, messageIDs, mboxFromID); err != nil { return nil, nil, err } @@ -57,8 +58,10 @@ func MoveMessagesFromMailbox( stateUpdates = append(stateUpdates, newExistsStateUpdateWithExists(mboxToID, responders, s)) } - for _, messageID := range messageIDs { - stateUpdates = append(stateUpdates, NewMessageIDAndMailboxIDResponderStateUpdate(messageID, mboxFromID, NewExpunge(messageID))) + if removeOldMessages { + for _, messageID := range messageIDs { + stateUpdates = append(stateUpdates, NewMessageIDAndMailboxIDResponderStateUpdate(messageID, mboxFromID, NewExpunge(messageID))) + } } return messageUIDs, stateUpdates, nil diff --git a/tests/move_test.go b/tests/move_test.go index 31efe1a9..29f1489d 100644 --- a/tests/move_test.go +++ b/tests/move_test.go @@ -1,8 +1,14 @@ package tests import ( + "context" "fmt" + "github.com/ProtonMail/gluon/connector" + goimap "github.com/emersion/go-imap" + "github.com/emersion/go-imap/client" + "github.com/stretchr/testify/require" "testing" + "time" "github.com/ProtonMail/gluon/imap" ) @@ -214,3 +220,126 @@ func TestConcurrency(t *testing.T) { } }) } + +// disableRemoveFromMailboxConnector fails the first append and panics if move or remove takes place on the +// connector. +type simulateLabelConnector struct { + *connector.Dummy + mboxID imap.MailboxID +} + +func (r *simulateLabelConnector) CreateMailbox(ctx context.Context, name []string) (imap.Mailbox, error) { + mbox, err := r.Dummy.CreateMailbox(ctx, name) + if err != nil { + return mbox, err + } + + if len(r.mboxID) == 0 { + r.mboxID = mbox.ID + } + + return mbox, nil +} + +func (r *simulateLabelConnector) MoveMessages( + ctx context.Context, + ids []imap.MessageID, + from imap.MailboxID, + to imap.MailboxID, +) (bool, error) { + if _, err := r.Dummy.MoveMessages(ctx, ids, from, to); err != nil { + return false, err + } + + return to != r.mboxID, nil +} + +type simulateLabelConnectorBuilder struct{} + +func (simulateLabelConnectorBuilder) New(usernames []string, password []byte, period time.Duration, flags, permFlags, attrs imap.FlagSet) Connector { + return &simulateLabelConnector{ + Dummy: connector.NewDummy(usernames, password, period, flags, permFlags, attrs), + } +} + +func TestMoveLabelBehavior(t *testing.T) { + runOneToOneTestClientWithAuth(t, defaultServerOptions(t, withConnectorBuilder(&simulateLabelConnectorBuilder{})), func(client *client.Client, _ *testSession) { + require.NoError(t, doAppendWithClient(client, "inbox", "To: Foo@foo.com", time.Now())) + require.NoError(t, doAppendWithClient(client, "inbox", "To: Bar@foo.com", time.Now())) + require.NoError(t, doAppendWithClient(client, "inbox", "To: Z@foo.com", time.Now())) + + require.NoError(t, client.Create("mylabel")) + + // Move message to label + { + status, err := client.Select("INBOX", false) + require.NoError(t, err) + require.Equal(t, uint32(3), status.Messages) + + // Move one message to label + require.NoError(t, client.Move(createSeqSet("1"), "mylabel")) + + // Inbox should have 3 messages + status, err = client.Status("INBOX", []goimap.StatusItem{goimap.StatusMessages}) + require.NoError(t, err) + require.Equal(t, uint32(3), status.Messages) + + // Check all messages are still present + newFetchCommand(t, client).withItems("ENVELOPE").fetch("1:3"). + forSeqNum(1, func(builder *validatorBuilder) { + builder.ignoreFlags() + builder.wantEnvelope(func(builder *envelopeValidatorBuilder) { + builder.wantTo("Foo@foo.com") + }) + }). + forSeqNum(2, func(builder *validatorBuilder) { + builder.ignoreFlags() + builder.wantEnvelope(func(builder *envelopeValidatorBuilder) { + builder.wantTo("Bar@foo.com") + }) + }). + forSeqNum(3, func(builder *validatorBuilder) { + builder.ignoreFlags() + builder.wantEnvelope(func(builder *envelopeValidatorBuilder) { + builder.wantTo("Z@foo.com") + }) + }). + checkAndRequireMessageCount(3) + + // Label should have 1 message + status, err = client.Status("mylabel", []goimap.StatusItem{goimap.StatusMessages}) + require.NoError(t, err) + require.Equal(t, uint32(1), status.Messages) + + } + + // Move message to inbox from label + { + status, err := client.Select("mylabel", false) + require.NoError(t, err) + require.Equal(t, uint32(1), status.Messages) + + // Check it has the right message + newFetchCommand(t, client).withItems("ENVELOPE").fetch("1").forSeqNum(1, func(builder *validatorBuilder) { + builder.ignoreFlags() + builder.wantEnvelope(func(builder *envelopeValidatorBuilder) { + builder.wantTo("Foo@foo.com") + }) + }).checkAndRequireMessageCount(1) + + // Move one message to label + require.NoError(t, client.Move(createSeqSet("1"), "INBOX")) + + // Inbox should have 3 messages + status, err = client.Status("INBOX", []goimap.StatusItem{goimap.StatusMessages}) + require.NoError(t, err) + require.Equal(t, uint32(3), status.Messages) + + // Label should have 1 message + status, err = client.Status("mylabel", []goimap.StatusItem{goimap.StatusMessages}) + require.NoError(t, err) + require.Equal(t, uint32(0), status.Messages) + } + + }) +} diff --git a/tests/recovery_mailbox_test.go b/tests/recovery_mailbox_test.go index 651367cd..1494dad6 100644 --- a/tests/recovery_mailbox_test.go +++ b/tests/recovery_mailbox_test.go @@ -338,7 +338,7 @@ func (r *disableRemoveFromMailboxConnector) MoveMessages( _ []imap.MessageID, _ imap.MailboxID, _ imap.MailboxID, -) error { +) (bool, error) { panic("Should not be called") }