From a6b305efd135c4a5fbc7bce70051024b79428342 Mon Sep 17 00:00:00 2001 From: ellisandrews-toast Date: Wed, 3 Jun 2026 16:58:00 -0400 Subject: [PATCH 1/5] Add parent_relationship API and RelationshipChainResolver with validations --- .../schema_definition/factory.rb | 5 +- .../indexing/relationship_chain_resolver.rb | 209 ++++++++++++++++++ .../sourced_from_update_targets_resolver.rb | 27 ++- .../schema_elements/relationship.rb | 79 ++++++- .../schema_elements/type_with_subfields.rb | 3 +- .../schema_definition/factory.rbs | 3 +- .../indexing/relationship_chain_resolver.rbs | 66 ++++++ .../sourced_from_update_targets_resolver.rbs | 1 + .../schema_elements/relationship.rbs | 18 +- .../update_targets_spec.rb | 207 +++++++++++++++++ 10 files changed, 610 insertions(+), 8 deletions(-) create mode 100644 elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rb create mode 100644 elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rbs diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/factory.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/factory.rb index 786fbf82a..366a923fe 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/factory.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/factory.rb @@ -304,13 +304,14 @@ def new_field_source(relationship_name:, field_path:) end @@field_source_new = prevent_non_factory_instantiation_of(SchemaElements::FieldSource) - def new_relationship(field, cardinality:, related_type:, foreign_key:, direction:) + def new_relationship(field, cardinality:, related_type:, foreign_key:, direction:, indexing_only:) @@relationship_new.call( field, cardinality: cardinality, related_type: related_type, foreign_key: foreign_key, - direction: direction + direction: direction, + indexing_only: indexing_only ) end @@relationship_new = prevent_non_factory_instantiation_of(SchemaElements::Relationship) diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rb new file mode 100644 index 000000000..b177d0e07 --- /dev/null +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rb @@ -0,0 +1,209 @@ +# Copyright 2024 - 2026 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 + +require "elastic_graph/errors" + +module ElasticGraph + module SchemaDefinition + module Indexing + # The result of resolving a relationship chain. + # + # @private + ResolvedRelationshipChain = ::Data.define( + :root_indexed_type, # ObjectType - the indexed type at the root + :root_relationship, # Relationship - the root relationship (no parent_relationship) + :path_segments # Array - ordered root-to-leaf + ) + + # Describes how to navigate from a parent type into a nested child element. + # For list fields, `match_field_name` and `source_field_name` identify which element + # to update: the element where `element[match_field_name] == event[source_field_name]`. + # For non-list (object) fields, these are nil since there's no ambiguity. + # + # @private + PathSegment = ::Data.define( + :field, # Field - the field to navigate into at this level + :match_field_name, # String? - field name on the nested element to match against (nil for object fields) + :source_field_name # String? - field name on the source event providing the match value (nil for object fields) + ) + + # Resolves a chain of `parent_relationship` links from a leaf embedded type up to the + # root indexed type. Produces a `ResolvedRelationshipChain` on success, or errors + # describing what's invalid. + # + # @private + class RelationshipChainResolver + def initialize(schema_def_state:) + @schema_def_state = schema_def_state + end + + # Resolves the chain starting from `starting_relationship` (which must have a + # `parent_ref`) on `starting_type`. + # + # Returns a tuple of [resolved_chain, errors]. + # If errors is non-empty, resolved_chain will be nil. + def resolve(starting_relationship, starting_type) + errors = [] # : ::Array[::String] + path_segments = [] # : ::Array[PathSegment] + visited_type_names = Set[starting_type.name] + + current_rel, current_type = resolve_chain( + starting_relationship, starting_type, path_segments, errors, visited_type_names + ) + + return [nil, errors] if errors.any? + + # The recursion terminated because current_rel has no parent_ref — + # this is the root relationship. Validate that current_type is indexed. + unless current_type.root_document_type? + errors << "The `parent_relationship` chain from #{rel_description(starting_type, starting_relationship)} " \ + "terminates at `#{current_type.name}`, but `#{current_type.name}` is not an indexed type. " \ + "The chain must terminate at an indexed type." + return [nil, errors] + end + + resolved_chain = ResolvedRelationshipChain.new( + root_indexed_type: current_type, + root_relationship: current_rel, + path_segments: path_segments.reverse # reverse so root-to-leaf order + ) + + [resolved_chain, errors] + end + + private + + # Recursively walks from leaf to root, building path segments in reverse. + # Stops when the current relationship has no parent_ref (i.e., it's the root). + def resolve_chain(current_rel, current_type, path_segments, errors, visited_type_names) + parent_ref = current_rel.parent_ref + return [current_rel, current_type] unless parent_ref + + parent_type, parent_rel = resolve_parent_ref(current_rel, current_type, parent_ref, errors, visited_type_names) + return [current_rel, current_type] if errors.any? + + parent_type = _ = parent_type # : indexableType + parent_rel = _ = parent_rel # : SchemaElements::Relationship + + build_path_segment(current_rel, current_type, parent_type, path_segments, errors) + return [current_rel, current_type] if errors.any? + + visited_type_names.add(parent_type.name) + resolve_chain(parent_rel, parent_type, path_segments, errors, visited_type_names) + end + + # Resolves a parent_ref into the concrete parent type and relationship. + # Returns [parent_type, parent_rel] on success, or appends to errors and returns nils. + def resolve_parent_ref(current_rel, current_type, ref, errors, visited_type_names) + unless current_rel.indexing_only + errors << "#{rel_description(current_type, current_rel)} uses `parent_relationship` but is not declared with " \ + "`indexing_only: true`. Relationships with `parent_relationship` must be indexing-only." + return [nil, nil] + end + + parent_type_name = ref.type_ref.name + if visited_type_names.include?(parent_type_name) + errors << "#{rel_description(current_type, current_rel)} creates a circular `parent_relationship` chain " \ + "— `#{parent_type_name}` was already visited. The chain must terminate at a root indexed type." + return [nil, nil] + end + + parent_type = (_ = ref.type_ref.as_object_type) # : indexableType? + unless parent_type + errors << "#{rel_description(current_type, current_rel)} references parent type " \ + "`#{parent_type_name}` via `parent_relationship`, but that type does not exist. Is it misspelled?" + return [nil, nil] + end + + parent_rel = parent_type.relationships_by_name[ref.relationship_name] + unless parent_rel + errors << "#{rel_description(current_type, current_rel)} references parent relationship " \ + "`#{parent_type.name}.#{ref.relationship_name}` via `parent_relationship`, " \ + "but that relationship does not exist. Is it misspelled?" + return [nil, nil] + end + + current_source_type_name = current_rel.related_type.unwrap_non_null.name + parent_source_type_name = parent_rel.related_type.unwrap_non_null.name + unless current_source_type_name == parent_source_type_name + errors << "#{rel_description(current_type, current_rel)} relates to `#{current_source_type_name}`, " \ + "but its parent relationship `#{parent_type.name}.#{ref.relationship_name}` relates to " \ + "`#{parent_source_type_name}`. All relationships in a `parent_relationship` chain must relate to the same source type." + return [nil, nil] + end + + [parent_type, parent_rel] + end + + # Builds a PathSegment for the current level and appends it to path_segments. + # Uses the explicitly specified field name if provided, otherwise auto-discovers it. + def build_path_segment(current_rel, current_type, parent_type, path_segments, errors) + parent_ref = current_rel.parent_ref # : SchemaElements::Relationship::ParentRef + field = resolve_field(parent_ref, parent_type, current_rel, current_type, errors) + return unless field + + # For list fields, `match_field_name` and `source_field_name` identify which element + # to update. `match_field_name` is always "id" because ElasticGraph relationships join + # on `id` via foreign keys. For non-list fields, these are nil since there's no ambiguity. + path_segments << if field.type.list? + PathSegment.new( + field: field, + match_field_name: "id", + source_field_name: current_rel.foreign_key + ) + else + PathSegment.new( + field: field, + match_field_name: nil, + source_field_name: nil + ) + end + end + + def resolve_field(parent_ref, parent_type, current_rel, current_type, errors) + if parent_ref.field_name + field = parent_type.graphql_fields_by_name[parent_ref.field_name] + unless field + errors << "#{rel_description(current_type, current_rel)} references field `#{parent_type.name}.#{parent_ref.field_name}` " \ + "via `parent_relationship`, but that field does not exist." + end + field + else + find_field_by_type(parent_type, current_type, current_rel, errors) + end + end + + def find_field_by_type(parent_type, child_type, current_rel, errors) + matches = parent_type.graphql_fields_by_name.values.select do |field| + field.type.fully_unwrapped.name == child_type.name + end + + if matches.size > 1 + field_names = matches.map(&:name).join(", ") + parent_ref = current_rel.parent_ref # : SchemaElements::Relationship::ParentRef + errors << "#{rel_description(child_type, current_rel)} has an ambiguous `parent_relationship` — " \ + "`#{parent_type.name}` has multiple fields of type `#{child_type.name}` (#{field_names}). " \ + "Specify which field using the `parent_field_name:` option: " \ + "`r.parent_relationship \"#{parent_type.name}\", \"#{parent_ref.relationship_name}\", parent_field_name: \"\"`" + nil + elsif matches.empty? + errors << "#{rel_description(child_type, current_rel)} declares `#{parent_type.name}` as its parent type " \ + "via `parent_relationship`, but `#{parent_type.name}` has no field of type `#{child_type.name}`." + nil + else + matches.first + end + end + + def rel_description(type, relationship) + "`#{type.name}.#{relationship.name}`" + end + end + end + end +end diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rb index 1f2496d2f..1efac8af7 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rb @@ -7,6 +7,7 @@ # frozen_string_literal: true require "elastic_graph/errors" +require "elastic_graph/schema_definition/indexing/relationship_chain_resolver" require "elastic_graph/schema_definition/indexing/relationship_resolver" require "elastic_graph/schema_definition/indexing/update_target_resolver" @@ -48,7 +49,7 @@ def resolve_for_type(object_type, &error_reporter) fields_with_sources_by_relationship_name = sourced_fields_by_relationship_name(object_type) defined_relationships = object_type.relationships_by_name.keys - (defined_relationships | fields_with_sources_by_relationship_name.keys).filter_map do |relationship_name| + results = (defined_relationships | fields_with_sources_by_relationship_name.keys).filter_map do |relationship_name| empty_fields = [] # : ::Array[SchemaElements::Field] sourced_fields = fields_with_sources_by_relationship_name.fetch(relationship_name) { empty_fields } relationship_resolver = RelationshipResolver.new( @@ -65,6 +66,14 @@ def resolve_for_type(object_type, &error_reporter) resolve_update_target(object_type, resolved_relationship, sourced_fields, &error_reporter) end end + + # Non-indexed types may have `parent_relationship` chains that need validation. + # Indexed types use the top-level sourced_from path above instead. + if object_type.own_index_def.nil? + resolve_nested_relationships(object_type, &error_reporter) + end + + results end def resolve_update_target(object_type, resolved_relationship, sourced_fields) @@ -91,6 +100,22 @@ def resolve_update_target(object_type, resolved_relationship, sourced_fields) [resolved_relationship.related_type.name, update_target] if update_target end + def resolve_nested_relationships(object_type) + relationships_with_parent_ref = object_type.relationships_by_name.each_value.select(&:parent_ref) + return if relationships_with_parent_ref.empty? + + sourced_relationship_names = object_type.fields_with_sources.map { |f| (_ = f.source).relationship_name } + chain_resolver = RelationshipChainResolver.new(schema_def_state: @schema_def_state) + + relationships_with_parent_ref.each do |relationship| + next unless sourced_relationship_names.include?(relationship.name) + + # TODO: use resolved_chain to build nested update targets once that logic is implemented. + _resolved_chain, chain_errors = chain_resolver.resolve(relationship, object_type) + chain_errors.each { |error| yield :sourced_field, error } + end + end + def sourced_fields_by_relationship_name(object_type) if object_type.own_index_def.nil? # For now, only indexed types can have `sourced_from` fields, and resolving `fields_with_sources` on an unindexed union type diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/relationship.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/relationship.rb index 29c7ec2f0..b8f632547 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/relationship.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/relationship.rb @@ -37,24 +37,40 @@ module SchemaElements # end # end class Relationship < DelegateClass(Field) - # @dynamic related_type, hide_relationship_runtime_metadata, hide_relationship_runtime_metadata= + # @dynamic related_type, foreign_key, hide_relationship_runtime_metadata, hide_relationship_runtime_metadata=, parent_ref, indexing_only + + # @private + ParentRef = ::Data.define(:type_ref, :relationship_name, :field_name) # @return [ObjectType, InterfaceType, UnionType] the type this relationship relates to attr_reader :related_type + # @return [String] the foreign key field name (the `via` parameter) + # @private + attr_reader :foreign_key + # @private attr_accessor :hide_relationship_runtime_metadata # @private - def initialize(field, cardinality:, related_type:, foreign_key:, direction:) + attr_reader :parent_ref + + # @return [Boolean] true if this relationship is for indexing only (not exposed in GraphQL) + # @private + attr_reader :indexing_only + + # @private + def initialize(field, cardinality:, related_type:, foreign_key:, direction:, indexing_only: false) super(field) self.hide_relationship_runtime_metadata = false @cardinality = cardinality @related_type = related_type @foreign_key = foreign_key @direction = direction + @indexing_only = indexing_only @equivalent_field_paths_by_local_path = {} @additional_filter = {} + @parent_ref = nil end # Adds additional filter conditions to a relationship beyond the foreign key. @@ -136,6 +152,65 @@ def equivalent_field(path, locally_named: path) end end + # Indicates that this relationship chains through a parent relationship to reach the root indexed type. + # + # Use this API when defining relationships on embedded (non-indexed) types that need to use `sourced_from` + # on their fields. By chaining relationships through parent types, ElasticGraph can resolve the path from + # the nested type up to the root indexed type and properly update nested fields when source events arrive. + # + # @param parent_type_name [String] name of the parent type in the nesting hierarchy + # @param parent_relationship_name [String] name of the relationship on the parent type + # @param parent_field_name [String, nil] name of the field on the parent type that embeds this type. + # When omitted, auto-discovered by finding the field on the parent type whose type matches this type. + # Required when the parent type has multiple fields of this type. + # @return [void] + # + # @example Define a nested sourced_from relationship chain + # ElasticGraph.define_schema do |schema| + # schema.object_type "Team" do |t| + # t.field "id", "ID!" + # t.field "name", "String" + # t.field "players", "[Player!]!" do |f| + # f.mapping type: "nested" + # end + # t.relates_to_many "statLines", "StatLine", via: "teamId", dir: :in, indexing_only: true + # t.index "teams" do |i| + # i.has_had_multiple_sources! + # end + # end + # + # schema.object_type "Player" do |t| + # t.field "id", "ID!" + # t.field "name", "String" + # t.field "goalsScored", "Int" do |f| + # f.sourced_from "statLine", "goals" + # end + # t.relates_to_one "statLine", "StatLine", via: "playerId", dir: :in, indexing_only: true do |r| + # r.parent_relationship "Team", "statLines" + # end + # end + # + # schema.object_type "StatLine" do |t| + # t.field "id", "ID!" + # t.field "teamId", "ID" + # t.field "playerId", "ID" + # t.field "goals", "Int" + # t.index "stat_lines" + # end + # end + def parent_relationship(parent_type_name, parent_relationship_name, parent_field_name: nil) + if @parent_ref + raise Errors::SchemaError, "`parent_relationship` has been called multiple times on `#{parent_type.name}.#{name}`, " \ + "but each relationship can have only one `parent_relationship`." + end + + @parent_ref = ParentRef.new( + type_ref: schema_def_state.type_ref(parent_type_name), + relationship_name: parent_relationship_name, + field_name: parent_field_name + ) + end + # Gets the `routing_value_source` from this relationship for the given `index`, based on the configured # routing used by `index` and the configured equivalent fields. # diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/type_with_subfields.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/type_with_subfields.rb index 8f4316ccf..833d5f2db 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/type_with_subfields.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/type_with_subfields.rb @@ -573,7 +573,8 @@ def relates_to(field_name, type, via:, dir:, foreign_key_type:, cardinality:, re cardinality: cardinality, related_type: schema_def_state.type_ref(related_type).to_final_form, foreign_key: via, - direction: dir + direction: dir, + indexing_only: indexing_only ) field.relationship = relationship diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/factory.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/factory.rbs index 978085dbb..62ab9131d 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/factory.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/factory.rbs @@ -130,7 +130,8 @@ module ElasticGraph cardinality: SchemaElements::Relationship::cardinality, related_type: SchemaElements::TypeReference, foreign_key: ::String, - direction: SchemaElements::foreignKeyDirection + direction: SchemaElements::foreignKeyDirection, + indexing_only: bool ) -> SchemaElements::Relationship @@relationship_new: ::Method diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rbs new file mode 100644 index 000000000..d1206a75e --- /dev/null +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rbs @@ -0,0 +1,66 @@ +module ElasticGraph + module SchemaDefinition + module Indexing + class RelationshipChainResolver + @schema_def_state: State + + def initialize: (schema_def_state: State) -> void + def resolve: (SchemaElements::Relationship, indexableType) -> [ResolvedRelationshipChain?, ::Array[::String]] + + private + + def resolve_chain: ( + SchemaElements::Relationship, + indexableType, + ::Array[PathSegment], + ::Array[::String], + ::Set[::String] + ) -> [SchemaElements::Relationship, indexableType] + + def resolve_parent_ref: ( + SchemaElements::Relationship, + indexableType, + SchemaElements::Relationship::ParentRef, + ::Array[::String], + ::Set[::String] + ) -> [indexableType?, SchemaElements::Relationship?] + + def build_path_segment: ( + SchemaElements::Relationship, + indexableType, + indexableType, + ::Array[PathSegment], + ::Array[::String] + ) -> void + + def resolve_field: (SchemaElements::Relationship::ParentRef, indexableType, SchemaElements::Relationship, indexableType, ::Array[::String]) -> SchemaElements::Field? + def find_field_by_type: (indexableType, indexableType, SchemaElements::Relationship, ::Array[::String]) -> SchemaElements::Field? + def rel_description: (indexableType, SchemaElements::Relationship) -> ::String + end + + class ResolvedRelationshipChain + attr_reader root_indexed_type: indexableType + attr_reader root_relationship: SchemaElements::Relationship + attr_reader path_segments: ::Array[PathSegment] + + def initialize: ( + root_indexed_type: indexableType, + root_relationship: SchemaElements::Relationship, + path_segments: ::Array[PathSegment] + ) -> void + end + + class PathSegment + attr_reader field: SchemaElements::Field + attr_reader match_field_name: ::String? + attr_reader source_field_name: ::String? + + def initialize: ( + field: SchemaElements::Field, + match_field_name: ::String?, + source_field_name: ::String? + ) -> void + end + end + end +end diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rbs index 316e8a3f1..1dfab0d25 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rbs @@ -11,6 +11,7 @@ module ElasticGraph def resolve_for_type: (indexableType) { (::Symbol, ::String) -> void } -> ::Array[[::String, SchemaArtifacts::RuntimeMetadata::UpdateTarget]] def resolve_update_target: (indexableType, ResolvedRelationship, ::Array[SchemaElements::Field]) { (::Symbol, ::String) -> void } -> [::String, SchemaArtifacts::RuntimeMetadata::UpdateTarget]? + def resolve_nested_relationships: (indexableType) { (::Symbol, ::String) -> void } -> void def sourced_fields_by_relationship_name: (indexableType) -> ::Hash[::String, ::Array[SchemaElements::Field]] def raise_if_errors: (::Array[::String], ::Array[::String]) -> void end diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_elements/relationship.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_elements/relationship.rbs index ee1d03122..2c5943a59 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_elements/relationship.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_elements/relationship.rbs @@ -7,26 +7,42 @@ module ElasticGraph class Relationship < RelationshipSupertype type cardinality = :one | :many + + class ParentRef + attr_reader type_ref: TypeReference + attr_reader relationship_name: ::String + attr_reader field_name: ::String? + + def initialize: (type_ref: TypeReference, relationship_name: ::String, field_name: ::String?) -> void + end + attr_reader related_type: TypeReference + attr_reader foreign_key: ::String attr_accessor hide_relationship_runtime_metadata: bool + attr_reader parent_ref: ParentRef? + attr_reader indexing_only: bool @cardinality: cardinality @related_type: TypeReference @foreign_key: ::String @direction: foreignKeyDirection + @indexing_only: bool @equivalent_field_paths_by_local_path: ::Hash[::String, ::String] @additional_filter: ::Hash[::String, untyped] + @parent_ref: ParentRef? def initialize: ( Field, cardinality: cardinality, related_type: TypeReference, foreign_key: ::String, - direction: foreignKeyDirection + direction: foreignKeyDirection, + ?indexing_only: bool ) -> void def additional_filter: (::Hash[::String, untyped]) -> void def equivalent_field: (::String, ?locally_named: ::String) -> void + def parent_relationship: (::String, ::String, ?parent_field_name: ::String?) -> void def routing_value_source_for_index: [T] (Indexing::Index) { (::String) -> bot } -> ::String? def rollover_timestamp_value_source_for_index: [T] (Indexing::Index) { (::String) -> bot } -> ::String? def validate_equivalent_fields: (SchemaElements::FieldPath::Resolver) -> ::Array[::String] diff --git a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb index 79dd3b5da..a4e09f61c 100644 --- a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb +++ b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb @@ -1298,6 +1298,213 @@ def raise_error_about_workspace_relationship(details, sourced_fields: true) end end + describe "`parent_relationship` validations" do + it "does not raise an error for a valid `parent_relationship` configuration" do + expect { nested_sourced_from_schema }.not_to raise_error + end + + it "works with a scalar (non-list) embedding field" do + expect { + nested_sourced_from_schema(players_field: "Player!") + }.not_to raise_error + end + + it "accepts an explicit `parent_field_name:` to identify the embedding field" do + expect { + nested_sourced_from_schema( + on_player_relationship: ->(r) { r.parent_relationship "Team", "statLines", parent_field_name: "players" } + ) + }.not_to raise_error + end + + it "raises an error when `parent_relationship` is called twice on the same relationship" do + expect { + nested_sourced_from_schema( + on_player_relationship: ->(r) { + r.parent_relationship "Team", "statLines" + r.parent_relationship "Team", "statLines" + } + ) + }.to raise_error Errors::SchemaError, a_string_including( + "`parent_relationship` has been called multiple times on `Player.statLine`" + ) + end + + it "raises an error when `parent_relationship` is used without `indexing_only: true`" do + expect { + nested_sourced_from_schema(player_indexing_only: false) + }.to raise_error Errors::SchemaError, a_string_including( + "`Player.statLine` uses `parent_relationship` but is not declared with `indexing_only: true`" + ) + end + + it "raises an error when a circular parent chain is detected" do + expect { + object_type_metadata_for "Team" do |s| + s.object_type "Team" do |t| + t.field "id", "ID!" + t.field "players", "[Player!]!" do |f| + f.mapping type: "object" + end + t.relates_to_many "statLines", "StatLine", via: "teamId", dir: :in, indexing_only: true do |r| + r.parent_relationship "Player", "statLine" + end + end + + s.object_type "Player" do |t| + t.field "id", "ID!" + t.field "teams", "[Team!]!" do |f| + f.mapping type: "object" + end + t.field "goals", "Int" do |f| + f.sourced_from "statLine", "goals" + end + t.relates_to_one "statLine", "StatLine", via: "playerId", dir: :in, indexing_only: true do |r| + r.parent_relationship "Team", "statLines" + end + end + + s.object_type "StatLine" do |t| + t.field "id", "ID!" + t.field "teamId", "ID" + t.field "playerId", "ID" + t.field "goals", "Int" + t.index "stat_lines" + end + end + }.to raise_error Errors::SchemaError, a_string_including( + "creates a circular `parent_relationship` chain" + ) + end + + it "raises an error when the parent type does not exist" do + expect { + nested_sourced_from_schema( + on_player_relationship: ->(r) { r.parent_relationship "NonExistentType", "statLines" } + ) + }.to raise_error Errors::SchemaError, a_string_including( + "`Player.statLine` references parent type `NonExistentType` via `parent_relationship`, but that type does not exist." + ) + end + + it "raises an error when the parent relationship does not exist on the parent type" do + expect { + nested_sourced_from_schema( + on_player_relationship: ->(r) { r.parent_relationship "Team", "nonExistentRelationship" } + ) + }.to raise_error Errors::SchemaError, a_string_including( + "`Player.statLine` references parent relationship `Team.nonExistentRelationship` via `parent_relationship`, but that relationship does not exist." + ) + end + + it "raises an error when source types disagree across the chain" do + expect { + nested_sourced_from_schema( + on_team: ->(t) { + t.relates_to_many "otherEvents", "OtherEvent", via: "teamId", dir: :in, indexing_only: true + }, + on_player_relationship: ->(r) { r.parent_relationship "Team", "otherEvents" } + ) do |s| + s.object_type "OtherEvent" do |t| + t.field "id", "ID!" + t.field "teamId", "ID" + t.index "other_events" + end + end + }.to raise_error Errors::SchemaError, a_string_including( + "`Player.statLine` relates to `StatLine`", + "but its parent relationship `Team.otherEvents` relates to `OtherEvent`", + "All relationships in a `parent_relationship` chain must relate to the same source type" + ) + end + + it "raises an error when the chain terminates at a non-indexed type" do + expect { + nested_sourced_from_schema(index_teams: false) + }.to raise_error Errors::SchemaError, a_string_including( + "The `parent_relationship` chain from `Player.statLine` terminates at `Team`", + "`Team` is not an indexed type", + "The chain must terminate at an indexed type" + ) + end + + it "raises an error when no embedding field is found on the parent type" do + expect { + nested_sourced_from_schema(players_field: nil) + }.to raise_error Errors::SchemaError, a_string_including( + "`Player.statLine` declares `Team` as its parent type via `parent_relationship`, but `Team` has no field of type `Player`" + ) + end + + it "raises an error when multiple embedding fields exist (ambiguous)" do + expect { + nested_sourced_from_schema(on_team: ->(t) { + t.field "bench_players", "[Player!]!" do |f| + f.mapping type: "object" + end + }) + }.to raise_error Errors::SchemaError, a_string_including( + "`Team` has multiple fields of type `Player`", + "has an ambiguous `parent_relationship`", + "parent_field_name:" + ) + end + + it "raises an error when an explicit `parent_field_name:` references a non-existent field" do + expect { + nested_sourced_from_schema( + on_player_relationship: ->(r) { r.parent_relationship "Team", "statLines", parent_field_name: "nonExistentField" } + ) + }.to raise_error Errors::SchemaError, a_string_including( + "`Player.statLine` references field `Team.nonExistentField` via `parent_relationship`, but that field does not exist." + ) + end + + def nested_sourced_from_schema( + on_team: nil, + on_player_relationship: ->(r) { r.parent_relationship "Team", "statLines" }, + player_indexing_only: true, + players_field: "[Player!]!", + index_teams: true + ) + object_type_metadata_for "Team" do |s| + s.object_type "Team" do |t| + t.field "id", "ID!" + if players_field + t.field "players", players_field do |f| + f.mapping type: "object" if players_field.start_with?("[") + end + end + t.relates_to_many "statLines", "StatLine", via: "teamId", dir: :in, indexing_only: true + on_team&.call(t) + if index_teams + t.index("teams") { |i| i.has_had_multiple_sources! } + end + end + + s.object_type "Player" do |t| + t.field "id", "ID!" + t.field "goals", "Int" do |f| + f.sourced_from "statLine", "goals" + end + t.relates_to_one "statLine", "StatLine", via: "playerId", dir: :in, indexing_only: player_indexing_only do |r| + on_player_relationship.call(r) + end + end + + s.object_type "StatLine" do |t| + t.field "id", "ID!" + t.field "teamId", "ID" + t.field "playerId", "ID" + t.field "goals", "Int" + t.index "stat_lines" + end + + yield s if block_given? + end + end + end + def update_targets_for( type, widget_name: "String", From 78c437cc6fb0b874e3ec4f7b486c942577147a86 Mon Sep 17 00:00:00 2001 From: ellisandrews-toast Date: Thu, 4 Jun 2026 13:57:23 -0400 Subject: [PATCH 2/5] Address review feedback on RelationshipChainResolver and nested chain validation --- .../indexing/relationship_chain_resolver.rb | 123 +++++++++--------- .../sourced_from_update_targets_resolver.rb | 18 +-- .../indexing/relationship_chain_resolver.rbs | 65 +++++---- .../sourced_from_update_targets_resolver.rbs | 2 +- .../update_targets_spec.rb | 12 +- 5 files changed, 109 insertions(+), 111 deletions(-) diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rb index b177d0e07..03b287f14 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rb @@ -15,7 +15,6 @@ module Indexing # # @private ResolvedRelationshipChain = ::Data.define( - :root_indexed_type, # ObjectType - the indexed type at the root :root_relationship, # Relationship - the root relationship (no parent_relationship) :path_segments # Array - ordered root-to-leaf ) @@ -42,34 +41,31 @@ def initialize(schema_def_state:) @schema_def_state = schema_def_state end - # Resolves the chain starting from `starting_relationship` (which must have a - # `parent_ref`) on `starting_type`. + # Resolves the chain starting from `starting_relationship` (which must have a `parent_ref`). # # Returns a tuple of [resolved_chain, errors]. # If errors is non-empty, resolved_chain will be nil. - def resolve(starting_relationship, starting_type) + def resolve(starting_relationship) errors = [] # : ::Array[::String] path_segments = [] # : ::Array[PathSegment] - visited_type_names = Set[starting_type.name] - - current_rel, current_type = resolve_chain( - starting_relationship, starting_type, path_segments, errors, visited_type_names - ) - - return [nil, errors] if errors.any? - - # The recursion terminated because current_rel has no parent_ref — - # this is the root relationship. Validate that current_type is indexed. - unless current_type.root_document_type? - errors << "The `parent_relationship` chain from #{rel_description(starting_type, starting_relationship)} " \ - "terminates at `#{current_type.name}`, but `#{current_type.name}` is not an indexed type. " \ + visited_relationships = Set[starting_relationship] + + # resolve_chain returns the chain's root relationship (the one with no parent_ref), or nil + # if it hit an error walking the chain (in which case the error is already recorded). + root_relationship = resolve_chain(starting_relationship, path_segments, errors, visited_relationships) + return [nil, errors] unless root_relationship + + # A valid chain must terminate at a relationship defined on an indexed type. + root_type = root_relationship.parent_type + unless root_type.root_document_type? + errors << "The `parent_relationship` chain from #{rel_description(starting_relationship)} " \ + "terminates at `#{root_type.name}`, but `#{root_type.name}` is not an indexed type. " \ "The chain must terminate at an indexed type." return [nil, errors] end resolved_chain = ResolvedRelationshipChain.new( - root_indexed_type: current_type, - root_relationship: current_rel, + root_relationship: root_relationship, path_segments: path_segments.reverse # reverse so root-to-leaf order ) @@ -78,73 +74,69 @@ def resolve(starting_relationship, starting_type) private - # Recursively walks from leaf to root, building path segments in reverse. - # Stops when the current relationship has no parent_ref (i.e., it's the root). - def resolve_chain(current_rel, current_type, path_segments, errors, visited_type_names) + # Recursively walks from leaf to root, building path segments in reverse. Returns the root + # relationship (the one with no parent_ref) on success, or nil if an error was encountered. + def resolve_chain(current_rel, path_segments, errors, visited_relationships) parent_ref = current_rel.parent_ref - return [current_rel, current_type] unless parent_ref - - parent_type, parent_rel = resolve_parent_ref(current_rel, current_type, parent_ref, errors, visited_type_names) - return [current_rel, current_type] if errors.any? + return current_rel unless parent_ref - parent_type = _ = parent_type # : indexableType - parent_rel = _ = parent_rel # : SchemaElements::Relationship + parent_rel = resolve_parent_ref(current_rel, parent_ref, errors, visited_relationships) + return nil unless parent_rel - build_path_segment(current_rel, current_type, parent_type, path_segments, errors) - return [current_rel, current_type] if errors.any? + build_path_segment(current_rel, parent_rel.parent_type, path_segments, errors) + return nil if errors.any? - visited_type_names.add(parent_type.name) - resolve_chain(parent_rel, parent_type, path_segments, errors, visited_type_names) + visited_relationships.add(parent_rel) + resolve_chain(parent_rel, path_segments, errors, visited_relationships) end - # Resolves a parent_ref into the concrete parent type and relationship. - # Returns [parent_type, parent_rel] on success, or appends to errors and returns nils. - def resolve_parent_ref(current_rel, current_type, ref, errors, visited_type_names) + # Resolves a parent_ref into the concrete parent relationship. + # Returns the parent relationship on success, or appends to errors and returns nil. + def resolve_parent_ref(current_rel, ref, errors, visited_relationships) unless current_rel.indexing_only - errors << "#{rel_description(current_type, current_rel)} uses `parent_relationship` but is not declared with " \ + errors << "#{rel_description(current_rel)} uses `parent_relationship` but is not declared with " \ "`indexing_only: true`. Relationships with `parent_relationship` must be indexing-only." - return [nil, nil] + return nil end - parent_type_name = ref.type_ref.name - if visited_type_names.include?(parent_type_name) - errors << "#{rel_description(current_type, current_rel)} creates a circular `parent_relationship` chain " \ - "— `#{parent_type_name}` was already visited. The chain must terminate at a root indexed type." - return [nil, nil] - end - - parent_type = (_ = ref.type_ref.as_object_type) # : indexableType? + parent_type = ref.type_ref.as_object_type # : SchemaElements::ObjectType? unless parent_type - errors << "#{rel_description(current_type, current_rel)} references parent type " \ - "`#{parent_type_name}` via `parent_relationship`, but that type does not exist. Is it misspelled?" - return [nil, nil] + errors << "#{rel_description(current_rel)} references parent type " \ + "`#{ref.type_ref.name}` via `parent_relationship`, but that type does not exist. Is it misspelled?" + return nil end parent_rel = parent_type.relationships_by_name[ref.relationship_name] unless parent_rel - errors << "#{rel_description(current_type, current_rel)} references parent relationship " \ + errors << "#{rel_description(current_rel)} references parent relationship " \ "`#{parent_type.name}.#{ref.relationship_name}` via `parent_relationship`, " \ "but that relationship does not exist. Is it misspelled?" - return [nil, nil] + return nil + end + + if visited_relationships.include?(parent_rel) + errors << "#{rel_description(current_rel)} creates a circular `parent_relationship` chain " \ + "— `#{parent_type.name}.#{ref.relationship_name}` was already visited. The chain must terminate at a root indexed type." + return nil end - current_source_type_name = current_rel.related_type.unwrap_non_null.name - parent_source_type_name = parent_rel.related_type.unwrap_non_null.name + current_source_type_name = current_rel.related_type.name + parent_source_type_name = parent_rel.related_type.name unless current_source_type_name == parent_source_type_name - errors << "#{rel_description(current_type, current_rel)} relates to `#{current_source_type_name}`, " \ + errors << "#{rel_description(current_rel)} relates to `#{current_source_type_name}`, " \ "but its parent relationship `#{parent_type.name}.#{ref.relationship_name}` relates to " \ "`#{parent_source_type_name}`. All relationships in a `parent_relationship` chain must relate to the same source type." - return [nil, nil] + return nil end - [parent_type, parent_rel] + parent_rel end # Builds a PathSegment for the current level and appends it to path_segments. # Uses the explicitly specified field name if provided, otherwise auto-discovers it. - def build_path_segment(current_rel, current_type, parent_type, path_segments, errors) + def build_path_segment(current_rel, parent_type, path_segments, errors) parent_ref = current_rel.parent_ref # : SchemaElements::Relationship::ParentRef - field = resolve_field(parent_ref, parent_type, current_rel, current_type, errors) + field = resolve_field(parent_ref, parent_type, current_rel, errors) return unless field # For list fields, `match_field_name` and `source_field_name` identify which element @@ -165,20 +157,21 @@ def build_path_segment(current_rel, current_type, parent_type, path_segments, er end end - def resolve_field(parent_ref, parent_type, current_rel, current_type, errors) + def resolve_field(parent_ref, parent_type, current_rel, errors) if parent_ref.field_name field = parent_type.graphql_fields_by_name[parent_ref.field_name] unless field - errors << "#{rel_description(current_type, current_rel)} references field `#{parent_type.name}.#{parent_ref.field_name}` " \ + errors << "#{rel_description(current_rel)} references field `#{parent_type.name}.#{parent_ref.field_name}` " \ "via `parent_relationship`, but that field does not exist." end field else - find_field_by_type(parent_type, current_type, current_rel, errors) + find_field_by_type(parent_type, current_rel, errors) end end - def find_field_by_type(parent_type, child_type, current_rel, errors) + def find_field_by_type(parent_type, current_rel, errors) + child_type = current_rel.parent_type matches = parent_type.graphql_fields_by_name.values.select do |field| field.type.fully_unwrapped.name == child_type.name end @@ -186,13 +179,13 @@ def find_field_by_type(parent_type, child_type, current_rel, errors) if matches.size > 1 field_names = matches.map(&:name).join(", ") parent_ref = current_rel.parent_ref # : SchemaElements::Relationship::ParentRef - errors << "#{rel_description(child_type, current_rel)} has an ambiguous `parent_relationship` — " \ + errors << "#{rel_description(current_rel)} has an ambiguous `parent_relationship` — " \ "`#{parent_type.name}` has multiple fields of type `#{child_type.name}` (#{field_names}). " \ "Specify which field using the `parent_field_name:` option: " \ "`r.parent_relationship \"#{parent_type.name}\", \"#{parent_ref.relationship_name}\", parent_field_name: \"\"`" nil elsif matches.empty? - errors << "#{rel_description(child_type, current_rel)} declares `#{parent_type.name}` as its parent type " \ + errors << "#{rel_description(current_rel)} declares `#{parent_type.name}` as its parent type " \ "via `parent_relationship`, but `#{parent_type.name}` has no field of type `#{child_type.name}`." nil else @@ -200,8 +193,8 @@ def find_field_by_type(parent_type, child_type, current_rel, errors) end end - def rel_description(type, relationship) - "`#{type.name}.#{relationship.name}`" + def rel_description(relationship) + "`#{relationship.parent_type.name}.#{relationship.name}`" end end end diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rb index 1efac8af7..98fd0d822 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rb @@ -67,11 +67,9 @@ def resolve_for_type(object_type, &error_reporter) end end - # Non-indexed types may have `parent_relationship` chains that need validation. - # Indexed types use the top-level sourced_from path above instead. - if object_type.own_index_def.nil? - resolve_nested_relationships(object_type, &error_reporter) - end + # Resolve any `parent_relationship` chains on this type. For now this only surfaces + # configuration errors; later PRs will use the resolved chains to build nested update targets. + resolve_relationship_chains(object_type, &error_reporter) results end @@ -100,18 +98,22 @@ def resolve_update_target(object_type, resolved_relationship, sourced_fields) [resolved_relationship.related_type.name, update_target] if update_target end - def resolve_nested_relationships(object_type) + def resolve_relationship_chains(object_type) relationships_with_parent_ref = object_type.relationships_by_name.each_value.select(&:parent_ref) return if relationships_with_parent_ref.empty? - sourced_relationship_names = object_type.fields_with_sources.map { |f| (_ = f.source).relationship_name } + sourced_relationship_names = object_type.fields_with_sources.map do |f| + source = f.source # : SchemaElements::FieldSource + source.relationship_name + end.to_set + chain_resolver = RelationshipChainResolver.new(schema_def_state: @schema_def_state) relationships_with_parent_ref.each do |relationship| next unless sourced_relationship_names.include?(relationship.name) # TODO: use resolved_chain to build nested update targets once that logic is implemented. - _resolved_chain, chain_errors = chain_resolver.resolve(relationship, object_type) + _resolved_chain, chain_errors = chain_resolver.resolve(relationship) chain_errors.each { |error| yield :sourced_field, error } end end diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rbs index d1206a75e..33bd6ab5c 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rbs @@ -1,65 +1,60 @@ module ElasticGraph module SchemaDefinition module Indexing + class ResolvedRelationshipChain + attr_reader root_relationship: SchemaElements::Relationship + attr_reader path_segments: ::Array[PathSegment] + + def initialize: ( + root_relationship: SchemaElements::Relationship, + path_segments: ::Array[PathSegment] + ) -> void + end + + class PathSegment + attr_reader field: SchemaElements::Field + attr_reader match_field_name: ::String? + attr_reader source_field_name: ::String? + + def initialize: ( + field: SchemaElements::Field, + match_field_name: ::String?, + source_field_name: ::String? + ) -> void + end + class RelationshipChainResolver @schema_def_state: State def initialize: (schema_def_state: State) -> void - def resolve: (SchemaElements::Relationship, indexableType) -> [ResolvedRelationshipChain?, ::Array[::String]] + def resolve: (SchemaElements::Relationship) -> [ResolvedRelationshipChain?, ::Array[::String]] private def resolve_chain: ( SchemaElements::Relationship, - indexableType, ::Array[PathSegment], ::Array[::String], - ::Set[::String] - ) -> [SchemaElements::Relationship, indexableType] + ::Set[SchemaElements::Relationship] + ) -> SchemaElements::Relationship? def resolve_parent_ref: ( SchemaElements::Relationship, - indexableType, SchemaElements::Relationship::ParentRef, ::Array[::String], - ::Set[::String] - ) -> [indexableType?, SchemaElements::Relationship?] + ::Set[SchemaElements::Relationship] + ) -> SchemaElements::Relationship? def build_path_segment: ( SchemaElements::Relationship, indexableType, - indexableType, ::Array[PathSegment], ::Array[::String] ) -> void - def resolve_field: (SchemaElements::Relationship::ParentRef, indexableType, SchemaElements::Relationship, indexableType, ::Array[::String]) -> SchemaElements::Field? - def find_field_by_type: (indexableType, indexableType, SchemaElements::Relationship, ::Array[::String]) -> SchemaElements::Field? - def rel_description: (indexableType, SchemaElements::Relationship) -> ::String - end - - class ResolvedRelationshipChain - attr_reader root_indexed_type: indexableType - attr_reader root_relationship: SchemaElements::Relationship - attr_reader path_segments: ::Array[PathSegment] - - def initialize: ( - root_indexed_type: indexableType, - root_relationship: SchemaElements::Relationship, - path_segments: ::Array[PathSegment] - ) -> void - end - - class PathSegment - attr_reader field: SchemaElements::Field - attr_reader match_field_name: ::String? - attr_reader source_field_name: ::String? - - def initialize: ( - field: SchemaElements::Field, - match_field_name: ::String?, - source_field_name: ::String? - ) -> void + def resolve_field: (SchemaElements::Relationship::ParentRef, indexableType, SchemaElements::Relationship, ::Array[::String]) -> SchemaElements::Field? + def find_field_by_type: (indexableType, SchemaElements::Relationship, ::Array[::String]) -> SchemaElements::Field? + def rel_description: (SchemaElements::Relationship) -> ::String end end end diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rbs index 1dfab0d25..c9c2ca72b 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rbs @@ -11,7 +11,7 @@ module ElasticGraph def resolve_for_type: (indexableType) { (::Symbol, ::String) -> void } -> ::Array[[::String, SchemaArtifacts::RuntimeMetadata::UpdateTarget]] def resolve_update_target: (indexableType, ResolvedRelationship, ::Array[SchemaElements::Field]) { (::Symbol, ::String) -> void } -> [::String, SchemaArtifacts::RuntimeMetadata::UpdateTarget]? - def resolve_nested_relationships: (indexableType) { (::Symbol, ::String) -> void } -> void + def resolve_relationship_chains: (indexableType) { (::Symbol, ::String) -> void } -> void def sourced_fields_by_relationship_name: (indexableType) -> ::Hash[::String, ::Array[SchemaElements::Field]] def raise_if_errors: (::Array[::String], ::Array[::String]) -> void end diff --git a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb index a4e09f61c..f0a0e0f12 100644 --- a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb +++ b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb @@ -1338,7 +1338,7 @@ def raise_error_about_workspace_relationship(details, sourced_fields: true) ) end - it "raises an error when a circular parent chain is detected" do + it "raises an error when a circular parent chain is detected", :dont_validate_graphql_schema do expect { object_type_metadata_for "Team" do |s| s.object_type "Team" do |t| @@ -1388,8 +1388,12 @@ def raise_error_about_workspace_relationship(details, sourced_fields: true) end it "raises an error when the parent relationship does not exist on the parent type" do + # `Player` is indexed here in addition to being embedded on `Team`. This confirms that + # `parent_relationship` chains are validated even for types that are both indexed and + # embedded (not just non-indexed embedded types). expect { nested_sourced_from_schema( + index_players: true, on_player_relationship: ->(r) { r.parent_relationship "Team", "nonExistentRelationship" } ) }.to raise_error Errors::SchemaError, a_string_including( @@ -1465,7 +1469,8 @@ def nested_sourced_from_schema( on_player_relationship: ->(r) { r.parent_relationship "Team", "statLines" }, player_indexing_only: true, players_field: "[Player!]!", - index_teams: true + index_teams: true, + index_players: false ) object_type_metadata_for "Team" do |s| s.object_type "Team" do |t| @@ -1490,6 +1495,9 @@ def nested_sourced_from_schema( t.relates_to_one "statLine", "StatLine", via: "playerId", dir: :in, indexing_only: player_indexing_only do |r| on_player_relationship.call(r) end + if index_players + t.index("players") { |i| i.has_had_multiple_sources! } + end end s.object_type "StatLine" do |t| From 2c8cf25d63936998b0a4a60ad8eec1e72f965ebd Mon Sep 17 00:00:00 2001 From: ellisandrews-toast Date: Thu, 4 Jun 2026 14:03:52 -0400 Subject: [PATCH 3/5] Remove guard that skipped chain validation for unreferenced parent relationships --- .../indexing/sourced_from_update_targets_resolver.rb | 7 ------- 1 file changed, 7 deletions(-) diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rb index 98fd0d822..ae7eaa318 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/sourced_from_update_targets_resolver.rb @@ -102,16 +102,9 @@ def resolve_relationship_chains(object_type) relationships_with_parent_ref = object_type.relationships_by_name.each_value.select(&:parent_ref) return if relationships_with_parent_ref.empty? - sourced_relationship_names = object_type.fields_with_sources.map do |f| - source = f.source # : SchemaElements::FieldSource - source.relationship_name - end.to_set - chain_resolver = RelationshipChainResolver.new(schema_def_state: @schema_def_state) relationships_with_parent_ref.each do |relationship| - next unless sourced_relationship_names.include?(relationship.name) - # TODO: use resolved_chain to build nested update targets once that logic is implemented. _resolved_chain, chain_errors = chain_resolver.resolve(relationship) chain_errors.each { |error| yield :sourced_field, error } From 7d74fc1543c874500831f80bc2cb24d449c2bef4 Mon Sep 17 00:00:00 2001 From: ellisandrews-toast Date: Mon, 8 Jun 2026 10:34:24 -0400 Subject: [PATCH 4/5] Clean up parent_relationship resolution per review feedback --- .../indexing/relationship_chain_resolver.rb | 32 ++++--- .../indexing/relationship_chain_resolver.rbs | 3 +- .../update_targets_spec.rb | 92 ++++++++++++++++++- 3 files changed, 111 insertions(+), 16 deletions(-) diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rb index 03b287f14..5a8a66e8e 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rb @@ -20,14 +20,15 @@ module Indexing ) # Describes how to navigate from a parent type into a nested child element. - # For list fields, `match_field_name` and `source_field_name` identify which element - # to update: the element where `element[match_field_name] == event[source_field_name]`. - # For non-list (object) fields, these are nil since there's no ambiguity. + # For list fields, `source_field_name` identifies which element to update: the element + # whose `id` matches `event[source_field_name]`. We implicitly match on the `id` field + # because ElasticGraph relationships always join on `id` via foreign keys; this could be + # made configurable in the future to support non-`id` primary keys. + # For non-list (object) fields, `source_field_name` is nil since there's no ambiguity. # # @private PathSegment = ::Data.define( :field, # Field - the field to navigate into at this level - :match_field_name, # String? - field name on the nested element to match against (nil for object fields) :source_field_name # String? - field name on the source event providing the match value (nil for object fields) ) @@ -39,6 +40,14 @@ module Indexing class RelationshipChainResolver def initialize(schema_def_state:) @schema_def_state = schema_def_state + + # Lazily groups each parent type's indexing fields by their fully-unwrapped field type name, + # so `find_field_by_type` can look up candidate embedding fields without re-scanning per chain. + @indexing_fields_by_field_type_name_by_parent_type = ::Hash.new do |hash, parent_type| + hash[parent_type] = parent_type.indexing_fields_by_name_in_index.values.group_by do |field| + field.type.fully_unwrapped.name + end + end end # Resolves the chain starting from `starting_relationship` (which must have a `parent_ref`). @@ -139,19 +148,18 @@ def build_path_segment(current_rel, parent_type, path_segments, errors) field = resolve_field(parent_ref, parent_type, current_rel, errors) return unless field - # For list fields, `match_field_name` and `source_field_name` identify which element - # to update. `match_field_name` is always "id" because ElasticGraph relationships join - # on `id` via foreign keys. For non-list fields, these are nil since there's no ambiguity. + # For list fields, `source_field_name` identifies which element to update: the one whose + # `id` matches `event[source_field_name]`. We implicitly match on `id` because ElasticGraph + # relationships always join on `id` via foreign keys. For non-list fields, it's nil since + # there's no ambiguity. path_segments << if field.type.list? PathSegment.new( field: field, - match_field_name: "id", source_field_name: current_rel.foreign_key ) else PathSegment.new( field: field, - match_field_name: nil, source_field_name: nil ) end @@ -159,7 +167,7 @@ def build_path_segment(current_rel, parent_type, path_segments, errors) def resolve_field(parent_ref, parent_type, current_rel, errors) if parent_ref.field_name - field = parent_type.graphql_fields_by_name[parent_ref.field_name] + field = parent_type.indexing_fields_by_name_in_index[parent_ref.field_name] unless field errors << "#{rel_description(current_rel)} references field `#{parent_type.name}.#{parent_ref.field_name}` " \ "via `parent_relationship`, but that field does not exist." @@ -172,9 +180,7 @@ def resolve_field(parent_ref, parent_type, current_rel, errors) def find_field_by_type(parent_type, current_rel, errors) child_type = current_rel.parent_type - matches = parent_type.graphql_fields_by_name.values.select do |field| - field.type.fully_unwrapped.name == child_type.name - end + matches = @indexing_fields_by_field_type_name_by_parent_type.dig(parent_type, child_type.name) || [] if matches.size > 1 field_names = matches.map(&:name).join(", ") diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rbs index 33bd6ab5c..6c161e20a 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/indexing/relationship_chain_resolver.rbs @@ -13,18 +13,17 @@ module ElasticGraph class PathSegment attr_reader field: SchemaElements::Field - attr_reader match_field_name: ::String? attr_reader source_field_name: ::String? def initialize: ( field: SchemaElements::Field, - match_field_name: ::String?, source_field_name: ::String? ) -> void end class RelationshipChainResolver @schema_def_state: State + @indexing_fields_by_field_type_name_by_parent_type: ::Hash[indexableType, ::Hash[::String, ::Array[SchemaElements::Field]]] def initialize: (schema_def_state: State) -> void def resolve: (SchemaElements::Relationship) -> [ResolvedRelationshipChain?, ::Array[::String]] diff --git a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb index f0a0e0f12..44246506b 100644 --- a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb +++ b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb @@ -1303,7 +1303,7 @@ def raise_error_about_workspace_relationship(details, sourced_fields: true) expect { nested_sourced_from_schema }.not_to raise_error end - it "works with a scalar (non-list) embedding field" do + it "works with a non-list (object) embedding field" do expect { nested_sourced_from_schema(players_field: "Player!") }.not_to raise_error @@ -1317,6 +1317,18 @@ def raise_error_about_workspace_relationship(details, sourced_fields: true) }.not_to raise_error end + it "discovers an embedding field declared with `indexing_only: true`" do + # An `indexing_only: true` field is absent from `graphql_fields_by_name` but present in + # `indexing_fields_by_name_in_index`, so this only resolves when the latter is used. + expect { + nested_sourced_from_schema(players_field: nil, on_team: ->(t) { + t.field "players", "[Player!]!", indexing_only: true do |f| + f.mapping type: "object" + end + }) + }.not_to raise_error + end + it "raises an error when `parent_relationship` is called twice on the same relationship" do expect { nested_sourced_from_schema( @@ -1377,6 +1389,65 @@ def raise_error_about_workspace_relationship(details, sourced_fields: true) ) end + it "resolves a chain spanning several levels", :dont_validate_graphql_schema do + # League -> Team -> Player -> GameAppearance is a 4-level `parent_relationship` chain (leaf + # to root: GameAppearance's relationship walks up through Player and Team to the indexed + # League). All four relationships relate to the same `StatLine` source type, so the chain is + # valid. This can only pass if `resolve_chain` actually recurses through every level. + expect { + object_type_metadata_for "League" do |s| + s.object_type "League" do |t| + t.field "id", "ID!" + t.field "teams", "[Team!]!" do |f| + f.mapping type: "object" + end + t.relates_to_many "statLines", "StatLine", via: "leagueId", dir: :in, indexing_only: true + t.index "leagues" + end + + s.object_type "Team" do |t| + t.field "id", "ID!" + t.field "players", "[Player!]!" do |f| + f.mapping type: "object" + end + t.relates_to_many "statLines", "StatLine", via: "teamId", dir: :in, indexing_only: true do |r| + r.parent_relationship "League", "statLines" + end + end + + s.object_type "Player" do |t| + t.field "id", "ID!" + t.field "gameAppearances", "[GameAppearance!]!" do |f| + f.mapping type: "object" + end + t.relates_to_many "statLines", "StatLine", via: "playerId", dir: :in, indexing_only: true do |r| + r.parent_relationship "Team", "statLines" + end + end + + s.object_type "GameAppearance" do |t| + t.field "id", "ID!" + t.field "goals", "Int" do |f| + f.sourced_from "statLine", "goals" + end + t.relates_to_one "statLine", "StatLine", via: "gameAppearanceId", dir: :in, indexing_only: true do |r| + r.parent_relationship "Player", "statLines" + end + end + + s.object_type "StatLine" do |t| + t.field "id", "ID!" + t.field "leagueId", "ID" + t.field "teamId", "ID" + t.field "playerId", "ID" + t.field "gameAppearanceId", "ID" + t.field "goals", "Int" + t.index "stat_lines" + end + end + }.not_to raise_error + end + it "raises an error when the parent type does not exist" do expect { nested_sourced_from_schema( @@ -1454,6 +1525,19 @@ def raise_error_about_workspace_relationship(details, sourced_fields: true) ) end + it "uses `parent_field_name:` to disambiguate when multiple embedding fields exist" do + expect { + nested_sourced_from_schema( + on_team: ->(t) { + t.field "bench_players", "[Player!]!" do |f| + f.mapping type: "object" + end + }, + on_player_relationship: ->(r) { r.parent_relationship "Team", "statLines", parent_field_name: "bench_players" } + ) + }.not_to raise_error + end + it "raises an error when an explicit `parent_field_name:` references a non-existent field" do expect { nested_sourced_from_schema( @@ -1475,13 +1559,16 @@ def nested_sourced_from_schema( object_type_metadata_for "Team" do |s| s.object_type "Team" do |t| t.field "id", "ID!" + if players_field t.field "players", players_field do |f| f.mapping type: "object" if players_field.start_with?("[") end end + t.relates_to_many "statLines", "StatLine", via: "teamId", dir: :in, indexing_only: true on_team&.call(t) + if index_teams t.index("teams") { |i| i.has_had_multiple_sources! } end @@ -1489,12 +1576,15 @@ def nested_sourced_from_schema( s.object_type "Player" do |t| t.field "id", "ID!" + t.field "goals", "Int" do |f| f.sourced_from "statLine", "goals" end + t.relates_to_one "statLine", "StatLine", via: "playerId", dir: :in, indexing_only: player_indexing_only do |r| on_player_relationship.call(r) end + if index_players t.index("players") { |i| i.has_had_multiple_sources! } end From aa22af13f539e481360f34088df4930db1b04c9c Mon Sep 17 00:00:00 2001 From: ellisandrews-toast Date: Mon, 8 Jun 2026 11:25:15 -0400 Subject: [PATCH 5/5] Remove unnecessary :dont_validate_graphql_schema from test --- .../object_types_by_name/update_targets_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb index 44246506b..7cf9c3a43 100644 --- a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb +++ b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/object_types_by_name/update_targets_spec.rb @@ -1389,7 +1389,7 @@ def raise_error_about_workspace_relationship(details, sourced_fields: true) ) end - it "resolves a chain spanning several levels", :dont_validate_graphql_schema do + it "resolves a chain spanning several levels" do # League -> Team -> Player -> GameAppearance is a 4-level `parent_relationship` chain (leaf # to root: GameAppearance's relationship walks up through Player and Team to the indexed # League). All four relationships relate to the same `StatLine` source type, so the chain is