From 60ee18402005ab081f65a3aa78291f1eb6c07746 Mon Sep 17 00:00:00 2001 From: Myron Marston Date: Tue, 4 Feb 2025 16:51:51 -0800 Subject: [PATCH] Bugfix: avoid `TypeError` on `not: {not: {anyOf: [...]}}`. We have special handling for double nots to "pull up" the filter clauses for greater efficiency. That handling didn't work right when an `anyOf` was under the double `not`, and we got a `TypeError`: ``` TypeError: no implicit conversion of Integer into Array from elastic_graph/graphql/filtering/filter_interpreter.rb:121:in `concat' from elastic_graph/graphql/filtering/filter_interpreter.rb:121:in `block (2 levels) in process_not_expression' from elastic_graph/graphql/filtering/filter_interpreter.rb:121:in `each' from elastic_graph/graphql/filtering/filter_interpreter.rb:121:in `block in process_not_expression' from elastic_graph/graphql/filtering/filter_interpreter.rb:120:in `each' from elastic_graph/graphql/filtering/filter_interpreter.rb:120:in `process_not_expression' from elastic_graph/graphql/filtering/filter_interpreter.rb:66:in `block in process_filter_hash' from elastic_graph/graphql/filtering/filter_interpreter.rb:61:in `each' from elastic_graph/graphql/filtering/filter_interpreter.rb:61:in `process_filter_hash' from elastic_graph/graphql/filtering/filter_interpreter.rb:43:in `block (2 levels) in build_query' from set.rb:511:in `each_key' from set.rb:511:in `each' from elastic_graph/graphql/filtering/filter_interpreter.rb:42:in `block in build_query' from :90:in `tap' from elastic_graph/graphql/filtering/filter_interpreter.rb:337:in `build_bool_hash' from elastic_graph/graphql/filtering/filter_interpreter.rb:41:in `build_query' from elastic_graph/graphql/datastore_query.rb:318:in `to_datastore_body' from elastic_graph/graphql/datastore_query.rb:229:in `empty?' from elastic_graph/graphql/datastore_query.rb:94:in `each' from elastic_graph/graphql/datastore_query.rb:94:in `partition' from elastic_graph/graphql/datastore_query.rb:94:in `perform' from elastic_graph/graphql/datastore_search_router.rb:34:in `msearch' from elastic_graph/graphql/resolvers/query_source.rb:26:in `fetch' from graphql/dataloader/source.rb:136:in `run_pending_keys' from graphql/dataloader.rb:294:in `block (2 levels) in spawn_source_fiber' from graphql/dataloader.rb:292:in `each' from graphql/dataloader.rb:292:in `block in spawn_source_fiber' from graphql/dataloader.rb:245:in `block in spawn_fiber' ``` This fixes that case. --- .../graphql/filtering/filter_interpreter.rb | 7 ++- .../spec/acceptance/search_spec.rb | 47 ++++++++++++++----- .../graphql/datastore_query/filtering_spec.rb | 14 ++++++ 3 files changed, 56 insertions(+), 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 index 5dec5b52..1528f585 100644 --- 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 index 6fd7a154..c7447b06 100644 --- 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 index 1b5b3b03..831d37d9 100644 --- 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