From 0baac3dc492dc08c937be94b22a72fa3308acf68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Burak=20Kaan=20K=C3=B6se?= Date: Wed, 21 Aug 2024 13:15:50 +0200 Subject: [PATCH] Addressing some Outlook sending issues due to API delay. --- Wino.Core.Domain/Interfaces/IRequestBundle.cs | 2 +- .../Models/Requests/RequestBase.cs | 7 +- Wino.Core/Requests/MarkFolderAsReadRequest.cs | 2 - Wino.Core/Requests/MoveRequest.cs | 2 + Wino.Core/Requests/SendDraftRequest.cs | 6 +- Wino.Core/Services/AccountService.cs | 1 - Wino.Core/Services/FolderService.cs | 101 ++++++++++++------ Wino.Core/Services/MailService.cs | 8 +- Wino.Core/Synchronizers/BaseSynchronizer.cs | 10 +- .../Synchronizers/OutlookSynchronizer.cs | 75 +++++++------ 10 files changed, 133 insertions(+), 81 deletions(-) diff --git a/Wino.Core.Domain/Interfaces/IRequestBundle.cs b/Wino.Core.Domain/Interfaces/IRequestBundle.cs index 3ad5b4db..407f201d 100644 --- a/Wino.Core.Domain/Interfaces/IRequestBundle.cs +++ b/Wino.Core.Domain/Interfaces/IRequestBundle.cs @@ -48,7 +48,7 @@ namespace Wino.Core.Domain.Interfaces /// We add small delay for the following synchronization after executing current requests to overcome this issue. /// Default is false. /// - bool DelayExecution { get; } + int ResynchronizationDelay { get; } } public interface IRequest : IRequestBase diff --git a/Wino.Core.Domain/Models/Requests/RequestBase.cs b/Wino.Core.Domain/Models/Requests/RequestBase.cs index bb2281cb..b3f5fe36 100644 --- a/Wino.Core.Domain/Models/Requests/RequestBase.cs +++ b/Wino.Core.Domain/Models/Requests/RequestBase.cs @@ -12,7 +12,7 @@ namespace Wino.Core.Domain.Models.Requests public abstract void ApplyUIChanges(); public abstract void RevertUIChanges(); - public virtual bool DelayExecution => false; + public virtual int ResynchronizationDelay => 0; } public abstract record FolderRequestBase(MailItemFolder Folder, MailSynchronizerOperation Operation) : IFolderRequest @@ -20,7 +20,7 @@ namespace Wino.Core.Domain.Models.Requests public abstract void ApplyUIChanges(); public abstract void RevertUIChanges(); - public virtual bool DelayExecution => false; + public virtual int ResynchronizationDelay => 0; } public abstract record BatchRequestBase(IEnumerable Items, MailSynchronizerOperation Operation) : IBatchChangeRequest @@ -28,6 +28,7 @@ namespace Wino.Core.Domain.Models.Requests public abstract void ApplyUIChanges(); public abstract void RevertUIChanges(); - public virtual bool DelayExecution => false; + public virtual int ResynchronizationDelay => 0; + public virtual bool ExecuteSerialBatch => false; } } diff --git a/Wino.Core/Requests/MarkFolderAsReadRequest.cs b/Wino.Core/Requests/MarkFolderAsReadRequest.cs index 213a38b8..1ae8db46 100644 --- a/Wino.Core/Requests/MarkFolderAsReadRequest.cs +++ b/Wino.Core/Requests/MarkFolderAsReadRequest.cs @@ -31,8 +31,6 @@ namespace Wino.Core.Requests } } - public override bool DelayExecution => false; - public List SynchronizationFolderIds => [Folder.Id]; } } diff --git a/Wino.Core/Requests/MoveRequest.cs b/Wino.Core/Requests/MoveRequest.cs index 981d10ae..5abd0217 100644 --- a/Wino.Core/Requests/MoveRequest.cs +++ b/Wino.Core/Requests/MoveRequest.cs @@ -42,5 +42,7 @@ namespace Wino.Core.Requests { Items.ForEach(item => WeakReferenceMessenger.Default.Send(new MailAddedMessage(item.Item))); } + + public override int ResynchronizationDelay => 3000; } } diff --git a/Wino.Core/Requests/SendDraftRequest.cs b/Wino.Core/Requests/SendDraftRequest.cs index 1e62fe78..eb1a2cc5 100644 --- a/Wino.Core/Requests/SendDraftRequest.cs +++ b/Wino.Core/Requests/SendDraftRequest.cs @@ -45,7 +45,8 @@ namespace Wino.Core.Requests } [EditorBrowsable(EditorBrowsableState.Never)] - public record BatchSendDraftRequestRequest(IEnumerable Items, SendDraftPreparationRequest Request) : BatchRequestBase(Items, MailSynchronizerOperation.Send) + public record BatchSendDraftRequestRequest(IEnumerable Items, + SendDraftPreparationRequest Request) : BatchRequestBase(Items, MailSynchronizerOperation.Send) { public override void ApplyUIChanges() { @@ -57,6 +58,7 @@ namespace Wino.Core.Requests Items.ForEach(item => WeakReferenceMessenger.Default.Send(new MailAddedMessage(item.Item))); } - public override bool DelayExecution => true; + public override int ResynchronizationDelay => 10000; + public override bool ExecuteSerialBatch => true; } } diff --git a/Wino.Core/Services/AccountService.cs b/Wino.Core/Services/AccountService.cs index e4e504e8..14a10023 100644 --- a/Wino.Core/Services/AccountService.cs +++ b/Wino.Core/Services/AccountService.cs @@ -531,7 +531,6 @@ namespace Wino.Core.Services if (shouldUpdateIdentifier) { - _logger.Debug("Updating synchronization identifier for {Name}. From: {SynchronizationDeltaIdentifier} To: {NewIdentifier}", account.Name, account.SynchronizationDeltaIdentifier, newIdentifier); account.SynchronizationDeltaIdentifier = newIdentifier; await UpdateAccountAsync(account); diff --git a/Wino.Core/Services/FolderService.cs b/Wino.Core/Services/FolderService.cs index 87b2a6cf..2573f0a2 100644 --- a/Wino.Core/Services/FolderService.cs +++ b/Wino.Core/Services/FolderService.cs @@ -558,36 +558,11 @@ namespace Wino.Core.Services public Task> GetMailFolderPairMetadatasAsync(string mailCopyId) => GetMailFolderPairMetadatasAsync(new List() { mailCopyId }); - public async Task> GetSynchronizationFoldersAsync(SynchronizationOptions options) { var folders = new List(); - if (options.Type == SynchronizationType.Inbox) - { - var inboxFolder = await GetSpecialFolderByAccountIdAsync(options.AccountId, SpecialFolderType.Inbox); - var sentFolder = await GetSpecialFolderByAccountIdAsync(options.AccountId, SpecialFolderType.Sent); - var draftFolder = await GetSpecialFolderByAccountIdAsync(options.AccountId, SpecialFolderType.Draft); - - // For properly creating threads we need Sent and Draft to be synchronized as well. - - if (sentFolder != null && sentFolder.IsSynchronizationEnabled) - { - folders.Add(sentFolder); - } - - if (draftFolder != null && draftFolder.IsSynchronizationEnabled) - { - folders.Add(draftFolder); - } - - // User might've disabled inbox synchronization somehow... - if (inboxFolder != null && inboxFolder.IsSynchronizationEnabled) - { - folders.Add(inboxFolder); - } - } - else if (options.Type == SynchronizationType.Full) + if (options.Type == SynchronizationType.Full) { // Only get sync enabled folders. @@ -598,19 +573,75 @@ namespace Wino.Core.Services folders.AddRange(synchronizationFolders); } - else if (options.Type == SynchronizationType.Custom) + else { - // Only get the specified and enabled folders. + // Inbox, Sent and Draft folders must always be synchronized regardless of whether they are enabled or not. + // Custom folder sync will add additional folders to the list if not specified. - var synchronizationFolders = await Connection.Table() - .Where(a => a.MailAccountId == options.AccountId && a.IsSynchronizationEnabled && options.SynchronizationFolderIds.Contains(a.Id)) - .ToListAsync(); + var mustHaveFolders = await GetInboxSynchronizationFoldersAsync(options.AccountId); - // Order is important for moving. - // By implementation, removing mail folders must be synchronized first. Requests are made in that order for custom sync. - // eg. Moving item from Folder A to Folder B. If we start syncing Folder B first, we might miss adding assignment for Folder A. + if (options.Type == SynchronizationType.Inbox) + { + return mustHaveFolders; + } + else if (options.Type == SynchronizationType.Custom) + { + // Only get the specified and enabled folders. - folders.AddRange(synchronizationFolders.OrderBy(a => options.SynchronizationFolderIds.IndexOf(a.Id))); + var synchronizationFolders = await Connection.Table() + .Where(a => a.MailAccountId == options.AccountId && options.SynchronizationFolderIds.Contains(a.Id)) + .ToListAsync(); + + // Order is important for moving. + // By implementation, removing mail folders must be synchronized first. Requests are made in that order for custom sync. + // eg. Moving item from Folder A to Folder B. If we start syncing Folder B first, we might miss adding assignment for Folder A. + + var orderedCustomFolders = synchronizationFolders.OrderBy(a => options.SynchronizationFolderIds.IndexOf(a.Id)); + + foreach (var item in orderedCustomFolders) + { + if (!mustHaveFolders.Any(a => a.Id == item.Id)) + { + mustHaveFolders.Add(item); + } + } + } + + return mustHaveFolders; + } + + return folders; + } + + private async Task> GetInboxSynchronizationFoldersAsync(Guid accountId) + { + var folders = new List(); + + var inboxFolder = await GetSpecialFolderByAccountIdAsync(accountId, SpecialFolderType.Inbox); + var sentFolder = await GetSpecialFolderByAccountIdAsync(accountId, SpecialFolderType.Sent); + var draftFolder = await GetSpecialFolderByAccountIdAsync(accountId, SpecialFolderType.Draft); + var deletedFolder = await GetSpecialFolderByAccountIdAsync(accountId, SpecialFolderType.Deleted); + + if (deletedFolder != null) + { + folders.Add(deletedFolder); + } + + if (inboxFolder != null) + { + folders.Add(inboxFolder); + } + + // For properly creating threads we need Sent and Draft to be synchronized as well. + + if (sentFolder != null) + { + folders.Add(sentFolder); + } + + if (draftFolder != null) + { + folders.Add(draftFolder); } return folders; diff --git a/Wino.Core/Services/MailService.cs b/Wino.Core/Services/MailService.cs index 98e1101d..e2f4ed0c 100644 --- a/Wino.Core/Services/MailService.cs +++ b/Wino.Core/Services/MailService.cs @@ -395,7 +395,7 @@ namespace Wino.Core.Services return; } - _logger.Debug("Inserting mail {MailCopyId} to Folder {FolderId}", mailCopy.Id, mailCopy.FolderId); + _logger.Debug("Inserting mail {MailCopyId} to {FolderName}", mailCopy.Id, mailCopy.AssignedFolder.FolderName); await Connection.InsertAsync(mailCopy).ConfigureAwait(false); @@ -427,7 +427,7 @@ namespace Wino.Core.Services return; } - _logger.Debug("Deleting mail {Id} with Folder {FolderId}", mailCopy.Id, mailCopy.FolderId); + _logger.Debug("Deleting mail {Id} from folder {FolderName}", mailCopy.Id, mailCopy.AssignedFolder.FolderName); await Connection.DeleteAsync(mailCopy).ConfigureAwait(false); @@ -473,6 +473,8 @@ namespace Wino.Core.Services public Task ChangeReadStatusAsync(string mailCopyId, bool isRead) => UpdateAllMailCopiesAsync(mailCopyId, (item) => { + if (item.IsRead == isRead) return false; + item.IsRead = isRead; return true; @@ -481,6 +483,8 @@ namespace Wino.Core.Services public Task ChangeFlagStatusAsync(string mailCopyId, bool isFlagged) => UpdateAllMailCopiesAsync(mailCopyId, (item) => { + if (item.IsFlagged == isFlagged) return false; + item.IsFlagged = isFlagged; return true; diff --git a/Wino.Core/Synchronizers/BaseSynchronizer.cs b/Wino.Core/Synchronizers/BaseSynchronizer.cs index 39d53f66..b0f39765 100644 --- a/Wino.Core/Synchronizers/BaseSynchronizer.cs +++ b/Wino.Core/Synchronizers/BaseSynchronizer.cs @@ -206,12 +206,18 @@ namespace Wino.Core.Synchronizers } // Let servers to finish their job. Sometimes the servers doesn't respond immediately. + // Bug: if Outlook can't create the message in Sent Items folder before this delay, + // message will not appear in user's inbox since it's not in the Sent Items folder. - bool shouldDelayExecution = batches.Any(a => a.DelayExecution); + bool shouldDelayExecution = + (Account.ProviderType == MailProviderType.Outlook || Account.ProviderType == MailProviderType.Office365) + && batches.Any(a => a.ResynchronizationDelay > 0); if (shouldDelayExecution) { - await Task.Delay(2000); + var maxDelay = batches.Aggregate(0, (max, next) => Math.Max(max, next.ResynchronizationDelay)); + + await Task.Delay(maxDelay); } // Start the internal synchronization. diff --git a/Wino.Core/Synchronizers/OutlookSynchronizer.cs b/Wino.Core/Synchronizers/OutlookSynchronizer.cs index ad612dee..990430c8 100644 --- a/Wino.Core/Synchronizers/OutlookSynchronizer.cs +++ b/Wino.Core/Synchronizers/OutlookSynchronizer.cs @@ -5,6 +5,7 @@ using System.IO; using System.Linq; using System.Net; using System.Text; +using System.Text.Json; using System.Text.Json.Nodes; using System.Text.RegularExpressions; using System.Threading; @@ -145,8 +146,7 @@ namespace Wino.Core.Synchronizers { var synchronizationFolders = await _outlookChangeProcessor.GetSynchronizationFoldersAsync(options).ConfigureAwait(false); - _logger.Information("Found {Count} folders to synchronize.", synchronizationFolders.Count); - _logger.Information(string.Format("Folders: {0}", string.Join(",", synchronizationFolders.Select(a => a.FolderName)))); + _logger.Information(string.Format("{1} Folders: {0}", string.Join(",", synchronizationFolders.Select(a => a.FolderName)), synchronizationFolders.Count)); for (int i = 0; i < synchronizationFolders.Count; i++) { @@ -184,8 +184,6 @@ namespace Wino.Core.Synchronizers { var downloadedMessageIds = new List(); - _logger.Debug("Started synchronization for folder {FolderName}", folder.FolderName); - cancellationToken.ThrowIfCancellationRequested(); string latestDeltaLink = string.Empty; @@ -194,6 +192,8 @@ namespace Wino.Core.Synchronizers Microsoft.Graph.Me.MailFolders.Item.Messages.Delta.DeltaGetResponse messageCollectionPage = null; + _logger.Debug("Synchronizing {FolderName}", folder.FolderName); + if (isInitialSync) { _logger.Debug("No sync identifier for Folder {FolderName}. Performing initial sync.", folder.FolderName); @@ -211,9 +211,6 @@ namespace Wino.Core.Synchronizers { var currentDeltaToken = folder.DeltaToken; - _logger.Debug("Sync identifier found for Folder {FolderName}. Performing delta sync.", folder.FolderName); - _logger.Debug("Current delta token: {CurrentDeltaToken}", currentDeltaToken); - var requestInformation = _graphClient.Me.MailFolders[folder.RemoteFolderId].Messages.Delta.ToGetRequestInformation((config) => { config.QueryParameters.Top = (int)InitialMessageDownloadCountPerFolder; @@ -257,9 +254,6 @@ namespace Wino.Core.Synchronizers _logger.Debug("Downloaded {Count} messages for folder {FolderName}", downloadedMessageIds.Count, folder.FolderName); } - _logger.Debug("Iterator completed for folder {FolderName}", folder.FolderName); - _logger.Debug("Extracted latest delta link is {LatestDeltaLink}", latestDeltaLink); - //Store delta link for tracking new changes. if (!string.IsNullOrEmpty(latestDeltaLink)) { @@ -507,6 +501,27 @@ namespace Wino.Core.Synchronizers return new ProfileInformation(senderName, profilePictureData); } + /// + /// POST requests are handled differently in batches in Graph SDK. + /// Batch basically ignores the step's coontent-type and body. + /// Manually create a POST request with empty body and send it. + /// + /// Post request information. + /// Content object to serialize. + /// Updated post request information. + private RequestInformation PreparePostRequestInformation(RequestInformation requestInformation, object content = null) + { + requestInformation.Headers.Clear(); + + string contentJson = content == null ? "{}" : JsonSerializer.Serialize(content); + + requestInformation.Content = new MemoryStream(Encoding.UTF8.GetBytes(contentJson)); + requestInformation.HttpMethod = Method.POST; + requestInformation.Headers.Add("Content-Type", "application/json"); + + return requestInformation; + } + #region Mail Integration public override bool DelaySendOperationSynchronization() => true; @@ -520,7 +535,8 @@ namespace Wino.Core.Synchronizers return CreateBatchedHttpBundle(request, (item) => { - return _graphClient.Me.Messages[item.Item.Id.ToString()].Move.ToPostRequestInformation(requestBody); + return PreparePostRequestInformation(_graphClient.Me.Messages[item.Item.Id.ToString()].Move.ToPostRequestInformation(requestBody), + requestBody); }); } @@ -665,20 +681,8 @@ namespace Wino.Core.Synchronizers // Send draft. - // POST requests are handled differently in batches in Graph SDK. - // Batch basically ignores the step's coontent-type and body. - // Manually create a POST request with empty body and send it. - var sendDraftRequest = _graphClient.Me.Messages[mailCopyId].Send.ToPostRequestInformation((config) => - { - config.Headers.Add("Content-Type", "application/json"); - }); - - sendDraftRequest.Headers.Clear(); - - sendDraftRequest.Content = new MemoryStream(Encoding.UTF8.GetBytes("{}")); - sendDraftRequest.HttpMethod = Method.POST; - sendDraftRequest.Headers.Add("Content-Type", "application/json"); + var sendDraftRequest = PreparePostRequestInformation(_graphClient.Me.Messages[mailCopyId].Send.ToPostRequestInformation()); var sendDraftRequestBundle = new HttpRequestBundle(sendDraftRequest, request); @@ -724,6 +728,8 @@ namespace Wino.Core.Synchronizers { var batchRequestInformations = BatchExtension.Batch(batchedRequests, (int)MaximumAllowedBatchRequestSize); + bool serializeRequests = false; + foreach (var batch in batchRequestInformations) { var batchContent = new BatchRequestContentCollection(_graphClient); @@ -734,6 +740,14 @@ namespace Wino.Core.Synchronizers { var bundle = batch.ElementAt(i); + if (bundle.Request is BatchRequestBase batchBundleRequest && batchBundleRequest.ExecuteSerialBatch) + { + // This bundle needs to run every request in serial. + // By default requests are executed in parallel. + + serializeRequests = true; + } + var request = bundle.Request; var nativeRequest = bundle.NativeRequest; @@ -753,7 +767,7 @@ namespace Wino.Core.Synchronizers // Set execution type to serial instead of parallel if needed. // Each step will depend on the previous one. - if (itemCount > 1) + if (serializeRequests) { for (int i = 1; i < itemCount; i++) { @@ -762,7 +776,6 @@ namespace Wino.Core.Synchronizers currentStep.Value.DependsOn = [previousStep.Key]; } - } // Execute batch. This will collect responses from network call for each batch step. @@ -791,9 +804,11 @@ namespace Wino.Core.Synchronizers { if (!httpResponseMessage.IsSuccessStatusCode) { + bundle.Request.RevertUIChanges(); + var content = await httpResponseMessage.Content.ReadAsStringAsync(); var errorJson = JsonObject.Parse(content); - var errorString = $"({httpResponseMessage.StatusCode}) {errorJson["error"]["code"]} - {errorJson["error"]["message"]}"; + var errorString = $"{httpResponseMessage.StatusCode} [{bundle.Request.GetType().Name}]\n{errorJson["error"]["code"]} - {errorJson["error"]["message"]}\n"; exceptionBag.Add(errorString); } @@ -818,12 +833,6 @@ namespace Wino.Core.Synchronizers public override async Task> CreateNewMailPackagesAsync(Message message, MailItemFolder assignedFolder, CancellationToken cancellationToken = default) { - bool isMailExists = await _outlookChangeProcessor.IsMailExistsAsync(message.Id); - - if (isMailExists) - { - return null; - } var mimeMessage = await DownloadMimeMessageAsync(message.Id, cancellationToken).ConfigureAwait(false); var mailCopy = message.AsMailCopy();