From d0435f6625e6a3925547b31ec9a736ff35e5e1db Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 15 Oct 2019 00:04:41 +1100 Subject: [PATCH 1/5] Moves read/write locking and isolation level to the responsibility of the sql provider based on the db type --- .../Persistence/DatabasenodeLockExtensions.cs | 43 ------------------- .../SqlSyntax/ISqlSyntaxProvider.cs | 9 ++++ .../SqlSyntax/SqlCeSyntaxProvider.cs | 37 ++++++++++++++++ .../SqlSyntax/SqlServerSyntaxProvider.cs | 38 ++++++++++++++++ .../SqlSyntax/SqlSyntaxProviderBase.cs | 7 ++- .../Persistence/UmbracoDatabase.cs | 7 +-- src/Umbraco.Core/Scoping/Scope.cs | 36 ++-------------- src/Umbraco.Core/Umbraco.Core.csproj | 1 - .../Models/UserExtensionsTests.cs | 3 ++ src/Umbraco.Tests/Persistence/LocksTests.cs | 13 +++--- 10 files changed, 104 insertions(+), 90 deletions(-) delete mode 100644 src/Umbraco.Core/Persistence/DatabasenodeLockExtensions.cs diff --git a/src/Umbraco.Core/Persistence/DatabasenodeLockExtensions.cs b/src/Umbraco.Core/Persistence/DatabasenodeLockExtensions.cs deleted file mode 100644 index 48edee3c94..0000000000 --- a/src/Umbraco.Core/Persistence/DatabasenodeLockExtensions.cs +++ /dev/null @@ -1,43 +0,0 @@ -using System; -using System.Collections.Generic; -using System.Data; -using System.Runtime.CompilerServices; - -namespace Umbraco.Core.Persistence -{ - internal static class DatabaseNodeLockExtensions - { - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static void ValidateDatabase(IUmbracoDatabase database) - { - if (database == null) - throw new ArgumentNullException("database"); - if (database.GetCurrentTransactionIsolationLevel() < IsolationLevel.RepeatableRead) - throw new InvalidOperationException("A transaction with minimum RepeatableRead isolation level is required."); - } - - // updating a record within a repeatable-read transaction gets an exclusive lock on - // that record which will be kept until the transaction is ended, effectively locking - // out all other accesses to that record - thus obtaining an exclusive lock over the - // protected resources. - public static void AcquireLockNodeWriteLock(this IUmbracoDatabase database, int nodeId) - { - ValidateDatabase(database); - - database.Execute("UPDATE umbracoLock SET value = (CASE WHEN (value=1) THEN -1 ELSE 1 END) WHERE id=@id", - new { @id = nodeId }); - } - - // reading a record within a repeatable-read transaction gets a shared lock on - // that record which will be kept until the transaction is ended, effectively preventing - // other write accesses to that record - thus obtaining a shared lock over the protected - // resources. - public static void AcquireLockNodeReadLock(this IUmbracoDatabase database, int nodeId) - { - ValidateDatabase(database); - - database.ExecuteScalar("SELECT value FROM umbracoLock WHERE id=@id", - new { @id = nodeId }); - } - } -} diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/ISqlSyntaxProvider.cs b/src/Umbraco.Core/Persistence/SqlSyntax/ISqlSyntaxProvider.cs index 55625ff04e..7ae001bf24 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/ISqlSyntaxProvider.cs +++ b/src/Umbraco.Core/Persistence/SqlSyntax/ISqlSyntaxProvider.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Data; using System.Text.RegularExpressions; using NPoco; using Umbraco.Core.Persistence.DatabaseAnnotations; @@ -76,6 +77,11 @@ namespace Umbraco.Core.Persistence.SqlSyntax string ConvertIntegerToOrderableString { get; } string ConvertDateToOrderableString { get; } string ConvertDecimalToOrderableString { get; } + + /// + /// Returns the default isolation level for the database + /// + IsolationLevel DefaultIsolationLevel { get; } IEnumerable GetTablesInSchema(IDatabase db); IEnumerable GetColumnsInSchema(IDatabase db); @@ -121,5 +127,8 @@ namespace Umbraco.Core.Persistence.SqlSyntax /// unspecified. /// bool TryGetDefaultConstraint(IDatabase db, string tableName, string columnName, out string constraintName); + + void ReadLock(IDatabase db, params int[] lockIds); + void WriteLock(IDatabase db, params int[] lockIds); } } diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs b/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs index cb4b7a5176..92ee934e52 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs +++ b/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs @@ -1,5 +1,7 @@ using System; using System.Collections.Generic; +using System.Data; +using System.Data.SqlServerCe; using System.Linq; using NPoco; using Umbraco.Core.Persistence.DatabaseAnnotations; @@ -52,6 +54,8 @@ namespace Umbraco.Core.Persistence.SqlSyntax return "(" + string.Join("+", args) + ")"; } + public override System.Data.IsolationLevel DefaultIsolationLevel => System.Data.IsolationLevel.RepeatableRead; + public override string FormatColumnRename(string tableName, string oldName, string newName) { //NOTE Sql CE doesn't support renaming a column, so a new column needs to be created, then copy data and finally remove old column @@ -152,6 +156,39 @@ where table_name=@0 and column_name=@1", tableName, columnName).FirstOrDefault() return result > 0; } + public override void WriteLock(IDatabase db, params int[] lockIds) + { + // soon as we get Database, a transaction is started + + if (db.Transaction.IsolationLevel < IsolationLevel.RepeatableRead) + throw new InvalidOperationException("A transaction with minimum RepeatableRead isolation level is required."); + + db.Execute(@"SET LOCK_TIMEOUT 1800;"); + // *not* using a unique 'WHERE IN' query here because the *order* of lockIds is important to avoid deadlocks + foreach (var lockId in lockIds) + { + var i = db.Execute(@"UPDATE umbracoLock SET value = value*-1 WHERE id=@id", new { id = lockId }); + if (i == 0) // ensure we are actually locking! + throw new Exception($"LockObject with id={lockId} does not exist."); + } + } + + public override void ReadLock(IDatabase db, params int[] lockIds) + { + // soon as we get Database, a transaction is started + + if (db.Transaction.IsolationLevel < IsolationLevel.RepeatableRead) + throw new InvalidOperationException("A transaction with minimum RepeatableRead isolation level is required."); + + // *not* using a unique 'WHERE IN' query here because the *order* of lockIds is important to avoid deadlocks + foreach (var lockId in lockIds) + { + var i = db.ExecuteScalar("SELECT value FROM umbracoLock WHERE id=@id", new { id = lockId }); + if (i == null) // ensure we are actually locking! + throw new Exception($"LockObject with id={lockId} does not exist."); + } + } + protected override string FormatIdentity(ColumnDefinition column) { return column.IsIdentity ? GetIdentityString(column) : string.Empty; diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs b/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs index fab7526a6b..7bf00522e3 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs +++ b/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Data; using System.Data.Common; +using System.Data.SqlClient; using System.Linq; using NPoco; using Umbraco.Core.Logging; @@ -179,6 +180,8 @@ namespace Umbraco.Core.Persistence.SqlSyntax return items.Select(x => x.TABLE_NAME).Cast().ToList(); } + public override IsolationLevel DefaultIsolationLevel => IsolationLevel.ReadCommitted; + public override IEnumerable GetColumnsInSchema(IDatabase db) { var items = db.Fetch("SELECT TABLE_NAME, COLUMN_NAME, ORDINAL_POSITION, COLUMN_DEFAULT, IS_NULLABLE, DATA_TYPE FROM INFORMATION_SCHEMA.COLUMNS WHERE TABLE_SCHEMA = (SELECT SCHEMA_NAME())"); @@ -246,6 +249,41 @@ where tbl.[name]=@0 and col.[name]=@1;", tableName, columnName) return result > 0; } + public override void WriteLock(IDatabase db, params int[] lockIds) + { + // soon as we get Database, a transaction is started + + if (db.Transaction.IsolationLevel < IsolationLevel.ReadCommitted) + throw new InvalidOperationException("A transaction with minimum ReadCommitted isolation level is required."); + + + // *not* using a unique 'WHERE IN' query here because the *order* of lockIds is important to avoid deadlocks + foreach (var lockId in lockIds) + { + db.Execute(@"SET LOCK_TIMEOUT 1800;"); + var i = db.Execute(@"UPDATE umbracoLock WITH (REPEATABLEREAD) SET value = value*-1 WHERE id=@id", new { id = lockId }); + if (i == 0) // ensure we are actually locking! + throw new InvalidOperationException($"LockObject with id={lockId} does not exist."); + } + } + + + public override void ReadLock(IDatabase db, params int[] lockIds) + { + // soon as we get Database, a transaction is started + + if (db.Transaction.IsolationLevel < IsolationLevel.ReadCommitted) + throw new InvalidOperationException("A transaction with minimum ReadCommitted isolation level is required."); + + // *not* using a unique 'WHERE IN' query here because the *order* of lockIds is important to avoid deadlocks + foreach (var lockId in lockIds) + { + var i = db.ExecuteScalar("SELECT value FROM umbracoLock WITH (REPEATABLEREAD) WHERE id=@id", new { id = lockId }); + if (i == null) // ensure we are actually locking! + throw new ArgumentException($"LockObject with id={lockId} does not exist.", nameof(lockIds)); + } + } + public override string FormatColumnRename(string tableName, string oldName, string newName) { return string.Format(RenameColumn, tableName, oldName, newName); diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs b/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs index 0c27ac2d50..b2e03df96e 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs +++ b/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs @@ -200,7 +200,9 @@ namespace Umbraco.Core.Persistence.SqlSyntax return "NVARCHAR"; } - + + public abstract IsolationLevel DefaultIsolationLevel { get; } + public virtual IEnumerable GetTablesInSchema(IDatabase db) { return new List(); @@ -225,6 +227,9 @@ namespace Umbraco.Core.Persistence.SqlSyntax public abstract bool TryGetDefaultConstraint(IDatabase db, string tableName, string columnName, out string constraintName); + public abstract void ReadLock(IDatabase db, params int[] lockIds); + public abstract void WriteLock(IDatabase db, params int[] lockIds); + public virtual bool DoesTableExist(IDatabase db, string tableName) { return false; diff --git a/src/Umbraco.Core/Persistence/UmbracoDatabase.cs b/src/Umbraco.Core/Persistence/UmbracoDatabase.cs index 51e0172f35..072813b4e6 100644 --- a/src/Umbraco.Core/Persistence/UmbracoDatabase.cs +++ b/src/Umbraco.Core/Persistence/UmbracoDatabase.cs @@ -20,9 +20,6 @@ namespace Umbraco.Core.Persistence /// public class UmbracoDatabase : Database, IUmbracoDatabase { - // Umbraco's default isolation level is RepeatableRead - private const IsolationLevel DefaultIsolationLevel = IsolationLevel.RepeatableRead; - private readonly ILogger _logger; private readonly RetryPolicy _connectionRetryPolicy; private readonly RetryPolicy _commandRetryPolicy; @@ -38,7 +35,7 @@ namespace Umbraco.Core.Persistence /// Also used by DatabaseBuilder for creating databases and installing/upgrading. /// public UmbracoDatabase(string connectionString, ISqlContext sqlContext, DbProviderFactory provider, ILogger logger, RetryPolicy connectionRetryPolicy = null, RetryPolicy commandRetryPolicy = null) - : base(connectionString, sqlContext.DatabaseType, provider, DefaultIsolationLevel) + : base(connectionString, sqlContext.DatabaseType, provider, sqlContext.SqlSyntax.DefaultIsolationLevel) { SqlContext = sqlContext; @@ -54,7 +51,7 @@ namespace Umbraco.Core.Persistence /// /// Internal for unit tests only. internal UmbracoDatabase(DbConnection connection, ISqlContext sqlContext, ILogger logger) - : base(connection, sqlContext.DatabaseType, DefaultIsolationLevel) + : base(connection, sqlContext.DatabaseType, sqlContext.SqlSyntax.DefaultIsolationLevel) { SqlContext = sqlContext; _logger = logger; diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index e9dd04c5fa..84273e23da 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -33,8 +33,6 @@ namespace Umbraco.Core.Scoping private ICompletable _fscope; private IEventDispatcher _eventDispatcher; - private const IsolationLevel DefaultIsolationLevel = IsolationLevel.RepeatableRead; - // initializes a new scope private Scope(ScopeProvider scopeProvider, ILogger logger, FileSystems fileSystems, Scope parent, ScopeContext scopeContext, bool detachable, @@ -205,7 +203,7 @@ namespace Umbraco.Core.Scoping { if (_isolationLevel != IsolationLevel.Unspecified) return _isolationLevel; if (ParentScope != null) return ParentScope.IsolationLevel; - return DefaultIsolationLevel; + return Database.SqlContext.SqlSyntax.DefaultIsolationLevel; } } @@ -488,37 +486,9 @@ namespace Umbraco.Core.Scoping ?? (_logUncompletedScopes = Current.Configs.CoreDebug().LogUncompletedScopes)).Value; /// - public void ReadLock(params int[] lockIds) - { - // soon as we get Database, a transaction is started - - if (Database.Transaction.IsolationLevel < IsolationLevel.RepeatableRead) - throw new InvalidOperationException("A transaction with minimum RepeatableRead isolation level is required."); - - // *not* using a unique 'WHERE IN' query here because the *order* of lockIds is important to avoid deadlocks - foreach (var lockId in lockIds) - { - var i = Database.ExecuteScalar("SELECT value FROM umbracoLock WHERE id=@id", new { id = lockId }); - if (i == null) // ensure we are actually locking! - throw new Exception($"LockObject with id={lockId} does not exist."); - } - } + public void ReadLock(params int[] lockIds) => Database.SqlContext.SqlSyntax.ReadLock(Database, lockIds); /// - public void WriteLock(params int[] lockIds) - { - // soon as we get Database, a transaction is started - - if (Database.Transaction.IsolationLevel < IsolationLevel.RepeatableRead) - throw new InvalidOperationException("A transaction with minimum RepeatableRead isolation level is required."); - - // *not* using a unique 'WHERE IN' query here because the *order* of lockIds is important to avoid deadlocks - foreach (var lockId in lockIds) - { - var i = Database.Execute("UPDATE umbracoLock SET value = (CASE WHEN (value=1) THEN -1 ELSE 1 END) WHERE id=@id", new { id = lockId }); - if (i == 0) // ensure we are actually locking! - throw new Exception($"LockObject with id={lockId} does not exist."); - } - } + public void WriteLock(params int[] lockIds) => Database.SqlContext.SqlSyntax.WriteLock(Database, lockIds); } } diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index a0d0fad6d9..62cd245265 100755 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -982,7 +982,6 @@ - diff --git a/src/Umbraco.Tests/Models/UserExtensionsTests.cs b/src/Umbraco.Tests/Models/UserExtensionsTests.cs index d9c8057377..bf76031b59 100644 --- a/src/Umbraco.Tests/Models/UserExtensionsTests.cs +++ b/src/Umbraco.Tests/Models/UserExtensionsTests.cs @@ -57,6 +57,9 @@ namespace Umbraco.Tests.Models [TestCase("1,-1", "1", "1")] // was an issue [TestCase("-1,1", "1", "1")] // was an issue + [TestCase("-1", "", "-1")] + [TestCase("", "-1", "-1")] + public void CombineStartNodes(string groupSn, string userSn, string expected) { // 1 diff --git a/src/Umbraco.Tests/Persistence/LocksTests.cs b/src/Umbraco.Tests/Persistence/LocksTests.cs index 56779ace0f..51298035a7 100644 --- a/src/Umbraco.Tests/Persistence/LocksTests.cs +++ b/src/Umbraco.Tests/Persistence/LocksTests.cs @@ -5,7 +5,6 @@ using System.Threading; using NPoco; using NUnit.Framework; using Umbraco.Core; -using Umbraco.Core.Persistence; using Umbraco.Core.Persistence.Dtos; using Umbraco.Tests.TestHelpers; using Umbraco.Tests.Testing; @@ -37,7 +36,7 @@ namespace Umbraco.Tests.Persistence { using (var scope = ScopeProvider.CreateScope()) { - scope.Database.AcquireLockNodeReadLock(Constants.Locks.Servers); + scope.WriteLock(Constants.Locks.Servers); scope.Complete(); } } @@ -62,7 +61,7 @@ namespace Umbraco.Tests.Persistence { try { - scope.Database.AcquireLockNodeReadLock(Constants.Locks.Servers); + scope.ReadLock(Constants.Locks.Servers); lock (locker) { acquired++; @@ -131,7 +130,7 @@ namespace Umbraco.Tests.Persistence if (entered == threadCount) m1.Set(); } ms[ic].WaitOne(); - scope.Database.AcquireLockNodeWriteLock(Constants.Locks.Servers); + scope.WriteLock(Constants.Locks.Servers); lock (locker) { acquired++; @@ -221,7 +220,7 @@ namespace Umbraco.Tests.Persistence { otherEv.WaitOne(); Console.WriteLine($"[{id1}] WAIT {id1}"); - scope.Database.AcquireLockNodeWriteLock(id1); + scope.WriteLock(id1); Console.WriteLine($"[{id1}] GRANT {id1}"); WriteLocks(scope.Database); myEv.Set(); @@ -232,7 +231,7 @@ namespace Umbraco.Tests.Persistence Thread.Sleep(200); // cannot wait due to deadlock... just give it a bit of time Console.WriteLine($"[{id1}] WAIT {id2}"); - scope.Database.AcquireLockNodeWriteLock(id2); + scope.WriteLock(id2); Console.WriteLine($"[{id1}] GRANT {id2}"); WriteLocks(scope.Database); } @@ -284,7 +283,7 @@ namespace Umbraco.Tests.Persistence { otherEv.WaitOne(); Console.WriteLine($"[{id}] WAIT {id}"); - scope.Database.AcquireLockNodeWriteLock(id); + scope.WriteLock(id); Console.WriteLine($"[{id}] GRANT {id}"); WriteLocks(scope.Database); myEv.Set(); From dcc46fa32d58d66d9d9015ce0a81c0ee8abf073e Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 15 Oct 2019 11:14:28 +1100 Subject: [PATCH 2/5] no generic exception --- src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs b/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs index 92ee934e52..4e541c80d1 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs +++ b/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs @@ -169,7 +169,7 @@ where table_name=@0 and column_name=@1", tableName, columnName).FirstOrDefault() { var i = db.Execute(@"UPDATE umbracoLock SET value = value*-1 WHERE id=@id", new { id = lockId }); if (i == 0) // ensure we are actually locking! - throw new Exception($"LockObject with id={lockId} does not exist."); + throw new InvalidOperationException($"LockObject with id={lockId} does not exist."); } } @@ -185,7 +185,7 @@ where table_name=@0 and column_name=@1", tableName, columnName).FirstOrDefault() { var i = db.ExecuteScalar("SELECT value FROM umbracoLock WHERE id=@id", new { id = lockId }); if (i == null) // ensure we are actually locking! - throw new Exception($"LockObject with id={lockId} does not exist."); + throw new InvalidOperationException($"LockObject with id={lockId} does not exist."); } } From 37024664f5d39452effd10c635aed256a76551b7 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 15 Oct 2019 11:15:17 +1100 Subject: [PATCH 3/5] streamlines exception types --- src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs | 4 ++-- .../Persistence/SqlSyntax/SqlServerSyntaxProvider.cs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs b/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs index 4e541c80d1..141a1edadd 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs +++ b/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs @@ -169,7 +169,7 @@ where table_name=@0 and column_name=@1", tableName, columnName).FirstOrDefault() { var i = db.Execute(@"UPDATE umbracoLock SET value = value*-1 WHERE id=@id", new { id = lockId }); if (i == 0) // ensure we are actually locking! - throw new InvalidOperationException($"LockObject with id={lockId} does not exist."); + throw new ArgumentException($"LockObject with id={lockId} does not exist."); } } @@ -185,7 +185,7 @@ where table_name=@0 and column_name=@1", tableName, columnName).FirstOrDefault() { var i = db.ExecuteScalar("SELECT value FROM umbracoLock WHERE id=@id", new { id = lockId }); if (i == null) // ensure we are actually locking! - throw new InvalidOperationException($"LockObject with id={lockId} does not exist."); + throw new ArgumentException($"LockObject with id={lockId} does not exist."); } } diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs b/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs index 7bf00522e3..f90b61968f 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs +++ b/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs @@ -263,7 +263,7 @@ where tbl.[name]=@0 and col.[name]=@1;", tableName, columnName) db.Execute(@"SET LOCK_TIMEOUT 1800;"); var i = db.Execute(@"UPDATE umbracoLock WITH (REPEATABLEREAD) SET value = value*-1 WHERE id=@id", new { id = lockId }); if (i == 0) // ensure we are actually locking! - throw new InvalidOperationException($"LockObject with id={lockId} does not exist."); + throw new ArgumentException($"LockObject with id={lockId} does not exist."); } } From 538a8cf41b7ecabcbb0ebeafeafc958afc507879 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Mon, 21 Oct 2019 07:23:11 +0200 Subject: [PATCH 4/5] Change to case when, just to support if someone change values to something invalid directly in the database, and fixed tes --- src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs | 4 ++-- .../Persistence/SqlSyntax/SqlServerSyntaxProvider.cs | 2 +- src/Umbraco.Tests/Persistence/LocksTests.cs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs b/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs index 141a1edadd..2ed0fb878c 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs +++ b/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs @@ -167,12 +167,12 @@ where table_name=@0 and column_name=@1", tableName, columnName).FirstOrDefault() // *not* using a unique 'WHERE IN' query here because the *order* of lockIds is important to avoid deadlocks foreach (var lockId in lockIds) { - var i = db.Execute(@"UPDATE umbracoLock SET value = value*-1 WHERE id=@id", new { id = lockId }); + var i = db.Execute(@"UPDATE umbracoLock SET value = (CASE WHEN (value=1) THEN -1 ELSE 1 END) WHERE id=@id", new { id = lockId }); if (i == 0) // ensure we are actually locking! throw new ArgumentException($"LockObject with id={lockId} does not exist."); } } - + public override void ReadLock(IDatabase db, params int[] lockIds) { // soon as we get Database, a transaction is started diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs b/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs index f90b61968f..3d0adf175e 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs +++ b/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs @@ -261,7 +261,7 @@ where tbl.[name]=@0 and col.[name]=@1;", tableName, columnName) foreach (var lockId in lockIds) { db.Execute(@"SET LOCK_TIMEOUT 1800;"); - var i = db.Execute(@"UPDATE umbracoLock WITH (REPEATABLEREAD) SET value = value*-1 WHERE id=@id", new { id = lockId }); + var i = db.Execute(@"UPDATE umbracoLock WITH (REPEATABLEREAD) SET value = (CASE WHEN (value=1) THEN -1 ELSE 1 END) WHERE id=@id", new { id = lockId }); if (i == 0) // ensure we are actually locking! throw new ArgumentException($"LockObject with id={lockId} does not exist."); } diff --git a/src/Umbraco.Tests/Persistence/LocksTests.cs b/src/Umbraco.Tests/Persistence/LocksTests.cs index 51298035a7..afcd481f9f 100644 --- a/src/Umbraco.Tests/Persistence/LocksTests.cs +++ b/src/Umbraco.Tests/Persistence/LocksTests.cs @@ -36,7 +36,7 @@ namespace Umbraco.Tests.Persistence { using (var scope = ScopeProvider.CreateScope()) { - scope.WriteLock(Constants.Locks.Servers); + scope.ReadLock(Constants.Locks.Servers); scope.Complete(); } } From debe4b381c097c64e9b638c6215e2d8c261ca263 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Mon, 21 Oct 2019 07:26:39 +0200 Subject: [PATCH 5/5] Fixed tests --- src/Umbraco.Tests/Persistence/UnitOfWorkTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Tests/Persistence/UnitOfWorkTests.cs b/src/Umbraco.Tests/Persistence/UnitOfWorkTests.cs index 5651dd1457..02ad6b3971 100644 --- a/src/Umbraco.Tests/Persistence/UnitOfWorkTests.cs +++ b/src/Umbraco.Tests/Persistence/UnitOfWorkTests.cs @@ -14,7 +14,7 @@ namespace Umbraco.Tests.Persistence public void ReadLockNonExisting() { var provider = TestObjects.GetScopeProvider(Logger); - Assert.Throws(() => + Assert.Throws(() => { using (var scope = provider.CreateScope()) { @@ -39,7 +39,7 @@ namespace Umbraco.Tests.Persistence public void WriteLockNonExisting() { var provider = TestObjects.GetScopeProvider(Logger); - Assert.Throws(() => + Assert.Throws(() => { using (var scope = provider.CreateScope()) {