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 38fea171..cb85c894 100644 --- a/elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_interpreter.rb +++ b/elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_interpreter.rb @@ -44,10 +44,7 @@ def build_query(filter_hashes, from_field_path: FieldPath.empty) end end - # ::Kernel.debugger - query = reduce_query(query) - # ::Kernel.debugger - query + reduce_query(query) end def to_s @@ -65,31 +62,21 @@ def to_s def reduce_query(bool_node) return bool_node if bool_node.nil? || bool_node == {} - # ::Kernel.debugger inner_node = bool_node[:bool] nested_node = bool_node[:nested] return bool_node if inner_node.nil? && nested_node.nil? - # ::Kernel.debugger - # Using `key?` to check if `inner_node` has any keys, as using `[key]` - # results in the specified key getting added with an empty array as value, - # due to `build_bool_hash` method. # `reduce_must_not` must be called before `reduce_filter`, as the method can add an `expression` to `:filter`. if reduce_must(inner_node) && reduce_must_not(inner_node) && reduce_filter(inner_node) && reduce_should(inner_node) && reduce_nested(nested_node) - # ::Kernel.debugger bool_node = nil end - return bool_node + bool_node end def reduce_must(inner_node) if !inner_node.nil? && inner_node.key?(:must) - expressions = inner_node[:must] - expressions = expressions.filter_map do |expression| - reduce_query(expression) - end - + # must should always have a expression, that cannot be reduced return false end @@ -108,8 +95,6 @@ def reduce_filter(inner_node) reduce_query(expression) end end - # ::Kernel.debugger - # ::Kernel.debugger return false unless expressions.empty? end @@ -120,17 +105,16 @@ def reduce_filter(inner_node) def reduce_should(inner_node) if !inner_node.nil? && inner_node.key?(:should) expressions = inner_node[:should] - expressions.delete(BooleanQuery::MATCH_NONE_FILTER_HASH) - # ::Kernel.debugger + if expressions.count(BooleanQuery::MATCH_NONE_FILTER_HASH) != expressions.size + expressions.delete(BooleanQuery::MATCH_NONE_FILTER_HASH) + end if expressions.include?(BooleanQuery::MATCH_ALL_FILTER_HASH) expressions.clear else expressions = expressions.filter_map do |expression| reduce_query(expression) end - # ::Kernel.debugger end - # ::Kernel.debugger return false unless expressions.empty? end @@ -141,18 +125,15 @@ def reduce_should(inner_node) def reduce_must_not(inner_node) if !inner_node.nil? && inner_node.key?(:must_not) expressions = inner_node[:must_not] - # ::Kernel.debugger expressions = expressions.filter_map do |expression| reduce_query(expression) end - # ::Kernel.debugger if expressions.empty? inner_node[:filter] << BooleanQuery::MATCH_NONE inner_node.delete(:must_not) return true end - # ::Kernel.debugger return false end @@ -160,12 +141,9 @@ def reduce_must_not(inner_node) end def reduce_nested(nested_node) - # ::Kernel.debugger if !nested_node.nil? && nested_node.key?(:query) expression = nested_node[:query] - # ::Kernel.debugger expression = reduce_query(expression) - # ::Kernel.debugger return false unless expression.nil? end @@ -225,10 +203,10 @@ def process_empty_or_nil_expression(bool_node, field_or_op) else BooleanQuery::MATCH_ALL_FILTER.merge_into(bool_node) end - # ::Kernel.debugger end def process_not_expression(bool_node, expression, field_path) + # @type var sub_filter: untyped sub_filter = build_bool_hash do |inner_node| process_filter_hash(inner_node, expression, field_path) end @@ -276,6 +254,7 @@ def process_any_satisfy_filter_expression_on_nested_object_list(bool_node, filte # the fact that documents with any list element values matching the predicates will match # the overall filter. def process_any_satisfy_filter_expression_on_scalar_list(bool_node, filter, field_path) + # @type var processed: untyped processed = build_bool_hash { |node| process_filter_hash(node, filter, field_path) } processed_bool_query = processed.fetch(:bool) @@ -437,15 +416,13 @@ def process_list_count_expression(bool_node, expression, field_path) ]} end - # ::Kernel.debugger process_sub_field_expression(bool_node, expression, field_path.counts_path) - # ::Kernel.debugger end def build_bool_hash(&block) bool_node = Hash.new { |h, k| h[k] = [] }.tap(&block) - # To ignore "empty" filter predicates we need to return `nil` here. + # To threat "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, diff --git a/elasticgraph-graphql/spec/acceptance/search_spec.rb b/elasticgraph-graphql/spec/acceptance/search_spec.rb index 64ef2537..2eaf4423 100644 --- a/elasticgraph-graphql/spec/acceptance/search_spec.rb +++ b/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 && }` 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., @@ -753,7 +775,7 @@ 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. + # 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. diff --git a/elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb b/elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb index 7fdd7242..71c108c8 100644 --- a/elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb +++ b/elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb @@ -889,7 +889,7 @@ def search_datastore(**options, &before_msearch) expect(results).to match_array(ids_of(widget2, widget4)) end - specify "`equal_to_any_of: []` or `any_of: []` matches no documents, but `any_predicate: nil` or `field: {}` is ignored, matching all documents" do + specify "`equal_to_any_of: []` or `any_of: []` matches no documents, but `any_predicate: nil` or `field: {}` is treated as `true`, matching all documents" do index_into( graphql, widget1 = build(:widget), @@ -898,6 +898,8 @@ 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_freeform_filter({"any_of" => [{"any_of" => []}]})).to eq [] expect(search_with_filter("id", "equal_to_any_of", nil)).to eq ids_of(widget1, widget2) expect(search_with_filter("amount_cents", "lt", nil)).to eq ids_of(widget1, widget2) @@ -909,6 +911,10 @@ def search_datastore(**options, &before_msearch) expect(search_with_filter("name_text", "matches_query", nil)).to eq ids_of(widget1, widget2) expect(search_with_filter("name_text", "matches_phrase", nil)).to eq ids_of(widget1, widget2) 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({"not" => {"any_of" => [{"id" => nil}]}})).to eq [] + expect(search_with_freeform_filter({"any_of" => [{"id" => nil}, {"amount_cents" => {"lt" => 1000}}]})).to eq ids_of(widget1, widget2) + 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 @@ -1160,9 +1166,9 @@ def search_datastore(**options, &before_msearch) it "returns the standard always false filter 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" => { @@ -1204,8 +1210,8 @@ def search_datastore(**options, &before_msearch) 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: 205), + build(:widget, amount_cents: 400) ) inner_not_result1 = search_with_freeform_filter({"amount_cents" => { 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 6c038910..6b626ccd 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 @@ -14,9 +14,8 @@ class GraphQL RSpec.describe DatastoreQuery, "filtering" do include_context "DatastoreQueryUnitSupport" - # TODO: RENAME let(:always_false_condition) do - {bool: {filter: Filtering::BooleanQuery::MATCH_NONE_FILTER.clauses}} + {bool: {filter: [Filtering::BooleanQuery::MATCH_NONE]}} end it "builds a `nil` datastore body when given no filters (passed as `nil`)" do @@ -1316,7 +1315,8 @@ def expect_script_query_with_params(query, expected_params) { "age" => { "gt" => 25 - }} + } + } ] })) @@ -1405,7 +1405,25 @@ def expect_script_query_with_params(query, expected_params) ]}) end - it "reduces an `any_of` composed entirely of empty predicates to a false condition" do + it "returns the standard always false filter for `any_of: []`" do + query = new_query(filter: { + "any_of" => [] + }) + + expect(datastore_body_of(query)).to query_datastore_with(always_false_condition) + end + + it "returns the standard always false filter for `any_of: [{any_of: []}]`" do + query = new_query(filter: { + "any_of" => [{"any_of" => []}] + }) + + expect(datastore_body_of(query)).to query_datastore_with({bool: {minimum_should_match: 1, should: [ + always_false_condition + ]}}) + end + + it "applies no filtering to an `any_of` composed entirely of empty predicates" do query = new_query(filter: { "age" => {"any_of" => [{"gt" => nil}, {"lt" => nil}]} }) @@ -1413,6 +1431,14 @@ def expect_script_query_with_params(query, expected_params) expect(datastore_body_of(query)).to not_filter_datastore_at_all end + it "applies no filtering to an `any_of` composed of empty predicate and non empty predicate" do + query = new_query(filter: { + "age" => {"any_of" => [{"gt" => nil}, {"lt" => 36}]} + }) + + expect(datastore_body_of(query)).to not_filter_datastore_at_all + end + it "does not filter at all when given only `any_of: nil` on a root field" do query = new_query(filter: { "age" => {"any_of" => nil} @@ -1429,6 +1455,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 diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/search_index_expression_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/search_index_expression_spec.rb index c742995b..ad3260eb 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/search_index_expression_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/search_index_expression_spec.rb @@ -369,6 +369,24 @@ class GraphQL 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 + parts = search_index_expression_parts_for({"any_of" => [{"any_of" => []}]}) + + expect(parts).to target_all_widget_indices + end + + it "excludes no indices when we have an `any_of: [{field: nil}]` filter because that will match all results" do + parts = search_index_expression_parts_for({"any_of" => [{"created_at" => nil}]}) + + expect(parts).to target_all_widget_indices + end + + it "excludes no indices when we have an `any_of: [{field: nil}, {...}]` filter because that will match all results" do + parts = search_index_expression_parts_for({"any_of" => [{"created_at" => nil}, {"id" => {"equal_to_any_of" => "some-id"}}]}) + + expect(parts).to target_all_widget_indices + end end context "for a query that includes aggregations" do diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/shard_routing_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/shard_routing_spec.rb index 31eaa5a1..ac650108 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/shard_routing_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/shard_routing_spec.rb @@ -259,6 +259,24 @@ 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: [{anyof: []}]` filter because that will match all results" do + expect(shard_routing_for(["name"], { + "any_of" => [{"any_of" => []}] + })).to search_all_shards + end + + it "searches all shards when we have an `any_of: [{field: nil}]` filter because that will match all results" do + expect(shard_routing_for(["name"], { + "any_of" => [{"name" => nil}] + })).to search_all_shards + end + + it "searches all shards when we have an `any_of: [{field: nil}, {...}]` filter because that will match all results" do + expect(shard_routing_for(["name"], { + "any_of" => [{"name" => nil}, {"id" => {"equal_to_any_of" => ["abc"]}}] + })).to search_all_shards + end + describe "not" do it "searches all shards when there are values in an `equal_to_any_of` filter" do expect(shard_routing_for(["name"],