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.
ProtonMail · Jan 5, 2023 · 675bf5d · 675bf5d
1 parent b04fb94
commit 675bf5d
Show file tree

Hide file tree

Showing 8 changed files with 145 additions and 11 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -338,7 +338,7 @@ func (r *disableRemoveFromMailboxConnector) MoveMessages(
 	_ []imap.MessageID,
 	_ imap.MailboxID,
 	_ imap.MailboxID,
-) error {
+) (bool, error) {
 	panic("Should not be called")
 }