Add more not tests

elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_interpreter.rb

@@ -103,31 +103,28 @@ def filters_on_sub_fields?(expression)
         end
 
         def process_not_expression(bool_node, expression, field_path)
-          if expression.nil? || expression == {}
-            BooleanQuery::ALWAYS_FALSE_FILTER.merge_into(bool_node)
-            return
+          sub_filter = build_bool_hash do |inner_node|
+            process_filter_hash(inner_node, expression || {}, field_path)
           end
 
-          sub_filter = build_bool_hash do |inner_node|
-            process_filter_hash(inner_node, expression, field_path)
+          unless sub_filter
+            # Since an empty expression is treated as `true`, convert to `false` when negating.
+            BooleanQuery::ALWAYS_FALSE_FILTER.merge_into(bool_node)
+            return
           end
 
-          if sub_filter
-            # Prevent any negated filters from being unnecessarily double-negated by
-            # converting them to a positive filter (i.e., !!A == A).
-            if sub_filter[:bool].key?(:must_not)
-              # Pull clauses up to current bool_node to remove negation
-              sub_filter[:bool][:must_not].each do |negated_clause|
-                negated_clause[:bool].each { |k, v| bool_node[k].concat(v) }
-              end
+          # Prevent any negated filters from being unnecessarily double-negated by
+          # converting them to a positive filter (i.e., !!A == A).
+          if sub_filter[:bool].key?(:must_not)
+            # Pull clauses up to current bool_node to remove negation
+            sub_filter[:bool][:must_not].each do |negated_clause|
+              negated_clause[:bool].each { |k, v| bool_node[k].concat(v) }
             end
-
-            # Don't drop any other filters! Let's negate them now.
-            other_filters = sub_filter[:bool].except(:must_not)
-            bool_node[:must_not] << {bool: other_filters} unless other_filters.empty?
-          else
-            BooleanQuery::ALWAYS_FALSE_FILTER.merge_into(bool_node)
           end
+
+          # Don't drop any other filters! Let's negate them now.
+          other_filters = sub_filter[:bool].except(:must_not)
+          bool_node[:must_not] << {bool: other_filters} unless other_filters.empty?
         end
 
         # There are two cases for `any_satisfy`, each of which is handled differently:
@@ -349,7 +346,7 @@ def process_list_count_expression(bool_node, expression, field_path)
         def build_bool_hash(&block)
           bool_node = Hash.new { |h, k| h[k] = [] }.tap(&block)
 
-          # To threat "empty" filter predicates as `true` we need to return `nil` here.
+          # To treat "empty" filter predicates as `true` we need to return `nil` here.
           return nil if bool_node.empty?
 
           # According to https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-bool-query.html#bool-min-should-match,

elasticgraph-graphql/spec/acceptance/search_spec.rb

@@ -181,7 +181,7 @@ module ElasticGraph
           string_hash_of(widget1, :id, :amount_cents)
         ])
 
-        # Verify that we can filter on `DateTime` fields (and that `nil` DateTime filter values are pruned treating them as `true`)
+        # Verify that we can filter on `DateTime` fields (and that `nil` DateTime filter values are treated as `true`)
         widgets = list_widgets_with(:created_at,
           filter: {created_at: {gte: "2019-07-02T12:00:00Z", lt: nil}},
           order_by: [:created_at_ASC])
@@ -229,7 +229,7 @@ module ElasticGraph
           string_hash_of(widget2, :id, created_at: "2019-07-02T12:00:00.000Z")
         ])
 
-        # Verify that we can filter on `Date` fields (and that `nil` Date filter values are pruned treating them as `true`)
+        # Verify that we can filter on `Date` fields (and that `nil` Date filter values are treated as `true`)
         widgets = list_widgets_with(:created_on,
           filter: {created_on: {lte: "2019-07-02", gte: nil}},
           order_by: [:created_on_ASC])
