Add query-time __typename filtering and end-to-end tests for index inheritance

[marcdaniels-toast]

Updates the graphql gem to handle abstract types whose subtypes share an inherited index: when querying an abstract type, a new AbstractTypeFilter query adapter injects a __typename filter to exclude sibling types that share the same index. The filter correctly handles mixed hierarchies where some subtypes use a shared inherited index while others have their own dedicated index.

Also fixes RequestedFields#requested_fields_under to use unwrap_fully when checking whether a type is abstract, so that __typename is correctly added to requested_fields when the abstract type is wrapped in a list (e.g. the nodes field in a relay connection returns [NamedInventor!]!). Without this fix, querying named_inventors { nodes { ... on Person { name } } } would return empty results.

Adds end-to-end acceptance tests covering the full interface index inheritance flow, and extends the test schema with the DistributionChannel/Retail/Store interface hierarchy and concrete types (PhysicalStore, OnlineStore, ThirdPartyWholesale) to exercise multi-level inheritance and mixed shared/dedicated index scenarios.

Improves YARD documentation on HasIndices#index, API#interface_type, ImplementsInterfaces#implements, and HasIndices#directly_queryable? to document index inheritance behavior, shared-index examples, and the dedicated-index override pattern.

Adds a blurb about index inheritance to ai-memory/README.md for use by agents.

Fixes #1029

[myronmarston]

This is really awesome!

[myronmarston]

It's not clear to me that this change is needed. I reverted this change and ran script/flatware_rspec elasticgraph-graphql...and nothing failed. Can you come up with some test changes that require your change here to pass? Ideally:

• There'd be something in the end-to-end acceptance test that fails w/o this fix.
• There'd also be a test in elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_adapter/requested_fields_spec.rb that only passes with this change.

If we can't come up with tests that need this change then I'd prefer to revert it.

[marcdaniels-toast]

This change is needed specifically because of index inheritance. Before index inheritance, every concrete subtype of a queryable abstract type had its own dedicated index, so the index itself identified the type and __typename never needed to be fetched from the document. This unwrap_fully fix was therefore unnecessary before index inheritance. With index inheritance, concrete subtypes can share their parent's index without declaring their own. In that case __typename is stored in documents and must be fetched to resolve the concrete type. The nodes field on a relay connection returns [NamedInventor!]! — a list-wrapped type. The original &.type&.abstract? check doesn't unwrap lists, so it returned false and __typename was never added to requested_fields. unwrap_fully strips the list and non-null wrappers to reach NamedInventor, correctly identifying it as abstract. Added a unit test for this in "requests __typename when using nodes on an abstract indexed type" in requested_fields_spec.rb.

As an aside, I noticed this inadvertently fixes the abstract type case of #1055 (parts { nodes { __typename } }). The fix works because individual_docs_needed is set to true whenever requested_fields is non-empty — so adding __typename to requested_fields is sufficient to produce a non-zero size. The fact that the concrete type case (addresses { nodes { __typename } }) remains broken even with edges { node { __typename } } suggests that user-selected __typename is being actively dropped from requested_fields somewhere (likely via graphql_dynamic_field?), leaving an empty set regardless of the query shape. That seems worth investigating as part of a proper fix for #1055.

[myronmarston]

As an aside, I noticed this inadvertently fixes the abstract type case of #1055 (parts { nodes { __typename } }).

Can you add unit and acceptance coverage for that case as part of this PR?

[marcdaniels-toast]

Thanks for the suggestion about using Claude to run exploratory test queries to try to find buggy edge cases. It found the aggregation type gap and the fix for that is included here.

A second thing it found is this:

We noticed that sourced_from doesn't support subtype-specific fields on index-inheriting types. The issue is that fields_with_sources operates via indexing_fields_by_name_in_index, which aggregates all fields at the index-owner level — so a sourced_from field on Person is seen as belonging to NamedInventor, and the relationship must be defined on NamedInventor to work. Any attempt to use a Person-specific relationship raises a schema error.

The natural workaround is to give the subtype its own dedicated index (overriding the inherited one), which restores normal sourced_from behavior. I think of this as less of a workaround and more of a good reason to chose a dedicated index for a concrete type. Do you think this is worth a separate issue filing and eventual fix, or is the dedicated-index "workaround" sufficient (perhaps with a clearer error message pointing users toward it)?

[myronmarston]

There are a few tweaks I'd recommend here:

