Fix how not works

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

@@ -103,26 +103,31 @@ def filters_on_sub_fields?(expression)
         end
 
         def process_not_expression(bool_node, expression, field_path)
-          return if expression.nil? || expression == {}
+          if expression.nil? || expression == {}
+            BooleanQuery::ALWAYS_FALSE_FILTER.merge_into(bool_node)
+            return
+          end
 
           sub_filter = build_bool_hash do |inner_node|
             process_filter_hash(inner_node, expression, field_path)
           end
 
-          return unless 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) }
+          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
             end
-          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?
+            # 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
         end
 
         # There are two cases for `any_satisfy`, each of which is handled differently:

elasticgraph-graphql/spec/acceptance/search_spec.rb

@@ -310,6 +310,28 @@ module ElasticGraph
           string_hash_of(widget3, :id, :amount_cents)
         ])
 
+        # Test `not: {any_of: []}` results in all matching documents
+        widgets = list_widgets_with(:amount_cents,
+          filter: {not: {any_of: []}})
+
+        expect(widgets).to match([
+          string_hash_of(widget3, :id, :amount_cents),
+          string_hash_of(widget2, :id, :amount_cents),
+          string_hash_of(widget1, :id, :amount_cents)
+        ])
+
+        # 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
+        widgets = list_widgets_with(:amount_cents,
+          filter: {not: {any_of: [{name: {equal_to_any_of: nil}}, {amount_cents: {gt: 150}}]}})
+
+        expect(widgets).to eq []
+
         # Test `equal_to_any_of` with `[nil, other_value]`
         widgets = list_widgets_with(:amount_cents,
           filter: {name: {equal_to_any_of: [nil, "thing2", "", " ", "\n"]}}, # empty strings should be ignored.,
@@ -350,17 +372,13 @@ module ElasticGraph
           string_hash_of(widget3, :id, :amount_cents)
         ])
 
-        # The negation of an ignored filter is an ignored filter: `{not: {equal_to_any_of: nil}}`
-        # evaluates to {not: {}} which will be ignored, therefore the filter will match all documents.
+        # The negation of an empty predicate is an always false filter. `{not: {equal_to_any_of: nil}}`
+        # evaluates to `{not: {true}}`, therefore the filter will match no documents.
         widgets = list_widgets_with(:amount_cents,
           filter: {id: {not: {equal_to_any_of: nil}}},
           order_by: [:amount_cents_ASC])
 
-        expect(widgets).to match([
-          string_hash_of(widget1, :id, :amount_cents),
-          string_hash_of(widget2, :id, :amount_cents),
-          string_hash_of(widget3, :id, :amount_cents)
-        ])
+        expect(widgets).to match []
 
         # Test that sorting by the same field twice in different directions doesn't fail.
         # (The extra sort should be effectively ignored).
@@ -765,8 +783,8 @@ module ElasticGraph
 
           # Verify `all_of: [{not: null}]` works as expected.
           results = query_teams_with(filter: {seasons_nested: {all_of: [{not: nil}]}})
-          # All teams should be returned since the `nil` part of the filter expression is pruned.
-          expect(results).to eq [{"id" => "t1"}, {"id" => "t2"}, {"id" => "t3"}, {"id" => "t4"}]
+          # No teams should be returned since the `nil` part of the filter expression evaluates to `true`.
+          expect(results).to eq []
 
           # Verify `all_of: [{not: null}]` works as expected.
           results = query_teams_with(filter: {seasons_nested: {all_of: [{all_of: nil}]}})
@@ -873,14 +891,15 @@ 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
           expect(list_widgets_with_options_and_inventor(
             filter: {options: {color: {not: {equal_to_any_of: nil}}}}
-          )).to contain_exactly(expected_widget1, expected_widget2)
+          )).to eq []
 
-          # not set to 'nil' should not cause any filtering on that value.
+          # not set to 'nil' should result in an always false filter
           expect(list_widgets_with_options_and_inventor(
             filter: {options: {color: {not: nil}}}
-          )).to contain_exactly(expected_widget1, expected_widget2)
+          )).to eq []
 
           # On type unions you can filter on a subfield that is present on all subtypes...
           expect(list_widgets_with_options_and_inventor(

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

@@ -898,6 +898,7 @@ 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)
@@ -912,6 +913,8 @@ 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
@@ -923,7 +926,7 @@ def search_datastore(**options, &before_msearch)
 
         expect(search_with_freeform_filter({"not" => {"any_of" => []}})).to eq ids_of(widget1, widget2)
 
-        pending "`not` doesn't yet compose correctly with an `any_of: [{}, ...]`, so these currently fail"
+        # 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 []
       end
@@ -1174,7 +1177,7 @@ def search_datastore(**options, &before_msearch)
           expect(triple_nested_not).to match_array(ids_of(widget1, widget3))
         end
 
-        it "is ignored when set to nil" do
+        it "returns the standard always false filter when set to nil" do
           index_into(
             graphql,
             widget1 = build(:widget, amount_cents: 100),
@@ -1212,12 +1215,46 @@ def search_datastore(**options, &before_msearch)
             "amount_cents" => {"equal_to_any_of" => nil}
           }})
 
