diff --git a/.editorconfig b/.editorconfig index a391533f562..d9c44dbd523 100644 --- a/.editorconfig +++ b/.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 diff --git a/exclusion.dic b/exclusion.dic index 7262d2bd690..6fbe72adbdd 100644 --- a/exclusion.dic +++ b/exclusion.dic @@ -8,4 +8,6 @@ enum trippable geotile yaml -rethrottle \ No newline at end of file +rethrottle +mergable +unmergable \ No newline at end of file diff --git a/src/Elastic.Clients.Elasticsearch/Types/QueryDsl/BoolQueryAndExtensions.cs b/src/Elastic.Clients.Elasticsearch/Types/QueryDsl/BoolQueryAndExtensions.cs index b3eacd47ad2..272f72428d0 100644 --- a/src/Elastic.Clients.Elasticsearch/Types/QueryDsl/BoolQueryAndExtensions.cs +++ b/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(out var leftBool); var hasRightBool = rightContainer.TryGet(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 { 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 } /// - /// 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). /// - 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 { leftContainer, rightContainer }); + } - return query != null; + return query is not null; } /// @@ -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,6 +164,7 @@ private static bool TryHandleBoolsWithOnlyShouldClauses(Query leftContainer, Que query = CreateMustContainer(new List { leftContainer, rightContainer }); query.HoldsOnlyShouldMusts = rightHasOnlyShoulds && leftHasOnlyShoulds; } + return true; } @@ -113,18 +172,18 @@ private static Query CreateMustContainer(Query left, Query right) => CreateMustContainer(new List { left, right }); private static Query CreateMustContainer(List mustClauses) => - new Query(new BoolQuery() { Must = mustClauses.ToListOrNullIfEmpty() }); + new(new BoolQuery() { Must = mustClauses.ToListOrNullIfEmpty() }); private static Query CreateMustContainer( List mustClauses, List mustNotClauses, List 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(); diff --git a/src/Elastic.Clients.Elasticsearch/Types/QueryDsl/SearchQuery.cs b/src/Elastic.Clients.Elasticsearch/Types/QueryDsl/SearchQuery.cs index 55ce44031ba..9ea7350f0b8 100644 --- a/src/Elastic.Clients.Elasticsearch/Types/QueryDsl/SearchQuery.cs +++ b/src/Elastic.Clients.Elasticsearch/Types/QueryDsl/SearchQuery.cs @@ -11,52 +11,55 @@ namespace Elastic.Clients.Elasticsearch.QueryDsl; /// 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 combine) + private static SearchQuery? Combine(SearchQuery? leftQuery, SearchQuery? rightQuery, Func 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(out var query)) + if (container.TryGet(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); diff --git a/tests/Tests/QueryDsl/BoolDsl/AndOperatorUsageTests.cs b/tests/Tests/QueryDsl/BoolDsl/AndOperatorUsageTests.cs index dd03c4701e3..62803b6b896 100644 --- a/tests/Tests/QueryDsl/BoolDsl/AndOperatorUsageTests.cs +++ b/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(out var boolQueryBefore).Should().BeTrue(); + + boolQueryBefore.Must.Should().BeNull(); + boolQueryBefore.Filter.Single().TryGet(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(out var boolQueryAfter).Should().BeTrue(); + boolQueryAfter.Must.Should().BeNull(); + boolQueryAfter.Filter.Single().TryGet(out var termQueryAfter).Should().BeTrue(); + termQueryAfter.Field.Should().Be("do-not"); + termQueryAfter.Value.TryGetString(out termValue).Should().BeTrue(); + termValue.Should().Be("change"); + + queryToExecute.TryGet(out var boolToExecute).Should().BeTrue(); + boolToExecute.Filter.Should().HaveCount(1); + boolToExecute.Must.Should().HaveCount(1); + boolToExecute.Filter.Single().TryGet(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(out var mustClauseBool).Should().BeTrue(); + mustClauseBool.Filter.Should().HaveCount(1); + mustClauseBool.Should.Should().HaveCount(1); + } + private static void AssertLotsOfAnds(Query lotsOfAnds) { lotsOfAnds.Should().NotBeNull();