From 47e2fb785bc2be6e46e40fbba27629e608ee69b8 Mon Sep 17 00:00:00 2001 From: Jakub Date: Mon, 19 Sep 2022 09:16:28 +0200 Subject: [PATCH] fix(GODT-1616): close not sending unilateral updates. --- internal/state/responders.go | 18 ++++--- internal/state/state.go | 4 +- internal/state/updates_mailbox.go | 5 +- tests/close_test.go | 80 +++++++++++++++++++++---------- 4 files changed, 67 insertions(+), 40 deletions(-) diff --git a/internal/state/responders.go b/internal/state/responders.go index ce4af5a0..327d7c31 100644 --- a/internal/state/responders.go +++ b/internal/state/responders.go @@ -6,6 +6,7 @@ import ( "sync" "github.com/ProtonMail/gluon/imap" + "github.com/ProtonMail/gluon/internal/contexts" "github.com/ProtonMail/gluon/internal/db" "github.com/ProtonMail/gluon/internal/db/ent" "github.com/ProtonMail/gluon/internal/ids" @@ -52,7 +53,7 @@ func NewMessageIDAndMailboxIDResponderStateUpdate(messageID imap.InternalMessage type Responder interface { // handle generates responses in the context of the given snapshot. - handle(snap *snapshot, stateID StateID) ([]response.Response, responderDBUpdate, error) + handle(ctx context.Context, snap *snapshot, stateID StateID) ([]response.Response, responderDBUpdate, error) // getMessageID returns the message ID that this Responder targets. getMessageID() imap.InternalMessageID @@ -90,7 +91,7 @@ type targetedExists struct { targetStateID StateID } -func (u *targetedExists) handle(snap *snapshot, stateID StateID) ([]response.Response, responderDBUpdate, error) { +func (u *targetedExists) handle(_ context.Context, snap *snapshot, stateID StateID) ([]response.Response, responderDBUpdate, error) { if snap.hasMessage(u.resp.messageID.InternalID) { return nil, nil, nil } @@ -229,17 +230,15 @@ func (e *ExistsStateUpdate) String() string { type expunge struct { messageID imap.InternalMessageID - asClose bool } -func NewExpunge(messageID imap.InternalMessageID, asClose bool) *expunge { +func NewExpunge(messageID imap.InternalMessageID) *expunge { return &expunge{ messageID: messageID, - asClose: asClose, } } -func (u *expunge) handle(snap *snapshot, _ StateID) ([]response.Response, responderDBUpdate, error) { +func (u *expunge) handle(ctx context.Context, snap *snapshot, _ StateID) ([]response.Response, responderDBUpdate, error) { if !snap.hasMessage(u.messageID) { return nil, nil, nil } @@ -254,7 +253,7 @@ func (u *expunge) handle(snap *snapshot, _ StateID) ([]response.Response, respon } // When handling a CLOSE command, EXPUNGE responses are not sent. - if u.asClose { + if contexts.IsClose(ctx) { return nil, nil, nil } @@ -266,9 +265,8 @@ func (u *expunge) getMessageID() imap.InternalMessageID { } func (u *expunge) String() string { - return fmt.Sprintf("Expung: message = %v closed = %v", + return fmt.Sprintf("Expung: message = %v", u.messageID.ShortID(), - u.asClose, ) } @@ -299,7 +297,7 @@ func NewFetch(messageID imap.InternalMessageID, flags imap.FlagSet, asUID, asSil } } -func (u *fetch) handle(snap *snapshot, _ StateID) ([]response.Response, responderDBUpdate, error) { +func (u *fetch) handle(_ context.Context, snap *snapshot, _ StateID) ([]response.Response, responderDBUpdate, error) { if !snap.hasMessage(u.messageID) { return nil, nil, nil } diff --git a/internal/state/state.go b/internal/state/state.go index 58aee00b..5f6ddb55 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -532,7 +532,7 @@ func (state *State) flushResponses(ctx context.Context, permitExpunge bool) ([]r for _, responder := range state.popResponders(permitExpunge) { logrus.WithField("state", state.StateID).WithField("Origin", "Flush").Tracef("Applying responder: %v", responder.String()) - res, dbUpdate, err := responder.handle(state.snap, state.StateID) + res, dbUpdate, err := responder.handle(ctx, state.snap, state.StateID) if err != nil { return nil, err } @@ -567,7 +567,7 @@ func (state *State) PushResponder(ctx context.Context, tx *ent.Tx, responder ... for _, responder := range responder { logrus.WithField("state", state.StateID).WithField("Origin", "Push").Tracef("Applying responder: %v", responder.String()) - res, dbUpdate, err := responder.handle(state.snap, state.StateID) + res, dbUpdate, err := responder.handle(ctx, state.snap, state.StateID) if err != nil { return err } diff --git a/internal/state/updates_mailbox.go b/internal/state/updates_mailbox.go index 395bb089..cad9a7ab 100644 --- a/internal/state/updates_mailbox.go +++ b/internal/state/updates_mailbox.go @@ -4,7 +4,6 @@ import ( "context" "github.com/ProtonMail/gluon/imap" - "github.com/ProtonMail/gluon/internal/contexts" "github.com/ProtonMail/gluon/internal/db" "github.com/ProtonMail/gluon/internal/db/ent" "github.com/ProtonMail/gluon/internal/ids" @@ -46,7 +45,7 @@ func MoveMessagesFromMailbox( } for _, messageID := range messageIDs { - stateUpdates = append(stateUpdates, NewMessageIDAndMailboxIDResponderStateUpdate(messageID, mboxFromID, NewExpunge(messageID, contexts.IsClose(ctx)))) + stateUpdates = append(stateUpdates, NewMessageIDAndMailboxIDResponderStateUpdate(messageID, mboxFromID, NewExpunge(messageID))) } return messageUIDs, stateUpdates, nil @@ -80,7 +79,7 @@ func RemoveMessagesFromMailbox(ctx context.Context, tx *ent.Tx, mboxID imap.Inte } stateUpdates := xslices.Map(messageIDs, func(id imap.InternalMessageID) Update { - return NewMessageIDAndMailboxIDResponderStateUpdate(id, mboxID, NewExpunge(id, contexts.IsClose(ctx))) + return NewMessageIDAndMailboxIDResponderStateUpdate(id, mboxID, NewExpunge(id)) }) return stateUpdates, nil diff --git a/tests/close_test.go b/tests/close_test.go index 8a00a003..bf1d53d7 100644 --- a/tests/close_test.go +++ b/tests/close_test.go @@ -14,40 +14,70 @@ func TestClose(t *testing.T) { // This test is still useful as we have no way of checking the fetch responses after the store command. // Additionally, we also need to ensure that there are no unilateral EXPUNGE messages returned from the server after close. // There is currently no way to check for this with the go imap client. - runOneToOneTestWithAuth(t, defaultServerOptions(t), func(c *testConnection, _ *testSession) { - c.C("b001 CREATE saved-messages") - c.S("b001 OK CREATE") + runManyToOneTestWithAuth(t, defaultServerOptions(t), []int{1, 2}, func(c map[int]*testConnection, _ *testSession) { + c[1].C("b001 CREATE saved-messages") + c[1].S("b001 OK CREATE") - c.doAppend(`saved-messages`, `To: 1@pm.me`, `\Seen`).expect("OK") - c.doAppend(`saved-messages`, `To: 2@pm.me`).expect("OK") - c.doAppend(`saved-messages`, `To: 3@pm.me`, `\Seen`).expect("OK") - c.doAppend(`saved-messages`, `To: 4@pm.me`).expect("OK") - c.doAppend(`saved-messages`, `To: 5@pm.me`, `\Seen`).expect("OK") + c[1].doAppend(`saved-messages`, `To: 1@pm.me`, `\Seen`).expect("OK") + c[1].doAppend(`saved-messages`, `To: 2@pm.me`).expect("OK") + c[1].doAppend(`saved-messages`, `To: 3@pm.me`, `\Seen`).expect("OK") + c[1].doAppend(`saved-messages`, `To: 4@pm.me`).expect("OK") + c[1].doAppend(`saved-messages`, `To: 5@pm.me`, `\Seen`).expect("OK") - c.C(`A002 SELECT saved-messages`) - c.Se(`A002 OK [READ-WRITE] SELECT`) + c[1].C(`A002 SELECT saved-messages`) + c[1].Se(`A002 OK [READ-WRITE] SELECT`) + + c[2].C(`B001 SELECT saved-messages`) + c[2].Se(`B001 OK [READ-WRITE] SELECT`) + c[2].C(`B002 FETCH 1:* (UID FLAGS)`) + c[2].S( + `* 1 FETCH (UID 1 FLAGS (\Seen))`, + `* 2 FETCH (UID 2 FLAGS ())`, + `* 3 FETCH (UID 3 FLAGS (\Seen))`, + `* 4 FETCH (UID 4 FLAGS ())`, + `* 5 FETCH (UID 5 FLAGS (\Seen))`, + ) + c[2].OK("B002") // TODO: Match flags in any order. - c.C(`A003 STORE 1 +FLAGS (\Deleted)`) - c.S(`* 1 FETCH (FLAGS (\Deleted \Recent \Seen))`) - c.Sx("A003 OK.*") + c[1].C(`A003 STORE 1 +FLAGS (\Deleted)`) + c[1].S(`* 1 FETCH (FLAGS (\Deleted \Recent \Seen))`) + c[1].Sx("A003 OK.*") - c.C(`A004 STORE 2 +FLAGS (\Deleted)`) - c.S(`* 2 FETCH (FLAGS (\Deleted \Recent))`) - c.Sx("A004 OK.*") + c[1].C(`A004 STORE 2 +FLAGS (\Deleted)`) + c[1].S(`* 2 FETCH (FLAGS (\Deleted \Recent))`) + c[1].Sx("A004 OK.*") - c.C(`A005 STORE 3 +FLAGS (\Deleted)`) - c.S(`* 3 FETCH (FLAGS (\Deleted \Recent \Seen))`) - c.Sx("A005 OK.*") + c[1].C(`A005 STORE 3 +FLAGS (\Deleted)`) + c[1].S(`* 3 FETCH (FLAGS (\Deleted \Recent \Seen))`) + c[1].Sx("A005 OK.*") + + c[2].C("B003 NOOP") + c[2].S( + `* 1 FETCH (FLAGS (\Deleted \Seen))`, + `* 2 FETCH (FLAGS (\Deleted))`, + `* 3 FETCH (FLAGS (\Deleted \Seen))`, + ) + c[2].OK("B003") - // TODO: GOMSRV-106 - Ensure this also works for cases where multiple clients have the same mailbox open - c.C(`A202 CLOSE`) - c.S("A202 OK CLOSE") + c[2].C("B004 UID EXPUNGE 1") + c[2].Se(`* 1 EXPUNGE`) + c[2].OK("B004") + + c[1].C(`A202 CLOSE`) + c[1].S("A202 OK CLOSE") + + c[2].C("B003 NOOP") + c[2].S( + `* 1 EXPUNGE`, + `* 1 EXPUNGE`, + ) + c[2].OK("B003") // There are 2 messages in saved-messages. - c.C(`A006 STATUS saved-messages (MESSAGES)`) - c.S(`* STATUS "saved-messages" (MESSAGES 2)`) - c.S(`A006 OK STATUS`) + c[1].C(`A006 STATUS saved-messages (MESSAGES)`) + c[1].S(`* STATUS "saved-messages" (MESSAGES 2)`) + c[1].S(`A006 OK STATUS`) }) }