diff --git a/src/Umbraco.Core/Persistence/NPocoSqlExtensions.cs b/src/Umbraco.Core/Persistence/NPocoSqlExtensions.cs index 178b68b4f8..36777a42fd 100644 --- a/src/Umbraco.Core/Persistence/NPocoSqlExtensions.cs +++ b/src/Umbraco.Core/Persistence/NPocoSqlExtensions.cs @@ -916,6 +916,82 @@ namespace Umbraco.Core.Persistence #endregion + #region Hints + + /// + /// Appends the relevant ForUpdate hint. + /// + /// The Sql statement. + /// The Sql statement. + public static Sql ForUpdate(this Sql sql) + { + // MySql wants "FOR UPDATE" at the end, and T-Sql wants "WITH (UPDLOCK)" in the FROM statement, + // and we want to implement it in the least expensive way, so parsing the entire string here is + // a no, so we use reflection to work on the Sql expression before it is built. + // TODO propose a clean way to do that type of thing in NPoco + + if (sql.SqlContext.DatabaseType.IsMySql()) + { + sql.Append("FOR UDPATE"); + return sql; + } + + // go find the first FROM clause, and append the lock hint + Sql s = sql; + var updated = false; + + while (s != null) + { + var sqlText = SqlInspector.GetSqlText(s); + if (sqlText.StartsWith("FROM ", StringComparison.OrdinalIgnoreCase)) + { + SqlInspector.SetSqlText(s, sqlText + " WITH (UPDLOCK)"); + updated = true; + break; + } + + s = SqlInspector.GetSqlRhs(sql); + } + + if (updated) + SqlInspector.Reset(sql); + + return sql; + } + + #endregion + + #region Sql Inspection + + private static SqlInspectionUtilities _sqlInspector; + + private static SqlInspectionUtilities SqlInspector => _sqlInspector ?? (_sqlInspector = new SqlInspectionUtilities()); + + private class SqlInspectionUtilities + { + private readonly Func _getSqlText; + private readonly Action _setSqlText; + private readonly Func _getSqlRhs; + private readonly Action _setSqlFinal; + + public SqlInspectionUtilities() + { + (_getSqlText, _setSqlText) = ReflectionUtilities.EmitFieldGetterAndSetter("_sql"); + _getSqlRhs = ReflectionUtilities.EmitFieldGetter("_rhs"); + _setSqlFinal = ReflectionUtilities.EmitFieldSetter("_sqlFinal"); + } + + public string GetSqlText(Sql sql) => _getSqlText(sql); + + public void SetSqlText(Sql sql, string sqlText) => _setSqlText(sql, sqlText); + + public Sql GetSqlRhs(Sql sql) => _getSqlRhs(sql); + + public void Reset(Sql sql) => _setSqlFinal(sql, null); + } + + #endregion + #region Utilities private static string[] GetColumns(this Sql sql, string tableAlias = null, string referenceName = null, Expression>[] columnExpressions = null, bool withAlias = true) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs index 05fe456bbd..0ec5f9353b 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs @@ -191,7 +191,20 @@ ORDER BY colName"; public bool ValidateLoginSession(int userId, Guid sessionId) { - var found = Database.FirstOrDefault("WHERE sessionId=@sessionId", new {sessionId = sessionId}); + // with RepeatableRead transaction mode, read-then-update operations can + // cause deadlocks, and the ForUpdate() hint is required to tell the database + // to acquire an exclusive lock when reading + + // that query is going to run a *lot*, make it a template + var t = SqlContext.Templates.Get("Umbraco.Core.UserRepository.ValidateLoginSession", s => s + .Select() + .From() + .Where(x => x.SessionId == SqlTemplate.Arg("sessionId")) + .ForUpdate()); + + var sql = t.Sql(sessionId); + + var found = Database.Query(sql).FirstOrDefault(); if (found == null || found.UserId != userId || found.LoggedOutUtc.HasValue) return false; @@ -211,34 +224,21 @@ ORDER BY colName"; public int ClearLoginSessions(int userId) { - //TODO: I know this doesn't follow the normal repository conventions which would require us to crete 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 count = Database.ExecuteScalar("SELECT COUNT(*) FROM umbracoUserLogin WHERE userId=@userId", new { userId = userId }); - Database.Execute("DELETE FROM umbracoUserLogin WHERE userId=@userId", new {userId = userId}); - return count; + return Database.Delete(Sql().Where(x => x.UserId == userId)); } public int ClearLoginSessions(TimeSpan timespan) { - //TODO: I know this doesn't follow the normal repository conventions which would require us to crete 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 fromDate = DateTime.UtcNow - timespan; - - var count = Database.ExecuteScalar("SELECT COUNT(*) FROM umbracoUserLogin WHERE lastValidatedUtc=@fromDate", new { fromDate = fromDate }); - Database.Execute("DELETE FROM umbracoUserLogin WHERE lastValidatedUtc=@fromDate", new { fromDate = fromDate }); - return count; + return Database.Delete(Sql().Where(x => x.LastValidatedUtc < fromDate)); } public void ClearLoginSession(Guid sessionId) { - //TODO: I know this doesn't follow the normal repository conventions which would require us to crete 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 - Database.Execute("UPDATE umbracoUserLogin SET loggedOutUtc=@now WHERE sessionId=@sessionId", - new { now = DateTime.UtcNow, sessionId = sessionId }); + // fixme why is that one updating and not deleting? + Database.Execute(Sql() + .Update(u => u.Set(x => x.LoggedOutUtc, DateTime.UtcNow)) + .Where(x => x.SessionId == sessionId)); } protected override IEnumerable PerformGetAll(params int[] ids) @@ -718,7 +718,7 @@ ORDER BY colName"; if (string.IsNullOrWhiteSpace(orderBy)) throw new ArgumentException("Value cannot be null or whitespace.", nameof(orderBy)); Sql filterSql = null; - var customFilterWheres = filter != null ? filter.GetWhereClauses().ToArray() : null; + var customFilterWheres = filter?.GetWhereClauses().ToArray(); var hasCustomFilter = customFilterWheres != null && customFilterWheres.Length > 0; if (hasCustomFilter || includeUserGroups != null && includeUserGroups.Length > 0 diff --git a/src/Umbraco.Core/ReflectionUtilities.cs b/src/Umbraco.Core/ReflectionUtilities.cs index db0cde87db..3cca2b5b62 100644 --- a/src/Umbraco.Core/ReflectionUtilities.cs +++ b/src/Umbraco.Core/ReflectionUtilities.cs @@ -21,6 +21,96 @@ namespace Umbraco.Core /// public static partial class ReflectionUtilities { + #region Fields + + /// + /// Emits a field getter. + /// + /// The declaring type. + /// The field type. + /// The name of the field. + /// A field getter function. + /// Occurs when is null or empty. + /// Occurs when the field does not exist. + /// Occurs when does not match the type of the field. + public static Func EmitFieldGetter(string fieldName) + { + var field = GetField(fieldName); + return EmitFieldGetter(field); + } + + /// + /// Emits a field setter. + /// + /// The declaring type. + /// The field type. + /// The name of the field. + /// A field setter action. + /// Occurs when is null or empty. + /// Occurs when the field does not exist. + /// Occurs when does not match the type of the field. + public static Action EmitFieldSetter(string fieldName) + { + var field = GetField(fieldName); + return EmitFieldSetter(field); + } + + /// + /// Emits a field getter and setter. + /// + /// The declaring type. + /// The field type. + /// The name of the field. + /// A field getter and setter functions. + /// Occurs when is null or empty. + /// Occurs when the field does not exist. + /// Occurs when does not match the type of the field. + public static (Func, Action) EmitFieldGetterAndSetter(string fieldName) + { + var field = GetField(fieldName); + return (EmitFieldGetter(field), EmitFieldSetter(field)); + } + + private static FieldInfo GetField(string fieldName) + { + if (string.IsNullOrWhiteSpace(fieldName)) throw new ArgumentNullOrEmptyException(nameof(fieldName)); + + // get the field + var field = typeof(TDeclaring).GetField(fieldName, BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic); + if (field == null) throw new InvalidOperationException($"Could not find field {typeof(TDeclaring)}.{fieldName}."); + + // validate field type + if (field.FieldType != typeof(TValue)) // strict + throw new ArgumentException($"Value type {typeof(TValue)} does not match field {typeof(TDeclaring)}.{fieldName} type {field.FieldType}."); + + return field; + } + + private static Func EmitFieldGetter(FieldInfo field) + { + // emit + var (dm, ilgen) = CreateIlGenerator(field.DeclaringType?.Module, new [] { typeof(TDeclaring) }, typeof(TValue)); + ilgen.Emit(OpCodes.Ldarg_0); + ilgen.Emit(OpCodes.Ldfld, field); + ilgen.Return(); + + return (Func) (object) dm.CreateDelegate(typeof(Func)); + } + + private static Action EmitFieldSetter(FieldInfo field) + { + // emit + var (dm, ilgen) = CreateIlGenerator(field.DeclaringType?.Module, new [] { typeof(TDeclaring), typeof(TValue) }, typeof(void)); + ilgen.Emit(OpCodes.Ldarg_0); + ilgen.Emit(OpCodes.Ldarg_1); + ilgen.Emit(OpCodes.Stfld, field); + ilgen.Return(); + + return (Action) (object) dm.CreateDelegate(typeof(Action)); + } + + #endregion + #region Properties /// @@ -207,7 +297,7 @@ namespace Umbraco.Core /// is specified and does not match the function's returned type. public static TLambda EmitCtor(bool mustExist = true, Type declaring = null) { - (_, var lambdaParameters, var lambdaReturned) = AnalyzeLambda(true, true); + var (_, lambdaParameters, lambdaReturned) = AnalyzeLambda(true, true); // determine returned / declaring type if (declaring == null) declaring = lambdaReturned; @@ -239,7 +329,7 @@ namespace Umbraco.Core { if (ctor == null) throw new ArgumentNullException(nameof(ctor)); - (_, var lambdaParameters, var lambdaReturned) = AnalyzeLambda(true, true); + var (_, lambdaParameters, lambdaReturned) = AnalyzeLambda(true, true); return EmitCtorSafe(lambdaParameters, lambdaReturned, ctor); } @@ -281,7 +371,7 @@ namespace Umbraco.Core { if (ctor == null) throw new ArgumentNullException(nameof(ctor)); - (_, var lambdaParameters, var lambdaReturned) = AnalyzeLambda(true, true); + var (_, lambdaParameters, lambdaReturned) = AnalyzeLambda(true, true); // emit - unsafe - use lambda's args and assume they are correct return EmitCtor(lambdaReturned, lambdaParameters, ctor); @@ -293,7 +383,7 @@ namespace Umbraco.Core var ctorParameters = GetParameters(ctor); // emit - (var dm, var ilgen) = CreateIlGenerator(ctor.DeclaringType?.Module, lambdaParameters, declaring); + var (dm, ilgen) = CreateIlGenerator(ctor.DeclaringType?.Module, lambdaParameters, declaring); EmitLdargs(ilgen, lambdaParameters, ctorParameters); ilgen.Emit(OpCodes.Newobj, ctor); // ok to just return, it's only objects ilgen.Return(); @@ -342,7 +432,7 @@ namespace Umbraco.Core { if (string.IsNullOrWhiteSpace(methodName)) throw new ArgumentNullOrEmptyException(nameof(methodName)); - (var lambdaDeclaring, var lambdaParameters, var lambdaReturned) = AnalyzeLambda(true, out var isFunction); + var (lambdaDeclaring, lambdaParameters, lambdaReturned) = AnalyzeLambda(true, out var isFunction); // get the method infos var method = declaring.GetMethod(methodName, BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Static, null, lambdaParameters, null); @@ -374,7 +464,7 @@ namespace Umbraco.Core var methodParameters = method.GetParameters().Select(x => x.ParameterType).ToArray(); var isStatic = method.IsStatic; - (var lambdaDeclaring, var lambdaParameters, var lambdaReturned) = AnalyzeLambda(isStatic, out var isFunction); + var (lambdaDeclaring, lambdaParameters, lambdaReturned) = AnalyzeLambda(isStatic, out var isFunction); // if not static, then the first lambda arg must be the method declaring type if (!isStatic && (methodDeclaring == null || !methodDeclaring.IsAssignableFrom(lambdaDeclaring))) @@ -408,7 +498,7 @@ namespace Umbraco.Core if (method == null) throw new ArgumentNullException(nameof(method)); var isStatic = method.IsStatic; - (var lambdaDeclaring, var lambdaParameters, var lambdaReturned) = AnalyzeLambda(isStatic, out _); + var (lambdaDeclaring, lambdaParameters, lambdaReturned) = AnalyzeLambda(isStatic, out _); // emit - unsafe - use lambda's args and assume they are correct return EmitMethod(lambdaDeclaring, lambdaReturned, lambdaParameters, method); @@ -433,7 +523,7 @@ namespace Umbraco.Core throw new ArgumentNullOrEmptyException(nameof(methodName)); // validate lambda type - (var lambdaDeclaring, var lambdaParameters, var lambdaReturned) = AnalyzeLambda(false, out var isFunction); + var (lambdaDeclaring, lambdaParameters, lambdaReturned) = AnalyzeLambda(false, out var isFunction); // get the method infos var method = lambdaDeclaring.GetMethod(methodName, BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance, null, lambdaParameters, null); @@ -464,7 +554,7 @@ namespace Umbraco.Core var methodArgTypes = GetParameters(method, withDeclaring: !method.IsStatic); // emit IL - (var dm, var ilgen) = CreateIlGenerator(method.DeclaringType?.Module, parameters, lambdaReturned); + var (dm, ilgen) = CreateIlGenerator(method.DeclaringType?.Module, parameters, lambdaReturned); EmitLdargs(ilgen, parameters, methodArgTypes); ilgen.CallMethod(method); EmitOutputAdapter(ilgen, lambdaReturned, method.ReturnType); @@ -486,7 +576,7 @@ namespace Umbraco.Core { var typeLambda = typeof(TLambda); - (var declaring, var parameters, var returned) = AnalyzeLambda(isStatic, out var maybeFunction); + var (declaring, parameters, returned) = AnalyzeLambda(isStatic, out var maybeFunction); if (isFunction) { diff --git a/src/Umbraco.Tests/Clr/ReflectionUtilitiesTests.cs b/src/Umbraco.Tests/Clr/ReflectionUtilitiesTests.cs index 0b3d5235c8..31f936213d 100644 --- a/src/Umbraco.Tests/Clr/ReflectionUtilitiesTests.cs +++ b/src/Umbraco.Tests/Clr/ReflectionUtilitiesTests.cs @@ -528,6 +528,26 @@ namespace Umbraco.Tests.Clr Assert.AreEqual(1, values5D["intValue2"]); // JsonProperty changes property name } + [Test] + public void EmitFieldGetterSetterEmits() + { + var getter1 = ReflectionUtilities.EmitFieldGetter("Field1"); + var getter2 = ReflectionUtilities.EmitFieldGetter("Field2"); + var c = new Class1(); + Assert.AreEqual(33, getter1(c)); + Assert.AreEqual(66, getter2(c)); + + var setter2 = ReflectionUtilities.EmitFieldSetter("Field2"); + setter2(c, 99); + Assert.AreEqual(99, getter2(c)); + + // works on readonly fields! + var (getter3, setter3) = ReflectionUtilities.EmitFieldGetterAndSetter("Field3"); + Assert.AreEqual(22, getter3(c)); + setter3(c, 44); + Assert.AreEqual(44, getter3(c)); + } + // fixme - missing tests specifying 'returned' on method, property #region IL Code @@ -550,6 +570,12 @@ namespace Umbraco.Tests.Clr // conv.i4 public void SetIntValue2(Class4 object4, object d) => object4.IntValue = (int) (double) d; + // get field + public int GetIntField(Class1 object1) => object1.Field1; + + // set field + public void SetIntField(Class1 object1, int i) => object1.Field1 = i; + #endregion #region Test Objects @@ -583,6 +609,10 @@ namespace Umbraco.Tests.Clr public int Value2 { set { } } public int Value3 { get { return 42; } set { } } private int ValueP1 => 42; + + public int Field1 = 33; + private int Field2 = 66; + public readonly int Field3 = 22; } public class Class2 { } diff --git a/src/Umbraco.Tests/Persistence/NPocoTests/NPocoSqlTests.cs b/src/Umbraco.Tests/Persistence/NPocoTests/NPocoSqlTests.cs index a7b75cbd6d..6fa2af74cf 100644 --- a/src/Umbraco.Tests/Persistence/NPocoTests/NPocoSqlTests.cs +++ b/src/Umbraco.Tests/Persistence/NPocoTests/NPocoSqlTests.cs @@ -1,4 +1,5 @@ -using System.Diagnostics; +using System; +using System.Diagnostics; using NUnit.Framework; using Umbraco.Core; using Umbraco.Core.Persistence; @@ -293,5 +294,24 @@ namespace Umbraco.Tests.Persistence.NPocoTests Debug.Print(sql.SQL); } + + [Test] + public void ForUpdate() + { + var sessionId = Guid.NewGuid(); + + var sql = Sql() + .SelectAll() + .From() + .Where(x => x.SessionId == sessionId); + + sql.WriteToConsole(); + Assert.AreEqual("SELECT * FROM [umbracoUserLogin] WHERE (([umbracoUserLogin].[sessionId] = @0))", sql.SQL.NoCrLf()); + + sql = sql.ForUpdate(); + + sql.WriteToConsole(); + Assert.AreEqual("SELECT * FROM [umbracoUserLogin] WITH (UPDLOCK) WHERE (([umbracoUserLogin].[sessionId] = @0))", sql.SQL.NoCrLf()); + } } }