@@ -320,13 +320,13 @@ module ElasticGraph
           string_hash_of(widget1, :id, :amount_cents)
         ])
 
-        # Test `not: {any_of: emptyPredicate}` results in no matching documents
+        # Test `not: {any_of: [emptyPredicate]}` results in no matching documents
         widgets = list_widgets_with(:amount_cents,
           filter: {not: {any_of: [{name: {equal_to_any_of: nil}}]}})
 
         expect(widgets).to eq []
 
-        # Test `not: {any_of: emptyPredicate && <non emptyPredicate>}` results in no matching documents
+        # Test `not: {any_of: [emptyPredicate && nonEmptyPredicate]}` results in no matching documents
         widgets = list_widgets_with(:amount_cents,
           filter: {not: {any_of: [{name: {equal_to_any_of: nil}}, {amount_cents: {gt: 150}}]}})
 
@@ -648,11 +648,11 @@ module ElasticGraph
           }}}})
           expect(results).to eq [{"id" => "t1"}, {"id" => "t2"}, {"id" => "t3"}]
 
-          # Verify `any_of: [{forbes_valuations: nil}]` returns all results.
+          # Verify `any_of: [emptyPredicate]` returns all results.
           results = query_teams_with(filter: {any_of: [{forbes_valuations: nil}]})
           expect(results).to eq [{"id" => "t1"}, {"id" => "t2"}, {"id" => "t3"}, {"id" => "t4"}]
 
-          # Verify `any_of: [{forbes_valuations: nil}]` returns all results.
+          # Verify `any_of: [emptyPredicate, nonEmptyPredicate]` returns all results.
           results = query_teams_with(filter: {any_of: [{forbes_valuations: nil}, {id: {equal_to_any_of: ["t3"]}}]})
           expect(results).to eq [{"id" => "t1"}, {"id" => "t2"}, {"id" => "t3"}, {"id" => "t4"}]
 
@@ -788,12 +788,12 @@ module ElasticGraph
 
           # Verify `all_of: [{not: null}]` works as expected.
           results = query_teams_with(filter: {seasons_nested: {all_of: [{all_of: nil}]}})
-          # All teams should be returned since the `nil` part of the filter expression is pruned.
+          # All teams should be returned since the `nil` part of the filter expression is treated as `true`.
           expect(results).to eq [{"id" => "t1"}, {"id" => "t2"}, {"id" => "t3"}, {"id" => "t4"}]
 
           # Verify `all_of: [{}]` works as expected.
           results = query_teams_with(filter: {seasons_nested: {all_of: [{}]}})
-          # All teams should be returned since the `nil` part of the filter expression is pruned.
+          # All teams should be returned since the `nil` part of the filter expression is treated as `true`.
           expect(results).to eq [{"id" => "t1"}, {"id" => "t2"}, {"id" => "t3"}, {"id" => "t4"}]
         end
 
@@ -891,12 +891,12 @@ def expect_error_from(filter, *error_snippets)
             filter: {options: {color: {equal_to_any_of: nil}}}
           )).to contain_exactly(expected_widget1, expected_widget2)
 
-          # not used with an empty predicate should result in an always false filter
+          # `{not: emptyPredicate}` should result in an always false filter
           expect(list_widgets_with_options_and_inventor(
             filter: {options: {color: {not: {equal_to_any_of: nil}}}}
           )).to eq []
 
-          # not set to 'nil' should result in an always false filter
+          # `{not: nil}` should result in an always false filter
           expect(list_widgets_with_options_and_inventor(
             filter: {options: {color: {not: nil}}}
           )).to eq []

elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb

@@ -283,7 +283,7 @@ def search_datastore(**options, &before_msearch)
             expect(results).to match_array(ids_of(team2))
           end
 
