Rename document_type_stored_in → document_types_stored_in returning Set#1159
Merged
myronmarston merged 5 commits intoblock:mainfrom Apr 30, 2026
Merged
Conversation
Changes indexed_document_types_by_index_definition_name to accumulate all indexed document types per index into a Set (was: store only the first type seen via ||=). This is necessary for correctness with index inheritance, where multiple concrete types share the same index and all need to be visible to callers like non_subtypes_in_shared_index. Renames document_type_stored_in → document_types_stored_in to return the full Set. Updates the two callers: - graphql_adapter_builder: .first is safe — this branch is only reached for individually-indexed types, which always have exactly one type per index (no __typename in the document means no shared index). - search_response_adapter_builder: use __typename from the document to resolve the concrete type directly when present; shared-index types carry __typename so field paths in all_highlights resolve to the right type. Falls back to .first for individually-indexed types. Generated with Claude Code
marcdaniels-toast
requested review from
BrianSigafoos-SQ,
ayousufi,
bsorbo,
jwils,
jwondrusch and
myronmarston
as code owners
Required by non_subtypes_in_shared_index (on the index inheritance branch), which needs to enumerate all types sharing an index when deciding whether a __typename filter is needed on abstract type queries. Generated with Claude Code
myronmarston
requested changes
Apr 29, 2026
Collaborator
myronmarston
left a comment
myronmarston
left a comment
There was a problem hiding this comment.
Thanks for spinning this off!
- Make indexed_document_types_by_index_definition_name private; public callers should use document_types_stored_in instead - Add unit test for multi-type index inheritance case - Add DistributionChannel type hierarchy to test schema to support acceptance test for __typename-aware all_highlights resolution - Add acceptance test proving all_highlights resolves against the concrete type for documents in a shared-index hierarchy - Update hidden_types_spec to account for new types - Add distribution_channels and physical_stores index definitions to config settings Generated with Claude Code
…es_by_index_definition_name Generated with Claude Code
myronmarston
approved these changes
Apr 29, 2026
Contributor
Author
|
@myronmarston I pushed one more commit that improves the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR was created as a companion to #1150 (an initial step). It paves the way for the
non_subtypes_in_shared_indexmethod by ensuring the GraphQL schema correctly tracks all document types per index, rather than only the first one seen.indexed_document_types_by_index_definition_namenow returnsSetper indexPreviously, the hash stored only the first type seen for a given index name (via
||=). With index inheritance, multiple concrete types can share the same index — so the old approach would non-deterministically drop all but one.Changed to accumulate all indexed document types per index into a
Set. This is required fornon_subtypes_in_shared_indexto work correctly: that method uses this hash to enumerate all types sharing an index when deciding whether a__typenamefilter is needed on abstract type queries. With the old approach, other types sharing the same index could be silently omitted, leading to incorrect or missing__typenamefilters.document_type_stored_in→document_types_stored_inRenamed to reflect that the method now returns a
Set[Type]rather than a singleType. Updated the two callers:graphql_adapter_builder(resolve_type): uses.first— safe here because this branch is only reached for individually-indexed types (no__typenamein the document means no shared index), so the set always contains exactly one type.search_response_adapter_builder(all_highlights): now prefers__typenamefrom the document source when present. Documents stored in a shared index carry__typenameto identify their concrete type; using it directly ensures highlight field paths are resolved against the right type rather than whichever abstract or sibling type.firstmight return. Falls back to.firstfor individually-indexed types.Test plan
search_response_adapter_builder_spec.rbverifies thatall_highlightsuses__typenameto resolve the concrete type: a hit from theretailersindex with__typename: "OnlineStore"correctly resolves a highlight onurl(anOnlineStore-specific field absent from theRetailerinterface — without the fix, the wrong type would be used and the highlight would be silently dropped)all_highlightsacceptance test tosearch_spec.rbfor shared-index types that would fail without thesearch_response_adapter_builderchange made here. That acceptance test requires additions to the stock schema types and remaining index inheritance support that I didn't want to cloud this more focused PR step.