fix #2461 &= in the bool DSL is viral. (#2487)

The performance and allocations fixes in #2235 also introduced virality
on the QueryContainer when using &= assignment. The composed result was
correct but the QueryContainer composed of `&=` would be left marked as
`CreatedByTheBoolDsl` which meant it was marked for reuse.

If you then later combined it with && (andAssignedQuery && newQuery) the resulting
query would be correct but if you rely on `andAssignedQuery` not being
mutated you'd be in for a nasty suprise.

A QueryContainer composed with `&&` did not expose the same virality
problems.

Removed the notion of trying to reuse all together since the big
performance gain from #2235 is that we can flatten many boolean should
queries using `&=` or `&&` or visa versa many boolean must queries with
`|=` or `||`.

(cherry-picked from commit 10ee84a)

Author: russcam

src/Nest/QueryDsl/Abstractions/Container/QueryContainer-Dsl.cs

@@ -68,7 +68,7 @@ private static bool IfEitherIsEmptyReturnTheOtherOrEmpty(QueryContainer leftCont
 			if (!leftWritable && !rightWritable) return true;
 
 			queryContainer = leftWritable ? leftContainer : rightContainer;
-			return !leftWritable || !rightWritable;
+			return true;
 		}
 
 		public static QueryContainer operator !(QueryContainer queryContainer) => queryContainer == null || (!queryContainer.IsWritable)

src/Nest/QueryDsl/Abstractions/Query/BoolQueryAndExtensions.cs

@@ -26,8 +26,7 @@ internal static QueryContainer CombineAsMust(this QueryContainer leftContainer,
 			var filterClauses = OrphanFilters(leftContainer).EagerConcat(OrphanFilters(rightContainer));
 			var mustClauses = OrphanMusts(leftContainer).EagerConcat(OrphanMusts(rightContainer));
 
-			var reuseContainer = ContainerContainingBool(leftContainer, rightContainer);
-			var container = CreateMustContainer(mustClauses, mustNotClauses, filterClauses, reuseContainer);
+			var container = CreateMustContainer(mustClauses, mustNotClauses, filterClauses);
 			return container;
 		}
 