-          it "correctly prunes nil filters under `any_satisfy` treating them as `true`" do
+          it "correctly treats nil filters under `any_satisfy` treating as `true`" do
             results = search_with_filter("current_players_nested", "any_satisfy", {
               "name" => {"equal_to_any_of" => nil},
               "nicknames" => {"any_satisfy" => {"equal_to_any_of" => nil}}
@@ -898,7 +898,6 @@ def search_datastore(**options, &before_msearch)
 
         expect(search_with_filter("id", "equal_to_any_of", [])).to eq []
         expect(search_with_filter("id", "any_of", [])).to eq []
-        expect(search_with_freeform_filter({"not" => {"any_of" => []}})).to eq ids_of(widget1, widget2)
         expect(search_with_filter("id", "any_of", [{"any_of" => []}])).to eq []
 
         expect(search_with_filter("id", "equal_to_any_of", nil)).to eq ids_of(widget1, widget2)
@@ -913,8 +912,6 @@ def search_datastore(**options, &before_msearch)
         expect(search_with_freeform_filter({"id" => {}})).to eq ids_of(widget1, widget2)
         expect(search_with_freeform_filter({"any_of" => [{"id" => nil}]})).to eq ids_of(widget1, widget2)
         expect(search_with_freeform_filter({"any_of" => [{"id" => nil}, {"amount_cents" => {"lt" => 0}}]})).to eq ids_of(widget1, widget2)
-        expect(search_with_freeform_filter({"not" => {"any_of" => [{"id" => nil}]}})).to eq []
-        expect(search_with_freeform_filter({"not" => {"any_of" => [{"id" => nil}, {"amount_cents" => {"lt" => 1000}}]}})).to eq []
       end
 
       specify "`not: {any_of: []}` matches all documents, but `not: {any_of: [field: nil, ...]}` is treated as `false` matching no documents" do
@@ -925,10 +922,10 @@ def search_datastore(**options, &before_msearch)
         )
 
         expect(search_with_freeform_filter({"not" => {"any_of" => []}})).to eq ids_of(widget1, widget2)
+        expect(search_with_freeform_filter({"not" => {"not" => {"any_of" => []}}})).to eq []
 
-        # pending "`not` doesn't yet compose correctly with an `any_of: [{}, ...]`, so these currently fail"
         expect(search_with_freeform_filter({"not" => {"any_of" => [{"id" => nil}]}})).to eq []
-        expect(search_with_freeform_filter({"not" => {"any_of" => [{"id" => nil}, {"amount_cents" => {"lt" => 0}}]}})).to eq []
+        expect(search_with_freeform_filter({"not" => {"any_of" => [{"id" => nil}, {"amount_cents" => {"lt" => 1000}}]}})).to eq []
       end
 
       it "`equal_to_any_of:` with `nil` matches documents with null values or not null values" do
@@ -1177,12 +1174,12 @@ def search_datastore(**options, &before_msearch)
           expect(triple_nested_not).to match_array(ids_of(widget1, widget3))
         end
 
-        it "returns the standard always false filter when set to nil" do
+        it "matches no documents when set to `nil`" do
           index_into(
             graphql,
-            widget1 = build(:widget, amount_cents: 100),
-            widget2 = build(:widget, amount_cents: 205),
-            widget3 = build(:widget, amount_cents: 400)
+            build(:widget, amount_cents: 100),
+            build(:widget, amount_cents: 205),
+            build(:widget, amount_cents: 400)
           )
 
           inner_not_result1 = search_with_freeform_filter({"amount_cents" => {
@@ -1215,12 +1212,21 @@ def search_datastore(**options, &before_msearch)
             "amount_cents" => {"equal_to_any_of" => nil}
           }})
 
+          inner_not_result4 = search_with_freeform_filter({"amount_cents" => {
+            "not" => {}
+          }})
+
+          outer_not_result4 = search_with_freeform_filter({"not" => {
+            "amount_cents" => {}
+          }})
+
           expect(inner_not_result1).to eq(outer_not_result1).and eq []
           expect(inner_not_result2).to eq(outer_not_result2).and eq []
           expect(inner_not_result3).to eq(outer_not_result3).and eq []
+          expect(inner_not_result4).to eq(outer_not_result4).and eq []
         end
 
-        it "is pruned when set to nil inside `any_of` treating it as `true`" do
+        it "is treated as `true` when set to nil inside `any_of`" do
           index_into(
             graphql,
             widget1 = build(:widget, amount_cents: 100),
@@ -1254,7 +1260,6 @@ def search_datastore(**options, &before_msearch)
         end
       end
 
-
       def search_with_freeform_filter(filter, **options)
         ids_of(search_datastore(filter: filter, sort: [], **options).to_a)
       end

elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/sub_aggregations_spec.rb

@@ -193,7 +193,7 @@ class GraphQL
         }]
       end
 
