From 8eab1f1b557dc26c1a346928922bbe6acd09c896 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 30 Jul 2020 22:56:31 +1000 Subject: [PATCH 01/14] Fixes #8433 - member login sql locks --- .../Repositories/IMemberRepository.cs | 15 ++++++- .../Implement/MemberRepository.cs | 41 +++++++++++++++++++ .../Services/IMembershipMemberService.cs | 15 ++++++- .../Services/Implement/MemberService.cs | 24 +++++------ .../Services/Implement/UserService.cs | 7 ++++ .../Services/MemberServiceTests.cs | 22 ++++++++++ .../Providers/UmbracoMembershipProvider.cs | 8 +--- 7 files changed, 111 insertions(+), 21 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs b/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs index 245c024356..4213c7ecbe 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs @@ -1,4 +1,5 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; using Umbraco.Core.Models; using Umbraco.Core.Persistence.Querying; @@ -35,5 +36,17 @@ namespace Umbraco.Core.Persistence.Repositories /// /// int GetCountByQuery(IQuery query); + + /// + /// Sets a members last login date based on their username + /// + /// + /// + /// + /// This is a specialized method because whenever a member logs in, the membership provider requires us to set the 'online' which requires + /// updating their login date. This operation must be fast and cannot use database locks which is fine if we are only executing a single query + /// for this data since there won't be any other data contention issues. + /// + void SetLastLogin(string username, DateTime date); } } diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs index 42e7d1c32f..b71dd152fe 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs @@ -505,6 +505,47 @@ namespace Umbraco.Core.Persistence.Repositories.Implement return Database.ExecuteScalar(fullSql); } + /// + public void SetLastLogin(string username, DateTime date) + { + // Update the cms property value for the member + + var sqlSelectTemplateProperty = SqlContext.Templates.Get("Umbraco.Core.MemberRepository.SetLastLogin1", s => s + .Select(x => x.Id) + .From() + .InnerJoin().On((l, r) => l.Id == r.PropertyTypeId) + .InnerJoin().On((l, r) => l.Id == r.VersionId) + .InnerJoin().On((l, r) => l.NodeId == r.NodeId) + .InnerJoin().On((l, r) => l.NodeId == r.NodeId) + .Where(x => x.NodeObjectType == SqlTemplate.Arg("nodeObjectType")) + .Where(x => x.Alias == SqlTemplate.Arg("propertyTypeAlias")) + .Where(x => x.LoginName == SqlTemplate.Arg("username"))); + var sqlSelectProperty = sqlSelectTemplateProperty.Sql(Constants.ObjectTypes.Member, Constants.Conventions.Member.LastLoginDate, username); + + var update = Sql() + .Update(u => u + .Set(x => x.DateValue, date)) + .WhereIn(x => x.Id, sqlSelectProperty); + + Database.Execute(update); + + // Update the umbracoContentVersion value for the member + + var sqlSelectTemplateVersion = SqlContext.Templates.Get("Umbraco.Core.MemberRepository.SetLastLogin2", s => s + .Select(x => x.Id) + .From() + .InnerJoin().On((l, r) => l.NodeId == r.NodeId) + .InnerJoin().On((l, r) => l.NodeId == r.NodeId) + .Where(x => x.NodeObjectType == SqlTemplate.Arg("nodeObjectType")) + .Where(x => x.LoginName == SqlTemplate.Arg("username"))); + var sqlSelectVersion = sqlSelectTemplateVersion.Sql(Constants.ObjectTypes.Member, username); + + Database.Execute(Sql() + .Update(u => u + .Set(x => x.VersionDate, date)) + .WhereIn(x => x.Id, sqlSelectVersion)); + } + /// /// Gets paged member results. /// diff --git a/src/Umbraco.Core/Services/IMembershipMemberService.cs b/src/Umbraco.Core/Services/IMembershipMemberService.cs index 448b0c761a..3c6b4f6672 100644 --- a/src/Umbraco.Core/Services/IMembershipMemberService.cs +++ b/src/Umbraco.Core/Services/IMembershipMemberService.cs @@ -1,4 +1,5 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; using Umbraco.Core.Models; using Umbraco.Core.Models.Membership; using Umbraco.Core.Persistence.Querying; @@ -107,6 +108,18 @@ namespace Umbraco.Core.Services /// or to Delete void Delete(T membershipUser); + /// + /// Sets the last login date for the member if they are found by username + /// + /// + /// + /// + /// This is a specialized method because whenever a member logs in, the membership provider requires us to set the 'online' which requires + /// updating their login date. This operation must be fast and cannot use database locks which is fine if we are only executing a single query + /// for this data since there won't be any other data contention issues. + /// + void SetLastLogin(string username, DateTime date); + /// /// Saves an /// diff --git a/src/Umbraco.Core/Services/Implement/MemberService.cs b/src/Umbraco.Core/Services/Implement/MemberService.cs index a64e30495b..a914cfa2ea 100644 --- a/src/Umbraco.Core/Services/Implement/MemberService.cs +++ b/src/Umbraco.Core/Services/Implement/MemberService.cs @@ -806,12 +806,17 @@ namespace Umbraco.Core.Services.Implement #region Save - /// - /// Saves an - /// - /// to Save - /// Optional parameter to raise events. - /// Default is True otherwise set to False to not raise events + /// + public void SetLastLogin(string username, DateTime date) + { + using (var scope = ScopeProvider.CreateScope()) + { + _memberRepository.SetLastLogin(username, date); + scope.Complete(); + } + } + + /// public void Save(IMember member, bool raiseEvents = true) { //trimming username and email to make sure we have no trailing space @@ -847,12 +852,7 @@ namespace Umbraco.Core.Services.Implement } } - /// - /// Saves a list of objects - /// - /// to save - /// Optional parameter to raise events. - /// Default is True otherwise set to False to not raise events + /// public void Save(IEnumerable members, bool raiseEvents = true) { var membersA = members.ToArray(); diff --git a/src/Umbraco.Core/Services/Implement/UserService.cs b/src/Umbraco.Core/Services/Implement/UserService.cs index 363bc72bc3..e4906863fa 100644 --- a/src/Umbraco.Core/Services/Implement/UserService.cs +++ b/src/Umbraco.Core/Services/Implement/UserService.cs @@ -254,6 +254,13 @@ namespace Umbraco.Core.Services.Implement } } + // explicit implementation because we don't need it now but due to the way that the members membership provider is put together + // this method must exist in this service as an implementation (legacy) + void IMembershipMemberService.SetLastLogin(string username, DateTime date) + { + throw new NotSupportedException("This method is not implemented or supported for users"); + } + /// /// Saves an /// diff --git a/src/Umbraco.Tests/Services/MemberServiceTests.cs b/src/Umbraco.Tests/Services/MemberServiceTests.cs index 57d6b67af8..02b0da56e1 100644 --- a/src/Umbraco.Tests/Services/MemberServiceTests.cs +++ b/src/Umbraco.Tests/Services/MemberServiceTests.cs @@ -48,6 +48,28 @@ namespace Umbraco.Tests.Services ((MemberService)ServiceContext.MemberService).MembershipProvider = provider; } + [Test] + public void Can_Set_Last_Login_Date() + { + var now = DateTime.Now; + var memberType = ServiceContext.MemberTypeService.Get("member"); + IMember member = new Member("xname", "xemail", "xusername", "xrawpassword", memberType, true) + { + LastLoginDate = now, + UpdateDate = now + }; + ServiceContext.MemberService.Save(member); + + var newDate = now.AddDays(10); + ServiceContext.MemberService.SetLastLogin(member.Username, newDate); + + //re-get + member = ServiceContext.MemberService.GetById(member.Id); + + Assert.That(member.LastLoginDate, Is.EqualTo(newDate).Within(1).Seconds); + Assert.That(member.UpdateDate, Is.EqualTo(newDate).Within(1).Seconds); + } + [Test] public void Can_Create_Member_With_Properties() { diff --git a/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs b/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs index 1f7e2c8084..479ba0af75 100644 --- a/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs +++ b/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs @@ -348,15 +348,9 @@ namespace Umbraco.Web.Security.Providers if (userIsOnline) { - member.LastLoginDate = DateTime.Now; - member.UpdateDate = DateTime.Now; - //don't raise events for this! It just sets the member dates, if we do raise events this will - // cause all distributed cache to execute - which will clear out some caches we don't want. - // http://issues.umbraco.org/issue/U4-3451 - // when upgrading from 7.2 to 7.3 trying to save will throw if (UmbracoVersion.Current >= new Version(7, 3, 0, 0)) - MemberService.Save(member, false); + MemberService.SetLastLogin(username, DateTime.Now); } return ConvertToMembershipUser(member); From 8b82f8e1c7873464999a3814f8b5b0a90b4ceb8a Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 31 Jul 2020 00:19:36 +1000 Subject: [PATCH 02/14] Ensures that the member resolved when setting last login date has the right data applied, finishes an outstanding TODO that will massively boost performance for sites that heavily use members (lots of logins) --- .../Repositories/IMemberRepository.cs | 2 + .../Implement/MemberRepository.cs | 34 ++++--- .../Implement/SimpleGetRepository.cs | 2 + .../Services/Implement/MemberService.cs | 9 +- .../Services/MemberServiceTests.cs | 14 +++ .../Cache/DistributedCacheExtensions.cs | 7 +- src/Umbraco.Web/Cache/MemberCacheRefresher.cs | 96 +++++++++++++++---- .../Providers/UmbracoMembershipProvider.cs | 9 +- 8 files changed, 129 insertions(+), 44 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs b/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs index 4213c7ecbe..c737c2bf66 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs @@ -7,6 +7,8 @@ namespace Umbraco.Core.Persistence.Repositories { public interface IMemberRepository : IContentRepository { + IMember GetByUsername(string username); + /// /// Finds members in a given role /// diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs index b71dd152fe..7ed7c4e2bf 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs @@ -25,6 +25,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement private readonly IMemberTypeRepository _memberTypeRepository; private readonly ITagRepository _tagRepository; private readonly IMemberGroupRepository _memberGroupRepository; + private readonly IRepositoryCachePolicy _memberByUsernameCachePolicy; public MemberRepository(IScopeAccessor scopeAccessor, AppCaches cache, ILogger logger, IMemberTypeRepository memberTypeRepository, IMemberGroupRepository memberGroupRepository, ITagRepository tagRepository, ILanguageRepository languageRepository, IRelationRepository relationRepository, IRelationTypeRepository relationTypeRepository, @@ -34,6 +35,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement _memberTypeRepository = memberTypeRepository ?? throw new ArgumentNullException(nameof(memberTypeRepository)); _tagRepository = tagRepository ?? throw new ArgumentNullException(nameof(tagRepository)); _memberGroupRepository = memberGroupRepository; + + _memberByUsernameCachePolicy = new DefaultRepositoryCachePolicy(GlobalIsolatedCache, ScopeAccessor, DefaultOptions); } protected override MemberRepository This => this; @@ -569,20 +572,6 @@ namespace Umbraco.Core.Persistence.Repositories.Implement ordering); } - private string _pagedResultsByQueryWhere; - - private string GetPagedResultsByQueryWhere() - { - if (_pagedResultsByQueryWhere == null) - _pagedResultsByQueryWhere = " AND (" - + $"({SqlSyntax.GetQuotedTableName("umbracoNode")}.{SqlSyntax.GetQuotedColumnName("text")} LIKE @0)" - + " OR " - + $"({SqlSyntax.GetQuotedTableName("cmsMember")}.{SqlSyntax.GetQuotedColumnName("LoginName")} LIKE @0)" - + ")"; - - return _pagedResultsByQueryWhere; - } - protected override string ApplySystemOrdering(ref Sql sql, Ordering ordering) { if (ordering.OrderBy.InvariantEquals("email")) @@ -672,5 +661,22 @@ namespace Umbraco.Core.Persistence.Repositories.Implement member.ResetDirtyProperties(false); return member; } + + public IMember GetByUsername(string username) + { + return _memberByUsernameCachePolicy.Get(username, PerformGetByUsername, PerformGetAllByUsername); + } + + private IMember PerformGetByUsername(string username) + { + var query = Query().Where(x => x.Username.Equals(username)); + return PerformGetByQuery(query).FirstOrDefault(); + } + + private IEnumerable PerformGetAllByUsername(params string[] usernames) + { + var query = Query().WhereIn(x => x.Username, usernames); + return PerformGetByQuery(query); + } } } diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/SimpleGetRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/SimpleGetRepository.cs index f7e59820c3..d327fba67f 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/SimpleGetRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/SimpleGetRepository.cs @@ -11,6 +11,8 @@ using Umbraco.Core.Scoping; namespace Umbraco.Core.Persistence.Repositories.Implement { + // TODO: Obsolete this, change all implementations of this like in Dictionary to just use custom Cache policies like in the member repository. + /// /// Simple abstract ReadOnly repository used to simply have PerformGet and PeformGetAll with an underlying cache /// diff --git a/src/Umbraco.Core/Services/Implement/MemberService.cs b/src/Umbraco.Core/Services/Implement/MemberService.cs index a914cfa2ea..1369b605d5 100644 --- a/src/Umbraco.Core/Services/Implement/MemberService.cs +++ b/src/Umbraco.Core/Services/Implement/MemberService.cs @@ -447,15 +447,10 @@ namespace Umbraco.Core.Services.Implement /// public IMember GetByUsername(string username) { - // TODO: Somewhere in here, whether at this level or the repository level, we need to add - // a caching mechanism since this method is used by all the membership providers and could be - // called quite a bit when dealing with members. - using (var scope = ScopeProvider.CreateScope(autoComplete: true)) { - scope.ReadLock(Constants.Locks.MemberTree); - var query = Query().Where(x => x.Username.Equals(username)); - return _memberRepository.Get(query).FirstOrDefault(); + scope.ReadLock(Constants.Locks.MemberTree); + return _memberRepository.GetByUsername(username); } } diff --git a/src/Umbraco.Tests/Services/MemberServiceTests.cs b/src/Umbraco.Tests/Services/MemberServiceTests.cs index 02b0da56e1..384ef45aa0 100644 --- a/src/Umbraco.Tests/Services/MemberServiceTests.cs +++ b/src/Umbraco.Tests/Services/MemberServiceTests.cs @@ -48,6 +48,20 @@ namespace Umbraco.Tests.Services ((MemberService)ServiceContext.MemberService).MembershipProvider = provider; } + [Test] + public void Can_Get_By_Username() + { + var now = DateTime.Now; + var memberType = ServiceContext.MemberTypeService.Get("member"); + IMember member = new Member("xname", "xemail", "xusername", "xrawpassword", memberType, true); + ServiceContext.MemberService.Save(member); + + var member2 = ServiceContext.MemberService.GetByUsername(member.Username); + + Assert.IsNotNull(member2); + Assert.AreEqual(member.Email, member2.Email); + } + [Test] public void Can_Set_Last_Login_Date() { diff --git a/src/Umbraco.Web/Cache/DistributedCacheExtensions.cs b/src/Umbraco.Web/Cache/DistributedCacheExtensions.cs index b00a4818f6..f360d37d03 100644 --- a/src/Umbraco.Web/Cache/DistributedCacheExtensions.cs +++ b/src/Umbraco.Web/Cache/DistributedCacheExtensions.cs @@ -128,15 +128,16 @@ namespace Umbraco.Web.Cache public static void RefreshMemberCache(this DistributedCache dc, params IMember[] members) { - dc.Refresh(MemberCacheRefresher.UniqueId, x => x.Id, members); + if (members.Length == 0) return; + dc.RefreshByPayload(MemberCacheRefresher.UniqueId, members.Select(x => new MemberCacheRefresher.JsonPayload(x.Id, x.Username))); } public static void RemoveMemberCache(this DistributedCache dc, params IMember[] members) { - dc.Remove(MemberCacheRefresher.UniqueId, x => x.Id, members); + if (members.Length == 0) return; + dc.RefreshByPayload(MemberCacheRefresher.UniqueId, members.Select(x => new MemberCacheRefresher.JsonPayload(x.Id, x.Username))); } - #endregion #region MemberGroupCache diff --git a/src/Umbraco.Web/Cache/MemberCacheRefresher.cs b/src/Umbraco.Web/Cache/MemberCacheRefresher.cs index 1565b1c849..736a858af3 100644 --- a/src/Umbraco.Web/Cache/MemberCacheRefresher.cs +++ b/src/Umbraco.Web/Cache/MemberCacheRefresher.cs @@ -1,4 +1,6 @@ -using System; +using Newtonsoft.Json; +using System; +using System.Collections.Generic; using Umbraco.Core.Cache; using Umbraco.Core.Models; using Umbraco.Core.Persistence.Repositories; @@ -7,14 +9,30 @@ using Umbraco.Core.Services; namespace Umbraco.Web.Cache { - public sealed class MemberCacheRefresher : TypedCacheRefresherBase + public sealed class MemberCacheRefresher : PayloadCacheRefresherBase { private readonly IdkMap _idkMap; + private readonly LegacyMemberCacheRefresher _legacyMemberRefresher; public MemberCacheRefresher(AppCaches appCaches, IdkMap idkMap) : base(appCaches) { _idkMap = idkMap; + _legacyMemberRefresher = new LegacyMemberCacheRefresher(this, appCaches); + } + + public class JsonPayload + { + [JsonConstructor] + public JsonPayload(int id, string username) + { + Id = id; + Username = username; + } + + public int Id { get; } + public string Username { get; } + } #region Define @@ -31,38 +49,45 @@ namespace Umbraco.Web.Cache #region Refresher + public override void Refresh(JsonPayload[] payloads) + { + ClearCache(payloads); + base.Refresh(payloads); + } + public override void Refresh(int id) { - ClearCache(id); + ClearCache(new JsonPayload(id, null)); base.Refresh(id); } public override void Remove(int id) { - ClearCache(id); + ClearCache(new JsonPayload(id, null)); base.Remove(id); } - public override void Refresh(IMember instance) - { - ClearCache(instance.Id); - base.Refresh(instance); - } + [Obsolete("This is no longer used and will be removed from the codebase in the future")] + public void Refresh(IMember instance) => _legacyMemberRefresher.Refresh(instance); - public override void Remove(IMember instance) - { - ClearCache(instance.Id); - base.Remove(instance); - } + [Obsolete("This is no longer used and will be removed from the codebase in the future")] + public void Remove(IMember instance) => _legacyMemberRefresher.Remove(instance); - private void ClearCache(int id) + private void ClearCache(params JsonPayload[] payloads) { - _idkMap.ClearCache(id); AppCaches.ClearPartialViewCache(); - var memberCache = AppCaches.IsolatedCaches.Get(); - if (memberCache) - memberCache.Result.Clear(RepositoryCacheKeys.GetKey(id)); + + foreach (var p in payloads) + { + _idkMap.ClearCache(p.Id); + if (memberCache) + { + memberCache.Result.Clear(RepositoryCacheKeys.GetKey(p.Id)); + memberCache.Result.Clear(RepositoryCacheKeys.GetKey(p.Username)); + } + } + } #endregion @@ -75,5 +100,38 @@ namespace Umbraco.Web.Cache } #endregion + + #region Backwards Compat + + // TODO: this is here purely for backwards compat but should be removed in netcore + private class LegacyMemberCacheRefresher : TypedCacheRefresherBase + { + private readonly MemberCacheRefresher _parent; + + public LegacyMemberCacheRefresher(MemberCacheRefresher parent, AppCaches appCaches) : base(appCaches) + { + _parent = parent; + } + + public override Guid RefresherUniqueId => _parent.RefresherUniqueId; + + public override string Name => _parent.Name; + + protected override MemberCacheRefresher This => _parent; + + public override void Refresh(IMember instance) + { + _parent.ClearCache(new JsonPayload(instance.Id, instance.Username)); + base.Refresh(instance.Id); + } + + public override void Remove(IMember instance) + { + _parent.ClearCache(new JsonPayload(instance.Id, instance.Username)); + base.Remove(instance); + } + } + + #endregion } } diff --git a/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs b/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs index 479ba0af75..46ae732f97 100644 --- a/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs +++ b/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs @@ -350,7 +350,14 @@ namespace Umbraco.Web.Security.Providers { // when upgrading from 7.2 to 7.3 trying to save will throw if (UmbracoVersion.Current >= new Version(7, 3, 0, 0)) - MemberService.SetLastLogin(username, DateTime.Now); + { + var now = DateTime.Now; + // update the database data directly instead of a full member save which requires DB locks + MemberService.SetLastLogin(username, now); + member.LastLoginDate = now; + member.UpdateDate = now; + } + } return ConvertToMembershipUser(member); From 5cf94a76392632b3b920a66cea2bc587c54baed8 Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 10 Aug 2020 22:29:04 +1000 Subject: [PATCH 03/14] Updates member repository to take a lock on the property value rows being updated --- .../Repositories/Implement/MemberRepository.cs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs index 7ed7c4e2bf..782bf5fc45 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs @@ -511,6 +511,14 @@ namespace Umbraco.Core.Persistence.Repositories.Implement /// public void SetLastLogin(string username, DateTime date) { + // Important - these queries are designed to execute without an exclusive WriteLock taken in our distributed lock + // table. However due to the data that we are updating which relies on version data we cannot update this data + // without taking some locks, otherwise we'll end up with strange situations because when a member is updated, that operation + // deletes and re-inserts all property data. So if there are concurrent transactions, one deleting and re-inserting and another trying + // to update there can be problems. This is only an issue for cmsPropertyData, not umbracoContentVersion because that table just + // maintains a single row and it isn't deleted/re-inserted. + // So the important part here is the ForUpdate() call on the select to fetch the property data to update. + // Update the cms property value for the member var sqlSelectTemplateProperty = SqlContext.Templates.Get("Umbraco.Core.MemberRepository.SetLastLogin1", s => s @@ -522,7 +530,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement .InnerJoin().On((l, r) => l.NodeId == r.NodeId) .Where(x => x.NodeObjectType == SqlTemplate.Arg("nodeObjectType")) .Where(x => x.Alias == SqlTemplate.Arg("propertyTypeAlias")) - .Where(x => x.LoginName == SqlTemplate.Arg("username"))); + .Where(x => x.LoginName == SqlTemplate.Arg("username")) + .ForUpdate()); var sqlSelectProperty = sqlSelectTemplateProperty.Sql(Constants.ObjectTypes.Member, Constants.Conventions.Member.LastLoginDate, username); var update = Sql() From 295db967debb772b20934373c0ec5a88d2b61e4b Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 10 Aug 2020 22:55:45 +1000 Subject: [PATCH 04/14] Updates member repository to select/update instead of delete/insert for property data --- .../Implement/MemberRepository.cs | 19 ++++++++++++++---- .../Services/MemberServiceTests.cs | 20 ++++++++++++++++++- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs index 782bf5fc45..1a6513dbcb 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs @@ -385,12 +385,23 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (changedCols.Count > 0) Database.Update(dto, changedCols); - // replace the property data - var deletePropertyDataSql = SqlContext.Sql().Delete().Where(x => x.VersionId == member.VersionId); - Database.Execute(deletePropertyDataSql); + // replace the property data, lookup the data to update with a UPDLOCK + var propDataSql = SqlContext.Sql().Select("*").From().Where(x => x.VersionId == member.VersionId).ForUpdate(); + var existingPropData = Database.Fetch(propDataSql).ToDictionary(x => x.PropertyTypeId); var propertyDataDtos = PropertyFactory.BuildDtos(member.ContentType.Variations, member.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); foreach (var propertyDataDto in propertyDataDtos) - Database.Insert(propertyDataDto); + { + // Check if this already exists and update, else insert a new one + if (existingPropData.TryGetValue(propertyDataDto.PropertyTypeId, out var propData)) + { + propertyDataDto.Id = propData.Id; + Database.Update(propertyDataDto); + } + else + { + Database.Insert(propertyDataDto); + } + } SetEntityTags(entity, _tagRepository); diff --git a/src/Umbraco.Tests/Services/MemberServiceTests.cs b/src/Umbraco.Tests/Services/MemberServiceTests.cs index 384ef45aa0..ce84c2701b 100644 --- a/src/Umbraco.Tests/Services/MemberServiceTests.cs +++ b/src/Umbraco.Tests/Services/MemberServiceTests.cs @@ -48,10 +48,28 @@ namespace Umbraco.Tests.Services ((MemberService)ServiceContext.MemberService).MembershipProvider = provider; } + [Test] + public void Can_Update_Member_Property_Value() + { + IMemberType memberType = MockedContentTypes.CreateSimpleMemberType(); + ServiceContext.MemberTypeService.Save(memberType); + IMember member = MockedMember.CreateSimpleMember(memberType, "hello", "helloworld@test123.com", "hello", "hello"); + member.SetValue("title", "title of mine"); + ServiceContext.MemberService.Save(member); + + // re-get + member = ServiceContext.MemberService.GetById(member.Id); + member.SetValue("title", "another title of mine"); + ServiceContext.MemberService.Save(member); + + // re-get + member = ServiceContext.MemberService.GetById(member.Id); + Assert.AreEqual("another title of mine", member.GetValue("title")); + } + [Test] public void Can_Get_By_Username() { - var now = DateTime.Now; var memberType = ServiceContext.MemberTypeService.Get("member"); IMember member = new Member("xname", "xemail", "xusername", "xrawpassword", memberType, true); ServiceContext.MemberService.Save(member); From 2d3d919c31da7405cb1b197563b7b0828171b43c Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 10 Aug 2020 22:57:34 +1000 Subject: [PATCH 05/14] adds notes --- .../Persistence/Repositories/Implement/MemberRepository.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs index 1a6513dbcb..687e35aef3 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs @@ -385,7 +385,11 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (changedCols.Count > 0) Database.Update(dto, changedCols); - // replace the property data, lookup the data to update with a UPDLOCK + // Replace the property data + // Lookup the data to update with a UPDLOCK (using ForUpdate()) this is because we have another method that doesn't take an explicit WriteLock + // in SetLastLogin which is called very often and we want to avoid the lock timeout for the explicit lock table but we still need to ensure atomic + // operations between that method and this one. + var propDataSql = SqlContext.Sql().Select("*").From().Where(x => x.VersionId == member.VersionId).ForUpdate(); var existingPropData = Database.Fetch(propDataSql).ToDictionary(x => x.PropertyTypeId); var propertyDataDtos = PropertyFactory.BuildDtos(member.ContentType.Variations, member.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); From 9fc3699a48c9f2e60e51110037ae8bb823a853f9 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 11 Aug 2020 11:52:36 +1000 Subject: [PATCH 06/14] Reduces the Member update calls when logging a member in --- src/Umbraco.Web/Security/MembershipHelper.cs | 37 ++++++------------- .../Providers/UmbracoMembershipProvider.cs | 21 +++++++++-- 2 files changed, 28 insertions(+), 30 deletions(-) diff --git a/src/Umbraco.Web/Security/MembershipHelper.cs b/src/Umbraco.Web/Security/MembershipHelper.cs index f74897d565..aa0623e9e6 100644 --- a/src/Umbraco.Web/Security/MembershipHelper.cs +++ b/src/Umbraco.Web/Security/MembershipHelper.cs @@ -261,8 +261,9 @@ namespace Umbraco.Web.Security { return false; } - //Set member online - var member = provider.GetUser(username, true); + // Get the member, do not set to online - this is done implicitly as part of ValidateUser which is consistent with + // how the .NET framework SqlMembershipProvider works. Passing in true will just cause more unnecessary SQL queries/locks. + var member = provider.GetUser(username, false); if (member == null) { //this should not happen @@ -778,33 +779,17 @@ namespace Umbraco.Web.Security /// private IMember GetCurrentPersistedMember() { - return _appCaches.RequestCache.GetCacheItem( - GetCacheKey("GetCurrentPersistedMember"), () => - { - var provider = _membershipProvider; + var provider = _membershipProvider; - if (provider.IsUmbracoMembershipProvider() == false) - { - throw new NotSupportedException("An IMember model can only be retrieved when using the built-in Umbraco membership providers"); - } - var username = provider.GetCurrentUserName(); - var member = _memberService.GetByUsername(username); - return member; - }); - } - - private static string GetCacheKey(string key, params object[] additional) - { - var sb = new StringBuilder(); - sb.Append(typeof (MembershipHelper).Name); - sb.Append("-"); - sb.Append(key); - foreach (var s in additional) + if (provider.IsUmbracoMembershipProvider() == false) { - sb.Append("-"); - sb.Append(s); + throw new NotSupportedException("An IMember model can only be retrieved when using the built-in Umbraco membership providers"); } - return sb.ToString(); + var username = provider.GetCurrentUserName(); + + // The result of this is cached by the MemberRepository + var member = _memberService.GetByUsername(username); + return member; } } diff --git a/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs b/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs index 46ae732f97..bf9ee654c4 100644 --- a/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs +++ b/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs @@ -556,6 +556,8 @@ namespace Umbraco.Web.Security.Providers var authenticated = CheckPassword(password, member.RawPasswordValue); + var requiresFullSave = false; + if (authenticated == false) { // TODO: Increment login attempts - lock if too many. @@ -575,6 +577,8 @@ namespace Umbraco.Web.Security.Providers { Current.Logger.Info("Login attempt failed for username {Username} from IP address {IpAddress}", username, GetCurrentRequestIpAddress()); } + + requiresFullSave = true; } else { @@ -582,6 +586,7 @@ namespace Umbraco.Web.Security.Providers { //we have successfully logged in, reset the AccessFailedCount member.FailedPasswordAttempts = 0; + requiresFullSave = true; } member.LastLoginDate = DateTime.Now; @@ -589,15 +594,23 @@ namespace Umbraco.Web.Security.Providers Current.Logger.Info("Login attempt succeeded for username {Username} from IP address {IpAddress}", username, GetCurrentRequestIpAddress()); } - //don't raise events for this! It just sets the member dates, if we do raise events this will + // don't raise events for this! It just sets the member dates, if we do raise events this will // cause all distributed cache to execute - which will clear out some caches we don't want. // http://issues.umbraco.org/issue/U4-3451 // TODO: In v8 we aren't going to have an overload to disable events, so we'll need to make a different method // for this type of thing (i.e. UpdateLastLogin or similar). - // when upgrading from 7.2 to 7.3 trying to save will throw - if (UmbracoVersion.Current >= new Version(7, 3, 0, 0)) - MemberService.Save(member, false); + if (requiresFullSave) + { + // when upgrading from 7.2 to 7.3 trying to save will throw + if (UmbracoVersion.Current >= new Version(7, 3, 0, 0)) + MemberService.Save(member, false); + } + else + { + // set the last login date without full save (fast, no locks) + MemberService.SetLastLogin(member.Username, member.LastLoginDate); + } return new ValidateUserResult { From 9a1de6468b369588fb2fbf9473f2d7d81cc1e785 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 19 Aug 2020 13:33:49 +1000 Subject: [PATCH 07/14] notes and unit test, just wanted to see if this query could be improved but it can't --- .../Repositories/Implement/UserRepository.cs | 8 +-- .../Repositories/UserRepositoryTest.cs | 61 ++++++++++++++----- 2 files changed, 50 insertions(+), 19 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs index 3be5102b83..4721037674 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs @@ -168,10 +168,7 @@ ORDER BY colName"; } public Guid CreateLoginSession(int userId, string requestingIpAddress, bool cleanStaleSessions = true) - { - // TODO: I know this doesn't follow the normal repository conventions which would require us to create a UserSessionRepository - //and also business logic models for these objects but that's just so overkill for what we are doing - //and now that everything is properly in a transaction (Scope) there doesn't seem to be much reason for using that anymore + { var now = DateTime.UtcNow; var dto = new UserLoginDto { @@ -201,13 +198,14 @@ ORDER BY colName"; // that query is going to run a *lot*, make it a template var t = SqlContext.Templates.Get("Umbraco.Core.UserRepository.ValidateLoginSession", s => s .Select() + .SelectTop(1) .From() .Where(x => x.SessionId == SqlTemplate.Arg("sessionId")) .ForUpdate()); var sql = t.Sql(sessionId); - var found = Database.Query(sql).FirstOrDefault(); + var found = Database.FirstOrDefault(sql); if (found == null || found.UserId != userId || found.LoggedOutUtc.HasValue) return false; diff --git a/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs b/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs index bbefb79f6b..b2efbd34b8 100644 --- a/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs +++ b/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs @@ -16,6 +16,7 @@ using Umbraco.Tests.Testing; using Umbraco.Core.Persistence; using Umbraco.Core.PropertyEditors; using System; +using Umbraco.Core.Persistence.Dtos; namespace Umbraco.Tests.Persistence.Repositories { @@ -76,12 +77,44 @@ namespace Umbraco.Tests.Persistence.Repositories return new UserGroupRepository(accessor, AppCaches.Disabled, Logger); } + [Test] + public void Validate_Login_Session() + { + // Arrange + var provider = TestObjects.GetScopeProvider(Logger); + var user = MockedUser.CreateUser(); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + repository.Save(user); + } + + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + var sessionId = repository.CreateLoginSession(user.Id, "1.2.3.4"); + + // manually update this record to be in the past + scope.Database.Execute(SqlContext.Sql() + .Update(u => u.Set(x => x.LoggedOutUtc, DateTime.UtcNow.AddDays(-100))) + .Where(x => x.SessionId == sessionId)); + + var isValid = repository.ValidateLoginSession(user.Id, sessionId); + Assert.IsFalse(isValid); + + // create a new one + sessionId = repository.CreateLoginSession(user.Id, "1.2.3.4"); + isValid = repository.ValidateLoginSession(user.Id, sessionId); + Assert.IsTrue(isValid); + } + } + [Test] public void Can_Perform_Add_On_UserRepository() { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -101,7 +134,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -125,7 +158,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -150,7 +183,7 @@ namespace Umbraco.Tests.Persistence.Repositories // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var userRepository = CreateRepository(provider); var contentRepository = CreateContentRepository(provider, out var contentTypeRepo); @@ -209,7 +242,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -237,7 +270,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); var userGroupRepository = CreateUserGroupRepository(provider); @@ -260,7 +293,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -280,7 +313,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -301,7 +334,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -322,7 +355,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -341,7 +374,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -360,7 +393,7 @@ namespace Umbraco.Tests.Persistence.Repositories public void Can_Get_Paged_Results_By_Query_And_Filter_And_Groups() { var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -393,7 +426,7 @@ namespace Umbraco.Tests.Persistence.Repositories public void Can_Get_Paged_Results_With_Filter_And_Groups() { var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -426,7 +459,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); var userGroupRepository = CreateUserGroupRepository(provider); From 6e08a0396ea5fc65861c88ea40c69747a321bbfb Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 31 Aug 2020 23:41:19 +1000 Subject: [PATCH 08/14] Umbraco to re-index data on background thread Fixes issue with scopes being disposed or referenced incorrectly due to yield returns as this can capture a scope in the enumerator which will get passed to a background thread in Examine and cause some issues. We saw this issue in netcore but the issue must still exist in v8 but we don't visibly see it for some reason. The other issue is that the ValueSet lookup for content was done 3x when an IContent is saved and it should only be done 2x, one for published, one for unpublished. The other issue is that the data lookups to build a ValueSet are intended to be done on a background thread. This is the case in v7 because the IEnumerable is lazy and passed all the way down to Examine's background thread but this doesn't occur in v8 because we need to iterate/split that value set before it's sent to Examine so the ValueSet is eagerly built within the request. We can easily resolve this by using the background task manager and just pushing a task when IContent/IMedia/IMember is saved. This will return the execution to the UI quicker. --- src/Umbraco.Examine/UmbracoContentIndex.cs | 6 +- src/Umbraco.Web/Scheduling/SimpleTask.cs | 32 +++++++++ src/Umbraco.Web/Search/ExamineComponent.cs | 84 +++++++++++++++------- src/Umbraco.Web/Umbraco.Web.csproj | 1 + 4 files changed, 95 insertions(+), 28 deletions(-) create mode 100644 src/Umbraco.Web/Scheduling/SimpleTask.cs diff --git a/src/Umbraco.Examine/UmbracoContentIndex.cs b/src/Umbraco.Examine/UmbracoContentIndex.cs index e266ca789d..67e430b4e9 100644 --- a/src/Umbraco.Examine/UmbracoContentIndex.cs +++ b/src/Umbraco.Examine/UmbracoContentIndex.cs @@ -62,7 +62,7 @@ namespace Umbraco.Examine /// protected override void PerformIndexItems(IEnumerable values, Action onComplete) { - //We don't want to re-enumerate this list, but we need to split it into 2x enumerables: invalid and valid items. + // We don't want to re-enumerate this list, but we need to split it into 2x enumerables: invalid and valid items. // The Invalid items will be deleted, these are items that have invalid paths (i.e. moved to the recycle bin, etc...) // Then we'll index the Value group all together. // We return 0 or 1 here so we can order the results and do the invalid first and then the valid. @@ -80,7 +80,7 @@ namespace Umbraco.Examine || !validator.ValidateProtectedContent(path, v.Category)) ? 0 : 1; - }); + }).ToList(); var hasDeletes = false; var hasUpdates = false; @@ -99,7 +99,7 @@ namespace Umbraco.Examine { hasUpdates = true; //these are the valid ones, so just index them all at once - base.PerformIndexItems(group, onComplete); + base.PerformIndexItems(group.ToList(), onComplete); } } diff --git a/src/Umbraco.Web/Scheduling/SimpleTask.cs b/src/Umbraco.Web/Scheduling/SimpleTask.cs new file mode 100644 index 0000000000..a8603915b0 --- /dev/null +++ b/src/Umbraco.Web/Scheduling/SimpleTask.cs @@ -0,0 +1,32 @@ +using System; +using System.Threading; +using System.Threading.Tasks; + +namespace Umbraco.Web.Scheduling +{ + /// + /// A simple task that executes a delegate synchronously + /// + internal class SimpleTask : IBackgroundTask + { + private readonly Action _action; + + public SimpleTask(Action action) + { + _action = action; + } + + public bool IsAsync => false; + + public void Run() => _action(); + + public Task RunAsync(CancellationToken token) + { + throw new NotImplementedException(); + } + + public void Dispose() + { + } + } +} diff --git a/src/Umbraco.Web/Search/ExamineComponent.cs b/src/Umbraco.Web/Search/ExamineComponent.cs index 149b4d1436..6f1a7cc960 100644 --- a/src/Umbraco.Web/Search/ExamineComponent.cs +++ b/src/Umbraco.Web/Search/ExamineComponent.cs @@ -17,9 +17,13 @@ using Umbraco.Core.Persistence.DatabaseModelDefinitions; using Examine.LuceneEngine.Directories; using Umbraco.Core.Composing; using System.ComponentModel; +using System.Threading; +using Umbraco.Web.Scheduling; namespace Umbraco.Web.Search { + + public sealed class ExamineComponent : Umbraco.Core.Composing.IComponent { private readonly IExamineManager _examineManager; @@ -34,7 +38,8 @@ namespace Umbraco.Web.Search private readonly IMainDom _mainDom; private readonly IProfilingLogger _logger; private readonly IUmbracoIndexesCreator _indexCreator; - + private readonly BackgroundTaskRunner _indexItemTaskRunner; + // the default enlist priority is 100 // enlist with a lower priority to ensure that anything "default" runs after us @@ -62,6 +67,7 @@ namespace Umbraco.Web.Search _mainDom = mainDom; _logger = profilingLogger; _indexCreator = indexCreator; + _indexItemTaskRunner = new BackgroundTaskRunner(_logger); } public void Initialize() @@ -574,12 +580,18 @@ namespace Umbraco.Web.Search } } + /// + /// An action that will execute at the end of the Scope being completed + /// private abstract class DeferedAction { public virtual void Execute() { } } + /// + /// Re-indexes an item on a background thread + /// private class DeferedReIndexForContent : DeferedAction { private readonly ExamineComponent _examineComponent; @@ -600,21 +612,32 @@ namespace Umbraco.Web.Search public static void Execute(ExamineComponent examineComponent, IContent content, bool isPublished) { - foreach (var index in examineComponent._examineManager.Indexes.OfType() - //filter the indexers - .Where(x => isPublished || !x.PublishedValuesOnly) - .Where(x => x.EnableDefaultEventHandler)) + // perform the ValueSet lookup on a background thread + examineComponent._indexItemTaskRunner.Add(new SimpleTask(() => { - //for content we have a different builder for published vs unpublished - var builder = index.PublishedValuesOnly - ? examineComponent._publishedContentValueSetBuilder - : (IValueSetBuilder)examineComponent._contentValueSetBuilder; + // for content we have a different builder for published vs unpublished + // we don't want to build more value sets than is needed so we'll lazily build 2 one for published one for non-published + var builders = new Dictionary>> + { + [true] = new Lazy>(() => examineComponent._publishedContentValueSetBuilder.GetValueSets(content).ToList()), + [false] = new Lazy>(() => examineComponent._contentValueSetBuilder.GetValueSets(content).ToList()) + }; - index.IndexItems(builder.GetValueSets(content)); - } + foreach (var index in examineComponent._examineManager.Indexes.OfType() + //filter the indexers + .Where(x => isPublished || !x.PublishedValuesOnly) + .Where(x => x.EnableDefaultEventHandler)) + { + var valueSet = builders[index.PublishedValuesOnly].Value; + index.IndexItems(valueSet); + } + })); } } + /// + /// Re-indexes an item on a background thread + /// private class DeferedReIndexForMedia : DeferedAction { private readonly ExamineComponent _examineComponent; @@ -635,18 +658,25 @@ namespace Umbraco.Web.Search public static void Execute(ExamineComponent examineComponent, IMedia media, bool isPublished) { - var valueSet = examineComponent._mediaValueSetBuilder.GetValueSets(media).ToList(); - - foreach (var index in examineComponent._examineManager.Indexes.OfType() - //filter the indexers - .Where(x => isPublished || !x.PublishedValuesOnly) - .Where(x => x.EnableDefaultEventHandler)) + // perform the ValueSet lookup on a background thread + examineComponent._indexItemTaskRunner.Add(new SimpleTask(() => { - index.IndexItems(valueSet); - } + var valueSet = examineComponent._mediaValueSetBuilder.GetValueSets(media).ToList(); + + foreach (var index in examineComponent._examineManager.Indexes.OfType() + //filter the indexers + .Where(x => isPublished || !x.PublishedValuesOnly) + .Where(x => x.EnableDefaultEventHandler)) + { + index.IndexItems(valueSet); + } + })); } } + /// + /// Re-indexes an item on a background thread + /// private class DeferedReIndexForMember : DeferedAction { private readonly ExamineComponent _examineComponent; @@ -665,13 +695,17 @@ namespace Umbraco.Web.Search public static void Execute(ExamineComponent examineComponent, IMember member) { - var valueSet = examineComponent._memberValueSetBuilder.GetValueSets(member).ToList(); - foreach (var index in examineComponent._examineManager.Indexes.OfType() - //filter the indexers - .Where(x => x.EnableDefaultEventHandler)) + // perform the ValueSet lookup on a background thread + examineComponent._indexItemTaskRunner.Add(new SimpleTask(() => { - index.IndexItems(valueSet); - } + var valueSet = examineComponent._memberValueSetBuilder.GetValueSets(member).ToList(); + foreach (var index in examineComponent._examineManager.Indexes.OfType() + //filter the indexers + .Where(x => x.EnableDefaultEventHandler)) + { + index.IndexItems(valueSet); + } + })); } } diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index ed457b1364..35a00d06c2 100755 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -247,6 +247,7 @@ + From 664504833bce98d63541d5876ddcf11f7983438d Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 31 Aug 2020 23:57:26 +1000 Subject: [PATCH 09/14] Move scope management outside of iterator method --- src/Umbraco.Examine/ContentValueSetBuilder.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Umbraco.Examine/ContentValueSetBuilder.cs b/src/Umbraco.Examine/ContentValueSetBuilder.cs index b8477a9047..dfd64fcd47 100644 --- a/src/Umbraco.Examine/ContentValueSetBuilder.cs +++ b/src/Umbraco.Examine/ContentValueSetBuilder.cs @@ -60,6 +60,11 @@ namespace Umbraco.Examine scope.Complete(); } + return GetValueSetsEnumerable(content, creatorIds, writerIds); + } + + private IEnumerable GetValueSetsEnumerable(IContent[] content, Dictionary creatorIds, Dictionary writerIds) + { // TODO: There is a lot of boxing going on here and ultimately all values will be boxed by Lucene anyways // but I wonder if there's a way to reduce the boxing that we have to do or if it will matter in the end since // Lucene will do it no matter what? One idea was to create a `FieldValue` struct which would contain `object`, `object[]`, `ValueType` and `ValueType[]` From 1ab77adc8af2c120f353d37cba7e34dd40220787 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 4 Sep 2020 13:25:56 +1000 Subject: [PATCH 10/14] Ensure event handlers are unsubscribed in Core Components --- .../Compose/AuditEventsComponent.cs | 14 ++- .../Compose/RelateOnCopyComponent.cs | 4 +- .../Compose/RelateOnTrashComponent.cs | 7 +- .../Compose/ModelsBuilderComponent.cs | 44 ++++--- .../BackOfficeUserAuditEventsComponent.cs | 14 ++- .../Compose/NotificationsComponent.cs | 110 ++++++++++++------ .../Compose/PublicAccessComponent.cs | 4 +- .../Logging/WebProfilerComponent.cs | 16 ++- .../PropertyEditorsComponent.cs | 61 ++++++---- .../Routing/RedirectTrackingComponent.cs | 7 +- src/Umbraco.Web/Search/ExamineComponent.cs | 8 +- 11 files changed, 203 insertions(+), 86 deletions(-) diff --git a/src/Umbraco.Core/Compose/AuditEventsComponent.cs b/src/Umbraco.Core/Compose/AuditEventsComponent.cs index 453fd6314a..170d81577f 100644 --- a/src/Umbraco.Core/Compose/AuditEventsComponent.cs +++ b/src/Umbraco.Core/Compose/AuditEventsComponent.cs @@ -42,7 +42,19 @@ namespace Umbraco.Core.Compose } public void Terminate() - { } + { + UserService.SavedUserGroup -= OnSavedUserGroupWithUsers; + + UserService.SavedUser -= OnSavedUser; + UserService.DeletedUser -= OnDeletedUser; + UserService.UserGroupPermissionsAssigned -= UserGroupPermissionAssigned; + + MemberService.Saved -= OnSavedMember; + MemberService.Deleted -= OnDeletedMember; + MemberService.AssignedRoles -= OnAssignedRoles; + MemberService.RemovedRoles -= OnRemovedRoles; + MemberService.Exported -= OnMemberExported; + } internal static IUser UnknownUser => new User { Id = Constants.Security.UnknownUserId, Name = Constants.Security.UnknownUserName, Email = "" }; diff --git a/src/Umbraco.Core/Compose/RelateOnCopyComponent.cs b/src/Umbraco.Core/Compose/RelateOnCopyComponent.cs index b56ff8b87e..a9f9ed1ee0 100644 --- a/src/Umbraco.Core/Compose/RelateOnCopyComponent.cs +++ b/src/Umbraco.Core/Compose/RelateOnCopyComponent.cs @@ -14,7 +14,9 @@ namespace Umbraco.Core.Compose } public void Terminate() - { } + { + ContentService.Copied -= ContentServiceCopied; + } private static void ContentServiceCopied(IContentService sender, Events.CopyEventArgs e) { diff --git a/src/Umbraco.Core/Compose/RelateOnTrashComponent.cs b/src/Umbraco.Core/Compose/RelateOnTrashComponent.cs index 4e01c50fc6..a216a584ba 100644 --- a/src/Umbraco.Core/Compose/RelateOnTrashComponent.cs +++ b/src/Umbraco.Core/Compose/RelateOnTrashComponent.cs @@ -18,7 +18,12 @@ namespace Umbraco.Core.Compose } public void Terminate() - { } + { + ContentService.Moved -= ContentService_Moved; + ContentService.Trashed -= ContentService_Trashed; + MediaService.Moved -= MediaService_Moved; + MediaService.Trashed -= MediaService_Trashed; + } private static void ContentService_Moved(IContentService sender, MoveEventArgs e) { diff --git a/src/Umbraco.ModelsBuilder.Embedded/Compose/ModelsBuilderComponent.cs b/src/Umbraco.ModelsBuilder.Embedded/Compose/ModelsBuilderComponent.cs index 0e41c9ac62..7434131dc2 100644 --- a/src/Umbraco.ModelsBuilder.Embedded/Compose/ModelsBuilderComponent.cs +++ b/src/Umbraco.ModelsBuilder.Embedded/Compose/ModelsBuilderComponent.cs @@ -50,32 +50,38 @@ namespace Umbraco.ModelsBuilder.Embedded.Compose } public void Terminate() - { } + { + ServerVariablesParser.Parsing -= ServerVariablesParser_Parsing; + ContentModelBinder.ModelBindingException -= ContentModelBinder_ModelBindingException; + FileService.SavingTemplate -= FileService_SavingTemplate; + } private void InstallServerVars() { // register our url - for the backoffice api - ServerVariablesParser.Parsing += (sender, serverVars) => - { - if (!serverVars.ContainsKey("umbracoUrls")) - throw new ArgumentException("Missing umbracoUrls."); - var umbracoUrlsObject = serverVars["umbracoUrls"]; - if (umbracoUrlsObject == null) - throw new ArgumentException("Null umbracoUrls"); - if (!(umbracoUrlsObject is Dictionary umbracoUrls)) - throw new ArgumentException("Invalid umbracoUrls"); + ServerVariablesParser.Parsing += ServerVariablesParser_Parsing; + } - if (!serverVars.ContainsKey("umbracoPlugins")) - throw new ArgumentException("Missing umbracoPlugins."); - if (!(serverVars["umbracoPlugins"] is Dictionary umbracoPlugins)) - throw new ArgumentException("Invalid umbracoPlugins"); + private void ServerVariablesParser_Parsing(object sender, Dictionary serverVars) + { + if (!serverVars.ContainsKey("umbracoUrls")) + throw new ArgumentException("Missing umbracoUrls."); + var umbracoUrlsObject = serverVars["umbracoUrls"]; + if (umbracoUrlsObject == null) + throw new ArgumentException("Null umbracoUrls"); + if (!(umbracoUrlsObject is Dictionary umbracoUrls)) + throw new ArgumentException("Invalid umbracoUrls"); - if (HttpContext.Current == null) throw new InvalidOperationException("HttpContext is null"); - var urlHelper = new UrlHelper(new RequestContext(new HttpContextWrapper(HttpContext.Current), new RouteData())); + if (!serverVars.ContainsKey("umbracoPlugins")) + throw new ArgumentException("Missing umbracoPlugins."); + if (!(serverVars["umbracoPlugins"] is Dictionary umbracoPlugins)) + throw new ArgumentException("Invalid umbracoPlugins"); - umbracoUrls["modelsBuilderBaseUrl"] = urlHelper.GetUmbracoApiServiceBaseUrl(controller => controller.BuildModels()); - umbracoPlugins["modelsBuilder"] = GetModelsBuilderSettings(); - }; + if (HttpContext.Current == null) throw new InvalidOperationException("HttpContext is null"); + var urlHelper = new UrlHelper(new RequestContext(new HttpContextWrapper(HttpContext.Current), new RouteData())); + + umbracoUrls["modelsBuilderBaseUrl"] = urlHelper.GetUmbracoApiServiceBaseUrl(controller => controller.BuildModels()); + umbracoPlugins["modelsBuilder"] = GetModelsBuilderSettings(); } private Dictionary GetModelsBuilderSettings() diff --git a/src/Umbraco.Web/Compose/BackOfficeUserAuditEventsComponent.cs b/src/Umbraco.Web/Compose/BackOfficeUserAuditEventsComponent.cs index 4244d575af..d0341ec3b6 100644 --- a/src/Umbraco.Web/Compose/BackOfficeUserAuditEventsComponent.cs +++ b/src/Umbraco.Web/Compose/BackOfficeUserAuditEventsComponent.cs @@ -35,7 +35,19 @@ namespace Umbraco.Web.Compose } public void Terminate() - { } + { + //BackOfficeUserManager.AccountLocked -= ; + //BackOfficeUserManager.AccountUnlocked -= ; + BackOfficeUserManager.ForgotPasswordRequested -= OnForgotPasswordRequest; + BackOfficeUserManager.ForgotPasswordChangedSuccess -= OnForgotPasswordChange; + BackOfficeUserManager.LoginFailed -= OnLoginFailed; + //BackOfficeUserManager.LoginRequiresVerification -= ; + BackOfficeUserManager.LoginSuccess -= OnLoginSuccess; + BackOfficeUserManager.LogoutSuccess -= OnLogoutSuccess; + BackOfficeUserManager.PasswordChanged -= OnPasswordChanged; + BackOfficeUserManager.PasswordReset -= OnPasswordReset; + //BackOfficeUserManager.ResetAccessFailedCount -= ; + } private IUser GetPerformingUser(int userId) { diff --git a/src/Umbraco.Web/Compose/NotificationsComponent.cs b/src/Umbraco.Web/Compose/NotificationsComponent.cs index 6fcc1269cb..24c7992dd2 100644 --- a/src/Umbraco.Web/Compose/NotificationsComponent.cs +++ b/src/Umbraco.Web/Compose/NotificationsComponent.cs @@ -32,42 +32,78 @@ namespace Umbraco.Web.Compose public void Initialize() { //Send notifications for the send to publish action - ContentService.SentToPublish += (sender, args) => _notifier.Notify(_actions.GetAction(), args.Entity); - + ContentService.SentToPublish += ContentService_SentToPublish; //Send notifications for the published action - ContentService.Published += (sender, args) => _notifier.Notify(_actions.GetAction(), args.PublishedEntities.ToArray()); - + ContentService.Published += ContentService_Published; //Send notifications for the saved action - ContentService.Sorted += (sender, args) => ContentServiceSorted(_notifier, sender, args, _actions); - + ContentService.Sorted += ContentService_Sorted; //Send notifications for the update and created actions - ContentService.Saved += (sender, args) => ContentServiceSaved(_notifier, sender, args, _actions); - + ContentService.Saved += ContentService_Saved; //Send notifications for the unpublish action - ContentService.Unpublished += (sender, args) => _notifier.Notify(_actions.GetAction(), args.PublishedEntities.ToArray()); - + ContentService.Unpublished += ContentService_Unpublished; //Send notifications for the move/move to recycle bin and restore actions - ContentService.Moved += (sender, args) => ContentServiceMoved(_notifier, sender, args, _actions); - + ContentService.Moved += ContentService_Moved; //Send notifications for the delete action when content is moved to the recycle bin - ContentService.Trashed += (sender, args) => _notifier.Notify(_actions.GetAction(), args.MoveInfoCollection.Select(m => m.Entity).ToArray()); - + ContentService.Trashed += ContentService_Trashed; //Send notifications for the copy action - ContentService.Copied += (sender, args) => _notifier.Notify(_actions.GetAction(), args.Original); - + ContentService.Copied += ContentService_Copied; //Send notifications for the rollback action - ContentService.RolledBack += (sender, args) => _notifier.Notify(_actions.GetAction(), args.Entity); - + ContentService.RolledBack += ContentService_RolledBack; //Send notifications for the public access changed action - PublicAccessService.Saved += (sender, args) => PublicAccessServiceSaved(_notifier, sender, args, _contentService, _actions); - - UserService.UserGroupPermissionsAssigned += (sender, args) => UserServiceUserGroupPermissionsAssigned(_notifier, sender, args, _contentService, _actions); + PublicAccessService.Saved += PublicAccessService_Saved; + + UserService.UserGroupPermissionsAssigned += UserService_UserGroupPermissionsAssigned; } public void Terminate() - { } + { + ContentService.SentToPublish -= ContentService_SentToPublish; + ContentService.Published -= ContentService_Published; + ContentService.Sorted -= ContentService_Sorted; + ContentService.Saved -= ContentService_Saved; + ContentService.Unpublished -= ContentService_Unpublished; + ContentService.Moved -= ContentService_Moved; + ContentService.Trashed -= ContentService_Trashed; + ContentService.Copied -= ContentService_Copied; + ContentService.RolledBack -= ContentService_RolledBack; + PublicAccessService.Saved -= PublicAccessService_Saved; + UserService.UserGroupPermissionsAssigned -= UserService_UserGroupPermissionsAssigned; + } - private void ContentServiceSorted(Notifier notifier, IContentService sender, Core.Events.SaveEventArgs args, ActionCollection actions) + private void UserService_UserGroupPermissionsAssigned(IUserService sender, Core.Events.SaveEventArgs args) + => UserServiceUserGroupPermissionsAssigned(args, _contentService); + + private void PublicAccessService_Saved(IPublicAccessService sender, Core.Events.SaveEventArgs args) + => PublicAccessServiceSaved(args, _contentService); + + private void ContentService_RolledBack(IContentService sender, Core.Events.RollbackEventArgs args) + => _notifier.Notify(_actions.GetAction(), args.Entity); + + private void ContentService_Copied(IContentService sender, Core.Events.CopyEventArgs args) + => _notifier.Notify(_actions.GetAction(), args.Original); + + private void ContentService_Trashed(IContentService sender, Core.Events.MoveEventArgs args) + => _notifier.Notify(_actions.GetAction(), args.MoveInfoCollection.Select(m => m.Entity).ToArray()); + + private void ContentService_Moved(IContentService sender, Core.Events.MoveEventArgs args) + => ContentServiceMoved(args); + + private void ContentService_Unpublished(IContentService sender, Core.Events.PublishEventArgs args) + => _notifier.Notify(_actions.GetAction(), args.PublishedEntities.ToArray()); + + private void ContentService_Saved(IContentService sender, Core.Events.ContentSavedEventArgs args) + => ContentServiceSaved(args); + + private void ContentService_Sorted(IContentService sender, Core.Events.SaveEventArgs args) + => ContentServiceSorted(sender, args); + + private void ContentService_Published(IContentService sender, Core.Events.ContentPublishedEventArgs args) + => _notifier.Notify(_actions.GetAction(), args.PublishedEntities.ToArray()); + + private void ContentService_SentToPublish(IContentService sender, Core.Events.SendToPublishEventArgs args) + => _notifier.Notify(_actions.GetAction(), args.Entity); + + private void ContentServiceSorted(IContentService sender, Core.Events.SaveEventArgs args) { var parentId = args.SavedEntities.Select(x => x.ParentId).Distinct().ToList(); if (parentId.Count != 1) return; // this shouldn't happen, for sorting all entities will have the same parent id @@ -79,10 +115,10 @@ namespace Umbraco.Web.Compose var parent = sender.GetById(parentId[0]); if (parent == null) return; // this shouldn't happen - notifier.Notify(actions.GetAction(), new[] { parent }); + _notifier.Notify(_actions.GetAction(), new[] { parent }); } - private void ContentServiceSaved(Notifier notifier, IContentService sender, Core.Events.SaveEventArgs args, ActionCollection actions) + private void ContentServiceSaved(Core.Events.SaveEventArgs args) { var newEntities = new List(); var updatedEntities = new List(); @@ -102,21 +138,21 @@ namespace Umbraco.Web.Compose updatedEntities.Add(entity); } } - notifier.Notify(actions.GetAction(), newEntities.ToArray()); - notifier.Notify(actions.GetAction(), updatedEntities.ToArray()); + _notifier.Notify(_actions.GetAction(), newEntities.ToArray()); + _notifier.Notify(_actions.GetAction(), updatedEntities.ToArray()); } - private void UserServiceUserGroupPermissionsAssigned(Notifier notifier, IUserService sender, Core.Events.SaveEventArgs args, IContentService contentService, ActionCollection actions) + private void UserServiceUserGroupPermissionsAssigned(Core.Events.SaveEventArgs args, IContentService contentService) { var entities = contentService.GetByIds(args.SavedEntities.Select(e => e.EntityId)).ToArray(); - if(entities.Any() == false) + if (entities.Any() == false) { return; } - notifier.Notify(actions.GetAction(), entities); + _notifier.Notify(_actions.GetAction(), entities); } - - private void ContentServiceMoved(Notifier notifier, IContentService sender, Core.Events.MoveEventArgs args, ActionCollection actions) + + private void ContentServiceMoved(Core.Events.MoveEventArgs args) { // notify about the move for all moved items _notifier.Notify(_actions.GetAction(), args.MoveInfoCollection.Select(m => m.Entity).ToArray()); @@ -126,22 +162,22 @@ namespace Umbraco.Web.Compose .Where(m => m.OriginalPath.Contains(Constants.System.RecycleBinContentString)) .Select(m => m.Entity) .ToArray(); - if(restoredEntities.Any()) + if (restoredEntities.Any()) { _notifier.Notify(_actions.GetAction(), restoredEntities); } } - private void PublicAccessServiceSaved(Notifier notifier, IPublicAccessService sender, Core.Events.SaveEventArgs args, IContentService contentService, ActionCollection actions) + private void PublicAccessServiceSaved(Core.Events.SaveEventArgs args, IContentService contentService) { var entities = contentService.GetByIds(args.SavedEntities.Select(e => e.ProtectedNodeId)).ToArray(); - if(entities.Any() == false) + if (entities.Any() == false) { return; } - notifier.Notify(actions.GetAction(), entities); + _notifier.Notify(_actions.GetAction(), entities); } - + /// /// This class is used to send the notifications /// diff --git a/src/Umbraco.Web/Compose/PublicAccessComponent.cs b/src/Umbraco.Web/Compose/PublicAccessComponent.cs index ef7532253b..dcf57ee984 100644 --- a/src/Umbraco.Web/Compose/PublicAccessComponent.cs +++ b/src/Umbraco.Web/Compose/PublicAccessComponent.cs @@ -14,7 +14,9 @@ namespace Umbraco.Web.Compose } public void Terminate() - { } + { + MemberGroupService.Saved -= MemberGroupService_Saved; + } static void MemberGroupService_Saved(IMemberGroupService sender, Core.Events.SaveEventArgs e) { diff --git a/src/Umbraco.Web/Logging/WebProfilerComponent.cs b/src/Umbraco.Web/Logging/WebProfilerComponent.cs index 2959e12ad7..1cb2142199 100755 --- a/src/Umbraco.Web/Logging/WebProfilerComponent.cs +++ b/src/Umbraco.Web/Logging/WebProfilerComponent.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Web; using Umbraco.Core.Composing; using Umbraco.Core.Logging; @@ -9,6 +10,7 @@ namespace Umbraco.Web.Logging { private readonly WebProfiler _profiler; private readonly bool _profile; + private readonly List _terminate = new List(); public WebProfilerComponent(IProfiler profiler, ILogger logger) { @@ -35,15 +37,23 @@ namespace Umbraco.Web.Logging } public void Terminate() - { } + { + UmbracoApplicationBase.ApplicationInit -= InitializeApplication; + foreach (var t in _terminate) t(); + } private void InitializeApplication(object sender, EventArgs args) { if (!(sender is HttpApplication app)) return; // for *each* application (this will run more than once) - app.BeginRequest += (s, a) => _profiler.UmbracoApplicationBeginRequest(s, a); - app.EndRequest += (s, a) => _profiler.UmbracoApplicationEndRequest(s, a); + void beginRequest(object s, EventArgs a) => _profiler.UmbracoApplicationBeginRequest(s, a); + app.BeginRequest += beginRequest; + _terminate.Add(() => app.BeginRequest -= beginRequest); + + void endRequest(object s, EventArgs a) => _profiler.UmbracoApplicationEndRequest(s, a); + app.EndRequest += endRequest; + _terminate.Add(() => app.EndRequest -= endRequest); } } } diff --git a/src/Umbraco.Web/PropertyEditors/PropertyEditorsComponent.cs b/src/Umbraco.Web/PropertyEditors/PropertyEditorsComponent.cs index df603d9e88..eaa209c055 100644 --- a/src/Umbraco.Web/PropertyEditors/PropertyEditorsComponent.cs +++ b/src/Umbraco.Web/PropertyEditors/PropertyEditorsComponent.cs @@ -1,7 +1,11 @@ -using System.Linq; +using System; +using System.Collections.Generic; +using System.Linq; using Umbraco.Core.Composing; +using Umbraco.Core.Events; using Umbraco.Core.Models; using Umbraco.Core.PropertyEditors; +using Umbraco.Core.Services; using Umbraco.Core.Services.Implement; namespace Umbraco.Web.PropertyEditors @@ -9,6 +13,7 @@ namespace Umbraco.Web.PropertyEditors internal sealed class PropertyEditorsComponent : IComponent { private readonly PropertyEditorCollection _propertyEditors; + private readonly List _terminate = new List(); public PropertyEditorsComponent(PropertyEditorCollection propertyEditors) { @@ -27,32 +32,48 @@ namespace Umbraco.Web.PropertyEditors } public void Terminate() - { } - - private static void Initialize(FileUploadPropertyEditor fileUpload) { - MediaService.Saving += fileUpload.MediaServiceSaving; - ContentService.Copied += fileUpload.ContentServiceCopied; - - MediaService.Deleted += (sender, args) - => args.MediaFilesToDelete.AddRange(fileUpload.ServiceDeleted(args.DeletedEntities.Cast())); - ContentService.Deleted += (sender, args) - => args.MediaFilesToDelete.AddRange(fileUpload.ServiceDeleted(args.DeletedEntities.Cast())); - MemberService.Deleted += (sender, args) - => args.MediaFilesToDelete.AddRange(fileUpload.ServiceDeleted(args.DeletedEntities.Cast())); + foreach (var t in _terminate) t(); } - private static void Initialize(ImageCropperPropertyEditor imageCropper) + private void Initialize(FileUploadPropertyEditor fileUpload) + { + MediaService.Saving += fileUpload.MediaServiceSaving; + _terminate.Add(() => MediaService.Saving -= fileUpload.MediaServiceSaving); + ContentService.Copied += fileUpload.ContentServiceCopied; + _terminate.Add(() => ContentService.Copied -= fileUpload.ContentServiceCopied); + + void mediaServiceDeleted(IMediaService sender, DeleteEventArgs args) => args.MediaFilesToDelete.AddRange(fileUpload.ServiceDeleted(args.DeletedEntities.Cast())); + MediaService.Deleted += mediaServiceDeleted; + _terminate.Add(() => MediaService.Deleted -= mediaServiceDeleted); + + void contentServiceDeleted(IContentService sender, DeleteEventArgs args) => args.MediaFilesToDelete.AddRange(fileUpload.ServiceDeleted(args.DeletedEntities.Cast())); + ContentService.Deleted += contentServiceDeleted; + _terminate.Add(() => ContentService.Deleted -= contentServiceDeleted); + + void memberServiceDeleted(IMemberService sender, DeleteEventArgs args) => args.MediaFilesToDelete.AddRange(fileUpload.ServiceDeleted(args.DeletedEntities.Cast())); + MemberService.Deleted += memberServiceDeleted; + _terminate.Add(() => MemberService.Deleted -= memberServiceDeleted); + } + + private void Initialize(ImageCropperPropertyEditor imageCropper) { MediaService.Saving += imageCropper.MediaServiceSaving; + _terminate.Add(() => MediaService.Saving -= imageCropper.MediaServiceSaving); ContentService.Copied += imageCropper.ContentServiceCopied; + _terminate.Add(() => ContentService.Copied -= imageCropper.ContentServiceCopied); - MediaService.Deleted += (sender, args) - => args.MediaFilesToDelete.AddRange(imageCropper.ServiceDeleted(args.DeletedEntities.Cast())); - ContentService.Deleted += (sender, args) - => args.MediaFilesToDelete.AddRange(imageCropper.ServiceDeleted(args.DeletedEntities.Cast())); - MemberService.Deleted += (sender, args) - => args.MediaFilesToDelete.AddRange(imageCropper.ServiceDeleted(args.DeletedEntities.Cast())); + void mediaServiceDeleted(IMediaService sender, DeleteEventArgs args) => args.MediaFilesToDelete.AddRange(imageCropper.ServiceDeleted(args.DeletedEntities.Cast())); + MediaService.Deleted += mediaServiceDeleted; + _terminate.Add(() => MediaService.Deleted -= mediaServiceDeleted); + + void contentServiceDeleted(IContentService sender, DeleteEventArgs args) => args.MediaFilesToDelete.AddRange(imageCropper.ServiceDeleted(args.DeletedEntities.Cast())); + ContentService.Deleted += contentServiceDeleted; + _terminate.Add(() => ContentService.Deleted -= contentServiceDeleted); + + void memberServiceDeleted(IMemberService sender, DeleteEventArgs args) => args.MediaFilesToDelete.AddRange(imageCropper.ServiceDeleted(args.DeletedEntities.Cast())); + MemberService.Deleted += memberServiceDeleted; + _terminate.Add(() => MemberService.Deleted -= memberServiceDeleted); } } } diff --git a/src/Umbraco.Web/Routing/RedirectTrackingComponent.cs b/src/Umbraco.Web/Routing/RedirectTrackingComponent.cs index fcf6f55f44..490fade3c6 100644 --- a/src/Umbraco.Web/Routing/RedirectTrackingComponent.cs +++ b/src/Umbraco.Web/Routing/RedirectTrackingComponent.cs @@ -52,7 +52,12 @@ namespace Umbraco.Web.Routing } public void Terminate() - { } + { + ContentService.Publishing -= ContentService_Publishing; + ContentService.Published -= ContentService_Published; + ContentService.Moving -= ContentService_Moving; + ContentService.Moved -= ContentService_Moved; + } private void ContentService_Publishing(IContentService sender, PublishEventArgs args) { diff --git a/src/Umbraco.Web/Search/ExamineComponent.cs b/src/Umbraco.Web/Search/ExamineComponent.cs index 149b4d1436..24be243cea 100644 --- a/src/Umbraco.Web/Search/ExamineComponent.cs +++ b/src/Umbraco.Web/Search/ExamineComponent.cs @@ -117,7 +117,13 @@ namespace Umbraco.Web.Search } public void Terminate() - { } + { + ContentCacheRefresher.CacheUpdated -= ContentCacheRefresherUpdated; + ContentTypeCacheRefresher.CacheUpdated -= ContentTypeCacheRefresherUpdated; + MediaCacheRefresher.CacheUpdated -= MediaCacheRefresherUpdated; + MemberCacheRefresher.CacheUpdated -= MemberCacheRefresherUpdated; + LanguageCacheRefresher.CacheUpdated -= LanguageCacheRefresherUpdated; + } [EditorBrowsable(EditorBrowsableState.Never)] [Obsolete("This method should not be used and will be removed in future versions, rebuilding indexes can be done with the IndexRebuilder or the BackgroundIndexRebuilder")] From 3950f3286e7aa647e0345db887a95dcef0af86f8 Mon Sep 17 00:00:00 2001 From: Warren Buckley Date: Mon, 21 Sep 2020 11:27:47 +0100 Subject: [PATCH 11/14] Merge pull request #8815 from umbraco/v8/bugfix/events-unsubscribe Ensure event handlers are unsubscribed in Core Components (cherry picked from commit e7f98a569064c61436bf2dae695f68af9a13cdca) --- .../Compose/AuditEventsComponent.cs | 14 ++- .../Compose/RelateOnCopyComponent.cs | 4 +- .../Compose/RelateOnTrashComponent.cs | 7 +- .../Compose/ModelsBuilderComponent.cs | 44 ++++--- .../BackOfficeUserAuditEventsComponent.cs | 14 ++- .../Compose/NotificationsComponent.cs | 110 ++++++++++++------ .../Compose/PublicAccessComponent.cs | 4 +- .../Logging/WebProfilerComponent.cs | 16 ++- .../PropertyEditorsComponent.cs | 61 ++++++---- .../Routing/RedirectTrackingComponent.cs | 7 +- src/Umbraco.Web/Search/ExamineComponent.cs | 8 +- 11 files changed, 203 insertions(+), 86 deletions(-) diff --git a/src/Umbraco.Core/Compose/AuditEventsComponent.cs b/src/Umbraco.Core/Compose/AuditEventsComponent.cs index 033b46c13f..62610ca9c7 100644 --- a/src/Umbraco.Core/Compose/AuditEventsComponent.cs +++ b/src/Umbraco.Core/Compose/AuditEventsComponent.cs @@ -42,7 +42,19 @@ namespace Umbraco.Core.Compose } public void Terminate() - { } + { + UserService.SavedUserGroup -= OnSavedUserGroupWithUsers; + + UserService.SavedUser -= OnSavedUser; + UserService.DeletedUser -= OnDeletedUser; + UserService.UserGroupPermissionsAssigned -= UserGroupPermissionAssigned; + + MemberService.Saved -= OnSavedMember; + MemberService.Deleted -= OnDeletedMember; + MemberService.AssignedRoles -= OnAssignedRoles; + MemberService.RemovedRoles -= OnRemovedRoles; + MemberService.Exported -= OnMemberExported; + } internal static IUser UnknownUser => new User { Id = Constants.Security.UnknownUserId, Name = Constants.Security.UnknownUserName, Email = "" }; diff --git a/src/Umbraco.Core/Compose/RelateOnCopyComponent.cs b/src/Umbraco.Core/Compose/RelateOnCopyComponent.cs index b56ff8b87e..a9f9ed1ee0 100644 --- a/src/Umbraco.Core/Compose/RelateOnCopyComponent.cs +++ b/src/Umbraco.Core/Compose/RelateOnCopyComponent.cs @@ -14,7 +14,9 @@ namespace Umbraco.Core.Compose } public void Terminate() - { } + { + ContentService.Copied -= ContentServiceCopied; + } private static void ContentServiceCopied(IContentService sender, Events.CopyEventArgs e) { diff --git a/src/Umbraco.Core/Compose/RelateOnTrashComponent.cs b/src/Umbraco.Core/Compose/RelateOnTrashComponent.cs index 4e01c50fc6..a216a584ba 100644 --- a/src/Umbraco.Core/Compose/RelateOnTrashComponent.cs +++ b/src/Umbraco.Core/Compose/RelateOnTrashComponent.cs @@ -18,7 +18,12 @@ namespace Umbraco.Core.Compose } public void Terminate() - { } + { + ContentService.Moved -= ContentService_Moved; + ContentService.Trashed -= ContentService_Trashed; + MediaService.Moved -= MediaService_Moved; + MediaService.Trashed -= MediaService_Trashed; + } private static void ContentService_Moved(IContentService sender, MoveEventArgs e) { diff --git a/src/Umbraco.ModelsBuilder.Embedded/Compose/ModelsBuilderComponent.cs b/src/Umbraco.ModelsBuilder.Embedded/Compose/ModelsBuilderComponent.cs index 0e41c9ac62..7434131dc2 100644 --- a/src/Umbraco.ModelsBuilder.Embedded/Compose/ModelsBuilderComponent.cs +++ b/src/Umbraco.ModelsBuilder.Embedded/Compose/ModelsBuilderComponent.cs @@ -50,32 +50,38 @@ namespace Umbraco.ModelsBuilder.Embedded.Compose } public void Terminate() - { } + { + ServerVariablesParser.Parsing -= ServerVariablesParser_Parsing; + ContentModelBinder.ModelBindingException -= ContentModelBinder_ModelBindingException; + FileService.SavingTemplate -= FileService_SavingTemplate; + } private void InstallServerVars() { // register our url - for the backoffice api - ServerVariablesParser.Parsing += (sender, serverVars) => - { - if (!serverVars.ContainsKey("umbracoUrls")) - throw new ArgumentException("Missing umbracoUrls."); - var umbracoUrlsObject = serverVars["umbracoUrls"]; - if (umbracoUrlsObject == null) - throw new ArgumentException("Null umbracoUrls"); - if (!(umbracoUrlsObject is Dictionary umbracoUrls)) - throw new ArgumentException("Invalid umbracoUrls"); + ServerVariablesParser.Parsing += ServerVariablesParser_Parsing; + } - if (!serverVars.ContainsKey("umbracoPlugins")) - throw new ArgumentException("Missing umbracoPlugins."); - if (!(serverVars["umbracoPlugins"] is Dictionary umbracoPlugins)) - throw new ArgumentException("Invalid umbracoPlugins"); + private void ServerVariablesParser_Parsing(object sender, Dictionary serverVars) + { + if (!serverVars.ContainsKey("umbracoUrls")) + throw new ArgumentException("Missing umbracoUrls."); + var umbracoUrlsObject = serverVars["umbracoUrls"]; + if (umbracoUrlsObject == null) + throw new ArgumentException("Null umbracoUrls"); + if (!(umbracoUrlsObject is Dictionary umbracoUrls)) + throw new ArgumentException("Invalid umbracoUrls"); - if (HttpContext.Current == null) throw new InvalidOperationException("HttpContext is null"); - var urlHelper = new UrlHelper(new RequestContext(new HttpContextWrapper(HttpContext.Current), new RouteData())); + if (!serverVars.ContainsKey("umbracoPlugins")) + throw new ArgumentException("Missing umbracoPlugins."); + if (!(serverVars["umbracoPlugins"] is Dictionary umbracoPlugins)) + throw new ArgumentException("Invalid umbracoPlugins"); - umbracoUrls["modelsBuilderBaseUrl"] = urlHelper.GetUmbracoApiServiceBaseUrl(controller => controller.BuildModels()); - umbracoPlugins["modelsBuilder"] = GetModelsBuilderSettings(); - }; + if (HttpContext.Current == null) throw new InvalidOperationException("HttpContext is null"); + var urlHelper = new UrlHelper(new RequestContext(new HttpContextWrapper(HttpContext.Current), new RouteData())); + + umbracoUrls["modelsBuilderBaseUrl"] = urlHelper.GetUmbracoApiServiceBaseUrl(controller => controller.BuildModels()); + umbracoPlugins["modelsBuilder"] = GetModelsBuilderSettings(); } private Dictionary GetModelsBuilderSettings() diff --git a/src/Umbraco.Web/Compose/BackOfficeUserAuditEventsComponent.cs b/src/Umbraco.Web/Compose/BackOfficeUserAuditEventsComponent.cs index 4244d575af..d0341ec3b6 100644 --- a/src/Umbraco.Web/Compose/BackOfficeUserAuditEventsComponent.cs +++ b/src/Umbraco.Web/Compose/BackOfficeUserAuditEventsComponent.cs @@ -35,7 +35,19 @@ namespace Umbraco.Web.Compose } public void Terminate() - { } + { + //BackOfficeUserManager.AccountLocked -= ; + //BackOfficeUserManager.AccountUnlocked -= ; + BackOfficeUserManager.ForgotPasswordRequested -= OnForgotPasswordRequest; + BackOfficeUserManager.ForgotPasswordChangedSuccess -= OnForgotPasswordChange; + BackOfficeUserManager.LoginFailed -= OnLoginFailed; + //BackOfficeUserManager.LoginRequiresVerification -= ; + BackOfficeUserManager.LoginSuccess -= OnLoginSuccess; + BackOfficeUserManager.LogoutSuccess -= OnLogoutSuccess; + BackOfficeUserManager.PasswordChanged -= OnPasswordChanged; + BackOfficeUserManager.PasswordReset -= OnPasswordReset; + //BackOfficeUserManager.ResetAccessFailedCount -= ; + } private IUser GetPerformingUser(int userId) { diff --git a/src/Umbraco.Web/Compose/NotificationsComponent.cs b/src/Umbraco.Web/Compose/NotificationsComponent.cs index 6fcc1269cb..24c7992dd2 100644 --- a/src/Umbraco.Web/Compose/NotificationsComponent.cs +++ b/src/Umbraco.Web/Compose/NotificationsComponent.cs @@ -32,42 +32,78 @@ namespace Umbraco.Web.Compose public void Initialize() { //Send notifications for the send to publish action - ContentService.SentToPublish += (sender, args) => _notifier.Notify(_actions.GetAction(), args.Entity); - + ContentService.SentToPublish += ContentService_SentToPublish; //Send notifications for the published action - ContentService.Published += (sender, args) => _notifier.Notify(_actions.GetAction(), args.PublishedEntities.ToArray()); - + ContentService.Published += ContentService_Published; //Send notifications for the saved action - ContentService.Sorted += (sender, args) => ContentServiceSorted(_notifier, sender, args, _actions); - + ContentService.Sorted += ContentService_Sorted; //Send notifications for the update and created actions - ContentService.Saved += (sender, args) => ContentServiceSaved(_notifier, sender, args, _actions); - + ContentService.Saved += ContentService_Saved; //Send notifications for the unpublish action - ContentService.Unpublished += (sender, args) => _notifier.Notify(_actions.GetAction(), args.PublishedEntities.ToArray()); - + ContentService.Unpublished += ContentService_Unpublished; //Send notifications for the move/move to recycle bin and restore actions - ContentService.Moved += (sender, args) => ContentServiceMoved(_notifier, sender, args, _actions); - + ContentService.Moved += ContentService_Moved; //Send notifications for the delete action when content is moved to the recycle bin - ContentService.Trashed += (sender, args) => _notifier.Notify(_actions.GetAction(), args.MoveInfoCollection.Select(m => m.Entity).ToArray()); - + ContentService.Trashed += ContentService_Trashed; //Send notifications for the copy action - ContentService.Copied += (sender, args) => _notifier.Notify(_actions.GetAction(), args.Original); - + ContentService.Copied += ContentService_Copied; //Send notifications for the rollback action - ContentService.RolledBack += (sender, args) => _notifier.Notify(_actions.GetAction(), args.Entity); - + ContentService.RolledBack += ContentService_RolledBack; //Send notifications for the public access changed action - PublicAccessService.Saved += (sender, args) => PublicAccessServiceSaved(_notifier, sender, args, _contentService, _actions); - - UserService.UserGroupPermissionsAssigned += (sender, args) => UserServiceUserGroupPermissionsAssigned(_notifier, sender, args, _contentService, _actions); + PublicAccessService.Saved += PublicAccessService_Saved; + + UserService.UserGroupPermissionsAssigned += UserService_UserGroupPermissionsAssigned; } public void Terminate() - { } + { + ContentService.SentToPublish -= ContentService_SentToPublish; + ContentService.Published -= ContentService_Published; + ContentService.Sorted -= ContentService_Sorted; + ContentService.Saved -= ContentService_Saved; + ContentService.Unpublished -= ContentService_Unpublished; + ContentService.Moved -= ContentService_Moved; + ContentService.Trashed -= ContentService_Trashed; + ContentService.Copied -= ContentService_Copied; + ContentService.RolledBack -= ContentService_RolledBack; + PublicAccessService.Saved -= PublicAccessService_Saved; + UserService.UserGroupPermissionsAssigned -= UserService_UserGroupPermissionsAssigned; + } - private void ContentServiceSorted(Notifier notifier, IContentService sender, Core.Events.SaveEventArgs args, ActionCollection actions) + private void UserService_UserGroupPermissionsAssigned(IUserService sender, Core.Events.SaveEventArgs args) + => UserServiceUserGroupPermissionsAssigned(args, _contentService); + + private void PublicAccessService_Saved(IPublicAccessService sender, Core.Events.SaveEventArgs args) + => PublicAccessServiceSaved(args, _contentService); + + private void ContentService_RolledBack(IContentService sender, Core.Events.RollbackEventArgs args) + => _notifier.Notify(_actions.GetAction(), args.Entity); + + private void ContentService_Copied(IContentService sender, Core.Events.CopyEventArgs args) + => _notifier.Notify(_actions.GetAction(), args.Original); + + private void ContentService_Trashed(IContentService sender, Core.Events.MoveEventArgs args) + => _notifier.Notify(_actions.GetAction(), args.MoveInfoCollection.Select(m => m.Entity).ToArray()); + + private void ContentService_Moved(IContentService sender, Core.Events.MoveEventArgs args) + => ContentServiceMoved(args); + + private void ContentService_Unpublished(IContentService sender, Core.Events.PublishEventArgs args) + => _notifier.Notify(_actions.GetAction(), args.PublishedEntities.ToArray()); + + private void ContentService_Saved(IContentService sender, Core.Events.ContentSavedEventArgs args) + => ContentServiceSaved(args); + + private void ContentService_Sorted(IContentService sender, Core.Events.SaveEventArgs args) + => ContentServiceSorted(sender, args); + + private void ContentService_Published(IContentService sender, Core.Events.ContentPublishedEventArgs args) + => _notifier.Notify(_actions.GetAction(), args.PublishedEntities.ToArray()); + + private void ContentService_SentToPublish(IContentService sender, Core.Events.SendToPublishEventArgs args) + => _notifier.Notify(_actions.GetAction(), args.Entity); + + private void ContentServiceSorted(IContentService sender, Core.Events.SaveEventArgs args) { var parentId = args.SavedEntities.Select(x => x.ParentId).Distinct().ToList(); if (parentId.Count != 1) return; // this shouldn't happen, for sorting all entities will have the same parent id @@ -79,10 +115,10 @@ namespace Umbraco.Web.Compose var parent = sender.GetById(parentId[0]); if (parent == null) return; // this shouldn't happen - notifier.Notify(actions.GetAction(), new[] { parent }); + _notifier.Notify(_actions.GetAction(), new[] { parent }); } - private void ContentServiceSaved(Notifier notifier, IContentService sender, Core.Events.SaveEventArgs args, ActionCollection actions) + private void ContentServiceSaved(Core.Events.SaveEventArgs args) { var newEntities = new List(); var updatedEntities = new List(); @@ -102,21 +138,21 @@ namespace Umbraco.Web.Compose updatedEntities.Add(entity); } } - notifier.Notify(actions.GetAction(), newEntities.ToArray()); - notifier.Notify(actions.GetAction(), updatedEntities.ToArray()); + _notifier.Notify(_actions.GetAction(), newEntities.ToArray()); + _notifier.Notify(_actions.GetAction(), updatedEntities.ToArray()); } - private void UserServiceUserGroupPermissionsAssigned(Notifier notifier, IUserService sender, Core.Events.SaveEventArgs args, IContentService contentService, ActionCollection actions) + private void UserServiceUserGroupPermissionsAssigned(Core.Events.SaveEventArgs args, IContentService contentService) { var entities = contentService.GetByIds(args.SavedEntities.Select(e => e.EntityId)).ToArray(); - if(entities.Any() == false) + if (entities.Any() == false) { return; } - notifier.Notify(actions.GetAction(), entities); + _notifier.Notify(_actions.GetAction(), entities); } - - private void ContentServiceMoved(Notifier notifier, IContentService sender, Core.Events.MoveEventArgs args, ActionCollection actions) + + private void ContentServiceMoved(Core.Events.MoveEventArgs args) { // notify about the move for all moved items _notifier.Notify(_actions.GetAction(), args.MoveInfoCollection.Select(m => m.Entity).ToArray()); @@ -126,22 +162,22 @@ namespace Umbraco.Web.Compose .Where(m => m.OriginalPath.Contains(Constants.System.RecycleBinContentString)) .Select(m => m.Entity) .ToArray(); - if(restoredEntities.Any()) + if (restoredEntities.Any()) { _notifier.Notify(_actions.GetAction(), restoredEntities); } } - private void PublicAccessServiceSaved(Notifier notifier, IPublicAccessService sender, Core.Events.SaveEventArgs args, IContentService contentService, ActionCollection actions) + private void PublicAccessServiceSaved(Core.Events.SaveEventArgs args, IContentService contentService) { var entities = contentService.GetByIds(args.SavedEntities.Select(e => e.ProtectedNodeId)).ToArray(); - if(entities.Any() == false) + if (entities.Any() == false) { return; } - notifier.Notify(actions.GetAction(), entities); + _notifier.Notify(_actions.GetAction(), entities); } - + /// /// This class is used to send the notifications /// diff --git a/src/Umbraco.Web/Compose/PublicAccessComponent.cs b/src/Umbraco.Web/Compose/PublicAccessComponent.cs index ef7532253b..dcf57ee984 100644 --- a/src/Umbraco.Web/Compose/PublicAccessComponent.cs +++ b/src/Umbraco.Web/Compose/PublicAccessComponent.cs @@ -14,7 +14,9 @@ namespace Umbraco.Web.Compose } public void Terminate() - { } + { + MemberGroupService.Saved -= MemberGroupService_Saved; + } static void MemberGroupService_Saved(IMemberGroupService sender, Core.Events.SaveEventArgs e) { diff --git a/src/Umbraco.Web/Logging/WebProfilerComponent.cs b/src/Umbraco.Web/Logging/WebProfilerComponent.cs index 2959e12ad7..1cb2142199 100755 --- a/src/Umbraco.Web/Logging/WebProfilerComponent.cs +++ b/src/Umbraco.Web/Logging/WebProfilerComponent.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Web; using Umbraco.Core.Composing; using Umbraco.Core.Logging; @@ -9,6 +10,7 @@ namespace Umbraco.Web.Logging { private readonly WebProfiler _profiler; private readonly bool _profile; + private readonly List _terminate = new List(); public WebProfilerComponent(IProfiler profiler, ILogger logger) { @@ -35,15 +37,23 @@ namespace Umbraco.Web.Logging } public void Terminate() - { } + { + UmbracoApplicationBase.ApplicationInit -= InitializeApplication; + foreach (var t in _terminate) t(); + } private void InitializeApplication(object sender, EventArgs args) { if (!(sender is HttpApplication app)) return; // for *each* application (this will run more than once) - app.BeginRequest += (s, a) => _profiler.UmbracoApplicationBeginRequest(s, a); - app.EndRequest += (s, a) => _profiler.UmbracoApplicationEndRequest(s, a); + void beginRequest(object s, EventArgs a) => _profiler.UmbracoApplicationBeginRequest(s, a); + app.BeginRequest += beginRequest; + _terminate.Add(() => app.BeginRequest -= beginRequest); + + void endRequest(object s, EventArgs a) => _profiler.UmbracoApplicationEndRequest(s, a); + app.EndRequest += endRequest; + _terminate.Add(() => app.EndRequest -= endRequest); } } } diff --git a/src/Umbraco.Web/PropertyEditors/PropertyEditorsComponent.cs b/src/Umbraco.Web/PropertyEditors/PropertyEditorsComponent.cs index df603d9e88..eaa209c055 100644 --- a/src/Umbraco.Web/PropertyEditors/PropertyEditorsComponent.cs +++ b/src/Umbraco.Web/PropertyEditors/PropertyEditorsComponent.cs @@ -1,7 +1,11 @@ -using System.Linq; +using System; +using System.Collections.Generic; +using System.Linq; using Umbraco.Core.Composing; +using Umbraco.Core.Events; using Umbraco.Core.Models; using Umbraco.Core.PropertyEditors; +using Umbraco.Core.Services; using Umbraco.Core.Services.Implement; namespace Umbraco.Web.PropertyEditors @@ -9,6 +13,7 @@ namespace Umbraco.Web.PropertyEditors internal sealed class PropertyEditorsComponent : IComponent { private readonly PropertyEditorCollection _propertyEditors; + private readonly List _terminate = new List(); public PropertyEditorsComponent(PropertyEditorCollection propertyEditors) { @@ -27,32 +32,48 @@ namespace Umbraco.Web.PropertyEditors } public void Terminate() - { } - - private static void Initialize(FileUploadPropertyEditor fileUpload) { - MediaService.Saving += fileUpload.MediaServiceSaving; - ContentService.Copied += fileUpload.ContentServiceCopied; - - MediaService.Deleted += (sender, args) - => args.MediaFilesToDelete.AddRange(fileUpload.ServiceDeleted(args.DeletedEntities.Cast())); - ContentService.Deleted += (sender, args) - => args.MediaFilesToDelete.AddRange(fileUpload.ServiceDeleted(args.DeletedEntities.Cast())); - MemberService.Deleted += (sender, args) - => args.MediaFilesToDelete.AddRange(fileUpload.ServiceDeleted(args.DeletedEntities.Cast())); + foreach (var t in _terminate) t(); } - private static void Initialize(ImageCropperPropertyEditor imageCropper) + private void Initialize(FileUploadPropertyEditor fileUpload) + { + MediaService.Saving += fileUpload.MediaServiceSaving; + _terminate.Add(() => MediaService.Saving -= fileUpload.MediaServiceSaving); + ContentService.Copied += fileUpload.ContentServiceCopied; + _terminate.Add(() => ContentService.Copied -= fileUpload.ContentServiceCopied); + + void mediaServiceDeleted(IMediaService sender, DeleteEventArgs args) => args.MediaFilesToDelete.AddRange(fileUpload.ServiceDeleted(args.DeletedEntities.Cast())); + MediaService.Deleted += mediaServiceDeleted; + _terminate.Add(() => MediaService.Deleted -= mediaServiceDeleted); + + void contentServiceDeleted(IContentService sender, DeleteEventArgs args) => args.MediaFilesToDelete.AddRange(fileUpload.ServiceDeleted(args.DeletedEntities.Cast())); + ContentService.Deleted += contentServiceDeleted; + _terminate.Add(() => ContentService.Deleted -= contentServiceDeleted); + + void memberServiceDeleted(IMemberService sender, DeleteEventArgs args) => args.MediaFilesToDelete.AddRange(fileUpload.ServiceDeleted(args.DeletedEntities.Cast())); + MemberService.Deleted += memberServiceDeleted; + _terminate.Add(() => MemberService.Deleted -= memberServiceDeleted); + } + + private void Initialize(ImageCropperPropertyEditor imageCropper) { MediaService.Saving += imageCropper.MediaServiceSaving; + _terminate.Add(() => MediaService.Saving -= imageCropper.MediaServiceSaving); ContentService.Copied += imageCropper.ContentServiceCopied; + _terminate.Add(() => ContentService.Copied -= imageCropper.ContentServiceCopied); - MediaService.Deleted += (sender, args) - => args.MediaFilesToDelete.AddRange(imageCropper.ServiceDeleted(args.DeletedEntities.Cast())); - ContentService.Deleted += (sender, args) - => args.MediaFilesToDelete.AddRange(imageCropper.ServiceDeleted(args.DeletedEntities.Cast())); - MemberService.Deleted += (sender, args) - => args.MediaFilesToDelete.AddRange(imageCropper.ServiceDeleted(args.DeletedEntities.Cast())); + void mediaServiceDeleted(IMediaService sender, DeleteEventArgs args) => args.MediaFilesToDelete.AddRange(imageCropper.ServiceDeleted(args.DeletedEntities.Cast())); + MediaService.Deleted += mediaServiceDeleted; + _terminate.Add(() => MediaService.Deleted -= mediaServiceDeleted); + + void contentServiceDeleted(IContentService sender, DeleteEventArgs args) => args.MediaFilesToDelete.AddRange(imageCropper.ServiceDeleted(args.DeletedEntities.Cast())); + ContentService.Deleted += contentServiceDeleted; + _terminate.Add(() => ContentService.Deleted -= contentServiceDeleted); + + void memberServiceDeleted(IMemberService sender, DeleteEventArgs args) => args.MediaFilesToDelete.AddRange(imageCropper.ServiceDeleted(args.DeletedEntities.Cast())); + MemberService.Deleted += memberServiceDeleted; + _terminate.Add(() => MemberService.Deleted -= memberServiceDeleted); } } } diff --git a/src/Umbraco.Web/Routing/RedirectTrackingComponent.cs b/src/Umbraco.Web/Routing/RedirectTrackingComponent.cs index fcf6f55f44..490fade3c6 100644 --- a/src/Umbraco.Web/Routing/RedirectTrackingComponent.cs +++ b/src/Umbraco.Web/Routing/RedirectTrackingComponent.cs @@ -52,7 +52,12 @@ namespace Umbraco.Web.Routing } public void Terminate() - { } + { + ContentService.Publishing -= ContentService_Publishing; + ContentService.Published -= ContentService_Published; + ContentService.Moving -= ContentService_Moving; + ContentService.Moved -= ContentService_Moved; + } private void ContentService_Publishing(IContentService sender, PublishEventArgs args) { diff --git a/src/Umbraco.Web/Search/ExamineComponent.cs b/src/Umbraco.Web/Search/ExamineComponent.cs index 149b4d1436..24be243cea 100644 --- a/src/Umbraco.Web/Search/ExamineComponent.cs +++ b/src/Umbraco.Web/Search/ExamineComponent.cs @@ -117,7 +117,13 @@ namespace Umbraco.Web.Search } public void Terminate() - { } + { + ContentCacheRefresher.CacheUpdated -= ContentCacheRefresherUpdated; + ContentTypeCacheRefresher.CacheUpdated -= ContentTypeCacheRefresherUpdated; + MediaCacheRefresher.CacheUpdated -= MediaCacheRefresherUpdated; + MemberCacheRefresher.CacheUpdated -= MemberCacheRefresherUpdated; + LanguageCacheRefresher.CacheUpdated -= LanguageCacheRefresherUpdated; + } [EditorBrowsable(EditorBrowsableState.Never)] [Obsolete("This method should not be used and will be removed in future versions, rebuilding indexes can be done with the IndexRebuilder or the BackgroundIndexRebuilder")] From c00017220a97380b05fcdc8d65002ddefc5a44cc Mon Sep 17 00:00:00 2001 From: Sebastiaan Janssen Date: Tue, 22 Sep 2020 14:37:54 +0200 Subject: [PATCH 12/14] NuGet.org had TLS 1.0 and 1.1 disabled - https://devblogs.microsoft.com/nuget/deprecating-tls-1-0-and-1-1-on-nuget-org/. Therefore we need this line specifying Tls12 (cherry picked from commit c91281c4b80bea1bba6c23b8e697835691d109f3) --- build/build-bootstrap.ps1 | 1 + 1 file changed, 1 insertion(+) diff --git a/build/build-bootstrap.ps1 b/build/build-bootstrap.ps1 index 82c789ff22..9a31a57acf 100644 --- a/build/build-bootstrap.ps1 +++ b/build/build-bootstrap.ps1 @@ -34,6 +34,7 @@ if (-not (test-path $nuget)) { Write-Host "Download NuGet..." + [Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12 Invoke-WebRequest $source -OutFile $nuget if (-not $?) { throw "Failed to download NuGet." } } From 041d0469b618fa70c0ff07a3141f16a1e8d5b918 Mon Sep 17 00:00:00 2001 From: Claus Date: Tue, 22 Sep 2020 11:42:44 +0200 Subject: [PATCH 13/14] Merge remote-tracking branch 'origin/v8/bugfix/examine-reindex' into v8/8.6 (cherry picked from commit 8fddb52f9a76a60fcb212a436417891489b4465f) --- src/Umbraco.Examine/ContentValueSetBuilder.cs | 5 ++ src/Umbraco.Examine/UmbracoContentIndex.cs | 6 +- src/Umbraco.Web/Scheduling/SimpleTask.cs | 32 +++++++ src/Umbraco.Web/Search/ExamineComponent.cs | 84 +++++++++++++------ src/Umbraco.Web/Umbraco.Web.csproj | 1 + 5 files changed, 100 insertions(+), 28 deletions(-) create mode 100644 src/Umbraco.Web/Scheduling/SimpleTask.cs diff --git a/src/Umbraco.Examine/ContentValueSetBuilder.cs b/src/Umbraco.Examine/ContentValueSetBuilder.cs index b8477a9047..dfd64fcd47 100644 --- a/src/Umbraco.Examine/ContentValueSetBuilder.cs +++ b/src/Umbraco.Examine/ContentValueSetBuilder.cs @@ -60,6 +60,11 @@ namespace Umbraco.Examine scope.Complete(); } + return GetValueSetsEnumerable(content, creatorIds, writerIds); + } + + private IEnumerable GetValueSetsEnumerable(IContent[] content, Dictionary creatorIds, Dictionary writerIds) + { // TODO: There is a lot of boxing going on here and ultimately all values will be boxed by Lucene anyways // but I wonder if there's a way to reduce the boxing that we have to do or if it will matter in the end since // Lucene will do it no matter what? One idea was to create a `FieldValue` struct which would contain `object`, `object[]`, `ValueType` and `ValueType[]` diff --git a/src/Umbraco.Examine/UmbracoContentIndex.cs b/src/Umbraco.Examine/UmbracoContentIndex.cs index e266ca789d..67e430b4e9 100644 --- a/src/Umbraco.Examine/UmbracoContentIndex.cs +++ b/src/Umbraco.Examine/UmbracoContentIndex.cs @@ -62,7 +62,7 @@ namespace Umbraco.Examine /// protected override void PerformIndexItems(IEnumerable values, Action onComplete) { - //We don't want to re-enumerate this list, but we need to split it into 2x enumerables: invalid and valid items. + // We don't want to re-enumerate this list, but we need to split it into 2x enumerables: invalid and valid items. // The Invalid items will be deleted, these are items that have invalid paths (i.e. moved to the recycle bin, etc...) // Then we'll index the Value group all together. // We return 0 or 1 here so we can order the results and do the invalid first and then the valid. @@ -80,7 +80,7 @@ namespace Umbraco.Examine || !validator.ValidateProtectedContent(path, v.Category)) ? 0 : 1; - }); + }).ToList(); var hasDeletes = false; var hasUpdates = false; @@ -99,7 +99,7 @@ namespace Umbraco.Examine { hasUpdates = true; //these are the valid ones, so just index them all at once - base.PerformIndexItems(group, onComplete); + base.PerformIndexItems(group.ToList(), onComplete); } } diff --git a/src/Umbraco.Web/Scheduling/SimpleTask.cs b/src/Umbraco.Web/Scheduling/SimpleTask.cs new file mode 100644 index 0000000000..a8603915b0 --- /dev/null +++ b/src/Umbraco.Web/Scheduling/SimpleTask.cs @@ -0,0 +1,32 @@ +using System; +using System.Threading; +using System.Threading.Tasks; + +namespace Umbraco.Web.Scheduling +{ + /// + /// A simple task that executes a delegate synchronously + /// + internal class SimpleTask : IBackgroundTask + { + private readonly Action _action; + + public SimpleTask(Action action) + { + _action = action; + } + + public bool IsAsync => false; + + public void Run() => _action(); + + public Task RunAsync(CancellationToken token) + { + throw new NotImplementedException(); + } + + public void Dispose() + { + } + } +} diff --git a/src/Umbraco.Web/Search/ExamineComponent.cs b/src/Umbraco.Web/Search/ExamineComponent.cs index 24be243cea..ca16aaee65 100644 --- a/src/Umbraco.Web/Search/ExamineComponent.cs +++ b/src/Umbraco.Web/Search/ExamineComponent.cs @@ -17,9 +17,13 @@ using Umbraco.Core.Persistence.DatabaseModelDefinitions; using Examine.LuceneEngine.Directories; using Umbraco.Core.Composing; using System.ComponentModel; +using System.Threading; +using Umbraco.Web.Scheduling; namespace Umbraco.Web.Search { + + public sealed class ExamineComponent : Umbraco.Core.Composing.IComponent { private readonly IExamineManager _examineManager; @@ -34,7 +38,8 @@ namespace Umbraco.Web.Search private readonly IMainDom _mainDom; private readonly IProfilingLogger _logger; private readonly IUmbracoIndexesCreator _indexCreator; - + private readonly BackgroundTaskRunner _indexItemTaskRunner; + // the default enlist priority is 100 // enlist with a lower priority to ensure that anything "default" runs after us @@ -62,6 +67,7 @@ namespace Umbraco.Web.Search _mainDom = mainDom; _logger = profilingLogger; _indexCreator = indexCreator; + _indexItemTaskRunner = new BackgroundTaskRunner(_logger); } public void Initialize() @@ -580,12 +586,18 @@ namespace Umbraco.Web.Search } } + /// + /// An action that will execute at the end of the Scope being completed + /// private abstract class DeferedAction { public virtual void Execute() { } } + /// + /// Re-indexes an item on a background thread + /// private class DeferedReIndexForContent : DeferedAction { private readonly ExamineComponent _examineComponent; @@ -606,21 +618,32 @@ namespace Umbraco.Web.Search public static void Execute(ExamineComponent examineComponent, IContent content, bool isPublished) { - foreach (var index in examineComponent._examineManager.Indexes.OfType() - //filter the indexers - .Where(x => isPublished || !x.PublishedValuesOnly) - .Where(x => x.EnableDefaultEventHandler)) + // perform the ValueSet lookup on a background thread + examineComponent._indexItemTaskRunner.Add(new SimpleTask(() => { - //for content we have a different builder for published vs unpublished - var builder = index.PublishedValuesOnly - ? examineComponent._publishedContentValueSetBuilder - : (IValueSetBuilder)examineComponent._contentValueSetBuilder; + // for content we have a different builder for published vs unpublished + // we don't want to build more value sets than is needed so we'll lazily build 2 one for published one for non-published + var builders = new Dictionary>> + { + [true] = new Lazy>(() => examineComponent._publishedContentValueSetBuilder.GetValueSets(content).ToList()), + [false] = new Lazy>(() => examineComponent._contentValueSetBuilder.GetValueSets(content).ToList()) + }; - index.IndexItems(builder.GetValueSets(content)); - } + foreach (var index in examineComponent._examineManager.Indexes.OfType() + //filter the indexers + .Where(x => isPublished || !x.PublishedValuesOnly) + .Where(x => x.EnableDefaultEventHandler)) + { + var valueSet = builders[index.PublishedValuesOnly].Value; + index.IndexItems(valueSet); + } + })); } } + /// + /// Re-indexes an item on a background thread + /// private class DeferedReIndexForMedia : DeferedAction { private readonly ExamineComponent _examineComponent; @@ -641,18 +664,25 @@ namespace Umbraco.Web.Search public static void Execute(ExamineComponent examineComponent, IMedia media, bool isPublished) { - var valueSet = examineComponent._mediaValueSetBuilder.GetValueSets(media).ToList(); - - foreach (var index in examineComponent._examineManager.Indexes.OfType() - //filter the indexers - .Where(x => isPublished || !x.PublishedValuesOnly) - .Where(x => x.EnableDefaultEventHandler)) + // perform the ValueSet lookup on a background thread + examineComponent._indexItemTaskRunner.Add(new SimpleTask(() => { - index.IndexItems(valueSet); - } + var valueSet = examineComponent._mediaValueSetBuilder.GetValueSets(media).ToList(); + + foreach (var index in examineComponent._examineManager.Indexes.OfType() + //filter the indexers + .Where(x => isPublished || !x.PublishedValuesOnly) + .Where(x => x.EnableDefaultEventHandler)) + { + index.IndexItems(valueSet); + } + })); } } + /// + /// Re-indexes an item on a background thread + /// private class DeferedReIndexForMember : DeferedAction { private readonly ExamineComponent _examineComponent; @@ -671,13 +701,17 @@ namespace Umbraco.Web.Search public static void Execute(ExamineComponent examineComponent, IMember member) { - var valueSet = examineComponent._memberValueSetBuilder.GetValueSets(member).ToList(); - foreach (var index in examineComponent._examineManager.Indexes.OfType() - //filter the indexers - .Where(x => x.EnableDefaultEventHandler)) + // perform the ValueSet lookup on a background thread + examineComponent._indexItemTaskRunner.Add(new SimpleTask(() => { - index.IndexItems(valueSet); - } + var valueSet = examineComponent._memberValueSetBuilder.GetValueSets(member).ToList(); + foreach (var index in examineComponent._examineManager.Indexes.OfType() + //filter the indexers + .Where(x => x.EnableDefaultEventHandler)) + { + index.IndexItems(valueSet); + } + })); } } diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index 5e1176cdc6..e75330140e 100644 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -271,6 +271,7 @@ + From a2cb264f270eb790d43f56105480ed1381edab3e Mon Sep 17 00:00:00 2001 From: Claus Date: Tue, 22 Sep 2020 13:14:03 +0200 Subject: [PATCH 14/14] Merge remote-tracking branch 'origin/v8/bugfix/8433-member-login-sql-locks' into v8/8.6 (cherry picked from commit 6122bffa24a6ee5e554c285ee01629552abc0118) --- .../Repositories/IMemberRepository.cs | 17 ++- .../Implement/MemberRepository.cs | 107 +++++++++++++++--- .../Implement/SimpleGetRepository.cs | 2 + .../Repositories/Implement/UserRepository.cs | 8 +- .../Services/IMembershipMemberService.cs | 15 ++- .../Services/Implement/MemberService.cs | 33 +++--- .../Services/Implement/UserService.cs | 7 ++ .../Repositories/UserRepositoryTest.cs | 61 +++++++--- .../Services/MemberServiceTests.cs | 54 +++++++++ .../Cache/DistributedCacheExtensions.cs | 7 +- src/Umbraco.Web/Cache/MemberCacheRefresher.cs | 96 ++++++++++++---- src/Umbraco.Web/Security/MembershipHelper.cs | 37 ++---- .../Providers/UmbracoMembershipProvider.cs | 36 ++++-- 13 files changed, 363 insertions(+), 117 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs b/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs index 245c024356..c737c2bf66 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs @@ -1,4 +1,5 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; using Umbraco.Core.Models; using Umbraco.Core.Persistence.Querying; @@ -6,6 +7,8 @@ namespace Umbraco.Core.Persistence.Repositories { public interface IMemberRepository : IContentRepository { + IMember GetByUsername(string username); + /// /// Finds members in a given role /// @@ -35,5 +38,17 @@ namespace Umbraco.Core.Persistence.Repositories /// /// int GetCountByQuery(IQuery query); + + /// + /// Sets a members last login date based on their username + /// + /// + /// + /// + /// This is a specialized method because whenever a member logs in, the membership provider requires us to set the 'online' which requires + /// updating their login date. This operation must be fast and cannot use database locks which is fine if we are only executing a single query + /// for this data since there won't be any other data contention issues. + /// + void SetLastLogin(string username, DateTime date); } } diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs index 42e7d1c32f..687e35aef3 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/MemberRepository.cs @@ -25,6 +25,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement private readonly IMemberTypeRepository _memberTypeRepository; private readonly ITagRepository _tagRepository; private readonly IMemberGroupRepository _memberGroupRepository; + private readonly IRepositoryCachePolicy _memberByUsernameCachePolicy; public MemberRepository(IScopeAccessor scopeAccessor, AppCaches cache, ILogger logger, IMemberTypeRepository memberTypeRepository, IMemberGroupRepository memberGroupRepository, ITagRepository tagRepository, ILanguageRepository languageRepository, IRelationRepository relationRepository, IRelationTypeRepository relationTypeRepository, @@ -34,6 +35,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement _memberTypeRepository = memberTypeRepository ?? throw new ArgumentNullException(nameof(memberTypeRepository)); _tagRepository = tagRepository ?? throw new ArgumentNullException(nameof(tagRepository)); _memberGroupRepository = memberGroupRepository; + + _memberByUsernameCachePolicy = new DefaultRepositoryCachePolicy(GlobalIsolatedCache, ScopeAccessor, DefaultOptions); } protected override MemberRepository This => this; @@ -382,12 +385,27 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (changedCols.Count > 0) Database.Update(dto, changedCols); - // replace the property data - var deletePropertyDataSql = SqlContext.Sql().Delete().Where(x => x.VersionId == member.VersionId); - Database.Execute(deletePropertyDataSql); + // Replace the property data + // Lookup the data to update with a UPDLOCK (using ForUpdate()) this is because we have another method that doesn't take an explicit WriteLock + // in SetLastLogin which is called very often and we want to avoid the lock timeout for the explicit lock table but we still need to ensure atomic + // operations between that method and this one. + + var propDataSql = SqlContext.Sql().Select("*").From().Where(x => x.VersionId == member.VersionId).ForUpdate(); + var existingPropData = Database.Fetch(propDataSql).ToDictionary(x => x.PropertyTypeId); var propertyDataDtos = PropertyFactory.BuildDtos(member.ContentType.Variations, member.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); foreach (var propertyDataDto in propertyDataDtos) - Database.Insert(propertyDataDto); + { + // Check if this already exists and update, else insert a new one + if (existingPropData.TryGetValue(propertyDataDto.PropertyTypeId, out var propData)) + { + propertyDataDto.Id = propData.Id; + Database.Update(propertyDataDto); + } + else + { + Database.Insert(propertyDataDto); + } + } SetEntityTags(entity, _tagRepository); @@ -505,6 +523,56 @@ namespace Umbraco.Core.Persistence.Repositories.Implement return Database.ExecuteScalar(fullSql); } + /// + public void SetLastLogin(string username, DateTime date) + { + // Important - these queries are designed to execute without an exclusive WriteLock taken in our distributed lock + // table. However due to the data that we are updating which relies on version data we cannot update this data + // without taking some locks, otherwise we'll end up with strange situations because when a member is updated, that operation + // deletes and re-inserts all property data. So if there are concurrent transactions, one deleting and re-inserting and another trying + // to update there can be problems. This is only an issue for cmsPropertyData, not umbracoContentVersion because that table just + // maintains a single row and it isn't deleted/re-inserted. + // So the important part here is the ForUpdate() call on the select to fetch the property data to update. + + // Update the cms property value for the member + + var sqlSelectTemplateProperty = SqlContext.Templates.Get("Umbraco.Core.MemberRepository.SetLastLogin1", s => s + .Select(x => x.Id) + .From() + .InnerJoin().On((l, r) => l.Id == r.PropertyTypeId) + .InnerJoin().On((l, r) => l.Id == r.VersionId) + .InnerJoin().On((l, r) => l.NodeId == r.NodeId) + .InnerJoin().On((l, r) => l.NodeId == r.NodeId) + .Where(x => x.NodeObjectType == SqlTemplate.Arg("nodeObjectType")) + .Where(x => x.Alias == SqlTemplate.Arg("propertyTypeAlias")) + .Where(x => x.LoginName == SqlTemplate.Arg("username")) + .ForUpdate()); + var sqlSelectProperty = sqlSelectTemplateProperty.Sql(Constants.ObjectTypes.Member, Constants.Conventions.Member.LastLoginDate, username); + + var update = Sql() + .Update(u => u + .Set(x => x.DateValue, date)) + .WhereIn(x => x.Id, sqlSelectProperty); + + Database.Execute(update); + + // Update the umbracoContentVersion value for the member + + var sqlSelectTemplateVersion = SqlContext.Templates.Get("Umbraco.Core.MemberRepository.SetLastLogin2", s => s + .Select(x => x.Id) + .From() + .InnerJoin().On((l, r) => l.NodeId == r.NodeId) + .InnerJoin().On((l, r) => l.NodeId == r.NodeId) + .Where(x => x.NodeObjectType == SqlTemplate.Arg("nodeObjectType")) + .Where(x => x.LoginName == SqlTemplate.Arg("username"))); + var sqlSelectVersion = sqlSelectTemplateVersion.Sql(Constants.ObjectTypes.Member, username); + + Database.Execute(Sql() + .Update(u => u + .Set(x => x.VersionDate, date)) + .WhereIn(x => x.Id, sqlSelectVersion)); + } + /// /// Gets paged member results. /// @@ -528,20 +596,6 @@ namespace Umbraco.Core.Persistence.Repositories.Implement ordering); } - private string _pagedResultsByQueryWhere; - - private string GetPagedResultsByQueryWhere() - { - if (_pagedResultsByQueryWhere == null) - _pagedResultsByQueryWhere = " AND (" - + $"({SqlSyntax.GetQuotedTableName("umbracoNode")}.{SqlSyntax.GetQuotedColumnName("text")} LIKE @0)" - + " OR " - + $"({SqlSyntax.GetQuotedTableName("cmsMember")}.{SqlSyntax.GetQuotedColumnName("LoginName")} LIKE @0)" - + ")"; - - return _pagedResultsByQueryWhere; - } - protected override string ApplySystemOrdering(ref Sql sql, Ordering ordering) { if (ordering.OrderBy.InvariantEquals("email")) @@ -631,5 +685,22 @@ namespace Umbraco.Core.Persistence.Repositories.Implement member.ResetDirtyProperties(false); return member; } + + public IMember GetByUsername(string username) + { + return _memberByUsernameCachePolicy.Get(username, PerformGetByUsername, PerformGetAllByUsername); + } + + private IMember PerformGetByUsername(string username) + { + var query = Query().Where(x => x.Username.Equals(username)); + return PerformGetByQuery(query).FirstOrDefault(); + } + + private IEnumerable PerformGetAllByUsername(params string[] usernames) + { + var query = Query().WhereIn(x => x.Username, usernames); + return PerformGetByQuery(query); + } } } diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/SimpleGetRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/SimpleGetRepository.cs index f7e59820c3..d327fba67f 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/SimpleGetRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/SimpleGetRepository.cs @@ -11,6 +11,8 @@ using Umbraco.Core.Scoping; namespace Umbraco.Core.Persistence.Repositories.Implement { + // TODO: Obsolete this, change all implementations of this like in Dictionary to just use custom Cache policies like in the member repository. + /// /// Simple abstract ReadOnly repository used to simply have PerformGet and PeformGetAll with an underlying cache /// diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs index 3be5102b83..4721037674 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs @@ -168,10 +168,7 @@ ORDER BY colName"; } public Guid CreateLoginSession(int userId, string requestingIpAddress, bool cleanStaleSessions = true) - { - // TODO: I know this doesn't follow the normal repository conventions which would require us to create a UserSessionRepository - //and also business logic models for these objects but that's just so overkill for what we are doing - //and now that everything is properly in a transaction (Scope) there doesn't seem to be much reason for using that anymore + { var now = DateTime.UtcNow; var dto = new UserLoginDto { @@ -201,13 +198,14 @@ ORDER BY colName"; // that query is going to run a *lot*, make it a template var t = SqlContext.Templates.Get("Umbraco.Core.UserRepository.ValidateLoginSession", s => s .Select() + .SelectTop(1) .From() .Where(x => x.SessionId == SqlTemplate.Arg("sessionId")) .ForUpdate()); var sql = t.Sql(sessionId); - var found = Database.Query(sql).FirstOrDefault(); + var found = Database.FirstOrDefault(sql); if (found == null || found.UserId != userId || found.LoggedOutUtc.HasValue) return false; diff --git a/src/Umbraco.Core/Services/IMembershipMemberService.cs b/src/Umbraco.Core/Services/IMembershipMemberService.cs index 448b0c761a..3c6b4f6672 100644 --- a/src/Umbraco.Core/Services/IMembershipMemberService.cs +++ b/src/Umbraco.Core/Services/IMembershipMemberService.cs @@ -1,4 +1,5 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; using Umbraco.Core.Models; using Umbraco.Core.Models.Membership; using Umbraco.Core.Persistence.Querying; @@ -107,6 +108,18 @@ namespace Umbraco.Core.Services /// or to Delete void Delete(T membershipUser); + /// + /// Sets the last login date for the member if they are found by username + /// + /// + /// + /// + /// This is a specialized method because whenever a member logs in, the membership provider requires us to set the 'online' which requires + /// updating their login date. This operation must be fast and cannot use database locks which is fine if we are only executing a single query + /// for this data since there won't be any other data contention issues. + /// + void SetLastLogin(string username, DateTime date); + /// /// Saves an /// diff --git a/src/Umbraco.Core/Services/Implement/MemberService.cs b/src/Umbraco.Core/Services/Implement/MemberService.cs index 9b97c6b161..877bb5c9ce 100644 --- a/src/Umbraco.Core/Services/Implement/MemberService.cs +++ b/src/Umbraco.Core/Services/Implement/MemberService.cs @@ -447,15 +447,10 @@ namespace Umbraco.Core.Services.Implement /// public IMember GetByUsername(string username) { - // TODO: Somewhere in here, whether at this level or the repository level, we need to add - // a caching mechanism since this method is used by all the membership providers and could be - // called quite a bit when dealing with members. - using (var scope = ScopeProvider.CreateScope(autoComplete: true)) { - scope.ReadLock(Constants.Locks.MemberTree); - var query = Query().Where(x => x.Username.Equals(username)); - return _memberRepository.Get(query).FirstOrDefault(); + scope.ReadLock(Constants.Locks.MemberTree); + return _memberRepository.GetByUsername(username); } } @@ -806,12 +801,17 @@ namespace Umbraco.Core.Services.Implement #region Save - /// - /// Saves an - /// - /// to Save - /// Optional parameter to raise events. - /// Default is True otherwise set to False to not raise events + /// + public void SetLastLogin(string username, DateTime date) + { + using (var scope = ScopeProvider.CreateScope()) + { + _memberRepository.SetLastLogin(username, date); + scope.Complete(); + } + } + + /// public void Save(IMember member, bool raiseEvents = true) { //trimming username and email to make sure we have no trailing space @@ -847,12 +847,7 @@ namespace Umbraco.Core.Services.Implement } } - /// - /// Saves a list of objects - /// - /// to save - /// Optional parameter to raise events. - /// Default is True otherwise set to False to not raise events + /// public void Save(IEnumerable members, bool raiseEvents = true) { var membersA = members.ToArray(); diff --git a/src/Umbraco.Core/Services/Implement/UserService.cs b/src/Umbraco.Core/Services/Implement/UserService.cs index 6f5cbfca56..bf732263a9 100644 --- a/src/Umbraco.Core/Services/Implement/UserService.cs +++ b/src/Umbraco.Core/Services/Implement/UserService.cs @@ -254,6 +254,13 @@ namespace Umbraco.Core.Services.Implement } } + // explicit implementation because we don't need it now but due to the way that the members membership provider is put together + // this method must exist in this service as an implementation (legacy) + void IMembershipMemberService.SetLastLogin(string username, DateTime date) + { + throw new NotSupportedException("This method is not implemented or supported for users"); + } + /// /// Saves an /// diff --git a/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs b/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs index bbefb79f6b..b2efbd34b8 100644 --- a/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs +++ b/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs @@ -16,6 +16,7 @@ using Umbraco.Tests.Testing; using Umbraco.Core.Persistence; using Umbraco.Core.PropertyEditors; using System; +using Umbraco.Core.Persistence.Dtos; namespace Umbraco.Tests.Persistence.Repositories { @@ -76,12 +77,44 @@ namespace Umbraco.Tests.Persistence.Repositories return new UserGroupRepository(accessor, AppCaches.Disabled, Logger); } + [Test] + public void Validate_Login_Session() + { + // Arrange + var provider = TestObjects.GetScopeProvider(Logger); + var user = MockedUser.CreateUser(); + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + repository.Save(user); + } + + using (var scope = provider.CreateScope(autoComplete: true)) + { + var repository = CreateRepository(provider); + var sessionId = repository.CreateLoginSession(user.Id, "1.2.3.4"); + + // manually update this record to be in the past + scope.Database.Execute(SqlContext.Sql() + .Update(u => u.Set(x => x.LoggedOutUtc, DateTime.UtcNow.AddDays(-100))) + .Where(x => x.SessionId == sessionId)); + + var isValid = repository.ValidateLoginSession(user.Id, sessionId); + Assert.IsFalse(isValid); + + // create a new one + sessionId = repository.CreateLoginSession(user.Id, "1.2.3.4"); + isValid = repository.ValidateLoginSession(user.Id, sessionId); + Assert.IsTrue(isValid); + } + } + [Test] public void Can_Perform_Add_On_UserRepository() { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -101,7 +134,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -125,7 +158,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -150,7 +183,7 @@ namespace Umbraco.Tests.Persistence.Repositories // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var userRepository = CreateRepository(provider); var contentRepository = CreateContentRepository(provider, out var contentTypeRepo); @@ -209,7 +242,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -237,7 +270,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); var userGroupRepository = CreateUserGroupRepository(provider); @@ -260,7 +293,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -280,7 +313,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -301,7 +334,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -322,7 +355,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -341,7 +374,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -360,7 +393,7 @@ namespace Umbraco.Tests.Persistence.Repositories public void Can_Get_Paged_Results_By_Query_And_Filter_And_Groups() { var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -393,7 +426,7 @@ namespace Umbraco.Tests.Persistence.Repositories public void Can_Get_Paged_Results_With_Filter_And_Groups() { var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); @@ -426,7 +459,7 @@ namespace Umbraco.Tests.Persistence.Repositories { // Arrange var provider = TestObjects.GetScopeProvider(Logger); - using (var scope = provider.CreateScope()) + using (var scope = provider.CreateScope(autoComplete: true)) { var repository = CreateRepository(provider); var userGroupRepository = CreateUserGroupRepository(provider); diff --git a/src/Umbraco.Tests/Services/MemberServiceTests.cs b/src/Umbraco.Tests/Services/MemberServiceTests.cs index 1385bbe0b3..816bc6d7dd 100644 --- a/src/Umbraco.Tests/Services/MemberServiceTests.cs +++ b/src/Umbraco.Tests/Services/MemberServiceTests.cs @@ -48,6 +48,60 @@ namespace Umbraco.Tests.Services ((MemberService)ServiceContext.MemberService).MembershipProvider = provider; } + [Test] + public void Can_Update_Member_Property_Value() + { + IMemberType memberType = MockedContentTypes.CreateSimpleMemberType(); + ServiceContext.MemberTypeService.Save(memberType); + IMember member = MockedMember.CreateSimpleMember(memberType, "hello", "helloworld@test123.com", "hello", "hello"); + member.SetValue("title", "title of mine"); + ServiceContext.MemberService.Save(member); + + // re-get + member = ServiceContext.MemberService.GetById(member.Id); + member.SetValue("title", "another title of mine"); + ServiceContext.MemberService.Save(member); + + // re-get + member = ServiceContext.MemberService.GetById(member.Id); + Assert.AreEqual("another title of mine", member.GetValue("title")); + } + + [Test] + public void Can_Get_By_Username() + { + var memberType = ServiceContext.MemberTypeService.Get("member"); + IMember member = new Member("xname", "xemail", "xusername", "xrawpassword", memberType, true); + ServiceContext.MemberService.Save(member); + + var member2 = ServiceContext.MemberService.GetByUsername(member.Username); + + Assert.IsNotNull(member2); + Assert.AreEqual(member.Email, member2.Email); + } + + [Test] + public void Can_Set_Last_Login_Date() + { + var now = DateTime.Now; + var memberType = ServiceContext.MemberTypeService.Get("member"); + IMember member = new Member("xname", "xemail", "xusername", "xrawpassword", memberType, true) + { + LastLoginDate = now, + UpdateDate = now + }; + ServiceContext.MemberService.Save(member); + + var newDate = now.AddDays(10); + ServiceContext.MemberService.SetLastLogin(member.Username, newDate); + + //re-get + member = ServiceContext.MemberService.GetById(member.Id); + + Assert.That(member.LastLoginDate, Is.EqualTo(newDate).Within(1).Seconds); + Assert.That(member.UpdateDate, Is.EqualTo(newDate).Within(1).Seconds); + } + [Test] public void Can_Create_Member_With_Properties() { diff --git a/src/Umbraco.Web/Cache/DistributedCacheExtensions.cs b/src/Umbraco.Web/Cache/DistributedCacheExtensions.cs index b00a4818f6..f360d37d03 100644 --- a/src/Umbraco.Web/Cache/DistributedCacheExtensions.cs +++ b/src/Umbraco.Web/Cache/DistributedCacheExtensions.cs @@ -128,15 +128,16 @@ namespace Umbraco.Web.Cache public static void RefreshMemberCache(this DistributedCache dc, params IMember[] members) { - dc.Refresh(MemberCacheRefresher.UniqueId, x => x.Id, members); + if (members.Length == 0) return; + dc.RefreshByPayload(MemberCacheRefresher.UniqueId, members.Select(x => new MemberCacheRefresher.JsonPayload(x.Id, x.Username))); } public static void RemoveMemberCache(this DistributedCache dc, params IMember[] members) { - dc.Remove(MemberCacheRefresher.UniqueId, x => x.Id, members); + if (members.Length == 0) return; + dc.RefreshByPayload(MemberCacheRefresher.UniqueId, members.Select(x => new MemberCacheRefresher.JsonPayload(x.Id, x.Username))); } - #endregion #region MemberGroupCache diff --git a/src/Umbraco.Web/Cache/MemberCacheRefresher.cs b/src/Umbraco.Web/Cache/MemberCacheRefresher.cs index 1565b1c849..736a858af3 100644 --- a/src/Umbraco.Web/Cache/MemberCacheRefresher.cs +++ b/src/Umbraco.Web/Cache/MemberCacheRefresher.cs @@ -1,4 +1,6 @@ -using System; +using Newtonsoft.Json; +using System; +using System.Collections.Generic; using Umbraco.Core.Cache; using Umbraco.Core.Models; using Umbraco.Core.Persistence.Repositories; @@ -7,14 +9,30 @@ using Umbraco.Core.Services; namespace Umbraco.Web.Cache { - public sealed class MemberCacheRefresher : TypedCacheRefresherBase + public sealed class MemberCacheRefresher : PayloadCacheRefresherBase { private readonly IdkMap _idkMap; + private readonly LegacyMemberCacheRefresher _legacyMemberRefresher; public MemberCacheRefresher(AppCaches appCaches, IdkMap idkMap) : base(appCaches) { _idkMap = idkMap; + _legacyMemberRefresher = new LegacyMemberCacheRefresher(this, appCaches); + } + + public class JsonPayload + { + [JsonConstructor] + public JsonPayload(int id, string username) + { + Id = id; + Username = username; + } + + public int Id { get; } + public string Username { get; } + } #region Define @@ -31,38 +49,45 @@ namespace Umbraco.Web.Cache #region Refresher + public override void Refresh(JsonPayload[] payloads) + { + ClearCache(payloads); + base.Refresh(payloads); + } + public override void Refresh(int id) { - ClearCache(id); + ClearCache(new JsonPayload(id, null)); base.Refresh(id); } public override void Remove(int id) { - ClearCache(id); + ClearCache(new JsonPayload(id, null)); base.Remove(id); } - public override void Refresh(IMember instance) - { - ClearCache(instance.Id); - base.Refresh(instance); - } + [Obsolete("This is no longer used and will be removed from the codebase in the future")] + public void Refresh(IMember instance) => _legacyMemberRefresher.Refresh(instance); - public override void Remove(IMember instance) - { - ClearCache(instance.Id); - base.Remove(instance); - } + [Obsolete("This is no longer used and will be removed from the codebase in the future")] + public void Remove(IMember instance) => _legacyMemberRefresher.Remove(instance); - private void ClearCache(int id) + private void ClearCache(params JsonPayload[] payloads) { - _idkMap.ClearCache(id); AppCaches.ClearPartialViewCache(); - var memberCache = AppCaches.IsolatedCaches.Get(); - if (memberCache) - memberCache.Result.Clear(RepositoryCacheKeys.GetKey(id)); + + foreach (var p in payloads) + { + _idkMap.ClearCache(p.Id); + if (memberCache) + { + memberCache.Result.Clear(RepositoryCacheKeys.GetKey(p.Id)); + memberCache.Result.Clear(RepositoryCacheKeys.GetKey(p.Username)); + } + } + } #endregion @@ -75,5 +100,38 @@ namespace Umbraco.Web.Cache } #endregion + + #region Backwards Compat + + // TODO: this is here purely for backwards compat but should be removed in netcore + private class LegacyMemberCacheRefresher : TypedCacheRefresherBase + { + private readonly MemberCacheRefresher _parent; + + public LegacyMemberCacheRefresher(MemberCacheRefresher parent, AppCaches appCaches) : base(appCaches) + { + _parent = parent; + } + + public override Guid RefresherUniqueId => _parent.RefresherUniqueId; + + public override string Name => _parent.Name; + + protected override MemberCacheRefresher This => _parent; + + public override void Refresh(IMember instance) + { + _parent.ClearCache(new JsonPayload(instance.Id, instance.Username)); + base.Refresh(instance.Id); + } + + public override void Remove(IMember instance) + { + _parent.ClearCache(new JsonPayload(instance.Id, instance.Username)); + base.Remove(instance); + } + } + + #endregion } } diff --git a/src/Umbraco.Web/Security/MembershipHelper.cs b/src/Umbraco.Web/Security/MembershipHelper.cs index f74897d565..aa0623e9e6 100644 --- a/src/Umbraco.Web/Security/MembershipHelper.cs +++ b/src/Umbraco.Web/Security/MembershipHelper.cs @@ -261,8 +261,9 @@ namespace Umbraco.Web.Security { return false; } - //Set member online - var member = provider.GetUser(username, true); + // Get the member, do not set to online - this is done implicitly as part of ValidateUser which is consistent with + // how the .NET framework SqlMembershipProvider works. Passing in true will just cause more unnecessary SQL queries/locks. + var member = provider.GetUser(username, false); if (member == null) { //this should not happen @@ -778,33 +779,17 @@ namespace Umbraco.Web.Security /// private IMember GetCurrentPersistedMember() { - return _appCaches.RequestCache.GetCacheItem( - GetCacheKey("GetCurrentPersistedMember"), () => - { - var provider = _membershipProvider; + var provider = _membershipProvider; - if (provider.IsUmbracoMembershipProvider() == false) - { - throw new NotSupportedException("An IMember model can only be retrieved when using the built-in Umbraco membership providers"); - } - var username = provider.GetCurrentUserName(); - var member = _memberService.GetByUsername(username); - return member; - }); - } - - private static string GetCacheKey(string key, params object[] additional) - { - var sb = new StringBuilder(); - sb.Append(typeof (MembershipHelper).Name); - sb.Append("-"); - sb.Append(key); - foreach (var s in additional) + if (provider.IsUmbracoMembershipProvider() == false) { - sb.Append("-"); - sb.Append(s); + throw new NotSupportedException("An IMember model can only be retrieved when using the built-in Umbraco membership providers"); } - return sb.ToString(); + var username = provider.GetCurrentUserName(); + + // The result of this is cached by the MemberRepository + var member = _memberService.GetByUsername(username); + return member; } } diff --git a/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs b/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs index 1f7e2c8084..bf9ee654c4 100644 --- a/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs +++ b/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs @@ -348,15 +348,16 @@ namespace Umbraco.Web.Security.Providers if (userIsOnline) { - member.LastLoginDate = DateTime.Now; - member.UpdateDate = DateTime.Now; - //don't raise events for this! It just sets the member dates, if we do raise events this will - // cause all distributed cache to execute - which will clear out some caches we don't want. - // http://issues.umbraco.org/issue/U4-3451 - // when upgrading from 7.2 to 7.3 trying to save will throw if (UmbracoVersion.Current >= new Version(7, 3, 0, 0)) - MemberService.Save(member, false); + { + var now = DateTime.Now; + // update the database data directly instead of a full member save which requires DB locks + MemberService.SetLastLogin(username, now); + member.LastLoginDate = now; + member.UpdateDate = now; + } + } return ConvertToMembershipUser(member); @@ -555,6 +556,8 @@ namespace Umbraco.Web.Security.Providers var authenticated = CheckPassword(password, member.RawPasswordValue); + var requiresFullSave = false; + if (authenticated == false) { // TODO: Increment login attempts - lock if too many. @@ -574,6 +577,8 @@ namespace Umbraco.Web.Security.Providers { Current.Logger.Info("Login attempt failed for username {Username} from IP address {IpAddress}", username, GetCurrentRequestIpAddress()); } + + requiresFullSave = true; } else { @@ -581,6 +586,7 @@ namespace Umbraco.Web.Security.Providers { //we have successfully logged in, reset the AccessFailedCount member.FailedPasswordAttempts = 0; + requiresFullSave = true; } member.LastLoginDate = DateTime.Now; @@ -588,15 +594,23 @@ namespace Umbraco.Web.Security.Providers Current.Logger.Info("Login attempt succeeded for username {Username} from IP address {IpAddress}", username, GetCurrentRequestIpAddress()); } - //don't raise events for this! It just sets the member dates, if we do raise events this will + // don't raise events for this! It just sets the member dates, if we do raise events this will // cause all distributed cache to execute - which will clear out some caches we don't want. // http://issues.umbraco.org/issue/U4-3451 // TODO: In v8 we aren't going to have an overload to disable events, so we'll need to make a different method // for this type of thing (i.e. UpdateLastLogin or similar). - // when upgrading from 7.2 to 7.3 trying to save will throw - if (UmbracoVersion.Current >= new Version(7, 3, 0, 0)) - MemberService.Save(member, false); + if (requiresFullSave) + { + // when upgrading from 7.2 to 7.3 trying to save will throw + if (UmbracoVersion.Current >= new Version(7, 3, 0, 0)) + MemberService.Save(member, false); + } + else + { + // set the last login date without full save (fast, no locks) + MemberService.SetLastLogin(member.Username, member.LastLoginDate); + } return new ValidateUserResult {