Skip to content

Commit 3ff7f57

Browse files
gmarzgmarz
committed
Fix compound query conditionless logic
A compound query should only be conditionless when any of its inner queries are not writable. Previously we were only checking if the inner queries were also not conditionless. Closes #2086
1 parent bee0d77 commit 3ff7f57

File tree

10 files changed

+142
-60
lines changed

10 files changed

+142
-60
lines changed

‎src/Nest/CommonAbstractions/Extensions/Extensions.cs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@ namespace Nest
1515
{
1616
internal static class Extensions
1717
{
18+
internal static bool NotWritable(this QueryContainer q) => q == null || !q.IsWritable;
19+
20+
internal static bool NotWritable(this IEnumerable<QueryContainer> qs) => qs == null || qs.All(q => q.NotWritable());
21+
1822
internal static TReturn InvokeOrDefault<T, TReturn>(this Func<T, TReturn> func, T @default)
1923
where T : class, TReturn where TReturn : class =>
2024
func?.Invoke(@default) ?? @default;

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

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -95,21 +95,8 @@ public class BoolQuery : QueryBase, IBoolQuery
9595
internal override void InternalWrapInContainer(IQueryContainer c) => c.Bool = this;
9696

9797
protected override bool Conditionless => IsConditionless(this);
98-
internal static bool IsConditionless(IBoolQuery q)
99-
{
100-
var musts = q.Must == null || q.Must.All(qq => qq.IsConditionless());
101-
if (!musts) return false;
102-
103-
var shoulds = q.Should == null || q.Should.All(qq => qq.IsConditionless());
104-
if (!shoulds) return false;
105-
106-
var filters = q.Filter == null || q.Filter.All(qq => qq.IsConditionless());
107-
if (!filters) return false;
108-
109-
var mustNots = q.MustNot == null || q.MustNot.All(qq => qq.IsConditionless());
110-
111-
return mustNots;
112-
}
98+
internal static bool IsConditionless(IBoolQuery q) =>
99+
q.Must.NotWritable() && q.MustNot.NotWritable() && q.Should.NotWritable() && q.Filter.NotWritable();
113100
}
114101

115102
public class BoolQueryDescriptor<T>

‎src/Nest/QueryDsl/Compound/Boosting/BoostingQuery.cs‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,10 @@ public class BoostingQuery : QueryBase, IBoostingQuery
2727
public double? NegativeBoost { get; set; }
2828

2929
internal override void InternalWrapInContainer(IQueryContainer c) => c.Boosting = this;
30-
internal static bool IsConditionless(IBoostingQuery q) =>
31-
q.NegativeQuery.IsConditionless() && q.PositiveQuery.IsConditionless();
30+
internal static bool IsConditionless(IBoostingQuery q) => q.NegativeQuery.NotWritable() && q.PositiveQuery.NotWritable();
3231
}
3332

34-
public class BoostingQueryDescriptor<T>
33+
public class BoostingQueryDescriptor<T>
3534
: QueryDescriptorBase<BoostingQueryDescriptor<T>, IBoostingQuery>
3635
, IBoostingQuery where T : class
3736
{
@@ -42,10 +41,10 @@ public class BoostingQueryDescriptor<T>
4241

4342
public BoostingQueryDescriptor<T> NegativeBoost(double? boost) => Assign(a => a.NegativeBoost = boost);
4443

45-
public BoostingQueryDescriptor<T> Positive(Func<QueryContainerDescriptor<T>, QueryContainer> selector) =>
44+
public BoostingQueryDescriptor<T> Positive(Func<QueryContainerDescriptor<T>, QueryContainer> selector) =>
4645
Assign(a => a.PositiveQuery = selector?.Invoke(new QueryContainerDescriptor<T>()));
4746

48-
public BoostingQueryDescriptor<T> Negative(Func<QueryContainerDescriptor<T>, QueryContainer> selector) =>
47+
public BoostingQueryDescriptor<T> Negative(Func<QueryContainerDescriptor<T>, QueryContainer> selector) =>
4948
Assign(a => a.NegativeQuery = selector?.Invoke(new QueryContainerDescriptor<T>()));
5049
}
5150
}