@@ -79,66 +78,41 @@ private static bool TryHandleBoolsWithOnlyShouldClauses(
 			c = null;
 			var leftHasOnlyShoulds = leftBool.HasOnlyShouldClauses();
 			var rightHasOnlyShoulds = rightBool.HasOnlyShouldClauses();
-			if (leftHasOnlyShoulds || rightHasOnlyShoulds)
+			if (!leftHasOnlyShoulds && !rightHasOnlyShoulds) return false;
+			if (leftContainer.HoldsOnlyShouldMusts && rightHasOnlyShoulds)
 			{
-				if (leftContainer.HoldsOnlyShouldMusts && rightHasOnlyShoulds)
-				{
-					leftBool.Must = leftBool.Must.AddIfNotNull(rightContainer);
-					c = leftContainer;
-				}
-				else if (rightContainer.HoldsOnlyShouldMusts && leftHasOnlyShoulds)
-				{
-					rightBool.Must = rightBool.Must.AddIfNotNull(leftContainer);
-					c = rightContainer;
-				}
-				else
-				{
-					//c = CreateMustContainer(leftContainer, rightContainer);
-					c = CreateMustContainer(new Containers { leftContainer, rightContainer }, null);
-					c.HoldsOnlyShouldMusts = rightHasOnlyShoulds && leftHasOnlyShoulds;
-				}
+				leftBool.Must = leftBool.Must.AddIfNotNull(rightContainer);
+				c = leftContainer;
 			}
-			return c != null;
+			else if (rightContainer.HoldsOnlyShouldMusts && leftHasOnlyShoulds)
+			{
+				rightBool.Must = rightBool.Must.AddIfNotNull(leftContainer);
+				c = rightContainer;
+			}
+			else
+			{
+				c = CreateMustContainer(new Containers { leftContainer, rightContainer }, null);
+				c.HoldsOnlyShouldMusts = rightHasOnlyShoulds && leftHasOnlyShoulds;
+			}
+			return true;
 		}
 
 		private static QueryContainer CreateMustContainer(QueryContainer left, QueryContainer right)
 		{
-			return CreateMustContainer(new Containers { left, right }, ContainerContainingBool(left, right));
-		}
-
-		private static QueryContainer ContainerContainingBool(QueryContainer left, QueryContainer right)
-		{
-			var l = (left?.Self().Bool?.CreatedByBoolDsl).GetValueOrDefault();
-			var r = (right?.Self().Bool?.CreatedByBoolDsl).GetValueOrDefault();
-			return l ? left : r ? right : null;
+			return CreateMustContainer(new Containers { left, right }, null);
 		}
 
 		private static QueryContainer CreateMustContainer(List<QueryContainer> mustClauses, QueryContainer reuse)
 		{
-			var existingBool = reuse?.Self().Bool;
-			if (existingBool != null && existingBool.CreatedByBoolDsl)
-			{
-				existingBool.Must = mustClauses.ToListOrNullIfEmpty();
-				return reuse;
-			}
-			return new QueryContainer(new BoolQuery(createdByBoolDsl: true) { Must = mustClauses.ToListOrNullIfEmpty() });
+			return new QueryContainer(new BoolQuery() { Must = mustClauses.ToListOrNullIfEmpty() });
 		}
 
 		private static QueryContainer CreateMustContainer(
 			List<QueryContainer> mustClauses,
 			List<QueryContainer> mustNotClauses,
-			List<QueryContainer> filters,
-			QueryContainer reuse
+			List<QueryContainer> filters
 			)
 		{
-			var existingBool = reuse?.Self().Bool;
-			if (existingBool != null && existingBool.CreatedByBoolDsl)
-			{
-				existingBool.Must = mustClauses.ToListOrNullIfEmpty();
-				existingBool.MustNot = mustNotClauses.ToListOrNullIfEmpty();
-				existingBool.Filter = filters.ToListOrNullIfEmpty();
-				return reuse;
-			}
 			return new QueryContainer(new BoolQuery
 			{
 				Must = mustClauses.ToListOrNullIfEmpty(),
@@ -148,11 +122,8 @@ QueryContainer reuse
 		}
 
 		private static bool CanMergeAnd(this IBoolQuery boolQuery) =>
-			//boolQuery != null && boolQuery.IsWritable && !boolQuery.Locked && !boolQuery.Should.HasAny();
 			boolQuery != null && !boolQuery.Locked && !boolQuery.Should.HasAny();
 
-		private static bool CanMergeAnd(this IQueryContainer container) => container.Bool.CanMergeAnd();
-
 		private static IEnumerable<QueryContainer> OrphanMusts(QueryContainer container)
 		{
 			var lBoolQuery = container.Self().Bool;

src/Nest/QueryDsl/Abstractions/Query/BoolQueryOrExtensions.cs

@@ -56,7 +56,8 @@ private static bool CanMergeShould(this IBoolQuery boolQuery) =>
 			boolQuery != null && boolQuery.IsWritable && !boolQuery.Locked && boolQuery.HasOnlyShouldClauses();
 
 		private static QueryContainer CreateShouldContainer(List<QueryContainer> shouldClauses) =>
-			new BoolQuery(createdByBoolDsl: true)
+			//new BoolQuery(createdByBoolDsl: true)
+			new BoolQuery()
 			{
 				Should = shouldClauses.ToListOrNullIfEmpty()
 			};

src/Nest/QueryDsl/Abstractions/Query/QueryBase.cs

@@ -82,14 +82,8 @@ private static bool IfEitherIsEmptyReturnTheOtherOrEmpty(QueryBase leftQuery, Qu
 			return anyEmpty;
 		}
 
-		public static implicit operator QueryContainer(QueryBase query)
-		{
-			if (query == null)
-				return null;
-			return new QueryContainer(query);
-
-
-		}
+		public static implicit operator QueryContainer(QueryBase query) =>
+			query == null ? null : new QueryContainer(query);
 
 		internal void WrapInContainer(IQueryContainer container)
 		{

src/Nest/QueryDsl/Abstractions/Query/QueryDescriptorBase.cs

@@ -23,6 +23,6 @@ public abstract class QueryDescriptorBase<TDescriptor, TInterface>
 
 		public TDescriptor Strict(bool strict = true) => Assign(a => a.IsStrict = strict);
 
-		bool IQuery.IsWritable { get { return Self.IsVerbatim || !Self.Conditionless; } }
+		bool IQuery.IsWritable => Self.IsVerbatim || !Self.Conditionless;
 	}
 }

src/Nest/QueryDsl/Compound/Bool/BoolQuery.cs

@@ -51,15 +51,12 @@ public interface IBoolQuery : IQuery
 		bool? DisableCoord { get; set; }
 
 		bool Locked { get; }
-		bool CreatedByBoolDsl { get; }
 	}
 
 	public class BoolQuery : QueryBase, IBoolQuery
 	{
 		internal static bool Locked(IBoolQuery q) => !q.Name.IsNullOrEmpty() || q.Boost.HasValue || q.DisableCoord.HasValue || q.MinimumShouldMatch != null;
 		bool IBoolQuery.Locked => BoolQuery.Locked(this);
-		private readonly bool _createdByBoolDsl;
-		bool IBoolQuery.CreatedByBoolDsl => _createdByBoolDsl;
 
 		private IList<QueryContainer> _must;
 		private IList<QueryContainer> _mustNot;
@@ -68,16 +65,6 @@ public class BoolQuery : QueryBase, IBoolQuery
 
 		public BoolQuery() { }
 
-		/// <summary>
-		/// Internal constructor which we use internally in the bool dsl so we know its safe to reuse a boolean query instance
-		/// </summary>
-		/// <param name="createdByBoolDsl">ignored</param>
-		internal BoolQuery(bool createdByBoolDsl)
-		{
-			this._createdByBoolDsl = true;
-		}
-
-
 		/// <summary>
 		/// The clause(s) that must appear in matching documents
 		/// </summary>
@@ -136,7 +123,6 @@ public class BoolQueryDescriptor<T>
 		, IBoolQuery where T : class
 	{
 		bool IBoolQuery.Locked => BoolQuery.Locked(this);
-		bool IBoolQuery.CreatedByBoolDsl { get; } = false;
 
 		protected override bool Conditionless => BoolQuery.IsConditionless(this);
 		private IList<QueryContainer> _must;

src/Tests/QueryDsl/BoolDsl/BoolDsl.doc.cs

@@ -336,6 +336,33 @@ private static void AssertDoesNotJoinOntoLockedBool(IQueryContainer c, string fi
 			nestedBool.Bool.Name.Should().Be(firstName);
 		}
 
+		//hide
+		[U] public void AndShouldNotBeViralAnyWayYouComposeIt()
+		{
+			QueryContainer andAssign = new TermQuery { Field = "a", Value = "a" };
+			andAssign &= new TermQuery { Field = "b", Value = "b" };
+
+			AssertAndIsNotViral(andAssign, "&=");
+
+			QueryContainer andOperator =
+				new TermQuery { Field = "a", Value = "a" } && new TermQuery { Field = "b", Value = "b" };
+
+			AssertAndIsNotViral(andOperator, "&&");
+		}
+
+		//hide
+		private static void AssertAndIsNotViral(QueryContainer query, string origin)
+		{
+			IQueryContainer original = query;
+			original.Bool.Must.Should().HaveCount(2, $"query composed using {origin} should have 2 must clauses before");
+			IQueryContainer result = query && new TermQuery { Field = "c", Value = "c" };
+			result.Bool.Must.Should().HaveCount(3, $"query composed using {origin} combined with && should have 3 must clauses");
+			original.Bool.Must.Should().HaveCount(2, $"query composed using {origin} should still have 2 must clauses after composition");
+		}
+
+
+
+
 		/** === Perfomance considerations
 		*
 		* If you have a requirement of combining many many queries using the bool dsl please take the following into account.
@@ -349,8 +376,10 @@ private static void SlowCombine()
 			var c = new QueryContainer();
 			var q = new TermQuery { Field = "x", Value = "x" };
 
-			for (var i=0;i<1000;i++)
+			for (var i = 0; i < 1000; i++)
+			{
 				c &= q;
+			}
 		}
 		/**
 		 *....

src/Tests/tests.yaml

@@ -4,7 +4,10 @@ mode: m
 elasticsearch_version: 2.4.2
 # cluster filter allows you to only run the integration tests of a particular cluster (cluster suffix not needed)
 cluster_filter: 
+# test filter allows you to run a particular test
+test_filter:
 # whether we want to forcefully reseed on the node, if you are starting the tests with a node already running
 force_reseed: true
-# do not spawn nodes as part of the test setup but rely on a manually started es node being up
-do_not_spawn: false
+# do not spawn nodes as part of the test setup if we find a node is already running
+# this is opt in during development in CI we never want to see our tests running against an already running node
+test_against_already_running_elasticsearch: true