elasticsearch-net

elasticsearch-net copied to clipboard

NEST/Elasticsearch.Net: 7.9.0, but I fear this has been since 2.4.6

Elasticsearch version: All, it is an issue in the driver

Description of the problem including expected versus actual behavior: A query container expected not to be modified, is modified when the query is executed. It is probably related to #2461 #2235

Steps to reproduce:

[TestClass]
public class UnitTest1
{
    private static readonly IElasticClient Client = new ElasticClient(new ConnectionSettings(new SingleNodeConnectionPool(new Uri("http://localhost:9200")), new InMemoryConnection()));
    private static readonly QueryContainer FilterQueryContainerExpectedToNotChange = +new TermQuery { Field = "don't", Value = "change" };
    [TestMethod]
    public void TestMethod1()
    {
        // sanity check
        ITermQuery termFilterBefore = ((IQueryContainer)((IQueryContainer) FilterQueryContainerExpectedToNotChange).Bool.Filter.First()).Term;
        Assert.IsNotNull(termFilterBefore);
        Assert.AreEqual(termFilterBefore.Field, "don't");
        Assert.AreEqual(termFilterBefore.Value, "change");
        
        IEnumerable<QueryContainer> mustBefore = ((IQueryContainer) FilterQueryContainerExpectedToNotChange).Bool.Must;
        Assert.IsNull(mustBefore); // no must before, like we expect

        var queryToExecute = new BoolQuery
                             {
                                 Should = new[] {new QueryContainer(new MatchAllQuery())}, // if either of these are removed, it works as expected
                                 Filter = new[] {new QueryContainer(new MatchAllQuery())}, // if either of these are removed, it works as expected
                             }
                            && FilterQueryContainerExpectedToNotChange;
        // execute query
        var request = new SearchDescriptor<Tuple<int>>().Index("foo").Query(q => queryToExecute);
        var response = Client.Search<Tuple<int>>(request);
        var mustAfter = ((IQueryContainer) FilterQueryContainerExpectedToNotChange).Bool.Must;
        // surely there should be no must after, right?
        Assert.IsNull(mustAfter);
    }
}

Expected behavior FilterQueryContainerExpectedToNotChange should not change

For reference, the generated query is

{
  "query": {
    "bool": {
      "filter": [{
        "term": {
          "don't": {
            "value": "change"
          }
        }
      }],
      "must": [{
        "bool": {
          "filter": [{
            "match_all": {}
          }],
          "should": [{
            "match_all": {}
          }]
        }
      }]
    }
  }
}

It looks like in combining the bool query with FilterQueryContainerExpectedToNotChange using &&, there are scenarios where one of the queries to combine will be modified. In this particular case, this line looks to be responsible

https://github.com/elastic/elasticsearch-net/blob/2ca859203f9898278d438d6ad001799f91065cc1/src/Nest/QueryDsl/Abstractions/Query/BoolQueryAndExtensions.cs#L64-L69

The question I think though is whether this should be considered a bug. On one hand, I can see the behaviour as being surprising. On another, I can see the modification avoids allocating a new bool query into which the queries are combined.

I've decided not to change this in 7.x since some consumers may depend on the current behaviour. However, for 8.x, we are only just reintroducing the operator support, and I've opted to avoid the side effect by updating the combination logic in #7615. This adds an extra allocation, but that trade-off is acceptable in this case.