From 8e0693ab037968bacff80739ca9580ef3883e134 Mon Sep 17 00:00:00 2001 From: Jakub Date: Tue, 18 Jan 2022 18:23:00 +0100 Subject: [PATCH] GODT-1499: Remove message from DB once is not on server any more --- internal/store/cache.go | 15 ++++++++++ pkg/pmapi/errors.go | 4 +-- test/context/pmapi_controller.go | 1 + test/fakeapi/controller_control.go | 16 ++++++++++ test/fakeapi/messages.go | 2 +- test/features/imap/message/fetch.feature | 11 +++++++ test/liveapi/messages.go | 11 +++++++ test/liveapi/persistent_clients.go | 38 ++++++++++++++++++++++-- test/store_setup_test.go | 14 +++++++++ 9 files changed, 107 insertions(+), 5 deletions(-) diff --git a/internal/store/cache.go b/internal/store/cache.go index 9e1676f3..37da020b 100644 --- a/internal/store/cache.go +++ b/internal/store/cache.go @@ -23,6 +23,7 @@ import ( "github.com/ProtonMail/gopenpgp/v2/crypto" "github.com/ProtonMail/proton-bridge/internal/store/cache" "github.com/ProtonMail/proton-bridge/pkg/message" + "github.com/ProtonMail/proton-bridge/pkg/pmapi" bolt "go.etcd.io/bbolt" ) @@ -126,6 +127,7 @@ func (store *Store) getCachedMessage(messageID string) ([]byte, error) { literal, err := job.GetResult() if err != nil { + store.checkAndRemoveDeletedMessage(err, messageID) return nil, err } @@ -184,8 +186,21 @@ func (store *Store) BuildAndCacheMessage(ctx context.Context, messageID string) literal, err := job.GetResult() if err != nil { + store.checkAndRemoveDeletedMessage(err, messageID) return err } return store.cache.Set(store.user.ID(), messageID, literal) } + +func (store *Store) checkAndRemoveDeletedMessage(err error, msgID string) { + if _, ok := err.(pmapi.ErrUnprocessableEntity); !ok { + return + } + l := store.log.WithError(err).WithField("msgID", msgID) + l.Warn("Deleting message which was not found on API") + + if deleteErr := store.deleteMessageEvent(msgID); deleteErr != nil { + l.WithField("deleteErr", deleteErr).Error("Failed to delete non-existed API message from DB") + } +} diff --git a/pkg/pmapi/errors.go b/pkg/pmapi/errors.go index 3f8ff5db..beb6873d 100644 --- a/pkg/pmapi/errors.go +++ b/pkg/pmapi/errors.go @@ -32,9 +32,9 @@ var ( ) type ErrUnprocessableEntity struct { - originalError error + OriginalError error } func (err ErrUnprocessableEntity) Error() string { - return err.originalError.Error() + return err.OriginalError.Error() } diff --git a/test/context/pmapi_controller.go b/test/context/pmapi_controller.go index 24c4ec49..e3ff1dfe 100644 --- a/test/context/pmapi_controller.go +++ b/test/context/pmapi_controller.go @@ -45,6 +45,7 @@ type PMAPIController interface { GetCalls(method, path string) [][]byte LockEvents(username string) UnlockEvents(username string) + RemoveUserMessageWithoutEvent(username, messageID string) error } func newPMAPIController(listener listener.Listener) (PMAPIController, pmapi.Manager) { diff --git a/test/fakeapi/controller_control.go b/test/fakeapi/controller_control.go index 8935a753..1b7e35e5 100644 --- a/test/fakeapi/controller_control.go +++ b/test/fakeapi/controller_control.go @@ -234,3 +234,19 @@ func (ctl *Controller) LockEvents(string) {} // UnlockEvents doesn't needs to be implemented for fakeAPI. func (ctl *Controller) UnlockEvents(string) {} + +func (ctl *Controller) RemoveUserMessageWithoutEvent(username string, messageID string) error { + msgs, ok := ctl.messagesByUsername[username] + if !ok { + return nil + } + + for i, message := range msgs { + if message.ID == messageID { + ctl.messagesByUsername[username] = append(msgs[:i], msgs[i+1:]...) + return nil + } + } + + return errors.New("message not found") +} diff --git a/test/fakeapi/messages.go b/test/fakeapi/messages.go index 0cdf4d9c..348eae41 100644 --- a/test/fakeapi/messages.go +++ b/test/fakeapi/messages.go @@ -37,7 +37,7 @@ func (api *FakePMAPI) GetMessage(_ context.Context, apiID string) (*pmapi.Messag if msg := api.getMessage(apiID); msg != nil { return msg, nil } - return nil, fmt.Errorf("message %s not found", apiID) + return nil, pmapi.ErrUnprocessableEntity{OriginalError: fmt.Errorf("message %s not found", apiID)} } // ListMessages does not implement following filters: diff --git a/test/features/imap/message/fetch.feature b/test/features/imap/message/fetch.feature index 0acb62a9..c2fe1586 100644 --- a/test/features/imap/message/fetch.feature +++ b/test/features/imap/message/fetch.feature @@ -177,3 +177,14 @@ Feature: IMAP fetch messages # We had bug to incorectly set empty date, so let's make sure # there is no reference anywhere in the response. And IMAP response does not contain "\nDate: Thu, 01 Jan 1970" + + Scenario: Fetch of message which was deleted without event processed + Given there are 10 messages in mailbox "INBOX" for "user" + And message "5" was deleted forever without event processed for "user" + And there is IMAP client logged in as "user" + And there is IMAP client selected in "INBOX" + When IMAP client fetches bodies "1:*" + Then IMAP response is "NO" + When IMAP client fetches bodies "1:*" + Then IMAP response is "OK" + And IMAP response has 9 messages diff --git a/test/liveapi/messages.go b/test/liveapi/messages.go index 54aada32..0502ec5d 100644 --- a/test/liveapi/messages.go +++ b/test/liveapi/messages.go @@ -104,3 +104,14 @@ func (ctl *Controller) GetMessages(username, labelID string) ([]*pmapi.Message, return messages, nil } + +func (ctl *Controller) RemoveUserMessageWithoutEvent(username string, messageID string) error { + client, err := getPersistentClient(username) + if err != nil { + return err + } + + addMessageIDToSkipEventOnceDeleted(messageID) + + return client.DeleteMessages(context.Background(), []string{messageID}) +} diff --git a/test/liveapi/persistent_clients.go b/test/liveapi/persistent_clients.go index accc163e..35680068 100644 --- a/test/liveapi/persistent_clients.go +++ b/test/liveapi/persistent_clients.go @@ -48,7 +48,8 @@ var persistentClients = struct { byName map[string]clientAuthGetter saltByName map[string]string - eventsPaused sync.WaitGroup + eventsPaused sync.WaitGroup + skipDeletedMessageID map[string]struct{} }{} type persistentClient struct { @@ -79,7 +80,40 @@ func (pc *persistentClient) GetEvent(ctx context.Context, eventID string) (*pmap if !ok { return nil, errors.New("cannot convert to normal client") } - return normalClient.GetEvent(ctx, eventID) + + event, err := normalClient.GetEvent(ctx, eventID) + if err != nil { + return event, err + } + + return skipDeletedMessageIDs(event), nil +} + +func addMessageIDToSkipEventOnceDeleted(msgID string) { + if persistentClients.skipDeletedMessageID == nil { + persistentClients.skipDeletedMessageID = map[string]struct{}{} + } + persistentClients.skipDeletedMessageID[msgID] = struct{}{} +} + +func skipDeletedMessageIDs(event *pmapi.Event) *pmapi.Event { + if len(event.Messages) == 0 { + return event + } + + n := 0 + for i, m := range event.Messages { + if _, ok := persistentClients.skipDeletedMessageID[m.ID]; ok && m.Action == pmapi.EventDelete { + delete(persistentClients.skipDeletedMessageID, m.ID) + continue + } + + event.Messages[i] = m + n++ + } + event.Messages = event.Messages[:n] + + return event } func SetupPersistentClients() { diff --git a/test/store_setup_test.go b/test/store_setup_test.go index 0f8cc622..d1eb106e 100644 --- a/test/store_setup_test.go +++ b/test/store_setup_test.go @@ -38,6 +38,7 @@ func StoreSetupFeatureContext(s *godog.ScenarioContext) { s.Step(`^there are messages for "([^"]*)" as follows$`, thereAreSomeMessagesForUserAsFollows) s.Step(`^there are (\d+) messages in mailbox(?:es)? "([^"]*)" for address "([^"]*)" of "([^"]*)"$`, thereAreSomeMessagesInMailboxesForAddressOfUser) s.Step(`^wait for Sphinx to create duplication indices$`, waitForSphinx) + s.Step(`^message(?:s)? "([^"]*)" (?:was|were) deleted forever without event processed for "([^"]*)"$`, messageWasDeletedWithoutEvent) } func thereIsUserWithMailboxes(bddUserID string, mailboxes *godog.Table) error { @@ -319,3 +320,16 @@ func waitForSphinx() error { time.Sleep(15 * time.Second) return nil } + +func messageWasDeletedWithoutEvent(bddMessageID, bddUserID string) error { + account := ctx.GetTestAccount(bddUserID) + if account == nil { + return godog.ErrPending + } + apiID, err := ctx.GetAPIMessageID(account.Username(), bddMessageID) + if err != nil { + return internalError(err, "getting BDD message ID %s", bddMessageID) + } + + return ctx.GetPMAPIController().RemoveUserMessageWithoutEvent(account.Username(), apiID) +}