Drop index template fields once the schema no longer references them

Author: jwils

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

@@ -37,8 +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 template: leaves the existing
-        #     fields alone (see `put_index_template` for why).
+        #   - 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,16 +67,21 @@ def validate
 
         private
 
-        # Creates or updates the index template. While the datastore allows template fields to be dropped
-        # (in contrast to concrete indices), we preserve any fields that exist on the current template but
-        # are no longer desired (see `MappingUpdate.build_mapping_update`): new rollover indices are
-        # auto-created from the template at indexing time with `dynamic: strict` mappings, so a dropped
-        # field would cause indexing failures on those new indices for as long as any indexer (or replayed
-        # event) can still write that field. 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.
+        # 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
           action_description = if index_template_exists?
-            "Updated index template: `#{@index_template.name}`:\n#{config_diff}"
+            "Updated index template: `#{@index_template.name}`:\n#{config_diff}#{removed_fields_note}"
           else
             "Created index template: `#{@index_template.name}`"
           end
@@ -84,6 +90,25 @@ def put_index_template
           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."
@@ -117,7 +142,11 @@ def settings_updates
         end
 
         def desired_mapping_for_update
-          @desired_mapping_for_update ||= MappingUpdate.build_mapping_update(desired: desired_mapping, current: current_mapping)
+          @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

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

@@ -18,20 +18,43 @@ module MappingUpdate
         # 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.
-        def self.build_mapping_update(desired:, current:)
+        #
+        # `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)
 
-          merged_properties = desired_properties.merge(current_properties) do |_key, desired_value, current_value|
+          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)
+              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

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

@@ -26,6 +26,8 @@ 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
 

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

@@ -4,7 +4,14 @@ module ElasticGraph
       module MappingUpdate
         EMPTY_PROPERTIES: ::Hash[::String, untyped]
 
-        def self.build_mapping_update: (desired: ::Hash[::String, untyped], current: ::Hash[::String, untyped]) -> ::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

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

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"
 

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

@@ -203,25 +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 attempting to drop a mapping field, preserving the existing field" 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", "properties.name", "properties.options.properties.size")
-        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(

elasticgraph-admin/spec/unit/elastic_graph/admin/index_definition_configurator/mapping_update_spec.rb

@@ -51,6 +51,83 @@ module IndexDefinitionConfigurator
             })
           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" => {

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,