From de92d4b160ecd1767af18a962140c4a40aa93566 Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Tue, 21 Jul 2026 09:27:04 -0500 Subject: [PATCH 1/5] Pin protobuf field and enum value numbers in a sidecar artifact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Field and enum value numbers were assigned sequentially in definition order, so reordering or removing a field renumbered everything after it — breaking wire compatibility with previously serialized data. `schema_artifacts:dump` now reads and writes a `proto_field_numbers.yaml` sidecar artifact: - Existing numbers stay fixed even if field order changes; new fields get the lowest unused numbers. - `schema.proto` keeps the public GraphQL field names while the sidecar stores private `name_in_index` overrides. - A field renamed with `field.renamed_from` reuses its existing number under the new public name. - Enum value numbers are pinned in an `enums` section; removed values keep their numbers reserved so they are never reused (`0` remains the generated `*_UNSPECIFIED` value). - The sidecar is safe to hand-edit but strictly validated: unknown keys, non-integer numbers, out-of-range numbers, and collisions raise clear errors instead of silently reassigning numbers. The parsed mappings are modeled by a `FieldNumberMappings` class that owns conversion to and from the dumped artifact format. --- config/site/support/doctest_helper.rb | 1 + elasticgraph-proto_ingestion/README.md | 58 +++- .../lib/elastic_graph/proto_ingestion.rb | 3 + .../field_number_mappings.rb | 309 ++++++++++++++++++ .../proto_ingestion_state.rb | 2 +- .../schema_definition/results_extension.rb | 30 +- .../schema_definition/schema.rb | 52 ++- .../schema_artifact_extension.rb | 39 +++ .../schema_artifact_manager_extension.rb | 21 ++ .../schema_elements/enum_type_extension.rb | 16 +- .../object_interface_and_union_extension.rb | 28 +- .../schema_elements/scalar_type_extension.rb | 2 +- .../schema_definition/state_extension.rb | 19 +- .../sig/elastic_graph/proto_ingestion.rbs | 1 + .../field_number_mappings.rbs | 56 ++++ .../proto_ingestion_state.rbs | 5 +- .../schema_definition/results_extension.rbs | 2 + .../schema_definition/schema.rbs | 15 +- .../schema_artifact_extension.rbs | 11 + .../schema_artifact_manager_extension.rbs | 3 + .../schema_elements/enum_type_extension.rbs | 4 +- .../object_interface_and_union_extension.rbs | 6 +- .../schema_elements/scalar_type_extension.rbs | 2 +- .../schema_definition/state_extension.rbs | 1 + .../schema_definition/rake_tasks_spec.rb | 75 ++++- .../spec/support/proto_schema_support.rb | 20 +- .../field_number_mappings_spec.rb | 207 ++++++++++++ .../schema_artifact_manager_extension_spec.rb | 86 ++++- .../schema_edge_cases_spec.rb | 56 ++++ .../schema_definition/schema_spec.rb | 293 ++++++++++++++++- .../schema_definition/test_support.rb | 4 + .../schema_artifact_manager.rbs | 9 + .../schema_definition/test_support.rbs | 2 + 33 files changed, 1380 insertions(+), 58 deletions(-) create mode 100644 elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb create mode 100644 elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rb create mode 100644 elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs create mode 100644 elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rbs create mode 100644 elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb diff --git a/config/site/support/doctest_helper.rb b/config/site/support/doctest_helper.rb index 70c2c9ca2..6a7e55c22 100644 --- a/config/site/support/doctest_helper.rb +++ b/config/site/support/doctest_helper.rb @@ -64,6 +64,7 @@ module ElasticGraph @api = SchemaDefinition::API.new( SchemaArtifacts::RuntimeMetadata::SchemaElementNames.new(form: :camelCase, overrides: {}), true, + path_to_schema: "#{@tmp_dir}/schema.rb", extension_modules: extension_modules ) diff --git a/elasticgraph-proto_ingestion/README.md b/elasticgraph-proto_ingestion/README.md index ba6ffe1b6..26650d252 100644 --- a/elasticgraph-proto_ingestion/README.md +++ b/elasticgraph-proto_ingestion/README.md @@ -72,7 +72,8 @@ ElasticGraph.define_schema do |schema| end ``` -After running `bundle exec rake schema_artifacts:dump`, ElasticGraph will generate `schema.proto`. +After running `bundle exec rake schema_artifacts:dump`, ElasticGraph will generate a `schema.proto` +schema artifact, and will maintain a `proto_field_numbers.yaml` file alongside your schema definition. ## Schema Definition API @@ -118,3 +119,58 @@ Additionally: - Lists of lists (e.g. `[[Float!]!]!`) are not supported because Protocol Buffers cannot represent them directly. Schema artifact generation raises an error identifying the unsupported field. - Enum types generate `enum` definitions whose values are prefixed with the enum type name in `UPPER_SNAKE_CASE`, including a zero-valued `*_UNSPECIFIED` entry. + +## Stable Field Numbers + +`schema_artifacts:dump` automatically reads and writes `proto_field_numbers.yaml`, +stored alongside your schema definition (as a sibling of the file `path_to_schema` +points to). Existing numbers stay fixed even if field order changes, and new fields +get the next available numbers. + +Unlike the schema artifacts--which are safe to delete and regenerate at any time--this +file is part of your schema definition: it is an input to `schema.proto` generation. +While prototyping, you may delete it and regenerate it to reset the number assignments. +Once generated protos have been used to serialize data or consumed by another codebase, +however, you must not delete and regenerate it or the original number assignments will be +lost. Commit it to version control alongside your schema definition; `schema_artifacts:check` +reports it as out of date when your schema has changed but the file has not been dumped, +so CI will catch a forgotten dump. + +The file is safe to hand-edit (e.g. when resolving a merge conflict), but it is strictly +validated: unknown keys, non-integer numbers, out-of-range numbers, and duplicate numbers +are all rejected at dump time rather than silently reassigning numbers. + +Alternatives inside generated interface and union `oneof` blocks use the same stable +message-field mappings, so adding or removing a concrete subtype does not renumber the +remaining alternatives. + +`schema.proto` always uses the public GraphQL field names. When a field uses a +different `name_in_index`, the sidecar YAML stores that override privately: + +```yaml +messages: + Widget: + fields: + id: 1 + display_name: + field_number: 2 + name_in_index: displayName +``` + +If a field is renamed with `field.renamed_from`, `elasticgraph-proto_ingestion` reuses the +existing field number under the new public field name. + +## Stable Enum Value Numbers + +Enum value numbers are pinned the same way, in an `enums` section of the sidecar. Existing +values keep their numbers when other values are added or removed, new values get the next +available numbers, and removed values keep their numbers reserved so they are never reused +(number `0` is always the generated `*_UNSPECIFIED` value): + +```yaml +enums: + WidgetColor: + values: + RED: 1 + BLUE: 2 +``` diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion.rb index e48ad46e2..df5f14429 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion.rb @@ -11,5 +11,8 @@ module ElasticGraph module ProtoIngestion # The name of the generated Protocol Buffers schema file. PROTO_SCHEMA_FILE = "schema.proto" + + # The name of the generated proto field-number mapping file. + PROTO_FIELD_NUMBERS_FILE = "proto_field_numbers.yaml" end end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb new file mode 100644 index 000000000..94b0c0d9c --- /dev/null +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb @@ -0,0 +1,309 @@ +# Copyright 2024 - 2026 Block, Inc. +# +# Use of this source code is governed by an MIT-style +# license that can be found in the LICENSE file or at +# https://opensource.org/licenses/MIT. +# +# frozen_string_literal: true + +require "elastic_graph/errors" + +module ElasticGraph + module ProtoIngestion + module SchemaDefinition + # Registry of the protobuf field and enum value numbers assigned to an ElasticGraph schema. + # Parses and validates the numbers stored in the `proto_field_numbers.yaml` artifact, hands + # out the next available numbers for new fields and enum values, and serializes the updated + # mappings for the next artifact dump so that numbers stay stable over time. + class FieldNumberMappings + # Stored mapping for a single message field. + # + # @!attribute [r] field_number + # @return [Integer] + # @!attribute [r] name_in_index + # @return [String] + FieldMapping = ::Data.define(:field_number, :name_in_index) + + # The largest field number protobuf allows (2^29 - 1), per + # https://protobuf.dev/programming-guides/proto3/#assigning. + MAX_FIELD_NUMBER = 536_870_911 + # Field numbers protobuf reserves for its own implementation; they may not be used as + # field tags, per https://protobuf.dev/programming-guides/proto3/#assigning. + RESERVED_FIELD_NUMBER_RANGE = 19_000..19_999 + # The largest enum value number protobuf allows (the int32 maximum), per + # https://protobuf.dev/programming-guides/proto3/#enum. + MAX_ENUM_VALUE_NUMBER = 2_147_483_647 + + # Builds an instance from mappings in the `proto_field_numbers.yaml` artifact format, + # validating the structure and every mapped number. + # + # @param artifact [Hash, nil] parsed contents of the artifact (or a hash in the same format) + # @return [FieldNumberMappings] + # @raise [Errors::SchemaError] if the mappings deviate from the artifact format or contain invalid numbers + def self.from_artifact(artifact) + return new(mappings_by_message: {}, value_numbers_by_enum: {}) if artifact.nil? + + unless artifact.is_a?(::Hash) + raise Errors::SchemaError, "Protobuf field-number mappings must be a Hash, got: #{artifact.class}." + end + + verify_known_keys(artifact, ["messages", "enums"], "protobuf field-number mappings") + + empty_section = {} # : ::Hash[untyped, untyped] + + new( + mappings_by_message: parse_messages(artifact.fetch("messages", empty_section)), + value_numbers_by_enum: parse_enums(artifact.fetch("enums", empty_section)) + ) + end + + # @param mappings_by_message [Hash>] validated field mappings + # @param value_numbers_by_enum [Hash>] validated enum value numbers + # @api private + def initialize(mappings_by_message:, value_numbers_by_enum:) + @mappings_by_message = mappings_by_message + @value_numbers_by_enum = value_numbers_by_enum + @used_field_numbers_by_message = {} + @used_enum_value_numbers_by_enum = {} + end + + # Returns the stable protobuf number for a message field, assigning the next available + # number if the field has no stored mapping. When the field was renamed, the mapping + # stored under one of its `previous_field_names` (and its number) carries over. + # + # @param message_name [String] + # @param public_field_name [String] + # @param name_in_index [String] + # @param previous_field_names [Array] old public names of the field, if renamed + # @return [Integer] + def field_number_for(message_name:, public_field_name:, name_in_index:, previous_field_names:) + mappings_for_message = @mappings_by_message[message_name] ||= {} + used_numbers = used_field_numbers_for(message_name, mappings_for_message) + + stored_mapping = mappings_for_message.fetch(public_field_name) do + migrate_renamed_field_mapping(mappings_for_message, previous_field_names) || + FieldMapping.new(field_number: allocate_field_number(used_numbers), name_in_index: name_in_index) + end + + # Re-stamp `name_in_index` so that a change to it gets reflected in the next dump. + mapping = stored_mapping.with(name_in_index: name_in_index) + mappings_for_message[public_field_name] = mapping + + mapping.field_number + end + + # Returns the stable protobuf numbers for an enum's values, assigning the next available + # numbers to values that have no stored mapping. + # + # @param enum_name [String] + # @param value_names [Array] + # @return [Hash] + def enum_value_numbers_for(enum_name, value_names) + value_numbers = @value_numbers_by_enum[enum_name] ||= {} + used_numbers = used_enum_value_numbers_for(enum_name, value_numbers) + + value_names.to_h do |value_name| + number = value_numbers[value_name] ||= allocate_enum_value_number(used_numbers) + [value_name, number] + end + end + + # Serializes the mappings back to the `proto_field_numbers.yaml` artifact format, with + # messages and enums sorted by name and their fields and values sorted by number. + # + # @return [Hash] + def to_artifact + { + "messages" => @mappings_by_message + .sort_by(&:first) + .to_h do |message_name, mappings_by_field_name| + [message_name, { + "fields" => mappings_by_field_name.sort_by { |field_name, mapping| [mapping.field_number, field_name] }.to_h do |field_name, mapping| + artifact_mapping = + if mapping.name_in_index == field_name + mapping.field_number + else + { + "field_number" => mapping.field_number, + "name_in_index" => mapping.name_in_index + } + end + + [field_name, artifact_mapping] + end + }] + end, + "enums" => @value_numbers_by_enum + .sort_by(&:first) + .to_h do |enum_name, value_numbers| + [enum_name, { + "values" => value_numbers.sort_by { |value_name, number| [number, value_name] }.to_h + }] + end + } + end + + private + + def used_field_numbers_for(message_name, mappings_for_message) + @used_field_numbers_by_message[message_name] ||= ::Set.new(mappings_for_message.each_value.map(&:field_number)) + end + + def used_enum_value_numbers_for(enum_name, value_numbers) + @used_enum_value_numbers_by_enum[enum_name] ||= ::Set.new(value_numbers.values) + end + + # Claims and returns the smallest valid field tag not yet present in `used_numbers`, skipping + # the protobuf-reserved 19000..19999 range. Callers maintain the set (numbers loaded from a + # previously dumped artifact plus numbers allocated so far) so that allocation is + # constant-time per candidate instead of rescanning all mappings. + def allocate_field_number(used_numbers) + candidate = 1 + candidate += 1 while used_numbers.include?(candidate) || RESERVED_FIELD_NUMBER_RANGE.cover?(candidate) + used_numbers << candidate + candidate + end + + # Claims and returns the smallest positive enum value number not yet present in + # `used_numbers`. Unlike field tags, enum value numbers have no protobuf-reserved range. + def allocate_enum_value_number(used_numbers) + candidate = 1 + candidate += 1 while used_numbers.include?(candidate) + used_numbers << candidate + candidate + end + + def migrate_renamed_field_mapping(mappings_for_message, previous_field_names) + previous_field_names.each do |old_field_name| + mapping = mappings_for_message.delete(old_field_name) + return mapping if mapping + end + + nil + end + + private_class_method def self.parse_messages(messages_section) + unless messages_section.is_a?(::Hash) + raise Errors::SchemaError, "Protobuf field-number mappings must have a `messages` Hash." + end + + messages_section.to_h do |message_name, message_entry| + unless message_entry.is_a?(::Hash) + raise Errors::SchemaError, "Field-number mapping for message `#{message_name}` must be a Hash." + end + + verify_known_keys(message_entry, ["fields"], "field-number mapping for message `#{message_name}`") + + fields = message_entry["fields"] + unless fields.is_a?(::Hash) + raise Errors::SchemaError, "Field-number mapping for message `#{message_name}` must contain a `fields` Hash." + end + + parsed_fields = fields.to_h do |field_name, field_entry| + [field_name, parse_field_entry(message_name, field_name, field_entry)] + end + + verify_no_number_collisions( + parsed_fields.transform_values(&:field_number), + "field-number mapping collision in message `#{message_name}`" + ) + + [message_name, parsed_fields] + end + end + + private_class_method def self.parse_enums(enums_section) + unless enums_section.is_a?(::Hash) + raise Errors::SchemaError, "Protobuf enum value-number mappings must have an `enums` Hash." + end + + enums_section.to_h do |enum_name, enum_entry| + unless enum_entry.is_a?(::Hash) + raise Errors::SchemaError, "Enum value-number mapping for enum `#{enum_name}` must be a Hash." + end + + verify_known_keys(enum_entry, ["values"], "enum value-number mapping for enum `#{enum_name}`") + + values = enum_entry["values"] + unless values.is_a?(::Hash) + raise Errors::SchemaError, "Enum value-number mapping for enum `#{enum_name}` must contain a `values` Hash." + end + + parsed_values = values.to_h do |value_name, value_number| + unless value_number.is_a?(::Integer) && value_number.between?(1, MAX_ENUM_VALUE_NUMBER) + raise Errors::SchemaError, "Enum value-number mapping for `#{enum_name}.#{value_name}` " \ + "must be a positive integer no greater than #{MAX_ENUM_VALUE_NUMBER} " \ + "(0 is reserved for the `_UNSPECIFIED` value), got: #{value_number.inspect}." + end + + [value_name, value_number] + end + + verify_no_number_collisions(parsed_values, "enum value-number mapping collision in enum `#{enum_name}`") + + [enum_name, parsed_values] + end + end + + private_class_method def self.parse_field_entry(message_name, field_name, field_entry) + unless field_entry.is_a?(::Hash) + return FieldMapping.new( + field_number: validated_field_number(message_name, field_name, field_entry), + name_in_index: field_name + ) + end + + verify_known_keys(field_entry, ["field_number", "name_in_index"], "field-number mapping for `#{message_name}.#{field_name}`") + + field_number = field_entry.fetch("field_number") do + raise Errors::SchemaError, "Field-number mapping for `#{message_name}.#{field_name}` must include `field_number`." + end + + name_in_index = field_entry.fetch("name_in_index", field_name) + unless name_in_index.is_a?(::String) + raise Errors::SchemaError, "Field-number mapping for `#{message_name}.#{field_name}` " \ + "must use a String `name_in_index`, got: #{name_in_index.inspect}." + end + + FieldMapping.new( + field_number: validated_field_number(message_name, field_name, field_number), + name_in_index: name_in_index + ) + end + + private_class_method def self.validated_field_number(message_name, field_name, field_number) + unless field_number.is_a?(::Integer) + raise Errors::SchemaError, "Field-number mapping for `#{message_name}.#{field_name}` " \ + "must be an integer, got: #{field_number.inspect}." + end + + unless field_number.between?(1, MAX_FIELD_NUMBER) && !RESERVED_FIELD_NUMBER_RANGE.cover?(field_number) + raise Errors::SchemaError, "Field-number mapping for `#{message_name}.#{field_name}` " \ + "must be a valid protobuf field number (1 to #{MAX_FIELD_NUMBER}, excluding the reserved " \ + "#{RESERVED_FIELD_NUMBER_RANGE.begin}-#{RESERVED_FIELD_NUMBER_RANGE.end} range), got: #{field_number.inspect}." + end + + field_number + end + + private_class_method def self.verify_no_number_collisions(numbers_by_name, collision_description) + numbers_by_name.group_by(&:last).each_value do |entries| + next if entries.size < 2 + + names = entries.map(&:first).sort.map { |name| "`#{name}`" }.join(" and ") + raise Errors::SchemaError, "Protobuf #{collision_description}: " \ + "#{names} are both mapped to number #{entries.first.last}." + end + end + + private_class_method def self.verify_known_keys(hash, known_keys, description) + unknown_keys = hash.keys - known_keys + return if unknown_keys.empty? + + raise Errors::SchemaError, "Unknown key(s) in #{description}: #{unknown_keys.map(&:inspect).join(", ")}. " \ + "Supported keys: #{known_keys.map { |key| "`#{key}`" }.join(", ")}." + end + end + end + end +end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rb index 22d33d749..207d7d35f 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rb @@ -12,7 +12,7 @@ module SchemaDefinition # Holds the proto ingestion extension's schema definition state. # # @private - class ProtoIngestionState < ::Struct.new(:package_name) + class ProtoIngestionState < ::Struct.new(:package_name, :field_number_mappings) end end end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/results_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/results_extension.rb index 50fa81213..b2bd60084 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/results_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/results_extension.rb @@ -20,19 +20,31 @@ def proto_schema @proto_schema ||= protobuf_schema_generator.to_proto end + # Returns proto field-number mappings suitable for artifact storage. + # + # @return [Hash] + def proto_field_number_mappings + # Numbers get assigned as `schema.proto` renders, so we must render before reading them. + proto_schema + protobuf_schema_generator.field_number_mappings_for_artifact + end + private def protobuf_schema_generator - # The cast is needed because Steep can't see the `extend(StateExtension)` applied at - # runtime in {APIExtension.extended}. - extension_state = state # : ElasticGraph::SchemaDefinition::State & StateExtension - ingestion_state = extension_state.proto_ingestion_state + @protobuf_schema_generator ||= begin + # The cast is needed because Steep can't see the `extend(StateExtension)` applied at + # runtime in {APIExtension.extended}. + extension_state = state # : ElasticGraph::SchemaDefinition::State & StateExtension + ingestion_state = extension_state.proto_ingestion_state - Schema.new( - state: extension_state, - all_types: all_types, - package_name: ingestion_state.package_name - ) + Schema.new( + state: extension_state, + all_types: all_types, + package_name: ingestion_state.package_name, + proto_field_number_mappings: ingestion_state.field_number_mappings + ) + end end end end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb index 2458b982a..6f831eebb 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb @@ -7,6 +7,7 @@ # frozen_string_literal: true require "elastic_graph/errors" +require "elastic_graph/proto_ingestion/schema_definition/field_number_mappings" require "elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension" require "elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension" require "elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension" @@ -19,10 +20,17 @@ class Schema # @param state [ElasticGraph::SchemaDefinition::State] # @param all_types [Array] # @param package_name [String] - def initialize(state:, all_types:, package_name:) + # @param proto_field_number_mappings [Hash, nil] mappings in the `proto_field_numbers.yaml` artifact format + def initialize( + state:, + all_types:, + package_name:, + proto_field_number_mappings: {} + ) @state = state @all_types = all_types @package_name = package_name + @field_number_mappings = FieldNumberMappings.from_artifact(proto_field_number_mappings) end # Renders the schema as a valid `proto3` file. @@ -43,6 +51,32 @@ def to_proto sections.join("\n\n") + "\n" end + # Exposes the field-number and enum-value-number mappings for writing to artifact YAML. + # + # @return [Hash] + def field_number_mappings_for_artifact + @field_number_mappings.to_artifact + end + + # Returns the stable protobuf number for a message field. + # + # @api private + def field_number_for(message_name:, type_name:, public_field_name:, name_in_index:) + @field_number_mappings.field_number_for( + message_name: message_name, + public_field_name: public_field_name, + name_in_index: name_in_index, + previous_field_names: previous_field_names_for(type_name, public_field_name) + ) + end + + # Returns the stable protobuf numbers for an enum's values. + # + # @api private + def enum_value_numbers_for(enum_name, value_names) + @field_number_mappings.enum_value_numbers_for(enum_name, value_names) + end + private # Selects the indexed root types and every type transitively referenced by their protobuf @@ -65,7 +99,7 @@ def proto_types def render_definitions(types) types .sort_by(&:proto_name) - .filter_map { |type| type.to_proto(@package_name) } + .filter_map { |type| type.to_proto(self, @package_name) } .join("\n\n") end @@ -81,6 +115,20 @@ def validate_unique_enum_value_prefixes(types) enum_type_by_prefix[type.proto_enum_value_prefix] = type end end + + def previous_field_names_for(type_name, public_field_name) + previous_field_names_by_type_name_and_field_name.dig(type_name, public_field_name) || [] + end + + # Inverts the state's `old_field_name => renamed_field` index into the form we need here: + # the old public names a field's current public name was renamed from. + def previous_field_names_by_type_name_and_field_name + @previous_field_names_by_type_name_and_field_name ||= @state.renamed_fields_by_type_name_and_old_field_name.transform_values do |old_to_new| + old_to_new + .group_by { |_, renamed_field| renamed_field.name } + .transform_values { |renames| renames.map(&:first) } + end + end end end end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rb new file mode 100644 index 000000000..d265e8063 --- /dev/null +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rb @@ -0,0 +1,39 @@ +# 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 + module ProtoIngestion + module SchemaDefinition + # Extension module for {ElasticGraph::SchemaDefinition::SchemaArtifact} that replaces the standard + # "DO NOT EDIT BY HAND" preamble on `proto_field_numbers.yaml`. Unlike the proper schema artifacts, + # the file is an input to schema generation: it may be updated by hand and, after generated protos + # have been used, must not be deleted and regenerated. + # + # @private + module SchemaArtifactExtension + private + + def comment_preamble + [ + "This file is part of your schema definition--not a regenerable schema artifact. It is an", + "input to `schema.proto` generation: ElasticGraph reads it to keep protobuf field and enum", + "value numbers stable as your schema evolves, and `rake schema_artifacts:dump` maintains it", + "for you.", + "", + "You may update it by hand (e.g. to assign a specific number). While prototyping, you may", + "delete this file and regenerate it to reset the number assignments.", + "", + "Once generated protos have been used to serialize data or consumed by another codebase,", + "you must NOT delete this file and regenerate it. The original number assignments would be", + "lost, and previously serialized protobuf messages would be misread." + ].map { |line| "#{comment_prefix} #{line}".rstrip }.join("\n") + end + end + end + end +end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rb index 352c2df84..82c0dec82 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rb @@ -6,7 +6,9 @@ # # frozen_string_literal: true +require "elastic_graph/errors" require "elastic_graph/proto_ingestion" +require "elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension" module ElasticGraph module ProtoIngestion @@ -25,16 +27,35 @@ def artifacts_from_schema_def return base_artifacts if proto_schema.empty? base_artifacts + [ + proto_field_numbers_artifact, new_raw_artifact(PROTO_SCHEMA_FILE, proto_schema.chomp, comment_prefix: "//") ] end + # Builds the `proto_field_numbers.yaml` artifact. The file is part of the schema definition + # rather than a proper schema artifact--it's an input to `schema.proto` generation--so it + # lives alongside `path_to_schema` instead of in the schema artifacts directory. + def proto_field_numbers_artifact + new_yaml_artifact(PROTO_FIELD_NUMBERS_FILE, protobuf_schema_definition_results.proto_field_number_mappings) + .with(file_name: proto_field_numbers_path) + .extend(SchemaArtifactExtension) + end + + def proto_field_numbers_path + proto_ingestion_schema_definition_state.proto_field_numbers_path || + raise(Errors::SchemaError, "Cannot dump `#{PROTO_FIELD_NUMBERS_FILE}` without a configured `path_to_schema`.") + end + # Returns the wrapped {ElasticGraph::SchemaDefinition::Results} narrowed to include this # gem's `ResultsExtension`. Centralizes the Steep cast that's needed because Steep can't # see the `extend(ResultsExtension)` applied at runtime. def protobuf_schema_definition_results schema_definition_results # : ElasticGraph::SchemaDefinition::Results & ResultsExtension end + + def proto_ingestion_schema_definition_state + protobuf_schema_definition_results.state # : ElasticGraph::SchemaDefinition::State & StateExtension + end end end end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rb index da0884fd5..8af5e3e6d 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rb @@ -42,8 +42,8 @@ def value(value_name) # Renders this enum's protobuf definition. # # @return [String] - def to_proto(_package_name) - render_proto_enum + def to_proto(schema, _package_name) + render_proto_enum(schema) end # Returns the schema types referenced by this definition. @@ -83,13 +83,15 @@ def configure_derived_scalar_type(scalar_type) private - def render_proto_enum + def render_proto_enum(schema) documentation = ProtoDocumentation.comment_lines_for(doc_comment).map { |line| "#{line}\n" }.join - values = [proto_zero_value] + values_by_name.values - value_definitions = values.each.with_index.map do |raw_value, number| + values = values_by_name.values + value_numbers = schema.enum_value_numbers_for(proto_name, values_by_name.keys) + value_definitions = [proto_zero_value.to_proto(0, proto_enum_value_prefix: proto_enum_value_prefix)] + value_definitions.concat(values.map do |raw_value| value = raw_value # : ::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & EnumValueExtension - value.to_proto(number, proto_enum_value_prefix: proto_enum_value_prefix) - end + value.to_proto(value_numbers.fetch(value.name), proto_enum_value_prefix: proto_enum_value_prefix) + end) <<~PROTO.chomp #{documentation}enum #{proto_name} { diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb index d7267f332..d75d3908f 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb @@ -19,8 +19,8 @@ module ObjectInterfaceAndUnionExtension # Renders this type's protobuf message definition. # # @return [String] - def to_proto(package_name) - render_proto_message(proto_name, package_name) + def to_proto(schema, package_name) + render_proto_message(schema, proto_name, package_name) end # Returns the schema types referenced by this definition. @@ -68,19 +68,25 @@ def proto_type_reference(package_name) private - def render_proto_message(message_name, package_name) - return render_proto_oneof(message_name, package_name) if abstract? + def render_proto_message(schema, message_name, package_name) + return render_proto_oneof(schema, message_name, package_name) if abstract? fields = proto_fields documentation = ProtoDocumentation.comment_lines_for(doc_comment).map { |line| "#{line}\n" }.join - field_definitions = fields.each.with_index(1).map do |(schema_field, field), field_number| + field_definitions = fields.map do |schema_field, field| repeated, field_type = proto_field_type_for( field.type, package_name: package_name, context_field_name: field.name ) + field_number = schema.field_number_for( + message_name: message_name, + type_name: name, + public_field_name: schema_field.name, + name_in_index: field.name_in_index + ) label = "repeated " if repeated - line = " #{label}#{field_type} #{field.name} = #{field_number};" + line = " #{label}#{field_type} #{schema_field.name} = #{field_number};" field_documentation = ProtoDocumentation .comment_lines_for(schema_field.doc_comment, indent: " ") .map { |comment_line| "#{comment_line}\n" } @@ -96,13 +102,19 @@ def render_proto_message(message_name, package_name) PROTO end - def render_proto_oneof(message_name, package_name) + def render_proto_oneof(schema, message_name, package_name) # @type var abstract_type: ::ElasticGraph::SchemaDefinition::Mixins::HasSubtypes abstract_type = _ = self documentation = ProtoDocumentation.comment_lines_for(doc_comment).map { |line| "#{line}\n" }.join - alternatives = abstract_type.recursively_resolve_subtypes.each.with_index(1).map do |subtype, field_number| + alternatives = abstract_type.recursively_resolve_subtypes.map do |subtype| proto_subtype = _ = subtype field_name = Support::Casing.to_upper_snake(proto_subtype.proto_name).downcase + field_number = schema.field_number_for( + message_name: message_name, + type_name: name, + public_field_name: field_name, + name_in_index: field_name + ) " #{proto_subtype.proto_type_reference(package_name)} #{field_name} = #{field_number};" end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rb index f3dfaf898..404258788 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rb @@ -64,7 +64,7 @@ def initialize_proto_extension # Scalars map to protobuf field types and do not render standalone definitions. # # @return [nil] - def to_proto(_package_name) + def to_proto(_schema, _package_name) nil end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/state_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/state_extension.rb index f59e96343..7c02d18d2 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/state_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/state_extension.rb @@ -6,7 +6,9 @@ # # frozen_string_literal: true +require "elastic_graph/proto_ingestion" require "elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state" +require "yaml" module ElasticGraph module ProtoIngestion @@ -19,11 +21,26 @@ module StateExtension attr_reader :proto_ingestion_state def self.extended(state) + field_number_mappings = + if (path = state.proto_field_numbers_path) && ::File.exist?(path) + ::YAML.safe_load_file(path, aliases: false) + else + {} # : ::Hash[::String, untyped] + end + state.instance_variable_set( :@proto_ingestion_state, - ProtoIngestionState.new(package_name: "elasticgraph") + ProtoIngestionState.new(package_name: "elasticgraph", field_number_mappings: field_number_mappings) ) end + + def proto_field_numbers_path + return unless (path = path_to_schema) + + # The `./` prefix is dropped so that a schema definition at the root yields a path that + # reads like the other artifact paths when the rake tasks report on it. + ::File.join(::File.dirname(path), PROTO_FIELD_NUMBERS_FILE).delete_prefix("./") + end end end end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion.rbs index 7cb944875..81cc74611 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion.rbs @@ -1,5 +1,6 @@ module ElasticGraph module ProtoIngestion PROTO_SCHEMA_FILE: ::String + PROTO_FIELD_NUMBERS_FILE: ::String end end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs new file mode 100644 index 000000000..468f7e4e1 --- /dev/null +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs @@ -0,0 +1,56 @@ +module ElasticGraph + module ProtoIngestion + module SchemaDefinition + class FieldNumberMappings + class FieldMapping + attr_reader field_number: ::Integer + attr_reader name_in_index: ::String + + def self.new: (field_number: ::Integer, name_in_index: ::String) -> instance + def with: (?field_number: ::Integer, ?name_in_index: ::String) -> instance + end + + type mappingsByFieldName = ::Hash[::String, FieldMapping] + + MAX_FIELD_NUMBER: ::Integer + RESERVED_FIELD_NUMBER_RANGE: ::Range[::Integer] + MAX_ENUM_VALUE_NUMBER: ::Integer + + @mappings_by_message: ::Hash[::String, mappingsByFieldName] + @value_numbers_by_enum: ::Hash[::String, ::Hash[::String, ::Integer]] + @used_field_numbers_by_message: ::Hash[::String, ::Set[::Integer]] + @used_enum_value_numbers_by_enum: ::Hash[::String, ::Set[::Integer]] + + def self.from_artifact: (untyped) -> instance + def self.parse_messages: (untyped) -> ::Hash[::String, mappingsByFieldName] + def self.parse_enums: (untyped) -> ::Hash[::String, ::Hash[::String, ::Integer]] + def self.parse_field_entry: (::String, ::String, untyped) -> FieldMapping + def self.validated_field_number: (::String, ::String, untyped) -> ::Integer + def self.verify_no_number_collisions: (::Hash[::String, ::Integer], ::String) -> void + def self.verify_known_keys: (::Hash[untyped, untyped], ::Array[::String], ::String) -> void + + def initialize: ( + mappings_by_message: ::Hash[::String, mappingsByFieldName], + value_numbers_by_enum: ::Hash[::String, ::Hash[::String, ::Integer]] + ) -> void + + def field_number_for: ( + message_name: ::String, + public_field_name: ::String, + name_in_index: ::String, + previous_field_names: ::Array[::String] + ) -> ::Integer + def enum_value_numbers_for: (::String, ::Array[::String]) -> ::Hash[::String, ::Integer] + def to_artifact: () -> ::Hash[::String, untyped] + + private + + def used_field_numbers_for: (::String, mappingsByFieldName) -> ::Set[::Integer] + def used_enum_value_numbers_for: (::String, ::Hash[::String, ::Integer]) -> ::Set[::Integer] + def allocate_field_number: (::Set[::Integer]) -> ::Integer + def allocate_enum_value_number: (::Set[::Integer]) -> ::Integer + def migrate_renamed_field_mapping: (mappingsByFieldName, ::Array[::String]) -> FieldMapping? + end + end + end +end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rbs index c66c5d97d..377ead2f9 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rbs @@ -3,10 +3,9 @@ module ElasticGraph module SchemaDefinition class ProtoIngestionStateSupertype attr_accessor package_name: ::String + attr_accessor field_number_mappings: untyped - def initialize: ( - package_name: ::String - ) -> void + def initialize: (package_name: ::String, field_number_mappings: untyped) -> void end class ProtoIngestionState < ProtoIngestionStateSupertype diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/results_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/results_extension.rbs index 96de3ae03..1d769a23e 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/results_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/results_extension.rbs @@ -3,12 +3,14 @@ module ElasticGraph module SchemaDefinition module ResultsExtension : ::ElasticGraph::SchemaDefinition::Results def proto_schema: () -> ::String + def proto_field_number_mappings: () -> ::Hash[::String, untyped] private def protobuf_schema_generator: () -> Schema @proto_schema: ::String? + @protobuf_schema_generator: Schema? end end end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs index ce7a1cdf7..cb6d5ed5a 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs @@ -5,20 +5,33 @@ module ElasticGraph @state: ::ElasticGraph::SchemaDefinition::State @all_types: ::Array[::ElasticGraph::SchemaDefinition::SchemaElements::graphQLType] @package_name: ::String + @field_number_mappings: FieldNumberMappings + @previous_field_names_by_type_name_and_field_name: ::Hash[::String, ::Hash[::String, ::Array[::String]]]? def initialize: ( state: ::ElasticGraph::SchemaDefinition::State, all_types: ::Array[::ElasticGraph::SchemaDefinition::SchemaElements::graphQLType], - package_name: ::String + package_name: ::String, + ?proto_field_number_mappings: untyped ) -> void def to_proto: () -> ::String + def field_number_mappings_for_artifact: () -> ::Hash[::String, untyped] + def field_number_for: ( + message_name: ::String, + type_name: ::String, + public_field_name: ::String, + name_in_index: ::String + ) -> ::Integer + def enum_value_numbers_for: (::String, ::Array[::String]) -> ::Hash[::String, ::Integer] private def proto_types: () -> ::Array[untyped] def render_definitions: (::Array[untyped] types) -> ::String def validate_unique_enum_value_prefixes: (::Array[untyped] types) -> void + def previous_field_names_for: (::String, ::String) -> ::Array[::String] + def previous_field_names_by_type_name_and_field_name: () -> ::Hash[::String, ::Hash[::String, ::Array[::String]]] end end end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rbs new file mode 100644 index 000000000..3eaecde79 --- /dev/null +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rbs @@ -0,0 +1,11 @@ +module ElasticGraph + module ProtoIngestion + module SchemaDefinition + module SchemaArtifactExtension : ::ElasticGraph::SchemaDefinition::SchemaArtifact[untyped] + private + + def comment_preamble: () -> ::String + end + end + end +end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rbs index 4298c1a99..6b13106d8 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rbs @@ -5,7 +5,10 @@ module ElasticGraph private def artifacts_from_schema_def: () -> ::Array[::ElasticGraph::SchemaDefinition::SchemaArtifact[untyped]] + def proto_field_numbers_artifact: () -> ::ElasticGraph::SchemaDefinition::SchemaArtifact[::Hash[::String, untyped]] + def proto_field_numbers_path: () -> ::String def protobuf_schema_definition_results: () -> (::ElasticGraph::SchemaDefinition::Results & ResultsExtension) + def proto_ingestion_schema_definition_state: () -> (::ElasticGraph::SchemaDefinition::State & StateExtension) end end end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rbs index 4987ddf7e..6652855ba 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rbs @@ -12,13 +12,13 @@ module ElasticGraph def proto_name: () -> ::String def proto_type_reference: (::String package_name) -> ::String def proto_enum_value_prefix: () -> ::String - def to_proto: (::String package_name) -> ::String + def to_proto: (Schema schema, ::String package_name) -> ::String def referenced_proto_types: () -> ::Array[::ElasticGraph::SchemaDefinition::SchemaElements::graphQLType] def configure_derived_scalar_type: (::ElasticGraph::SchemaDefinition::SchemaElements::ScalarType) -> void private - def render_proto_enum: () -> ::String + def render_proto_enum: (Schema schema) -> ::String def proto_zero_value: () -> (::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & EnumValueExtension) def proto_zero_value_name: () -> ::String def values_by_proto_name: () -> ::Hash[::String, ::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & EnumValueExtension] diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rbs index 4265583ee..608f27a7d 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rbs @@ -10,7 +10,7 @@ module ElasticGraph def proto_name: () -> ::String def proto_type_reference: (::String package_name) -> ::String - def to_proto: (::String package_name) -> ::String + def to_proto: (Schema schema, ::String package_name) -> ::String def referenced_proto_types: () -> ::Array[::ElasticGraph::SchemaDefinition::SchemaElements::graphQLType] def self.list_depth_and_base_type: ( ::ElasticGraph::SchemaDefinition::SchemaElements::TypeReference @@ -18,8 +18,8 @@ module ElasticGraph private - def render_proto_message: (::String message_name, ::String package_name) -> ::String - def render_proto_oneof: (::String message_name, ::String package_name) -> ::String + def render_proto_message: (Schema schema, ::String message_name, ::String package_name) -> ::String + def render_proto_oneof: (Schema schema, ::String message_name, ::String package_name) -> ::String def proto_fields: () -> ::Array[[ ::ElasticGraph::SchemaDefinition::SchemaElements::Field, ::ElasticGraph::SchemaDefinition::Indexing::Field diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rbs index f5032bd25..94f0dcf15 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rbs @@ -11,7 +11,7 @@ module ElasticGraph def initialize_proto_extension: () { () -> void } -> void def proto_name: () -> ::String def proto_type_reference: (::String package_name) -> ::String - def to_proto: (::String package_name) -> nil + def to_proto: (Schema schema, ::String package_name) -> nil def referenced_proto_types: () -> ::Array[::ElasticGraph::SchemaDefinition::SchemaElements::graphQLType] end end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/state_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/state_extension.rbs index 3b29bc557..60dca722f 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/state_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/state_extension.rbs @@ -5,6 +5,7 @@ module ElasticGraph attr_reader proto_ingestion_state: ProtoIngestionState def self.extended: (::ElasticGraph::SchemaDefinition::State & StateExtension) -> void + def proto_field_numbers_path: () -> ::String? end end end diff --git a/elasticgraph-proto_ingestion/spec/integration/elastic_graph/proto_ingestion/schema_definition/rake_tasks_spec.rb b/elasticgraph-proto_ingestion/spec/integration/elastic_graph/proto_ingestion/schema_definition/rake_tasks_spec.rb index 66fae144e..a0c534486 100644 --- a/elasticgraph-proto_ingestion/spec/integration/elastic_graph/proto_ingestion/schema_definition/rake_tasks_spec.rb +++ b/elasticgraph-proto_ingestion/spec/integration/elastic_graph/proto_ingestion/schema_definition/rake_tasks_spec.rb @@ -9,6 +9,7 @@ require "elastic_graph/proto_ingestion" require "elastic_graph/proto_ingestion/schema_definition/api_extension" require "elastic_graph/schema_definition/rake_tasks" +require "yaml" module ElasticGraph module ProtoIngestion @@ -50,7 +51,70 @@ module SchemaDefinition expect { output = run_rake_with_proto("schema_artifacts:dump") expect(output.lines).to include(a_string_including("already up to date", PROTO_SCHEMA_FILE)) - }.to maintain { read_artifact(PROTO_SCHEMA_FILE) } + }.to maintain { read_artifact(PROTO_SCHEMA_FILE) }.and maintain { read_proto_field_numbers } + end + + it "persists proto field-number mappings and reuses them on the next dump" do + write_proto_schema(table_defs: <<~EOS) + s.object_type "Product" do |t| + t.field "id", "ID" + t.field "name", "String" + t.index "products" + end + EOS + + expect { + run_rake_with_proto("schema_artifacts:dump") + }.to change { read_proto_field_numbers } + .from(nil) + .to(a_string_starting_with("# This file is part of your schema definition")) + + # The file is maintained alongside the schema definition, not in the schema artifacts directory. + expect(read_artifact(PROTO_FIELD_NUMBERS_FILE)).to be_nil + expect(parsed_proto_field_numbers).to eq({ + "enums" => {}, + "messages" => { + "Product" => { + "fields" => { + "id" => 1, + "name" => 2 + } + } + } + }) + + write_proto_schema(table_defs: <<~EOS) + s.object_type "Product" do |t| + t.field "name", "String" + t.field "id", "ID" + t.index "products" + end + EOS + + run_rake_with_proto("schema_artifacts:dump") + + expect(read_artifact(PROTO_SCHEMA_FILE)).to include("string id = 1;", "string name = 2;") + end + end + + describe "schema_artifacts:check" do + it "reports `#{PROTO_FIELD_NUMBERS_FILE}` until it has been dumped alongside the schema definition" do + write_proto_schema(table_defs: <<~EOS) + s.object_type "Product" do |t| + t.field "id", "ID" + t.index "products" + end + EOS + + # The leading space verifies the file is reported at the root (alongside the schema + # definition) rather than nested in the schema artifacts directory. + expect { + run_rake_with_proto("schema_artifacts:check") + }.to abort_with a_string_including(" #{PROTO_FIELD_NUMBERS_FILE} (file does not exist)") + + run_rake_with_proto("schema_artifacts:dump") + + expect(run_rake_with_proto("schema_artifacts:check")).to include(PROTO_FIELD_NUMBERS_FILE, "up to date") end end @@ -81,6 +145,15 @@ def read_artifact(name) path = File.join("config", "schema", "artifacts", name) File.read(path) if File.exist?(path) end + + # The field-numbers file is dumped as a sibling of `path_to_schema` (`schema.rb`, above). + def read_proto_field_numbers + File.read(PROTO_FIELD_NUMBERS_FILE) if File.exist?(PROTO_FIELD_NUMBERS_FILE) + end + + def parsed_proto_field_numbers + ::YAML.safe_load(read_proto_field_numbers) + end end end end diff --git a/elasticgraph-proto_ingestion/spec/support/proto_schema_support.rb b/elasticgraph-proto_ingestion/spec/support/proto_schema_support.rb index 20aa71487..81a2dc80d 100644 --- a/elasticgraph-proto_ingestion/spec/support/proto_schema_support.rb +++ b/elasticgraph-proto_ingestion/spec/support/proto_schema_support.rb @@ -18,13 +18,25 @@ def define_proto_schema(**options, &block) define_proto_schema_results(**options, &block).proto_schema end - def define_proto_schema_results(**options, &block) + # Defines a schema and returns its `Results`. Pass the results of a previous + # `define_proto_schema_results` call as `prior_results` to seed the new schema with the + # field-number mappings the previous one generated, standing in for the + # `proto_field_numbers.yaml` file that `schema_artifacts:dump` would have written between + # the two schema definitions. (Loading that file is covered by + # `schema_artifact_manager_extension_spec` and `rake_tasks_spec`.) `prior_results` is the + # standard way to test mapping behavior; pass raw `proto_field_number_mappings:` only for + # scenarios a prior dump cannot produce (such as a hand-edited or invalid file). + def define_proto_schema_results(prior_results = nil, proto_field_number_mappings: nil, **options, &block) + mappings = proto_field_number_mappings || prior_results&.proto_field_number_mappings + define_schema( schema_element_name_form: :snake_case, extension_modules: [SchemaDefinition::APIExtension], - **options, - &block - ) + **options + ) do |schema| + schema.state.proto_ingestion_state.field_number_mappings = mappings if mappings + block.call(schema) + end end def proto_type_def_from(proto, type) diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb new file mode 100644 index 000000000..2accd0700 --- /dev/null +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb @@ -0,0 +1,207 @@ +# 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/proto_ingestion/schema_definition/field_number_mappings" + +module ElasticGraph + module ProtoIngestion + module SchemaDefinition + RSpec.describe FieldNumberMappings do + describe ".from_artifact" do + it "returns empty mappings for `nil`, as parsing an empty artifact file yields" do + mappings = FieldNumberMappings.from_artifact(nil) + + expect(mappings.to_artifact).to eq({"enums" => {}, "messages" => {}}) + end + + it "validates that the artifact is a hash" do + expect { + FieldNumberMappings.from_artifact("bad") + }.to raise_error(Errors::SchemaError, a_string_including("must be a Hash")) + end + + it "rejects unknown keys at every level so hand-edit typos do not silently discard mappings" do + [ + [{"messagez" => {}}, "\"messagez\""], + [{messages: {}}, ":messages"], + [{"messages" => {"Account" => {"fieldz" => {}}}}, "\"fieldz\""], + [{"messages" => {"Account" => {"fields" => {"id" => {"field_number" => 1, "name_in_indexx" => "x"}}}}}, "\"name_in_indexx\""], + [{"enums" => {"Status" => {"valuez" => {}}}}, "\"valuez\""] + ].each do |artifact, unknown_key| + expect { + FieldNumberMappings.from_artifact(artifact) + }.to raise_error(Errors::SchemaError, a_string_including("Unknown key(s)", unknown_key)) + end + end + + it "validates that `messages` is a hash" do + expect { + FieldNumberMappings.from_artifact({"messages" => "bad"}) + }.to raise_error(Errors::SchemaError, a_string_including("must have a `messages` Hash")) + end + + it "validates that each message's mapping is a hash" do + expect { + FieldNumberMappings.from_artifact({"messages" => {"Account" => "bad"}}) + }.to raise_error(Errors::SchemaError, a_string_including("mapping for message `Account` must be a Hash")) + end + + it "validates that each message's nested `fields` is a hash" do + expect { + FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => "bad"}}}) + }.to raise_error(Errors::SchemaError, a_string_including("must contain a `fields` Hash")) + end + + it "validates that mapped field numbers are valid protobuf field tags" do + [0, FieldNumberMappings::MAX_FIELD_NUMBER + 1, 19_000, 19_999].each do |invalid_number| + expect { + FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => {"id" => invalid_number}}}}) + }.to raise_error(Errors::SchemaError, a_string_including( + "must be a valid protobuf field number", "excluding the reserved 19000-19999 range", invalid_number.to_s + )) + end + end + + it "rejects field numbers given as strings rather than coercing them" do + ["abc", "7"].each do |non_integer| + expect { + FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => {"id" => non_integer}}}}) + }.to raise_error(Errors::SchemaError, a_string_including("must be an integer", non_integer.inspect)) + end + end + + it "rejects field numbers that are not integers rather than silently truncating them" do + expect { + FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => {"id" => 1.5}}}}) + }.to raise_error(Errors::SchemaError, a_string_including("`Account.id`", "must be an integer", "1.5")) + end + + it "raises a clear error when two fields of a message are mapped to the same number" do + expect { + FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => {"id" => 1, "name" => 1}}}}) + }.to raise_error(Errors::SchemaError, a_string_including( + "field-number mapping collision in message `Account`", + "`id` and `name`", + "number 1" + )) + end + + it "validates that structured field mappings include `field_number`" do + expect { + FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => {"id" => {"name_in_index" => "account_id"}}}}}) + }.to raise_error(Errors::SchemaError, a_string_including("must include `field_number`")) + end + + it "validates that structured field mappings use a String `name_in_index`" do + expect { + FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => {"id" => {"field_number" => 7, "name_in_index" => 123}}}}}) + }.to raise_error(Errors::SchemaError, a_string_including("must use a String `name_in_index`")) + end + + it "defaults a structured field mapping without `name_in_index` to the field name" do + mappings = FieldNumberMappings.from_artifact( + {"messages" => {"Account" => {"fields" => {"display_name" => {"field_number" => 7}}}}} + ) + + expect(mappings.to_artifact.dig("messages", "Account", "fields")).to eq({"display_name" => 7}) + end + + it "accepts maximum protobuf numbers while allowing enum values in the field-reserved range" do + artifact = { + "messages" => {"Account" => {"fields" => {"id" => FieldNumberMappings::MAX_FIELD_NUMBER}}}, + # Enum value numbers have no protobuf-reserved range, so 19000-19999 is fine here. + "enums" => {"Status" => {"values" => { + "ACTIVE" => FieldNumberMappings::MAX_ENUM_VALUE_NUMBER, + "INACTIVE" => 19_005 + }}} + } + + expect(FieldNumberMappings.from_artifact(artifact).to_artifact).to eq(artifact) + end + + it "validates that `enums` is a hash" do + expect { + FieldNumberMappings.from_artifact({"enums" => "bad"}) + }.to raise_error(Errors::SchemaError, a_string_including("must have an `enums` Hash")) + end + + it "validates that each enum's mapping is a hash" do + expect { + FieldNumberMappings.from_artifact({"enums" => {"Status" => "bad"}}) + }.to raise_error(Errors::SchemaError, a_string_including("mapping for enum `Status` must be a Hash")) + end + + it "validates that each enum's nested `values` is a hash" do + expect { + FieldNumberMappings.from_artifact({"enums" => {"Status" => {"values" => "bad"}}}) + }.to raise_error(Errors::SchemaError, a_string_including("must contain a `values` Hash")) + end + + it "validates that mapped enum value numbers are positive integers, since 0 is reserved" do + expect { + FieldNumberMappings.from_artifact({"enums" => {"Status" => {"values" => {"ACTIVE" => 0}}}}) + }.to raise_error(Errors::SchemaError, a_string_including("must be a positive integer", "_UNSPECIFIED")) + end + + it "validates that mapped enum value numbers do not exceed the int32 maximum" do + expect { + FieldNumberMappings.from_artifact( + {"enums" => {"Status" => {"values" => {"ACTIVE" => FieldNumberMappings::MAX_ENUM_VALUE_NUMBER + 1}}}} + ) + }.to raise_error(Errors::SchemaError, a_string_including("must be a positive integer no greater than 2147483647")) + end + + it "rejects enum value numbers given as strings rather than coercing them" do + expect { + FieldNumberMappings.from_artifact({"enums" => {"Status" => {"values" => {"ACTIVE" => "5"}}}}) + }.to raise_error(Errors::SchemaError, a_string_including("must be a positive integer", "\"5\"")) + end + + it "rejects enum value numbers that are not integers rather than silently truncating them" do + expect { + FieldNumberMappings.from_artifact({"enums" => {"Status" => {"values" => {"ACTIVE" => 1.5}}}}) + }.to raise_error(Errors::SchemaError, a_string_including("must be a positive integer", "1.5")) + end + + it "raises a clear error when two values of an enum are mapped to the same number" do + expect { + FieldNumberMappings.from_artifact({"enums" => {"Status" => {"values" => {"ACTIVE" => 1, "INACTIVE" => 1}}}}) + }.to raise_error(Errors::SchemaError, a_string_including( + "enum value-number mapping collision in enum `Status`", + "`ACTIVE` and `INACTIVE`", + "number 1" + )) + end + end + + describe "#to_artifact" do + it "sorts messages and enums by name, and their fields and values by number" do + mappings = FieldNumberMappings.from_artifact( + { + "messages" => { + "ZMessage" => {"fields" => {"first" => 2, "second" => 1}}, + "AMessage" => {"fields" => {"only" => 3}} + }, + "enums" => { + "ZEnum" => {"values" => {"FIRST" => 2, "SECOND" => 1}}, + "AEnum" => {"values" => {"ONLY" => 3}} + } + } + ) + + artifact = mappings.to_artifact + expect(artifact.fetch("messages").keys).to eq(["AMessage", "ZMessage"]) + expect(artifact.dig("messages", "ZMessage", "fields").keys).to eq(["second", "first"]) + expect(artifact.fetch("enums").keys).to eq(["AEnum", "ZEnum"]) + expect(artifact.dig("enums", "ZEnum", "values").keys).to eq(["SECOND", "FIRST"]) + end + end + end + end + end +end diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension_spec.rb index fc4b89f49..2dc4ccaf4 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension_spec.rb @@ -7,19 +7,40 @@ # frozen_string_literal: true require "elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension" +require "fileutils" require "stringio" module ElasticGraph module ProtoIngestion module SchemaDefinition RSpec.describe SchemaArtifactManagerExtension, :in_temp_dir do - it "dumps the proto schema artifact alongside the base artifacts" do - artifact_base_names = artifacts_for(define_indexed_type_schema) + it "locates `#{PROTO_FIELD_NUMBERS_FILE}` alongside the schema definition since it is an input to schema generation, unlike the proper schema artifacts" do + artifacts_by_file_name = artifacts_for(define_indexed_type_schema).to_h do |artifact| + [artifact.file_name, artifact] + end + + expect(artifacts_by_file_name.keys).to include( + ::File.join("config", PROTO_FIELD_NUMBERS_FILE), + ::File.join("artifacts", PROTO_SCHEMA_FILE) + ) + end - expect(artifact_base_names).to include(PROTO_SCHEMA_FILE) + it "documents the file's hand-editable input-file semantics instead of the standard `DO NOT EDIT BY HAND` preamble" do + artifact = artifacts_for(define_indexed_type_schema).find do |candidate| + ::File.basename(candidate.file_name) == PROTO_FIELD_NUMBERS_FILE + end + + artifact.dump(::StringIO.new) + contents = ::File.read(::File.join("config", PROTO_FIELD_NUMBERS_FILE)) + + expect(contents).to start_with("# This file is part of your schema definition") + expect(contents).to include("While prototyping, you may") + expect(contents).to include("Once generated protos have been used") + expect(contents).to include("you must NOT delete this file and regenerate it") + expect(contents).not_to include("DO NOT EDIT BY HAND") end - it "omits the proto schema artifact when the schema defines no indexed types" do + it "omits proto artifacts when the schema defines no indexed types" do results = define_proto_schema_results do |s| s.object_type "Point" do |t| t.field "x", "Float" @@ -34,13 +55,58 @@ module SchemaDefinition end end - artifact_base_names = artifacts_for(results) + artifact_base_names = artifact_base_names_for(results) + + expect(artifact_base_names).not_to include(PROTO_SCHEMA_FILE, PROTO_FIELD_NUMBERS_FILE) + end + + it "seeds proto generation with previously dumped field and enum numbers before an artifact manager is constructed" do + ::FileUtils.mkdir_p("config") + ::File.write(::File.join("config", PROTO_FIELD_NUMBERS_FILE), <<~YAML) + messages: + Widget: + fields: + id: 7 + enums: + Status: + values: + INACTIVE: 5 + YAML + + results = define_proto_schema_results(path_to_schema: ::File.join("config", "schema.rb")) do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE", "INACTIVE" + end + + s.object_type "Widget" do |t| + t.field "id", "ID" + t.field "status", "Status" + t.index "widgets" + end + end + + expect(results.proto_schema).to include( + "string id = 7;", + "STATUS_ACTIVE = 1;", + "STATUS_INACTIVE = 5;" + ) + end - expect(artifact_base_names).not_to include(PROTO_SCHEMA_FILE) + it "raises a clear error when dumping proto artifacts without a configured `path_to_schema`" do + results = define_proto_schema_results do |s| + s.object_type "Widget" do |t| + t.field "id", "ID" + t.index "widgets" + end + end + + expect { + artifacts_for(results) + }.to raise_error Errors::SchemaError, a_string_including("without a configured `path_to_schema`") end def define_indexed_type_schema - define_proto_schema_results do |s| + define_proto_schema_results(path_to_schema: ::File.join("config", "schema.rb")) do |s| s.object_type "Widget" do |t| t.field "id", "ID" t.index "widgets" @@ -55,7 +121,11 @@ def artifacts_for(results) output: ::StringIO.new ) - manager.send(:artifacts_from_schema_def).map { |artifact| ::File.basename(artifact.file_name) } + manager.send(:artifacts_from_schema_def) + end + + def artifact_base_names_for(results) + artifacts_for(results).map { |artifact| ::File.basename(artifact.file_name) } end end end diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb index ca265658c..a06fd6ce4 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb @@ -154,6 +154,62 @@ module SchemaDefinition expect(proto_type_def_from(proto, "Account")).to include(".elasticgraph.Status status = 2;") expect(proto_type_def_from(proto, "User")).to include(".elasticgraph.Status status = 2;") end + + it "raises when a hand-edited mappings artifact is invalid" do + # An invalid artifact can only arise from hand-editing (a prior dump is always valid), + # so this test must seed raw mappings instead of results from a prior dump. + results = define_proto_schema_results(proto_field_number_mappings: { + "messages" => {"Account" => {"fields" => {"id" => 0}}} + }) do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.index "accounts" + end + end + + expect { + results.proto_schema + }.to raise_error(Errors::SchemaError, a_string_including("must be a valid protobuf field number")) + end + + it "skips the protobuf-reserved range when allocating new field numbers" do + # Reaching the real reserved range (19000-19999) would require ~19,000 fields, so we + # stub it to a small range to verify the allocator respects the constant. + stub_const("ElasticGraph::ProtoIngestion::SchemaDefinition::FieldNumberMappings::RESERVED_FIELD_NUMBER_RANGE", 3..4) + + proto = define_proto_schema do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "name", "String" + t.field "email", "String" + t.index "accounts" + end + end + + expect(proto).to include("string id = 1;", "string name = 2;", "string email = 5;") + end + + it "allocates the next available field number when a renamed field has no old mapping entry" do + results1 = define_proto_schema_results do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.index "accounts" + end + end + + # `display_name` declares a rename from `full_name`, but no `full_name` mapping was ever dumped. + results2 = define_proto_schema_results(results1) do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "display_name", "String" do |f| + f.renamed_from "full_name" + end + t.index "accounts" + end + end + + expect(results2.proto_schema).to include("string id = 1;", "string display_name = 2;") + end end end end diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb index 5333d859c..f9037a4ea 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb @@ -242,8 +242,119 @@ module SchemaDefinition expect(proto_type_def_from(proto, "Event")).to include("int64 occurred_at = 2;") end - it "uses public field names in schema.proto when `name_in_index` differs" do - proto = define_proto_schema do |s| + it "assigns the lowest unused field number to a new field, rather than the number after the maximum used one" do + # A gap below the maximum used number can only arise from a hand-edited artifact: + # organic schema evolution never creates one, since removed fields keep their numbers + # reserved. So this test must seed raw mappings instead of results from a prior dump. + results = define_proto_schema_results(proto_field_number_mappings: { + "messages" => { + "Account" => { + "fields" => { + "id" => 7 + } + } + } + }) do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "name", "String" + t.index "accounts" + end + end + + expect(results.proto_schema).to include("string id = 7;", "string name = 1;") + end + + it "preserves proto field numbers when fields are re-ordered" do + results1 = define_proto_schema_results do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "name", "String" + t.field "age", "Int" + t.index "accounts" + end + end + + expect(proto_type_def_from(results1.proto_schema, "Account")).to include( + "string id = 1;", "string name = 2;", "int32 age = 3;" + ) + + results2 = define_proto_schema_results(results1) do |s| + s.object_type "Account" do |t| + t.field "age", "Int" + t.field "id", "ID" + t.field "name", "String" + t.index "accounts" + end + end + + expect(proto_type_def_from(results2.proto_schema, "Account")).to include( + "string id = 1;", "string name = 2;", "int32 age = 3;" + ) + expect(results2.proto_field_number_mappings).to eq(results1.proto_field_number_mappings) + end + + it "exposes generated field-number mappings as an artifact hash" do + results = define_proto_schema_results do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "name", "String" + t.index "accounts" + end + end + + expect(results.proto_field_number_mappings).to eq({ + "enums" => {}, + "messages" => { + "Account" => { + "fields" => { + "id" => 1, + "name" => 2 + } + } + } + }) + end + + it "keeps a removed field's number reserved instead of reusing it for a new field" do + results1 = define_proto_schema_results do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "legacy_field", "String" + t.index "accounts" + end + end + + expect(results1.proto_schema).to include("string legacy_field = 2;") + + # `legacy_field` has been removed and `name` added since the mappings were dumped. + results2 = define_proto_schema_results(results1) do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "name", "String" + t.index "accounts" + end + end + + expect(results2.proto_schema).to include("string id = 1;", "string name = 3;") + + # `legacy_field` keeps its number reserved in the artifact so it is never reused. + expect(results2.proto_field_number_mappings).to eq({ + "enums" => {}, + "messages" => { + "Account" => { + "fields" => { + "id" => 1, + "legacy_field" => 2, + "name" => 3 + } + } + } + }) + end + + it "uses public field names in schema.proto and stores name_in_index overrides in the mapping artifact" do + results = define_proto_schema_results do |s| s.object_type "Widget" do |t| t.field "id", "ID" t.field "display_name", "String", name_in_index: "display_name_in_index" @@ -251,9 +362,181 @@ module SchemaDefinition end end - widget = proto_type_def_from(proto, "Widget") - expect(widget).to include("string display_name = 2;") - expect(widget).not_to include("display_name_in_index") + expect(results.proto_schema).to include("string display_name = 2;") + expect(results.proto_schema).not_to include("display_name_in_index") + + expect(results.proto_field_number_mappings).to eq({ + "enums" => {}, + "messages" => { + "Widget" => { + "fields" => { + "id" => 1, + "display_name" => { + "field_number" => 2, + "name_in_index" => "display_name_in_index" + } + } + } + } + }) + end + + it "updates a stored `name_in_index` in the mapping artifact when the field's index name changes" do + results1 = define_proto_schema_results do |s| + s.object_type "Widget" do |t| + t.field "id", "ID" + t.field "display_name", "String", name_in_index: "old_index_name" + t.index "widgets" + end + end + + results2 = define_proto_schema_results(results1) do |s| + s.object_type "Widget" do |t| + t.field "id", "ID" + t.field "display_name", "String", name_in_index: "new_index_name" + t.index "widgets" + end + end + + expect(results2.proto_field_number_mappings.dig("messages", "Widget", "fields")).to eq({ + "id" => 1, + "display_name" => { + "field_number" => 2, + "name_in_index" => "new_index_name" + } + }) + end + + it "preserves a field number across a public field rename" do + results1 = define_proto_schema_results do |s| + s.object_type "Account" do |t| + t.field "full_name", "String" + t.field "id", "ID" + t.index "accounts" + end + end + + expect(results1.proto_schema).to include("string full_name = 1;") + + results2 = define_proto_schema_results(results1) do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "display_name", "String" do |f| + f.renamed_from "full_name" + end + t.index "accounts" + end + end + + expect(results2.proto_schema).to include("string id = 2;", "string display_name = 1;") + expect(results2.proto_field_number_mappings).to eq({ + "enums" => {}, + "messages" => { + "Account" => { + "fields" => { + "id" => 2, + "display_name" => 1 + } + } + } + }) + end + + it "preserves enum value numbers as the enum evolves, reserving removed values' numbers" do + results1 = define_proto_schema_results do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE", "PAUSED", "INACTIVE" + end + + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "status", "Status" + t.index "accounts" + end + end + + expect(proto_type_def_from(results1.proto_schema, "Status")).to include( + "STATUS_ACTIVE = 1;", + "STATUS_PAUSED = 2;", + "STATUS_INACTIVE = 3;" + ) + + # `PAUSED` has been removed and `ARCHIVED` added since the mappings were dumped. + results2 = define_proto_schema_results(results1) do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE", "INACTIVE", "ARCHIVED" + end + + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "status", "Status" + t.index "accounts" + end + end + + expect(proto_type_def_from(results2.proto_schema, "Status")).to include( + "STATUS_ACTIVE = 1;", + "STATUS_INACTIVE = 3;", + "STATUS_ARCHIVED = 4;" + ) + + # `PAUSED` keeps its number reserved in the artifact so it is never reused for a new value. + expect(results2.proto_field_number_mappings.fetch("enums")).to eq({ + "Status" => { + "values" => { + "ACTIVE" => 1, + "PAUSED" => 2, + "INACTIVE" => 3, + "ARCHIVED" => 4 + } + } + }) + end + + it "preserves stable field numbers for oneof alternatives as subtypes are added and removed" do + results1 = define_proto_schema_results do |s| + ["Truck", "Car", "Bike"].each do |type_name| + s.object_type type_name do |t| + t.field "id", "ID" + end + end + + s.union_type "Vehicle" do |t| + t.subtypes "Truck", "Car", "Bike" + t.index "vehicles" + end + end + + expect(proto_type_def_from(results1.proto_schema, "Vehicle")).to include( + "Truck truck = 1;", "Car car = 2;", "Bike bike = 3;" + ) + + # `Truck` has been removed and `Scooter` added since the mappings were dumped. + results2 = define_proto_schema_results(results1) do |s| + ["Car", "Bike", "Scooter"].each do |type_name| + s.object_type type_name do |t| + t.field "id", "ID" + end + end + + s.union_type "Vehicle" do |t| + t.subtypes "Car", "Bike", "Scooter" + t.index "vehicles" + end + end + + vehicle = proto_type_def_from(results2.proto_schema, "Vehicle") + expect(vehicle).to include("Car car = 2;", "Bike bike = 3;", "Scooter scooter = 4;") + + # `truck` keeps its number reserved in the artifact so it is never reused. + expect(results2.proto_field_number_mappings.fetch("messages").fetch("Vehicle")).to eq({ + "fields" => { + "truck" => 1, + "car" => 2, + "bike" => 3, + "scooter" => 4 + } + }) end it "renders independently each time it is called" do diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/test_support.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/test_support.rb index 92c4a0595..8c977a857 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/test_support.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/test_support.rb @@ -22,6 +22,7 @@ def define_schema( schema_element_name_form:, schema_element_name_overrides: {}, index_document_sizes: true, + path_to_schema: nil, extension_modules: [], derived_type_name_formats: {}, type_name_overrides: {}, @@ -37,6 +38,7 @@ def define_schema( define_schema_with_schema_elements( schema_elements, index_document_sizes: index_document_sizes, + path_to_schema: path_to_schema, extension_modules: extension_modules, derived_type_name_formats: derived_type_name_formats, type_name_overrides: type_name_overrides, @@ -49,6 +51,7 @@ def define_schema( def define_schema_with_schema_elements( schema_elements, index_document_sizes: true, + path_to_schema: nil, extension_modules: [], derived_type_name_formats: {}, type_name_overrides: {}, @@ -58,6 +61,7 @@ def define_schema_with_schema_elements( api = API.new( schema_elements, index_document_sizes, + path_to_schema: path_to_schema, extension_modules: extension_modules, derived_type_name_formats: derived_type_name_formats, type_name_overrides: type_name_overrides, diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_artifact_manager.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_artifact_manager.rbs index 88c63edb6..1a496e41a 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_artifact_manager.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_artifact_manager.rbs @@ -57,6 +57,15 @@ module ElasticGraph ^(::String) -> T, ::Array[::String], ::String) -> void + + def with: ( + ?file_name: ::String, + ?desired_contents: T, + ?dumper: ^(T) -> ::String, + ?loader: ^(::String) -> T, + ?extra_comment_lines: ::Array[::String], + ?comment_prefix: ::String + ) -> self end class SchemaArtifact[T] < SchemaArtifactSupertype[T] diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/test_support.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/test_support.rbs index 18dcc688f..03caf1699 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/test_support.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/test_support.rbs @@ -5,6 +5,7 @@ module ElasticGraph schema_element_name_form: SchemaArtifacts::RuntimeMetadata::SchemaElementNames::form, ?schema_element_name_overrides: ::Hash[::Symbol, ::String], ?index_document_sizes: bool, + ?path_to_schema: ::String?, ?extension_modules: ::Array[::Module], ?derived_type_name_formats: ::Hash[::Symbol, ::String], ?type_name_overrides: ::Hash[::Symbol, ::String], @@ -15,6 +16,7 @@ module ElasticGraph def define_schema_with_schema_elements: ( SchemaArtifacts::RuntimeMetadata::SchemaElementNames, ?index_document_sizes: bool, + ?path_to_schema: ::String?, ?extension_modules: ::Array[::Module], ?derived_type_name_formats: ::Hash[::Symbol, ::String], ?type_name_overrides: ::Hash[::Symbol, ::String], From 39a62ec58cb556f88b25b3edc7e10d28810b65ca Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Fri, 7 Aug 2026 11:53:08 -0500 Subject: [PATCH 2/5] Keep index names out of protobuf mappings Protobuf field-number state only needs public wire names and stable\nnumbers. Leave public-to-index name translation to record preparation,\nso every field mapping has the same simple integer representation. --- elasticgraph-proto_ingestion/README.md | 15 +-- .../field_number_mappings.rb | 97 ++++--------------- .../schema_definition/schema.rb | 3 +- .../object_interface_and_union_extension.rb | 6 +- .../field_number_mappings.rbs | 22 ++--- .../schema_definition/schema.rbs | 3 +- .../field_number_mappings_spec.rb | 33 ++----- .../schema_definition/schema_spec.rb | 39 ++------ 8 files changed, 48 insertions(+), 170 deletions(-) diff --git a/elasticgraph-proto_ingestion/README.md b/elasticgraph-proto_ingestion/README.md index 26650d252..9681f6642 100644 --- a/elasticgraph-proto_ingestion/README.md +++ b/elasticgraph-proto_ingestion/README.md @@ -144,18 +144,9 @@ Alternatives inside generated interface and union `oneof` blocks use the same st message-field mappings, so adding or removing a concrete subtype does not renumber the remaining alternatives. -`schema.proto` always uses the public GraphQL field names. When a field uses a -different `name_in_index`, the sidecar YAML stores that override privately: - -```yaml -messages: - Widget: - fields: - id: 1 - display_name: - field_number: 2 - name_in_index: displayName -``` +Both `schema.proto` and the sidecar use public GraphQL field names. Index field names, +including `name_in_index` overrides, are not part of the protobuf wire schema or its +stable-numbering state. If a field is renamed with `field.renamed_from`, `elasticgraph-proto_ingestion` reuses the existing field number under the new public field name. diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb index 94b0c0d9c..36dc4b459 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb @@ -16,14 +16,6 @@ module SchemaDefinition # out the next available numbers for new fields and enum values, and serializes the updated # mappings for the next artifact dump so that numbers stay stable over time. class FieldNumberMappings - # Stored mapping for a single message field. - # - # @!attribute [r] field_number - # @return [Integer] - # @!attribute [r] name_in_index - # @return [String] - FieldMapping = ::Data.define(:field_number, :name_in_index) - # The largest field number protobuf allows (2^29 - 1), per # https://protobuf.dev/programming-guides/proto3/#assigning. MAX_FIELD_NUMBER = 536_870_911 @@ -41,7 +33,7 @@ class FieldNumberMappings # @return [FieldNumberMappings] # @raise [Errors::SchemaError] if the mappings deviate from the artifact format or contain invalid numbers def self.from_artifact(artifact) - return new(mappings_by_message: {}, value_numbers_by_enum: {}) if artifact.nil? + return new(field_numbers_by_message: {}, value_numbers_by_enum: {}) if artifact.nil? unless artifact.is_a?(::Hash) raise Errors::SchemaError, "Protobuf field-number mappings must be a Hash, got: #{artifact.class}." @@ -52,16 +44,16 @@ def self.from_artifact(artifact) empty_section = {} # : ::Hash[untyped, untyped] new( - mappings_by_message: parse_messages(artifact.fetch("messages", empty_section)), + field_numbers_by_message: parse_messages(artifact.fetch("messages", empty_section)), value_numbers_by_enum: parse_enums(artifact.fetch("enums", empty_section)) ) end - # @param mappings_by_message [Hash>] validated field mappings + # @param field_numbers_by_message [Hash>] validated field-number mappings # @param value_numbers_by_enum [Hash>] validated enum value numbers # @api private - def initialize(mappings_by_message:, value_numbers_by_enum:) - @mappings_by_message = mappings_by_message + def initialize(field_numbers_by_message:, value_numbers_by_enum:) + @field_numbers_by_message = field_numbers_by_message @value_numbers_by_enum = value_numbers_by_enum @used_field_numbers_by_message = {} @used_enum_value_numbers_by_enum = {} @@ -73,23 +65,14 @@ def initialize(mappings_by_message:, value_numbers_by_enum:) # # @param message_name [String] # @param public_field_name [String] - # @param name_in_index [String] # @param previous_field_names [Array] old public names of the field, if renamed # @return [Integer] - def field_number_for(message_name:, public_field_name:, name_in_index:, previous_field_names:) - mappings_for_message = @mappings_by_message[message_name] ||= {} - used_numbers = used_field_numbers_for(message_name, mappings_for_message) - - stored_mapping = mappings_for_message.fetch(public_field_name) do - migrate_renamed_field_mapping(mappings_for_message, previous_field_names) || - FieldMapping.new(field_number: allocate_field_number(used_numbers), name_in_index: name_in_index) - end - - # Re-stamp `name_in_index` so that a change to it gets reflected in the next dump. - mapping = stored_mapping.with(name_in_index: name_in_index) - mappings_for_message[public_field_name] = mapping + def field_number_for(message_name:, public_field_name:, previous_field_names:) + field_numbers = @field_numbers_by_message[message_name] ||= {} + used_numbers = used_field_numbers_for(message_name, field_numbers) - mapping.field_number + field_numbers[public_field_name] ||= migrate_renamed_field_number(field_numbers, previous_field_names) || + allocate_field_number(used_numbers) end # Returns the stable protobuf numbers for an enum's values, assigning the next available @@ -114,23 +97,11 @@ def enum_value_numbers_for(enum_name, value_names) # @return [Hash] def to_artifact { - "messages" => @mappings_by_message + "messages" => @field_numbers_by_message .sort_by(&:first) - .to_h do |message_name, mappings_by_field_name| + .to_h do |message_name, field_numbers_by_name| [message_name, { - "fields" => mappings_by_field_name.sort_by { |field_name, mapping| [mapping.field_number, field_name] }.to_h do |field_name, mapping| - artifact_mapping = - if mapping.name_in_index == field_name - mapping.field_number - else - { - "field_number" => mapping.field_number, - "name_in_index" => mapping.name_in_index - } - end - - [field_name, artifact_mapping] - end + "fields" => field_numbers_by_name.sort_by { |field_name, number| [number, field_name] }.to_h }] end, "enums" => @value_numbers_by_enum @@ -145,8 +116,8 @@ def to_artifact private - def used_field_numbers_for(message_name, mappings_for_message) - @used_field_numbers_by_message[message_name] ||= ::Set.new(mappings_for_message.each_value.map(&:field_number)) + def used_field_numbers_for(message_name, field_numbers) + @used_field_numbers_by_message[message_name] ||= ::Set.new(field_numbers.values) end def used_enum_value_numbers_for(enum_name, value_numbers) @@ -173,10 +144,10 @@ def allocate_enum_value_number(used_numbers) candidate end - def migrate_renamed_field_mapping(mappings_for_message, previous_field_names) + def migrate_renamed_field_number(field_numbers, previous_field_names) previous_field_names.each do |old_field_name| - mapping = mappings_for_message.delete(old_field_name) - return mapping if mapping + field_number = field_numbers.delete(old_field_name) + return field_number if field_number end nil @@ -199,12 +170,12 @@ def migrate_renamed_field_mapping(mappings_for_message, previous_field_names) raise Errors::SchemaError, "Field-number mapping for message `#{message_name}` must contain a `fields` Hash." end - parsed_fields = fields.to_h do |field_name, field_entry| - [field_name, parse_field_entry(message_name, field_name, field_entry)] + parsed_fields = fields.to_h do |field_name, field_number| + [field_name, validated_field_number(message_name, field_name, field_number)] end verify_no_number_collisions( - parsed_fields.transform_values(&:field_number), + parsed_fields, "field-number mapping collision in message `#{message_name}`" ) @@ -245,32 +216,6 @@ def migrate_renamed_field_mapping(mappings_for_message, previous_field_names) end end - private_class_method def self.parse_field_entry(message_name, field_name, field_entry) - unless field_entry.is_a?(::Hash) - return FieldMapping.new( - field_number: validated_field_number(message_name, field_name, field_entry), - name_in_index: field_name - ) - end - - verify_known_keys(field_entry, ["field_number", "name_in_index"], "field-number mapping for `#{message_name}.#{field_name}`") - - field_number = field_entry.fetch("field_number") do - raise Errors::SchemaError, "Field-number mapping for `#{message_name}.#{field_name}` must include `field_number`." - end - - name_in_index = field_entry.fetch("name_in_index", field_name) - unless name_in_index.is_a?(::String) - raise Errors::SchemaError, "Field-number mapping for `#{message_name}.#{field_name}` " \ - "must use a String `name_in_index`, got: #{name_in_index.inspect}." - end - - FieldMapping.new( - field_number: validated_field_number(message_name, field_name, field_number), - name_in_index: name_in_index - ) - end - private_class_method def self.validated_field_number(message_name, field_name, field_number) unless field_number.is_a?(::Integer) raise Errors::SchemaError, "Field-number mapping for `#{message_name}.#{field_name}` " \ diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb index 6f831eebb..cea13e0a0 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb @@ -61,11 +61,10 @@ def field_number_mappings_for_artifact # Returns the stable protobuf number for a message field. # # @api private - def field_number_for(message_name:, type_name:, public_field_name:, name_in_index:) + def field_number_for(message_name:, type_name:, public_field_name:) @field_number_mappings.field_number_for( message_name: message_name, public_field_name: public_field_name, - name_in_index: name_in_index, previous_field_names: previous_field_names_for(type_name, public_field_name) ) end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb index d75d3908f..ef14ab5c8 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb @@ -82,8 +82,7 @@ def render_proto_message(schema, message_name, package_name) field_number = schema.field_number_for( message_name: message_name, type_name: name, - public_field_name: schema_field.name, - name_in_index: field.name_in_index + public_field_name: schema_field.name ) label = "repeated " if repeated line = " #{label}#{field_type} #{schema_field.name} = #{field_number};" @@ -112,8 +111,7 @@ def render_proto_oneof(schema, message_name, package_name) field_number = schema.field_number_for( message_name: message_name, type_name: name, - public_field_name: field_name, - name_in_index: field_name + public_field_name: field_name ) " #{proto_subtype.proto_type_reference(package_name)} #{field_name} = #{field_number};" end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs index 468f7e4e1..8ad765a97 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs @@ -2,42 +2,32 @@ module ElasticGraph module ProtoIngestion module SchemaDefinition class FieldNumberMappings - class FieldMapping - attr_reader field_number: ::Integer - attr_reader name_in_index: ::String - - def self.new: (field_number: ::Integer, name_in_index: ::String) -> instance - def with: (?field_number: ::Integer, ?name_in_index: ::String) -> instance - end - - type mappingsByFieldName = ::Hash[::String, FieldMapping] + type fieldNumbersByName = ::Hash[::String, ::Integer] MAX_FIELD_NUMBER: ::Integer RESERVED_FIELD_NUMBER_RANGE: ::Range[::Integer] MAX_ENUM_VALUE_NUMBER: ::Integer - @mappings_by_message: ::Hash[::String, mappingsByFieldName] + @field_numbers_by_message: ::Hash[::String, fieldNumbersByName] @value_numbers_by_enum: ::Hash[::String, ::Hash[::String, ::Integer]] @used_field_numbers_by_message: ::Hash[::String, ::Set[::Integer]] @used_enum_value_numbers_by_enum: ::Hash[::String, ::Set[::Integer]] def self.from_artifact: (untyped) -> instance - def self.parse_messages: (untyped) -> ::Hash[::String, mappingsByFieldName] + def self.parse_messages: (untyped) -> ::Hash[::String, fieldNumbersByName] def self.parse_enums: (untyped) -> ::Hash[::String, ::Hash[::String, ::Integer]] - def self.parse_field_entry: (::String, ::String, untyped) -> FieldMapping def self.validated_field_number: (::String, ::String, untyped) -> ::Integer def self.verify_no_number_collisions: (::Hash[::String, ::Integer], ::String) -> void def self.verify_known_keys: (::Hash[untyped, untyped], ::Array[::String], ::String) -> void def initialize: ( - mappings_by_message: ::Hash[::String, mappingsByFieldName], + field_numbers_by_message: ::Hash[::String, fieldNumbersByName], value_numbers_by_enum: ::Hash[::String, ::Hash[::String, ::Integer]] ) -> void def field_number_for: ( message_name: ::String, public_field_name: ::String, - name_in_index: ::String, previous_field_names: ::Array[::String] ) -> ::Integer def enum_value_numbers_for: (::String, ::Array[::String]) -> ::Hash[::String, ::Integer] @@ -45,11 +35,11 @@ module ElasticGraph private - def used_field_numbers_for: (::String, mappingsByFieldName) -> ::Set[::Integer] + def used_field_numbers_for: (::String, fieldNumbersByName) -> ::Set[::Integer] def used_enum_value_numbers_for: (::String, ::Hash[::String, ::Integer]) -> ::Set[::Integer] def allocate_field_number: (::Set[::Integer]) -> ::Integer def allocate_enum_value_number: (::Set[::Integer]) -> ::Integer - def migrate_renamed_field_mapping: (mappingsByFieldName, ::Array[::String]) -> FieldMapping? + def migrate_renamed_field_number: (fieldNumbersByName, ::Array[::String]) -> ::Integer? end end end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs index cb6d5ed5a..7ada008d7 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs @@ -20,8 +20,7 @@ module ElasticGraph def field_number_for: ( message_name: ::String, type_name: ::String, - public_field_name: ::String, - name_in_index: ::String + public_field_name: ::String ) -> ::Integer def enum_value_numbers_for: (::String, ::Array[::String]) -> ::Hash[::String, ::Integer] diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb index 2accd0700..81d10aadb 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb @@ -30,7 +30,6 @@ module SchemaDefinition [{"messagez" => {}}, "\"messagez\""], [{messages: {}}, ":messages"], [{"messages" => {"Account" => {"fieldz" => {}}}}, "\"fieldz\""], - [{"messages" => {"Account" => {"fields" => {"id" => {"field_number" => 1, "name_in_indexx" => "x"}}}}}, "\"name_in_indexx\""], [{"enums" => {"Status" => {"valuez" => {}}}}, "\"valuez\""] ].each do |artifact, unknown_key| expect { @@ -75,10 +74,14 @@ module SchemaDefinition end end - it "rejects field numbers that are not integers rather than silently truncating them" do - expect { - FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => {"id" => 1.5}}}}) - }.to raise_error(Errors::SchemaError, a_string_including("`Account.id`", "must be an integer", "1.5")) + it "rejects field numbers that are not integers rather than coercing or silently truncating them" do + [1.5, {"field_number" => 7}].each do |non_integer| + expect { + FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => {"id" => non_integer}}}}) + }.to raise_error(Errors::SchemaError, a_string_including( + "`Account.id`", "must be an integer", non_integer.inspect + )) + end end it "raises a clear error when two fields of a message are mapped to the same number" do @@ -91,26 +94,6 @@ module SchemaDefinition )) end - it "validates that structured field mappings include `field_number`" do - expect { - FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => {"id" => {"name_in_index" => "account_id"}}}}}) - }.to raise_error(Errors::SchemaError, a_string_including("must include `field_number`")) - end - - it "validates that structured field mappings use a String `name_in_index`" do - expect { - FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => {"id" => {"field_number" => 7, "name_in_index" => 123}}}}}) - }.to raise_error(Errors::SchemaError, a_string_including("must use a String `name_in_index`")) - end - - it "defaults a structured field mapping without `name_in_index` to the field name" do - mappings = FieldNumberMappings.from_artifact( - {"messages" => {"Account" => {"fields" => {"display_name" => {"field_number" => 7}}}}} - ) - - expect(mappings.to_artifact.dig("messages", "Account", "fields")).to eq({"display_name" => 7}) - end - it "accepts maximum protobuf numbers while allowing enum values in the field-reserved range" do artifact = { "messages" => {"Account" => {"fields" => {"id" => FieldNumberMappings::MAX_FIELD_NUMBER}}}, diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb index f9037a4ea..aa35ead47 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb @@ -353,35 +353,7 @@ module SchemaDefinition }) end - it "uses public field names in schema.proto and stores name_in_index overrides in the mapping artifact" do - results = define_proto_schema_results do |s| - s.object_type "Widget" do |t| - t.field "id", "ID" - t.field "display_name", "String", name_in_index: "display_name_in_index" - t.index "widgets" - end - end - - expect(results.proto_schema).to include("string display_name = 2;") - expect(results.proto_schema).not_to include("display_name_in_index") - - expect(results.proto_field_number_mappings).to eq({ - "enums" => {}, - "messages" => { - "Widget" => { - "fields" => { - "id" => 1, - "display_name" => { - "field_number" => 2, - "name_in_index" => "display_name_in_index" - } - } - } - } - }) - end - - it "updates a stored `name_in_index` in the mapping artifact when the field's index name changes" do + it "keeps index field names out of the protobuf schema and field-number mappings" do results1 = define_proto_schema_results do |s| s.object_type "Widget" do |t| t.field "id", "ID" @@ -398,12 +370,13 @@ module SchemaDefinition end end + expect(results1.proto_schema).to include("string display_name = 2;") + expect(results1.proto_schema).not_to include("old_index_name") + expect(results2.proto_schema).to eq(results1.proto_schema) + expect(results2.proto_field_number_mappings.dig("messages", "Widget", "fields")).to eq({ "id" => 1, - "display_name" => { - "field_number" => 2, - "name_in_index" => "new_index_name" - } + "display_name" => 2 }) end From 0d979c2f646f7f824c3520cfd2523eb3cfd5272e Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Fri, 7 Aug 2026 17:13:07 -0500 Subject: [PATCH 3/5] Persist next protobuf field numbers Store a per-message allocation cursor beside the stable field mappings.\nNew fields advance from that cursor instead of reclaiming gaps that may\nhave belonged to fields absent from a hand-edited mapping. --- elasticgraph-proto_ingestion/README.md | 11 +- .../field_number_mappings.rb | 109 +++++++++++++----- .../field_number_mappings.rbs | 19 ++- .../schema_definition/rake_tasks_spec.rb | 3 +- .../field_number_mappings_spec.rb | 71 +++++++++++- .../schema_artifact_manager_extension_spec.rb | 2 + .../schema_edge_cases_spec.rb | 5 +- .../schema_definition/schema_spec.rb | 25 ++-- 8 files changed, 194 insertions(+), 51 deletions(-) diff --git a/elasticgraph-proto_ingestion/README.md b/elasticgraph-proto_ingestion/README.md index 9681f6642..e05502211 100644 --- a/elasticgraph-proto_ingestion/README.md +++ b/elasticgraph-proto_ingestion/README.md @@ -125,7 +125,16 @@ Additionally: `schema_artifacts:dump` automatically reads and writes `proto_field_numbers.yaml`, stored alongside your schema definition (as a sibling of the file `path_to_schema` points to). Existing numbers stay fixed even if field order changes, and new fields -get the next available numbers. +use each message's stored `next_number`, so gaps below that cursor are never filled: + +```yaml +messages: + Widget: + fields: + id: 1 + display_name: 2 + next_number: 3 +``` Unlike the schema artifacts--which are safe to delete and regenerate at any time--this file is part of your schema definition: it is an input to `schema.proto` generation. diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb index 36dc4b459..82fda40a9 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb @@ -16,6 +16,15 @@ module SchemaDefinition # out the next available numbers for new fields and enum values, and serializes the updated # mappings for the next artifact dump so that numbers stay stable over time. class FieldNumberMappings + # Stored field numbers and allocation cursor for a single protobuf message. + # + # @!attribute [r] field_numbers_by_name + # @return [Hash] + # @!attribute [r] next_number + # @return [Integer] + MessageMapping = ::Data.define(:field_numbers_by_name, :next_number) + private_constant :MessageMapping + # The largest field number protobuf allows (2^29 - 1), per # https://protobuf.dev/programming-guides/proto3/#assigning. MAX_FIELD_NUMBER = 536_870_911 @@ -33,7 +42,7 @@ class FieldNumberMappings # @return [FieldNumberMappings] # @raise [Errors::SchemaError] if the mappings deviate from the artifact format or contain invalid numbers def self.from_artifact(artifact) - return new(field_numbers_by_message: {}, value_numbers_by_enum: {}) if artifact.nil? + return new(message_mappings_by_name: {}, value_numbers_by_enum: {}) if artifact.nil? unless artifact.is_a?(::Hash) raise Errors::SchemaError, "Protobuf field-number mappings must be a Hash, got: #{artifact.class}." @@ -44,23 +53,22 @@ def self.from_artifact(artifact) empty_section = {} # : ::Hash[untyped, untyped] new( - field_numbers_by_message: parse_messages(artifact.fetch("messages", empty_section)), + message_mappings_by_name: parse_messages(artifact.fetch("messages", empty_section)), value_numbers_by_enum: parse_enums(artifact.fetch("enums", empty_section)) ) end - # @param field_numbers_by_message [Hash>] validated field-number mappings + # @param message_mappings_by_name [Hash] validated message mappings # @param value_numbers_by_enum [Hash>] validated enum value numbers # @api private - def initialize(field_numbers_by_message:, value_numbers_by_enum:) - @field_numbers_by_message = field_numbers_by_message + def initialize(message_mappings_by_name:, value_numbers_by_enum:) + @message_mappings_by_name = message_mappings_by_name @value_numbers_by_enum = value_numbers_by_enum - @used_field_numbers_by_message = {} @used_enum_value_numbers_by_enum = {} end - # Returns the stable protobuf number for a message field, assigning the next available - # number if the field has no stored mapping. When the field was renamed, the mapping + # Returns the stable protobuf number for a message field, assigning the message's stored + # `next_number` if the field has no mapping. When the field was renamed, the mapping # stored under one of its `previous_field_names` (and its number) carries over. # # @param message_name [String] @@ -68,11 +76,19 @@ def initialize(field_numbers_by_message:, value_numbers_by_enum:) # @param previous_field_names [Array] old public names of the field, if renamed # @return [Integer] def field_number_for(message_name:, public_field_name:, previous_field_names:) - field_numbers = @field_numbers_by_message[message_name] ||= {} - used_numbers = used_field_numbers_for(message_name, field_numbers) + message_mapping = @message_mappings_by_name[message_name] ||= MessageMapping.new( + field_numbers_by_name: {}, + next_number: 1 + ) + field_numbers = message_mapping.field_numbers_by_name + + return field_numbers.fetch(public_field_name) if field_numbers.key?(public_field_name) - field_numbers[public_field_name] ||= migrate_renamed_field_number(field_numbers, previous_field_names) || - allocate_field_number(used_numbers) + if (renamed_field_number = migrate_renamed_field_number(field_numbers, previous_field_names)) + return field_numbers[public_field_name] = renamed_field_number + end + + field_numbers[public_field_name] = allocate_field_number(message_name, message_mapping) end # Returns the stable protobuf numbers for an enum's values, assigning the next available @@ -97,11 +113,12 @@ def enum_value_numbers_for(enum_name, value_names) # @return [Hash] def to_artifact { - "messages" => @field_numbers_by_message + "messages" => @message_mappings_by_name .sort_by(&:first) - .to_h do |message_name, field_numbers_by_name| + .to_h do |message_name, message_mapping| [message_name, { - "fields" => field_numbers_by_name.sort_by { |field_name, number| [number, field_name] }.to_h + "fields" => message_mapping.field_numbers_by_name.sort_by { |field_name, number| [number, field_name] }.to_h, + "next_number" => message_mapping.next_number }] end, "enums" => @value_numbers_by_enum @@ -116,23 +133,26 @@ def to_artifact private - def used_field_numbers_for(message_name, field_numbers) - @used_field_numbers_by_message[message_name] ||= ::Set.new(field_numbers.values) - end - def used_enum_value_numbers_for(enum_name, value_numbers) @used_enum_value_numbers_by_enum[enum_name] ||= ::Set.new(value_numbers.values) end - # Claims and returns the smallest valid field tag not yet present in `used_numbers`, skipping - # the protobuf-reserved 19000..19999 range. Callers maintain the set (numbers loaded from a - # previously dumped artifact plus numbers allocated so far) so that allocation is - # constant-time per candidate instead of rescanning all mappings. - def allocate_field_number(used_numbers) - candidate = 1 - candidate += 1 while used_numbers.include?(candidate) || RESERVED_FIELD_NUMBER_RANGE.cover?(candidate) - used_numbers << candidate - candidate + # Claims and returns the message's stored allocation cursor, advancing it past the claimed + # number and protobuf's reserved 19000..19999 range. + def allocate_field_number(message_name, message_mapping) + field_number = message_mapping.next_number + if field_number > MAX_FIELD_NUMBER + raise Errors::SchemaError, "Cannot allocate another protobuf field number for message `#{message_name}`: " \ + "the maximum field number (#{MAX_FIELD_NUMBER}) has been reached." + end + + next_number = field_number + 1 + if RESERVED_FIELD_NUMBER_RANGE.cover?(next_number) + next_number = RESERVED_FIELD_NUMBER_RANGE.end + 1 + end + + @message_mappings_by_name[message_name] = message_mapping.with(next_number: next_number) + field_number end # Claims and returns the smallest positive enum value number not yet present in @@ -163,7 +183,7 @@ def migrate_renamed_field_number(field_numbers, previous_field_names) raise Errors::SchemaError, "Field-number mapping for message `#{message_name}` must be a Hash." end - verify_known_keys(message_entry, ["fields"], "field-number mapping for message `#{message_name}`") + verify_known_keys(message_entry, ["fields", "next_number"], "field-number mapping for message `#{message_name}`") fields = message_entry["fields"] unless fields.is_a?(::Hash) @@ -179,7 +199,15 @@ def migrate_renamed_field_number(field_numbers, previous_field_names) "field-number mapping collision in message `#{message_name}`" ) - [message_name, parsed_fields] + next_number = + if message_entry.key?("next_number") + validated_next_number(message_name, message_entry.fetch("next_number"), parsed_fields) + else + candidate = (parsed_fields.values.max || 0) + 1 + RESERVED_FIELD_NUMBER_RANGE.cover?(candidate) ? RESERVED_FIELD_NUMBER_RANGE.end + 1 : candidate + end + + [message_name, MessageMapping.new(field_numbers_by_name: parsed_fields, next_number: next_number)] end end @@ -231,6 +259,27 @@ def migrate_renamed_field_number(field_numbers, previous_field_names) field_number end + private_class_method def self.validated_next_number(message_name, next_number, field_numbers) + unless next_number.is_a?(::Integer) + raise Errors::SchemaError, "Protobuf `next_number` for message `#{message_name}` " \ + "must be an integer, got: #{next_number.inspect}." + end + + unless next_number.between?(1, MAX_FIELD_NUMBER + 1) && !RESERVED_FIELD_NUMBER_RANGE.cover?(next_number) + raise Errors::SchemaError, "Protobuf `next_number` for message `#{message_name}` must be between 1 and " \ + "#{MAX_FIELD_NUMBER + 1}, excluding the reserved #{RESERVED_FIELD_NUMBER_RANGE.begin}-" \ + "#{RESERVED_FIELD_NUMBER_RANGE.end} range, got: #{next_number.inspect}." + end + + max_field_number = field_numbers.values.max + if max_field_number && next_number <= max_field_number + raise Errors::SchemaError, "Protobuf `next_number` for message `#{message_name}` must be greater than " \ + "every mapped field number (maximum: #{max_field_number}), got: #{next_number}." + end + + next_number + end + private_class_method def self.verify_no_number_collisions(numbers_by_name, collision_description) numbers_by_name.group_by(&:last).each_value do |entries| next if entries.size < 2 diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs index 8ad765a97..e062ae035 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs @@ -4,24 +4,32 @@ module ElasticGraph class FieldNumberMappings type fieldNumbersByName = ::Hash[::String, ::Integer] + class MessageMapping + attr_reader field_numbers_by_name: fieldNumbersByName + attr_reader next_number: ::Integer + + def self.new: (field_numbers_by_name: fieldNumbersByName, next_number: ::Integer) -> instance + def with: (?field_numbers_by_name: fieldNumbersByName, ?next_number: ::Integer) -> instance + end + MAX_FIELD_NUMBER: ::Integer RESERVED_FIELD_NUMBER_RANGE: ::Range[::Integer] MAX_ENUM_VALUE_NUMBER: ::Integer - @field_numbers_by_message: ::Hash[::String, fieldNumbersByName] + @message_mappings_by_name: ::Hash[::String, MessageMapping] @value_numbers_by_enum: ::Hash[::String, ::Hash[::String, ::Integer]] - @used_field_numbers_by_message: ::Hash[::String, ::Set[::Integer]] @used_enum_value_numbers_by_enum: ::Hash[::String, ::Set[::Integer]] def self.from_artifact: (untyped) -> instance - def self.parse_messages: (untyped) -> ::Hash[::String, fieldNumbersByName] + def self.parse_messages: (untyped) -> ::Hash[::String, MessageMapping] def self.parse_enums: (untyped) -> ::Hash[::String, ::Hash[::String, ::Integer]] def self.validated_field_number: (::String, ::String, untyped) -> ::Integer + def self.validated_next_number: (::String, untyped, fieldNumbersByName) -> ::Integer def self.verify_no_number_collisions: (::Hash[::String, ::Integer], ::String) -> void def self.verify_known_keys: (::Hash[untyped, untyped], ::Array[::String], ::String) -> void def initialize: ( - field_numbers_by_message: ::Hash[::String, fieldNumbersByName], + message_mappings_by_name: ::Hash[::String, MessageMapping], value_numbers_by_enum: ::Hash[::String, ::Hash[::String, ::Integer]] ) -> void @@ -35,9 +43,8 @@ module ElasticGraph private - def used_field_numbers_for: (::String, fieldNumbersByName) -> ::Set[::Integer] def used_enum_value_numbers_for: (::String, ::Hash[::String, ::Integer]) -> ::Set[::Integer] - def allocate_field_number: (::Set[::Integer]) -> ::Integer + def allocate_field_number: (::String, MessageMapping) -> ::Integer def allocate_enum_value_number: (::Set[::Integer]) -> ::Integer def migrate_renamed_field_number: (fieldNumbersByName, ::Array[::String]) -> ::Integer? end diff --git a/elasticgraph-proto_ingestion/spec/integration/elastic_graph/proto_ingestion/schema_definition/rake_tasks_spec.rb b/elasticgraph-proto_ingestion/spec/integration/elastic_graph/proto_ingestion/schema_definition/rake_tasks_spec.rb index a0c534486..fa686a45a 100644 --- a/elasticgraph-proto_ingestion/spec/integration/elastic_graph/proto_ingestion/schema_definition/rake_tasks_spec.rb +++ b/elasticgraph-proto_ingestion/spec/integration/elastic_graph/proto_ingestion/schema_definition/rake_tasks_spec.rb @@ -78,7 +78,8 @@ module SchemaDefinition "fields" => { "id" => 1, "name" => 2 - } + }, + "next_number" => 3 } } }) diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb index 81d10aadb..cd2af272f 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb @@ -94,9 +94,59 @@ module SchemaDefinition )) end + it "validates that a message's `next_number` is an integer" do + expect { + FieldNumberMappings.from_artifact({ + "messages" => {"Account" => {"fields" => {"id" => 7}, "next_number" => "8"}} + }) + }.to raise_error(Errors::SchemaError, a_string_including( + "`next_number` for message `Account`", "must be an integer", '"8"' + )) + end + + it "validates that a message's `next_number` is in the allocatable range" do + [0, 19_000, FieldNumberMappings::MAX_FIELD_NUMBER + 2].each do |invalid_number| + expect { + FieldNumberMappings.from_artifact({ + "messages" => {"Account" => {"fields" => {}, "next_number" => invalid_number}} + }) + }.to raise_error(Errors::SchemaError, a_string_including( + "must be between 1 and #{FieldNumberMappings::MAX_FIELD_NUMBER + 1}", + "excluding the reserved 19000-19999 range", + invalid_number.to_s + )) + end + end + + it "validates that a message's `next_number` is greater than every mapped field number" do + expect { + FieldNumberMappings.from_artifact({ + "messages" => {"Account" => {"fields" => {"id" => 7}, "next_number" => 7}} + }) + }.to raise_error(Errors::SchemaError, a_string_including( + "must be greater than every mapped field number", "maximum: 7", "got: 7" + )) + end + + it "derives and dumps `next_number` when upgrading a mapping that does not store it" do + { + 7 => 8, + 18_999 => 20_000 + }.each do |field_number, expected_next_number| + mappings = FieldNumberMappings.from_artifact({ + "messages" => {"Account" => {"fields" => {"id" => field_number}}} + }) + + expect(mappings.to_artifact.dig("messages", "Account", "next_number")).to eq(expected_next_number) + end + end + it "accepts maximum protobuf numbers while allowing enum values in the field-reserved range" do artifact = { - "messages" => {"Account" => {"fields" => {"id" => FieldNumberMappings::MAX_FIELD_NUMBER}}}, + "messages" => {"Account" => { + "fields" => {"id" => FieldNumberMappings::MAX_FIELD_NUMBER}, + "next_number" => FieldNumberMappings::MAX_FIELD_NUMBER + 1 + }}, # Enum value numbers have no protobuf-reserved range, so 19000-19999 is fine here. "enums" => {"Status" => {"values" => { "ACTIVE" => FieldNumberMappings::MAX_ENUM_VALUE_NUMBER, @@ -162,6 +212,24 @@ module SchemaDefinition end end + describe "#field_number_for" do + it "raises a clear error when the field-number range has been exhausted" do + mappings = FieldNumberMappings.from_artifact({ + "messages" => {"Account" => { + "fields" => {"id" => FieldNumberMappings::MAX_FIELD_NUMBER}, + "next_number" => FieldNumberMappings::MAX_FIELD_NUMBER + 1 + }} + }) + + expect { + mappings.field_number_for(message_name: "Account", public_field_name: "name", previous_field_names: []) + }.to raise_error(Errors::SchemaError, a_string_including( + "Cannot allocate another protobuf field number for message `Account`", + "maximum field number (#{FieldNumberMappings::MAX_FIELD_NUMBER}) has been reached" + )) + end + end + describe "#to_artifact" do it "sorts messages and enums by name, and their fields and values by number" do mappings = FieldNumberMappings.from_artifact( @@ -180,6 +248,7 @@ module SchemaDefinition artifact = mappings.to_artifact expect(artifact.fetch("messages").keys).to eq(["AMessage", "ZMessage"]) expect(artifact.dig("messages", "ZMessage", "fields").keys).to eq(["second", "first"]) + expect(artifact.dig("messages", "ZMessage", "next_number")).to eq(3) expect(artifact.fetch("enums").keys).to eq(["AEnum", "ZEnum"]) expect(artifact.dig("enums", "ZEnum", "values").keys).to eq(["SECOND", "FIRST"]) end diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension_spec.rb index 2dc4ccaf4..d21cc108d 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension_spec.rb @@ -67,6 +67,7 @@ module SchemaDefinition Widget: fields: id: 7 + next_number: 8 enums: Status: values: @@ -87,6 +88,7 @@ module SchemaDefinition expect(results.proto_schema).to include( "string id = 7;", + ".elasticgraph.Status status = 8;", "STATUS_ACTIVE = 1;", "STATUS_INACTIVE = 5;" ) diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb index a06fd6ce4..6b87e8338 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb @@ -177,7 +177,7 @@ module SchemaDefinition # stub it to a small range to verify the allocator respects the constant. stub_const("ElasticGraph::ProtoIngestion::SchemaDefinition::FieldNumberMappings::RESERVED_FIELD_NUMBER_RANGE", 3..4) - proto = define_proto_schema do |s| + results = define_proto_schema_results do |s| s.object_type "Account" do |t| t.field "id", "ID" t.field "name", "String" @@ -186,7 +186,8 @@ module SchemaDefinition end end - expect(proto).to include("string id = 1;", "string name = 2;", "string email = 5;") + expect(results.proto_schema).to include("string id = 1;", "string name = 2;", "string email = 5;") + expect(results.proto_field_number_mappings.dig("messages", "Account", "next_number")).to eq(6) end it "allocates the next available field number when a renamed field has no old mapping entry" do diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb index aa35ead47..d7b3db962 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb @@ -242,16 +242,16 @@ module SchemaDefinition expect(proto_type_def_from(proto, "Event")).to include("int64 occurred_at = 2;") end - it "assigns the lowest unused field number to a new field, rather than the number after the maximum used one" do - # A gap below the maximum used number can only arise from a hand-edited artifact: - # organic schema evolution never creates one, since removed fields keep their numbers - # reserved. So this test must seed raw mappings instead of results from a prior dump. + it "assigns a new field the stored `next_number` rather than filling an earlier gap" do + # A cursor with gaps below it can only arise from a hand-edited artifact, so this test + # must seed raw mappings instead of results from a prior dump. results = define_proto_schema_results(proto_field_number_mappings: { "messages" => { "Account" => { "fields" => { "id" => 7 - } + }, + "next_number" => 10 } } }) do |s| @@ -262,7 +262,8 @@ module SchemaDefinition end end - expect(results.proto_schema).to include("string id = 7;", "string name = 1;") + expect(results.proto_schema).to include("string id = 7;", "string name = 10;") + expect(results.proto_field_number_mappings.dig("messages", "Account", "next_number")).to eq(11) end it "preserves proto field numbers when fields are re-ordered" do @@ -310,7 +311,8 @@ module SchemaDefinition "fields" => { "id" => 1, "name" => 2 - } + }, + "next_number" => 3 } } }) @@ -347,7 +349,8 @@ module SchemaDefinition "id" => 1, "legacy_field" => 2, "name" => 3 - } + }, + "next_number" => 4 } } }) @@ -409,7 +412,8 @@ module SchemaDefinition "fields" => { "id" => 2, "display_name" => 1 - } + }, + "next_number" => 3 } } }) @@ -508,7 +512,8 @@ module SchemaDefinition "car" => 2, "bike" => 3, "scooter" => 4 - } + }, + "next_number" => 5 }) end From a904dfc55dfb31c91d963b1df1953840d12f3e33 Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Sat, 8 Aug 2026 12:34:15 -0500 Subject: [PATCH 4/5] Address protobuf numbering review feedback --- elasticgraph-proto_ingestion/README.md | 19 +- .../field_number_mappings.rb | 362 ++++++++++-------- .../schema_definition/schema.rb | 32 +- .../schema_artifact_extension.rb | 39 -- .../schema_artifact_manager_extension.rb | 21 +- .../schema_elements/enum_type_extension.rb | 4 + .../object_interface_and_union_extension.rb | 16 +- .../schema_definition/state_extension.rb | 4 +- .../field_number_mappings.rbs | 37 +- .../schema_definition/schema.rbs | 4 + .../schema_artifact_extension.rbs | 11 - .../schema_artifact_manager_extension.rbs | 2 + .../field_number_mappings_spec.rb | 335 ++++++++-------- .../schema_artifact_manager_extension_spec.rb | 6 +- .../schema_edge_cases_spec.rb | 4 +- .../schema_definition/schema_spec.rb | 45 ++- .../schema_artifact_manager.rb | 56 ++- .../schema_artifact_manager.rbs | 21 +- .../schema_definition/schema_artifact_spec.rb | 37 +- 19 files changed, 611 insertions(+), 444 deletions(-) delete mode 100644 elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rb delete mode 100644 elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rbs diff --git a/elasticgraph-proto_ingestion/README.md b/elasticgraph-proto_ingestion/README.md index e05502211..8b77d6a2c 100644 --- a/elasticgraph-proto_ingestion/README.md +++ b/elasticgraph-proto_ingestion/README.md @@ -124,8 +124,9 @@ Additionally: `schema_artifacts:dump` automatically reads and writes `proto_field_numbers.yaml`, stored alongside your schema definition (as a sibling of the file `path_to_schema` -points to). Existing numbers stay fixed even if field order changes, and new fields -use each message's stored `next_number`, so gaps below that cursor are never filled: +points to). Existing numbers stay fixed even if field or enum value order changes. New fields, +`oneof` alternatives, and enum values use their type's stored `next_number`, so gaps below that +cursor are never filled: ```yaml messages: @@ -153,6 +154,11 @@ Alternatives inside generated interface and union `oneof` blocks use the same st message-field mappings, so adding or removing a concrete subtype does not renumber the remaining alternatives. +Removed fields and `oneof` alternatives remain in the sidecar. Their numbers are explicitly +reserved in `schema.proto`, with comments recording the prior names, while every generated +message includes a comment identifying its next field number. If a removed field or alternative +is restored under the same name, it reuses its original number and is no longer reserved. + Both `schema.proto` and the sidecar use public GraphQL field names. Index field names, including `name_in_index` overrides, are not part of the protobuf wire schema or its stable-numbering state. @@ -163,9 +169,11 @@ existing field number under the new public field name. ## Stable Enum Value Numbers Enum value numbers are pinned the same way, in an `enums` section of the sidecar. Existing -values keep their numbers when other values are added or removed, new values get the next -available numbers, and removed values keep their numbers reserved so they are never reused -(number `0` is always the generated `*_UNSPECIFIED` value): +values keep their numbers when other values are added or removed, new values claim the stored +`next_number`, and removed values keep their numbers reserved so they are never reused +(number `0` is always the generated `*_UNSPECIFIED` value). `schema.proto` explicitly reserves +each removed value number, includes a comment recording its prior name, and identifies the next +value number for each enum: ```yaml enums: @@ -173,4 +181,5 @@ enums: values: RED: 1 BLUE: 2 + next_number: 3 ``` diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb index 82fda40a9..b151162d2 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb @@ -7,6 +7,8 @@ # frozen_string_literal: true require "elastic_graph/errors" +require "elastic_graph/support/from_yaml_file" +require "elastic_graph/support/json_schema/validator_factory" module ElasticGraph module ProtoIngestion @@ -16,6 +18,8 @@ module SchemaDefinition # out the next available numbers for new fields and enum values, and serializes the updated # mappings for the next artifact dump so that numbers stay stable over time. class FieldNumberMappings + extend Support::FromYamlFile + # Stored field numbers and allocation cursor for a single protobuf message. # # @!attribute [r] field_numbers_by_name @@ -25,6 +29,15 @@ class FieldNumberMappings MessageMapping = ::Data.define(:field_numbers_by_name, :next_number) private_constant :MessageMapping + # Stored value numbers and allocation cursor for a single protobuf enum. + # + # @!attribute [r] value_numbers_by_name + # @return [Hash] + # @!attribute [r] next_number + # @return [Integer] + EnumMapping = ::Data.define(:value_numbers_by_name, :next_number) + private_constant :EnumMapping + # The largest field number protobuf allows (2^29 - 1), per # https://protobuf.dev/programming-guides/proto3/#assigning. MAX_FIELD_NUMBER = 536_870_911 @@ -35,36 +48,110 @@ class FieldNumberMappings # https://protobuf.dev/programming-guides/proto3/#enum. MAX_ENUM_VALUE_NUMBER = 2_147_483_647 - # Builds an instance from mappings in the `proto_field_numbers.yaml` artifact format, - # validating the structure and every mapped number. + # JSON schema for the `proto_field_numbers.yaml` artifact. + JSON_SCHEMA = { + "$schema" => "http://json-schema.org/draft-07/schema#", + "definitions" => { + "field_number" => { + "type" => "integer", + "minimum" => 1, + "maximum" => MAX_FIELD_NUMBER, + "not" => { + "minimum" => RESERVED_FIELD_NUMBER_RANGE.begin, + "maximum" => RESERVED_FIELD_NUMBER_RANGE.end + } + }, + "next_field_number" => { + "type" => "integer", + "minimum" => 1, + "maximum" => MAX_FIELD_NUMBER + 1, + "not" => { + "minimum" => RESERVED_FIELD_NUMBER_RANGE.begin, + "maximum" => RESERVED_FIELD_NUMBER_RANGE.end + } + }, + "enum_value_number" => { + "type" => "integer", + "minimum" => 1, + "maximum" => MAX_ENUM_VALUE_NUMBER + }, + "next_enum_value_number" => { + "type" => "integer", + "minimum" => 1, + "maximum" => MAX_ENUM_VALUE_NUMBER + 1 + } + }, + "type" => "object", + "properties" => { + "messages" => { + "type" => "object", + "additionalProperties" => { + "type" => "object", + "properties" => { + "fields" => { + "type" => "object", + "additionalProperties" => {"$ref" => "#/definitions/field_number"} + }, + "next_number" => {"$ref" => "#/definitions/next_field_number"} + }, + "required" => ["fields", "next_number"], + "additionalProperties" => false + } + }, + "enums" => { + "type" => "object", + "additionalProperties" => { + "type" => "object", + "properties" => { + "values" => { + "type" => "object", + "additionalProperties" => {"$ref" => "#/definitions/enum_value_number"} + }, + "next_number" => {"$ref" => "#/definitions/next_enum_value_number"} + }, + "required" => ["values", "next_number"], + "additionalProperties" => false + } + } + }, + "additionalProperties" => false + } + + VALIDATOR = Support::JSONSchema::Validator.new( + schema: Support::JSONSchema::ValidatorFactory.new( + schema: JSON_SCHEMA, + sanitize_pii: false + ).root_schema, + sanitize_pii: false + ) + private_constant :VALIDATOR + + # Builds an instance from parsed `proto_field_numbers.yaml`, validating its structure and + # every mapped number. # - # @param artifact [Hash, nil] parsed contents of the artifact (or a hash in the same format) + # @param parsed_yaml [Hash, nil] parsed contents of the artifact (or a hash in the same format) # @return [FieldNumberMappings] # @raise [Errors::SchemaError] if the mappings deviate from the artifact format or contain invalid numbers - def self.from_artifact(artifact) - return new(message_mappings_by_name: {}, value_numbers_by_enum: {}) if artifact.nil? - - unless artifact.is_a?(::Hash) - raise Errors::SchemaError, "Protobuf field-number mappings must be a Hash, got: #{artifact.class}." + def self.from_parsed_yaml(parsed_yaml) + parsed_yaml ||= {} # : ::Hash[::String, untyped] + if (validation_error = VALIDATOR.validate_with_error_message(parsed_yaml)) + raise Errors::SchemaError, "Invalid protobuf field-number mappings:\n\n#{validation_error}" end - verify_known_keys(artifact, ["messages", "enums"], "protobuf field-number mappings") - empty_section = {} # : ::Hash[untyped, untyped] new( - message_mappings_by_name: parse_messages(artifact.fetch("messages", empty_section)), - value_numbers_by_enum: parse_enums(artifact.fetch("enums", empty_section)) + message_mappings_by_name: parse_messages(parsed_yaml.fetch("messages", empty_section)), + enum_mappings_by_name: parse_enums(parsed_yaml.fetch("enums", empty_section)) ) end # @param message_mappings_by_name [Hash] validated message mappings - # @param value_numbers_by_enum [Hash>] validated enum value numbers + # @param enum_mappings_by_name [Hash] validated enum mappings # @api private - def initialize(message_mappings_by_name:, value_numbers_by_enum:) + def initialize(message_mappings_by_name:, enum_mappings_by_name:) @message_mappings_by_name = message_mappings_by_name - @value_numbers_by_enum = value_numbers_by_enum - @used_enum_value_numbers_by_enum = {} + @enum_mappings_by_name = enum_mappings_by_name end # Returns the stable protobuf number for a message field, assigning the message's stored @@ -76,19 +163,45 @@ def initialize(message_mappings_by_name:, value_numbers_by_enum:) # @param previous_field_names [Array] old public names of the field, if renamed # @return [Integer] def field_number_for(message_name:, public_field_name:, previous_field_names:) - message_mapping = @message_mappings_by_name[message_name] ||= MessageMapping.new( - field_numbers_by_name: {}, - next_number: 1 - ) + message_mapping = @message_mappings_by_name.fetch(message_name) do + MessageMapping.new(field_numbers_by_name: {}, next_number: 1) + end field_numbers = message_mapping.field_numbers_by_name return field_numbers.fetch(public_field_name) if field_numbers.key?(public_field_name) - if (renamed_field_number = migrate_renamed_field_number(field_numbers, previous_field_names)) - return field_numbers[public_field_name] = renamed_field_number - end + old_field_name = previous_field_names.find { |field_name| field_numbers.key?(field_name) } + updated_mapping = + if old_field_name + message_mapping.with( + field_numbers_by_name: field_numbers + .except(old_field_name) + .merge(public_field_name => field_numbers.fetch(old_field_name)) + ) + else + allocate_field_number(message_name, public_field_name, message_mapping) + end + + @message_mappings_by_name = @message_mappings_by_name.merge(message_name => updated_mapping) + updated_mapping.field_numbers_by_name.fetch(public_field_name) + end + + # Returns the next field number that will be assigned for the given message. + # + # @param message_name [String] + # @return [Integer] + def next_field_number_for(message_name) + @message_mappings_by_name[message_name]&.next_number || 1 + end - field_numbers[public_field_name] = allocate_field_number(message_name, message_mapping) + # Returns field names and numbers retained in the mappings but absent from the message. + # + # @param message_name [String] + # @param active_field_names [Array] + # @return [Hash] + def reserved_field_numbers_for(message_name, active_field_names) + field_numbers = @message_mappings_by_name[message_name]&.field_numbers_by_name || {} + reserved_numbers_by_name(field_numbers, active_field_names) end # Returns the stable protobuf numbers for an enum's values, assigning the next available @@ -98,20 +211,57 @@ def field_number_for(message_name:, public_field_name:, previous_field_names:) # @param value_names [Array] # @return [Hash] def enum_value_numbers_for(enum_name, value_names) - value_numbers = @value_numbers_by_enum[enum_name] ||= {} - used_numbers = used_enum_value_numbers_for(enum_name, value_numbers) + enum_mapping = @enum_mappings_by_name.fetch(enum_name) do + EnumMapping.new(value_numbers_by_name: {}, next_number: 1) + end + value_numbers = enum_mapping.value_numbers_by_name + new_value_names = value_names - value_numbers.keys + first_new_number = enum_mapping.next_number + last_new_number = first_new_number + new_value_names.size - 1 + + if new_value_names.any? && last_new_number > MAX_ENUM_VALUE_NUMBER + raise Errors::SchemaError, "Cannot allocate another protobuf enum value number for enum `#{enum_name}`: " \ + "the maximum enum value number (#{MAX_ENUM_VALUE_NUMBER}) has been reached." + end + + new_value_numbers = new_value_names.each_with_index.to_h do |value_name, index| + [value_name, first_new_number + index] + end + updated_value_numbers = value_numbers.merge(new_value_numbers) + updated_mapping = enum_mapping.with( + value_numbers_by_name: updated_value_numbers, + next_number: last_new_number + 1 + ) + @enum_mappings_by_name = @enum_mappings_by_name.merge(enum_name => updated_mapping) value_names.to_h do |value_name| - number = value_numbers[value_name] ||= allocate_enum_value_number(used_numbers) - [value_name, number] + [value_name, updated_value_numbers.fetch(value_name)] end end + # Returns the next value number that will be assigned for the given enum. + # + # @param enum_name [String] + # @return [Integer] + def next_enum_value_number_for(enum_name) + @enum_mappings_by_name[enum_name]&.next_number || 1 + end + + # Returns value names and numbers retained in the mappings but absent from the enum. + # + # @param enum_name [String] + # @param active_value_names [Array] + # @return [Hash] + def reserved_enum_value_numbers_for(enum_name, active_value_names) + value_numbers = @enum_mappings_by_name[enum_name]&.value_numbers_by_name || {} + reserved_numbers_by_name(value_numbers, active_value_names) + end + # Serializes the mappings back to the `proto_field_numbers.yaml` artifact format, with # messages and enums sorted by name and their fields and values sorted by number. # # @return [Hash] - def to_artifact + def to_dumpable_hash { "messages" => @message_mappings_by_name .sort_by(&:first) @@ -121,11 +271,12 @@ def to_artifact "next_number" => message_mapping.next_number }] end, - "enums" => @value_numbers_by_enum + "enums" => @enum_mappings_by_name .sort_by(&:first) - .to_h do |enum_name, value_numbers| + .to_h do |enum_name, enum_mapping| [enum_name, { - "values" => value_numbers.sort_by { |value_name, number| [number, value_name] }.to_h + "values" => enum_mapping.value_numbers_by_name.sort_by { |value_name, number| [number, value_name] }.to_h, + "next_number" => enum_mapping.next_number }] end } @@ -133,13 +284,16 @@ def to_artifact private - def used_enum_value_numbers_for(enum_name, value_numbers) - @used_enum_value_numbers_by_enum[enum_name] ||= ::Set.new(value_numbers.values) + def reserved_numbers_by_name(numbers_by_name, active_names) + numbers_by_name + .except(*active_names) + .sort_by { |name, number| [number, name] } + .to_h end - # Claims and returns the message's stored allocation cursor, advancing it past the claimed - # number and protobuf's reserved 19000..19999 range. - def allocate_field_number(message_name, message_mapping) + # Returns a message mapping with the stored allocation cursor assigned to `field_name` and + # advanced past the claimed number and protobuf's reserved 19000..19999 range. + def allocate_field_number(message_name, field_name, message_mapping) field_number = message_mapping.next_number if field_number > MAX_FIELD_NUMBER raise Errors::SchemaError, "Cannot allocate another protobuf field number for message `#{message_name}`: " \ @@ -151,133 +305,47 @@ def allocate_field_number(message_name, message_mapping) next_number = RESERVED_FIELD_NUMBER_RANGE.end + 1 end - @message_mappings_by_name[message_name] = message_mapping.with(next_number: next_number) - field_number - end - - # Claims and returns the smallest positive enum value number not yet present in - # `used_numbers`. Unlike field tags, enum value numbers have no protobuf-reserved range. - def allocate_enum_value_number(used_numbers) - candidate = 1 - candidate += 1 while used_numbers.include?(candidate) - used_numbers << candidate - candidate - end - - def migrate_renamed_field_number(field_numbers, previous_field_names) - previous_field_names.each do |old_field_name| - field_number = field_numbers.delete(old_field_name) - return field_number if field_number - end - - nil + message_mapping.with( + field_numbers_by_name: message_mapping.field_numbers_by_name.merge(field_name => field_number), + next_number: next_number + ) end private_class_method def self.parse_messages(messages_section) - unless messages_section.is_a?(::Hash) - raise Errors::SchemaError, "Protobuf field-number mappings must have a `messages` Hash." - end - messages_section.to_h do |message_name, message_entry| - unless message_entry.is_a?(::Hash) - raise Errors::SchemaError, "Field-number mapping for message `#{message_name}` must be a Hash." - end - - verify_known_keys(message_entry, ["fields", "next_number"], "field-number mapping for message `#{message_name}`") - - fields = message_entry["fields"] - unless fields.is_a?(::Hash) - raise Errors::SchemaError, "Field-number mapping for message `#{message_name}` must contain a `fields` Hash." - end - - parsed_fields = fields.to_h do |field_name, field_number| - [field_name, validated_field_number(message_name, field_name, field_number)] - end + fields = message_entry.fetch("fields") # : ::Hash[::String, ::Integer] verify_no_number_collisions( - parsed_fields, + fields, "field-number mapping collision in message `#{message_name}`" ) - next_number = - if message_entry.key?("next_number") - validated_next_number(message_name, message_entry.fetch("next_number"), parsed_fields) - else - candidate = (parsed_fields.values.max || 0) + 1 - RESERVED_FIELD_NUMBER_RANGE.cover?(candidate) ? RESERVED_FIELD_NUMBER_RANGE.end + 1 : candidate - end + next_number = message_entry.fetch("next_number") # : ::Integer - [message_name, MessageMapping.new(field_numbers_by_name: parsed_fields, next_number: next_number)] + verify_next_number_is_after_mapped_numbers("message `#{message_name}`", next_number, fields) + [message_name, MessageMapping.new(field_numbers_by_name: fields, next_number: next_number)] end end private_class_method def self.parse_enums(enums_section) - unless enums_section.is_a?(::Hash) - raise Errors::SchemaError, "Protobuf enum value-number mappings must have an `enums` Hash." - end - enums_section.to_h do |enum_name, enum_entry| - unless enum_entry.is_a?(::Hash) - raise Errors::SchemaError, "Enum value-number mapping for enum `#{enum_name}` must be a Hash." - end - - verify_known_keys(enum_entry, ["values"], "enum value-number mapping for enum `#{enum_name}`") - - values = enum_entry["values"] - unless values.is_a?(::Hash) - raise Errors::SchemaError, "Enum value-number mapping for enum `#{enum_name}` must contain a `values` Hash." - end + values = enum_entry.fetch("values") # : ::Hash[::String, ::Integer] + verify_no_number_collisions(values, "enum value-number mapping collision in enum `#{enum_name}`") - parsed_values = values.to_h do |value_name, value_number| - unless value_number.is_a?(::Integer) && value_number.between?(1, MAX_ENUM_VALUE_NUMBER) - raise Errors::SchemaError, "Enum value-number mapping for `#{enum_name}.#{value_name}` " \ - "must be a positive integer no greater than #{MAX_ENUM_VALUE_NUMBER} " \ - "(0 is reserved for the `_UNSPECIFIED` value), got: #{value_number.inspect}." - end - - [value_name, value_number] - end - - verify_no_number_collisions(parsed_values, "enum value-number mapping collision in enum `#{enum_name}`") - - [enum_name, parsed_values] + next_number = enum_entry.fetch("next_number") # : ::Integer + verify_next_number_is_after_mapped_numbers("enum `#{enum_name}`", next_number, values) + [enum_name, EnumMapping.new(value_numbers_by_name: values, next_number: next_number)] end end - private_class_method def self.validated_field_number(message_name, field_name, field_number) - unless field_number.is_a?(::Integer) - raise Errors::SchemaError, "Field-number mapping for `#{message_name}.#{field_name}` " \ - "must be an integer, got: #{field_number.inspect}." + private_class_method def self.verify_next_number_is_after_mapped_numbers(mapping_description, next_number, numbers) + max_number = numbers.values.max + if max_number && next_number <= max_number + raise Errors::SchemaError, "Protobuf `next_number` for #{mapping_description} must be greater than " \ + "every mapped number (maximum: #{max_number}), got: #{next_number}." end - unless field_number.between?(1, MAX_FIELD_NUMBER) && !RESERVED_FIELD_NUMBER_RANGE.cover?(field_number) - raise Errors::SchemaError, "Field-number mapping for `#{message_name}.#{field_name}` " \ - "must be a valid protobuf field number (1 to #{MAX_FIELD_NUMBER}, excluding the reserved " \ - "#{RESERVED_FIELD_NUMBER_RANGE.begin}-#{RESERVED_FIELD_NUMBER_RANGE.end} range), got: #{field_number.inspect}." - end - - field_number - end - - private_class_method def self.validated_next_number(message_name, next_number, field_numbers) - unless next_number.is_a?(::Integer) - raise Errors::SchemaError, "Protobuf `next_number` for message `#{message_name}` " \ - "must be an integer, got: #{next_number.inspect}." - end - - unless next_number.between?(1, MAX_FIELD_NUMBER + 1) && !RESERVED_FIELD_NUMBER_RANGE.cover?(next_number) - raise Errors::SchemaError, "Protobuf `next_number` for message `#{message_name}` must be between 1 and " \ - "#{MAX_FIELD_NUMBER + 1}, excluding the reserved #{RESERVED_FIELD_NUMBER_RANGE.begin}-" \ - "#{RESERVED_FIELD_NUMBER_RANGE.end} range, got: #{next_number.inspect}." - end - - max_field_number = field_numbers.values.max - if max_field_number && next_number <= max_field_number - raise Errors::SchemaError, "Protobuf `next_number` for message `#{message_name}` must be greater than " \ - "every mapped field number (maximum: #{max_field_number}), got: #{next_number}." - end - - next_number + nil end private_class_method def self.verify_no_number_collisions(numbers_by_name, collision_description) @@ -289,14 +357,6 @@ def migrate_renamed_field_number(field_numbers, previous_field_names) "#{names} are both mapped to number #{entries.first.last}." end end - - private_class_method def self.verify_known_keys(hash, known_keys, description) - unknown_keys = hash.keys - known_keys - return if unknown_keys.empty? - - raise Errors::SchemaError, "Unknown key(s) in #{description}: #{unknown_keys.map(&:inspect).join(", ")}. " \ - "Supported keys: #{known_keys.map { |key| "`#{key}`" }.join(", ")}." - end end end end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb index cea13e0a0..baf636fc4 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb @@ -30,7 +30,7 @@ def initialize( @state = state @all_types = all_types @package_name = package_name - @field_number_mappings = FieldNumberMappings.from_artifact(proto_field_number_mappings) + @field_number_mappings = FieldNumberMappings.from_parsed_yaml(proto_field_number_mappings) end # Renders the schema as a valid `proto3` file. @@ -55,7 +55,7 @@ def to_proto # # @return [Hash] def field_number_mappings_for_artifact - @field_number_mappings.to_artifact + @field_number_mappings.to_dumpable_hash end # Returns the stable protobuf number for a message field. @@ -69,6 +69,20 @@ def field_number_for(message_name:, type_name:, public_field_name:) ) end + # Returns the next protobuf field number that will be assigned for a message. + # + # @api private + def next_field_number_for(message_name) + @field_number_mappings.next_field_number_for(message_name) + end + + # Returns field names and numbers that must be reserved in a protobuf message. + # + # @api private + def reserved_field_numbers_for(message_name, active_field_names) + @field_number_mappings.reserved_field_numbers_for(message_name, active_field_names) + end + # Returns the stable protobuf numbers for an enum's values. # # @api private @@ -76,6 +90,20 @@ def enum_value_numbers_for(enum_name, value_names) @field_number_mappings.enum_value_numbers_for(enum_name, value_names) end + # Returns the next protobuf value number that will be assigned for an enum. + # + # @api private + def next_enum_value_number_for(enum_name) + @field_number_mappings.next_enum_value_number_for(enum_name) + end + + # Returns value names and numbers that must be reserved in a protobuf enum. + # + # @api private + def reserved_enum_value_numbers_for(enum_name, active_value_names) + @field_number_mappings.reserved_enum_value_numbers_for(enum_name, active_value_names) + end + private # Selects the indexed root types and every type transitively referenced by their protobuf diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rb deleted file mode 100644 index d265e8063..000000000 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rb +++ /dev/null @@ -1,39 +0,0 @@ -# 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 - module ProtoIngestion - module SchemaDefinition - # Extension module for {ElasticGraph::SchemaDefinition::SchemaArtifact} that replaces the standard - # "DO NOT EDIT BY HAND" preamble on `proto_field_numbers.yaml`. Unlike the proper schema artifacts, - # the file is an input to schema generation: it may be updated by hand and, after generated protos - # have been used, must not be deleted and regenerated. - # - # @private - module SchemaArtifactExtension - private - - def comment_preamble - [ - "This file is part of your schema definition--not a regenerable schema artifact. It is an", - "input to `schema.proto` generation: ElasticGraph reads it to keep protobuf field and enum", - "value numbers stable as your schema evolves, and `rake schema_artifacts:dump` maintains it", - "for you.", - "", - "You may update it by hand (e.g. to assign a specific number). While prototyping, you may", - "delete this file and regenerate it to reset the number assignments.", - "", - "Once generated protos have been used to serialize data or consumed by another codebase,", - "you must NOT delete this file and regenerate it. The original number assignments would be", - "lost, and previously serialized protobuf messages would be misread." - ].map { |line| "#{comment_prefix} #{line}".rstrip }.join("\n") - end - end - end - end -end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rb index 82c0dec82..c78c88268 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rb @@ -8,7 +8,6 @@ require "elastic_graph/errors" require "elastic_graph/proto_ingestion" -require "elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension" module ElasticGraph module ProtoIngestion @@ -18,6 +17,20 @@ module SchemaDefinition # # @private module SchemaArtifactManagerExtension + PROTO_FIELD_NUMBERS_COMMENT_PREAMBLE_LINES = [ + "This file is part of your schema definition--not a regenerable schema artifact. It is an", + "input to `schema.proto` generation: ElasticGraph reads it to keep protobuf field and enum", + "value numbers stable as your schema evolves, and `rake schema_artifacts:dump` maintains it", + "for you.", + "", + "You may update it by hand (e.g. to assign a specific number). While prototyping, you may", + "delete this file and regenerate it to reset the number assignments.", + "", + "Once generated protos have been used to serialize data or consumed by another codebase,", + "you must NOT delete this file and regenerate it. The original number assignments would be", + "lost, and previously serialized protobuf messages would be misread." + ].freeze + private # Overrides the base `artifacts_from_schema_def` method to add proto artifacts. @@ -37,8 +50,10 @@ def artifacts_from_schema_def # lives alongside `path_to_schema` instead of in the schema artifacts directory. def proto_field_numbers_artifact new_yaml_artifact(PROTO_FIELD_NUMBERS_FILE, protobuf_schema_definition_results.proto_field_number_mappings) - .with(file_name: proto_field_numbers_path) - .extend(SchemaArtifactExtension) + .with( + file_name: proto_field_numbers_path, + comment_preamble_lines: PROTO_FIELD_NUMBERS_COMMENT_PREAMBLE_LINES + ) end def proto_field_numbers_path diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rb index 8af5e3e6d..199e8eaec 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rb @@ -92,6 +92,10 @@ def render_proto_enum(schema) value = raw_value # : ::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & EnumValueExtension value.to_proto(value_numbers.fetch(value.name), proto_enum_value_prefix: proto_enum_value_prefix) end) + schema.reserved_enum_value_numbers_for(proto_name, values_by_name.keys).each do |value_name, value_number| + value_definitions << " reserved #{value_number}; // Previously used by #{value_name}." + end + value_definitions << " // Next value number: #{schema.next_enum_value_number_for(proto_name)}" <<~PROTO.chomp #{documentation}enum #{proto_name} { diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb index ef14ab5c8..30dbfc879 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb @@ -72,6 +72,7 @@ def render_proto_message(schema, message_name, package_name) return render_proto_oneof(schema, message_name, package_name) if abstract? fields = proto_fields + active_field_names = fields.map { |schema_field, _| schema_field.name } documentation = ProtoDocumentation.comment_lines_for(doc_comment).map { |line| "#{line}\n" }.join field_definitions = fields.map do |schema_field, field| repeated, field_type = proto_field_type_for( @@ -93,6 +94,10 @@ def render_proto_message(schema, message_name, package_name) "#{field_documentation}#{line}" end + schema.reserved_field_numbers_for(message_name, active_field_names).each do |field_name, field_number| + field_definitions << " reserved #{field_number}; // Previously used by #{field_name}." + end + field_definitions << " // Next field number: #{schema.next_field_number_for(message_name)}" <<~PROTO.chomp #{documentation}message #{message_name} { @@ -105,9 +110,11 @@ def render_proto_oneof(schema, message_name, package_name) # @type var abstract_type: ::ElasticGraph::SchemaDefinition::Mixins::HasSubtypes abstract_type = _ = self documentation = ProtoDocumentation.comment_lines_for(doc_comment).map { |line| "#{line}\n" }.join + active_field_names = [] # : ::Array[::String] alternatives = abstract_type.recursively_resolve_subtypes.map do |subtype| proto_subtype = _ = subtype field_name = Support::Casing.to_upper_snake(proto_subtype.proto_name).downcase + active_field_names << field_name field_number = schema.field_number_for( message_name: message_name, type_name: name, @@ -115,12 +122,15 @@ def render_proto_oneof(schema, message_name, package_name) ) " #{proto_subtype.proto_type_reference(package_name)} #{field_name} = #{field_number};" end + body_lines = [" oneof value {", *alternatives, " }"] + schema.reserved_field_numbers_for(message_name, active_field_names).each do |field_name, field_number| + body_lines << " reserved #{field_number}; // Previously used by #{field_name}." + end + body_lines << " // Next field number: #{schema.next_field_number_for(message_name)}" <<~PROTO.chomp #{documentation}message #{message_name} { - oneof value { - #{alternatives.join("\n")} - } + #{body_lines.join("\n")} } PROTO end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/state_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/state_extension.rb index 7c02d18d2..d5061e85e 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/state_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/state_extension.rb @@ -7,8 +7,8 @@ # frozen_string_literal: true require "elastic_graph/proto_ingestion" +require "elastic_graph/proto_ingestion/schema_definition/field_number_mappings" require "elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state" -require "yaml" module ElasticGraph module ProtoIngestion @@ -23,7 +23,7 @@ module StateExtension def self.extended(state) field_number_mappings = if (path = state.proto_field_numbers_path) && ::File.exist?(path) - ::YAML.safe_load_file(path, aliases: false) + FieldNumberMappings.from_yaml_file(path).to_dumpable_hash else {} # : ::Hash[::String, untyped] end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs index e062ae035..92471ced6 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs @@ -2,6 +2,8 @@ module ElasticGraph module ProtoIngestion module SchemaDefinition class FieldNumberMappings + extend Support::FromYamlFile[FieldNumberMappings] + type fieldNumbersByName = ::Hash[::String, ::Integer] class MessageMapping @@ -12,25 +14,32 @@ module ElasticGraph def with: (?field_numbers_by_name: fieldNumbersByName, ?next_number: ::Integer) -> instance end + class EnumMapping + attr_reader value_numbers_by_name: fieldNumbersByName + attr_reader next_number: ::Integer + + def self.new: (value_numbers_by_name: fieldNumbersByName, next_number: ::Integer) -> instance + def with: (?value_numbers_by_name: fieldNumbersByName, ?next_number: ::Integer) -> instance + end + MAX_FIELD_NUMBER: ::Integer RESERVED_FIELD_NUMBER_RANGE: ::Range[::Integer] MAX_ENUM_VALUE_NUMBER: ::Integer + JSON_SCHEMA: ::Hash[::String, untyped] + VALIDATOR: Support::JSONSchema::Validator @message_mappings_by_name: ::Hash[::String, MessageMapping] - @value_numbers_by_enum: ::Hash[::String, ::Hash[::String, ::Integer]] - @used_enum_value_numbers_by_enum: ::Hash[::String, ::Set[::Integer]] + @enum_mappings_by_name: ::Hash[::String, EnumMapping] - def self.from_artifact: (untyped) -> instance + def self.from_parsed_yaml: (untyped) ?{ (untyped) -> void } -> instance def self.parse_messages: (untyped) -> ::Hash[::String, MessageMapping] - def self.parse_enums: (untyped) -> ::Hash[::String, ::Hash[::String, ::Integer]] - def self.validated_field_number: (::String, ::String, untyped) -> ::Integer - def self.validated_next_number: (::String, untyped, fieldNumbersByName) -> ::Integer + def self.parse_enums: (untyped) -> ::Hash[::String, EnumMapping] + def self.verify_next_number_is_after_mapped_numbers: (::String, ::Integer, fieldNumbersByName) -> void def self.verify_no_number_collisions: (::Hash[::String, ::Integer], ::String) -> void - def self.verify_known_keys: (::Hash[untyped, untyped], ::Array[::String], ::String) -> void def initialize: ( message_mappings_by_name: ::Hash[::String, MessageMapping], - value_numbers_by_enum: ::Hash[::String, ::Hash[::String, ::Integer]] + enum_mappings_by_name: ::Hash[::String, EnumMapping] ) -> void def field_number_for: ( @@ -38,15 +47,17 @@ module ElasticGraph public_field_name: ::String, previous_field_names: ::Array[::String] ) -> ::Integer + def next_field_number_for: (::String) -> ::Integer + def reserved_field_numbers_for: (::String, ::Array[::String]) -> fieldNumbersByName def enum_value_numbers_for: (::String, ::Array[::String]) -> ::Hash[::String, ::Integer] - def to_artifact: () -> ::Hash[::String, untyped] + def next_enum_value_number_for: (::String) -> ::Integer + def reserved_enum_value_numbers_for: (::String, ::Array[::String]) -> ::Hash[::String, ::Integer] + def to_dumpable_hash: () -> ::Hash[::String, untyped] private - def used_enum_value_numbers_for: (::String, ::Hash[::String, ::Integer]) -> ::Set[::Integer] - def allocate_field_number: (::String, MessageMapping) -> ::Integer - def allocate_enum_value_number: (::Set[::Integer]) -> ::Integer - def migrate_renamed_field_number: (fieldNumbersByName, ::Array[::String]) -> ::Integer? + def reserved_numbers_by_name: (fieldNumbersByName, ::Array[::String]) -> fieldNumbersByName + def allocate_field_number: (::String, ::String, MessageMapping) -> MessageMapping end end end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs index 7ada008d7..9c8226f97 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs @@ -22,7 +22,11 @@ module ElasticGraph type_name: ::String, public_field_name: ::String ) -> ::Integer + def next_field_number_for: (::String) -> ::Integer + def reserved_field_numbers_for: (::String, ::Array[::String]) -> ::Hash[::String, ::Integer] def enum_value_numbers_for: (::String, ::Array[::String]) -> ::Hash[::String, ::Integer] + def next_enum_value_number_for: (::String) -> ::Integer + def reserved_enum_value_numbers_for: (::String, ::Array[::String]) -> ::Hash[::String, ::Integer] private diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rbs deleted file mode 100644 index 3eaecde79..000000000 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_extension.rbs +++ /dev/null @@ -1,11 +0,0 @@ -module ElasticGraph - module ProtoIngestion - module SchemaDefinition - module SchemaArtifactExtension : ::ElasticGraph::SchemaDefinition::SchemaArtifact[untyped] - private - - def comment_preamble: () -> ::String - end - end - end -end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rbs index 6b13106d8..120aa8962 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rbs @@ -2,6 +2,8 @@ module ElasticGraph module ProtoIngestion module SchemaDefinition module SchemaArtifactManagerExtension : ::ElasticGraph::SchemaDefinition::SchemaArtifactManager + PROTO_FIELD_NUMBERS_COMMENT_PREAMBLE_LINES: ::Array[::String] + private def artifacts_from_schema_def: () -> ::Array[::ElasticGraph::SchemaDefinition::SchemaArtifact[untyped]] diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb index cd2af272f..6c0a8d94d 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb @@ -7,250 +7,239 @@ # frozen_string_literal: true require "elastic_graph/proto_ingestion/schema_definition/field_number_mappings" +require "elastic_graph/support/json_schema/meta_schema_validator" module ElasticGraph module ProtoIngestion module SchemaDefinition RSpec.describe FieldNumberMappings do - describe ".from_artifact" do + describe ".from_parsed_yaml" do it "returns empty mappings for `nil`, as parsing an empty artifact file yields" do - mappings = FieldNumberMappings.from_artifact(nil) - - expect(mappings.to_artifact).to eq({"enums" => {}, "messages" => {}}) - end - - it "validates that the artifact is a hash" do - expect { - FieldNumberMappings.from_artifact("bad") - }.to raise_error(Errors::SchemaError, a_string_including("must be a Hash")) - end - - it "rejects unknown keys at every level so hand-edit typos do not silently discard mappings" do - [ - [{"messagez" => {}}, "\"messagez\""], - [{messages: {}}, ":messages"], - [{"messages" => {"Account" => {"fieldz" => {}}}}, "\"fieldz\""], - [{"enums" => {"Status" => {"valuez" => {}}}}, "\"valuez\""] - ].each do |artifact, unknown_key| - expect { - FieldNumberMappings.from_artifact(artifact) - }.to raise_error(Errors::SchemaError, a_string_including("Unknown key(s)", unknown_key)) + mappings = FieldNumberMappings.from_parsed_yaml(nil) + + expect(mappings.to_dumpable_hash).to eq({"enums" => {}, "messages" => {}}) + end + + describe "artifact structure validation" do + it "uses a valid JSON schema to reject malformed artifacts" do + expect(Support::JSONSchema.strict_meta_schema_validator.valid?(FieldNumberMappings::JSON_SCHEMA)).to be(true) + + invalid_artifacts = [ + "bad", + {"messagez" => {}}, + {"messages" => {"Account" => {"fields" => {"id" => 7}}}}, + {"enums" => {"Status" => {"values" => {"ACTIVE" => 1}}}}, + {"messages" => {"Account" => {"fields" => {"id" => "7"}}}}, + {"messages" => {"Account" => {"fields" => {"id" => 19_000}}}}, + {"messages" => {"Account" => {"fields" => {}, "next_number" => 19_000}}}, + {"enums" => {"Status" => {"values" => {"ACTIVE" => 0}}}}, + {"enums" => {"Status" => {"values" => {}, "next_number" => 0}}} + ] + + invalid_artifacts.each do |artifact| + expect { + FieldNumberMappings.from_parsed_yaml(artifact) + }.to raise_error(Errors::SchemaError, a_string_including( + "Invalid protobuf field-number mappings", "Validation errors" + )) + end end end - it "validates that `messages` is a hash" do - expect { - FieldNumberMappings.from_artifact({"messages" => "bad"}) - }.to raise_error(Errors::SchemaError, a_string_including("must have a `messages` Hash")) - end - - it "validates that each message's mapping is a hash" do - expect { - FieldNumberMappings.from_artifact({"messages" => {"Account" => "bad"}}) - }.to raise_error(Errors::SchemaError, a_string_including("mapping for message `Account` must be a Hash")) - end - - it "validates that each message's nested `fields` is a hash" do - expect { - FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => "bad"}}}) - }.to raise_error(Errors::SchemaError, a_string_including("must contain a `fields` Hash")) - end - - it "validates that mapped field numbers are valid protobuf field tags" do - [0, FieldNumberMappings::MAX_FIELD_NUMBER + 1, 19_000, 19_999].each do |invalid_number| + describe "mapping consistency validation" do + it "raises clear errors when fields or enum values collide" do expect { - FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => {"id" => invalid_number}}}}) + FieldNumberMappings.from_parsed_yaml({ + "messages" => {"Account" => {"fields" => {"id" => 1, "name" => 1}, "next_number" => 2}} + }) }.to raise_error(Errors::SchemaError, a_string_including( - "must be a valid protobuf field number", "excluding the reserved 19000-19999 range", invalid_number.to_s + "field-number mapping collision in message `Account`", "`id` and `name`", "number 1" )) - end - end - it "rejects field numbers given as strings rather than coercing them" do - ["abc", "7"].each do |non_integer| expect { - FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => {"id" => non_integer}}}}) - }.to raise_error(Errors::SchemaError, a_string_including("must be an integer", non_integer.inspect)) + FieldNumberMappings.from_parsed_yaml({ + "enums" => {"Status" => {"values" => {"ACTIVE" => 1, "INACTIVE" => 1}, "next_number" => 2}} + }) + }.to raise_error(Errors::SchemaError, a_string_including( + "enum value-number mapping collision in enum `Status`", "`ACTIVE` and `INACTIVE`", "number 1" + )) end - end - it "rejects field numbers that are not integers rather than coercing or silently truncating them" do - [1.5, {"field_number" => 7}].each do |non_integer| + it "validates that each `next_number` is greater than every mapped number" do expect { - FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => {"id" => non_integer}}}}) + FieldNumberMappings.from_parsed_yaml({ + "messages" => {"Account" => {"fields" => {"id" => 7}, "next_number" => 7}} + }) }.to raise_error(Errors::SchemaError, a_string_including( - "`Account.id`", "must be an integer", non_integer.inspect + "`next_number` for message `Account`", "greater than every mapped number", "maximum: 7", "got: 7" )) - end - end - - it "raises a clear error when two fields of a message are mapped to the same number" do - expect { - FieldNumberMappings.from_artifact({"messages" => {"Account" => {"fields" => {"id" => 1, "name" => 1}}}}) - }.to raise_error(Errors::SchemaError, a_string_including( - "field-number mapping collision in message `Account`", - "`id` and `name`", - "number 1" - )) - end - it "validates that a message's `next_number` is an integer" do - expect { - FieldNumberMappings.from_artifact({ - "messages" => {"Account" => {"fields" => {"id" => 7}, "next_number" => "8"}} - }) - }.to raise_error(Errors::SchemaError, a_string_including( - "`next_number` for message `Account`", "must be an integer", '"8"' - )) - end - - it "validates that a message's `next_number` is in the allocatable range" do - [0, 19_000, FieldNumberMappings::MAX_FIELD_NUMBER + 2].each do |invalid_number| expect { - FieldNumberMappings.from_artifact({ - "messages" => {"Account" => {"fields" => {}, "next_number" => invalid_number}} + FieldNumberMappings.from_parsed_yaml({ + "enums" => {"Status" => {"values" => {"ACTIVE" => 7}, "next_number" => 7}} }) }.to raise_error(Errors::SchemaError, a_string_including( - "must be between 1 and #{FieldNumberMappings::MAX_FIELD_NUMBER + 1}", - "excluding the reserved 19000-19999 range", - invalid_number.to_s + "`next_number` for enum `Status`", "greater than every mapped number", "maximum: 7", "got: 7" )) end end - it "validates that a message's `next_number` is greater than every mapped field number" do - expect { - FieldNumberMappings.from_artifact({ - "messages" => {"Account" => {"fields" => {"id" => 7}, "next_number" => 7}} - }) - }.to raise_error(Errors::SchemaError, a_string_including( - "must be greater than every mapped field number", "maximum: 7", "got: 7" - )) - end - - it "derives and dumps `next_number` when upgrading a mapping that does not store it" do - { - 7 => 8, - 18_999 => 20_000 - }.each do |field_number, expected_next_number| - mappings = FieldNumberMappings.from_artifact({ - "messages" => {"Account" => {"fields" => {"id" => field_number}}} - }) + describe "protobuf number boundaries" do + it "accepts maximum numbers while allowing enum values in the field-reserved range" do + artifact = { + "messages" => {"Account" => { + "fields" => {"id" => FieldNumberMappings::MAX_FIELD_NUMBER}, + "next_number" => FieldNumberMappings::MAX_FIELD_NUMBER + 1 + }}, + # Enum value numbers have no protobuf-reserved range, so 19000-19999 is fine here. + "enums" => {"Status" => {"values" => { + "ACTIVE" => FieldNumberMappings::MAX_ENUM_VALUE_NUMBER, + "INACTIVE" => 19_005 + }, "next_number" => FieldNumberMappings::MAX_ENUM_VALUE_NUMBER + 1}} + } - expect(mappings.to_artifact.dig("messages", "Account", "next_number")).to eq(expected_next_number) + expect(FieldNumberMappings.from_parsed_yaml(artifact).to_dumpable_hash).to eq(artifact) end end + end - it "accepts maximum protobuf numbers while allowing enum values in the field-reserved range" do - artifact = { + describe ".from_yaml_file" do + it "loads mappings through `FromYamlFile`", :in_temp_dir do + ::File.write("proto_field_numbers.yaml", <<~YAML) + messages: + Account: + fields: + id: 7 + next_number: 8 + enums: + Status: + values: + ACTIVE: 3 + next_number: 8 + YAML + + mappings = FieldNumberMappings.from_yaml_file("proto_field_numbers.yaml") + + expect(mappings.to_dumpable_hash).to eq({ + "messages" => {"Account" => {"fields" => {"id" => 7}, "next_number" => 8}}, + "enums" => {"Status" => {"values" => {"ACTIVE" => 3}, "next_number" => 8}} + }) + end + end + + describe "#field_number_for" do + it "raises a clear error when the field-number range has been exhausted" do + mappings = FieldNumberMappings.from_parsed_yaml({ "messages" => {"Account" => { "fields" => {"id" => FieldNumberMappings::MAX_FIELD_NUMBER}, "next_number" => FieldNumberMappings::MAX_FIELD_NUMBER + 1 - }}, - # Enum value numbers have no protobuf-reserved range, so 19000-19999 is fine here. - "enums" => {"Status" => {"values" => { - "ACTIVE" => FieldNumberMappings::MAX_ENUM_VALUE_NUMBER, - "INACTIVE" => 19_005 - }}} - } - - expect(FieldNumberMappings.from_artifact(artifact).to_artifact).to eq(artifact) - end - - it "validates that `enums` is a hash" do - expect { - FieldNumberMappings.from_artifact({"enums" => "bad"}) - }.to raise_error(Errors::SchemaError, a_string_including("must have an `enums` Hash")) - end + }} + }) - it "validates that each enum's mapping is a hash" do expect { - FieldNumberMappings.from_artifact({"enums" => {"Status" => "bad"}}) - }.to raise_error(Errors::SchemaError, a_string_including("mapping for enum `Status` must be a Hash")) + mappings.field_number_for(message_name: "Account", public_field_name: "name", previous_field_names: []) + }.to raise_error(Errors::SchemaError, a_string_including( + "Cannot allocate another protobuf field number for message `Account`", + "maximum field number (#{FieldNumberMappings::MAX_FIELD_NUMBER}) has been reached" + )) end + end - it "validates that each enum's nested `values` is a hash" do - expect { - FieldNumberMappings.from_artifact({"enums" => {"Status" => {"values" => "bad"}}}) - }.to raise_error(Errors::SchemaError, a_string_including("must contain a `values` Hash")) - end + describe "allocation cursor readers" do + it "returns stored cursors, defaulting to 1 for unmapped messages and enums" do + mappings = FieldNumberMappings.from_parsed_yaml({ + "messages" => {"Account" => {"fields" => {"id" => 7}, "next_number" => 10}}, + "enums" => {"Status" => {"values" => {"ACTIVE" => 3}, "next_number" => 8}} + }) - it "validates that mapped enum value numbers are positive integers, since 0 is reserved" do - expect { - FieldNumberMappings.from_artifact({"enums" => {"Status" => {"values" => {"ACTIVE" => 0}}}}) - }.to raise_error(Errors::SchemaError, a_string_including("must be a positive integer", "_UNSPECIFIED")) + expect(mappings.next_field_number_for("Account")).to eq(10) + expect(mappings.next_field_number_for("UnmappedMessage")).to eq(1) + expect(mappings.next_enum_value_number_for("Status")).to eq(8) + expect(mappings.next_enum_value_number_for("UnmappedEnum")).to eq(1) end + end - it "validates that mapped enum value numbers do not exceed the int32 maximum" do - expect { - FieldNumberMappings.from_artifact( - {"enums" => {"Status" => {"values" => {"ACTIVE" => FieldNumberMappings::MAX_ENUM_VALUE_NUMBER + 1}}}} - ) - }.to raise_error(Errors::SchemaError, a_string_including("must be a positive integer no greater than 2147483647")) - end + describe "#enum_value_numbers_for" do + it "allocates from the saved cursor without filling gaps" do + mappings = FieldNumberMappings.from_parsed_yaml({ + "enums" => {"Status" => {"values" => {"ACTIVE" => 3}, "next_number" => 10}} + }) - it "rejects enum value numbers given as strings rather than coercing them" do - expect { - FieldNumberMappings.from_artifact({"enums" => {"Status" => {"values" => {"ACTIVE" => "5"}}}}) - }.to raise_error(Errors::SchemaError, a_string_including("must be a positive integer", "\"5\"")) + expect(mappings.enum_value_numbers_for("Status", ["ARCHIVED", "ACTIVE", "DELETED"])).to eq({ + "ARCHIVED" => 10, + "ACTIVE" => 3, + "DELETED" => 11 + }) + expect(mappings.to_dumpable_hash.dig("enums", "Status", "values")).to eq({ + "ACTIVE" => 3, + "ARCHIVED" => 10, + "DELETED" => 11 + }) + expect(mappings.next_enum_value_number_for("Status")).to eq(12) end - it "rejects enum value numbers that are not integers rather than silently truncating them" do - expect { - FieldNumberMappings.from_artifact({"enums" => {"Status" => {"values" => {"ACTIVE" => 1.5}}}}) - }.to raise_error(Errors::SchemaError, a_string_including("must be a positive integer", "1.5")) - end + it "raises a clear error when the enum value-number range has been exhausted" do + mappings = FieldNumberMappings.from_parsed_yaml({ + "enums" => {"Status" => { + "values" => {"ACTIVE" => FieldNumberMappings::MAX_ENUM_VALUE_NUMBER}, + "next_number" => FieldNumberMappings::MAX_ENUM_VALUE_NUMBER + 1 + }} + }) - it "raises a clear error when two values of an enum are mapped to the same number" do expect { - FieldNumberMappings.from_artifact({"enums" => {"Status" => {"values" => {"ACTIVE" => 1, "INACTIVE" => 1}}}}) + mappings.enum_value_numbers_for("Status", ["ACTIVE", "INACTIVE"]) }.to raise_error(Errors::SchemaError, a_string_including( - "enum value-number mapping collision in enum `Status`", - "`ACTIVE` and `INACTIVE`", - "number 1" + "Cannot allocate another protobuf enum value number for enum `Status`", + "maximum enum value number (#{FieldNumberMappings::MAX_ENUM_VALUE_NUMBER}) has been reached" )) end end - describe "#field_number_for" do - it "raises a clear error when the field-number range has been exhausted" do - mappings = FieldNumberMappings.from_artifact({ + describe "reserved number readers" do + it "returns mapped names that are not active, ordered by number" do + mappings = FieldNumberMappings.from_parsed_yaml({ "messages" => {"Account" => { - "fields" => {"id" => FieldNumberMappings::MAX_FIELD_NUMBER}, - "next_number" => FieldNumberMappings::MAX_FIELD_NUMBER + 1 - }} + "fields" => {"name" => 3, "legacy_id" => 1, "id" => 2}, + "next_number" => 4 + }}, + "enums" => {"Status" => {"values" => {"PAUSED" => 2, "ACTIVE" => 1}, "next_number" => 3}} }) - expect { - mappings.field_number_for(message_name: "Account", public_field_name: "name", previous_field_names: []) - }.to raise_error(Errors::SchemaError, a_string_including( - "Cannot allocate another protobuf field number for message `Account`", - "maximum field number (#{FieldNumberMappings::MAX_FIELD_NUMBER}) has been reached" - )) + reserved_field_numbers = mappings.reserved_field_numbers_for("Account", ["id"]) + expect(reserved_field_numbers).to eq({ + "legacy_id" => 1, + "name" => 3 + }) + expect(reserved_field_numbers.keys).to eq(["legacy_id", "name"]) + expect(mappings.reserved_enum_value_numbers_for("Status", ["ACTIVE"])).to eq({"PAUSED" => 2}) + expect(mappings.reserved_field_numbers_for("MissingMessage", [])).to eq({}) + expect(mappings.reserved_enum_value_numbers_for("MissingEnum", [])).to eq({}) end end - describe "#to_artifact" do + describe "#to_dumpable_hash" do it "sorts messages and enums by name, and their fields and values by number" do - mappings = FieldNumberMappings.from_artifact( + mappings = FieldNumberMappings.from_parsed_yaml( { "messages" => { - "ZMessage" => {"fields" => {"first" => 2, "second" => 1}}, - "AMessage" => {"fields" => {"only" => 3}} + "ZMessage" => {"fields" => {"first" => 2, "second" => 1}, "next_number" => 3}, + "AMessage" => {"fields" => {"only" => 3}, "next_number" => 4} }, "enums" => { - "ZEnum" => {"values" => {"FIRST" => 2, "SECOND" => 1}}, - "AEnum" => {"values" => {"ONLY" => 3}} + "ZEnum" => {"values" => {"FIRST" => 2, "SECOND" => 1}, "next_number" => 3}, + "AEnum" => {"values" => {"ONLY" => 3}, "next_number" => 4} } } ) - artifact = mappings.to_artifact + artifact = mappings.to_dumpable_hash expect(artifact.fetch("messages").keys).to eq(["AMessage", "ZMessage"]) expect(artifact.dig("messages", "ZMessage", "fields").keys).to eq(["second", "first"]) expect(artifact.dig("messages", "ZMessage", "next_number")).to eq(3) expect(artifact.fetch("enums").keys).to eq(["AEnum", "ZEnum"]) expect(artifact.dig("enums", "ZEnum", "values").keys).to eq(["SECOND", "FIRST"]) + expect(artifact.dig("enums", "ZEnum", "next_number")).to eq(3) + expect(artifact.dig("enums", "AEnum", "next_number")).to eq(4) end end end diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension_spec.rb index d21cc108d..9b07dbeb4 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension_spec.rb @@ -72,6 +72,7 @@ module SchemaDefinition Status: values: INACTIVE: 5 + next_number: 8 YAML results = define_proto_schema_results(path_to_schema: ::File.join("config", "schema.rb")) do |s| @@ -89,8 +90,9 @@ module SchemaDefinition expect(results.proto_schema).to include( "string id = 7;", ".elasticgraph.Status status = 8;", - "STATUS_ACTIVE = 1;", - "STATUS_INACTIVE = 5;" + "STATUS_ACTIVE = 8;", + "STATUS_INACTIVE = 5;", + "// Next value number: 9" ) end diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb index 6b87e8338..63d4d6694 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb @@ -169,7 +169,9 @@ module SchemaDefinition expect { results.proto_schema - }.to raise_error(Errors::SchemaError, a_string_including("must be a valid protobuf field number")) + }.to raise_error(Errors::SchemaError, a_string_including( + "Invalid protobuf field-number mappings", "/messages/Account/fields/id", "less than: 1" + )) end it "skips the protobuf-reserved range when allocating new field numbers" do diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb index d7b3db962..51f98dbc1 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb @@ -52,11 +52,13 @@ module SchemaDefinition .elasticgraph.Status status = 2; .elasticgraph.Address address = 3; repeated string tags = 4; + // Next field number: 5 } message Address { string street = 1; string city = 2; + // Next field number: 3 } // The status of an account. @@ -69,6 +71,7 @@ module SchemaDefinition // The account is active. STATUS_ACTIVE = 1; STATUS_INACTIVE = 2; + // Next value number: 3 } PROTO end @@ -140,6 +143,7 @@ module SchemaDefinition .elasticgraph.Car car = 1; .elasticgraph.Bike bike = 2; } + // Next field number: 3 } PROTO expect(proto_type_def_from(proto, "Inventor")).to eq(<<~PROTO.strip) @@ -148,6 +152,7 @@ module SchemaDefinition .elasticgraph.Person person = 1; .elasticgraph.Company company = 2; } + // Next field number: 3 } PROTO expect(proto_type_def_from(proto, "Car")).to include("string id = 1;", "int32 doors = 2;") @@ -181,6 +186,7 @@ module SchemaDefinition oneof value { .elasticgraph.Car car = 1; } + // Next field number: 2 } PROTO expect(proto_type_def_from(proto, "Car")).to include("string id = 1;") @@ -205,6 +211,7 @@ module SchemaDefinition oneof value { .elasticgraph.DeliveryVehicle delivery_vehicle = 1; } + // Next field number: 2 } PROTO end @@ -263,6 +270,7 @@ module SchemaDefinition end expect(results.proto_schema).to include("string id = 7;", "string name = 10;") + expect(proto_type_def_from(results.proto_schema, "Account")).to include("// Next field number: 11") expect(results.proto_field_number_mappings.dig("messages", "Account", "next_number")).to eq(11) end @@ -318,7 +326,7 @@ module SchemaDefinition }) end - it "keeps a removed field's number reserved instead of reusing it for a new field" do + it "keeps a removed field's number reserved and restores it if the field is re-added" do results1 = define_proto_schema_results do |s| s.object_type "Account" do |t| t.field "id", "ID" @@ -339,6 +347,7 @@ module SchemaDefinition end expect(results2.proto_schema).to include("string id = 1;", "string name = 3;") + expect(results2.proto_schema).to include("reserved 2; // Previously used by legacy_field.") # `legacy_field` keeps its number reserved in the artifact so it is never reused. expect(results2.proto_field_number_mappings).to eq({ @@ -354,6 +363,22 @@ module SchemaDefinition } } }) + + # `legacy_field` is restored after the intermediate artifact has been dumped. + results3 = define_proto_schema_results(results2) do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "legacy_field", "String" + t.field "name", "String" + t.index "accounts" + end + end + + expect(results3.proto_schema).to include( + "string id = 1;", "string legacy_field = 2;", "string name = 3;", "// Next field number: 4" + ) + expect(results3.proto_schema).not_to include("reserved 2;") + expect(results3.proto_field_number_mappings).to eq(results2.proto_field_number_mappings) end it "keeps index field names out of the protobuf schema and field-number mappings" do @@ -405,6 +430,7 @@ module SchemaDefinition end expect(results2.proto_schema).to include("string id = 2;", "string display_name = 1;") + expect(results2.proto_schema).not_to include("reserved 1;") expect(results2.proto_field_number_mappings).to eq({ "enums" => {}, "messages" => { @@ -435,7 +461,8 @@ module SchemaDefinition expect(proto_type_def_from(results1.proto_schema, "Status")).to include( "STATUS_ACTIVE = 1;", "STATUS_PAUSED = 2;", - "STATUS_INACTIVE = 3;" + "STATUS_INACTIVE = 3;", + "// Next value number: 4" ) # `PAUSED` has been removed and `ARCHIVED` added since the mappings were dumped. @@ -454,7 +481,9 @@ module SchemaDefinition expect(proto_type_def_from(results2.proto_schema, "Status")).to include( "STATUS_ACTIVE = 1;", "STATUS_INACTIVE = 3;", - "STATUS_ARCHIVED = 4;" + "STATUS_ARCHIVED = 4;", + "reserved 2; // Previously used by PAUSED.", + "// Next value number: 5" ) # `PAUSED` keeps its number reserved in the artifact so it is never reused for a new value. @@ -465,7 +494,8 @@ module SchemaDefinition "PAUSED" => 2, "INACTIVE" => 3, "ARCHIVED" => 4 - } + }, + "next_number" => 5 } }) end @@ -503,7 +533,12 @@ module SchemaDefinition end vehicle = proto_type_def_from(results2.proto_schema, "Vehicle") - expect(vehicle).to include("Car car = 2;", "Bike bike = 3;", "Scooter scooter = 4;") + expect(vehicle).to include( + "Car car = 2;", + "Bike bike = 3;", + "Scooter scooter = 4;", + "reserved 1; // Previously used by truck." + ) # `truck` keeps its number reserved in the artifact so it is never reused. expect(results2.proto_field_number_mappings.fetch("messages").fetch("Vehicle")).to eq({ diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_artifact_manager.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_artifact_manager.rb index 10c29023b..9e78ff6b2 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_artifact_manager.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_artifact_manager.rb @@ -179,23 +179,25 @@ def truncate_diff(diff, lines) def new_yaml_artifact(file_name, desired_contents, extra_comment_lines: []) SchemaArtifact.new( - ::File.join(@schema_artifacts_directory, file_name), - desired_contents, - ->(hash) { ::YAML.dump(hash) }, - ->(string) { ::YAML.safe_load(string) }, - extra_comment_lines, - "#" + file_name: ::File.join(@schema_artifacts_directory, file_name), + desired_contents: desired_contents, + dumper: ->(hash) { ::YAML.dump(hash) }, + loader: ->(string) { ::YAML.safe_load(string) }, + extra_comment_lines: extra_comment_lines, + comment_prefix: "#", + comment_preamble_lines: SchemaArtifact::COMMENT_PREAMBLE_LINES ) end def new_raw_artifact(file_name, desired_contents, comment_prefix: "#") SchemaArtifact.new( - ::File.join(@schema_artifacts_directory, file_name), - desired_contents, - _ = :itself.to_proc, - _ = :itself.to_proc, - [], - comment_prefix + file_name: ::File.join(@schema_artifacts_directory, file_name), + desired_contents: desired_contents, + dumper: _ = :itself.to_proc, + loader: _ = :itself.to_proc, + extra_comment_lines: [], + comment_prefix: comment_prefix, + comment_preamble_lines: SchemaArtifact::COMMENT_PREAMBLE_LINES ) end @@ -217,7 +219,20 @@ def pruned_runtime_metadata(graphql_schema_string) end # @private - class SchemaArtifact < Support::MemoizableData.define(:file_name, :desired_contents, :dumper, :loader, :extra_comment_lines, :comment_prefix) + class SchemaArtifact < Support::MemoizableData.define( + :file_name, + :desired_contents, + :dumper, + :loader, + :extra_comment_lines, + :comment_prefix, + :comment_preamble_lines + ) + COMMENT_PREAMBLE_LINES = [ + "Generated by `bundle exec rake schema_artifacts:dump`.", + "DO NOT EDIT BY HAND. Any edits will be lost the next time the rake task is run." + ].freeze + def dump(output) if out_of_date? dirname = File.dirname(file_name) @@ -237,8 +252,7 @@ def out_of_date? def existing_dumped_contents return nil unless exists? - # We drop the first 2 lines because it is the comment block containing dynamic elements. - file_contents = ::File.read(file_name).split("\n").drop(2).join("\n") + file_contents = ::File.read(file_name).split("\n").drop(comment_preamble_lines_with_extras.size).join("\n") loader.call(file_contents) end @@ -267,13 +281,13 @@ def dumped_contents end def comment_preamble - lines = [ - "Generated by `bundle exec rake schema_artifacts:dump`.", - "DO NOT EDIT BY HAND. Any edits will be lost the next time the rake task is run." - ] + comment_preamble_lines_with_extras.map { |line| "#{comment_prefix} #{line}".rstrip }.join("\n") + end + + def comment_preamble_lines_with_extras + return comment_preamble_lines if extra_comment_lines.empty? - lines = extra_comment_lines + [""] + lines unless extra_comment_lines.empty? - lines.map { |line| "#{comment_prefix} #{line}".rstrip }.join("\n") + extra_comment_lines + [""] + comment_preamble_lines end end end diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_artifact_manager.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_artifact_manager.rbs index 1a496e41a..e01d9b3ed 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_artifact_manager.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_artifact_manager.rbs @@ -49,14 +49,17 @@ module ElasticGraph attr_reader loader: ^(::String) -> T attr_reader extra_comment_lines: ::Array[::String] attr_reader comment_prefix: ::String + attr_reader comment_preamble_lines: ::Array[::String] def initialize: ( - ::String, - T, - ^(T) -> ::String, - ^(::String) -> T, - ::Array[::String], - ::String) -> void + file_name: ::String, + desired_contents: T, + dumper: ^(T) -> ::String, + loader: ^(::String) -> T, + extra_comment_lines: ::Array[::String], + comment_prefix: ::String, + comment_preamble_lines: ::Array[::String] + ) -> void def with: ( ?file_name: ::String, @@ -64,11 +67,14 @@ module ElasticGraph ?dumper: ^(T) -> ::String, ?loader: ^(::String) -> T, ?extra_comment_lines: ::Array[::String], - ?comment_prefix: ::String + ?comment_prefix: ::String, + ?comment_preamble_lines: ::Array[::String] ) -> self end class SchemaArtifact[T] < SchemaArtifactSupertype[T] + COMMENT_PREAMBLE_LINES: ::Array[::String] + def dump: (io) -> void def out_of_date?: () -> bool def existing_dumped_contents: () -> T? @@ -82,6 +88,7 @@ module ElasticGraph @dumped_contents: ::String? def dumped_contents: () -> ::String def comment_preamble: () -> ::String + def comment_preamble_lines_with_extras: () -> ::Array[::String] end end end diff --git a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/schema_artifact_spec.rb b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/schema_artifact_spec.rb index f071d0600..7defad81f 100644 --- a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/schema_artifact_spec.rb +++ b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/schema_artifact_spec.rb @@ -14,12 +14,13 @@ module SchemaDefinition RSpec.describe SchemaArtifact, :in_temp_dir do it "renders the comment preamble using the configured comment prefix so artifacts can use their format's comment syntax" do artifact = SchemaArtifact.new( - "widgets.proto", - "message Widget {}", - :itself.to_proc, - :itself.to_proc, - [], - "//" + file_name: "widgets.proto", + desired_contents: "message Widget {}", + dumper: :itself.to_proc, + loader: :itself.to_proc, + extra_comment_lines: [], + comment_prefix: "//", + comment_preamble_lines: SchemaArtifact::COMMENT_PREAMBLE_LINES ) artifact.dump(::StringIO.new) @@ -30,6 +31,30 @@ module SchemaDefinition message Widget {} EOS end + + it "renders configured comment preamble lines" do + artifact = SchemaArtifact.new( + file_name: "widgets.proto", + desired_contents: "message Widget {}", + dumper: :itself.to_proc, + loader: :itself.to_proc, + extra_comment_lines: ["Extra context."], + comment_prefix: "//", + comment_preamble_lines: ["Custom preamble."] + ) + + artifact.dump(::StringIO.new) + + expect(::File.read("widgets.proto")).to eq(<<~EOS.strip) + // Extra context. + // + // Custom preamble. + message Widget {} + EOS + + identical_artifact = artifact.with + expect(identical_artifact).not_to be_out_of_date + end end end end From 11fc5c1f1c72750aaea32f422bc112b6fa21a5a9 Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Sat, 8 Aug 2026 12:57:46 -0500 Subject: [PATCH 5/5] Simplify JSON schema validation coverage --- .../field_number_mappings_spec.rb | 28 ++++++------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb index 6c0a8d94d..318159090 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb @@ -21,28 +21,16 @@ module SchemaDefinition end describe "artifact structure validation" do - it "uses a valid JSON schema to reject malformed artifacts" do + it "validates mappings against a valid JSON schema" do expect(Support::JSONSchema.strict_meta_schema_validator.valid?(FieldNumberMappings::JSON_SCHEMA)).to be(true) - invalid_artifacts = [ - "bad", - {"messagez" => {}}, - {"messages" => {"Account" => {"fields" => {"id" => 7}}}}, - {"enums" => {"Status" => {"values" => {"ACTIVE" => 1}}}}, - {"messages" => {"Account" => {"fields" => {"id" => "7"}}}}, - {"messages" => {"Account" => {"fields" => {"id" => 19_000}}}}, - {"messages" => {"Account" => {"fields" => {}, "next_number" => 19_000}}}, - {"enums" => {"Status" => {"values" => {"ACTIVE" => 0}}}}, - {"enums" => {"Status" => {"values" => {}, "next_number" => 0}}} - ] - - invalid_artifacts.each do |artifact| - expect { - FieldNumberMappings.from_parsed_yaml(artifact) - }.to raise_error(Errors::SchemaError, a_string_including( - "Invalid protobuf field-number mappings", "Validation errors" - )) - end + expect { + FieldNumberMappings.from_parsed_yaml({ + "enums" => {"Status" => {"values" => {"ACTIVE" => 1}}} + }) + }.to raise_error(Errors::SchemaError, a_string_including( + "Invalid protobuf field-number mappings", "Validation errors" + )) end end