-      it "prunes empty filters treating them as `true`" do
+      it "treats empty filters as `true`" do
         query = aggregation_query_of(name: "teams", sub_aggregations: [
           nested_sub_aggregation_of(path_in_index: ["current_players_nested"], query: sub_aggregation_query_of(name: "current_players_nested", filter: {
             "name" => {"equal_to_any_of" => nil}

elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/resolvers/ungrouped_sub_aggregation_shared_examples.rb

@@ -253,7 +253,7 @@ module Aggregation
           }]
         end
 
-        it "prunes an empty filter treating it as `true`" do
+        it "treats an empty filter treating as `true`" do
           aggs = {
             "target:seasons_nested" => {"doc_count" => 423, "meta" => outer_meta}
           }

elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/filtering_spec.rb

@@ -378,13 +378,13 @@ class GraphQL
           ]}})
         end
 
-        it "is pruned when `null` is passed treating it as `true`" do
+        it "is treated as `true` when `null` is passed" do
           query = new_query(filter: {"tags" => {"all_of" => nil}})
 
           expect(datastore_body_of(query)).to not_filter_datastore_at_all
         end
 
-        it "is pruned when `[]` is passed treating it as `true`" do
+        it "is treated as `true` when `[]` is passed" do
           query = new_query(filter: {"tags" => {"all_of" => []}})
 
           expect(datastore_body_of(query)).to not_filter_datastore_at_all
@@ -786,7 +786,7 @@ class GraphQL
             })
           end
 
-          it "prunes `count` filter predicates that have a `nil` or `{}` value treating it as `true`" do
+          it "treats `count` filter predicates that have a `nil` or `{}` value as `true`" do
             query = new_query(filter: {"past_names" => {LIST_COUNTS_FIELD => nil}})
             expect(datastore_body_of(query)).to not_filter_datastore_at_all
 
@@ -1343,7 +1343,7 @@ def expect_script_query_with_params(query, expected_params)
       end
 
       describe "behavior of empty/null filter values" do
