diff --git a/elasticgraph-admin/lib/elastic_graph/admin/index_definition_configurator/for_index.rb b/elasticgraph-admin/lib/elastic_graph/admin/index_definition_configurator/for_index.rb index c70d77bb2..229a41f20 100644 --- a/elasticgraph-admin/lib/elastic_graph/admin/index_definition_configurator/for_index.rb +++ b/elasticgraph-admin/lib/elastic_graph/admin/index_definition_configurator/for_index.rb @@ -7,6 +7,7 @@ # frozen_string_literal: true require "elastic_graph/admin/cluster_configurator/action_reporter" +require "elastic_graph/admin/index_definition_configurator/mapping_update" require "elastic_graph/datastore_core/index_config_normalizer" require "elastic_graph/indexer/hash_differ" require "elastic_graph/support/hash_util" @@ -32,9 +33,8 @@ def initialize(datastore_client, index, env_agnostic_index_config, output) # and the state of the index in the datastore, does one of the following: # # - If the index did not already exist: creates the index with the desired mappings and settings. - # - If the desired mapping has fewer fields than what is in the index: raises an exception, - # because the datastore provides no way to remove fields from a mapping and it would be confusing - # for this method to silently ignore the issue. + # - If the desired mapping has fewer fields than what is in the index: leaves the existing fields + # alone, because the datastore provides no way to remove fields from a mapping. # - If the settings have desired changes: updates the settings, restoring any setting that # no longer has a desired value to its default. # - If the mapping has desired changes: updates the mappings. @@ -75,14 +75,9 @@ def create_new_index end def update_mapping - @datastore_client.put_index_mapping(index: @index.name, body: desired_mapping) + @datastore_client.put_index_mapping(index: @index.name, body: desired_mapping_for_update) action_description = "Updated mappings for index `#{@index.name}`:\n#{mapping_diff}" - if mapping_removals.any? - action_description += "\n\nNote: the extra fields listed here will not actually get removed. " \ - "Mapping removals are unsupported (but ElasticGraph will leave them alone and they'll cause no problems)." - end - report_action action_description end @@ -100,10 +95,6 @@ def index_exists? !current_config.empty? end - def mapping_removals - @mapping_removals ||= mapping_fields_from(current_mapping) - mapping_fields_from(desired_mapping) - end - def mapping_type_changes @mapping_type_changes ||= begin flattened_current = Support::HashUtil.flatten_and_stringify_keys(current_mapping) @@ -116,7 +107,7 @@ def mapping_type_changes end def has_mapping_updates? - current_mapping != desired_mapping + current_mapping != desired_mapping_for_update end def settings_updates @@ -127,15 +118,8 @@ def settings_updates end end - def mapping_fields_from(mapping_hash, prefix = "") - (mapping_hash["properties"] || []).flat_map do |key, params| - field = prefix + key - if params.key?("properties") - [field] + mapping_fields_from(params, "#{field}.") - else - [field] - end - end + def desired_mapping_for_update + @desired_mapping_for_update ||= MappingUpdate.build_mapping_update(desired: desired_mapping, current: current_mapping) end def desired_mapping @@ -178,7 +162,7 @@ def current_config end def mapping_diff - @mapping_diff ||= Indexer::HashDiffer.diff(current_mapping, desired_mapping) || "(no diff)" + @mapping_diff ||= Indexer::HashDiffer.diff(current_mapping, desired_mapping_for_update) || "(no diff)" end def settings_diff diff --git a/elasticgraph-admin/lib/elastic_graph/admin/index_definition_configurator/for_index_template.rb b/elasticgraph-admin/lib/elastic_graph/admin/index_definition_configurator/for_index_template.rb index bce4ea0bf..40b0bc0a4 100644 --- a/elasticgraph-admin/lib/elastic_graph/admin/index_definition_configurator/for_index_template.rb +++ b/elasticgraph-admin/lib/elastic_graph/admin/index_definition_configurator/for_index_template.rb @@ -8,6 +8,7 @@ require "elastic_graph/admin/cluster_configurator/action_reporter" require "elastic_graph/admin/index_definition_configurator/for_index" +require "elastic_graph/admin/index_definition_configurator/mapping_update" require "elastic_graph/datastore_core/index_config_normalizer" require "elastic_graph/indexer/hash_differ" require "elastic_graph/support/hash_util" @@ -36,9 +37,9 @@ def initialize(datastore_client, index_template, env_agnostic_index_config_paren # and the state of the index in the datastore, does one of the following: # # - If the index did not already exist: creates the index with the desired mappings and settings. - # - If the desired mapping has fewer fields than what is in the index: raises an exception, - # because the datastore provides no way to remove fields from a mapping and it would be confusing - # for this method to silently ignore the issue. + # - If the desired mapping has fewer fields than what is in the index template: preserves fields + # that the schema definition still references (via `deleted_field`/`renamed_from` declarations) + # and drops fields whose last schema reference has been removed (see `put_index_template`). # - If the settings have desired changes: updates the settings, restoring any setting that # no longer has a desired value to its default. # - If the mapping has desired changes: updates the mappings. @@ -66,23 +67,48 @@ def validate private + # Creates or updates the index template. Fields that exist on the current template but not in the + # desired configuration are dropped only when it is provably safe. New rollover indices are + # auto-created from the template at indexing time with `dynamic: strict` mappings, so dropping a + # field that something can still write would cause indexing failures on those new indices--we have + # previously had a near-SEV from dropping a template field while deployed indexers were still + # running an old version of the code that used it. So, as long as a field is referenced by a + # `deleted_field` or `renamed_from` declaration in the schema definition (meaning an old JSON + # schema version may still reference it, and an indexer deployed with an older schema version or a + # replayed old event may still write it), we preserve it via `field_paths_protected_from_removal`. + # Once the last reference is removed from the schema definition--which `eg-schema_def` only + # sanctions once no JSON schema version references the field--the field is dropped on the next + # admin run, keeping templates from accumulating stale fields forever. def put_index_template - desired_template_config_payload = Support::HashUtil.deep_merge( - desired_config_parent, - {"template" => {"mappings" => merge_properties(desired_mapping, current_mapping)}} - ) - - action_description = "Updated index template: `#{@index_template.name}`:\n#{config_diff}" - - if mapping_removals.any? - action_description += "\n\nNote: the extra fields listed here will not actually get removed. " \ - "Mapping removals are unsupported (but ElasticGraph will leave them alone and they'll cause no problems)." + action_description = if index_template_exists? + "Updated index template: `#{@index_template.name}`:\n#{config_diff}#{removed_fields_note}" + else + "Created index template: `#{@index_template.name}`" end - @datastore_client.put_index_template(name: @index_template.name, body: desired_template_config_payload) + @datastore_client.put_index_template(name: @index_template.name, body: desired_config_parent_for_update) report_action action_description end + # Dropped fields show up as deletions in the reported diff, but we also call them out explicitly + # since a field removal is the part of a mapping update most worth a second look from operators. + def removed_fields_note + removed_fields = mapping_field_paths_of(current_mapping) - mapping_field_paths_of(desired_mapping_for_update) + return "" if removed_fields.empty? + + "\n\n" + <<~EOS.chomp + Removed #{removed_fields.size} stale field(s) from index template `#{@index_template.name}` that are no longer referenced by the schema: #{removed_fields.join(", ")}. + New rollover indices created from this template will reject documents that still contain these fields (mappings are `dynamic: strict`). + EOS + end + + def mapping_field_paths_of(mapping, parent_path: "") + mapping.fetch("properties", MappingUpdate::EMPTY_PROPERTIES).flat_map do |field_name, field_mapping| + path = "#{parent_path}#{field_name}" + [path] + mapping_field_paths_of(field_mapping, parent_path: "#{path}.") + end + end + def cannot_modify_mapping_field_type_error "The datastore does not support modifying the type of a field from an existing index definition. " \ "You are attempting to update type of fields (#{mapping_type_changes.inspect}) from the #{@index_template.name} index definition." @@ -92,10 +118,6 @@ def index_template_exists? !current_config_parent.empty? end - def mapping_removals - @mapping_removals ||= mapping_fields_from(current_mapping) - mapping_fields_from(desired_mapping) - end - def mapping_type_changes @mapping_type_changes ||= begin flattened_current = Support::HashUtil.flatten_and_stringify_keys(current_mapping) @@ -108,7 +130,7 @@ def mapping_type_changes end def has_mapping_updates? - current_mapping != desired_mapping + current_mapping != desired_mapping_for_update end def settings_updates @@ -119,15 +141,19 @@ def settings_updates end end - def mapping_fields_from(mapping_hash, prefix = "") - (mapping_hash["properties"] || []).flat_map do |key, params| - field = prefix + key - if params.key?("properties") - [field] + mapping_fields_from(params, "#{field}.") - else - [field] - end - end + def desired_mapping_for_update + @desired_mapping_for_update ||= MappingUpdate.build_mapping_update( + desired: desired_mapping, + current: current_mapping, + protected_field_paths: @index_template.field_paths_protected_from_removal + ) + end + + def desired_config_parent_for_update + @desired_config_parent_for_update ||= Support::HashUtil.deep_merge( + desired_config_parent, + {"template" => {"mappings" => desired_mapping_for_update}} + ) end def desired_mapping @@ -178,43 +204,13 @@ def current_config_parent end def config_diff - @config_diff ||= Indexer::HashDiffer.diff(current_config_parent, desired_config_parent) || "(no diff)" + @config_diff ||= Indexer::HashDiffer.diff(current_config_parent, desired_config_parent_for_update) || "(no diff)" end def report_action(message) @reporter.report_action(message) end - # Helper method used to merge properties between a _desired_ configuration and a _current_ configuration. - # This is used when we are figuring out how to update an index template. We do not want to delete existing - # fields from a template--while the datastore would allow it, our schema evolution strategy depends upon - # us not dropping old unused fields. The datastore doesn't allow it on indices, anyway (though it does allow - # it on index templates). We've ran into trouble (a near SEV) when allowing the logic here to delete an unused - # field from an index template. The indexer "mapping completeness" check started failing because an old version - # of the code (from back when the field in question was still used) noticed the expected field was missing and - # started failing on every event. - # - # This helps us avoid that problem by retaining any currently existing fields. - # - # Long term, if we want to support fully "garbage collecting" these old fields on templates, we will need - # to have them get dropped in a follow up step. We could have our `update_datastore_config` script notice that - # the deployed prod indexers are at a version that will tolerate the fields being dropped, or support it - # via an opt-in flag or something. - def merge_properties(desired_object, current_object) - desired_properties = desired_object.fetch("properties") { _ = {} } - current_properties = current_object.fetch("properties") { _ = {} } - - merged_properties = desired_properties.merge(current_properties) do |key, desired, current| - if current.is_a?(::Hash) && current.key?("properties") && desired.key?("properties") - merge_properties(desired, current) - else - desired - end - end - - desired_object.merge("properties" => merged_properties) - end - def related_index_configurators # Here we fan out and get a configurator for each related index. These are generally concrete # index that are based on a template, either via being specified in our config YAML, or via diff --git a/elasticgraph-admin/lib/elastic_graph/admin/index_definition_configurator/mapping_update.rb b/elasticgraph-admin/lib/elastic_graph/admin/index_definition_configurator/mapping_update.rb new file mode 100644 index 000000000..fa8f68fa4 --- /dev/null +++ b/elasticgraph-admin/lib/elastic_graph/admin/index_definition_configurator/mapping_update.rb @@ -0,0 +1,61 @@ +# 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 + +module ElasticGraph + class Admin + module IndexDefinitionConfigurator + module MappingUpdate + # Steep's `UnannotatedEmptyCollection` diagnostic requires the annotation to be on the line + # containing the `{}` literal and cannot see it through `.freeze`, hence the two-step assignment. + empty_properties = {} # : ::Hash[::String, untyped] + EMPTY_PROPERTIES = empty_properties.freeze + + # Elasticsearch/OpenSearch do not support removing mapping fields from an index. Preserve current + # fields when building index mapping update payloads and diffs, while still allowing updates to + # existing field parameters and additions of new fields. + # + # `protected_field_paths` limits which fields missing from the desired mapping get preserved: when + # provided, a missing field is preserved only if its path (or a descendant's path) is in the set, + # and other missing fields are dropped from the built update. When `nil`, all missing fields are + # preserved. + def self.build_mapping_update(desired:, current:, protected_field_paths: nil, parent_path: "") + desired_properties = desired.fetch("properties", EMPTY_PROPERTIES) + current_properties = current.fetch("properties", EMPTY_PROPERTIES) + + preserved_current_properties = current_properties.select do |field_name, _| + desired_properties.key?(field_name) || preserve_missing_field?(protected_field_paths, "#{parent_path}#{field_name}") + end + + merged_properties = desired_properties.merge(preserved_current_properties) do |field_name, desired_value, current_value| + if current_value.is_a?(::Hash) && current_value.key?("properties") && desired_value.key?("properties") + build_mapping_update( + desired: desired_value, + current: current_value, + protected_field_paths: protected_field_paths, + parent_path: "#{parent_path}#{field_name}." + ) + else + desired_value + end + end + + desired.merge("properties" => merged_properties) + end + + # A missing field is preserved if it is protected itself, or if any protected path sits underneath + # it (dropping the field would drop the protected descendant along with it). + def self.preserve_missing_field?(protected_field_paths, path) + return true if protected_field_paths.nil? + + protected_field_paths.include?(path) || protected_field_paths.any? { |protected_path| protected_path.start_with?("#{path}.") } + end + private_class_method :preserve_missing_field? + end + end + end +end diff --git a/elasticgraph-admin/sig/elastic_graph/admin/index_definition_configurator/for_index.rbs b/elasticgraph-admin/sig/elastic_graph/admin/index_definition_configurator/for_index.rbs index 54fbd0c4b..a2a49fa72 100644 --- a/elasticgraph-admin/sig/elastic_graph/admin/index_definition_configurator/for_index.rbs +++ b/elasticgraph-admin/sig/elastic_graph/admin/index_definition_configurator/for_index.rbs @@ -28,9 +28,6 @@ module ElasticGraph def cannot_modify_mapping_field_type_error: () -> ::String def index_exists?: () -> bool - @mapping_removals: ::Array[::String]? - def mapping_removals: () -> ::Array[::String] - @mapping_type_changes: ::Array[::String]? def mapping_type_changes: () -> ::Array[::String] @@ -39,7 +36,8 @@ module ElasticGraph @settings_updates: DatastoreCore::indexSettingsHash? def settings_updates: () -> DatastoreCore::indexSettingsHash - def mapping_fields_from: (DatastoreCore::indexMappingHash, ?::String) -> ::Array[::String] + @desired_mapping_for_update: DatastoreCore::indexMappingHash? + def desired_mapping_for_update: () -> DatastoreCore::indexMappingHash def desired_mapping: () -> DatastoreCore::indexMappingHash diff --git a/elasticgraph-admin/sig/elastic_graph/admin/index_definition_configurator/for_index_template.rbs b/elasticgraph-admin/sig/elastic_graph/admin/index_definition_configurator/for_index_template.rbs index 68943db03..14826d9d0 100644 --- a/elasticgraph-admin/sig/elastic_graph/admin/index_definition_configurator/for_index_template.rbs +++ b/elasticgraph-admin/sig/elastic_graph/admin/index_definition_configurator/for_index_template.rbs @@ -26,12 +26,11 @@ module ElasticGraph @clock: singleton(::Time) def put_index_template: () -> void + def removed_fields_note: () -> ::String + def mapping_field_paths_of: (DatastoreCore::indexMappingHash, ?parent_path: ::String) -> ::Array[::String] def cannot_modify_mapping_field_type_error: () -> ::String def index_template_exists?: () -> bool - @mapping_removals: ::Array[::String]? - def mapping_removals: () -> ::Array[::String] - @mapping_type_changes: ::Array[::String]? def mapping_type_changes: () -> ::Array[::String] @@ -40,7 +39,11 @@ module ElasticGraph @settings_updates: DatastoreCore::indexSettingsHash? def settings_updates: () -> DatastoreCore::indexSettingsHash - def mapping_fields_from: (DatastoreCore::indexMappingHash, ?::String) -> ::Array[::String] + @desired_mapping_for_update: DatastoreCore::indexMappingHash? + def desired_mapping_for_update: () -> DatastoreCore::indexMappingHash + + @desired_config_parent_for_update: ::Hash[::String, untyped]? + def desired_config_parent_for_update: () -> ::Hash[::String, untyped] def desired_mapping: () -> DatastoreCore::indexMappingHash @@ -62,7 +65,6 @@ module ElasticGraph def config_diff: () -> ::String def report_action: (::String) -> void - def merge_properties: (::Hash[::String, untyped], ::Hash[::String, untyped]) -> ::Hash[::String, untyped] @related_index_configurators: ::Array[ForIndex]? def related_index_configurators: () -> ::Array[ForIndex] diff --git a/elasticgraph-admin/sig/elastic_graph/admin/index_definition_configurator/mapping_update.rbs b/elasticgraph-admin/sig/elastic_graph/admin/index_definition_configurator/mapping_update.rbs new file mode 100644 index 000000000..1e3e36207 --- /dev/null +++ b/elasticgraph-admin/sig/elastic_graph/admin/index_definition_configurator/mapping_update.rbs @@ -0,0 +1,18 @@ +module ElasticGraph + class Admin + module IndexDefinitionConfigurator + module MappingUpdate + EMPTY_PROPERTIES: ::Hash[::String, untyped] + + def self.build_mapping_update: ( + desired: ::Hash[::String, untyped], + current: ::Hash[::String, untyped], + ?protected_field_paths: ::Set[::String]?, + ?parent_path: ::String + ) -> ::Hash[::String, untyped] + + def self.preserve_missing_field?: (::Set[::String]?, ::String) -> bool + end + end + end +end diff --git a/elasticgraph-admin/spec/integration/elastic_graph/admin/index_definition_configurator/for_index_spec.rb b/elasticgraph-admin/spec/integration/elastic_graph/admin/index_definition_configurator/for_index_spec.rb index 113f36276..dc7e215cd 100644 --- a/elasticgraph-admin/spec/integration/elastic_graph/admin/index_definition_configurator/for_index_spec.rb +++ b/elasticgraph-admin/spec/integration/elastic_graph/admin/index_definition_configurator/for_index_spec.rb @@ -35,6 +35,27 @@ module IndexDefinitionConfigurator }.to make_no_datastore_write_calls("main") end + # Note: this behavior differs from index templates (see `for_index_template_spec.rb`): the datastore + # allows template fields to be dropped, but provides no way to remove fields from a concrete index. + it "is a no-op when attempting to drop a mapping field because the datastore does not support it" do + configure_index_definition(schema_def) + output_io.string = +"" # use `+` so it is not a frozen string literal. + + expect { + # Here we remove the `name` field and the `options.size` field to verify it works for both root and nested fields. + configure_index_definition(schema_def( + avoid_defining_widget_fields: %w[name], + avoid_defining_widget_options_fields: %w[size] + )) + }.to maintain { + props = get_index_definition_configuration(unique_index_name).dig("mappings", "properties") + [props.keys.sort, props.dig("options", "properties").keys.sort] + }.from([[*index_meta_fields, "created_at", "id", "name", "options"], ["color", "size"]]) + .and make_no_datastore_write_calls("main") + + expect(output_io.string).to exclude("Updated mappings", "properties.name", "properties.options.properties.size") + end + def make_datastore_calls_to_configure_index_def(index_name, subresource = nil) make_datastore_write_calls("main", "PUT #{put_index_definition_url(index_name, subresource)}") end diff --git a/elasticgraph-admin/spec/integration/elastic_graph/admin/index_definition_configurator/for_index_template_spec.rb b/elasticgraph-admin/spec/integration/elastic_graph/admin/index_definition_configurator/for_index_template_spec.rb index ef04c9cd9..faf70dab2 100644 --- a/elasticgraph-admin/spec/integration/elastic_graph/admin/index_definition_configurator/for_index_template_spec.rb +++ b/elasticgraph-admin/spec/integration/elastic_graph/admin/index_definition_configurator/for_index_template_spec.rb @@ -95,6 +95,56 @@ def fetch_artifact_configuration(schema_artifacts, index_def_name) .and make_datastore_calls_to_configure_index_def(unique_index_name, :settings) end + it "drops mapping fields whose last schema reference has been removed, while leaving concrete indices unchanged" do + configure_index_definition(schema_def) + output_io.string = +"" # use `+` so it is not a frozen string literal. + + expect { + # Here we remove the `name` field and the `options.size` field to verify it works for both root and nested fields. + configure_index_definition(schema_def( + avoid_defining_widget_fields: %w[name], + avoid_defining_widget_options_fields: %w[size] + )) + }.to change { + props = get_index_template_definition_configuration(unique_index_name).dig("mappings", "properties") + [props.keys.sort, props.dig("options", "properties").keys.sort] + }.from([[*index_meta_fields, "created_at", "id", "name", "options"], ["color", "size"]]) + .to([[*index_meta_fields, "created_at", "id", "options"], ["color"]]) + .and maintain { + props = main_datastore_client.get_index(concrete_index_name_for_now(unique_index_name)).dig("mappings", "properties") + [props.keys.sort, props.dig("options", "properties").keys.sort] + }.from([[*index_meta_fields, "created_at", "id", "name", "options"], ["color", "size"]]) + .and make_datastore_write_calls("main", "PUT #{put_index_template_definition_url(unique_index_name)}") + + expect(output_io.string).to include( + "Updated index template: `#{unique_index_name}`", + "properties.name", "properties.options.properties.size", + "Removed 2 stale field(s) from index template `#{unique_index_name}` that are no longer referenced by the schema: name, options.size.", + "New rollover indices created from this template will reject documents that still contain these fields (mappings are `dynamic: strict`)." + ) + end + + it "preserves removed mapping fields that `deleted_field` declarations still reference, since indexers running an older schema version may still write them" do + configure_index_definition(schema_def) + output_io.string = +"" # use `+` so it is not a frozen string literal. + + expect { + # Here we remove the `name` field and the `options.size` field to verify it works for both root and nested fields. + configure_index_definition(schema_def( + avoid_defining_widget_fields: %w[name], + avoid_defining_widget_options_fields: %w[size], + configure_widget: ->(t) { t.deleted_field "name" }, + configure_widget_options: ->(t) { t.deleted_field "size" } + )) + }.to maintain { + props = get_index_template_definition_configuration(unique_index_name).dig("mappings", "properties") + [props.keys.sort, props.dig("options", "properties").keys.sort] + }.from([[*index_meta_fields, "created_at", "id", "name", "options"], ["color", "size"]]) + .and make_no_datastore_write_calls("main") + + expect(output_io.string).to exclude("Updated index template", "Removed") + end + it "creates concrete indices based on `setting_overrides_by_timestamp` configuration, and avoids creating an extra index for 'now'" do jan_2020_index_name = unique_index_name + "_rollover__2020-01" diff --git a/elasticgraph-admin/spec/integration/elastic_graph/admin/index_definition_configurator/shared_examples.rb b/elasticgraph-admin/spec/integration/elastic_graph/admin/index_definition_configurator/shared_examples.rb index 0ee07c00a..8783e6faf 100644 --- a/elasticgraph-admin/spec/integration/elastic_graph/admin/index_definition_configurator/shared_examples.rb +++ b/elasticgraph-admin/spec/integration/elastic_graph/admin/index_definition_configurator/shared_examples.rb @@ -41,7 +41,6 @@ def simulate_presence_of_extra_setting(admin, index_definition_name, name, value RSpec.shared_examples_for IndexDefinitionConfigurator, :uses_datastore do let(:output_io) { StringIO.new } let(:clock) { class_double(::Time, now: ::Time.utc(2024, 3, 20, 12, 0, 0)) } - let(:mapping_removal_note_snippet) { "extra fields listed here will not actually get removed" } let(:index_meta_fields) { ["__nested_sourced_data", "__sources", "__typename", "__versions"] } it "idempotently creates an index or index template, avoiding unneeded datastore write calls" do @@ -60,7 +59,7 @@ def simulate_presence_of_extra_setting(admin, index_definition_name, name, value }.to maintain { get_index_definition_configuration(unique_index_name) } .and make_no_datastore_write_calls("main") - expect(output_io.string).not_to include(mapping_removal_note_snippet) + expect(output_io.string).to exclude("+ mappings", "+ settings") end it "allows new top-level fields to be added to an existing index or index template" do @@ -77,7 +76,6 @@ def simulate_presence_of_extra_setting(admin, index_definition_name, name, value # The printed description of what was changed should not mention settings that are not being updated. # (Requires us to normalize settings properly in the logic for this to be the case). expect(output_io.string).to include("properties.amount_cents").and exclude("coerce", "ignore_malformed", "number_of_replicas", "number_of_shards") - expect(output_io.string).not_to include(mapping_removal_note_snippet) end it "handles both `object` lists and `nested` lists" do @@ -153,6 +151,7 @@ def simulate_presence_of_extra_setting(admin, index_definition_name, name, value end } )) + output_io.string = +"" # use `+` so it is not a frozen string literal. expect { configure_index_definition(schema_def) @@ -160,6 +159,8 @@ def simulate_presence_of_extra_setting(admin, index_definition_name, name, value .from({"type" => "keyword", "meta" => {"foo" => "1"}}) .to({"type" => "keyword"}) .and make_datastore_calls_to_configure_index_def(unique_index_name, :mappings) + + expect(output_io.string).to include("properties.name.meta") end it "allows some previously unset settings to be changed on an existing index or index template" do @@ -202,24 +203,6 @@ def simulate_presence_of_extra_setting(admin, index_definition_name, name, value }.to make_no_datastore_write_calls("main") end - it "is a no-op when we attempt to drop a field because the datastore doesn't support dropping mapping fields" do - configure_index_definition(schema_def) - - expect { - # Here we remove the `name` field and the `options.size` field to verify it works for both root and nested fields. - configure_index_definition(schema_def( - avoid_defining_widget_fields: %w[name], - avoid_defining_widget_options_fields: %w[size] - )) - }.to maintain { - props = get_index_definition_configuration(unique_index_name).dig("mappings", "properties") - [props.keys.sort, props.dig("options", "properties").keys.sort] - }.from([[*index_meta_fields, "created_at", "id", "name", "options"], ["color", "size"]]) - .and make_datastore_calls_to_configure_index_def(unique_index_name, :mappings) - - expect(output_io.string).to include(mapping_removal_note_snippet) - end - it "maintains `_meta.ElasticGraph.sources` as a stateful append-only-set that remembers sources that were once active but we no longer have" do expect { configure_index_definition(schema_def( diff --git a/elasticgraph-admin/spec/unit/elastic_graph/admin/index_definition_configurator/mapping_update_spec.rb b/elasticgraph-admin/spec/unit/elastic_graph/admin/index_definition_configurator/mapping_update_spec.rb new file mode 100644 index 000000000..412d90617 --- /dev/null +++ b/elasticgraph-admin/spec/unit/elastic_graph/admin/index_definition_configurator/mapping_update_spec.rb @@ -0,0 +1,154 @@ +# 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/admin/index_definition_configurator/mapping_update" + +module ElasticGraph + class Admin + module IndexDefinitionConfigurator + RSpec.describe MappingUpdate do + describe ".build_mapping_update" do + it "preserves current mapping fields that are missing from the desired mapping, at both root and nested levels" do + current = { + "properties" => { + "name" => {"type" => "keyword"}, + "options" => { + "properties" => { + "color" => {"type" => "keyword"}, + "size" => {"type" => "keyword"} + } + } + } + } + + desired = { + "properties" => { + "options" => { + "properties" => { + "color" => {"type" => "keyword"}, + "weight" => {"type" => "integer"} + } + } + } + } + + expect(described_class.build_mapping_update(desired: desired, current: current)).to eq({ + "properties" => { + "name" => {"type" => "keyword"}, + "options" => { + "properties" => { + "color" => {"type" => "keyword"}, + "weight" => {"type" => "integer"}, + "size" => {"type" => "keyword"} + } + } + } + }) + end + + it "drops current-only fields that are not protected when `protected_field_paths` is provided, while preserving protected ones" do + current = { + "properties" => { + "name" => {"type" => "keyword"}, + "legacy_field" => {"type" => "keyword"}, + "options" => { + "properties" => { + "color" => {"type" => "keyword"}, + "size" => {"type" => "keyword"}, + "weight" => {"type" => "integer"} + } + } + } + } + + desired = { + "properties" => { + "name" => {"type" => "keyword"}, + "options" => { + "properties" => { + "color" => {"type" => "keyword"} + } + } + } + } + + result = described_class.build_mapping_update( + desired: desired, + current: current, + protected_field_paths: ["options.size"].to_set + ) + + expect(result).to eq({ + "properties" => { + "name" => {"type" => "keyword"}, + "options" => { + "properties" => { + "color" => {"type" => "keyword"}, + "size" => {"type" => "keyword"} + } + } + } + }) + end + + it "preserves an entire current-only parent field when a protected path sits underneath it, since dropping the parent would drop the protected field" do + current = { + "properties" => { + "old_parent" => { + "properties" => { + "keep_me" => {"type" => "keyword"}, + "sibling" => {"type" => "keyword"} + } + } + } + } + + desired = {"properties" => {}} + + result = described_class.build_mapping_update( + desired: desired, + current: current, + protected_field_paths: ["old_parent.keep_me"].to_set + ) + + expect(result).to eq({ + "properties" => { + "old_parent" => { + "properties" => { + "keep_me" => {"type" => "keyword"}, + "sibling" => {"type" => "keyword"} + } + } + } + }) + end + + it "favors the desired mapping for fields present in both, allowing existing field parameters to be removed" do + current = { + "properties" => { + "name" => {"type" => "keyword", "meta" => {"foo" => "1"}} + } + } + + desired = { + "properties" => { + "name" => {"type" => "keyword"} + } + } + + expect(described_class.build_mapping_update(desired: desired, current: current)).to eq({ + "properties" => { + "name" => {"type" => "keyword"} + } + }) + end + end + end + end + end +end diff --git a/elasticgraph-datastore_core/lib/elastic_graph/datastore_core/index_definition.rb b/elasticgraph-datastore_core/lib/elastic_graph/datastore_core/index_definition.rb index 94557efcd..7fb5ae8d7 100644 --- a/elasticgraph-datastore_core/lib/elastic_graph/datastore_core/index_definition.rb +++ b/elasticgraph-datastore_core/lib/elastic_graph/datastore_core/index_definition.rb @@ -30,6 +30,7 @@ def self.with(name:, runtime_metadata:, config:, datastore_clients_by_name:, sch default_sort_clauses: runtime_metadata.default_sort_fields.map(&:to_query_clause), current_sources: runtime_metadata.current_sources, fields_by_path: runtime_metadata.fields_by_path, + field_paths_protected_from_removal: runtime_metadata.field_paths_protected_from_removal, env_index_config: env_index_config, defined_clusters: config.clusters.keys.to_set, datastore_clients_by_name: datastore_clients_by_name, diff --git a/elasticgraph-datastore_core/lib/elastic_graph/datastore_core/index_definition/index.rb b/elasticgraph-datastore_core/lib/elastic_graph/datastore_core/index_definition/index.rb index 3d4ad6d30..cf83d7544 100644 --- a/elasticgraph-datastore_core/lib/elastic_graph/datastore_core/index_definition/index.rb +++ b/elasticgraph-datastore_core/lib/elastic_graph/datastore_core/index_definition/index.rb @@ -14,14 +14,14 @@ module ElasticGraph class DatastoreCore module IndexDefinition class Index < Support::MemoizableData.define( - :name, :route_with, :default_sort_clauses, :current_sources, :fields_by_path, + :name, :route_with, :default_sort_clauses, :current_sources, :fields_by_path, :field_paths_protected_from_removal, :env_index_config, :defined_clusters, :datastore_clients_by_name, :env_agnostic_settings, :has_had_multiple_sources, :sourced_from_nested_paths_by_qualified_relationship ) # `Data.define` provides all these methods: # @dynamic name, route_with, default_sort_clauses, current_sources, fields_by_path, env_index_config, env_agnostic_settings # @dynamic defined_clusters, datastore_clients_by_name, initialize, has_had_multiple_sources - # @dynamic sourced_from_nested_paths_by_qualified_relationship + # @dynamic sourced_from_nested_paths_by_qualified_relationship, field_paths_protected_from_removal # `include IndexDefinition::Base` provides all these methods. Steep should be able to detect it # but can't for some reason so we have to declare them with `@dynamic`. diff --git a/elasticgraph-datastore_core/lib/elastic_graph/datastore_core/index_definition/rollover_index_template.rb b/elasticgraph-datastore_core/lib/elastic_graph/datastore_core/index_definition/rollover_index_template.rb index 4b0ba4b42..a1f35df70 100644 --- a/elasticgraph-datastore_core/lib/elastic_graph/datastore_core/index_definition/rollover_index_template.rb +++ b/elasticgraph-datastore_core/lib/elastic_graph/datastore_core/index_definition/rollover_index_template.rb @@ -21,14 +21,14 @@ module ElasticGraph class DatastoreCore module IndexDefinition class RolloverIndexTemplate < Support::MemoizableData.define( - :name, :route_with, :default_sort_clauses, :current_sources, :fields_by_path, :env_index_config, + :name, :route_with, :default_sort_clauses, :current_sources, :fields_by_path, :field_paths_protected_from_removal, :env_index_config, :index_args, :defined_clusters, :datastore_clients_by_name, :timestamp_field_path, :frequency, :env_agnostic_settings, :has_had_multiple_sources, :sourced_from_nested_paths_by_qualified_relationship ) # `Data.define` provides all these methods: # @dynamic name, route_with, default_sort_clauses, current_sources, fields_by_path, env_index_config, env_agnostic_settings # @dynamic index_args, defined_clusters, datastore_clients_by_name, timestamp_field_path, frequency, initialize, has_had_multiple_sources - # @dynamic sourced_from_nested_paths_by_qualified_relationship + # @dynamic sourced_from_nested_paths_by_qualified_relationship, field_paths_protected_from_removal # `include IndexDefinition::Base` provides all these methods. Steep should be able to detect it # but can't for some reason so we have to declare them with `@dynamic`. diff --git a/elasticgraph-datastore_core/sig/elastic_graph/datastore_core/index_definition.rbs b/elasticgraph-datastore_core/sig/elastic_graph/datastore_core/index_definition.rbs index b82b6a313..61964c24d 100644 --- a/elasticgraph-datastore_core/sig/elastic_graph/datastore_core/index_definition.rbs +++ b/elasticgraph-datastore_core/sig/elastic_graph/datastore_core/index_definition.rbs @@ -26,6 +26,7 @@ module ElasticGraph def default_sort_clauses: () -> ::Array[::Hash[::String, ::String]] def current_sources: () -> ::Set[::String] def fields_by_path: () -> ::Hash[::String, SchemaArtifacts::RuntimeMetadata::IndexField] + def field_paths_protected_from_removal: () -> ::Set[::String] def has_had_multiple_sources: () -> bool def sourced_from_nested_paths_by_qualified_relationship: () -> ::Hash[::String, ::Array[SchemaArtifacts::RuntimeMetadata::sourcedFromNestedPathSegment]] def env_index_config: () -> Configuration::IndexDefinition diff --git a/elasticgraph-datastore_core/spec/integration/elastic_graph/datastore_core/index_definition/index_spec.rb b/elasticgraph-datastore_core/spec/integration/elastic_graph/datastore_core/index_definition/index_spec.rb index c80f07c9a..bafd7dc80 100644 --- a/elasticgraph-datastore_core/spec/integration/elastic_graph/datastore_core/index_definition/index_spec.rb +++ b/elasticgraph-datastore_core/spec/integration/elastic_graph/datastore_core/index_definition/index_spec.rb @@ -88,6 +88,7 @@ def index_def_named(name, rollover: nil) default_sort_fields: [], current_sources: [SELF_RELATIONSHIP_NAME], fields_by_path: {}, + field_paths_protected_from_removal: [], has_had_multiple_sources: false, sourced_from_nested_paths_by_qualified_relationship: {} ) diff --git a/elasticgraph-datastore_core/spec/integration/elastic_graph/datastore_core/index_definition/rollover_index_template_spec.rb b/elasticgraph-datastore_core/spec/integration/elastic_graph/datastore_core/index_definition/rollover_index_template_spec.rb index f7d05161b..42b08bec9 100644 --- a/elasticgraph-datastore_core/spec/integration/elastic_graph/datastore_core/index_definition/rollover_index_template_spec.rb +++ b/elasticgraph-datastore_core/spec/integration/elastic_graph/datastore_core/index_definition/rollover_index_template_spec.rb @@ -557,6 +557,7 @@ def index_def_named(name, rollover: nil) default_sort_fields: [], current_sources: [SELF_RELATIONSHIP_NAME], fields_by_path: {}, + field_paths_protected_from_removal: [], has_had_multiple_sources: false, sourced_from_nested_paths_by_qualified_relationship: {} ) diff --git a/elasticgraph-local/spec/acceptance/rake_tasks_spec.rb b/elasticgraph-local/spec/acceptance/rake_tasks_spec.rb index 3c8d60519..1b2be1f64 100644 --- a/elasticgraph-local/spec/acceptance/rake_tasks_spec.rb +++ b/elasticgraph-local/spec/acceptance/rake_tasks_spec.rb @@ -33,7 +33,7 @@ module Local # ...dumps schema artifacts... "datastore_config.yaml` is already up to date", # ...configures Elasticsearch... - "Updated index template: `widgets`", + "Created index template: `widgets`", # ...indexes a document... "Published batch of 1 document" ) diff --git a/elasticgraph-schema_artifacts/lib/elastic_graph/schema_artifacts/runtime_metadata/index_definition.rb b/elasticgraph-schema_artifacts/lib/elastic_graph/schema_artifacts/runtime_metadata/index_definition.rb index df542e18d..7057a73ae 100644 --- a/elasticgraph-schema_artifacts/lib/elastic_graph/schema_artifacts/runtime_metadata/index_definition.rb +++ b/elasticgraph-schema_artifacts/lib/elastic_graph/schema_artifacts/runtime_metadata/index_definition.rb @@ -17,22 +17,24 @@ module RuntimeMetadata # Runtime metadata related to a datastore index definition. # # @private - class IndexDefinition < ::Data.define(:route_with, :rollover, :default_sort_fields, :current_sources, :fields_by_path, :has_had_multiple_sources, :sourced_from_nested_paths_by_qualified_relationship) + class IndexDefinition < ::Data.define(:route_with, :rollover, :default_sort_fields, :current_sources, :fields_by_path, :field_paths_protected_from_removal, :has_had_multiple_sources, :sourced_from_nested_paths_by_qualified_relationship) ROUTE_WITH = "route_with" ROLLOVER = "rollover" DEFAULT_SORT_FIELDS = "default_sort_fields" CURRENT_SOURCES = "current_sources" FIELDS_BY_PATH = "fields_by_path" + FIELD_PATHS_PROTECTED_FROM_REMOVAL = "field_paths_protected_from_removal" HAS_HAD_MULTIPLE_SOURCES = "has_had_multiple_sources" SOURCED_FROM_NESTED_PATHS_BY_QUALIFIED_RELATIONSHIP = "sourced_from_nested_paths_by_qualified_relationship" - def initialize(route_with:, rollover:, default_sort_fields:, current_sources:, fields_by_path:, has_had_multiple_sources:, sourced_from_nested_paths_by_qualified_relationship:) + def initialize(route_with:, rollover:, default_sort_fields:, current_sources:, fields_by_path:, field_paths_protected_from_removal:, has_had_multiple_sources:, sourced_from_nested_paths_by_qualified_relationship:) super( route_with: route_with, rollover: rollover, default_sort_fields: default_sort_fields, current_sources: current_sources.to_set, fields_by_path: fields_by_path, + field_paths_protected_from_removal: field_paths_protected_from_removal.to_set, has_had_multiple_sources: has_had_multiple_sources, sourced_from_nested_paths_by_qualified_relationship: sourced_from_nested_paths_by_qualified_relationship ) @@ -45,6 +47,7 @@ def self.from_hash(hash) default_sort_fields: hash[DEFAULT_SORT_FIELDS]&.map { |h| SortField.from_hash(h) } || [], current_sources: hash[CURRENT_SOURCES] || [], fields_by_path: (hash[FIELDS_BY_PATH] || {}).transform_values { |h| IndexField.from_hash(h) }, + field_paths_protected_from_removal: hash[FIELD_PATHS_PROTECTED_FROM_REMOVAL] || [], has_had_multiple_sources: hash[HAS_HAD_MULTIPLE_SOURCES] || false, sourced_from_nested_paths_by_qualified_relationship: (hash[SOURCED_FROM_NESTED_PATHS_BY_QUALIFIED_RELATIONSHIP] || {}).transform_values { |segments| segments.map { |h| SourcedFromNestedPathSegment.from_hash(h) } } ) @@ -56,6 +59,7 @@ def to_dumpable_hash CURRENT_SOURCES => current_sources.sort, DEFAULT_SORT_FIELDS => default_sort_fields.map(&:to_dumpable_hash), FIELDS_BY_PATH => HashDumper.dump_hash(fields_by_path, &:to_dumpable_hash), + FIELD_PATHS_PROTECTED_FROM_REMOVAL => field_paths_protected_from_removal.sort, HAS_HAD_MULTIPLE_SOURCES => (true if has_had_multiple_sources), ROLLOVER => rollover&.to_dumpable_hash, ROUTE_WITH => route_with, diff --git a/elasticgraph-schema_artifacts/sig/elastic_graph/schema_artifacts/runtime_metadata/index_definition.rbs b/elasticgraph-schema_artifacts/sig/elastic_graph/schema_artifacts/runtime_metadata/index_definition.rbs index 645309b9d..a174af695 100644 --- a/elasticgraph-schema_artifacts/sig/elastic_graph/schema_artifacts/runtime_metadata/index_definition.rbs +++ b/elasticgraph-schema_artifacts/sig/elastic_graph/schema_artifacts/runtime_metadata/index_definition.rbs @@ -7,6 +7,7 @@ module ElasticGraph attr_reader default_sort_fields: ::Array[SortField] attr_reader current_sources: ::Set[::String] attr_reader fields_by_path: ::Hash[::String, IndexField] + attr_reader field_paths_protected_from_removal: ::Set[::String] attr_reader has_had_multiple_sources: bool attr_reader sourced_from_nested_paths_by_qualified_relationship: ::Hash[::String, ::Array[sourcedFromNestedPathSegment]] @@ -16,6 +17,7 @@ module ElasticGraph default_sort_fields: ::Array[SortField], current_sources: ::Set[::String], fields_by_path: ::Hash[::String, IndexField], + field_paths_protected_from_removal: ::Set[::String], has_had_multiple_sources: bool, sourced_from_nested_paths_by_qualified_relationship: ::Hash[::String, ::Array[sourcedFromNestedPathSegment]] ) -> void @@ -26,6 +28,7 @@ module ElasticGraph ?default_sort_fields: ::Array[SortField], ?current_sources: ::Enumerable[::String], ?fields_by_path: ::Hash[::String, IndexField], + ?field_paths_protected_from_removal: ::Enumerable[::String], ?has_had_multiple_sources: bool, ?sourced_from_nested_paths_by_qualified_relationship: ::Hash[::String, ::Array[sourcedFromNestedPathSegment]] ) -> IndexDefinition @@ -37,6 +40,7 @@ module ElasticGraph DEFAULT_SORT_FIELDS: "default_sort_fields" CURRENT_SOURCES: "current_sources" FIELDS_BY_PATH: "fields_by_path" + FIELD_PATHS_PROTECTED_FROM_REMOVAL: "field_paths_protected_from_removal" HAS_HAD_MULTIPLE_SOURCES: "has_had_multiple_sources" SOURCED_FROM_NESTED_PATHS_BY_QUALIFIED_RELATIONSHIP: "sourced_from_nested_paths_by_qualified_relationship" @@ -46,6 +50,7 @@ module ElasticGraph default_sort_fields: ::Array[SortField], current_sources: ::Enumerable[::String], fields_by_path: ::Hash[::String, IndexField], + field_paths_protected_from_removal: ::Enumerable[::String], has_had_multiple_sources: bool, sourced_from_nested_paths_by_qualified_relationship: ::Hash[::String, ::Array[sourcedFromNestedPathSegment]] ) -> void diff --git a/elasticgraph-schema_artifacts/spec/unit/elastic_graph/schema_artifacts/runtime_metadata/index_definition_spec.rb b/elasticgraph-schema_artifacts/spec/unit/elastic_graph/schema_artifacts/runtime_metadata/index_definition_spec.rb index 0939b9fb3..1801601cf 100644 --- a/elasticgraph-schema_artifacts/spec/unit/elastic_graph/schema_artifacts/runtime_metadata/index_definition_spec.rb +++ b/elasticgraph-schema_artifacts/spec/unit/elastic_graph/schema_artifacts/runtime_metadata/index_definition_spec.rb @@ -24,6 +24,7 @@ module RuntimeMetadata default_sort_fields: [], current_sources: Set.new, fields_by_path: {}, + field_paths_protected_from_removal: Set.new, has_had_multiple_sources: false, sourced_from_nested_paths_by_qualified_relationship: {} ) @@ -44,6 +45,14 @@ module RuntimeMetadata }) end + it "round-trips `field_paths_protected_from_removal` through a dumped hash as a sorted list" do + index_def = index_definition_with(field_paths_protected_from_removal: ["options.size", "name"]) + + dumped = index_def.to_dumpable_hash + expect(dumped["field_paths_protected_from_removal"]).to eq ["name", "options.size"] + expect(IndexDefinition.from_hash(dumped).field_paths_protected_from_removal).to eq ["name", "options.size"].to_set + end + it "prunes `has_had_multiple_sources: false` from the dumped hash but includes `has_had_multiple_sources: true`" do index_def_without_flag = index_definition_with(has_had_multiple_sources: false) expect(index_def_without_flag.to_dumpable_hash["has_had_multiple_sources"]).to be_nil diff --git a/elasticgraph-schema_artifacts/spec/unit/elastic_graph/schema_artifacts/runtime_metadata/schema_spec.rb b/elasticgraph-schema_artifacts/spec/unit/elastic_graph/schema_artifacts/runtime_metadata/schema_spec.rb index 058254673..3909a6e65 100644 --- a/elasticgraph-schema_artifacts/spec/unit/elastic_graph/schema_artifacts/runtime_metadata/schema_spec.rb +++ b/elasticgraph-schema_artifacts/spec/unit/elastic_graph/schema_artifacts/runtime_metadata/schema_spec.rb @@ -127,6 +127,7 @@ module RuntimeMetadata fields_by_path: { "foo.bar" => IndexField.new(source: "other") }, + field_paths_protected_from_removal: ["foo.legacy"], has_had_multiple_sources: false, sourced_from_nested_paths_by_qualified_relationship: { "currency" => [ @@ -141,6 +142,7 @@ module RuntimeMetadata default_sort_fields: [], current_sources: [SELF_RELATIONSHIP_NAME], fields_by_path: {}, + field_paths_protected_from_removal: [], has_had_multiple_sources: false, sourced_from_nested_paths_by_qualified_relationship: {} ), @@ -150,6 +152,7 @@ module RuntimeMetadata default_sort_fields: [], current_sources: [SELF_RELATIONSHIP_NAME], fields_by_path: {}, + field_paths_protected_from_removal: [], has_had_multiple_sources: true, sourced_from_nested_paths_by_qualified_relationship: {} ) @@ -280,6 +283,7 @@ module RuntimeMetadata "source" => "other" } }, + "field_paths_protected_from_removal" => ["foo.legacy"], "sourced_from_nested_paths_by_qualified_relationship" => { "currency" => [ {"field" => "costs", "source_field" => "cost_id"}, diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/index.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/index.rb index 460d46789..ccb9b7b4c 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/index.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/indexing/index.rb @@ -267,6 +267,7 @@ def runtime_metadata rollover: rollover_config&.runtime_metadata, current_sources: indexed_type.current_sources, fields_by_path: indexed_type.index_field_runtime_metadata_tuples.to_h, + field_paths_protected_from_removal: indexed_type.protected_from_removal_field_paths.uniq, default_sort_fields: default_sort_pairs.each_slice(2).map do |(graphql_field_path_name, direction)| SchemaArtifacts::RuntimeMetadata::SortField.new( field_path: public_field_path(graphql_field_path_name, explanation: "it is referenced as an index `default_sort` field").path_in_index, diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_subtypes.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_subtypes.rb index 7c0b200a7..a01820118 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_subtypes.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_subtypes.rb @@ -77,6 +77,12 @@ def index_field_runtime_metadata_tuples( end end + def protected_from_removal_field_paths(path_prefix: "") + resolve_subtypes.flat_map do |t| + t.protected_from_removal_field_paths(path_prefix: path_prefix) + end + end + private def merge_fields_by_name_from_subtypes diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/object_type.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/object_type.rb index 057d27394..ebaad5271 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/object_type.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/object_type.rb @@ -26,7 +26,7 @@ module SchemaElements # end class ObjectType < DelegateClass(TypeWithSubfields) # DelegateClass(TypeWithSubfields) provides the following methods: - # @dynamic name, type_ref, to_sdl, derived_graphql_types, to_indexing_field_type, current_sources, index_field_runtime_metadata_tuples, graphql_only?, relay_pagination_type, relationships_by_name + # @dynamic name, type_ref, to_sdl, derived_graphql_types, to_indexing_field_type, current_sources, index_field_runtime_metadata_tuples, protected_from_removal_field_paths, graphql_only?, relay_pagination_type, relationships_by_name include Mixins::SupportsFilteringAndAggregation # `include HasIndices` provides the following methods: 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 dfc7f5e90..bad5ae5dd 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 @@ -544,6 +544,35 @@ def index_field_runtime_metadata_tuples( end end + # Index field paths that no longer exist in the current schema but must be preserved on the index + # template mapping: as long as a `deleted_field` or `renamed_from` declaration references a field, + # an old JSON schema version may still reference it, meaning an indexer deployed with an older + # schema version may still write it. Note that the old field's `name_in_index` is not recorded + # anywhere, so we use the declared field name here--they are the same unless the old field used a + # custom `name_in_index`. + # + # @private + def protected_from_removal_field_paths(path_prefix: "") + deprecated_fields_by_old_name = + schema_def_state.deleted_fields_by_type_name_and_old_field_name.fetch(name) do + {} # : ::Hash[::String, DeprecatedElement] + end.merge(schema_def_state.renamed_fields_by_type_name_and_old_field_name.fetch(name) do + {} # : ::Hash[::String, DeprecatedElement] + end) + + own_paths = deprecated_fields_by_old_name.keys.map { |field_name| path_prefix + field_name } + + nested_paths = indexing_fields_by_name_in_index.flat_map do |name_in_index, field| + if (object_type = field.type.fully_unwrapped.as_object_type) + object_type.protected_from_removal_field_paths(path_prefix: "#{path_prefix}#{name_in_index}.") + else + [] # : ::Array[::String] + end + end + + own_paths + nested_paths + end + private def fields_sdl(&arg_selector) diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/api.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/api.rbs index c0e3b506e..07fae09ba 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/api.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/api.rbs @@ -35,6 +35,7 @@ module ElasticGraph ?parent_source: ::String, ?list_counts_state: SchemaElements::ListCountsState ) -> ::Array[[::String, SchemaArtifacts::RuntimeMetadata::IndexField]] + def protected_from_removal_field_paths: (?path_prefix: ::String) -> ::Array[::String] def derived_indexed_types: () -> ::Array[Indexing::DerivedIndexedType] def verify_graphql_correctness!: () -> void def relay_pagination_type: () -> bool diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/mixins/has_subtypes.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/mixins/has_subtypes.rbs index 737825d1e..8ca15c6db 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/mixins/has_subtypes.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/mixins/has_subtypes.rbs @@ -20,6 +20,8 @@ module ElasticGraph ?list_counts_state: SchemaElements::ListCountsState ) -> ::Array[[::String, SchemaArtifacts::RuntimeMetadata::IndexField]] + def protected_from_removal_field_paths: (?path_prefix: ::String) -> ::Array[::String] + private def merge_fields_by_name_from_subtypes: () { diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_elements/type_with_subfields.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_elements/type_with_subfields.rbs index a5a576f94..7fe6e512f 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_elements/type_with_subfields.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_elements/type_with_subfields.rbs @@ -87,6 +87,8 @@ module ElasticGraph ?list_counts_state: ListCountsState ) -> ::Array[[::String, SchemaArtifacts::RuntimeMetadata::IndexField]] + def protected_from_removal_field_paths: (?path_prefix: ::String) -> ::Array[::String] + private def fields_sdl: () ?{ (Field::argument) -> boolish } -> String diff --git a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/index_definitions_by_name_spec.rb b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/index_definitions_by_name_spec.rb index 180b1261d..3262ca2a3 100644 --- a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/index_definitions_by_name_spec.rb +++ b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/index_definitions_by_name_spec.rb @@ -1193,6 +1193,65 @@ def sourced_from_nested_paths_for_index(index_name, &schema) end end + describe "`field_paths_protected_from_removal`" do + it "is empty when the schema definition has no `deleted_field` or `renamed_from` declarations" do + widgets = index_definition_metadata_for("widgets") + + expect(widgets.field_paths_protected_from_removal).to be_empty + end + + it "includes fields referenced by `deleted_field` and `renamed_from` declarations on the indexed type" do + widgets = index_definition_metadata_for("widgets", on_my_type: ->(t) { + t.deleted_field "old_field" + t.field "new_name", "String" do |f| + f.renamed_from "old_name" + end + }) + + expect(widgets.field_paths_protected_from_removal).to eq ["old_field", "old_name"].to_set + end + + it "includes declarations on embedded object types at their full path, using the `name_in_index` of parent fields" do + runtime_metadata = define_schema do |s| + s.object_type "NestedFields" do |t| + t.field "some_id_gql", "ID", name_in_index: "some_id_index" + t.deleted_field "old_nested_field" + end + + s.object_type "MyType" do |t| + t.field "id", "ID!" + t.field "nested_fields_gql", "NestedFields", name_in_index: "nested_fields_index" + t.index "widgets" + end + end.runtime_metadata + + widgets = runtime_metadata.index_definitions_by_name.fetch("widgets") + expect(widgets.field_paths_protected_from_removal).to eq ["nested_fields_index.old_nested_field"].to_set + end + + it "includes declarations from all subtypes of an indexed union type" do + runtime_metadata = define_schema do |s| + s.object_type "Widget" do |t| + t.field "id", "ID!" + t.deleted_field "old_widget_field" + end + + s.object_type "Component" do |t| + t.field "id", "ID!" + t.deleted_field "old_component_field" + end + + s.union_type "Thing" do |t| + t.subtypes "Widget", "Component" + t.index "things" + end + end.runtime_metadata + + things = runtime_metadata.index_definitions_by_name.fetch("things") + expect(things.field_paths_protected_from_removal).to eq ["old_widget_field", "old_component_field"].to_set + end + end + def index_definition_metadata_for(name, type_definition_order: ["NestedFields", "MyType"], on_my_type: nil, **options, &block) type_defs = { "NestedFields" => ->(t) do diff --git a/spec_support/lib/elastic_graph/spec_support/runtime_metadata_support.rb b/spec_support/lib/elastic_graph/spec_support/runtime_metadata_support.rb index 33aee9181..3192d3321 100644 --- a/spec_support/lib/elastic_graph/spec_support/runtime_metadata_support.rb +++ b/spec_support/lib/elastic_graph/spec_support/runtime_metadata_support.rb @@ -117,13 +117,14 @@ def static_param_with(value) StaticParam.new(value: value) end - def index_definition_with(route_with: nil, rollover: nil, default_sort_fields: [], current_sources: [SELF_RELATIONSHIP_NAME], fields_by_path: {}, has_had_multiple_sources: false, sourced_from_nested_paths_by_qualified_relationship: {}) + def index_definition_with(route_with: nil, rollover: nil, default_sort_fields: [], current_sources: [SELF_RELATIONSHIP_NAME], fields_by_path: {}, field_paths_protected_from_removal: [], has_had_multiple_sources: false, sourced_from_nested_paths_by_qualified_relationship: {}) IndexDefinition.new( route_with: route_with, rollover: rollover, default_sort_fields: default_sort_fields, current_sources: current_sources, fields_by_path: fields_by_path, + field_paths_protected_from_removal: field_paths_protected_from_removal, has_had_multiple_sources: has_had_multiple_sources, sourced_from_nested_paths_by_qualified_relationship: sourced_from_nested_paths_by_qualified_relationship )