Skip to content

Commit c3efdb8

Browse files
MpdreamzMpdreamz
committed
fix #1983 incorrectly combining two bool should queries
When combining (x || y) && (x || y) we should not flatten to a single bool, this triggered a path not well tested. Added even more combinatorial query dsl tests
1 parent 5d05ea9 commit c3efdb8

File tree

6 files changed

+147
-44
lines changed

6 files changed

+147
-44
lines changed

‎src/Nest/QueryDsl/Abstractions/Query/BoolQueryExtensions.cs

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,6 @@ internal static QueryContainer CombineAsMust(this QueryContainer leftContainer,
1515
if ((leftBool?.Locked).GetValueOrDefault() || (rightBool?.Locked).GetValueOrDefault())
1616
return CreateMustContainer(new[] { leftContainer, rightContainer }, null, null);
1717

18-
if (leftBool.HasOnlyShouldClauses() && rightBool.HasOnlyShouldClauses())
19-
return new BoolQuery { Should = leftBool.Should.EagerConcat(rightBool.Should) };
20-
2118
if (leftBool.HasOnlyShouldClauses() || rightBool.HasOnlyShouldClauses())
2219
return CreateMustContainer(new[] { leftContainer, rightContainer }, null, null);
2320

@@ -51,18 +48,18 @@ internal static QueryContainer CombineAsShould(this QueryContainer leftContainer
5148

5249
private static bool HasOnlyShouldClauses(this IBoolQuery boolQuery) =>
5350
boolQuery != null && !boolQuery.Conditionless && (
54-
boolQuery.Should.HasAny()
55-
&& !boolQuery.Must.HasAny()
56-
&& !boolQuery.MustNot.HasAny()
57-
&& !boolQuery.Filter.HasAny()
51+
boolQuery.Should.HasAny()
52+
&& !boolQuery.Must.HasAny()
53+
&& !boolQuery.MustNot.HasAny()
54+
&& !boolQuery.Filter.HasAny()
5855
);
5956

6057
private static bool HasOnlyFilterClauses(this IBoolQuery boolQuery) =>
6158
boolQuery != null && !boolQuery.Conditionless && !boolQuery.Locked && (
62-
!boolQuery.Should.HasAny()
63-
&& !boolQuery.Must.HasAny()
64-
&& !boolQuery.MustNot.HasAny()
65-
&& boolQuery.Filter.HasAny()
59+
!boolQuery.Should.HasAny()
60+
&& !boolQuery.Must.HasAny()
61+
&& !boolQuery.MustNot.HasAny()
62+
&& boolQuery.Filter.HasAny()
6663
);
6764

6865
private static bool HasOnlyMustNotClauses(this IBoolQuery boolQuery) =>
@@ -75,8 +72,8 @@ private static bool HasOnlyMustNotClauses(this IBoolQuery boolQuery) =>
7572

7673
private static bool CanMergeShould(this IQueryContainer container) => container.Bool.CanMergeShould();
7774

78-
private static bool CanMergeShould(this IBoolQuery boolQuery) =>
79-
boolQuery == null || (!boolQuery.Locked
75+
private static bool CanMergeShould(this IBoolQuery boolQuery) =>
76+
boolQuery == null || (!boolQuery.Locked
8077
&& (boolQuery.HasOnlyShouldClauses() || boolQuery.HasOnlyMustNotClauses() || boolQuery.HasOnlyFilterClauses())
8178
);
8279

‎src/Tests/QueryDsl/BoolDsl/Operators/CombinatorialUsageTests.cs

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
using System.Linq;
2+
using FluentAssertions;
3+
using Nest;
4+
using Tests.Framework;
5+
6+
namespace Tests.QueryDsl.BoolDsl.Operators
7+
{
8+
public class CombinationUsageTests : OperatorUsageBase
9+
{
10+
[U] void DoesNotJoinTwoShouldsUsingAnd() => ReturnsBool(
11+
(Query || Query) && (Query || Query),
12+
q => (q.Query() || q.Query()) && (q.Query() || q.Query()),
13+
b =>
14+
{
15+
b.Must.Should().NotBeEmpty().And.HaveCount(2);
16+
b.Should.Should().BeNull();
17+
b.MustNot.Should().BeNull();
18+
b.Filter.Should().BeNull();
19+
20+
});
21+
22+
[U] void DoesJoinTwoShouldsUsingOr() => ReturnsBool(
23+
(Query || Query) || (Query || Query),
24+
q => (q.Query() || q.Query()) || (q.Query() || q.Query()),
25+
b =>
26+
{
27+
b.Should.Should().NotBeEmpty().And.HaveCount(4);
28+
b.Must.Should().BeNull();
29+
b.MustNot.Should().BeNull();
30+
b.Filter.Should().BeNull();
31+
});
32+
33+
[U] void DoesNotJoinTwoMustsUsingOr() => ReturnsBool(
34+
(Query && Query) || (Query && Query),
35+
q => (q.Query() && q.Query()) || (q.Query() && q.Query()),
36+
b =>
37+
{
38+
b.Should.Should().NotBeEmpty().And.HaveCount(2);
39+
b.Must.Should().BeNull();
40+
b.MustNot.Should().BeNull();
41+
b.Filter.Should().BeNull();
42+
});
43+
44+
[U] void DoesJoinTwoMustsUsingAnd() => ReturnsBool(
45+
(Query && Query) && (Query && Query),
46+
q => (q.Query() && q.Query()) && (q.Query() && q.Query()),
47+
b =>
48+
{
49+
b.Must.Should().NotBeEmpty().And.HaveCount(4);
50+
b.Should.Should().BeNull();
51+
b.MustNot.Should().BeNull();
52+
b.Filter.Should().BeNull();
53+
});
54+
55+
[U] void AndJoinsMustNot() => ReturnsBool(
56+
Query && !Query,
57+
q => q.Query() && !q.Query(),
58+
b =>
59+
{
60+
b.Must.Should().NotBeEmpty().And.HaveCount(1);
61+
b.MustNot.Should().NotBeEmpty().And.HaveCount(1);
62+
});
63+
64+
[U] void OrDoesNotJoinMustNot() => ReturnsBool(
65+
Query || !Query,
66+
q => q.Query() || !q.Query(),
67+
b =>
68+
{
69+
b.Should.Should().NotBeEmpty().And.HaveCount(2);
70+
});
71+
72+
[U] void OrDoesNotJoinFilter() => ReturnsBool(
73+
Query || !Query,
74+
q => q.Query() || +q.Query(),
75+
b =>
76+
{
77+
b.Should.Should().NotBeEmpty().And.HaveCount(2);
78+
b.Filter.Should().BeNull();
79+
});
80+
81+
[U] void AndJoinsFilter() => ReturnsBool(
82+
Query && +Query,
83+
q => q.Query() && +q.Query(),
84+
b =>
85+
{
86+
b.Must.Should().NotBeEmpty().And.HaveCount(1);
87+
b.Filter.Should().NotBeEmpty().And.HaveCount(1);
88+
});
89+
}
90+
}

‎src/Tests/QueryDsl/BoolDsl/Operators/OrOperatorUsageTests.cs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ [U] public void Or()
2424
b.MustNot.Should().BeNull();
2525
b.Filter.Should().BeNull();
2626
});
27-
27+
2828
ReturnsBool(Query || Query || ConditionlessQuery, q => q.Query() || q.Query() || q.ConditionlessQuery(), b =>
2929
{
3030
b.Should.Should().NotBeEmpty().And.HaveCount(2);
@@ -42,15 +42,15 @@ [U] public void Or()
4242
ReturnsSingleQuery(Query || NullQuery, q => q.Query() || q.NullQuery(),
4343
c => c.Term.Value.Should().NotBeNull());
4444

45-
ReturnsSingleQuery(NullQuery || Query, q=> q.NullQuery() || q.Query(),
45+
ReturnsSingleQuery(NullQuery || Query, q=> q.NullQuery() || q.Query(),
4646
c => c.Term.Value.Should().NotBeNull());
4747

4848
ReturnsSingleQuery(ConditionlessQuery || ConditionlessQuery || ConditionlessQuery || Query,
4949
q => q.ConditionlessQuery() || q.ConditionlessQuery() || q.ConditionlessQuery() || q.Query(),
5050
c => c.Term.Value.Should().NotBeNull());
5151

5252
ReturnsSingleQuery(
53-
NullQuery || NullQuery || ConditionlessQuery || Query,
53+
NullQuery || NullQuery || ConditionlessQuery || Query,
5454
q=>q.NullQuery() || q.NullQuery() || q.ConditionlessQuery() || q.Query(),
5555
c => c.Term.Value.Should().NotBeNull());
5656

@@ -76,15 +76,15 @@ [U] public void CombiningManyUsingAggregate()
7676

7777
[U] public void CombiningManyUsingForeachInitializingWithNull()
7878
{
79-
QueryContainer container = null;
79+
QueryContainer container = null;
8080
foreach(var i in Enumerable.Range(0, 100))
8181
container |= Query;
8282
LotsOfOrs(container);
8383
}
8484

8585
[U] public void CombiningManyUsingForeachInitializingWithDefault()
8686
{
87-
var container = new QueryContainer();
87+
var container = new QueryContainer();
8888
foreach(var i in Enumerable.Range(0, 100))
8989
container |= Query;
9090
LotsOfOrs(container);

‎src/Tests/QueryDsl/Compound/Bool/BoolDslComplexQueryUsageTests.cs

Lines changed: 40 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ public BoolDslComplexQueryUsageTests(ReadOnlyCluster cluster, EndpointUsage usag
2020
@bool = new
2121
{
2222
should = new object[] {
23+
//first bool
2324
new {
2425
@bool = new {
2526
must = new [] {
@@ -30,38 +31,49 @@ public BoolDslComplexQueryUsageTests(ReadOnlyCluster cluster, EndpointUsage usag
3031
},
3132
new {
3233
@bool = new {
33-
must = new object[] {
34+
must = new [] {
3435
new {
3536
@bool = new {
36-
should = new object [] {
37-
new {
38-
@bool = new {
39-
filter = new [] {
40-
new { term = new { x = new { value = "y" } } }
41-
}
42-
}
43-
},
37+
must = new object[] {
4438
new {
4539
@bool = new {
46-
filter = new [] {
47-
new { term = new { x = new { value = "y" } } }
40+
//complex nested bool
41+
should = new object[] {
42+
new {
43+
@bool = new {
44+
filter = new [] { new { term = new { x = new { value = "y" } } } }
45+
}
46+
},
47+
new {
48+
@bool = new {
49+
filter = new [] { new { term = new { x = new { value = "y" } } } }
50+
}
51+
},
52+
new {
53+
@bool = new {
54+
must_not = new [] {
55+
new { term = new { x = new { value = "y" } } },
56+
new { term = new { x = new { value = "y" } } }
57+
}
58+
}
59+
}
4860
}
4961
}
5062
},
63+
//simple nested or
5164
new {
5265
@bool = new {
53-
must_not = new [] {
66+
should = new [] {
67+
new { term = new { x = new { value = "y" } } },
5468
new { term = new { x = new { value = "y" } } },
5569
new { term = new { x = new { value = "y" } } }
5670
}
5771
}
58-
},
59-
new { term = new { x = new { value = "y" } } },
60-
new { term = new { x = new { value = "y" } } },
61-
new { term = new { x = new { value = "y" } } }
72+
}
6273
}
6374
}
6475
},
76+
//actual (locked) locked query
6577
base.QueryJson,
6678
}
6779
}
@@ -96,17 +108,17 @@ protected override QueryContainer QueryFluent(QueryContainerDescriptor<Project>
96108
//all conditionless bool
97109
&& (q.NullQuery() || +q.ConditionlessQuery() || !q.ConditionlessQuery())
98110
// actual bool query
99-
&& (base.QueryFluent(q)));
100-
101-
//hide
111+
&& (base.QueryFluent(q)));
112+
113+
//hide
102114
[U]
103115
protected void AsssertShape()
104116
{
105117
this.AssertShape(this.QueryInitializer);
106118
//this.AssertShape(this.QueryFluent(new QueryContainerDescriptor<Project>()));
107-
}
108-
109-
//hide
119+
}
120+
121+
//hide
110122
private void AssertShape(IQueryContainer container)
111123
{
112124
//top level bool
@@ -136,11 +148,14 @@ private void AssertShape(IQueryContainer container)
136148
var complexBool = (secondBool.Must.First() as IQueryContainer)?.Bool;
137149
complexBool.Should().NotBeNull();
138150
//complex bool is 3 ors and the next simple nested or bool query also has 3 should clauses
139-
//this can be rewritten to one boolquery with 6 clauses
140-
complexBool.Should.Should().HaveCount(6);
151+
complexBool.Must.Should().HaveCount(2);
152+
153+
var complexNestedBool = (complexBool.Must.First() as IQueryContainer)?.Bool;
154+
complexNestedBool.Should().NotBeNull();
155+
complexNestedBool.Should.Should().HaveCount(3);
141156

142157
//inner must nots
143-
var mustNotsBool = (complexBool.Should.Cast<IQueryContainer>().FirstOrDefault(q => q.Bool != null && q.Bool.MustNot != null))?.Bool;
158+
var mustNotsBool = (complexNestedBool.Should.Cast<IQueryContainer>().FirstOrDefault(q => q.Bool != null && q.Bool.MustNot != null))?.Bool;
144159
mustNotsBool.Should().NotBeNull();
145160
mustNotsBool.MustNot.Should().HaveCount(2); //one of the three must nots was conditionless
146161
}

‎src/Tests/Tests.csproj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,7 @@
545545
<Compile Include="ClientConcepts\HighLevel\Inference\IndicesPaths.doc.cs" />
546546
<Compile Include="ClientConcepts\HighLevel\Inference\PropertyInference.doc.cs" />
547547
<Compile Include="CommonOptions\DistanceUnit\DistanceUnits.doc.cs" />
548+
<Compile Include="QueryDsl\BoolDsl\Operators\CombinatorialUsageTests.cs" />
548549
<Compile Include="QueryDsl\QueryDslIntegrationTestsBase.cs" />
549550
<Compile Include="QueryDsl\TermLevel\Terms\TermsListQueryUsageTests.cs" />
550551
<Compile Include="Search\Request\ProfileUsageTests.cs" />

‎src/Tests/tests.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
# mode either u (unit test), i (integration test) or m (mixed mode)
2-
mode: m
2+
mode: u
33
# the elasticsearch version that should be started
44
elasticsearch_version: 2.3.0
55
# whether we want to forcefully reseed on the node, if you are starting the tests with a node already running

0 commit comments

Comments
 (0)