-        it "prunes out filtering predicates that are empty no-ops on root fields" do
+        it "treats filtering predicates that are empty no-ops on root fields as `true`" do
           query = new_query(filter: {
             "age" => {"gt" => nil},
             "name" => {"equal_to_any_of" => ["Jane"]},
@@ -1354,7 +1354,7 @@ def expect_script_query_with_params(query, expected_params)
           expect(datastore_body_of(query)).to filter_datastore_with(terms: {"name" => ["Jane"]})
         end
 
-        it "prunes out filtering predicates that are empty no-ops on subfields" do
+        it "treats filtering predicates that are empty no-ops on subfields as `true`" do
           query = new_query(filter: {
             "bio" => {
               "age" => {"gt" => nil},
@@ -1485,6 +1485,14 @@ def expect_script_query_with_params(query, expected_params)
           expect(datastore_body_of(query)).to query_datastore_with({bool: {must_not: [always_false_condition]}})
         end
 
+        it "filters to a false condition when given `not: {not: {any_of: []}}` on a sub field" do
+          query = new_query(filter: {
+            "age" => {"not" => {"not" => {"any_of" => []}}}
+          })
+
+          expect(datastore_body_of(query)).to query_datastore_with(always_false_condition)
+        end
+
         # Note: the GraphQL schema does not allow `any_of: {}` (`any_of` is a list field). However, we're testing
         # it here for completeness--as a defense-in-depth measure, it's good for the filter interpreter to handle
         # whatever is thrown at it. Including these tests allows us to exercise an edge case in the code that

elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/search_index_expression_spec.rb

@@ -170,7 +170,7 @@ class GraphQL
               expect(parts).to eq []
             end
 
-            it "prunes `nil` when `equal_to_any_of` includes `nil` with other timestamps treating it as `true`" do
+            it "treats `nil` when `equal_to_any_of` includes `nil` with other timestamps as `true`" do
               parts = search_index_expression_parts_for({"created_at" => {"equal_to_any_of" => [
                 "2021-01-15T12:30:00Z",
                 nil,
@@ -325,7 +325,7 @@ class GraphQL
           end
 
           %w[equal_to_any_of gt gte lt lte any_of].each do |operator|
-            it "prunes a `nil` value for a `#{operator}` filter treating it as `true`" do
+            it "treats a `nil` value for a `#{operator}` filter as `true`" do
               parts = search_index_expression_parts_for({"created_at" => {operator => nil}})
 
               expect(parts).to target_all_widget_indices
@@ -364,13 +364,15 @@ class GraphQL
               expect(parts).to target_all_widget_indices
             end
 
-            it "excludes no indices when we have an `any_of: []` filter because that will match all results" do
+            # TODO: Change behaviour so no indicies are matched when given `anyOf => []`
+            it "excludes all indices when we have an `any_of: []` filter because that will match no results" do
               parts = search_index_expression_parts_for({"any_of" => []})
 
               expect(parts).to target_all_widget_indices
             end
 
-            it "excludes no indices when we have an `any_of: [{anyof: []}]` filter because that will match all results" do
+            # TODO: Change behaviour so no indicies are matched when given `anyOf => {anyOf => []}`
+            it "excludes all indices when we have an `any_of: [{anyof: []}]` filter because that will match no results" do
               parts = search_index_expression_parts_for({"any_of" => [{"any_of" => []}]})
 
               expect(parts).to target_all_widget_indices

elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/shard_routing_spec.rb

@@ -253,13 +253,15 @@ def shard_routing_for(route_with_field_paths, filter_or_filters)
         ]})).to search_all_shards
       end
 
-      it "searches all shards when we have an `any_of: []` filter because that will match all results" do
+      # TODO: Change behaviour so no shards are matched when given `anyOf => []`
+      it "searches no shards when we have an `any_of: []` filter because that will match no results" do
         expect(shard_routing_for(["name"], {
           "any_of" => []
         })).to search_all_shards
       end
 
-      it "searches all shards when we have an `any_of: [{anyof: []}]` filter because that will match all results" do
+      # TODO: Change behaviour so no shards are matched when given `anyOf => {anyOf => []}`
+      it "searches no shards when we have an `any_of: [{anyof: []}]` filter because that will match no results" do
         expect(shard_routing_for(["name"], {
           "any_of" => [{"any_of" => []}]
         })).to search_all_shards

elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/sub_aggregations_spec.rb

@@ -178,7 +178,7 @@ def self.grouped_field_from(agg_hash)
         })
       end
 
-      it "prunes empty filters treating them as `true`" do
+      it "treats empty filters treating as `true`" do
         query = new_query(aggregations: [aggregation_query_of(name: "teams", sub_aggregations: [
           nested_sub_aggregation_of(path_in_index: ["current_players_nested"], query: sub_aggregation_query_of(name: "current_players_nested", filter: {
             "name" => {"equal_to_any_of" => nil}