From 8eab1f1b557dc26c1a346928922bbe6acd09c896 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 30 Jul 2020 22:56:31 +1000 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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);