Fix filter & where fragment appended after lock hint

src/NHibernate.Test/CollectionTest/Domain.cs

@@ -0,0 +1,18 @@
+using System.Collections.Generic;
+
+namespace NHibernate.Test.CollectionTest
+{
+	public class Env
+	{
+		public virtual long Id { get; set; }
+		public virtual IList<MachineRequest> RequestsFailed { get; set; }
+		public virtual IDictionary<long, MachineRequest> FailedRequestsById { get; set; }
+	}
+
+	public class MachineRequest
+	{
+		public virtual long Id { get; set; }
+		public virtual int RequestCompletionStatus { get; set; }
+		public virtual long EnvId { get; set; }
+	}
+}

src/NHibernate.Test/CollectionTest/Domain.hbm.xml

@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<hibernate-mapping xmlns="urn:nhibernate-mapping-2.2"
+                   namespace="NHibernate.Test.CollectionTest"
+                   assembly="NHibernate.Test">
+
+  <class name="Env">
+    <id name="Id">
+      <generator class="assigned"/>
+    </id>
+    <bag name="RequestsFailed" inverse="true" lazy="extra"
+         where="RequestCompletionStatus != 1">
+      <key column="EnvId"/>
+      <one-to-many class="MachineRequest"/>
+      <filter name="CurrentOnly"/>
+    </bag>
+    <map name="FailedRequestsById" inverse="true"
+         where="RequestCompletionStatus != 1">
+      <key column="EnvId"/>
+      <map-key type="Int64" column="Id"/>
+      <one-to-many class="MachineRequest"/>
+      <filter name="CurrentOnly"/>
+    </map>
+  </class>
+
+  <class name="MachineRequest">
+    <id name="Id">
+      <generator class="assigned"/>
+    </id>
+    <property name="EnvId"/>
+    <property name="RequestCompletionStatus"/>
+  </class>
+
+  <filter-def name="CurrentOnly" condition="1 = 0"/>
+</hibernate-mapping>

src/NHibernate.Test/CollectionTest/WhereWithReadLockFixture.cs