‎src/Nest/QueryDsl/Compound/ConstantScore/ConstantScoreQuery.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,17 +21,17 @@ public class ConstantScoreQuery : QueryBase, IConstantScoreQuery
2121
public QueryContainer Filter { get; set; }
2222

2323
internal override void InternalWrapInContainer(IQueryContainer c) => c.ConstantScore = this;
24-
internal static bool IsConditionless(IConstantScoreQuery q) => q.Filter.IsConditionless();
24+
internal static bool IsConditionless(IConstantScoreQuery q) => q.Filter.NotWritable();
2525
}
2626

27-
public class ConstantScoreQueryDescriptor<T>
27+
public class ConstantScoreQueryDescriptor<T>
2828
: QueryDescriptorBase<ConstantScoreQueryDescriptor<T>, IConstantScoreQuery>
2929
, IConstantScoreQuery where T : class
3030
{
3131
protected override bool Conditionless => ConstantScoreQuery.IsConditionless(this);
3232
QueryContainer IConstantScoreQuery.Filter { get; set; }
3333

34-
public ConstantScoreQueryDescriptor<T> Filter(Func<QueryContainerDescriptor<T>, QueryContainer> selector) =>
34+
public ConstantScoreQueryDescriptor<T> Filter(Func<QueryContainerDescriptor<T>, QueryContainer> selector) =>
3535
Assign(a => a.Filter = selector?.Invoke(new QueryContainerDescriptor<T>()));
3636
}
3737
}

‎src/Nest/QueryDsl/Compound/Dismax/DismaxQuery.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,21 +23,21 @@ public class DisMaxQuery : QueryBase, IDisMaxQuery
2323
public IEnumerable<QueryContainer> Queries { get; set; }
2424

2525
internal override void InternalWrapInContainer(IQueryContainer c) => c.DisMax = this;
26-
internal static bool IsConditionless(IDisMaxQuery q) => !q.Queries.HasAny() || q.Queries.All(qq => qq.IsConditionless);
26+
internal static bool IsConditionless(IDisMaxQuery q) => q.Queries.NotWritable();
2727
}
2828

29-
public class DisMaxQueryDescriptor<T>
29+
public class DisMaxQueryDescriptor<T>
3030
: QueryDescriptorBase<DisMaxQueryDescriptor<T>, IDisMaxQuery>
3131
, IDisMaxQuery where T : class
3232
{
3333
protected override bool Conditionless => DisMaxQuery.IsConditionless(this);
3434
double? IDisMaxQuery.TieBreaker { get; set; }
3535
IEnumerable<QueryContainer> IDisMaxQuery.Queries { get; set; }
3636

37-
public DisMaxQueryDescriptor<T> Queries(params Func<QueryContainerDescriptor<T>, QueryContainer>[] querySelectors) =>
37+
public DisMaxQueryDescriptor<T> Queries(params Func<QueryContainerDescriptor<T>, QueryContainer>[] querySelectors) =>
3838
Assign(a => a.Queries = querySelectors.Select(q=>q?.Invoke(new QueryContainerDescriptor<T>())).Where(q => q != null).ToListOrNullIfEmpty());
3939

40-
public DisMaxQueryDescriptor<T> Queries(IEnumerable<Func<QueryContainerDescriptor<T>, QueryContainer>> querySelectors) =>
40+
public DisMaxQueryDescriptor<T> Queries(IEnumerable<Func<QueryContainerDescriptor<T>, QueryContainer>> querySelectors) =>
4141
Assign(a => a.Queries = querySelectors.Select(q=>q?.Invoke(new QueryContainerDescriptor<T>())).Where(q => q != null).ToListOrNullIfEmpty());
4242

4343
public DisMaxQueryDescriptor<T> Queries(params QueryContainer[] queries) => Assign(a => a.Queries = queries.Where(q => q != null).ToListOrNullIfEmpty());

‎src/Nest/QueryDsl/Compound/Indices/IndicesQuery.cs‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,23 +36,23 @@ public class IndicesQuery : QueryBase, IIndicesQuery
3636
public Indices Indices { get; set; }
3737

3838
internal override void InternalWrapInContainer(IQueryContainer c) => c.Indices = this;
39-
internal static bool IsConditionless(IIndicesQuery q) =>
40-
q.Indices == null || (q.NoMatchQuery.IsConditionless() && q.Query.IsConditionless());
39+
internal static bool IsConditionless(IIndicesQuery q) =>
40+
q.Indices == null || q.NoMatchQuery.NotWritable() && q.Query.NotWritable();
4141
}
4242

