fix: Append of existing message should act as copy

The implementation was already there but some codepaths relied on the remote message ID being loaded, while the original implementation didn't do this. Now we always load the remote ID.
ProtonMail · Oct 24, 2022 · abc5ee9 · abc5ee9
1 parent eaf93a0
commit abc5ee9
Show file tree

Hide file tree

Showing 10 changed files with 95 additions and 30 deletions.
diff --git a/connector/dummy_simulate.go b/connector/dummy_simulate.go
@@ -16,7 +16,7 @@ func (conn *Dummy) SetFolderPrefix(pfx string) {
 	conn.pushUpdate(imap.NewMailboxCreated(mbox))
 }
 
-func (conn *Dummy) SetMailboxPrefix(pfx string) {
+func (conn *Dummy) SetLabelsPrefix(pfx string) {
 	defer conn.Flush()
 
 	conn.pfxLabel = pfx

diff --git a/connector/dummy_test.go b/connector/dummy_test.go
@@ -40,7 +40,7 @@ func TestDummyConnector_validateUpdate(t *testing.T) {
 	require.Error(t, conn.validateUpdate([]string{"Folders", "something"}, []string{"other"}))
 	require.Error(t, conn.validateUpdate([]string{"something"}, []string{"Folders", "other"}))
 
-	conn.SetMailboxPrefix("Labels")
+	conn.SetLabelsPrefix("Labels")
 	require.NoError(t, conn.validateUpdate([]string{"Labels", "something"}, []string{"Labels", "other"}))
 	require.NoError(t, conn.validateUpdate([]string{"Labels", "something"}, []string{"Labels", "other", "long"}))
 	require.Error(t, conn.validateUpdate([]string{"Labels", "something"}, []string{"other"}))

diff --git a/internal/db/message.go b/internal/db/message.go
@@ -585,6 +585,15 @@ func GetMessageIDFromRemoteID(ctx context.Context, client *ent.Client, id imap.M
 	return message.ID, nil
 }
 
+func GetMessageRemoteIDFromID(ctx context.Context, client *ent.Client, id imap.InternalMessageID) (imap.MessageID, error) {
+	message, err := client.Message.Query().Where(message.ID(id)).Select(message.FieldRemoteID).Only(ctx)
+	if err != nil {
+		return "", err
+	}
+
+	return message.RemoteID, nil
+}
+
 func NewFlagSet(msgUID *ent.UID, flags []*ent.MessageFlag) imap.FlagSet {
 	flagSet := imap.NewFlagSetFromSlice(xslices.Map(flags, func(flag *ent.MessageFlag) string {
 		return flag.Value

diff --git a/internal/ids/ids.go b/internal/ids/ids.go
@@ -46,13 +46,6 @@ func NewMessageIDPair(msg *ent.Message) MessageIDPair {
 	}
 }
 
-func NewMessageIDPairWithoutRemote(internalID imap.InternalMessageID) MessageIDPair {
-	return MessageIDPair{
-		InternalID: internalID,
-		RemoteID:   "",
-	}
-}
-
 func SplitMessageIDPairSlice(s []MessageIDPair) ([]imap.InternalMessageID, []imap.MessageID) {
 	l := len(s)
 

diff --git a/internal/state/mailbox.go b/internal/state/mailbox.go
@@ -166,7 +166,16 @@ func (m *Mailbox) Append(ctx context.Context, literal []byte, flags imap.FlagSet
 		}); err != nil || !exists {
 			logrus.WithError(err).Warn("The message has an unknown internal ID")
 		} else if res, err := db.WriteResult(ctx, m.state.db(), func(ctx context.Context, tx *ent.Tx) ([]db.UIDWithFlags, error) {
-			return m.state.actionAddMessagesToMailbox(ctx, tx, []ids.MessageIDPair{ids.NewMessageIDPairWithoutRemote(msgID)}, ids.NewMailboxIDPair(m.mbox), m.snap == m.state.snap)
+			remoteID, err := db.GetMessageRemoteIDFromID(ctx, tx.Client(), msgID)
+			if err != nil {
+				return nil, err
+			}
+
+			return m.state.actionAddMessagesToMailbox(ctx, tx,
+				[]ids.MessageIDPair{{InternalID: msgID, RemoteID: remoteID}},
+				ids.NewMailboxIDPair(m.mbox),
+				m.snap == m.state.snap,
+			)
 		}); err != nil {
 			return 0, err
 		} else {

diff --git a/internal/state/snapshot_messages_test.go b/internal/state/snapshot_messages_test.go
@@ -8,15 +8,16 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// nolint:govet
 func TestMessages(t *testing.T) {
 	msg := newMsgList(8)
 
-	msg.insert(ids.NewMessageIDPairWithoutRemote(1), 10, imap.NewFlagSet(imap.FlagSeen))
-	msg.insert(ids.NewMessageIDPairWithoutRemote(2), 20, imap.NewFlagSet(imap.FlagSeen))
-	msg.insert(ids.NewMessageIDPairWithoutRemote(3), 30, imap.NewFlagSet(imap.FlagSeen))
-	msg.insert(ids.NewMessageIDPairWithoutRemote(4), 40, imap.NewFlagSet(imap.FlagSeen))
-	msg.insert(ids.NewMessageIDPairWithoutRemote(5), 50, imap.NewFlagSet(imap.FlagSeen))
-	msg.insert(ids.NewMessageIDPairWithoutRemote(6), 60, imap.NewFlagSet(imap.FlagSeen))
+	msg.insert(ids.MessageIDPair{1, "1"}, 10, imap.NewFlagSet(imap.FlagSeen))
+	msg.insert(ids.MessageIDPair{2, "2"}, 20, imap.NewFlagSet(imap.FlagSeen))
+	msg.insert(ids.MessageIDPair{3, "3"}, 30, imap.NewFlagSet(imap.FlagSeen))
+	msg.insert(ids.MessageIDPair{4, "4"}, 40, imap.NewFlagSet(imap.FlagSeen))
+	msg.insert(ids.MessageIDPair{5, "5"}, 50, imap.NewFlagSet(imap.FlagSeen))
+	msg.insert(ids.MessageIDPair{6, "6"}, 60, imap.NewFlagSet(imap.FlagSeen))
 
 	msg.remove(2)
 	msg.remove(4)
@@ -66,15 +67,16 @@ func TestMessages(t *testing.T) {
 	}
 }
 
+// nolint:govet
 func TestMessageUIDRange(t *testing.T) {
 	msg := newMsgList(8)
 
-	msg.insert(ids.NewMessageIDPairWithoutRemote(1), 10, imap.NewFlagSet(imap.FlagSeen))
-	msg.insert(ids.NewMessageIDPairWithoutRemote(2), 20, imap.NewFlagSet(imap.FlagSeen))
-	msg.insert(ids.NewMessageIDPairWithoutRemote(3), 30, imap.NewFlagSet(imap.FlagSeen))
-	msg.insert(ids.NewMessageIDPairWithoutRemote(4), 40, imap.NewFlagSet(imap.FlagSeen))
-	msg.insert(ids.NewMessageIDPairWithoutRemote(5), 50, imap.NewFlagSet(imap.FlagSeen))
-	msg.insert(ids.NewMessageIDPairWithoutRemote(6), 60, imap.NewFlagSet(imap.FlagSeen))
+	msg.insert(ids.MessageIDPair{1, "1"}, 10, imap.NewFlagSet(imap.FlagSeen))
+	msg.insert(ids.MessageIDPair{2, "2"}, 20, imap.NewFlagSet(imap.FlagSeen))
+	msg.insert(ids.MessageIDPair{3, "3"}, 30, imap.NewFlagSet(imap.FlagSeen))
+	msg.insert(ids.MessageIDPair{4, "4"}, 40, imap.NewFlagSet(imap.FlagSeen))
+	msg.insert(ids.MessageIDPair{5, "5"}, 50, imap.NewFlagSet(imap.FlagSeen))
+	msg.insert(ids.MessageIDPair{6, "6"}, 60, imap.NewFlagSet(imap.FlagSeen))
 
 	// UIDRange Higher than maximum
 	{

diff --git a/tests/append_test.go b/tests/append_test.go
@@ -4,13 +4,13 @@ import (
 	"bytes"
 	"context"
 	"fmt"
-	"github.com/ProtonMail/gluon/connector"
-	"github.com/ProtonMail/gluon/imap"
 	"os"
 	"sync"
 	"testing"
 	"time"
 
+	"github.com/ProtonMail/gluon/connector"
+	"github.com/ProtonMail/gluon/imap"
 	goimap "github.com/emersion/go-imap"
 	uidplus "github.com/emersion/go-imap-uidplus"
 	"github.com/emersion/go-imap/client"

diff --git a/tests/path_config_test.go b/tests/path_config_test.go
@@ -5,7 +5,7 @@ import "testing"
 func TestPathConfig_ProtonPathConfig(t *testing.T) {
 	runOneToOneTestWithAuth(t, defaultServerOptions(t), func(c *testConnection, s *testSession) {
 		s.setFolderPrefix("user", "Folders")
-		s.setMailboxPrefix("user", "Labels")
+		s.setLabelsPrefix("user", "Labels")
 
 		c.C(`A001 CREATE Folders/TestFolder`)
 		c.Sx(`A001 OK`)
@@ -64,7 +64,7 @@ func TestPathConfig_ProtonPathConfig(t *testing.T) {
 func TestPathConfig_DotDelimiter(t *testing.T) {
 	runOneToOneTestWithAuth(t, defaultServerOptions(t, withDelimiter(".")), func(c *testConnection, s *testSession) {
 		s.setFolderPrefix("user", "Folders")
-		s.setMailboxPrefix("user", "Labels")
+		s.setLabelsPrefix("user", "Labels")
 
 		c.C(`A001 CREATE Folders.TestFolder`)
 		c.Sx(`A001 OK`)

diff --git a/tests/remote_test.go b/tests/remote_test.go
@@ -1,13 +1,20 @@
 package tests
 
 import (
+	"io"
+	"strconv"
 	"testing"
+	"time"
+
+	goimap "github.com/emersion/go-imap"
+	"github.com/emersion/go-imap/client"
+	"github.com/stretchr/testify/require"
 )
 
 func TestRemoteCopy(t *testing.T) {
 	runOneToOneTestWithAuth(t, defaultServerOptions(t), func(c *testConnection, s *testSession) {
 		s.setFolderPrefix("user", "Folders")
-		s.setMailboxPrefix("user", "Labels")
+		s.setLabelsPrefix("user", "Labels")
 
 		// Create two exclusive mailboxes, one with 100 messages.
 		s.mailboxCreated("user", []string{"Folders", "mbox1"}, "testdata/dovecot-crlf")
@@ -38,6 +45,51 @@ func TestRemoteCopy(t *testing.T) {
 	})
 }
 
+func TestAppendDuplicate(t *testing.T) {
+	const messagePath = "testdata/afternoon-meeting.eml"
+
+	runOneToOneTestClientWithAuth(t, defaultServerOptions(t), func(client *client.Client, s *testSession) {
+		s.setFolderPrefix("user", "Folders")
+		s.setLabelsPrefix("user", "Labels")
+
+		// Create exclusive mailboxes to be the origin and destination.
+		s.mailboxCreated("user", []string{"Folders", "origin"})
+		s.mailboxCreated("user", []string{"Folders", "destination"})
+
+		// Put the message in origin.
+		require.NoError(t, doAppendWithClientFromFile(t, client, "Folders/origin", messagePath, time.Now(), goimap.SeenFlag))
+
+		// Copy the message to destination.
+		status, err := client.Select("Folders/origin", false)
+		require.NoError(t, err)
+		require.Equal(t, uint32(1), status.Messages)
+
+		section, err := goimap.ParseBodySectionName("BODY[]")
+		require.NoError(t, err)
+
+		messages := newFetchCommand(t, client).withItems(section.FetchItem()).fetch(strconv.Itoa(1)).messages
+		require.Len(t, messages, 1)
+
+		body, err := io.ReadAll(messages[0].GetBody(section))
+		require.NoError(t, err)
+
+		require.NoError(t, doAppendWithClient(client, "Folders/destination", string(body), time.Now(), goimap.SeenFlag))
+
+		// Check that the message is in destination.
+		status, err = client.Select("Folders/destination", false)
+		require.NoError(t, err)
+		require.Equal(t, uint32(1), status.Messages)
+
+		// The folders are exclusive and so the remote will remove them automatically from the origin.
+		s.flush("user")
+
+		// Check that the message is no longer in origin.
+		status, err = client.Select("Folders/origin", false)
+		require.NoError(t, err)
+		require.Equal(t, uint32(0), status.Messages)
+	})
+}
+
 func TestRemoteDeletionPool(t *testing.T) {
 	runManyToOneTestWithAuth(t, defaultServerOptions(t), []int{1, 2}, func(c map[int]*testConnection, s *testSession) {
 		// Create a mailbox for the test to run in.

diff --git a/tests/session_test.go b/tests/session_test.go
@@ -26,7 +26,7 @@ type Connector interface {
 	connector.Connector
 
 	SetFolderPrefix(string)
-	SetMailboxPrefix(string)
+	SetLabelsPrefix(string)
 
 	MailboxCreated(imap.Mailbox) error
 	MailboxDeleted(imap.MailboxID) error
@@ -112,8 +112,8 @@ func (s *testSession) setFolderPrefix(user, prefix string) {
 	s.conns[s.userIDs[user]].SetFolderPrefix(prefix)
 }
 
-func (s *testSession) setMailboxPrefix(user, prefix string) {
-	s.conns[s.userIDs[user]].SetMailboxPrefix(prefix)
+func (s *testSession) setLabelsPrefix(user, prefix string) {
+	s.conns[s.userIDs[user]].SetLabelsPrefix(prefix)
 }
 
 func (s *testSession) mailboxCreated(user string, name []string, withData ...string) imap.MailboxID {