@@ -0,0 +1,129 @@
+using System.Reflection;
+using NHibernate.Cfg;
+using NHibernate.Dialect;
+using NHibernate.Engine;
+using NHibernate.Id;
+using NHibernate.Persister.Collection;
+using NHibernate.SqlCommand;
+using NUnit.Framework;
+
+namespace NHibernate.Test.CollectionTest
+{
+	public class DialectWithReadLockHint : GenericDialect
+	{
+		public const string ReadOnlyLock = " for read only";
+
+		public override string GetForUpdateString(LockMode lockMode)
+		{
+			if (lockMode == LockMode.Read)
+			{
+				return ReadOnlyLock;
+			}
+
+			return string.Empty;
+		}
+	}
+
+	// SQL Anywhere has this, but has no CI currently. So testing with an ad-hoc generic dialect.
+	// Trouble originally spotted with NHSpecificTest.BagWithLazyExtraAndFilter.Fixture.CanUseFilterForLazyExtra
+	// test, which was locally failing for SQL Anywhere.
+	[TestFixture]
+	public class WhereWithReadLockFixture
+	{
+		private ISessionFactoryImplementor _sessionFactory;
+		private ICollectionPersister _bagPersister;
+		private ICollectionPersister _mapPersister;
+
+		[OneTimeSetUp]
+		public void OneTimeSetUp()
+		{
+			var configuration = TestConfigurationHelper.GetDefaultConfiguration();
+			configuration.AddResource(
+				typeof(WhereWithReadLockFixture).Namespace + ".Domain.hbm.xml",
+				typeof(WhereWithReadLockFixture).Assembly);
+			configuration.SetProperty(Environment.Dialect, typeof(DialectWithReadLockHint).AssemblyQualifiedName);
+
+			_sessionFactory = (ISessionFactoryImplementor) configuration.BuildSessionFactory();
+			_bagPersister =
+				_sessionFactory.GetCollectionPersister(typeof(Env).FullName + "." + nameof(Env.RequestsFailed));
+			Assert.That(
+				_bagPersister,
+				Is.InstanceOf(typeof(AbstractCollectionPersister)),
+				"Unexpected bag persister type");
+			_mapPersister =
+				_sessionFactory.GetCollectionPersister(typeof(Env).FullName + "." + nameof(Env.FailedRequestsById));
+			Assert.That(
+				_mapPersister,
+				Is.InstanceOf(typeof(AbstractCollectionPersister)),
+				"Unexpected map persister type");
+		}
+
+		[OneTimeTearDown]
+		public void OneTimeTearDown()
+		{
+			_sessionFactory?.Dispose();
+		}
+
+		[Test]
+		public void GenerateLockHintAtEndForExtraLazyCount()
+		{
+			var selectMethod = typeof(AbstractCollectionPersister).GetMethod(
+				"GenerateSelectSizeString",
+				BindingFlags.Instance | BindingFlags.NonPublic);
+			Assert.That(selectMethod, Is.Not.Null, "Unable to find GenerateSelectSizeString method");
+
+			using (var s = _sessionFactory.OpenSession())
+			{
+				var select = (SqlString) selectMethod.Invoke(_bagPersister, new object[] { s });
+				Assert.That(select.ToString(), Does.EndWith(DialectWithReadLockHint.ReadOnlyLock));
+
+				s.EnableFilter("CurrentOnly");
+				select = (SqlString) selectMethod.Invoke(_bagPersister, new object[] { s });
+				Assert.That(select.ToString(), Does.EndWith(DialectWithReadLockHint.ReadOnlyLock));
+			}
+		}
+
+		[Test]
+		public void GenerateLockHintAtEndForDetectRowByIndex()
+		{
+			var sqlField = typeof(AbstractCollectionPersister).GetField(
+				"sqlDetectRowByIndexString",
+				BindingFlags.Instance | BindingFlags.NonPublic);
+			Assert.That(sqlField, Is.Not.Null, "Unable to find sqlDetectRowByIndexString field");
+
+			var sql = (SqlString) sqlField.GetValue(_mapPersister);
+			Assert.That(sql.ToString(), Does.EndWith(DialectWithReadLockHint.ReadOnlyLock));
+		}
+
+		[Test]
+		public void GenerateLockHintAtEndForSelectRowByIndex()
+		{
+			var sqlField = typeof(AbstractCollectionPersister).GetField(
+				"sqlSelectRowByIndexString",
+				BindingFlags.Instance | BindingFlags.NonPublic);
+			Assert.That(sqlField, Is.Not.Null, "Unable to find sqlSelectRowByIndexString field");
+
+			var sql = (SqlString) sqlField.GetValue(_mapPersister);
+			Assert.That(sql.ToString(), Does.EndWith(DialectWithReadLockHint.ReadOnlyLock));
+		}
+
+		[Test]
+		public void GenerateLockHintAtEndForDetectRowByElement()
+		{
+			var sqlField = typeof(AbstractCollectionPersister).GetField(
+				"sqlDetectRowByElementString",
+				BindingFlags.Instance | BindingFlags.NonPublic);
+			Assert.That(sqlField, Is.Not.Null, "Unable to find sqlDetectRowByElementString field");
+
+			var sql = (SqlString) sqlField.GetValue(_mapPersister);
+			Assert.That(sql.ToString(), Does.EndWith(DialectWithReadLockHint.ReadOnlyLock));
+		}
+
+		[Test]
+		public void GenerateLockHintAtEndForSelectByUniqueKey()
+		{
+			var sql = ((IPostInsertIdentityPersister) _bagPersister).GetSelectByUniqueKeyString("blah");
+			Assert.That(sql.ToString(), Does.EndWith(DialectWithReadLockHint.ReadOnlyLock));
+		}
+	}
+}

src/NHibernate/Persister/Collection/AbstractCollectionPersister.cs

@@ -896,23 +896,25 @@ public string SelectFragment(string alias, string columnSuffix)
 
 			return frag.ToSqlStringFragment(false);
 		}
-		private SqlString AddWhereFragment(SqlString sql)
+
+		private void AddWhereFragment(SqlSimpleSelectBuilder sql)
 		{
 			if (!hasWhere)
-				return sql;
-			return sql.Append(" and ").Append(sqlWhereString);
+				return;
+			sql.AddWhereFragment(sqlWhereString);
 		}
 
 		private SqlString GenerateSelectSizeString(ISessionImplementor sessionImplementor)
 		{
-			string selectValue = GetCountSqlSelectClause();
+			var selectValue = GetCountSqlSelectClause();
 
-			return new SqlSimpleSelectBuilder(dialect, factory)
-				.SetTableName(TableName)
-				.AddWhereFragment(KeyColumnNames, KeyType, "=")
-				.AddColumn(selectValue)
-				.ToSqlString()
-				.Append(FilterFragment(TableName, sessionImplementor.EnabledFilters));
+			return
+				new SqlSimpleSelectBuilder(dialect, factory)
+					.SetTableName(TableName)
+					.AddWhereFragment(KeyColumnNames, KeyType, "=")
+					.AddWhereFragment(FilterFragment(TableName, sessionImplementor.EnabledFilters))
+					.AddColumn(selectValue)
+					.ToSqlString();
 		}
 
 		protected virtual string GetCountSqlSelectClause()