43-
public class IndicesQueryDescriptor<T>
44-
: QueryDescriptorBase<IndicesQueryDescriptor<T>, IIndicesQuery>
43+
public class IndicesQueryDescriptor<T>
44+
: QueryDescriptorBase<IndicesQueryDescriptor<T>, IIndicesQuery>
4545
, IIndicesQuery where T : class
4646
{
4747
protected override bool Conditionless => IndicesQuery.IsConditionless(this);
4848
QueryContainer IIndicesQuery.Query { get; set; }
4949
QueryContainer IIndicesQuery.NoMatchQuery { get; set; }
5050
Indices IIndicesQuery.Indices { get; set; }
5151

52-
public IndicesQueryDescriptor<T> Query(Func<QueryContainerDescriptor<T>, QueryContainer> selector) =>
52+
public IndicesQueryDescriptor<T> Query(Func<QueryContainerDescriptor<T>, QueryContainer> selector) =>
5353
Assign(a => a.Query = selector?.Invoke(new QueryContainerDescriptor<T>()));
5454

55-
public IndicesQueryDescriptor<T> Query<TOther>(Func<QueryContainerDescriptor<TOther>, QueryContainer> selector) where TOther : class =>
55+
public IndicesQueryDescriptor<T> Query<TOther>(Func<QueryContainerDescriptor<TOther>, QueryContainer> selector) where TOther : class =>
5656
Assign(a => a.Query = selector?.Invoke(new QueryContainerDescriptor<TOther>()));
5757

5858
public IndicesQueryDescriptor<T> NoMatchQuery(NoMatchShortcut shortcut) =>
@@ -61,7 +61,7 @@ public IndicesQueryDescriptor<T> NoMatchQuery(NoMatchShortcut shortcut) =>
6161
public IndicesQueryDescriptor<T> NoMatchQuery(Func<QueryContainerDescriptor<T>, QueryContainer> selector) =>
6262
Assign(a => a.NoMatchQuery = selector?.Invoke(new QueryContainerDescriptor<T>()));
6363

64-
public IndicesQueryDescriptor<T> NoMatchQuery<TOther>(Func<QueryContainerDescriptor<TOther>, QueryContainer> selector) where TOther : class =>
64+
public IndicesQueryDescriptor<T> NoMatchQuery<TOther>(Func<QueryContainerDescriptor<TOther>, QueryContainer> selector) where TOther : class =>
6565
Assign(a => a.NoMatchQuery = selector?.Invoke(new QueryContainerDescriptor<TOther>()));
6666

6767
public IndicesQueryDescriptor<T> Indices(Indices indices) => Assign(a => a.Indices = indices);

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22
using Nest;
33
using Tests.Framework.Integration;
44
using Tests.Framework.MockData;
5+
using System;
6+
using Tests.Framework;
7+
using FluentAssertions;
58

69
namespace Tests.QueryDsl.Compound.Bool
710
{
@@ -75,5 +78,21 @@ protected override QueryContainer QueryFluent(QueryContainerDescriptor<Project>
7578
q.Filter = new [] { ConditionlessQuery };
7679
},
7780
};
81+
82+
[U]
83+
public void NullQueryDoesNotCauseANullReferenceException()
84+
{
85+
Action query = () => this.Client.Search<Project>(s => s
86+
.Query(q => q
87+
.Bool(b => b
88+
.Filter(f => f
89+
.Term(t => t.Name, null)
90+
)
91+
)
92+
)
93+
);
94+
95+
query.ShouldNotThrow();
96+
}
7897
}
7998
}

‎src/Tests/QueryDsl/Compound/Dismax/DismaxQueryUsageTests.cs‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22
using Nest;
33
using Tests.Framework.Integration;
44
using Tests.Framework.MockData;
5+
using Tests.Framework;
6+
using System;
7+
using FluentAssertions;
58