-          expect(inner_not_result1).to eq(outer_not_result1).and match_array(ids_of(widget1, widget2, widget3))
-          expect(inner_not_result2).to eq(outer_not_result2).and match_array(ids_of(widget1))
-          expect(inner_not_result3).to eq(outer_not_result3).and match_array(ids_of(widget1, widget2, widget3))
+          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 []
+        end
+
+        it "is ignored when set to nil inside `any_of`" do
+          index_into(
+            graphql,
+            widget1 = build(:widget, amount_cents: 100),
+            build(:widget, amount_cents: 205),
+            build(:widget, amount_cents: 400)
+          )
+
+          inner_not_result1 = search_with_freeform_filter({"amount_cents" => {
+            "any_of" => [
+              {"not" => nil},
+              {"lt" => 200}
+            ]
+          }})
+
+          outer_not_result1 = search_with_freeform_filter({
+            "any_of" => [
+              {
+                "not" => {
+                  "amount_cents" => nil
+                }
+              },
+              {
+                "amount_cents" => {
+                  "lt" => 200
+                }
+              }
+            ]
+          })
+
+          expect(inner_not_result1).to eq(outer_not_result1).and match_array(ids_of(widget1))
         end
       end
 
+
       def search_with_freeform_filter(filter, **options)
         ids_of(search_datastore(filter: filter, sort: [], **options).to_a)
       end

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

@@ -1277,14 +1277,21 @@ def expect_script_query_with_params(query, expected_params)
           }})
         end
 
-        it "is ignored when set to nil" do
+        it "returns the standard always false filter when set to nil" do
           body_for_inner_not = datastore_body_of(new_query(filter: {"age" => {"not" => nil}}))
           body_for_outer_not = datastore_body_of(new_query(filter: {"not" => {"age" => nil}}))
 
-          expect(body_for_inner_not).to eq(body_for_outer_not).and not_filter_datastore_at_all
+          expect(body_for_inner_not).to eq(body_for_outer_not).and query_datastore_with(always_false_condition)
         end
 
-        it "is ignored when set to nil when alongside other filters" do
+        it "returns the standard always false filter when set to an emptyPredicate" do
+          body_for_inner_not = datastore_body_of(new_query(filter: {"age" => {"not" => {}}}))
+          body_for_outer_not = datastore_body_of(new_query(filter: {"not" => {"age" => {}}}))
+
+          expect(body_for_inner_not).to eq(body_for_outer_not).and query_datastore_with(always_false_condition)
+        end
+
+        it "returns the standard always false filter when set to nil alongside other filters" do
           body_for_inner_not = datastore_body_of(new_query(filter: {"age" => {
             "not" => nil,
             "gt" => 25
@@ -1297,14 +1304,41 @@ def expect_script_query_with_params(query, expected_params)
             }
           }))
 
-          expect(body_for_inner_not).to eq(body_for_outer_not).and query_datastore_with({bool: {filter: [{range: {"age" => {gt: 25}}}]}})
+          expect(body_for_inner_not).to eq(body_for_outer_not).and query_datastore_with({bool: {filter: [{match_none: {}}, {range: {"age" => {gt: 25}}}]}})
         end
 
-        it "is ignored when the inner filter is also ignored" do
+        it "returns the standard always false filter when set to nil alongside other filters inside `any_of`" do
+          body_for_inner_not = datastore_body_of(new_query(filter: {"age" => {
+            "any_of" => [
+              {"not" => nil},
+              {"gt" => 25}
+            ]
+          }}))
+
+          body_for_outer_not = datastore_body_of(new_query(filter: {
+            "any_of" => [
+              {"not" => nil},
+              {
+                "age" => {
+                  "gt" => 25
+                }
+              }
+            ]
+          }))
+
+          expect(body_for_inner_not).to query_datastore_with({bool: {filter: [{bool: {minimum_should_match: 1, should: [
+            {bool: {filter: [{match_none: {}}]}}, {bool: {filter: [{range: {"age" => {gt: 25}}}]}}
+          ]}}]}})
+          expect(body_for_outer_not).to query_datastore_with({bool: {minimum_should_match: 1, should: [
+            {bool: {filter: [{match_none: {}}]}}, {bool: {filter: [{range: {"age" => {gt: 25}}}]}}
+          ]}})
+        end
+
+        it "returns the standard always false filter when the inner filter evaluates to true" do
           body_for_inner_not = datastore_body_of(new_query(filter: {"age" => {"not" => {"equal_to_any_of" => nil}}}))
           body_for_outer_not = datastore_body_of(new_query(filter: {"not" => {"age" => {"equal_to_any_of" => nil}}}))
 
-          expect(body_for_inner_not).to eq(body_for_outer_not).and not_filter_datastore_at_all
+          expect(body_for_inner_not).to eq(body_for_outer_not).and query_datastore_with(always_false_condition)
         end
       end
 
@@ -1427,6 +1461,30 @@ def expect_script_query_with_params(query, expected_params)
           expect(datastore_body_of(query)).to not_filter_datastore_at_all
         end
 
+        it "filters to a false condition when given `not: {any_of: {age: nil}}` on a root field" do
+          query = new_query(filter: {
+            "not" => {"any_of" => [{"age" => nil}]}
+          })
+
+          expect(datastore_body_of(query)).to query_datastore_with(always_false_condition)
+        end
+
+        it "filters to a false condition when given `not: {any_of: nil}` on a sub field" do
+          query = new_query(filter: {
+            "age" => {"not" => {"any_of" => nil}}
+          })
+
+          expect(datastore_body_of(query)).to query_datastore_with(always_false_condition)
+        end
+
+        it "filters to a true condition when given `not: {any_of: []}` on a sub field" do
+          query = new_query(filter: {
+            "age" => {"not" => {"any_of" => []}}
+          })
+
+          expect(datastore_body_of(query)).to query_datastore_with({bool: {must_not: [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