From 3489bf22bb678b0b3bfc95b713115e67d22c7749 Mon Sep 17 00:00:00 2001 From: Stephan Date: Thu, 1 Sep 2016 12:13:58 +0200 Subject: [PATCH 1/3] U4-8698 - fix notification service perfs issues --- .../Repositories/NotificationsRepository.cs | 20 +++ .../Repositories/UserRepository.cs | 64 ++++--- .../Services/NotificationService.cs | 161 +++++++++++++----- src/Umbraco.Core/Services/UserService.cs | 10 ++ 4 files changed, 182 insertions(+), 73 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/NotificationsRepository.cs b/src/Umbraco.Core/Persistence/Repositories/NotificationsRepository.cs index a15a7e1521..3549f50859 100644 --- a/src/Umbraco.Core/Persistence/Repositories/NotificationsRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/NotificationsRepository.cs @@ -18,6 +18,26 @@ namespace Umbraco.Core.Persistence.Repositories _unitOfWork = unitOfWork; } + public IEnumerable GetUsersNotifications(IEnumerable userIds, string action, IEnumerable nodeIds) + { + var nodeIdsA = nodeIds.ToArray(); + var syntax = ApplicationContext.Current.DatabaseContext.SqlSyntax; // bah + var sql = new Sql() + .Select("DISTINCT umbracoNode.id, umbracoUser2NodeNotify.userId, umbracoNode.nodeObjectType, umbracoUser2NodeNotify.action") + .From(syntax) + .InnerJoin(syntax).On(syntax, left => left.NodeId, right => right.NodeId) + .InnerJoin(syntax).On(syntax, left => left.UserId, right => right.Id) + .Where(x => x.Disabled == false) // only approved users + .Where(x => x.Action == action); // on the specified action + if (nodeIdsA.Length > 0) + sql + .WhereIn(x => x.NodeId, nodeIdsA); // for the specified nodes + sql + .OrderBy(x => x.Id, syntax) + .OrderBy(dto => dto.NodeId, syntax); + return _unitOfWork.Database.Fetch(sql).Select(x => new Notification(x.id, x.userId, x.nodeObjectType, x.action)); + } + public IEnumerable GetUserNotifications(IUser user) { var sql = new Sql() diff --git a/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs b/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs index 6bd83a8ad8..26fdb95b33 100644 --- a/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs @@ -320,51 +320,49 @@ namespace Umbraco.Core.Persistence.Repositories /// public IEnumerable GetPagedResultsByQuery(IQuery query, int pageIndex, int pageSize, out int totalRecords, Expression> orderBy) { - if (orderBy == null) throw new ArgumentNullException("orderBy"); - - var sql = new Sql(); - sql.Select("*").From(); - - Sql resultQuery; - if (query != null) - { - var translator = new SqlTranslator(sql, query); - resultQuery = translator.Translate(); - } - else - { - resultQuery = sql; - } - - //get the referenced column name + // get the referenced column name and find the corresp mapped column name var expressionMember = ExpressionHelper.GetMemberInfo(orderBy); - //now find the mapped column name var mapper = MappingResolver.Current.ResolveMapperByType(typeof(IUser)); var mappedField = mapper.Map(expressionMember.Name); + + if (orderBy == null) + throw new ArgumentNullException("orderBy"); if (mappedField.IsNullOrWhiteSpace()) - { throw new ArgumentException("Could not find a mapping for the column specified in the orderBy clause"); - } - //need to ensure the order by is in brackets, see: https://github.com/toptensoftware/PetaPoco/issues/177 - resultQuery.OrderBy(string.Format("({0})", mappedField)); - var pagedResult = Database.Page(pageIndex + 1, pageSize, resultQuery); + var sql = new Sql() + .Select("umbracoUser.Id") + .From(SqlSyntax); - totalRecords = Convert.ToInt32(pagedResult.TotalItems); + var idsQuery = query == null ? sql : new SqlTranslator(sql, query).Translate(); + + // need to ensure the order by is in brackets, see: https://github.com/toptensoftware/PetaPoco/issues/177 + idsQuery.OrderBy("(" + mappedField + ")"); + var page = Database.Page(pageIndex + 1, pageSize, idsQuery); + totalRecords = Convert.ToInt32(page.TotalItems); - //now that we have the user dto's we need to construct true members from the list. if (totalRecords == 0) - { return Enumerable.Empty(); - } - var ids = pagedResult.Items.Select(x => x.Id).ToArray(); - var result = ids.Length == 0 ? Enumerable.Empty() : GetAll(ids); - - //now we need to ensure this result is also ordered by the same order by clause - return result.OrderBy(orderBy.Compile()); + // now get the actual users and ensure they are ordered properly (same clause) + var ids = page.Items.ToArray(); + return ids.Length == 0 ? Enumerable.Empty() : GetAll(ids).OrderBy(orderBy.Compile()); } - + + internal IEnumerable GetNextUsers(int id, int count) + { + var idsQuery = new Sql() + .Select("umbracoUser.Id") + .From(SqlSyntax) + .Where(x => x.Id >= id) + .OrderBy(x => x.Id, SqlSyntax); + + var ids = Database.Page(0, count, idsQuery).Items.ToArray(); + + // now get the actual users and ensure they are ordered properly (same clause) + return ids.Length == 0 ? Enumerable.Empty() : GetAll(ids).OrderBy(x => x.Id); + } + /// /// Returns permissions for a given user for any number of nodes /// diff --git a/src/Umbraco.Core/Services/NotificationService.cs b/src/Umbraco.Core/Services/NotificationService.cs index 5d51e20008..68b91ec82a 100644 --- a/src/Umbraco.Core/Services/NotificationService.cs +++ b/src/Umbraco.Core/Services/NotificationService.cs @@ -55,38 +55,61 @@ namespace Umbraco.Core.Services Func createSubject, Func createBody) { - if ((entity is IContent) == false) + if (entity is IContent == false) throw new NotSupportedException(); var content = (IContent) entity; - // lazily get versions - into a list to ensure we can enumerate multiple times + // lazily get versions List allVersions = null; - int totalUsers; - var allUsers = _userService.GetAll(0, int.MaxValue, out totalUsers); - foreach (var u in allUsers.Where(x => x.IsApproved)) + // do not load *all* users in memory at once + // do not load notifications *per user* (N+1 select) + // cannot load users & notifications in 1 query (combination btw User2AppDto and User2NodeNotifyDto) + // => get batches of users, get all their notifications in 1 query + // re. users: + // users being (dis)approved = not an issue, filtered in memory not in SQL + // users being modified or created = not an issue, ordering by ID, as long as we don't *insert* low IDs + // users being deleted = not an issue for GetNextUsers + var id = 0; + var nodeIds = content.Path.Split(',').Select(int.Parse).ToArray(); + const int pagesz = 400; // load batches of 400 users + do { - var userNotifications = GetUserNotifications(u, content.Path); - var notificationForAction = userNotifications.FirstOrDefault(x => x.Action == action); - if (notificationForAction == null) continue; + // users are returned ordered by id, notifications are returned ordered by user id + var users = ((UserService) _userService).GetNextUsers(id, pagesz).Where(x => x.IsApproved).ToList(); + var notifications = GetUsersNotifications(users.Select(x => x.Id), action, nodeIds)/*.OrderBy(x => x.UserId)*/.ToList(); + if (notifications.Count == 0) break; - if (allVersions == null) // lazy load - allVersions = _contentService.GetVersions(entity.Id).ToList(); - - try + var i = 0; + foreach (var user in users) { - SendNotification(operatingUser, u, content, allVersions, - actionName, http, createSubject, createBody); + // continue if there's no notification for this user + if (notifications[i].UserId != user.Id) continue; // next user - _logger.Debug(string.Format("Notification type: {0} sent to {1} ({2})", - action, u.Name, u.Email)); + // lazy load all versions + if (allVersions == null) allVersions = _contentService.GetVersions(entity.Id).ToList(); + + // notify + try + { + SendNotification(operatingUser, user, content, allVersions, actionName, http, createSubject, createBody); + _logger.Debug(string.Format("Notification type: {0} sent to {1} ({2})", action, user.Name, user.Email)); + } + catch (Exception ex) + { + _logger.Error("An error occurred sending notification", ex); + } + + // skip other notifications for this user + while (i < notifications.Count && notifications[i++].UserId == user.Id) ; + if (i >= notifications.Count) break; // break if no more notifications } - catch (Exception ex) - { - _logger.Error("An error occurred sending notification", ex); - } - } + + // load more users if any + id = users.Count == pagesz ? users.Last().Id + 1 : -1; + + } while (id > 0); } /// @@ -106,15 +129,67 @@ namespace Umbraco.Core.Services Func createSubject, Func createBody) { - if ((entities is IEnumerable) == false) + if (entities is IEnumerable == false) throw new NotSupportedException(); - // ensure we can enumerate multiple times var entitiesL = entities as List ?? entities.Cast().ToList(); + var paths = new List(); - // lazily get versions - into lists to ensure we can enumerate multiple times + // lazily get versions var allVersionsDictionary = new Dictionary>(); + // see notes above + var id = 0; + const int pagesz = 400; // load batches of 400 users + do + { + // users are returned ordered by id, notifications are returned ordered by user id + var users = ((UserService)_userService).GetNextUsers(id, pagesz).Where(x => x.IsApproved).ToList(); + var notifications = GetUsersNotifications(users.Select(x => x.Id), action, Enumerable.Empty())/*.OrderBy(x => x.UserId)*/.ToList(); + if (notifications.Count == 0) break; + + var i = 0; + foreach (var user in users) + { + // continue if there's no notification for this user + if (notifications[i].UserId != user.Id) continue; // next user + + for (var j = 0; j < entitiesL.Count; j++) + { + var content = entitiesL[j]; + int[] path; + if (paths.Count < j) + paths.Add(path = content.Path.Split(',').Select(int.Parse).ToArray()); + else path = paths[j]; + + // test if the notification applies to the path ie to this entity + if (path.Contains(notifications[i].EntityId) == false) continue; // next entity + + var allVersions = allVersionsDictionary.ContainsKey(content.Id) + ? allVersionsDictionary[content.Id] + : allVersionsDictionary[content.Id] = _contentService.GetVersions(content.Id).ToList(); + + try + { + SendNotification(operatingUser, user, content, allVersions, actionName, http, createSubject, createBody); + _logger.Debug(string.Format("Notification type: {0} sent to {1} ({2})", action, user.Name, user.Email)); + } + catch (Exception ex) + { + _logger.Error("An error occurred sending notification", ex); + } + } + + // skip other notifications for this user + while (i < notifications.Count && notifications[i++].UserId == user.Id) ; + if (i >= notifications.Count) break; // break if no more notifications + } + + // load more users if any + id = users.Count == pagesz ? users.Last().Id + 1 : -1; + + } while (id > 0); + int totalUsers; var allUsers = _userService.GetAll(0, int.MaxValue, out totalUsers); foreach (var u in allUsers.Where(x => x.IsApproved)) @@ -127,13 +202,13 @@ namespace Umbraco.Core.Services var notificationForAction = userNotificationsByPath.FirstOrDefault(x => x.Action == action); if (notificationForAction == null) continue; - var allVersions = allVersionsDictionary.ContainsKey(content.Id) // lazy load + var allVersions = allVersionsDictionary.ContainsKey(content.Id) ? allVersionsDictionary[content.Id] : allVersionsDictionary[content.Id] = _contentService.GetVersions(content.Id).ToList(); try { - SendNotification(operatingUser, u, content, allVersions, + SendNotification(operatingUser, u, content, allVersions, actionName, http, createSubject, createBody); _logger.Debug(string.Format("Notification type: {0} sent to {1} ({2})", @@ -147,6 +222,12 @@ namespace Umbraco.Core.Services } } + private IEnumerable GetUsersNotifications(IEnumerable userIds, string action, IEnumerable nodeIds) + { + var uow = _uowProvider.GetUnitOfWork(); + var repository = new NotificationsRepository(uow); + return repository.GetUsersNotifications(userIds, action, nodeIds); + } /// /// Gets the notifications for the user @@ -184,7 +265,7 @@ namespace Umbraco.Core.Services public IEnumerable FilterUserNotificationsByPath(IEnumerable userNotifications, string path) { var pathParts = path.Split(new[] {','}, StringSplitOptions.RemoveEmptyEntries); - return userNotifications.Where(r => pathParts.InvariantContains(r.EntityId.ToString(CultureInfo.InvariantCulture))).ToList(); + return userNotifications.Where(r => pathParts.InvariantContains(r.EntityId.ToString(CultureInfo.InvariantCulture))).ToList(); } /// @@ -259,7 +340,7 @@ namespace Umbraco.Core.Services /// /// Callback to create the mail subject /// Callback to create the mail body - private void SendNotification(IUser performingUser, IUser mailingUser, IContent content, IEnumerable allVersions, string actionName, HttpContextBase http, + private void SendNotification(IUser performingUser, IUser mailingUser, IContent content, IEnumerable allVersions, string actionName, HttpContextBase http, Func createSubject, Func createBody) { @@ -290,16 +371,16 @@ namespace Umbraco.Core.Services { var oldProperty = oldDoc.Properties[p.PropertyType.Alias]; oldText = oldProperty.Value != null ? oldProperty.Value.ToString() : ""; - + // replace html with char equivalent ReplaceHtmlSymbols(ref oldText); ReplaceHtmlSymbols(ref newText); } - + // make sure to only highlight changes done using TinyMCE editor... other changes will be displayed using default summary // TODO: We should probably allow more than just tinymce?? - if ((p.PropertyType.PropertyEditorAlias == Constants.PropertyEditors.TinyMCEAlias) + if ((p.PropertyType.PropertyEditorAlias == Constants.PropertyEditors.TinyMCEAlias) && string.CompareOrdinal(oldText, newText) != 0) { summary.Append(""); @@ -340,14 +421,14 @@ namespace Umbraco.Core.Services string[] subjectVars = { http.Request.ServerVariables["SERVER_NAME"] + ":" + http.Request.Url.Port + - IOHelper.ResolveUrl(SystemDirectories.Umbraco), + IOHelper.ResolveUrl(SystemDirectories.Umbraco), actionName, content.Name }; string[] bodyVars = { - mailingUser.Name, - actionName, - content.Name, + mailingUser.Name, + actionName, + content.Name, performingUser.Name, http.Request.ServerVariables["SERVER_NAME"] + ":" + http.Request.Url.Port + IOHelper.ResolveUrl(SystemDirectories.Umbraco), content.Id.ToString(CultureInfo.InvariantCulture), summary.ToString(), @@ -357,10 +438,10 @@ namespace Umbraco.Core.Services /*umbraco.library.NiceUrl(documentObject.Id))*/ content.Id + ".aspx", protocol) - + }; - // create the mail message + // create the mail message var mail = new MailMessage(UmbracoConfig.For.UmbracoSettings().Content.NotificationEmailAddress, mailingUser.Email); // populate the message @@ -401,9 +482,9 @@ namespace Umbraco.Core.Services using (var sender = new SmtpClient()) { sender.Send(mail); - } + } } - + } catch (Exception ex) { @@ -484,7 +565,7 @@ namespace Umbraco.Core.Services pos++; } // while sb.Append(""); - } // if + } // if } // while // write rest of unchanged chars diff --git a/src/Umbraco.Core/Services/UserService.cs b/src/Umbraco.Core/Services/UserService.cs index c7a63a884b..8e984d1e5d 100644 --- a/src/Umbraco.Core/Services/UserService.cs +++ b/src/Umbraco.Core/Services/UserService.cs @@ -9,6 +9,7 @@ using Umbraco.Core.Logging; using Umbraco.Core.Models.Membership; using Umbraco.Core.Persistence; using Umbraco.Core.Persistence.Querying; +using Umbraco.Core.Persistence.Repositories; using Umbraco.Core.Persistence.UnitOfWork; using Umbraco.Core.Security; @@ -506,6 +507,15 @@ namespace Umbraco.Core.Services } } + internal IEnumerable GetNextUsers(int id, int count) + { + var uow = UowProvider.GetUnitOfWork(); + using (var repository = (UserRepository) RepositoryFactory.CreateUserRepository(uow)) + { + return repository.GetNextUsers(id, count); + } + } + #endregion #region Implementation of IUserService From 365da75d1b1e613f361ac5e4cad32c965f6c46fc Mon Sep 17 00:00:00 2001 From: Stephan Date: Thu, 1 Sep 2016 15:25:40 +0200 Subject: [PATCH 2/3] U4-8698 - fix notification service perfs issues --- src/Umbraco.Core/Constants-ObjectTypes.cs | 5 + .../Repositories/NotificationsRepository.cs | 7 +- .../Repositories/UserRepository.cs | 3 +- .../Services/NotificationService.cs | 182 ++++++++++-------- 4 files changed, 114 insertions(+), 83 deletions(-) diff --git a/src/Umbraco.Core/Constants-ObjectTypes.cs b/src/Umbraco.Core/Constants-ObjectTypes.cs index 7ec45db7be..3f9974166c 100644 --- a/src/Umbraco.Core/Constants-ObjectTypes.cs +++ b/src/Umbraco.Core/Constants-ObjectTypes.cs @@ -69,6 +69,11 @@ namespace Umbraco.Core /// public const string Document = "C66BA18E-EAF3-4CFF-8A22-41B16D66A972"; + /// + /// Guid for a Document object. + /// + public static readonly Guid DocumentGuid = new Guid(Document); + /// /// Guid for a Document Type object. /// diff --git a/src/Umbraco.Core/Persistence/Repositories/NotificationsRepository.cs b/src/Umbraco.Core/Persistence/Repositories/NotificationsRepository.cs index 3549f50859..9e6e3cf47c 100644 --- a/src/Umbraco.Core/Persistence/Repositories/NotificationsRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/NotificationsRepository.cs @@ -18,15 +18,16 @@ namespace Umbraco.Core.Persistence.Repositories _unitOfWork = unitOfWork; } - public IEnumerable GetUsersNotifications(IEnumerable userIds, string action, IEnumerable nodeIds) + public IEnumerable GetUsersNotifications(IEnumerable userIds, string action, IEnumerable nodeIds, Guid objectType) { var nodeIdsA = nodeIds.ToArray(); var syntax = ApplicationContext.Current.DatabaseContext.SqlSyntax; // bah var sql = new Sql() - .Select("DISTINCT umbracoNode.id, umbracoUser2NodeNotify.userId, umbracoNode.nodeObjectType, umbracoUser2NodeNotify.action") + .Select("DISTINCT umbracoNode.id nodeId, umbracoUser.id userId, umbracoNode.nodeObjectType, umbracoUser2NodeNotify.action") .From(syntax) .InnerJoin(syntax).On(syntax, left => left.NodeId, right => right.NodeId) .InnerJoin(syntax).On(syntax, left => left.UserId, right => right.Id) + .Where(x => x.NodeObjectType == objectType) .Where(x => x.Disabled == false) // only approved users .Where(x => x.Action == action); // on the specified action if (nodeIdsA.Length > 0) @@ -35,7 +36,7 @@ namespace Umbraco.Core.Persistence.Repositories sql .OrderBy(x => x.Id, syntax) .OrderBy(dto => dto.NodeId, syntax); - return _unitOfWork.Database.Fetch(sql).Select(x => new Notification(x.id, x.userId, x.nodeObjectType, x.action)); + return _unitOfWork.Database.Fetch(sql).Select(x => new Notification(x.nodeId, x.userId, x.action, objectType)); } public IEnumerable GetUserNotifications(IUser user) diff --git a/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs b/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs index 26fdb95b33..cbaa92da21 100644 --- a/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs @@ -357,7 +357,8 @@ namespace Umbraco.Core.Persistence.Repositories .Where(x => x.Id >= id) .OrderBy(x => x.Id, SqlSyntax); - var ids = Database.Page(0, count, idsQuery).Items.ToArray(); + // first page is index 1, not zero + var ids = Database.Page(1, count, idsQuery).Items.ToArray(); // now get the actual users and ensure they are ordered properly (same clause) return ids.Length == 0 ? Enumerable.Empty() : GetAll(ids).OrderBy(x => x.Id); diff --git a/src/Umbraco.Core/Services/NotificationService.cs b/src/Umbraco.Core/Services/NotificationService.cs index 68b91ec82a..80509016ef 100644 --- a/src/Umbraco.Core/Services/NotificationService.cs +++ b/src/Umbraco.Core/Services/NotificationService.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Globalization; using System.Linq; @@ -15,7 +16,6 @@ using Umbraco.Core.Models.Membership; using Umbraco.Core.Persistence.Repositories; using Umbraco.Core.Persistence.UnitOfWork; using Umbraco.Core.Strings; -using umbraco.interfaces; namespace Umbraco.Core.Services { @@ -58,9 +58,9 @@ namespace Umbraco.Core.Services if (entity is IContent == false) throw new NotSupportedException(); - var content = (IContent) entity; - - // lazily get versions + var content = (IContent) entity; + + // lazily get versions List allVersions = null; // do not load *all* users in memory at once @@ -78,7 +78,7 @@ namespace Umbraco.Core.Services { // users are returned ordered by id, notifications are returned ordered by user id var users = ((UserService) _userService).GetNextUsers(id, pagesz).Where(x => x.IsApproved).ToList(); - var notifications = GetUsersNotifications(users.Select(x => x.Id), action, nodeIds)/*.OrderBy(x => x.UserId)*/.ToList(); + var notifications = GetUsersNotifications(users.Select(x => x.Id), action, nodeIds, Constants.ObjectTypes.DocumentGuid).ToList(); if (notifications.Count == 0) break; var i = 0; @@ -90,16 +90,9 @@ namespace Umbraco.Core.Services // lazy load all versions if (allVersions == null) allVersions = _contentService.GetVersions(entity.Id).ToList(); - // notify - try - { - SendNotification(operatingUser, user, content, allVersions, actionName, http, createSubject, createBody); - _logger.Debug(string.Format("Notification type: {0} sent to {1} ({2})", action, user.Name, user.Email)); - } - catch (Exception ex) - { - _logger.Error("An error occurred sending notification", ex); - } + // queue notification + var req = CreateNotificationRequest(operatingUser, user, content, allVersions, actionName, http, createSubject, createBody); + Enqueue(req); // skip other notifications for this user while (i < notifications.Count && notifications[i++].UserId == user.Id) ; @@ -145,7 +138,7 @@ namespace Umbraco.Core.Services { // users are returned ordered by id, notifications are returned ordered by user id var users = ((UserService)_userService).GetNextUsers(id, pagesz).Where(x => x.IsApproved).ToList(); - var notifications = GetUsersNotifications(users.Select(x => x.Id), action, Enumerable.Empty())/*.OrderBy(x => x.UserId)*/.ToList(); + var notifications = GetUsersNotifications(users.Select(x => x.Id), action, Enumerable.Empty(), Constants.ObjectTypes.DocumentGuid).ToList(); if (notifications.Count == 0) break; var i = 0; @@ -169,15 +162,9 @@ namespace Umbraco.Core.Services ? allVersionsDictionary[content.Id] : allVersionsDictionary[content.Id] = _contentService.GetVersions(content.Id).ToList(); - try - { - SendNotification(operatingUser, user, content, allVersions, actionName, http, createSubject, createBody); - _logger.Debug(string.Format("Notification type: {0} sent to {1} ({2})", action, user.Name, user.Email)); - } - catch (Exception ex) - { - _logger.Error("An error occurred sending notification", ex); - } + // queue notification + var req = CreateNotificationRequest(operatingUser, user, content, allVersions, actionName, http, createSubject, createBody); + Enqueue(req); } // skip other notifications for this user @@ -189,44 +176,13 @@ namespace Umbraco.Core.Services id = users.Count == pagesz ? users.Last().Id + 1 : -1; } while (id > 0); - - int totalUsers; - var allUsers = _userService.GetAll(0, int.MaxValue, out totalUsers); - foreach (var u in allUsers.Where(x => x.IsApproved)) - { - var userNotifications = GetUserNotifications(u).ToArray(); - - foreach (var content in entitiesL) - { - var userNotificationsByPath = FilterUserNotificationsByPath(userNotifications, content.Path); - var notificationForAction = userNotificationsByPath.FirstOrDefault(x => x.Action == action); - if (notificationForAction == null) continue; - - var allVersions = allVersionsDictionary.ContainsKey(content.Id) - ? allVersionsDictionary[content.Id] - : allVersionsDictionary[content.Id] = _contentService.GetVersions(content.Id).ToList(); - - try - { - SendNotification(operatingUser, u, content, allVersions, - actionName, http, createSubject, createBody); - - _logger.Debug(string.Format("Notification type: {0} sent to {1} ({2})", - action, u.Name, u.Email)); - } - catch (Exception ex) - { - _logger.Error("An error occurred sending notification", ex); - } - } - } } - private IEnumerable GetUsersNotifications(IEnumerable userIds, string action, IEnumerable nodeIds) + private IEnumerable GetUsersNotifications(IEnumerable userIds, string action, IEnumerable nodeIds, Guid objectType) { var uow = _uowProvider.GetUnitOfWork(); var repository = new NotificationsRepository(uow); - return repository.GetUsersNotifications(userIds, action, nodeIds); + return repository.GetUsersNotifications(userIds, action, nodeIds, objectType); } /// @@ -340,7 +296,7 @@ namespace Umbraco.Core.Services /// /// Callback to create the mail subject /// Callback to create the mail body - private void SendNotification(IUser performingUser, IUser mailingUser, IContent content, IEnumerable allVersions, string actionName, HttpContextBase http, + private NotificationRequest CreateNotificationRequest(IUser performingUser, IUser mailingUser, IContent content, IEnumerable allVersions, string actionName, HttpContextBase http, Func createSubject, Func createBody) { @@ -471,26 +427,7 @@ namespace Umbraco.Core.Services string.Format("https://{0}", serverName)); } - - // send it asynchronously, we don't want to got up all of the request time to send emails! - ThreadPool.QueueUserWorkItem(state => - { - try - { - using (mail) - { - using (var sender = new SmtpClient()) - { - sender.Send(mail); - } - } - - } - catch (Exception ex) - { - _logger.Error("An error occurred sending notification", ex); - } - }); + return new NotificationRequest(mail, actionName, mailingUser.Name, mailingUser.Email); } private static string ReplaceLinks(string text, HttpRequestBase request) @@ -576,8 +513,95 @@ namespace Umbraco.Core.Services } // while return sb.ToString(); + } + + // manage notifications + // ideally, would need to use IBackgroundTasks - but they are not part of Core! + + private static readonly object Locker = new object(); + private static readonly BlockingCollection Queue = new BlockingCollection(); + private static volatile bool _running; + + private void Enqueue(NotificationRequest notification) + { + Queue.Add(notification); + if (_running) return; + lock (Locker) + { + if (_running) return; + Process(Queue); + _running = true; + } + } + + private class NotificationRequest + { + public NotificationRequest(MailMessage mail, string action, string userName, string email) + { + Mail = mail; + Action = action; + UserName = userName; + Email = email; + } + + public MailMessage Mail { get; private set; } + + public string Action { get; private set; } + + public string UserName { get; private set; } + + public string Email { get; private set; } } + private void Process(BlockingCollection notificationRequests) + { + ThreadPool.QueueUserWorkItem(state => + { + var s = new SmtpClient(); + try + { + _logger.Debug("Begin processing notifications."); + while (true) + { + NotificationRequest request; + while (notificationRequests.TryTake(out request, 8 * 1000)) // stay on for 8s + { + try + { + if (Sendmail != null) Sendmail(s, request.Mail, _logger); else s.Send(request.Mail); + _logger.Debug(string.Format("Notification \"{0}\" sent to {1} ({2})", request.Action, request.UserName, request.Email)); + } + catch (Exception ex) + { + _logger.Error("An error occurred sending notification", ex); + s.Dispose(); + s = new SmtpClient(); + } + finally + { + request.Mail.Dispose(); + } + } + lock (Locker) + { + if (notificationRequests.Count > 0) continue; // last chance + _running = false; // going down + break; + } + } + } + finally + { + s.Dispose(); + } + _logger.Debug("Done processing notifications."); + }); + } + + // for tests + internal static Action Sendmail; + //= (_, msg, logger) => logger.Debug("Email " + msg.To.ToString()); + #endregion } } \ No newline at end of file From 2cd6427958e013e96e80a86951c523c5fa263e04 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 5 Oct 2016 18:30:03 +0200 Subject: [PATCH 3/3] Fixes more issues with notifications which weren't sending properly do to a i++ instead of a ++1, have refactored that to read better. Also fixes up performance issues since we were returning every single fully resolved IContent for all versions when all we need is the top 2 version ids, have added methods to do that plus a unit test. Have tested the emails and the diffs and they all work. --- .../Interfaces/IRepositoryVersionable.cs | 12 +- .../Repositories/VersionableRepositoryBase.cs | 27 ++++ src/Umbraco.Core/Services/ContentService.cs | 15 ++ src/Umbraco.Core/Services/IContentService.cs | 8 ++ .../Services/NotificationService.cs | 135 +++++++++++------- .../Services/ContentServiceTests.cs | 23 +++ 6 files changed, 164 insertions(+), 56 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Interfaces/IRepositoryVersionable.cs b/src/Umbraco.Core/Persistence/Repositories/Interfaces/IRepositoryVersionable.cs index 229a6fc0ef..3e05d1feaf 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Interfaces/IRepositoryVersionable.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Interfaces/IRepositoryVersionable.cs @@ -34,11 +34,19 @@ namespace Umbraco.Core.Persistence.Repositories int CountDescendants(int parentId, string contentTypeAlias = null); /// - /// Gets a list of all versions for an . + /// Gets a list of all versions for an ordered so latest is first /// /// Id of the to retrieve versions from /// An enumerable list of the same object with different versions - IEnumerable GetAllVersions(int id); + IEnumerable GetAllVersions(int id); + + /// + /// Gets a list of all version Ids for the given content item + /// + /// + /// The maximum number of rows to return + /// + IEnumerable GetVersionIds(int id, int maxRows); /// /// Gets a specific version of an . diff --git a/src/Umbraco.Core/Persistence/Repositories/VersionableRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/VersionableRepositoryBase.cs index 4ef493a22a..4258af3672 100644 --- a/src/Umbraco.Core/Persistence/Repositories/VersionableRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/VersionableRepositoryBase.cs @@ -40,6 +40,11 @@ namespace Umbraco.Core.Persistence.Repositories #region IRepositoryVersionable Implementation + /// + /// Gets a list of all versions for an ordered so latest is first + /// + /// Id of the to retrieve versions from + /// An enumerable list of the same object with different versions public virtual IEnumerable GetAllVersions(int id) { var sql = new Sql(); @@ -60,6 +65,28 @@ namespace Umbraco.Core.Persistence.Repositories } } + /// + /// Gets a list of all version Ids for the given content item ordered so latest is first + /// + /// + /// The maximum number of rows to return + /// + public virtual IEnumerable GetVersionIds(int id, int maxRows) + { + var sql = new Sql(); + sql.Select("cmsDocument.versionId") + .From(SqlSyntax) + .InnerJoin(SqlSyntax) + .On(SqlSyntax, left => left.NodeId, right => right.NodeId) + .InnerJoin(SqlSyntax) + .On(SqlSyntax, left => left.NodeId, right => right.NodeId) + .Where(x => x.NodeObjectType == NodeObjectTypeId) + .Where(x => x.NodeId == id) + .OrderByDescending(x => x.UpdateDate, SqlSyntax); + + return Database.Fetch(SqlSyntax.SelectTop(sql, maxRows)); + } + public virtual void DeleteVersion(Guid versionId) { var dto = Database.FirstOrDefault("WHERE versionId = @VersionId", new { VersionId = versionId }); diff --git a/src/Umbraco.Core/Services/ContentService.cs b/src/Umbraco.Core/Services/ContentService.cs index b7bac5d83d..38b7aec9d7 100644 --- a/src/Umbraco.Core/Services/ContentService.cs +++ b/src/Umbraco.Core/Services/ContentService.cs @@ -448,6 +448,21 @@ namespace Umbraco.Core.Services } } + /// + /// Gets a list of all version Ids for the given content item ordered so latest is first + /// + /// + /// The maximum number of rows to return + /// + public IEnumerable GetVersionIds(int id, int maxRows) + { + using (var repository = RepositoryFactory.CreateContentRepository(UowProvider.GetUnitOfWork())) + { + var versions = repository.GetVersionIds(id, maxRows); + return versions; + } + } + /// /// Gets a collection of objects, which are ancestors of the current content. /// diff --git a/src/Umbraco.Core/Services/IContentService.cs b/src/Umbraco.Core/Services/IContentService.cs index fa4130f8c4..ec5db8c4fb 100644 --- a/src/Umbraco.Core/Services/IContentService.cs +++ b/src/Umbraco.Core/Services/IContentService.cs @@ -298,6 +298,14 @@ namespace Umbraco.Core.Services /// An Enumerable list of objects IEnumerable GetVersions(int id); + /// + /// Gets a list of all version Ids for the given content item ordered so latest is first + /// + /// + /// The maximum number of rows to return + /// + IEnumerable GetVersionIds(int id, int maxRows); + /// /// Gets a collection of objects, which reside at the first level / root /// diff --git a/src/Umbraco.Core/Services/NotificationService.cs b/src/Umbraco.Core/Services/NotificationService.cs index 80509016ef..52b26ab1b3 100644 --- a/src/Umbraco.Core/Services/NotificationService.cs +++ b/src/Umbraco.Core/Services/NotificationService.cs @@ -60,8 +60,8 @@ namespace Umbraco.Core.Services var content = (IContent) entity; - // lazily get versions - List allVersions = null; + // lazily get previous version + IContentBase prevVersion = null; // do not load *all* users in memory at once // do not load notifications *per user* (N+1 select) @@ -87,11 +87,14 @@ namespace Umbraco.Core.Services // continue if there's no notification for this user if (notifications[i].UserId != user.Id) continue; // next user - // lazy load all versions - if (allVersions == null) allVersions = _contentService.GetVersions(entity.Id).ToList(); + // lazy load prev version + if (prevVersion == null) + { + prevVersion = GetPreviousVersion(entity.Id); + } // queue notification - var req = CreateNotificationRequest(operatingUser, user, content, allVersions, actionName, http, createSubject, createBody); + var req = CreateNotificationRequest(operatingUser, user, content, prevVersion, actionName, http, createSubject, createBody); Enqueue(req); // skip other notifications for this user @@ -105,6 +108,21 @@ namespace Umbraco.Core.Services } while (id > 0); } + /// + /// Gets the previous version to the latest version of the content item if there is one + /// + /// + /// + private IContentBase GetPreviousVersion(int contentId) + { + // Regarding this: http://issues.umbraco.org/issue/U4-5180 + // we know they are descending from the service so we know that newest is first + // we are only selecting the top 2 rows since that is all we need + var allVersions = _contentService.GetVersionIds(contentId, 2).ToList(); + var prevVersionIndex = allVersions.Count > 1 ? 1 : 0; + return _contentService.GetByVersion(allVersions[prevVersionIndex]); + } + /// /// Sends the notifications for the specified user regarding the specified node and action. /// @@ -126,10 +144,15 @@ namespace Umbraco.Core.Services throw new NotSupportedException(); var entitiesL = entities as List ?? entities.Cast().ToList(); - var paths = new List(); + + //exit if there are no entities + if (entitiesL.Count == 0) return; + + //put all entity's paths into a list with the same indicies + var paths = entitiesL.Select(x => x.Path.Split(',').Select(int.Parse).ToArray()).ToArray(); // lazily get versions - var allVersionsDictionary = new Dictionary>(); + var prevVersionDictionary = new Dictionary(); // see notes above var id = 0; @@ -150,25 +173,28 @@ namespace Umbraco.Core.Services for (var j = 0; j < entitiesL.Count; j++) { var content = entitiesL[j]; - int[] path; - if (paths.Count < j) - paths.Add(path = content.Path.Split(',').Select(int.Parse).ToArray()); - else path = paths[j]; - + var path = paths[j]; + // test if the notification applies to the path ie to this entity if (path.Contains(notifications[i].EntityId) == false) continue; // next entity - - var allVersions = allVersionsDictionary.ContainsKey(content.Id) - ? allVersionsDictionary[content.Id] - : allVersionsDictionary[content.Id] = _contentService.GetVersions(content.Id).ToList(); - + + if (prevVersionDictionary.ContainsKey(content.Id) == false) + { + prevVersionDictionary[content.Id] = GetPreviousVersion(content.Id); + } + // queue notification - var req = CreateNotificationRequest(operatingUser, user, content, allVersions, actionName, http, createSubject, createBody); + var req = CreateNotificationRequest(operatingUser, user, content, prevVersionDictionary[content.Id], actionName, http, createSubject, createBody); Enqueue(req); } - // skip other notifications for this user - while (i < notifications.Count && notifications[i++].UserId == user.Id) ; + // skip other notifications for this user, essentially this means moving i to the next index of notifications + // for the next user. + do + { + i++; + } while (i < notifications.Count && notifications[i].UserId == user.Id); + if (i >= notifications.Count) break; // break if no more notifications } @@ -291,29 +317,23 @@ namespace Umbraco.Core.Services /// /// /// - /// + /// /// The action readable name - currently an action is just a single letter, this is the name associated with the letter /// /// Callback to create the mail subject /// Callback to create the mail body - private NotificationRequest CreateNotificationRequest(IUser performingUser, IUser mailingUser, IContent content, IEnumerable allVersions, string actionName, HttpContextBase http, + private NotificationRequest CreateNotificationRequest(IUser performingUser, IUser mailingUser, IContentBase content, IContentBase oldDoc, + string actionName, HttpContextBase http, Func createSubject, Func createBody) { if (performingUser == null) throw new ArgumentNullException("performingUser"); if (mailingUser == null) throw new ArgumentNullException("mailingUser"); if (content == null) throw new ArgumentNullException("content"); - if (allVersions == null) throw new ArgumentNullException("allVersions"); if (http == null) throw new ArgumentNullException("http"); if (createSubject == null) throw new ArgumentNullException("createSubject"); - if (createBody == null) throw new ArgumentNullException("createBody"); - - //Ensure they are sorted: http://issues.umbraco.org/issue/U4-5180 - var allVersionsAsArray = allVersions.OrderBy(x => x.UpdateDate).ToArray(); - - int versionCount = (allVersionsAsArray.Length > 1) ? (allVersionsAsArray.Length - 2) : (allVersionsAsArray.Length - 1); - var oldDoc = _contentService.GetByVersion(allVersionsAsArray[versionCount].Version); - + if (createBody == null) throw new ArgumentNullException("createBody"); + // build summary var summary = new StringBuilder(); var props = content.Properties.ToArray(); @@ -345,26 +365,31 @@ namespace Umbraco.Core.Services " Red for deleted characters Yellow for inserted characters"); summary.Append(""); summary.Append(""); - summary.Append(" New " + - p.PropertyType.Name + ""); - summary.Append("" + - ReplaceLinks(CompareText(oldText, newText, true, false, "", string.Empty), http.Request) + - ""); + summary.Append(" New "); + summary.Append(p.PropertyType.Name); + summary.Append(""); + summary.Append(""); + summary.Append(ReplaceLinks(CompareText(oldText, newText, true, false, "", string.Empty), http.Request)); + summary.Append(""); summary.Append(""); summary.Append(""); - summary.Append(" Old " + - p.PropertyType.Name + ""); - summary.Append("" + - ReplaceLinks(CompareText(newText, oldText, true, false, "", string.Empty), http.Request) + - ""); + summary.Append(" Old "); + summary.Append(p.PropertyType.Name); + summary.Append(""); + summary.Append(""); + summary.Append(ReplaceLinks(CompareText(newText, oldText, true, false, "", string.Empty), http.Request)); + summary.Append(""); summary.Append(""); } else { summary.Append(""); - summary.Append("" + - p.PropertyType.Name + ""); - summary.Append("" + newText + ""); + summary.Append(""); + summary.Append(p.PropertyType.Name); + summary.Append(""); + summary.Append(""); + summary.Append(newText); + summary.Append(""); summary.Append(""); } summary.Append( @@ -375,9 +400,7 @@ namespace Umbraco.Core.Services string[] subjectVars = { - http.Request.ServerVariables["SERVER_NAME"] + ":" + - http.Request.Url.Port + - IOHelper.ResolveUrl(SystemDirectories.Umbraco), + string.Concat(http.Request.ServerVariables["SERVER_NAME"], ":", http.Request.Url.Port, IOHelper.ResolveUrl(SystemDirectories.Umbraco)), actionName, content.Name }; @@ -386,13 +409,13 @@ namespace Umbraco.Core.Services actionName, content.Name, performingUser.Name, - http.Request.ServerVariables["SERVER_NAME"] + ":" + http.Request.Url.Port + IOHelper.ResolveUrl(SystemDirectories.Umbraco), + string.Concat(http.Request.ServerVariables["SERVER_NAME"], ":", http.Request.Url.Port, IOHelper.ResolveUrl(SystemDirectories.Umbraco)), content.Id.ToString(CultureInfo.InvariantCulture), summary.ToString(), string.Format("{2}://{0}/{1}", - http.Request.ServerVariables["SERVER_NAME"] + ":" + http.Request.Url.Port, + string.Concat(http.Request.ServerVariables["SERVER_NAME"], ":", http.Request.Url.Port), //TODO: RE-enable this so we can have a nice url /*umbraco.library.NiceUrl(documentObject.Id))*/ - content.Id + ".aspx", + string.Concat(content.Id, ".aspx"), protocol) }; @@ -411,10 +434,10 @@ namespace Umbraco.Core.Services { mail.IsBodyHtml = true; mail.Body = - @" + string.Concat(@" -" + createBody(mailingUser, bodyVars); +", createBody(mailingUser, bodyVars)); } // nh, issue 30724. Due to hardcoded http strings in resource files, we need to check for https replacements here @@ -432,8 +455,12 @@ namespace Umbraco.Core.Services private static string ReplaceLinks(string text, HttpRequestBase request) { - string domain = GlobalSettings.UseSSL ? "https://" : "http://"; - domain += request.ServerVariables["SERVER_NAME"] + ":" + request.Url.Port + "/"; + var sb = new StringBuilder(GlobalSettings.UseSSL ? "https://" : "http://"); + sb.Append(request.ServerVariables["SERVER_NAME"]); + sb.Append(":"); + sb.Append(request.Url.Port); + sb.Append("/"); + var domain = sb.ToString(); text = text.Replace("href=\"/", "href=\"" + domain); text = text.Replace("src=\"/", "src=\"" + domain); return text; diff --git a/src/Umbraco.Tests/Services/ContentServiceTests.cs b/src/Umbraco.Tests/Services/ContentServiceTests.cs index 87253dcb4c..ed4a59bab4 100644 --- a/src/Umbraco.Tests/Services/ContentServiceTests.cs +++ b/src/Umbraco.Tests/Services/ContentServiceTests.cs @@ -68,6 +68,29 @@ namespace Umbraco.Tests.Services Assert.IsTrue(contentService.PublishWithStatus(content).Success); } + [Test] + public void Get_Top_Version_Ids() + { + // Arrange + var contentService = ServiceContext.ContentService; + + // Act + var content = contentService.CreateContentWithIdentity("Test", -1, "umbTextpage", 0); + for (int i = 0; i < 20; i++) + { + content.SetValue("bodyText", "hello world " + Guid.NewGuid()); + contentService.SaveAndPublishWithStatus(content); + } + + + // Assert + var allVersions = contentService.GetVersionIds(content.Id, int.MaxValue); + Assert.AreEqual(21, allVersions.Count()); + + var topVersions = contentService.GetVersionIds(content.Id, 4); + Assert.AreEqual(4, topVersions.Count()); + } + [Test] public void Get_By_Ids_Sorted() {