69
#pragma warning disable 618 //Testing an obsolete method
710

@@ -53,5 +56,21 @@ protected override QueryContainer QueryFluent(QueryContainerDescriptor<Project>
5356
q => q.Queries = Enumerable.Empty<QueryContainer>(),
5457
q => q.Queries = new [] { ConditionlessQuery },
5558
};
59+
60+
[U]
61+
public void NullQueryDoesNotCauseANullReferenceException()
62+
{
63+
Action query = () => this.Client.Search<Project>(s => s
64+
.Query(q => q
65+
.DisMax(dm => dm
66+
.Queries(
67+
dmq => dmq.Term(t => t.Name, null)
68+
)
69+
)
70+
)
71+
);
72+
73+
query.ShouldNotThrow();
74+
}
5675
}
5776
}

‎src/Tests/QueryDsl/QueryDslUsageTestsBase.cs‎

Lines changed: 1 addition & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -79,31 +79,6 @@ [U] public void ConditionlessWhenExpectedToBe()
7979
((IQueryContainer)this.QueryInitializer).IsConditionless.Should().BeFalse();
8080
}
8181

82-
[U] public void NullQueryDoesNotCauseANullReferenceException()
83-
{
84-
Action query = () => this.Client.Search<Project>(s => s
85-
.Query(q => q
86-
.Bool(b => b
87-
.Filter(f => f
88-
.Term(t => t.Name, null)
89-
)
90-
)
91-
)
92-
);
93-
94-
query.ShouldNotThrow();
95-
96-
query = () => this.Client.Search<Project>(s => s
97-
.Query(q => q
98-
.DisMax(dm => dm
99-
.Queries(
100-
dmq => dmq.Term(t => t.Name, null)
101-
)
102-
)
103-
)
104-
);
105-
106-
query.ShouldNotThrow();
107-
}
82+
private void IsConditionless(IQueryContainer q, bool be) => q.IsConditionless.Should().Be(be);
10883
}
10984
}

‎src/Tests/QueryDsl/Verbatim/VerbatimAndStrictQueryUsageTests.cs‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,85 @@ protected override QueryContainer QueryFluent(QueryContainerDescriptor<Project>
137137
);
138138
}
139139

140+
public class CompoundVerbatimInnerQueryUsageTests : QueryDslUsageTestsBase
141+
{
142+
public CompoundVerbatimInnerQueryUsageTests(ReadOnlyCluster cluster, EndpointUsage usage) : base(cluster, usage) { }
143+
144+
protected override bool SupportsDeserialization => false;
145+
146+
protected override object QueryJson => new
147+
{
148+
@bool = new
149+
{
150+
filter = new []
151+
{
152+
new
153+
{
154+
@bool = new
155+
{
156+
must = new []
157+
{
158+
new
159+
{
160+
exists = new
161+
{
162+
field = "numberOfCommits"
163+
}
164+
}
165+
},
166+
must_not = new []
167+
{
168+
new
169+
{
170+
term = new
171+
{
172+
name = new
173+
{
174+
value = ""
175+
}
176+
}
177+
}
178+
}
179+
}
180+
}
181+
}
182+
}
183+
};
184+
185+
186+
187+
protected override QueryContainer QueryInitializer => new BoolQuery
188+
{
189+
Filter = new QueryContainer[] {
190+
!new TermQuery
191+
{
192+
IsVerbatim = true,
193+
Field = "name",
194+
Value = ""
195+
} &&
196+
new ExistsQuery
197+
{
198+
Field = "numberOfCommits"
199+
}
200+
}
201+
};
202+
203+
204+
protected override QueryContainer QueryFluent(QueryContainerDescriptor<Project> q) => q
205+
.Bool(b => b
206+
.Filter(f => !f
207+
.Term(t => t
208+
.Verbatim()
209+
.Field(p => p.Name)
210+
.Value("")
211+
) && f
212+
.Exists(e => e
213+
.Field(p => p.NumberOfCommits)
214+
)
215+
)
216+
);
217+
}
218+
140219
public class StrictQueryUsageTests
141220
{
142221
[U]

0 commit comments

Comments
 (0)