Skip to content

Drop index template fields once the schema no longer references them #1303

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jwils wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Drop index template fields once the schema no longer references them #1303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc2d25d
Improve admin mapping change output
jwils Jul 8, 2026
5d8214f
Drop index template fields once the schema no longer references them
jwils Jul 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 8 additions & 24 deletions elasticgraph-admin/lib/elastic_graph/admin/index_definition_configurator/for_index.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
Expand Down Expand Up @@ -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

Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
114 changes: 55 additions & 59 deletions ...icgraph-admin/lib/elastic_graph/admin/index_definition_configurator/for_index_template.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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."
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
61 changes: 61 additions & 0 deletions elasticgraph-admin/lib/elastic_graph/admin/index_definition_configurator/mapping_update.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
6 changes: 2 additions & 4 deletions elasticgraph-admin/sig/elastic_graph/admin/index_definition_configurator/for_index.rbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand All @@ -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

Expand Down
12 changes: 7 additions & 5 deletions ...cgraph-admin/sig/elastic_graph/admin/index_definition_configurator/for_index_template.rbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand All @@ -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

Expand All @@ -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]
Expand Down
18 changes: 18 additions & 0 deletions elasticgraph-admin/sig/elastic_graph/admin/index_definition_configurator/mapping_update.rbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Loading