From fd87ea9e3924ff4cb05e27a45d19b1755fcf6826 Mon Sep 17 00:00:00 2001 From: Matt Carpenter <61465245+mattcarp21@users.noreply.github.com> Date: Wed, 6 Nov 2024 16:25:37 -0500 Subject: [PATCH 1/7] Update the FullTextSearch docs to provide an example of their optionality (#25) * Update the FullTextSearch docs to provide an example of their optionality In order to ignore a `matchesQuery` or `matchesPhrase` filter predicate, you can supply `null` to the `MatchesQueryFilterInput` or `MatchesQueryFilterInput` types, respectively. However, this is not entirely clear from the docs, as they describe supplying `null`, but not to which field/value. This documentation update aims to make that more clear, and provides an example to highlight the ability to use a null-able variable with these predicates. ## Tested Ran the site locally. Here are the screenshots of updates: * update PR with suggestions from comments, including: * Remove the language around "ignoring" filters * Revert the changes to the high-level fulltext.md file * Alter the example query to use 2 spaces instead of 4-space tabs --- .../filtering/OptionalMatchingFilter.graphql | 18 ++++++++++++++++++ .../query-api/filtering/full-text-search.md | 8 ++++++++ 2 files changed, 26 insertions(+) create mode 100644 config/site/examples/music/queries/filtering/OptionalMatchingFilter.graphql diff --git a/config/site/examples/music/queries/filtering/OptionalMatchingFilter.graphql b/config/site/examples/music/queries/filtering/OptionalMatchingFilter.graphql new file mode 100644 index 00000000..89afeae9 --- /dev/null +++ b/config/site/examples/music/queries/filtering/OptionalMatchingFilter.graphql @@ -0,0 +1,18 @@ +query OptionalMatchingFilter( + $optionalMatchQuery: MatchesQueryFilterInput = null +) { + artists(filter: { + bio: { + description: { + matchesQuery: $optionalMatchQuery + } + } + }) { + nodes { + name + bio { + description + } + } + } +} \ No newline at end of file diff --git a/config/site/src/query-api/filtering/full-text-search.md b/config/site/src/query-api/filtering/full-text-search.md index ad3ce689..2071cb01 100644 --- a/config/site/src/query-api/filtering/full-text-search.md +++ b/config/site/src/query-api/filtering/full-text-search.md @@ -39,3 +39,11 @@ are supported to control both aspects to make matching stricter: {% highlight graphql %} {{ site.data.music_queries.filtering.PhraseSearch }} {% endhighlight %} + +### Bypassing matchesPhrase and matchesQuery + +In order to make a `matchesPhrase` or `matchesQuery` filter optional, you can supply `null` to the `MatchesQueryFilterInput` parameter, like this: + +{% highlight graphql %} +{{ site.data.music_queries.filtering.OptionalMatchingFilter }} +{% endhighlight %} From faab28a7b27d8e962471bee43027618201a38186 Mon Sep 17 00:00:00 2001 From: Myron Marston Date: Wed, 6 Nov 2024 16:49:59 -0800 Subject: [PATCH 2/7] Prepare test suite for for v2.4 of the GraphQL gem. Before v2.4 of the GraphQL gem, `GraphQL::Schema#types` returned _all_ types defined by the SDL string. Beginning in v2.4, orphaned types (that is, types not reachable from the root `Query` type) are no longer included. We have a number of unit tests that define orphaned types since we don't want or need a full schema for such a test. To avoid issues as part of upgrading to v2.4, we need to ensure that our tests don't depend on orphaned types that are unavailable in v2.4 and later. --- elasticgraph-graphql/spec/spec_helper.rb | 3 +- .../spec/support/ensure_no_orphaned_types.rb | 51 +++++++++++++++++++ .../graphql/query_executor_spec.rb | 1 + .../resolvers/get_record_field_value_spec.rb | 2 + .../relay_connection/array_adapter_spec.rb | 2 + .../resolvers/resolvable_value_spec.rb | 4 ++ .../graphql/schema/enum_value_spec.rb | 2 +- .../graphql/schema/field_spec.rb | 2 +- .../elastic_graph/graphql/schema/type_spec.rb | 18 +++---- .../unit/elastic_graph/graphql/schema_spec.rb | 16 ++---- 10 files changed, 74 insertions(+), 27 deletions(-) create mode 100644 elasticgraph-graphql/spec/support/ensure_no_orphaned_types.rb diff --git a/elasticgraph-graphql/spec/spec_helper.rb b/elasticgraph-graphql/spec/spec_helper.rb index 222428f7..d4342a43 100644 --- a/elasticgraph-graphql/spec/spec_helper.rb +++ b/elasticgraph-graphql/spec/spec_helper.rb @@ -28,8 +28,9 @@ def build_datastore_core(**options, &block) meta[:builds_graphql] = true end - config.when_first_matching_example_defined(:resolver) { require_relative "support/resolver" } + config.when_first_matching_example_defined(:ensure_no_orphaned_types) { require_relative "support/ensure_no_orphaned_types" } config.when_first_matching_example_defined(:query_adapter) { require_relative "support/query_adapter" } + config.when_first_matching_example_defined(:resolver) { require_relative "support/resolver" } config.prepend ElasticGraph::GraphQLSpecHelpers, absolute_file_path: %r{/elasticgraph-graphql/} end diff --git a/elasticgraph-graphql/spec/support/ensure_no_orphaned_types.rb b/elasticgraph-graphql/spec/support/ensure_no_orphaned_types.rb new file mode 100644 index 00000000..4d9e1110 --- /dev/null +++ b/elasticgraph-graphql/spec/support/ensure_no_orphaned_types.rb @@ -0,0 +1,51 @@ +# Copyright 2024 Block, Inc. +# +# Use of this source code is governed by an MIT-style +# license that can be found in the LICENSE file or at +# https://opensource.org/licenses/MIT. +# +# frozen_string_literal: true + +module ElasticGraph + class GraphQL + # Before v2.4 of the GraphQL gem, `GraphQL::Schema#types` returned _all_ types defined by the SDL string. + # Beginning in v2.4, orphaned types (that is, types not reachable from the root `Query` type) are no longer + # included. We have a number of unit tests that define orphaned types since we don't want or need a full + # schema for such a test. + # + # To avoid issues as part of upgrading to v2.4, we need to ensure that our tests don't depend on orphaned + # types that are unavailable in v2.4 and later. This mixin provides a simple solution: it adds on an indexed + # type (`IndexedTypeToEnsureNoOrphans`) with a field for each defined type, ensuring that no defined types + # are orphans. + # + # Apply it to an example or example group using the `:ensure_no_orphaned_types` tag. + module EnsureNoOrphanedTypes + def build_graphql(schema_definition: nil, **options, &block) + schema_def = lambda do |schema| + original_types = schema.state.types_by_name.keys + schema_definition.call(schema) + + # If a test is taking are of defining its own indexed types, we don't need to do anything further. + return if schema.state.object_types_by_name.values.any?(&:indexed?) + + added_types = schema.state.types_by_name.keys - original_types + + schema.object_type "IndexedTypeToEnsureNoOrphans" do |t| + added_types.each do |type_name| + t.field type_name, type_name + end + + t.field "id", "ID" + t.index "indexed_types" + end + end + + super(schema_definition: schema_def, **options, &block) + end + end + + ::RSpec.configure do |c| + c.include EnsureNoOrphanedTypes, :ensure_no_orphaned_types + end + end +end diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_executor_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_executor_spec.rb index 8c60d771..221a59d7 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_executor_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_executor_spec.rb @@ -42,6 +42,7 @@ class GraphQL } type Query { + float: Float # so the Float type exists colors(args: ColorArgs): [Color!]! colors2(args: ColorArgs): [Color2!]! } diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/get_record_field_value_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/get_record_field_value_spec.rb index fbfbd76b..d41c3a3a 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/get_record_field_value_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/get_record_field_value_spec.rb @@ -27,6 +27,7 @@ module Resolvers end schema.object_type "Person" do |t| + t.field "id", "ID" t.field "name", "String" t.field "identifiers", "PersonIdentifiers" t.field "ssn", "String", name_in_index: "identifiers.ssn", graphql_only: true @@ -36,6 +37,7 @@ module Resolvers t.field "nicknames", "[String!]" t.field "alt_nicknames", "[String!]", name_in_index: "nicknames", graphql_only: true t.field "doc_count", "Int" + t.index "people" end end end diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/relay_connection/array_adapter_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/relay_connection/array_adapter_spec.rb index 7f01af8e..4dbd78e2 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/relay_connection/array_adapter_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/relay_connection/array_adapter_spec.rb @@ -161,7 +161,9 @@ def build_graphql(**options) def generate_schema_artifacts(**options) super(**options) do |schema| schema.object_type "Widget" do |t| + t.field "id", "ID" t.paginated_collection_field "natural_numbers", "Int" + t.index "widgets" end end end diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/resolvable_value_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/resolvable_value_spec.rb index d16f7075..11e1e84f 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/resolvable_value_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/resolvers/resolvable_value_spec.rb @@ -35,6 +35,10 @@ module Resolvers favorite_quote(truncate_to: Int, foo_bar_bazz: Int): String favorite_quote2(trunc_to: Int): String } + + type Query { + person: Person + } EOS end end diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema/enum_value_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema/enum_value_spec.rb index 4a560dca..d97c0508 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema/enum_value_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema/enum_value_spec.rb @@ -12,7 +12,7 @@ module ElasticGraph class GraphQL class Schema - RSpec.describe EnumValue do + RSpec.describe EnumValue, :ensure_no_orphaned_types do it "inspects well" do enum_value = define_schema do |s| s.enum_type "ColorSpace" do |t| diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema/field_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema/field_spec.rb index 26d2808e..aaaeec54 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema/field_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema/field_spec.rb @@ -12,7 +12,7 @@ module ElasticGraph class GraphQL class Schema - RSpec.describe Field do + RSpec.describe Field, :ensure_no_orphaned_types do it "exposes the name as a lowercase symbol" do field = define_schema do |schema| schema.object_type "Color" do |t| diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema/type_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema/type_spec.rb index 95e72c27..991cd419 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema/type_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema/type_spec.rb @@ -12,7 +12,7 @@ module ElasticGraph class GraphQL class Schema - RSpec.describe Type do + RSpec.describe Type, :ensure_no_orphaned_types do it "exposes the name as a capitalized symbol" do type = define_schema do |schema| schema.object_type "Color" @@ -102,15 +102,6 @@ class Schema t.field "node", "Color!" end - # This must be raw SDL because our schema definition API provides no way to define custom `input` types--it - # generates them based on our indexed types. Using `raw_sdl` lets us define what the test expects. We may want - # to update what the test expects to use generated filter types in the future so we don't have to use `raw_sdl` - # here. - %w[Some PersonEdge PersonConnection].each do |type| - schema.raw_sdl "input #{type}FilterInput { foo: Int }" - schema.raw_sdl "input #{type}ListFilterInput { foo: Int }" - end - schema.object_type "WrappedTypes" do |t| t.field "int", "Int" t.field "non_null_int", "Int!" @@ -147,6 +138,9 @@ class Schema t.field "non_null_list_of_indexed_aggregation", "[PersonAggregation]!", filterable: false, groupable: false do |f| f.mapping type: "object" end + + t.field "id", "ID" + t.index "wrapped_types" end end end @@ -395,9 +389,9 @@ class Schema end it "can model an input type" do - type = schema.type_named(:SomeFilterInput) + type = schema.type_named(:IntFilterInput) - expect(type.name).to eq :SomeFilterInput + expect(type.name).to eq :IntFilterInput expect(type).to only_satisfy_predicates(:nullable?, :object?) expect(type.unwrap_fully).to be type expect(type.unwrap_non_null).to be type diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema_spec.rb index af71ac60..c15fe71d 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema_spec.rb @@ -11,7 +11,7 @@ module ElasticGraph class GraphQL - RSpec.describe Schema do + RSpec.describe Schema, :ensure_no_orphaned_types do it "can be instantiated with directives that have custom scalar arguments" do define_schema do |schema| schema.scalar_type "_FieldSet" do |t| @@ -79,17 +79,9 @@ class GraphQL s.object_type "Color" end - expect(schema.defined_types).to include( - schema.type_named(:Options), - schema.type_named(:Color), - schema.type_named(:Query) - ).and exclude( - schema.type_named(:Int), - schema.type_named(:Float), - schema.type_named(:Boolean), - schema.type_named(:String), - schema.type_named(:ID) - ) + expect(schema.defined_types).to all be_a Schema::Type + expect(schema.defined_types.map(&:name)).to include(:Options, :Color, :Query) + .and exclude(:Int, :Float, :Boolean, :String, :ID) end end From ed1fac21640542ce83b97609413a1eb6664c7e86 Mon Sep 17 00:00:00 2001 From: Myron Marston Date: Fri, 8 Nov 2024 16:26:59 -0800 Subject: [PATCH 3/7] Update discord link. The prior link seems to have expired--I apparently forgot to set it to never expire. --- CONTRIBUTING.md | 4 ++-- GOVERNANCE.md | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fc624249..d16e7d02 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -2,7 +2,7 @@ There are many ways to be an open source contributor, and we're here to help you on your way! You may: -* Propose ideas in our [discord](https://discord.com/invite/c79mzZnu) +* Propose ideas in our [discord](https://discord.gg/8m9FqJ7a7F) * Raise an issue or feature request in our [issue tracker](https://github.com/block/elasticgraph/issues) * Help another contributor with one of their questions, or a code review * Suggest improvements to our Getting Started documentation by supplying a Pull Request @@ -54,7 +54,7 @@ Anyone from the community is welcome (and encouraged!) to raise issues via ### Discussions -Design discussions and proposals take place in our [discord](https://discord.com/invite/c79mzZnu). +Design discussions and proposals take place in our [discord](https://discord.gg/8m9FqJ7a7F). We advocate an asynchronous, written debate model - so write up your thoughts and invite the community to join in! diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 7d9fa5e6..dae58f3a 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -12,7 +12,7 @@ Anyone may be a contributor to Block open source projects. Contribution may take the form of: -* Asking and answering questions on the [Discord](https://discord.com/invite/c79mzZnu) or [GitHub Issues](https://github.com/block/elasticgraph/issues). +* Asking and answering questions on the [Discord](https://discord.gg/8m9FqJ7a7F) or [GitHub Issues](https://github.com/block/elasticgraph/issues). * Filing an issue * Offering a feature or bug fix via a Pull Request * Suggesting documentation improvements From 817d91b3dede96022d756c30b8e03b03131fdeb3 Mon Sep 17 00:00:00 2001 From: Alex Patel Date: Mon, 28 Oct 2024 15:10:54 -0400 Subject: [PATCH 4/7] Change anyOf behaviour to evaluate to true for empty predicates --- .../graphql/filtering/boolean_query.rb | 7 +- .../graphql/filtering/filter_interpreter.rb | 37 ++++-- .../filtering/filter_value_set_extractor.rb | 2 +- .../spec/acceptance/search_spec.rb | 59 ++++++--- .../graphql/datastore_query/filtering_spec.rb | 77 ++++++++++-- .../datastore_query/sub_aggregations_spec.rb | 2 +- ...grouped_sub_aggregation_shared_examples.rb | 2 +- .../graphql/datastore_query/filtering_spec.rb | 118 ++++++++++++++++-- .../search_index_expression_spec.rb | 26 +++- .../datastore_query/shard_routing_spec.rb | 24 +++- .../datastore_query/sub_aggregations_spec.rb | 2 +- .../graphql/query_adapter/filters_spec.rb | 14 +-- 12 files changed, 299 insertions(+), 71 deletions(-) diff --git a/elasticgraph-graphql/lib/elastic_graph/graphql/filtering/boolean_query.rb b/elasticgraph-graphql/lib/elastic_graph/graphql/filtering/boolean_query.rb index 39bad74e..93f619cb 100644 --- a/elasticgraph-graphql/lib/elastic_graph/graphql/filtering/boolean_query.rb +++ b/elasticgraph-graphql/lib/elastic_graph/graphql/filtering/boolean_query.rb @@ -33,12 +33,7 @@ def merge_into(bool_node) bool_node[occurrence].concat(clauses) end - # For `any_of: []` we need a way to force the datastore to match no documents, but - # I haven't found any sort of literal `false` we can pass in the compound expression - # or even a literal `1 = 0` as is sometimes used in SQL. Instead, we use this for that - # case. - empty_array = [] # : ::Array[untyped] - ALWAYS_FALSE_FILTER = filter({ids: {values: empty_array}}) + ALWAYS_FALSE_FILTER = filter({match_none: {}}) end end end 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 a69c0772..567021a6 100644 --- a/elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_interpreter.rb +++ b/elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_interpreter.rb @@ -61,7 +61,7 @@ def process_filter_hash(bool_node, filter_hash, field_path) filter_hash.each do |field_or_op, expression| case filter_node_interpreter.identify_node_type(field_or_op, expression) when :empty - # This is an "empty" filter predicate and we can ignore it. + # This is an "empty" filter predicate and can be treated as `true`. when :not process_not_expression(bool_node, expression, field_path) when :list_any_filter @@ -103,13 +103,15 @@ def filters_on_sub_fields?(expression) end def process_not_expression(bool_node, expression, field_path) - return if expression.nil? || expression == {} - sub_filter = build_bool_hash do |inner_node| - process_filter_hash(inner_node, expression, field_path) + process_filter_hash(inner_node, expression || {}, field_path) end - return unless sub_filter + 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 # Prevent any negated filters from being unnecessarily double-negated by # converting them to a positive filter (i.e., !!A == A). @@ -187,21 +189,32 @@ def process_any_satisfy_filter_expression_on_scalar_list(bool_node, filter, fiel end end + # We want to provide the following semantics for `any_of`: + # + # * `filter: {anyOf: []}` -> return no results + # * `filter: {anyOf: [{field: null}]}` -> return all results + # * `filter: {anyOf: [{field: null}, {field: ...}]}` -> return all results def process_any_of_expression(bool_node, expressions, field_path) return if expressions.nil? || expressions == {} + if expressions.empty? + # When our `expressions` array is empty, we want to match no documents. However, that's + # not the behavior the datastore will give us if we have an empty array in the query under + # `should`. To get the behavior we want, we need to pass the datastore some filter criteria + # that will evaluate to false for every document. + BooleanQuery::ALWAYS_FALSE_FILTER.merge_into(bool_node) + return + end + shoulds = expressions.filter_map do |expression| build_bool_hash do |inner_bool_node| process_filter_hash(inner_bool_node, expression, field_path) end end - # When our `shoulds` array is empty, the filtering semantics we want is to match no documents. - # However, that's not the behavior the datastore will give us if we have an empty array in the - # query under `should`. To get the behavior we want, we need to pass the datastore some filter - # criteria that will evaluate to false for every document. - bool_query = shoulds.empty? ? BooleanQuery::ALWAYS_FALSE_FILTER : BooleanQuery.should(*shoulds) - bool_query.merge_into(bool_node) + return if shoulds.size < expressions.size + + BooleanQuery.should(*shoulds).merge_into(bool_node) end def process_all_of_expression(bool_node, expressions, field_path) @@ -333,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 ignore "empty" filter predicates 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, diff --git a/elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_value_set_extractor.rb b/elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_value_set_extractor.rb index 649127b7..c3667d66 100644 --- a/elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_value_set_extractor.rb +++ b/elasticgraph-graphql/lib/elastic_graph/graphql/filtering/filter_value_set_extractor.rb @@ -83,7 +83,7 @@ def filter_value_set_for_filter_hash(filter_hash, target_field_path_parts, trave # to a particular field. def filter_value_set_for_filter_hash_entry(field_or_op, filter_value, target_field_path_parts, traversed_field_path_parts, negate:) if filter_value.nil? - # Any filter with a `nil` value is effectively ignored by our filtering logic, so we need + # Any filter with a `nil` value is effectively treated as `true` by our filtering logic, so we need # to return our `@all_values_set` to indicate this filter matches all documents. @all_values_set elsif field_or_op == @schema_names.not diff --git a/elasticgraph-graphql/spec/acceptance/search_spec.rb b/elasticgraph-graphql/spec/acceptance/search_spec.rb index 7fd60ebb..6fd7a154 100644 --- a/elasticgraph-graphql/spec/acceptance/search_spec.rb +++ b/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 ignored) + # 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 ignored) + # 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]) @@ -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, 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}}]}}) + + 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). @@ -630,6 +648,14 @@ module ElasticGraph }}}}) expect(results).to eq [{"id" => "t1"}, {"id" => "t2"}, {"id" => "t3"}] + # 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: [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"}] + # Verify we can use `any_of` directly under `any_satisfy` on a nested field. results = query_teams_with(filter: {current_players_nested: {any_satisfy: {any_of: [ {name: {equal_to_any_of: ["Johnny Rocket"]}}, @@ -757,17 +783,17 @@ 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}]}}) - # 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 @@ -865,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: 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 contain_exactly(expected_widget1, expected_widget2) + )).to eq [] - # not set to 'nil' should not cause any filtering on that value. + # `{not: 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( 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 037d16e5..4a4a862c 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 @@ -283,7 +283,7 @@ def search_datastore(**options, &before_msearch) expect(results).to match_array(ids_of(team2)) end - it "correctly ignores nil filters under `any_satisfy`" 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}} @@ -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,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_filter("id", "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 +910,22 @@ 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({"any_of" => [{"id" => nil}, {"amount_cents" => {"lt" => 0}}]})).to eq ids_of(widget1, widget2) + end + + specify "`not: {any_of: []}` matches all documents, but `not: {any_of: [field: nil, ...]}` is treated as `false` matching no documents" do + index_into( + graphql, + widget1 = build(:widget, id: "one"), + widget2 = build(:widget, id: "two") + ) + + 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 [] + + 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 it "`equal_to_any_of:` with `nil` matches documents with null values or not null values" do @@ -1157,12 +1174,12 @@ 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 "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" => { @@ -1195,9 +1212,51 @@ 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)) + 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 treated as `true` 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 diff --git a/elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/sub_aggregations_spec.rb b/elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/sub_aggregations_spec.rb index 5d3c30db..df0c02d8 100644 --- a/elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/sub_aggregations_spec.rb +++ b/elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/sub_aggregations_spec.rb @@ -193,7 +193,7 @@ class GraphQL }] end - it "ignores empty filters" 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} diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/resolvers/ungrouped_sub_aggregation_shared_examples.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/resolvers/ungrouped_sub_aggregation_shared_examples.rb index 37099037..1555d68e 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/resolvers/ungrouped_sub_aggregation_shared_examples.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/aggregation/resolvers/ungrouped_sub_aggregation_shared_examples.rb @@ -253,7 +253,7 @@ module Aggregation }] end - it "ignores an empty filter" do + it "treats an empty filter treating as `true`" do aggs = { "target:seasons_nested" => {"doc_count" => 423, "meta" => outer_meta} } 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 acf71ccd..1b5b3b03 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 @@ -378,13 +378,13 @@ class GraphQL ]}}) end - it "is ignored when `null` is passed" 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 ignored when `[]` is passed" 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 "ignores `count` filter predicates that have a `nil` or `{}` value" 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 @@ -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,19 +1304,46 @@ 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 "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 "is ignored when the inner filter is also ignored" do + 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 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"]}, @@ -1320,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}, @@ -1365,6 +1399,24 @@ def expect_script_query_with_params(query, expected_params) expect(datastore_body_of(query)).to filter_datastore_with(terms: {"name" => []}) end + 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 an 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 "does not prune out `any_of: []` to be consistent with `equal_to_any_of: []`, instead providing an 'always false' condition to achieve the same behavior" do query = new_query(filter: { "age" => {"gt" => 18}, @@ -1377,12 +1429,20 @@ 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 "applies no filtering to an `any_of` composed entirely of empty predicates" do query = new_query(filter: { "age" => {"any_of" => [{"gt" => nil}, {"lt" => nil}]} }) - expect(datastore_body_of(query)).to filter_datastore_with(always_false_condition) + expect(datastore_body_of(query)).to not_filter_datastore_at_all + end + + it "applies no filtering for an `any_of` composed of an empty predicate and non empty predicate" do + query = new_query(filter: { + "any_of" => [{"age" => {}}, {"equal_to_any_of" => [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 @@ -1401,6 +1461,38 @@ 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 + + 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 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..7fac704b 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 @@ -170,7 +170,7 @@ class GraphQL expect(parts).to eq [] end - it "ignores `nil` when `equal_to_any_of` includes `nil` with other timestamps" 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 "ignores a `nil` value for a `#{operator}` filter" 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,11 +364,31 @@ 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 indices 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 + + # TODO: Change behaviour so no indices 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 + 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..19358886 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 @@ -253,12 +253,34 @@ 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 => []`. + # Updated references of ignore and prune to use language such as "treated ... as `true`" + 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 + # TODO: Change behaviour so no shards are matched when given `anyOf => {anyOf => []}` + # Updated references of ignore and prune to use language such as "treated ... as `true`" + 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 + 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"], diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/sub_aggregations_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/sub_aggregations_spec.rb index bd093b57..43c57ac1 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/sub_aggregations_spec.rb +++ b/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 "ignores empty filters" 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} diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_adapter/filters_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_adapter/filters_spec.rb index 6e75b556..cd393ab0 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_adapter/filters_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_adapter/filters_spec.rb @@ -533,7 +533,7 @@ class QueryAdapter end describe "`null` leaves" do - it "ignores `filter: null`" do + it "treats `filter: null` as true" do query = datastore_query_for(:Query, :components, <<~QUERY) query { components(filter: null) { @@ -547,7 +547,7 @@ class QueryAdapter expect(query.filters).to contain_exactly(exclude_incomplete_docs_filter) end - it "ignores a `field: null` filter since it will get pruned" do + it "treats `field: null` filter as `true`" do query = datastore_query_for(:Query, :components, <<~QUERY) query { components(filter: {cost: null}) { @@ -579,7 +579,7 @@ class QueryAdapter expect(query.filters).to contain_exactly({"cost" => nil, "name" => {"equal_to_any_of" => ["thingy"]}}) end - it "ignores a `field: {predicate: null}` filter since it will get pruned" do + it "treats `field: {predicate: null}` filter as `true`" do query = datastore_query_for(:Query, :components, <<~QUERY) query { components(filter: {cost: {equal_to_any_of: null}}) { @@ -614,7 +614,7 @@ class QueryAdapter }) end - it "ignores a `null` filter since it will get pruned" do + it "treats `null` filter as `true`" do query = datastore_query_for(:Query, :components, <<~QUERY) query { components(filter: {options: null}) { @@ -649,7 +649,7 @@ class QueryAdapter }) end - it "ignores a `parent_field: {child_field: null}` filter since it will get pruned" do + it "treats `parent_field: {child_field: null}` as true" do query = datastore_query_for(:Query, :components, <<~QUERY) query { components(filter: {options: {size: null}}) { @@ -684,7 +684,7 @@ class QueryAdapter }) end - it "ignores a `parent_field: {child_field: {predicate: null}}` filter since it will get pruned" do + it "treats `parent_field: {child_field: {predicate: null}}` as true" do query = datastore_query_for(:Query, :components, <<~QUERY) query { components(filter: {options: {size: {equal_to_any_of: null}}}) { @@ -932,7 +932,7 @@ class QueryAdapter ) end - it "includes the incomplete doc exclusion filter when there are no sub-clauses, because the filter is ignored" do + it "includes the incomplete doc exclusion filter when there are no sub-clauses, because the filter is treated as `false` for being empty" do query = datastore_query_for(:Query, :components, <<~QUERY) query { components(filter: {any_of: []}) { From 571c2fd5acf988046112e98bef08ddad55dbafca Mon Sep 17 00:00:00 2001 From: Myron Marston Date: Sun, 10 Nov 2024 23:31:29 -0800 Subject: [PATCH 5/7] Refactor: simplify and optimize `ElasticGraph::GraphQL::Schema`. - Avoid calling `::GraphQL::Schema#types` repeatedly. As reported in rmosolgo/graphql-ruby#5154, accessing `#types` repeatedly can be quite slow (at least on 2.4.0 - 2.4.2). While it's been optimized in 2.4.3, there's no need to access it more than once. Previously, we accessed it in `#lookup_type_by_name`; instead we can just use the type objects as we iterate over `::GraphQL::Schema#types` a single time. - Remove unused `#defined_types` method. - Inline the old `#lookup_type_by_name` logic into `#type_named`. --- .../lib/elastic_graph/graphql/schema.rb | 31 +++++++------------ .../unit/elastic_graph/graphql/schema_spec.rb | 15 --------- 2 files changed, 12 insertions(+), 34 deletions(-) diff --git a/elasticgraph-graphql/lib/elastic_graph/graphql/schema.rb b/elasticgraph-graphql/lib/elastic_graph/graphql/schema.rb index 19956248..354f2990 100644 --- a/elasticgraph-graphql/lib/elastic_graph/graphql/schema.rb +++ b/elasticgraph-graphql/lib/elastic_graph/graphql/schema.rb @@ -29,7 +29,7 @@ class Schema scalar_types.to_set.union(introspection_types) ) - attr_reader :element_names, :defined_types, :config, :graphql_schema, :runtime_metadata + attr_reader :element_names, :config, :graphql_schema, :runtime_metadata def initialize( graphql_schema_string:, @@ -53,7 +53,6 @@ def initialize( ) end - @types_by_name = Hash.new { |hash, key| hash[key] = lookup_type_by_name(key) } @build_resolver = build_resolver # Note: as part of loading the schema, the GraphQL gem may use the resolver (such @@ -68,7 +67,7 @@ def initialize( # Pre-load all defined types so that all field extras can get configured as part # of loading the schema, before we execute the first query. - @defined_types = build_defined_types_array(@graphql_schema) + @types_by_name = build_types_by_name end def type_from(graphql_type) @@ -80,7 +79,11 @@ def type_from(graphql_type) # get type objects for wrapped types, but you need to get it from a field object of that # type. def type_named(type_name) - @types_by_name[type_name.to_s] + @types_by_name.fetch(type_name.to_s) + rescue KeyError => e + msg = "No type named #{type_name} could be found" + msg += "; Possible alternatives: [#{e.corrections.join(", ").delete('"')}]." if e.corrections.any? + raise Errors::NotFoundError, msg end def document_type_stored_in(index_definition_name) @@ -103,7 +106,7 @@ def enum_value_named(type_name, enum_value_name) # The list of user-defined types that are indexed document types. (Indexed aggregation types will not be included in this.) def indexed_document_types - @indexed_document_types ||= defined_types.select(&:indexed_document?) + @indexed_document_types ||= @types_by_name.values.select(&:indexed_document?) end def to_s @@ -128,24 +131,14 @@ def resolver def_delegators :resolver, :call, :resolve_type, :coerce_input, :coerce_result end - def lookup_type_by_name(type_name) - type_from(@graphql_schema.types.fetch(type_name)) - rescue KeyError => e - msg = "No type named #{type_name} could be found" - msg += "; Possible alternatives: [#{e.corrections.join(", ").delete('"')}]." if e.corrections.any? - raise Errors::NotFoundError, msg - end - def resolver @resolver ||= @build_resolver.call(self) end - def build_defined_types_array(graphql_schema) - graphql_schema - .types - .values - .reject { |t| BUILT_IN_TYPE_NAMES.include?(t.graphql_name) } - .map { |t| type_named(t.graphql_name) } + def build_types_by_name + graphql_schema.types.transform_values do |graphql_type| + @types_by_graphql_type[graphql_type] + end end def indexed_document_types_by_index_definition_name diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema_spec.rb index c15fe71d..43ce5a61 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema_spec.rb @@ -70,21 +70,6 @@ class GraphQL end end - describe "#defined_types" do - it "returns a list containing all explicitly defined types (excluding built-ins)" do - schema = define_schema do |s| - s.enum_type "Options" do |t| - t.value "firstOption" - end - s.object_type "Color" - end - - expect(schema.defined_types).to all be_a Schema::Type - expect(schema.defined_types.map(&:name)).to include(:Options, :Color, :Query) - .and exclude(:Int, :Float, :Boolean, :String, :ID) - end - end - describe "#indexed_document_types" do it "returns a list containing all types defined as indexed types" do schema = define_schema do |s| From 97c9ca09486fbdc539eb20cdecca8bcdb064dc9f Mon Sep 17 00:00:00 2001 From: Myron Marston Date: Wed, 6 Nov 2024 16:49:59 -0800 Subject: [PATCH 6/7] Upgrade GraphQL gem to `~> 2.4.3`. --- elasticgraph-apollo/elasticgraph-apollo.gemspec | 2 +- elasticgraph-graphql/elasticgraph-graphql.gemspec | 2 +- elasticgraph-graphql/lib/elastic_graph/graphql.rb | 8 +++++++- elasticgraph-graphql/sig/graphql_gem.rbs | 3 +++ .../elasticgraph-query_registry.gemspec | 2 +- .../unit/elastic_graph/query_registry/rake_tasks_spec.rb | 2 +- .../elasticgraph-schema_definition.gemspec | 2 +- 7 files changed, 15 insertions(+), 6 deletions(-) diff --git a/elasticgraph-apollo/elasticgraph-apollo.gemspec b/elasticgraph-apollo/elasticgraph-apollo.gemspec index b2ba1ee0..f738e85e 100644 --- a/elasticgraph-apollo/elasticgraph-apollo.gemspec +++ b/elasticgraph-apollo/elasticgraph-apollo.gemspec @@ -13,7 +13,7 @@ ElasticGraphGemspecHelper.define_elasticgraph_gem(gemspec_file: __FILE__, catego spec.add_dependency "elasticgraph-graphql", eg_version spec.add_dependency "elasticgraph-support", eg_version - spec.add_dependency "graphql", "~> 2.3.19" + spec.add_dependency "graphql", "~> 2.4.3" spec.add_dependency "apollo-federation", "~> 3.8" # Note: technically, this is not purely a development dependency, but since `eg-schema_def` diff --git a/elasticgraph-graphql/elasticgraph-graphql.gemspec b/elasticgraph-graphql/elasticgraph-graphql.gemspec index a4893548..79ede4d0 100644 --- a/elasticgraph-graphql/elasticgraph-graphql.gemspec +++ b/elasticgraph-graphql/elasticgraph-graphql.gemspec @@ -13,7 +13,7 @@ ElasticGraphGemspecHelper.define_elasticgraph_gem(gemspec_file: __FILE__, catego spec.add_dependency "elasticgraph-datastore_core", eg_version spec.add_dependency "elasticgraph-schema_artifacts", eg_version - spec.add_dependency "graphql", "~> 2.3.19" + spec.add_dependency "graphql", "~> 2.4.3" spec.add_development_dependency "elasticgraph-admin", eg_version spec.add_development_dependency "elasticgraph-elasticsearch", eg_version diff --git a/elasticgraph-graphql/lib/elastic_graph/graphql.rb b/elasticgraph-graphql/lib/elastic_graph/graphql.rb index 42605acd..2f40f57c 100644 --- a/elasticgraph-graphql/lib/elastic_graph/graphql.rb +++ b/elasticgraph-graphql/lib/elastic_graph/graphql.rb @@ -142,7 +142,13 @@ def datastore_query_builder def graphql_gem_plugins @graphql_gem_plugins ||= begin require "graphql" - {::GraphQL::Dataloader => {}} + { + # We depend on this to avoid N+1 calls to the datastore. + ::GraphQL::Dataloader => {}, + # This is new in the graphql-ruby 2.4 release, and will be required in the future. + # We pass `preload: true` because the way we handle the schema depends on it being preloaded. + ::GraphQL::Schema::Visibility => {preload: true} + } end end diff --git a/elasticgraph-graphql/sig/graphql_gem.rbs b/elasticgraph-graphql/sig/graphql_gem.rbs index 802b6409..b54adc21 100644 --- a/elasticgraph-graphql/sig/graphql_gem.rbs +++ b/elasticgraph-graphql/sig/graphql_gem.rbs @@ -168,6 +168,9 @@ module GraphQL class Printer def self.print_schema: (Schema, **untyped) -> ::String end + + class Visibility + end end module StaticValidation diff --git a/elasticgraph-query_registry/elasticgraph-query_registry.gemspec b/elasticgraph-query_registry/elasticgraph-query_registry.gemspec index ce987cbd..be74a2aa 100644 --- a/elasticgraph-query_registry/elasticgraph-query_registry.gemspec +++ b/elasticgraph-query_registry/elasticgraph-query_registry.gemspec @@ -14,7 +14,7 @@ ElasticGraphGemspecHelper.define_elasticgraph_gem(gemspec_file: __FILE__, catego spec.add_dependency "elasticgraph-graphql", eg_version spec.add_dependency "elasticgraph-support", eg_version - spec.add_dependency "graphql", "~> 2.3.19" + spec.add_dependency "graphql", "~> 2.4.3" spec.add_dependency "rake", "~> 13.2" spec.add_development_dependency "elasticgraph-elasticsearch", eg_version diff --git a/elasticgraph-query_registry/spec/unit/elastic_graph/query_registry/rake_tasks_spec.rb b/elasticgraph-query_registry/spec/unit/elastic_graph/query_registry/rake_tasks_spec.rb index e8f887b2..348166ee 100644 --- a/elasticgraph-query_registry/spec/unit/elastic_graph/query_registry/rake_tasks_spec.rb +++ b/elasticgraph-query_registry/spec/unit/elastic_graph/query_registry/rake_tasks_spec.rb @@ -116,7 +116,7 @@ module QueryRegistry For client `client_bob`: - CountComponents.graphql (2 operations): - CountComponents: 🛑. Got 2 validation errors: - 1) Field 'total_edge_count2' doesn't exist on type 'ComponentConnection' + 1) Field 'total_edge_count2' doesn't exist on type 'ComponentConnection' (Did you mean `total_edge_count`?) path: query CountComponents.components.total_edge_count2 source: query_registry/client_bob/CountComponents.graphql:3:5 code: undefinedField diff --git a/elasticgraph-schema_definition/elasticgraph-schema_definition.gemspec b/elasticgraph-schema_definition/elasticgraph-schema_definition.gemspec index 76fb2045..0128c35b 100644 --- a/elasticgraph-schema_definition/elasticgraph-schema_definition.gemspec +++ b/elasticgraph-schema_definition/elasticgraph-schema_definition.gemspec @@ -16,7 +16,7 @@ ElasticGraphGemspecHelper.define_elasticgraph_gem(gemspec_file: __FILE__, catego spec.add_dependency "elasticgraph-json_schema", eg_version spec.add_dependency "elasticgraph-schema_artifacts", eg_version spec.add_dependency "elasticgraph-support", eg_version - spec.add_dependency "graphql", "~> 2.3.19" + spec.add_dependency "graphql", "~> 2.4.3" spec.add_dependency "rake", "~> 13.2" spec.add_development_dependency "elasticgraph-admin", eg_version From faf71e667287465cbbd9a115066e61eeb26dc403 Mon Sep 17 00:00:00 2001 From: Myron Marston Date: Tue, 12 Nov 2024 10:48:06 -0800 Subject: [PATCH 7/7] Restore `GraphQL::Schema#defined_types`. I removed it in #31 because ElasticGraph didn't rely on it or need it. However, I discovered that Block has an internal extension library that relies on it, and it's a generally useful API to offer for extension libraries. This restores it (but with a simplified implementation). --- .../lib/elastic_graph/graphql/schema.rb | 6 +++++- .../unit/elastic_graph/graphql/schema_spec.rb | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/elasticgraph-graphql/lib/elastic_graph/graphql/schema.rb b/elasticgraph-graphql/lib/elastic_graph/graphql/schema.rb index 354f2990..6cd69d7e 100644 --- a/elasticgraph-graphql/lib/elastic_graph/graphql/schema.rb +++ b/elasticgraph-graphql/lib/elastic_graph/graphql/schema.rb @@ -106,7 +106,11 @@ def enum_value_named(type_name, enum_value_name) # The list of user-defined types that are indexed document types. (Indexed aggregation types will not be included in this.) def indexed_document_types - @indexed_document_types ||= @types_by_name.values.select(&:indexed_document?) + @indexed_document_types ||= defined_types.select(&:indexed_document?) + end + + def defined_types + @defined_types ||= @types_by_name.except(*BUILT_IN_TYPE_NAMES).values end def to_s diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema_spec.rb index 43ce5a61..9383437c 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema_spec.rb @@ -70,6 +70,22 @@ class GraphQL end end + describe "#defined_types" do + # Note: `defined_type` isn't used internally by ElasticGraph itself, but it's exposed for use by extensions. + it "returns a list containing all explicitly defined types (excluding built-ins)" do + schema = define_schema do |s| + s.enum_type "Options" do |t| + t.value "firstOption" + end + s.object_type "Color" + end + + expect(schema.defined_types).to all be_a Schema::Type + expect(schema.defined_types.map(&:name)).to include(:Options, :Color, :Query) + .and exclude(:Int, :Float, :Boolean, :String, :ID) + end + end + describe "#indexed_document_types" do it "returns a list containing all types defined as indexed types" do schema = define_schema do |s|