@@ -936,14 +938,15 @@ private SqlString GenerateDetectRowByIndexString()
 			}
 
 			// TODO NH: may be we need something else when Index is mixed with Formula
-			var sqlString=
+			var builder =
 				new SqlSimpleSelectBuilder(dialect, factory)
 					.SetTableName(TableName)
 					.AddWhereFragment(KeyColumnNames, KeyType, "=")
 					.AddWhereFragment(IndexColumnNames, IndexType, "=")
 					.AddWhereFragment(indexFormulas, IndexType, "=")
-					.AddColumn("1").ToSqlString();
-			return AddWhereFragment(sqlString);
+					.AddColumn("1");
+			AddWhereFragment(builder);
+			return builder.ToSqlString();
 		}
 
 		private SqlString GenerateSelectRowByIndexString()
@@ -953,26 +956,29 @@ private SqlString GenerateSelectRowByIndexString()
 				return null;
 			}
 
-			var sqlString=new SqlSimpleSelectBuilder(dialect, factory)
-				.SetTableName(TableName)
-				.AddWhereFragment(KeyColumnNames, KeyType, "=")
-				.AddWhereFragment(IndexColumnNames, IndexType, "=")
-				.AddWhereFragment(indexFormulas, IndexType, "=")
-				.AddColumns(ElementColumnNames, elementColumnAliases)
-				.AddColumns(indexFormulas, indexColumnAliases).ToSqlString();
-			return AddWhereFragment(sqlString);
+			var builder =
+				new SqlSimpleSelectBuilder(dialect, factory)
+					.SetTableName(TableName)
+					.AddWhereFragment(KeyColumnNames, KeyType, "=")
+					.AddWhereFragment(IndexColumnNames, IndexType, "=")
+					.AddWhereFragment(indexFormulas, IndexType, "=")
+					.AddColumns(ElementColumnNames, elementColumnAliases)
+					.AddColumns(indexFormulas, indexColumnAliases);
+			AddWhereFragment(builder);
+			return builder.ToSqlString();
 		}
 
 		private SqlString GenerateDetectRowByElementString()
 		{
-			var sqlString=
+			var builder =
 				new SqlSimpleSelectBuilder(dialect, factory)
-				.SetTableName(TableName)
-				.AddWhereFragment(KeyColumnNames, KeyType, "=")
-				.AddWhereFragment(ElementColumnNames, ElementType, "=")
-				.AddWhereFragment(elementFormulas, ElementType, "=")
-				.AddColumn("1").ToSqlString();
-			return AddWhereFragment(sqlString);
+					.SetTableName(TableName)
+					.AddWhereFragment(KeyColumnNames, KeyType, "=")
+					.AddWhereFragment(ElementColumnNames, ElementType, "=")
+					.AddWhereFragment(elementFormulas, ElementType, "=")
+					.AddColumn("1");
+			AddWhereFragment(builder);
+			return builder.ToSqlString();
 		}
 
 		protected virtual SelectFragment GenerateSelectFragment(string alias, string columnSuffix)

src/NHibernate/SqlCommand/SqlSimpleSelectBuilder.cs

@@ -1,3 +1,4 @@
+using System;
 using System.Collections.Generic;
 using NHibernate.Engine;
 using NHibernate.Type;
@@ -170,6 +171,23 @@ public SqlSimpleSelectBuilder AddWhereFragment(string[] columnNames, IType type,
 			return this;
 		}
 
+		/// <summary>
+		/// Adds an arbitrary where fragment.
+		/// </summary>
+		/// <param name="fragment">The fragment.</param>
+		/// <returns>The SqlSimpleSelectBuilder</returns>
+		public SqlSimpleSelectBuilder AddWhereFragment(string fragment)
+		{
+			if (string.IsNullOrWhiteSpace(fragment))
+				return this;
+
+			if (fragment.Trim().StartsWith("and ", StringComparison.OrdinalIgnoreCase))
+				fragment = fragment.Substring(fragment.IndexOf("and", StringComparison.OrdinalIgnoreCase) + 3);
+
+			whereStrings.Add(new SqlString(fragment));
+			return this;
+		}
+
 		public virtual SqlSimpleSelectBuilder SetComment(System.String comment)
 		{
 			this.comment = comment;
@@ -238,4 +256,4 @@ public SqlString ToSqlString()
 
 		#endregion
 	}
-}
+}