Skip to content

Commit 26f8d60

Browse files
stevejgordonstevejgordon
authored
Avoid side effects when combining with And operator (#7615)
* Avoid modifying an existing bool query when combining * Update dictionary exclusions
1 parent e2be42d commit 26f8d60

File tree

5 files changed

+151
-42
lines changed

5 files changed

+151
-42
lines changed

‎.editorconfig

+1
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,7 @@ resharper_redundant_case_label_highlighting=do_not_show
201201
resharper_redundant_argument_default_value_highlighting=do_not_show
202202

203203
spelling_languages=en-us,en-gb
204+
spelling_exclusion_path=exclusion.dic
204205

205206
[Jenkinsfile]
206207
indent_style = space

‎exclusion.dic

+3-1
Original file line numberDiff line numberDiff line change
@@ -8,4 +8,6 @@ enum
88
trippable
99
geotile
1010
yaml
11-
rethrottle
11+
rethrottle
12+
mergable
13+
unmergable

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

+84-25
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// See the LICENSE file in the project root for more information.
44

55
using System.Collections.Generic;
6+
using System.Diagnostics.CodeAnalysis;
67
using System.Linq;
78

89
namespace Elastic.Clients.Elasticsearch.QueryDsl;
@@ -14,17 +15,23 @@ internal static Query CombineAsMust(this Query leftContainer, Query rightContain
1415
var hasLeftBool = leftContainer.TryGet<BoolQuery>(out var leftBool);
1516
var hasRightBool = rightContainer.TryGet<BoolQuery>(out var rightBool);
1617

17-
//neither side is a bool, no special handling needed wrap in a bool must
18+
// neither side is a bool, no special handling needed wrap in a bool must
1819
if (!hasLeftBool && !hasRightBool)
20+
{
1921
return CreateMustContainer(new List<Query> { leftContainer, rightContainer });
22+
}
2023

2124
else if (TryHandleBoolsWithOnlyShouldClauses(leftContainer, rightContainer, leftBool, rightBool, out var query))
25+
{
2226
return query;
27+
}
2328

2429
else if (TryHandleUnmergableBools(leftContainer, rightContainer, leftBool, rightBool, out query))
30+
{
2531
return query;
32+
}
2633

27-
//neither side is unmergable so neither is a bool with should clauses
34+
// neither side is unmergable so neither is a bool with should clauses
2835

2936
var mustNotClauses = OrphanMustNots(leftContainer).EagerConcat(OrphanMustNots(rightContainer));
3037
var filterClauses = OrphanFilters(leftContainer).EagerConcat(OrphanFilters(rightContainer));
@@ -36,45 +43,92 @@ internal static Query CombineAsMust(this Query leftContainer, Query rightContain
3643
}
3744

3845
/// <summary>
39-
/// Handles cases where either side is a bool which indicates it can't be merged yet the other side is mergable.
46+
/// Handles cases where either side is a bool which indicates it can't be merged, yet the other side is mergable.
4047
/// A side is considered unmergable if its locked (has important metadata) or has should clauses.
41-
/// Instead of always wrapping these cases in another bool we merge to unmergable side into to others must clause therefor flattening the
42-
/// generated graph
48+
/// Instead of always wrapping these cases in another bool we merge the unmergable side into to others must clause therefore flattening the
49+
/// 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).
4350
/// </summary>
44-
private static bool TryHandleUnmergableBools(Query leftContainer, Query rightContainer, BoolQuery leftBool, BoolQuery rightBool, out Query query)
51+
private static bool TryHandleUnmergableBools(Query leftContainer, Query rightContainer, BoolQuery? leftBool, BoolQuery? rightBool, [NotNullWhen(true)] out Query? query)
4552
{
4653
query = null;
47-
var leftCantMergeAnd = leftBool != null && !leftBool.CanMergeAnd();
48-
var rightCantMergeAnd = rightBool != null && !rightBool.CanMergeAnd();
54+
55+
var leftCantMergeAnd = leftBool is not null && !leftBool.CanMergeAnd();
56+
var rightCantMergeAnd = rightBool is not null && !rightBool.CanMergeAnd();
57+
4958
if (!leftCantMergeAnd && !rightCantMergeAnd)
59+
{
5060
return false;
61+
}
5162

5263
if (leftCantMergeAnd && rightCantMergeAnd)
64+
{
5365
query = CreateMustContainer(leftContainer, rightContainer);
66+
}
5467

55-
//right can't merge but left can and is a bool so we add left to the must clause of right
68+
// right can't merge but left can and is a bool so we add left to the must clause of right
5669
else if (!leftCantMergeAnd && leftBool != null && rightCantMergeAnd)
5770
{
58-
leftBool.Must = leftBool.Must.AddIfNotNull(rightContainer).ToArray();
59-
query = leftContainer;
71+
if (rightContainer is null)
72+
{
73+
query = leftContainer;
74+
}
75+
else
76+
{
77+
// We create a copy here to avoid side-effects on a user provided bool query such as
78+
// adding a must clause where one did not previously exist.
79+
80+
var leftBoolCopy = new BoolQuery
81+
{
82+
Boost = leftBool.Boost,
83+
MinimumShouldMatch = leftBool.MinimumShouldMatch,
84+
QueryName = leftBool.QueryName,
85+
Should = leftBool.Should,
86+
Must = leftBool.Must.AddIfNotNull(rightContainer).ToArray(),
87+
MustNot = leftBool.MustNot,
88+
Filter = leftBool.Filter
89+
};
90+
91+
query = leftBoolCopy;
92+
}
6093
}
6194

62-
//right can't merge and left is not a bool, we forcefully create a wrapped must container
63-
else if (!leftCantMergeAnd && leftBool == null && rightCantMergeAnd)
95+
// right can't merge and left is not a bool, we forcefully create a wrapped must container
96+
else if (!leftCantMergeAnd && leftBool is null && rightCantMergeAnd)
97+
{
6498
query = CreateMustContainer(leftContainer, rightContainer);
99+
}
65100

66-
//left can't merge but right can and is a bool so we add left to the must clause of right
67-
else if (leftCantMergeAnd && !rightCantMergeAnd && rightBool != null)
101+
// left can't merge but right can and is a bool so we add left to the must clause of right
102+
else if (leftCantMergeAnd && !rightCantMergeAnd && rightBool is not null)
68103
{
69-
rightBool.Must = rightBool.Must.AddIfNotNull(leftContainer).ToArray();
70-
query = rightContainer;
104+
if (leftContainer is null)
105+
{
106+
query = rightContainer;
107+
}
108+
else
109+
{
110+
var rightBoolCopy = new BoolQuery
111+
{
112+
Boost = rightBool.Boost,
113+
MinimumShouldMatch = rightBool.MinimumShouldMatch,
114+
QueryName = rightBool.QueryName,
115+
Should = rightBool.Should,
116+
Must = rightBool.Must.AddIfNotNull(leftContainer).ToArray(),
117+
MustNot = rightBool.MustNot,
118+
Filter = rightBool.Filter
119+
};
120+
121+
query = rightBoolCopy;
122+
}
71123
}
72124

73125
//left can't merge and right is not a bool, we forcefully create a wrapped must container
74126
else if (leftCantMergeAnd && !rightCantMergeAnd && rightBool == null)
127+
{
75128
query = CreateMustContainer(new List<Query> { leftContainer, rightContainer });
129+
}
76130

77-
return query != null;
131+
return query is not null;
78132
}
79133

80134
/// <summary>
@@ -86,10 +140,14 @@ private static bool TryHandleUnmergableBools(Query leftContainer, Query rightCon
86140
private static bool TryHandleBoolsWithOnlyShouldClauses(Query leftContainer, Query rightContainer, BoolQuery leftBool, BoolQuery rightBool, out Query query)
87141
{
88142
query = null;
143+
89144
var leftHasOnlyShoulds = leftBool.HasOnlyShouldClauses();
90145
var rightHasOnlyShoulds = rightBool.HasOnlyShouldClauses();
146+
91147
if (!leftHasOnlyShoulds && !rightHasOnlyShoulds)
148+
{
92149
return false;
150+
}
93151

94152
if (leftContainer.HoldsOnlyShouldMusts && rightHasOnlyShoulds)
95153
{
@@ -106,25 +164,26 @@ private static bool TryHandleBoolsWithOnlyShouldClauses(Query leftContainer, Que
106164
query = CreateMustContainer(new List<Query> { leftContainer, rightContainer });
107165
query.HoldsOnlyShouldMusts = rightHasOnlyShoulds && leftHasOnlyShoulds;
108166
}
167+
109168
return true;
110169
}
111170

112171
private static Query CreateMustContainer(Query left, Query right) =>
113172
CreateMustContainer(new List<Query> { left, right });
114173

115174
private static Query CreateMustContainer(List<Query> mustClauses) =>
116-
new Query(new BoolQuery() { Must = mustClauses.ToListOrNullIfEmpty() });
175+
new(new BoolQuery() { Must = mustClauses.ToListOrNullIfEmpty() });
117176

118177
private static Query CreateMustContainer(
119178
List<Query> mustClauses,
120179
List<Query> mustNotClauses,
121180
List<Query> filters
122-
) => new Query(new BoolQuery
123-
{
124-
Must = mustClauses.ToListOrNullIfEmpty(),
125-
MustNot = mustNotClauses.ToListOrNullIfEmpty(),
126-
Filter = filters.ToListOrNullIfEmpty()
127-
});
181+
) => new(new BoolQuery
182+
{
183+
Must = mustClauses.ToListOrNullIfEmpty(),
184+
MustNot = mustNotClauses.ToListOrNullIfEmpty(),
185+
Filter = filters.ToListOrNullIfEmpty()
186+
});
128187

129188
private static bool CanMergeAnd(this BoolQuery boolQuery) =>
130189
boolQuery != null && !boolQuery.Locked && !boolQuery.Should.HasAny();

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

+19-16
Original file line numberDiff line numberDiff line change
@@ -11,52 +11,55 @@ namespace Elastic.Clients.Elasticsearch.QueryDsl;
1111
/// </summary>
1212
public abstract partial class SearchQuery
1313
{
14-
//always evaluate to false so that each side of && equation is evaluated
14+
// Always evaluate to false so that each side of && equation is evaluated
1515
public static bool operator false(SearchQuery _) => false;
1616

17-
//always evaluate to false so that each side of && equation is evaluated
17+
// Always evaluate to false so that each side of && equation is evaluated
1818
public static bool operator true(SearchQuery _) => false;
1919

20-
public static SearchQuery operator &(SearchQuery leftQuery, SearchQuery rightQuery) => Combine(leftQuery, rightQuery, (l, r) => l && r);
20+
public static SearchQuery? operator &(SearchQuery? leftQuery, SearchQuery? rightQuery) => Combine(leftQuery, rightQuery, (l, r) => l && r);
2121

22-
public static SearchQuery? operator |(SearchQuery leftQuery, SearchQuery rightQuery) => Combine(leftQuery, rightQuery, (l, r) => l || r);
22+
public static SearchQuery? operator |(SearchQuery? leftQuery, SearchQuery? rightQuery) => Combine(leftQuery, rightQuery, (l, r) => l || r);
2323

24-
public static SearchQuery? operator !(SearchQuery query) => query is null
24+
public static SearchQuery? operator !(SearchQuery? query) => query is null
2525
? null
2626
: new BoolQuery { MustNot = new Query[] { query } };
2727

28-
public static SearchQuery? operator +(SearchQuery query) => query is null
28+
public static SearchQuery? operator +(SearchQuery? query) => query is null
2929
? null
3030
: new BoolQuery { Filter = new Query[] { query } };
3131

32-
private static SearchQuery Combine(SearchQuery leftQuery, SearchQuery rightQuery, Func<Query?, Query?, Query> combine)
32+
private static SearchQuery? Combine(SearchQuery? leftQuery, SearchQuery? rightQuery, Func<Query?, Query?, Query> combine)
3333
{
34+
// If both sides are null, return null
3435
if (leftQuery is null && rightQuery is null)
36+
{
3537
return null;
38+
}
3639

40+
// If the left side is null, return the right side
3741
if (leftQuery is null)
42+
{
3843
return rightQuery;
44+
}
3945

46+
// If the right side is null, return the left side
4047
if (rightQuery is null)
48+
{
4149
return leftQuery;
50+
}
4251

4352
var container = combine(leftQuery, rightQuery);
4453

45-
if (container.TryGet<BoolQuery>(out var query))
54+
if (container.TryGet<BoolQuery>(out var boolQuery))
4655
{
47-
return new BoolQuery
48-
{
49-
Must = query.Must,
50-
MustNot = query.MustNot,
51-
Should = query.Should,
52-
Filter = query.Filter,
53-
};
56+
return boolQuery;
5457
}
5558

5659
throw new Exception("Unable to combine queries.");
5760
}
5861

59-
public static implicit operator Query?(SearchQuery query) => query is null ? null : new Query(query);
62+
public static implicit operator Query?(SearchQuery? query) => query is null ? null : new Query(query);
6063

6164
// We no longer (currently) support verbatim/strict query containers so this may be simplified to a direct abstract method in the future.
6265
internal void WrapInContainer(Query container) => InternalWrapInContainer(container);

‎tests/Tests/QueryDsl/BoolDsl/AndOperatorUsageTests.cs

+44
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System.Linq;
77
using Elastic.Clients.Elasticsearch.QueryDsl;
88
using Tests.Domain;
9+
using Xunit;
910

1011
namespace Tests.QueryDsl.BoolDsl;
1112

@@ -296,6 +297,49 @@ public void AndAssigningNamedBoolShouldQueries()
296297
((BoolQuery)s.Variant).QueryName == "name");
297298
}
298299

300+
[U]
301+
public void AndAssigningShouldNotModifyOriginalBoolQuery()
302+
{
303+
// Based on https://github.com/elastic/elasticsearch-net/issues/5076
304+
305+
// create a bool query with a filter that might be reused by the consuming code and expected not to change.
306+
Query filterQueryContainerExpectedToNotChange = +new TermQuery("do-not") { Value = "change" };
307+
308+
filterQueryContainerExpectedToNotChange.TryGet<BoolQuery>(out var boolQueryBefore).Should().BeTrue();
309+
310+
boolQueryBefore.Must.Should().BeNull();
311+
boolQueryBefore.Filter.Single().TryGet<TermQuery>(out var termQueryBefore).Should().BeTrue();
312+
termQueryBefore.Field.Should().Be("do-not");
313+
termQueryBefore.Value.TryGetString(out var termValue).Should().BeTrue();
314+
termValue.Should().Be("change");
315+
316+
var queryToExecute = new BoolQuery
317+
{
318+
Should = new Query[] { new MatchAllQuery() },
319+
Filter = new Query[] { new MatchAllQuery() } // if either of these are removed, it works as expected
320+
}
321+
&& filterQueryContainerExpectedToNotChange;
322+
323+
filterQueryContainerExpectedToNotChange.TryGet<BoolQuery>(out var boolQueryAfter).Should().BeTrue();
324+
boolQueryAfter.Must.Should().BeNull();
325+
boolQueryAfter.Filter.Single().TryGet<TermQuery>(out var termQueryAfter).Should().BeTrue();
326+
termQueryAfter.Field.Should().Be("do-not");
327+
termQueryAfter.Value.TryGetString(out termValue).Should().BeTrue();
328+
termValue.Should().Be("change");
329+
330+
queryToExecute.TryGet<BoolQuery>(out var boolToExecute).Should().BeTrue();
331+
boolToExecute.Filter.Should().HaveCount(1);
332+
boolToExecute.Must.Should().HaveCount(1);
333+
boolToExecute.Filter.Single().TryGet<TermQuery>(out var queryToExecuteTermQuery).Should().BeTrue();
334+
queryToExecuteTermQuery.Field.Should().Be("do-not");
335+
queryToExecuteTermQuery.Value.TryGetString(out termValue).Should().BeTrue();
336+
termValue.Should().Be("change");
337+
338+
boolToExecute.Must.Single().TryGet<BoolQuery>(out var mustClauseBool).Should().BeTrue();
339+
mustClauseBool.Filter.Should().HaveCount(1);
340+
mustClauseBool.Should.Should().HaveCount(1);
341+
}
342+
299343
private static void AssertLotsOfAnds(Query lotsOfAnds)
300344
{
301345
lotsOfAnds.Should().NotBeNull();

0 commit comments

Comments
 (0)