Merge pull request #141 from block/myron/fix-TypeError

Bugfix: avoid `TypeError` on `not: {not: {anyOf: [...]}}`.
block · Feb 5, 2025 · a29eefe · a29eefe
2 parents e24159b + 60ee184
commit a29eefe
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 12 deletions.
diff --git a/elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_interpreter.rb b/elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_interpreter.rb
@@ -118,7 +118,12 @@ def process_not_expression(bool_node, expression, field_path)
           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) }
+              negated_clause_bool = negated_clause[:bool]
+              # `minimum_should_match` is not a list like the other clauses, and needs to be handled separately.
+              negated_clause_bool.except(:minimum_should_match).each { |k, v| bool_node[k].concat(v) }
+              if (min_should_match = negated_clause_bool[:minimum_should_match])
+                bool_node[:minimum_should_match] = min_should_match
+              end
             end
           end
 

diff --git a/elasticgraph-graphql/spec/acceptance/search_spec.rb b/elasticgraph-graphql/spec/acceptance/search_spec.rb
@@ -84,18 +84,28 @@ module ElasticGraph
         expect(list_widget_currencies(filter: {primary_continent: {equal_to_any_of: ["North America"]}})).to eq(unfiltered_widget_currencies)
         expect_to_have_routed_to_shards_with("main", ["widget_currencies_rollover__*", "North America"])
 
-        filter = {
-          options: {
-            any_of: [
-              {size: {equal_to_any_of: [enum_value(:MEDIUM)]}},
-              {
-                size: {equal_to_any_of: [enum_value(:SMALL)]},
-                color: {equal_to_any_of: [enum_value(:BLUE)]}
-              }
-            ]
+        options_any_of_filter = [
+          {size: {equal_to_any_of: [enum_value(:MEDIUM)]}},
+          {
+            size: {equal_to_any_of: [enum_value(:SMALL)]},
+            color: {equal_to_any_of: [enum_value(:BLUE)]}
           }
-        }
-        widgets = list_widgets_with(<<~EOS, order_by: [:amount_cents_ASC], filter: filter)
+        ]
+        widgets = list_widgets_with(<<~EOS, order_by: [:amount_cents_ASC], filter: {options: {any_of: options_any_of_filter}})
+          #{case_correctly("amount_cents")}
+          tags
+          fees {
+            currency
+            #{case_correctly("amount_cents")}
+          }
+        EOS
+
+        expect(widgets).to match([
+          string_hash_of(widget1, :id, :amount_cents, :tags, :fees),
+          string_hash_of(widget3, :id, :amount_cents, :tags, :fees)
+        ])
+
+        widgets = list_widgets_with(<<~EOS, order_by: [:amount_cents_ASC], filter: {options: {not: {not: {any_of: options_any_of_filter}}}})
           #{case_correctly("amount_cents")}
           tags
           fees {
@@ -163,6 +173,15 @@ module ElasticGraph
           string_hash_of(widget3, :id, :amount_cents)
         ])
 
+        # Verify we can use the `not` filter operator correctly
+        widgets = list_widgets_with(:amount_cents,
+          filter: {options: {not: {not: {color: {equal_to_any_of: [enum_value(:BLUE)]}}}}},
+          order_by: [:amount_cents_ASC])
+
+        expect(widgets).to match([
+          string_hash_of(widget1, :id, :amount_cents)
+        ])
+
         widgets = list_widgets_with(:amount_cents,
           filter: {options: {not: {color: {equal_to_any_of: [nil]}}}},
           order_by: [:amount_cents_ASC])
@@ -320,6 +339,12 @@ module ElasticGraph
           string_hash_of(widget1, :id, :amount_cents)
         ])
 
+        # Test `not: {not: {any_of: []}}` results in no matching documents
+        widgets = list_widgets_with(:amount_cents,
+          filter: {not: {not: {any_of: []}}})
+
+        expect(widgets).to match([])
+
         # 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}}]}})

diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/filtering_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/filtering_spec.rb
@@ -1340,6 +1340,20 @@ def expect_script_query_with_params(query, expected_params)
 
           expect(body_for_inner_not).to eq(body_for_outer_not).and query_datastore_with(always_false_condition)
         end
+
+        it "can double negate an `any_of`" do
+          inner_filter = {
+            "any_of" => [
+              {"age" => {"equal_to_any_of" => [25, 30]}},
+              {"height" => {"gt" => 10}}
+            ]
+          }
+
+          query1 = new_query(filter: {"not" => {"not" => inner_filter}})
+          query2 = new_query(filter: inner_filter)
+
+          expect(datastore_body_of(query1)).to eq(datastore_body_of(query2))
+        end
       end
 
       describe "behavior of empty/null filter values" do