From 63e95cfdb995cde0a6e25680991f2f44f8b6b33c Mon Sep 17 00:00:00 2001 From: Doan Van Tuan Date: Thu, 1 Feb 2018 14:33:05 +0700 Subject: [PATCH 01/12] GH1565 Add alias in "for update" clause to entity loading query containing outer join if database (e.g Postgres) does not support locking with outer join --- .../NHSpecificTest/GH1565/Mappings.hbm.xml | 15 ++++ .../GH1565/PostgresOuterJoinLockTest.cs | 68 +++++++++++++++++++ src/NHibernate/Dialect/PostgreSQLDialect.cs | 1 + .../Loader/AbstractEntityJoinWalker.cs | 2 +- src/NHibernate/SqlCommand/SqlSelectBuilder.cs | 24 ++++++- 5 files changed, 107 insertions(+), 3 deletions(-) create mode 100644 src/NHibernate.Test/NHSpecificTest/GH1565/Mappings.hbm.xml create mode 100644 src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs diff --git a/src/NHibernate.Test/NHSpecificTest/GH1565/Mappings.hbm.xml b/src/NHibernate.Test/NHSpecificTest/GH1565/Mappings.hbm.xml new file mode 100644 index 00000000000..45efe8beb14 --- /dev/null +++ b/src/NHibernate.Test/NHSpecificTest/GH1565/Mappings.hbm.xml @@ -0,0 +1,15 @@ + + + + + + + + + + + + + + diff --git a/src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs b/src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs new file mode 100644 index 00000000000..5a7af7b840a --- /dev/null +++ b/src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs @@ -0,0 +1,68 @@ +using NHibernate.Cfg; +using NHibernate.Dialect; +using NUnit.Framework; + +namespace NHibernate.Test.NHSpecificTest.GH1565 +{ + [TestFixture] + public class PostgresOuterJoinLockTest : BugTestCase + { + [Test] + public void LockWithOuterJoin_ShouldBePossible() + { + using (var session = OpenSession()) + { + using (var transaction = session.BeginTransaction()) + { + var entity = session.Get(id, LockMode.Upgrade); + Assert.That(entity.Id, Is.EqualTo(id)); + transaction.Rollback(); + } + } + } + + private int id; + protected override void OnSetUp() + { + base.OnSetUp(); + using (var session = OpenSession()) + { + using (var transaction = session.BeginTransaction()) + { + session.FlushMode = FlushMode.Auto; + var entity = new MainEntity(); + session.Save(entity); + transaction.Commit(); + id = entity.Id; + } + } + } + + protected override void OnTearDown() + { + base.OnTearDown(); + using (var session = OpenSession()) + { + session.CreateSQLQuery("delete from MainEntity").ExecuteUpdate(); + } + } + + protected override bool AppliesTo(Dialect.Dialect dialect) + { + return dialect is PostgreSQLDialect; + } + + protected override void Configure(Configuration configuration) + { + configuration.SetProperty(Cfg.Environment.Dialect, typeof(PostgreSQL83Dialect).FullName); + configuration.SetProperty(Cfg.Environment.ConnectionDriver, typeof(Driver.NpgsqlDriver).FullName); + } + } + + public class MainEntity + { + public virtual int Id { get; set; } = 0; + + public virtual string Data { get; set; } + } +} diff --git a/src/NHibernate/Dialect/PostgreSQLDialect.cs b/src/NHibernate/Dialect/PostgreSQLDialect.cs index 33510e20378..023fcc304fa 100644 --- a/src/NHibernate/Dialect/PostgreSQLDialect.cs +++ b/src/NHibernate/Dialect/PostgreSQLDialect.cs @@ -316,6 +316,7 @@ public override string CurrentTimestampSelectString public override bool SupportsUnboundedLobLocatorMaterialization => false; + public override bool SupportsOuterJoinForUpdate => false; #endregion [Serializable] diff --git a/src/NHibernate/Loader/AbstractEntityJoinWalker.cs b/src/NHibernate/Loader/AbstractEntityJoinWalker.cs index a789cbaeb90..94b90bdd588 100644 --- a/src/NHibernate/Loader/AbstractEntityJoinWalker.cs +++ b/src/NHibernate/Loader/AbstractEntityJoinWalker.cs @@ -118,7 +118,7 @@ private void InitStatementString(SqlString projection, SqlString condition, SqlS JoinFragment ojf = MergeOuterJoins(associations); SqlSelectBuilder select = new SqlSelectBuilder(Factory) - .SetLockMode(lockMode) + .SetLockMode(lockMode, alias) .SetSelectClause(selectClause) .SetFromClause(Dialect.AppendLockHint(lockMode, persister.FromTableFragment(alias)) +persister.FromJoinFragment(alias, true, true)) .SetWhereClause(condition) diff --git a/src/NHibernate/SqlCommand/SqlSelectBuilder.cs b/src/NHibernate/SqlCommand/SqlSelectBuilder.cs index 418c0295570..61f427427e9 100644 --- a/src/NHibernate/SqlCommand/SqlSelectBuilder.cs +++ b/src/NHibernate/SqlCommand/SqlSelectBuilder.cs @@ -21,7 +21,9 @@ public class SqlSelectBuilder : SqlBaseBuilder, ISqlStringBuilder private SqlString groupByClause; private SqlString havingClause; private LockMode lockMode; + private string aliasToLock; private string comment; + private bool hasOuterJoin = false; public SqlSelectBuilder(ISessionFactoryImplementor factory) : base(factory.Dialect, factory) {} @@ -40,6 +42,7 @@ public SqlSelectBuilder SetComment(string comment) public SqlSelectBuilder SetFromClause(string fromClause) { this.fromClause = fromClause; + hasOuterJoin = hasOuterJoin || fromClause.ToLowerInvariant().Contains("left outer join"); return this; } @@ -110,6 +113,9 @@ public SqlSelectBuilder SetOuterJoins(SqlString outerJoinsAfterFrom, SqlString o } this.outerJoinsAfterWhere = tmpOuterJoinsAfterWhere; + + hasOuterJoin = hasOuterJoin || + outerJoinsAfterFrom != null && outerJoinsAfterFrom.ToString().ToLowerInvariant().Contains("outer join"); return this; } @@ -181,9 +187,10 @@ public SqlSelectBuilder SetHavingClause(SqlString havingSqlString) return this; } - public SqlSelectBuilder SetLockMode(LockMode lockMode) + public SqlSelectBuilder SetLockMode(LockMode lockMode, string alias) { this.lockMode = lockMode; + aliasToLock = alias; return this; } @@ -267,7 +274,7 @@ public SqlString ToSqlString() if (lockMode != null) { - sqlBuilder.Add(Dialect.GetForUpdateString(lockMode)); + sqlBuilder.Add(GetForUpdateString()); } if (log.IsDebugEnabled()) @@ -291,6 +298,19 @@ public SqlString ToSqlString() return sqlBuilder.ToSqlString(); } + private string GetForUpdateString() + { + if (!Dialect.SupportsOuterJoinForUpdate && hasOuterJoin) + { + if (Equals(lockMode, LockMode.Upgrade)) + return Dialect.GetForUpdateString(aliasToLock); + if (Equals(lockMode, LockMode.UpgradeNoWait)) + return Dialect.GetForUpdateNowaitString(aliasToLock); + } + + return Dialect.GetForUpdateString(lockMode); + } + #endregion } } From f5b9b193810abc0acce6d8e2a33e1ccf0c396868 Mon Sep 17 00:00:00 2001 From: Doan Van Tuan Date: Thu, 1 Feb 2018 22:17:10 +0700 Subject: [PATCH 02/12] Attemp to fix TC build by removing Configure() method from test --- .../NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs b/src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs index 5a7af7b840a..371a4d3ec01 100644 --- a/src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs +++ b/src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs @@ -51,12 +51,6 @@ protected override bool AppliesTo(Dialect.Dialect dialect) { return dialect is PostgreSQLDialect; } - - protected override void Configure(Configuration configuration) - { - configuration.SetProperty(Cfg.Environment.Dialect, typeof(PostgreSQL83Dialect).FullName); - configuration.SetProperty(Cfg.Environment.ConnectionDriver, typeof(Driver.NpgsqlDriver).FullName); - } } public class MainEntity From abb3c82dbbf8029beccdea4604e3d10d08b1678f Mon Sep 17 00:00:00 2001 From: Doan Van Tuan Date: Thu, 1 Feb 2018 22:20:35 +0700 Subject: [PATCH 03/12] Reformat file, replace spaces with tabs --- .../GH1565/PostgresOuterJoinLockTest.cs | 96 +++++++++---------- 1 file changed, 48 insertions(+), 48 deletions(-) diff --git a/src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs b/src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs index 371a4d3ec01..1097cdb324f 100644 --- a/src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs +++ b/src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs @@ -4,59 +4,59 @@ namespace NHibernate.Test.NHSpecificTest.GH1565 { - [TestFixture] - public class PostgresOuterJoinLockTest : BugTestCase - { - [Test] - public void LockWithOuterJoin_ShouldBePossible() - { - using (var session = OpenSession()) - { - using (var transaction = session.BeginTransaction()) - { - var entity = session.Get(id, LockMode.Upgrade); - Assert.That(entity.Id, Is.EqualTo(id)); - transaction.Rollback(); - } - } - } + [TestFixture] + public class PostgresOuterJoinLockTest : BugTestCase + { + [Test] + public void LockWithOuterJoin_ShouldBePossible() + { + using (var session = OpenSession()) + { + using (var transaction = session.BeginTransaction()) + { + var entity = session.Get(id, LockMode.Upgrade); + Assert.That(entity.Id, Is.EqualTo(id)); + transaction.Rollback(); + } + } + } private int id; - protected override void OnSetUp() - { - base.OnSetUp(); - using (var session = OpenSession()) - { - using (var transaction = session.BeginTransaction()) - { - session.FlushMode = FlushMode.Auto; - var entity = new MainEntity(); - session.Save(entity); - transaction.Commit(); - id = entity.Id; - } - } - } + protected override void OnSetUp() + { + base.OnSetUp(); + using (var session = OpenSession()) + { + using (var transaction = session.BeginTransaction()) + { + session.FlushMode = FlushMode.Auto; + var entity = new MainEntity(); + session.Save(entity); + transaction.Commit(); + id = entity.Id; + } + } + } - protected override void OnTearDown() - { - base.OnTearDown(); - using (var session = OpenSession()) - { - session.CreateSQLQuery("delete from MainEntity").ExecuteUpdate(); - } - } + protected override void OnTearDown() + { + base.OnTearDown(); + using (var session = OpenSession()) + { + session.CreateSQLQuery("delete from MainEntity").ExecuteUpdate(); + } + } - protected override bool AppliesTo(Dialect.Dialect dialect) - { - return dialect is PostgreSQLDialect; - } - } + protected override bool AppliesTo(Dialect.Dialect dialect) + { + return dialect is PostgreSQLDialect; + } + } - public class MainEntity - { - public virtual int Id { get; set; } = 0; + public class MainEntity + { + public virtual int Id { get; set; } = 0; public virtual string Data { get; set; } - } + } } From 7effa3513bb91202c33508e29f237beef661bbb2 Mon Sep 17 00:00:00 2001 From: Doan Van Tuan Date: Sat, 3 Feb 2018 10:36:46 +0700 Subject: [PATCH 04/12] GH1565 Refine code as per reviewer comments: - Restore and mark SetLockMode(LockMode) obsolete - Only check for outer join if the dialect is not support locking entire outer join - Rename test class to reflect that it not limited to PostgreSQL --- ...Test.cs => LockEntityWithOuterJoinTest.cs} | 13 ++--- src/NHibernate/SqlCommand/SqlSelectBuilder.cs | 47 +++++++++++-------- 2 files changed, 31 insertions(+), 29 deletions(-) rename src/NHibernate.Test/NHSpecificTest/GH1565/{PostgresOuterJoinLockTest.cs => LockEntityWithOuterJoinTest.cs} (79%) diff --git a/src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs b/src/NHibernate.Test/NHSpecificTest/GH1565/LockEntityWithOuterJoinTest.cs similarity index 79% rename from src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs rename to src/NHibernate.Test/NHSpecificTest/GH1565/LockEntityWithOuterJoinTest.cs index 1097cdb324f..2815ff61c20 100644 --- a/src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs +++ b/src/NHibernate.Test/NHSpecificTest/GH1565/LockEntityWithOuterJoinTest.cs @@ -1,11 +1,9 @@ -using NHibernate.Cfg; -using NHibernate.Dialect; -using NUnit.Framework; +using NUnit.Framework; namespace NHibernate.Test.NHSpecificTest.GH1565 { [TestFixture] - public class PostgresOuterJoinLockTest : BugTestCase + public class LockEntityWithOuterJoinTest : BugTestCase { [Test] public void LockWithOuterJoin_ShouldBePossible() @@ -16,7 +14,7 @@ public void LockWithOuterJoin_ShouldBePossible() { var entity = session.Get(id, LockMode.Upgrade); Assert.That(entity.Id, Is.EqualTo(id)); - transaction.Rollback(); + transaction.Commit(); } } } @@ -46,11 +44,6 @@ protected override void OnTearDown() session.CreateSQLQuery("delete from MainEntity").ExecuteUpdate(); } } - - protected override bool AppliesTo(Dialect.Dialect dialect) - { - return dialect is PostgreSQLDialect; - } } public class MainEntity diff --git a/src/NHibernate/SqlCommand/SqlSelectBuilder.cs b/src/NHibernate/SqlCommand/SqlSelectBuilder.cs index 61f427427e9..4abcfd9df1f 100644 --- a/src/NHibernate/SqlCommand/SqlSelectBuilder.cs +++ b/src/NHibernate/SqlCommand/SqlSelectBuilder.cs @@ -1,4 +1,5 @@ +using System; using NHibernate.Engine; using NHibernate.Type; using NHibernate.Util; @@ -21,10 +22,9 @@ public class SqlSelectBuilder : SqlBaseBuilder, ISqlStringBuilder private SqlString groupByClause; private SqlString havingClause; private LockMode lockMode; - private string aliasToLock; + private string mainTableAlias; private string comment; - private bool hasOuterJoin = false; - + public SqlSelectBuilder(ISessionFactoryImplementor factory) : base(factory.Dialect, factory) {} @@ -42,7 +42,6 @@ public SqlSelectBuilder SetComment(string comment) public SqlSelectBuilder SetFromClause(string fromClause) { this.fromClause = fromClause; - hasOuterJoin = hasOuterJoin || fromClause.ToLowerInvariant().Contains("left outer join"); return this; } @@ -65,8 +64,7 @@ public SqlSelectBuilder SetFromClause(string tableName, string alias) /// The SqlSelectBuilder public SqlSelectBuilder SetFromClause(SqlString fromClause) { - // it is safe to do this because a fromClause will have no - // parameters + // it is safe to do this because a fromClause will have no parameters return SetFromClause(fromClause.ToString()); } @@ -113,9 +111,6 @@ public SqlSelectBuilder SetOuterJoins(SqlString outerJoinsAfterFrom, SqlString o } this.outerJoinsAfterWhere = tmpOuterJoinsAfterWhere; - - hasOuterJoin = hasOuterJoin || - outerJoinsAfterFrom != null && outerJoinsAfterFrom.ToString().ToLowerInvariant().Contains("outer join"); return this; } @@ -187,10 +182,18 @@ public SqlSelectBuilder SetHavingClause(SqlString havingSqlString) return this; } - public SqlSelectBuilder SetLockMode(LockMode lockMode, string alias) + [Obsolete("For some DBMS's such as PostgreSQL, a lock on query with OUTER JOIN is not possible without specifying the not-null side. " + + "Use the new method SetLockMode(LockMode, mainTableAlias) instead.")] + public SqlSelectBuilder SetLockMode(LockMode lockMode) + { + this.lockMode = lockMode; + return this; + } + + public SqlSelectBuilder SetLockMode(LockMode lockMode, string mainTableAlias) { this.lockMode = lockMode; - aliasToLock = alias; + this.mainTableAlias = mainTableAlias; return this; } @@ -300,17 +303,23 @@ public SqlString ToSqlString() private string GetForUpdateString() { - if (!Dialect.SupportsOuterJoinForUpdate && hasOuterJoin) - { - if (Equals(lockMode, LockMode.Upgrade)) - return Dialect.GetForUpdateString(aliasToLock); - if (Equals(lockMode, LockMode.UpgradeNoWait)) - return Dialect.GetForUpdateNowaitString(aliasToLock); - } + if (Dialect.SupportsOuterJoinForUpdate || !HasOuterJoin()) + return Dialect.GetForUpdateString(lockMode); + + if (Equals(lockMode, LockMode.Upgrade)) + return Dialect.GetForUpdateString(mainTableAlias); + if (Equals(lockMode, LockMode.UpgradeNoWait)) + return Dialect.GetForUpdateNowaitString(mainTableAlias); return Dialect.GetForUpdateString(lockMode); - } + bool HasOuterJoin() + { + return + StringHelper.ContainsCaseInsensitive(fromClause, "outer join") || + !string.IsNullOrWhiteSpace(outerJoinsAfterFrom?.ToString()); + } + } #endregion } } From 03814563cb542e21dbc39fd226c23d314cfb46ed Mon Sep 17 00:00:00 2001 From: Alexander Zaytsev Date: Tue, 13 Mar 2018 12:53:42 +1300 Subject: [PATCH 05/12] Adjust code as per review --- src/NHibernate/SqlCommand/SqlSelectBuilder.cs | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/NHibernate/SqlCommand/SqlSelectBuilder.cs b/src/NHibernate/SqlCommand/SqlSelectBuilder.cs index 4abcfd9df1f..0db9c90db85 100644 --- a/src/NHibernate/SqlCommand/SqlSelectBuilder.cs +++ b/src/NHibernate/SqlCommand/SqlSelectBuilder.cs @@ -303,22 +303,20 @@ public SqlString ToSqlString() private string GetForUpdateString() { - if (Dialect.SupportsOuterJoinForUpdate || !HasOuterJoin()) - return Dialect.GetForUpdateString(lockMode); - - if (Equals(lockMode, LockMode.Upgrade)) - return Dialect.GetForUpdateString(mainTableAlias); - if (Equals(lockMode, LockMode.UpgradeNoWait)) - return Dialect.GetForUpdateNowaitString(mainTableAlias); + if (!Dialect.SupportsOuterJoinForUpdate && HasOuterJoin()) + { + if (Equals(lockMode, LockMode.Upgrade)) + return Dialect.GetForUpdateString(mainTableAlias); + + if (Equals(lockMode, LockMode.UpgradeNoWait)) + return Dialect.GetForUpdateNowaitString(mainTableAlias); + } return Dialect.GetForUpdateString(lockMode); - bool HasOuterJoin() - { - return - StringHelper.ContainsCaseInsensitive(fromClause, "outer join") || - !string.IsNullOrWhiteSpace(outerJoinsAfterFrom?.ToString()); - } + bool HasOuterJoin() => + StringHelper.ContainsCaseInsensitive(fromClause, "outer join") || + outerJoinsAfterFrom?.IsEmptyOrWhitespace() == false; } #endregion } From 15437c63180746a37d62cb90c20558dbd4890c36 Mon Sep 17 00:00:00 2001 From: Alexander Zaytsev Date: Tue, 13 Mar 2018 13:10:24 +1300 Subject: [PATCH 06/12] Generate async code --- .../GH1565/LockEntityWithOuterJoinTest.cs | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 src/NHibernate.Test/Async/NHSpecificTest/GH1565/LockEntityWithOuterJoinTest.cs diff --git a/src/NHibernate.Test/Async/NHSpecificTest/GH1565/LockEntityWithOuterJoinTest.cs b/src/NHibernate.Test/Async/NHSpecificTest/GH1565/LockEntityWithOuterJoinTest.cs new file mode 100644 index 00000000000..56e189e798f --- /dev/null +++ b/src/NHibernate.Test/Async/NHSpecificTest/GH1565/LockEntityWithOuterJoinTest.cs @@ -0,0 +1,59 @@ +//------------------------------------------------------------------------------ +// +// This code was generated by AsyncGenerator. +// +// Changes to this file may cause incorrect behavior and will be lost if +// the code is regenerated. +// +//------------------------------------------------------------------------------ + + +using NUnit.Framework; + +namespace NHibernate.Test.NHSpecificTest.GH1565 +{ + using System.Threading.Tasks; + [TestFixture] + public class LockEntityWithOuterJoinTestAsync : BugTestCase + { + [Test] + public async Task LockWithOuterJoin_ShouldBePossibleAsync() + { + using (var session = OpenSession()) + { + using (var transaction = session.BeginTransaction()) + { + var entity = await (session.GetAsync(id, LockMode.Upgrade)); + Assert.That(entity.Id, Is.EqualTo(id)); + await (transaction.CommitAsync()); + } + } + } + + private int id; + protected override void OnSetUp() + { + base.OnSetUp(); + using (var session = OpenSession()) + { + using (var transaction = session.BeginTransaction()) + { + session.FlushMode = FlushMode.Auto; + var entity = new MainEntity(); + session.Save(entity); + transaction.Commit(); + id = entity.Id; + } + } + } + + protected override void OnTearDown() + { + base.OnTearDown(); + using (var session = OpenSession()) + { + session.CreateSQLQuery("delete from MainEntity").ExecuteUpdate(); + } + } + } +} From c65ab2b7c91a29fbb25b7899fa331106732ce2b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= Date: Tue, 13 Mar 2018 12:15:47 +0100 Subject: [PATCH 07/12] Remove undue whitespace addition and do condition swap recommended by bahusoid. --- src/NHibernate/SqlCommand/SqlSelectBuilder.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/NHibernate/SqlCommand/SqlSelectBuilder.cs b/src/NHibernate/SqlCommand/SqlSelectBuilder.cs index 0db9c90db85..e673c9d08d3 100644 --- a/src/NHibernate/SqlCommand/SqlSelectBuilder.cs +++ b/src/NHibernate/SqlCommand/SqlSelectBuilder.cs @@ -24,7 +24,7 @@ public class SqlSelectBuilder : SqlBaseBuilder, ISqlStringBuilder private LockMode lockMode; private string mainTableAlias; private string comment; - + public SqlSelectBuilder(ISessionFactoryImplementor factory) : base(factory.Dialect, factory) {} @@ -315,8 +315,8 @@ private string GetForUpdateString() return Dialect.GetForUpdateString(lockMode); bool HasOuterJoin() => - StringHelper.ContainsCaseInsensitive(fromClause, "outer join") || - outerJoinsAfterFrom?.IsEmptyOrWhitespace() == false; + outerJoinsAfterFrom?.IsEmptyOrWhitespace() == false || + StringHelper.ContainsCaseInsensitive(fromClause, "outer join"); } #endregion } From 60f60028462656282d2a4631b38122e3cefe7c45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= Date: Tue, 13 Mar 2018 12:57:05 +0100 Subject: [PATCH 08/12] Handle unimplemented/unsupported cases. --- src/NHibernate/Dialect/Dialect.cs | 12 ++++++-- src/NHibernate/Dialect/InformixDialect.cs | 3 +- src/NHibernate/Dialect/PostgreSQLDialect.cs | 7 ++++- src/NHibernate/SqlCommand/SqlSelectBuilder.cs | 30 +++++++++++++++---- 4 files changed, 42 insertions(+), 10 deletions(-) diff --git a/src/NHibernate/Dialect/Dialect.cs b/src/NHibernate/Dialect/Dialect.cs index d709ae605ba..9bd144fa943 100644 --- a/src/NHibernate/Dialect/Dialect.cs +++ b/src/NHibernate/Dialect/Dialect.cs @@ -526,8 +526,16 @@ public virtual string ForUpdateString get { return " for update"; } } - /// Is FOR UPDATE OF syntax supported? - /// True if the database supports FOR UPDATE OF syntax; false otherwise. + /// Is FOR UPDATE OF syntax supported? + /// if the database supports FOR UPDATE OF syntax; otherwise. + public virtual bool ForUpdateOf + // By default, just check ForUpdateOfColumns. ForUpdateOf needs to be overriden only for dialect suppoting + // "For Update Of" on table aliases. + => ForUpdateOfColumns; + + /// Is FOR UPDATE OF syntax expecting columns? + /// if the database expects a column list with FOR UPDATE OF syntax, + /// if it expects table alias instead or do not support FOR UPDATE OF syntax. public virtual bool ForUpdateOfColumns { // by default we report no support diff --git a/src/NHibernate/Dialect/InformixDialect.cs b/src/NHibernate/Dialect/InformixDialect.cs index 5ff406fd2d8..f53b7e59d72 100644 --- a/src/NHibernate/Dialect/InformixDialect.cs +++ b/src/NHibernate/Dialect/InformixDialect.cs @@ -175,8 +175,7 @@ public override string AddColumnString // throw new NotSupportedException(); //} - /// Is FOR UPDATE OF syntax supported? - /// True if the database supports FOR UPDATE OF syntax; false otherwise. + /// public override bool ForUpdateOfColumns { get { return true; } diff --git a/src/NHibernate/Dialect/PostgreSQLDialect.cs b/src/NHibernate/Dialect/PostgreSQLDialect.cs index ad2b113128d..339d5bc09c1 100644 --- a/src/NHibernate/Dialect/PostgreSQLDialect.cs +++ b/src/NHibernate/Dialect/PostgreSQLDialect.cs @@ -227,6 +227,12 @@ public override SqlString GetLimitString(SqlString queryString, SqlString offset return pagingBuilder.ToSqlString(); } + /// + public override bool ForUpdateOf => true; + + /// + public override bool SupportsOuterJoinForUpdate => false; + public override string GetForUpdateString(string aliases) { return ForUpdateString + " of " + aliases; @@ -318,7 +324,6 @@ public override string CurrentTimestampSelectString public override bool SupportsUnboundedLobLocatorMaterialization => false; - public override bool SupportsOuterJoinForUpdate => false; #endregion [Serializable] diff --git a/src/NHibernate/SqlCommand/SqlSelectBuilder.cs b/src/NHibernate/SqlCommand/SqlSelectBuilder.cs index e673c9d08d3..685796140c3 100644 --- a/src/NHibernate/SqlCommand/SqlSelectBuilder.cs +++ b/src/NHibernate/SqlCommand/SqlSelectBuilder.cs @@ -305,11 +305,31 @@ private string GetForUpdateString() { if (!Dialect.SupportsOuterJoinForUpdate && HasOuterJoin()) { - if (Equals(lockMode, LockMode.Upgrade)) - return Dialect.GetForUpdateString(mainTableAlias); - - if (Equals(lockMode, LockMode.UpgradeNoWait)) - return Dialect.GetForUpdateNowaitString(mainTableAlias); + var isUpgrade = Equals(lockMode, LockMode.Upgrade); + var isUpgradeNoWait = !isUpgrade && ( + Equals(lockMode, LockMode.UpgradeNoWait) || Equals(lockMode, LockMode.Force)); + if (!isUpgrade && !isUpgradeNoWait) + return string.Empty; + + if (!Dialect.ForUpdateOf) + { + log.Warn( + "Unsupported 'for update' case: 'for update' query with an outer join using a dialect not" + + "supporting it and not supporting 'for update of' clause. Discarding 'for" + + "update' clause."); + return string.Empty; + } + + if (Dialect.ForUpdateOfColumns) + { + log.Warn( + "Unimplemented 'for update' case: 'for update' query with an outer join using a dialect not" + + "supporting it and requiring columns for its 'for update of' syntax. Discarding 'for" + + "update' clause."); + return string.Empty; + } + + return isUpgrade ? Dialect.GetForUpdateString(mainTableAlias) : Dialect.GetForUpdateNowaitString(mainTableAlias); } return Dialect.GetForUpdateString(lockMode); From 133f2447b99faf2073edbf12a62b47fbad9df95b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= Date: Tue, 13 Mar 2018 12:58:44 +0100 Subject: [PATCH 09/12] Fix an undue reference equality check for lockmode. --- src/NHibernate/Dialect/Dialect.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/NHibernate/Dialect/Dialect.cs b/src/NHibernate/Dialect/Dialect.cs index 9bd144fa943..8ac3363a744 100644 --- a/src/NHibernate/Dialect/Dialect.cs +++ b/src/NHibernate/Dialect/Dialect.cs @@ -501,15 +501,15 @@ public virtual ILockingStrategy GetLockingStrategy(ILockable lockable, LockMode /// The appropriate for update fragment. public virtual string GetForUpdateString(LockMode lockMode) { - if (lockMode == LockMode.Upgrade) + if (Equals(lockMode, LockMode.Upgrade)) { return ForUpdateString; } - if (lockMode == LockMode.UpgradeNoWait) + if (Equals(lockMode, LockMode.UpgradeNoWait)) { return ForUpdateNowaitString; } - if (lockMode == LockMode.Force) + if (Equals(lockMode, LockMode.Force)) { return ForUpdateNowaitString; } From 267977dbc9f8bf59c701fc1cf2ec8cc8f80e6d05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= Date: Tue, 13 Mar 2018 13:06:40 +0100 Subject: [PATCH 10/12] Some more misleading xml comment fixes. --- src/NHibernate/Dialect/Dialect.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/NHibernate/Dialect/Dialect.cs b/src/NHibernate/Dialect/Dialect.cs index 8ac3363a744..3673b862c9c 100644 --- a/src/NHibernate/Dialect/Dialect.cs +++ b/src/NHibernate/Dialect/Dialect.cs @@ -575,11 +575,11 @@ public virtual string ForUpdateNowaitString } /// - /// Get the FOR UPDATE OF column_list NOWAIT fragment appropriate - /// for this dialect given the aliases of the columns to be write locked. + /// Get the FOR UPDATE OF column_list NOWAIT fragment appropriate + /// for this dialect given the aliases of the columns or tables to be write locked. /// - /// The columns to be write locked. - /// The appropriate FOR UPDATE colunm_list NOWAIT clause string. + /// The columns or tables to be write locked. + /// The appropriate FOR UPDATE colunm_or_table_list NOWAIT clause string. public virtual string GetForUpdateNowaitString(string aliases) { return GetForUpdateString(aliases); From d8f344917f726f7c67d2347edc0a652b28ad718a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= Date: Wed, 14 Mar 2018 01:32:58 +0100 Subject: [PATCH 11/12] Rename a new property (to be squashed, and all my other commits too). --- src/NHibernate/Dialect/Dialect.cs | 2 +- src/NHibernate/Dialect/PostgreSQLDialect.cs | 2 +- src/NHibernate/SqlCommand/SqlSelectBuilder.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/NHibernate/Dialect/Dialect.cs b/src/NHibernate/Dialect/Dialect.cs index 3673b862c9c..5c71a5e4767 100644 --- a/src/NHibernate/Dialect/Dialect.cs +++ b/src/NHibernate/Dialect/Dialect.cs @@ -528,7 +528,7 @@ public virtual string ForUpdateString /// Is FOR UPDATE OF syntax supported? /// if the database supports FOR UPDATE OF syntax; otherwise. - public virtual bool ForUpdateOf + public virtual bool SupportsForUpdateOf // By default, just check ForUpdateOfColumns. ForUpdateOf needs to be overriden only for dialect suppoting // "For Update Of" on table aliases. => ForUpdateOfColumns; diff --git a/src/NHibernate/Dialect/PostgreSQLDialect.cs b/src/NHibernate/Dialect/PostgreSQLDialect.cs index 339d5bc09c1..c1653a36a6f 100644 --- a/src/NHibernate/Dialect/PostgreSQLDialect.cs +++ b/src/NHibernate/Dialect/PostgreSQLDialect.cs @@ -228,7 +228,7 @@ public override SqlString GetLimitString(SqlString queryString, SqlString offset } /// - public override bool ForUpdateOf => true; + public override bool SupportsForUpdateOf => true; /// public override bool SupportsOuterJoinForUpdate => false; diff --git a/src/NHibernate/SqlCommand/SqlSelectBuilder.cs b/src/NHibernate/SqlCommand/SqlSelectBuilder.cs index 685796140c3..4588f6c7961 100644 --- a/src/NHibernate/SqlCommand/SqlSelectBuilder.cs +++ b/src/NHibernate/SqlCommand/SqlSelectBuilder.cs @@ -311,7 +311,7 @@ private string GetForUpdateString() if (!isUpgrade && !isUpgradeNoWait) return string.Empty; - if (!Dialect.ForUpdateOf) + if (!Dialect.SupportsForUpdateOf) { log.Warn( "Unsupported 'for update' case: 'for update' query with an outer join using a dialect not" + From d640fc9031186d77300ad626c271521ce10a6b89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= Date: Wed, 14 Mar 2018 01:49:20 +0100 Subject: [PATCH 12/12] Rename an existing property (to be squashed, and all my other commits too). Renaming an existing property without causing a breaking change causes some bloat. --- .../Async/Dialect/InformixDialect.cs | 3 ++- src/NHibernate/Async/Loader/Hql/QueryLoader.cs | 2 +- src/NHibernate/Dialect/Dialect.cs | 12 ++++++++++-- src/NHibernate/Dialect/InformixDialect.cs | 10 +++++++++- src/NHibernate/Dialect/Oracle8iDialect.cs | 7 +++++++ .../Dialect/SybaseSQLAnywhere10Dialect.cs | 18 ++++++++++++++++++ .../Loader/Criteria/CriteriaLoader.cs | 2 +- src/NHibernate/Loader/Hql/QueryLoader.cs | 4 ++-- src/NHibernate/SqlCommand/ForUpdateFragment.cs | 4 ++-- src/NHibernate/SqlCommand/SqlSelectBuilder.cs | 2 +- 10 files changed, 53 insertions(+), 11 deletions(-) diff --git a/src/NHibernate/Async/Dialect/InformixDialect.cs b/src/NHibernate/Async/Dialect/InformixDialect.cs index 88d46a2335a..ebcae053076 100644 --- a/src/NHibernate/Async/Dialect/InformixDialect.cs +++ b/src/NHibernate/Async/Dialect/InformixDialect.cs @@ -8,14 +8,15 @@ //------------------------------------------------------------------------------ +using System; using System.Data; using System.Data.Common; using System.Text; -using NHibernate.Cfg; using NHibernate.Dialect.Function; using NHibernate.Exceptions; using NHibernate.SqlCommand; using NHibernate.Util; +using Environment = NHibernate.Cfg.Environment; //using NHibernate.Dialect.Schema; diff --git a/src/NHibernate/Async/Loader/Hql/QueryLoader.cs b/src/NHibernate/Async/Loader/Hql/QueryLoader.cs index ed6e39a3c2e..b50cff730c6 100644 --- a/src/NHibernate/Async/Loader/Hql/QueryLoader.cs +++ b/src/NHibernate/Async/Loader/Hql/QueryLoader.cs @@ -121,4 +121,4 @@ internal async Task GetEnumerableAsync(QueryParameters queryParamet return result; } } -} \ No newline at end of file +} diff --git a/src/NHibernate/Dialect/Dialect.cs b/src/NHibernate/Dialect/Dialect.cs index 5c71a5e4767..16976405d49 100644 --- a/src/NHibernate/Dialect/Dialect.cs +++ b/src/NHibernate/Dialect/Dialect.cs @@ -529,19 +529,27 @@ public virtual string ForUpdateString /// Is FOR UPDATE OF syntax supported? /// if the database supports FOR UPDATE OF syntax; otherwise. public virtual bool SupportsForUpdateOf - // By default, just check ForUpdateOfColumns. ForUpdateOf needs to be overriden only for dialect suppoting + // By default, just check UsesColumnsWithForUpdateOf. ForUpdateOf needs to be overriden only for dialects supporting // "For Update Of" on table aliases. - => ForUpdateOfColumns; + => UsesColumnsWithForUpdateOf; /// Is FOR UPDATE OF syntax expecting columns? /// if the database expects a column list with FOR UPDATE OF syntax, /// if it expects table alias instead or do not support FOR UPDATE OF syntax. + // Since v5.1 + [Obsolete("Use UsesColumnsWithForUpdateOf instead")] public virtual bool ForUpdateOfColumns { // by default we report no support get { return false; } } + public virtual bool UsesColumnsWithForUpdateOf +#pragma warning disable 618 + // For avoiding a breaking change, we need to call the old name by default. + => ForUpdateOfColumns; +#pragma warning restore 618 + /// /// Does this dialect support FOR UPDATE in conjunction with outer joined rows? /// diff --git a/src/NHibernate/Dialect/InformixDialect.cs b/src/NHibernate/Dialect/InformixDialect.cs index f53b7e59d72..e430af4b29e 100644 --- a/src/NHibernate/Dialect/InformixDialect.cs +++ b/src/NHibernate/Dialect/InformixDialect.cs @@ -1,11 +1,12 @@ +using System; using System.Data; using System.Data.Common; using System.Text; -using NHibernate.Cfg; using NHibernate.Dialect.Function; using NHibernate.Exceptions; using NHibernate.SqlCommand; using NHibernate.Util; +using Environment = NHibernate.Cfg.Environment; //using NHibernate.Dialect.Schema; @@ -176,11 +177,18 @@ public override string AddColumnString //} /// + // Since v5.1 + [Obsolete("Use UsesColumnsWithForUpdateOf instead")] public override bool ForUpdateOfColumns { get { return true; } } + /* 6.0 TODO: uncomment once ForUpdateOfColumns is removed. + /// + public override bool UsesColumnsWithForUpdateOf => true; + */ + /// /// Does this dialect support FOR UPDATE in conjunction with outer joined rows? /// diff --git a/src/NHibernate/Dialect/Oracle8iDialect.cs b/src/NHibernate/Dialect/Oracle8iDialect.cs index b70f90910fb..d540f635d1b 100644 --- a/src/NHibernate/Dialect/Oracle8iDialect.cs +++ b/src/NHibernate/Dialect/Oracle8iDialect.cs @@ -485,11 +485,18 @@ public override bool UseMaxForLimit get { return true; } } + // Since v5.1 + [Obsolete("Use UsesColumnsWithForUpdateOf instead")] public override bool ForUpdateOfColumns { get { return true; } } + /* 6.0 TODO: uncomment once ForUpdateOfColumns is removed. + /// + public override bool UsesColumnsWithForUpdateOf => true; + */ + public override bool SupportsUnionAll { get { return true; } diff --git a/src/NHibernate/Dialect/SybaseSQLAnywhere10Dialect.cs b/src/NHibernate/Dialect/SybaseSQLAnywhere10Dialect.cs index 8d7ce1dc2ed..bd07ff22c2e 100644 --- a/src/NHibernate/Dialect/SybaseSQLAnywhere10Dialect.cs +++ b/src/NHibernate/Dialect/SybaseSQLAnywhere10Dialect.cs @@ -643,11 +643,29 @@ public override string GetForUpdateString(LockMode lockMode) /// In this dialect, we avoid this issue by supporting only /// FOR UPDATE BY LOCK. /// + // Since v5.1 + [Obsolete("Use UsesColumnsWithForUpdateOf instead")] public override bool ForUpdateOfColumns { get { return false; } } + /* 6.0 TODO: uncomment once ForUpdateOfColumns is removed. + /// + /// SQL Anywhere does support FOR UPDATE OF syntax. However, + /// in SQL Anywhere one cannot specify both FOR UPDATE OF syntax + /// and FOR UPDATE BY LOCK in the same statement. To achieve INTENT + /// locking when using FOR UPDATE OF syntax one must use a table hint + /// in the query's FROM clause, ie. + /// + /// SELECT * FROM FOO WITH( UPDLOCK ) FOR UPDATE OF ( column-list ). + /// + /// In this dialect, we avoid this issue by supporting only + /// FOR UPDATE BY LOCK. + /// + public override bool UsesColumnsWithForUpdateOf => false; + */ + /// /// SQL Anywhere supports FOR UPDATE over cursors containing /// outer joins. diff --git a/src/NHibernate/Loader/Criteria/CriteriaLoader.cs b/src/NHibernate/Loader/Criteria/CriteriaLoader.cs index 655ea38fdab..26a5cefd079 100644 --- a/src/NHibernate/Loader/Criteria/CriteriaLoader.cs +++ b/src/NHibernate/Loader/Criteria/CriteriaLoader.cs @@ -171,7 +171,7 @@ protected override SqlString ApplyLocks(SqlString sqlSelectString, IDictionary aliasedLockModes = new Dictionary(); - Dictionary keyColumnNames = dialect.ForUpdateOfColumns ? new Dictionary() : null; + Dictionary keyColumnNames = dialect.UsesColumnsWithForUpdateOf ? new Dictionary() : null; string[] drivingSqlAliases = Aliases; //NH-3710: if we are issuing an aggregation function, Aliases will be null diff --git a/src/NHibernate/Loader/Hql/QueryLoader.cs b/src/NHibernate/Loader/Hql/QueryLoader.cs index 23c769da343..16d6b198400 100644 --- a/src/NHibernate/Loader/Hql/QueryLoader.cs +++ b/src/NHibernate/Loader/Hql/QueryLoader.cs @@ -71,7 +71,7 @@ protected override SqlString ApplyLocks(SqlString sql, IDictionary lock mode // create a new map of sql-alias -> lock mode var aliasedLockModes = new Dictionary(); - Dictionary keyColumnNames = dialect.ForUpdateOfColumns ? new Dictionary() : null; + Dictionary keyColumnNames = dialect.UsesColumnsWithForUpdateOf ? new Dictionary() : null; foreach (var entry in lockModes) { @@ -456,4 +456,4 @@ protected override IEnumerable GetParameterSpecificatio return _queryTranslator.CollectedParameterSpecifications; } } -} \ No newline at end of file +} diff --git a/src/NHibernate/SqlCommand/ForUpdateFragment.cs b/src/NHibernate/SqlCommand/ForUpdateFragment.cs index 3a30299f115..4e5661abb4b 100644 --- a/src/NHibernate/SqlCommand/ForUpdateFragment.cs +++ b/src/NHibernate/SqlCommand/ForUpdateFragment.cs @@ -30,7 +30,7 @@ public ForUpdateFragment(Dialect.Dialect dialect, IDictionary if (LockMode.Read.LessThan(lockMode)) { string tableAlias = me.Key; - if (dialect.ForUpdateOfColumns) + if (dialect.UsesColumnsWithForUpdateOf) { string[] keyColumns = keyColumnNames[tableAlias]; if (keyColumns == null) @@ -89,4 +89,4 @@ public string ToSqlStringFragment() : dialect.GetForUpdateString(aliases.ToString()); } } -} \ No newline at end of file +} diff --git a/src/NHibernate/SqlCommand/SqlSelectBuilder.cs b/src/NHibernate/SqlCommand/SqlSelectBuilder.cs index 4588f6c7961..e287fb8b5c4 100644 --- a/src/NHibernate/SqlCommand/SqlSelectBuilder.cs +++ b/src/NHibernate/SqlCommand/SqlSelectBuilder.cs @@ -320,7 +320,7 @@ private string GetForUpdateString() return string.Empty; } - if (Dialect.ForUpdateOfColumns) + if (Dialect.UsesColumnsWithForUpdateOf) { log.Warn( "Unimplemented 'for update' case: 'for update' query with an outer join using a dialect not" +