Improve admin mapping change output

Author: jwils

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.merge_existing_fields_into(desired_mapping, 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

elasticgraph-admin/lib/elastic_graph/admin/index_definition_configurator/for_index_template.rb

@@ -36,9 +36,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: updates the template
+        #     to drop those fields. Related concrete indices preserve their existing fields because the datastore
+        #     provides no way to remove fields from index mappings.
         #   - 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.
@@ -67,19 +67,13 @@ def validate
         private
 
         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}"
+          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)
           report_action action_description
         end
 
@@ -92,10 +86,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)
@@ -119,17 +109,6 @@ 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
-        end
-
         def desired_mapping
           desired_config_parent.fetch("template").fetch("mappings")
         end
@@ -185,36 +164,6 @@ 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

elasticgraph-admin/lib/elastic_graph/admin/index_definition_configurator/mapping_update.rb

@@ -0,0 +1,36 @@
+# 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
+        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 concrete index update payloads and diffs, while still allowing updates to
+        # existing field parameters and additions of new fields.
+        def self.merge_existing_fields_into(desired_object, current_object)
+          desired_properties = desired_object.fetch("properties", EMPTY_PROPERTIES)
+          current_properties = current_object.fetch("properties", EMPTY_PROPERTIES)
+
+          merged_properties = desired_properties.merge(current_properties) do |_key, desired, current|
+            if current.is_a?(::Hash) && current.key?("properties") && desired.key?("properties")
+              merge_existing_fields_into(desired, current)
+            else
+              desired
+            end
+          end
+
+          desired_object.merge("properties" => merged_properties)
+        end
+      end
+    end
+  end
+end

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
 

elasticgraph-admin/sig/elastic_graph/admin/index_definition_configurator/for_index_template.rbs

@@ -29,9 +29,6 @@ module ElasticGraph
         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,8 +37,6 @@ module ElasticGraph
         @settings_updates: DatastoreCore::indexSettingsHash?
         def settings_updates: () -> DatastoreCore::indexSettingsHash
 
-        def mapping_fields_from: (DatastoreCore::indexMappingHash, ?::String) -> ::Array[::String]
-
         def desired_mapping: () -> DatastoreCore::indexMappingHash
 
         @desired_settings: DatastoreCore::indexSettingsHash?
@@ -62,7 +57,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]

elasticgraph-admin/sig/elastic_graph/admin/index_definition_configurator/mapping_update.rbs

@@ -0,0 +1,11 @@
+module ElasticGraph
+  class Admin
+    module IndexDefinitionConfigurator
+      module MappingUpdate
+        EMPTY_PROPERTIES: ::Hash[::String, untyped]
+
+        def self.merge_existing_fields_into: (::Hash[::String, untyped], ::Hash[::String, untyped]) -> ::Hash[::String, untyped]
+      end
+    end
+  end
+end

elasticgraph-admin/spec/integration/elastic_graph/admin/index_definition_configurator/for_index_spec.rb

@@ -35,6 +35,25 @@ module IndexDefinitionConfigurator
             }.to make_no_datastore_write_calls("main")
           end
 
+          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

elasticgraph-admin/spec/integration/elastic_graph/admin/index_definition_configurator/for_index_template_spec.rb

@@ -95,6 +95,30 @@ def fetch_artifact_configuration(schema_artifacts, index_def_name)
               .and make_datastore_calls_to_configure_index_def(unique_index_name, :settings)
           end
 
+          it "removes mapping fields from the index template 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")
+          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"