Avoid side effects when combining with And operator (#7615) (#7617)

* Avoid modifying an existing bool query when combining

* Update dictionary exclusions

Co-authored-by: Steve Gordon <[email protected]>

Author: github-actions[bot]

.editorconfig

@@ -201,6 +201,7 @@ resharper_redundant_case_label_highlighting=do_not_show
 resharper_redundant_argument_default_value_highlighting=do_not_show
 
 spelling_languages=en-us,en-gb
+spelling_exclusion_path=exclusion.dic
 
 [Jenkinsfile]
 indent_style = space

exclusion.dic

@@ -8,4 +8,6 @@ enum
 trippable
 geotile
 yaml
-rethrottle
+rethrottle
+mergable
+unmergable

src/Elastic.Clients.Elasticsearch/Types/QueryDsl/BoolQueryAndExtensions.cs

@@ -3,6 +3,7 @@
 // See the LICENSE file in the project root for more information.
 
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 
 namespace Elastic.Clients.Elasticsearch.QueryDsl;
@@ -14,17 +15,23 @@ internal static Query CombineAsMust(this Query leftContainer, Query rightContain
 		var hasLeftBool = leftContainer.TryGet<BoolQuery>(out var leftBool);
 		var hasRightBool = rightContainer.TryGet<BoolQuery>(out var rightBool);
 
-		//neither side is a bool, no special handling needed wrap in a bool must
+		// neither side is a bool, no special handling needed wrap in a bool must
 		if (!hasLeftBool && !hasRightBool)
+		{
 			return CreateMustContainer(new List<Query> { leftContainer, rightContainer });
+		}
 
 		else if (TryHandleBoolsWithOnlyShouldClauses(leftContainer, rightContainer, leftBool, rightBool, out var query))
+		{
 			return query;
+		}
 
 		else if (TryHandleUnmergableBools(leftContainer, rightContainer, leftBool, rightBool, out query))
+		{
 			return query;
+		}
 
-		//neither side is unmergable so neither is a bool with should clauses
+		// neither side is unmergable so neither is a bool with should clauses
 
 		var mustNotClauses = OrphanMustNots(leftContainer).EagerConcat(OrphanMustNots(rightContainer));
 		var filterClauses = OrphanFilters(leftContainer).EagerConcat(OrphanFilters(rightContainer));
@@ -36,45 +43,92 @@ internal static Query CombineAsMust(this Query leftContainer, Query rightContain
 	}
 
 	/// <summary>
-	/// Handles cases where either side is a bool which indicates it can't be merged yet the other side is mergable.
+	/// Handles cases where either side is a bool which indicates it can't be merged, yet the other side is mergable.
 	/// A side is considered unmergable if its locked (has important metadata) or has should clauses.
-	/// Instead of always wrapping these cases in another bool we merge to unmergable side into to others must clause therefor flattening the
-	/// generated graph
+	/// Instead of always wrapping these cases in another bool we merge the unmergable side into to others must clause therefore flattening the
+	/// generated graph. In such cases, we copy the existing BoolQuery so as not to cause a potentially surprising side-effect. (see https://github.com/elastic/elasticsearch-net/issues/5076).
 	/// </summary>
-	private static bool TryHandleUnmergableBools(Query leftContainer, Query rightContainer, BoolQuery leftBool, BoolQuery rightBool, out Query query)
+	private static bool TryHandleUnmergableBools(Query leftContainer, Query rightContainer, BoolQuery? leftBool, BoolQuery? rightBool, [NotNullWhen(true)] out Query? query)
 	{
 		query = null;
-		var leftCantMergeAnd = leftBool != null && !leftBool.CanMergeAnd();
-		var rightCantMergeAnd = rightBool != null && !rightBool.CanMergeAnd();
+
+		var leftCantMergeAnd = leftBool is not null && !leftBool.CanMergeAnd();
+		var rightCantMergeAnd = rightBool is not null && !rightBool.CanMergeAnd();
+
 		if (!leftCantMergeAnd && !rightCantMergeAnd)
+		{
 			return false;
+		}
 
 		if (leftCantMergeAnd && rightCantMergeAnd)
+		{
 			query = CreateMustContainer(leftContainer, rightContainer);
+		}
 
-		//right can't merge but left can and is a bool so we add left to the must clause of right
+		// right can't merge but left can and is a bool so we add left to the must clause of right
 		else if (!leftCantMergeAnd && leftBool != null && rightCantMergeAnd)
 		{
-			leftBool.Must = leftBool.Must.AddIfNotNull(rightContainer).ToArray();
-			query = leftContainer;
+			if (rightContainer is null)
+			{
+				query = leftContainer;
+			}
+			else
+			{
+				// We create a copy here to avoid side-effects on a user provided bool query such as
+				// adding a must clause where one did not previously exist.
+
+				var leftBoolCopy = new BoolQuery
+				{
+					Boost = leftBool.Boost,
+					MinimumShouldMatch = leftBool.MinimumShouldMatch,
+					QueryName = leftBool.QueryName,
+					Should = leftBool.Should,
+					Must = leftBool.Must.AddIfNotNull(rightContainer).ToArray(),
+					MustNot = leftBool.MustNot,
+					Filter = leftBool.Filter
+				};
+
+				query = leftBoolCopy;
+			}
 		}
 
-		//right can't merge and left is not a bool, we forcefully create a wrapped must container
-		else if (!leftCantMergeAnd && leftBool == null && rightCantMergeAnd)
+		// right can't merge and left is not a bool, we forcefully create a wrapped must container
+		else if (!leftCantMergeAnd && leftBool is null && rightCantMergeAnd)
+		{
 			query = CreateMustContainer(leftContainer, rightContainer);
+		}
 
-		//left can't merge but right can and is a bool so we add left to the must clause of right
-		else if (leftCantMergeAnd && !rightCantMergeAnd && rightBool != null)
+		// left can't merge but right can and is a bool so we add left to the must clause of right
+		else if (leftCantMergeAnd && !rightCantMergeAnd && rightBool is not null)
 		{
-			rightBool.Must = rightBool.Must.AddIfNotNull(leftContainer).ToArray();
-			query = rightContainer;
+			if (leftContainer is null)
+			{
+				query = rightContainer;
+			}
+			else
+			{
+				var rightBoolCopy = new BoolQuery
+				{
+					Boost = rightBool.Boost,
+					MinimumShouldMatch = rightBool.MinimumShouldMatch,
+					QueryName = rightBool.QueryName,
+					Should = rightBool.Should,
+					Must = rightBool.Must.AddIfNotNull(leftContainer).ToArray(),
+					MustNot = rightBool.MustNot,
+					Filter = rightBool.Filter
+				};
+
+				query = rightBoolCopy;
+			}
 		}
 
 		//left can't merge and right is not a bool, we forcefully create a wrapped must container
 		else if (leftCantMergeAnd && !rightCantMergeAnd && rightBool == null)
+		{
 			query = CreateMustContainer(new List<Query> { leftContainer, rightContainer });
+		}
 
-		return query != null;
+		return query is not null;
 	}
 
 	/// <summary>
@@ -86,10 +140,14 @@ private static bool TryHandleUnmergableBools(Query leftContainer, Query rightCon
 	private static bool TryHandleBoolsWithOnlyShouldClauses(Query leftContainer, Query rightContainer, BoolQuery leftBool, BoolQuery rightBool, out Query query)
 	{
 		query = null;
+
 		var leftHasOnlyShoulds = leftBool.HasOnlyShouldClauses();
 		var rightHasOnlyShoulds = rightBool.HasOnlyShouldClauses();
+
 		if (!leftHasOnlyShoulds && !rightHasOnlyShoulds)
+		{
 			return false;
+		}
 
 		if (leftContainer.HoldsOnlyShouldMusts && rightHasOnlyShoulds)
 		{
@@ -106,25 +164,26 @@ private static bool TryHandleBoolsWithOnlyShouldClauses(Query leftContainer, Que
 			query = CreateMustContainer(new List<Query> { leftContainer, rightContainer });
 			query.HoldsOnlyShouldMusts = rightHasOnlyShoulds && leftHasOnlyShoulds;
 		}
+
 		return true;
 	}
 
 	private static Query CreateMustContainer(Query left, Query right) =>
 		CreateMustContainer(new List<Query> { left, right });
 
 	private static Query CreateMustContainer(List<Query> mustClauses) =>
-		new Query(new BoolQuery() { Must = mustClauses.ToListOrNullIfEmpty() });
+		new(new BoolQuery() { Must = mustClauses.ToListOrNullIfEmpty() });
 
 	private static Query CreateMustContainer(
 		List<Query> mustClauses,
 		List<Query> mustNotClauses,
 		List<Query> filters
-		) => new Query(new BoolQuery
-	{
-		Must = mustClauses.ToListOrNullIfEmpty(),
-		MustNot = mustNotClauses.ToListOrNullIfEmpty(),
-		Filter = filters.ToListOrNullIfEmpty()
-	});
+		) => new(new BoolQuery
+		{
+			Must = mustClauses.ToListOrNullIfEmpty(),
+			MustNot = mustNotClauses.ToListOrNullIfEmpty(),
+			Filter = filters.ToListOrNullIfEmpty()
+		});
 
 	private static bool CanMergeAnd(this BoolQuery boolQuery) =>
 		boolQuery != null && !boolQuery.Locked && !boolQuery.Should.HasAny();

src/Elastic.Clients.Elasticsearch/Types/QueryDsl/SearchQuery.cs

@@ -11,52 +11,55 @@ namespace Elastic.Clients.Elasticsearch.QueryDsl;
 /// </summary>
 public abstract partial class SearchQuery
 {
-	//always evaluate to false so that each side of && equation is evaluated
+	// Always evaluate to false so that each side of && equation is evaluated
 	public static bool operator false(SearchQuery _) => false;
 
-	//always evaluate to false so that each side of && equation is evaluated
+	// Always evaluate to false so that each side of && equation is evaluated
 	public static bool operator true(SearchQuery _) => false;
 
-	public static SearchQuery operator &(SearchQuery leftQuery, SearchQuery rightQuery) => Combine(leftQuery, rightQuery, (l, r) => l && r);
+	public static SearchQuery? operator &(SearchQuery? leftQuery, SearchQuery? rightQuery) => Combine(leftQuery, rightQuery, (l, r) => l && r);
 
-	public static SearchQuery? operator |(SearchQuery leftQuery, SearchQuery rightQuery) => Combine(leftQuery, rightQuery, (l, r) => l || r);
+	public static SearchQuery? operator |(SearchQuery? leftQuery, SearchQuery? rightQuery) => Combine(leftQuery, rightQuery, (l, r) => l || r);
 
-	public static SearchQuery? operator !(SearchQuery query) => query is null 
+	public static SearchQuery? operator !(SearchQuery? query) => query is null
 		? null
 		: new BoolQuery { MustNot = new Query[] { query } };
 
-	public static SearchQuery? operator +(SearchQuery query) => query is null
+	public static SearchQuery? operator +(SearchQuery? query) => query is null
 		? null
 		: new BoolQuery { Filter = new Query[] { query } };
 
-	private static SearchQuery Combine(SearchQuery leftQuery, SearchQuery rightQuery, Func<Query?, Query?, Query> combine)
+	private static SearchQuery? Combine(SearchQuery? leftQuery, SearchQuery? rightQuery, Func<Query?, Query?, Query> combine)
 	{
+		// If both sides are null, return null
 		if (leftQuery is null && rightQuery is null)
+		{
 			return null;
+		}
 
+		// If the left side is null, return the right side
 		if (leftQuery is null)
+		{
 			return rightQuery;
+		}
 
+		// If the right side is null, return the left side
 		if (rightQuery is null)
+		{
 			return leftQuery;
+		}
 
 		var container = combine(leftQuery, rightQuery);
 
-		if (container.TryGet<BoolQuery>(out var query))
+		if (container.TryGet<BoolQuery>(out var boolQuery))
 		{
-			return new BoolQuery
-			{
-				Must = query.Must,
-				MustNot = query.MustNot,
-				Should = query.Should,
-				Filter = query.Filter,
-			};
+			return boolQuery;
 		}
 
 		throw new Exception("Unable to combine queries.");
 	}
 
-	public static implicit operator Query?(SearchQuery query) => query is null ? null : new Query(query);
+	public static implicit operator Query?(SearchQuery? query) => query is null ? null : new Query(query);
 
 	// We no longer (currently) support verbatim/strict query containers so this may be simplified to a direct abstract method in the future.
 	internal void WrapInContainer(Query container) => InternalWrapInContainer(container);

tests/Tests/QueryDsl/BoolDsl/AndOperatorUsageTests.cs

@@ -6,6 +6,7 @@
 using System.Linq;
 using Elastic.Clients.Elasticsearch.QueryDsl;
 using Tests.Domain;
+using Xunit;
 
 namespace Tests.QueryDsl.BoolDsl;
 
@@ -296,6 +297,49 @@ public void AndAssigningNamedBoolShouldQueries()
 			((BoolQuery)s.Variant).QueryName == "name");
 	}
 
+	[U]
+	public void AndAssigningShouldNotModifyOriginalBoolQuery()
+	{
+		// Based on https://github.com/elastic/elasticsearch-net/issues/5076
+
+		// create a bool query with a filter that might be reused by the consuming code and expected not to change.
+		Query filterQueryContainerExpectedToNotChange = +new TermQuery("do-not") { Value = "change" };
+
+		filterQueryContainerExpectedToNotChange.TryGet<BoolQuery>(out var boolQueryBefore).Should().BeTrue();
+
+		boolQueryBefore.Must.Should().BeNull();
+		boolQueryBefore.Filter.Single().TryGet<TermQuery>(out var termQueryBefore).Should().BeTrue();
+		termQueryBefore.Field.Should().Be("do-not");
+		termQueryBefore.Value.TryGetString(out var termValue).Should().BeTrue();
+		termValue.Should().Be("change");
+		
+		var queryToExecute = new BoolQuery
+		{
+			Should = new Query[] { new MatchAllQuery() },
+			Filter = new Query[] { new MatchAllQuery() } // if either of these are removed, it works as expected
+		}
+		&& filterQueryContainerExpectedToNotChange;
+
+		filterQueryContainerExpectedToNotChange.TryGet<BoolQuery>(out var boolQueryAfter).Should().BeTrue();
+		boolQueryAfter.Must.Should().BeNull();
+		boolQueryAfter.Filter.Single().TryGet<TermQuery>(out var termQueryAfter).Should().BeTrue();
+		termQueryAfter.Field.Should().Be("do-not");
+		termQueryAfter.Value.TryGetString(out termValue).Should().BeTrue();
+		termValue.Should().Be("change");
+
+		queryToExecute.TryGet<BoolQuery>(out var boolToExecute).Should().BeTrue();
+		boolToExecute.Filter.Should().HaveCount(1);
+		boolToExecute.Must.Should().HaveCount(1);
+		boolToExecute.Filter.Single().TryGet<TermQuery>(out var queryToExecuteTermQuery).Should().BeTrue();
+		queryToExecuteTermQuery.Field.Should().Be("do-not");
+		queryToExecuteTermQuery.Value.TryGetString(out termValue).Should().BeTrue();
+		termValue.Should().Be("change");
+
+		boolToExecute.Must.Single().TryGet<BoolQuery>(out var mustClauseBool).Should().BeTrue();
+		mustClauseBool.Filter.Should().HaveCount(1);
+		mustClauseBool.Should.Should().HaveCount(1);
+	}
+
 	private static void AssertLotsOfAnds(Query lotsOfAnds)
 	{
 		lotsOfAnds.Should().NotBeNull();