diff --git a/elasticgraph-graphql/lib/elastic_graph/graphql.rb b/elasticgraph-graphql/lib/elastic_graph/graphql.rb
index 3b12810a..3700c250 100644
--- a/elasticgraph-graphql/lib/elastic_graph/graphql.rb
+++ b/elasticgraph-graphql/lib/elastic_graph/graphql.rb
@@ -201,7 +201,7 @@ module ElasticGraph
         schema_element_names = runtime_metadata.schema_element_names
 
         [
-          GraphQL::QueryAdapter::AbstractTypeFilter.new,
+          GraphQL::QueryAdapter::AbstractTypeFilter.new(schema_element_names),
           GraphQL::QueryAdapter::Pagination.new(schema_element_names: schema_element_names),
           GraphQL::QueryAdapter::Filters.new(
             schema_element_names: schema_element_names,
diff --git a/elasticgraph-graphql/lib/elastic_graph/graphql/query_adapter/abstract_type_filter.rb b/elasticgraph-graphql/lib/elastic_graph/graphql/query_adapter/abstract_type_filter.rb
index a0c32a47..be7f1f42 100644
--- a/elasticgraph-graphql/lib/elastic_graph/graphql/query_adapter/abstract_type_filter.rb
+++ b/elasticgraph-graphql/lib/elastic_graph/graphql/query_adapter/abstract_type_filter.rb
@@ -20,6 +20,10 @@ module ElasticGraph
       # Subtypes with a dedicated index will not have `__typename` in their documents — the index
       # itself identifies the type — so `nil` is included to allow those documents through.
       class AbstractTypeFilter
+        def initialize(schema_element_names)
+          @equal_to_any_of = schema_element_names.equal_to_any_of
+        end
+
         def call(field:, query:, args:, lookahead:, context:)
           type = field.type.unwrap_fully
 
@@ -27,12 +31,11 @@ module ElasticGraph
           # apply the same __typename scoping as we do for document queries.
           doc_type = type.indexed_aggregation? ? type.aggregation_source_type : type
 
-          return query unless doc_type.abstract?
-          return query unless doc_type.non_subtypes_in_shared_index.any?
+          return query if doc_type.non_subtypes_in_shared_index.empty?
 
           subtypes = doc_type.subtypes # Note: subtypes returns all concrete subtypes at any depth
           query.merge_with(internal_filters: [{
-            "__typename" => {query.schema_element_names.equal_to_any_of => [nil] + subtypes.map(&:name)}
+            "__typename" => {@equal_to_any_of => [nil] + subtypes.map(&:name)}
           }])
         end
       end

• Might as well store @equal_to_any_of in the constructor; you can then use it each time call is called. Should be slightly more efficient than query.schema_element_names.equal_to_any_of.
• I don't think it's possible for non_subtypes_in_shared_index to be non-empty unless it's also abstract, so I don't think we need to check both.
• While unless is handy, it can lead to confusion and I think it's best to generally only use it when using if would require negation (via !).
  • In addition, empty? and any? are not strict inverses (although they can be used that way in some cases). any? is primarily designed to be used with a predicate block (e.g. has_odd = numbers.any? { |n| n.odd? }). As a convenience it allows you to omit the block, and array.any? is the same as array.any? { |elem| elem } which means the "falsey" values (nil and false) can cause .any? to act in a surprising way if you're thinking of it as an inverse of .empty?:

irb(main):001> array = [true, 3]
=> [true, 3]
irb(main):002> array.any?
=> true
irb(main):003> array.empty?
=> false
irb(main):004> array = [false, nil]
=> [false, nil]
irb(main):005> array.any?
=> false
irb(main):006> array.empty?
=> false

In this case, non_subtypes_in_shared_index doesn't have any nil or false values in it so unless doc_type.non_subtypes_in_shared_index.any? worked fine, but I prefer if doc_type.non_subtypes_in_shared_index.empty? to avoid any potential confusion.

[myronmarston]

It'd be good to cover the new aggregation_source_type and non_subtypes_in_shared_index methods with unit tests here:

https://github.com/block/elasticgraph/blob/3d5b66567940084157b85edf4eaf4daf5507f6cc/elasticgraph-graphql/spec/unit/elastic_graph/graphql/schema/type_spec.rb

[myronmarston]

👍

It'd be good to also have the acceptance test cover this. I played around with it locally and I think something like this works:

diff --git a/elasticgraph-graphql/spec/acceptance/search_spec.rb b/elasticgraph-graphql/spec/acceptance/search_spec.rb
index c4862897..6bdc46c3 100644
--- a/elasticgraph-graphql/spec/acceptance/search_spec.rb
+++ b/elasticgraph-graphql/spec/acceptance/search_spec.rb
@@ -745,6 +745,12 @@ module ElasticGraph
         stores = list_stores_with(*store_fragments)
         expect(stores.map { |s| s["__typename"] }).to contain_exactly(*store_typenames)
 
+        # Using `nodes` (instead of `edges { node }`) also works. This exercises the `nodes`
+        # code path where the field type is list-wrapped (e.g. `[Store!]!`), requiring
+        # `unwrap_fully` before checking `abstract?` for __typename inclusion.
+        stores_via_nodes = list_stores_via_nodes_with(*store_fragments)
+        expect(stores_via_nodes.map { |s| s["__typename"] }).to contain_exactly(*store_typenames)
+
         # Filters apply within the correct scope at each level.
         # At distribution_channels: active=false matches only wholesale2.
         inactive = list_distribution_channels_with(*all_channel_fragments, filter: {active: {equal_to_any_of: [false]}})
@@ -1603,6 +1609,10 @@ module ElasticGraph
         query_abstract_type_with("stores", *fragments, **query_args).dig("data", "stores", "edges").map { |e| e.fetch("node") }
       end
 
+      def list_stores_via_nodes_with(*fragments, **query_args)
+        query_abstract_type_via_nodes_with("stores", *fragments, **query_args).dig("data", "stores", "nodes")
+      end
+
       def query_abstract_type_with(field, *fragments, allow_errors: false, **query_args)
         fragment_string = fragments.join("\n")
         call_graphql_query(<<~QUERY, allow_errors: allow_errors)
@@ -1623,6 +1633,19 @@ module ElasticGraph
         QUERY
       end
 
+      def query_abstract_type_via_nodes_with(field, *fragments, **query_args)
+        fragment_string = fragments.join("\n")
+        call_graphql_query(<<~QUERY)
+          query {
+            #{field}#{graphql_args(query_args)} {
+              nodes {
+                #{fragment_string}
+              }
+            }
+          }
+        QUERY
+      end
+
       def list_widgets_by_nodes_with(fieldname, **query_args)
         query_widgets_by_nodes_with(fieldname, **query_args).dig("data", "widgets", "nodes")
       end

[myronmarston]

• The method name suggests this is just for aggregation types. But there's nothing aggregation-specific about the logic--I expect that it would work for any type.
  • Can we rename it to source_type to not suggest it's just for aggregations?
  • Can we document what it's return value is for a non-derived type? (Also, when you add specs for it, please cover that case!).
• Can we use @source_type ||= to memoize the result so that on repeated calls it just returns the memoized value?
• Can we update

  elasticgraph/elasticgraph-graphql/lib/elastic_graph/graphql/schema/type.rb

  Line 79 in 3d5b665

  @schema.type_named(@object_runtime_metadata.source_type).search_index_definitions

  to use this method?

[myronmarston]

Suggested change

	@schema.indexed_document_types_by_index_definition_name.fetch(index_def.name, [])
	@schema.indexed_document_types_by_index_definition_name.fetch(index_def.name)

I think something is wrong if indexed_document_types_by_index_definition_name lacks an entry for index_def.name so I'd rather not silently tolerate that case.

[myronmarston]

There are multiple abstract types at play here so saying "the abstract type" is unclear, particularly after the prior test which had the root abstract type (which is more clear as there's only one root, right?).

[myronmarston]

It looks like this is coverage for Type#non_subtypes_in_shared_index. Can it ve moved into type_spec.rb?

[myronmarston]

Thanks for bulking up our YARD docs!

Have you run be rake site:serve to confirm all the updated YARD docs render as expected? I've often found that YARD docs can be fiddly and I don't trust that they render correctly without viewing them.

[myronmarston]

While it's conventional to put implements calls at the top of the type definition, this comment suggests that it has to go before the index call in order for the index call to override the supertype index. Is that true? If index is called before implements it wont override it?

[myronmarston]

As an aside, I noticed this inadvertently fixes the abstract type case of #1055 (parts { nodes { __typename } }).

Can you add unit and acceptance coverage for that case as part of this PR?