From 156369c2ef9b968b5ab6eab33a8e8e0b3549033b Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Tue, 21 Jul 2026 09:27:04 -0500 Subject: [PATCH] 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. --- elasticgraph-proto_ingestion/README.md | 52 ++- .../lib/elastic_graph/proto_ingestion.rb | 3 + .../field_number_mappings.rb | 314 ++++++++++++++++++ .../proto_ingestion_state.rb | 2 +- .../schema_definition/results_extension.rb | 30 +- .../schema_definition/schema.rb | 50 ++- .../schema_artifact_manager_extension.rb | 25 ++ .../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 | 2 +- .../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_manager_extension.rbs | 2 + .../schema_elements/enum_type_extension.rbs | 4 +- .../object_interface_and_union_extension.rbs | 6 +- .../schema_elements/scalar_type_extension.rbs | 2 +- .../schema_definition/rake_tasks_spec.rb | 43 +++ .../spec/support/proto_schema_support.rb | 18 +- .../field_number_mappings_spec.rb | 207 ++++++++++++ .../schema_artifact_manager_extension_spec.rb | 49 ++- .../schema_edge_cases_spec.rb | 57 ++++ .../schema_definition/schema_spec.rb | 298 ++++++++++++++++- 26 files changed, 1236 insertions(+), 53 deletions(-) create mode 100644 elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb create mode 100644 elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs create mode 100644 elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb diff --git a/elasticgraph-proto_ingestion/README.md b/elasticgraph-proto_ingestion/README.md index ba6ffe1b6..5f8203b13 100644 --- a/elasticgraph-proto_ingestion/README.md +++ b/elasticgraph-proto_ingestion/README.md @@ -72,7 +72,10 @@ 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: + +- `schema.proto` +- `proto_field_numbers.yaml` ## Schema Definition API @@ -118,3 +121,50 @@ 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` +in the schema artifacts directory. Existing numbers stay fixed even if field order +changes, and new fields get the next available numbers. Field numbers follow protobuf's +rules: they must be between 1 and 536,870,911, and the protobuf-reserved 19000-19999 +range is never allocated and is rejected in mappings. + +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..2ac3b8186 --- /dev/null +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb @@ -0,0 +1,314 @@ +# 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) + + mapping = mappings_for_message.fetch(public_field_name) do + migrate_renamed_field_mapping(mappings_for_message, previous_field_names) || begin + next_field_number = next_available_field_number(used_numbers) + used_numbers << next_field_number + FieldMapping.new(field_number: next_field_number, name_in_index: name_in_index) + end + end + + mapping = mapping.with(name_in_index: name_in_index) if mapping.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.fetch(value_name) do + next_available_enum_value_number(used_numbers).tap do |allocated| + used_numbers << allocated + value_numbers[value_name] = allocated + end + end + [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 + + # 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 next_available_field_number(used_numbers) + candidate = 1 + candidate += 1 while used_numbers.include?(candidate) || RESERVED_FIELD_NUMBER_RANGE.cover?(candidate) + candidate + end + + # 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 next_available_enum_value_number(used_numbers) + candidate = 1 + candidate += 1 while used_numbers.include?(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..fa02e789c 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 + # Ensure generation has occurred before reading mappings from the generator. + 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..af7f76221 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,18 @@ 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) + renamed_public_field_names_by_type_name.dig(type_name, public_field_name) || [] + end + + def renamed_public_field_names_by_type_name + @renamed_public_field_names_by_type_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_manager_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rb index 352c2df84..ead26587d 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 @@ -7,6 +7,7 @@ # frozen_string_literal: true require "elastic_graph/proto_ingestion" +require "yaml" module ElasticGraph module ProtoIngestion @@ -20,11 +21,21 @@ module SchemaArtifactManagerExtension # Overrides the base `artifacts_from_schema_def` method to add proto artifacts. def artifacts_from_schema_def + load_existing_proto_field_number_mappings + base_artifacts = super proto_schema = protobuf_schema_definition_results.proto_schema return base_artifacts if proto_schema.empty? base_artifacts + [ + new_yaml_artifact( + PROTO_FIELD_NUMBERS_FILE, + protobuf_schema_definition_results.proto_field_number_mappings, + extra_comment_lines: [ + "This file reserves protobuf field and enum value numbers to keep them stable over time.", + "Do not renumber existing entries." + ] + ), new_raw_artifact(PROTO_SCHEMA_FILE, proto_schema.chomp, comment_prefix: "//") ] end @@ -35,6 +46,20 @@ def artifacts_from_schema_def def protobuf_schema_definition_results schema_definition_results # : ElasticGraph::SchemaDefinition::Results & ResultsExtension end + + def proto_ingestion_state + extension_state = protobuf_schema_definition_results.state # : ElasticGraph::SchemaDefinition::State & StateExtension + extension_state.proto_ingestion_state + end + + # Seeds the schema generator with the field-number mappings from the previously dumped + # artifact (if any) so that field numbers remain stable across dumps. + def load_existing_proto_field_number_mappings + full_path = ::File.join(@schema_artifacts_directory, PROTO_FIELD_NUMBERS_FILE) + return unless ::File.exist?(full_path) + + proto_ingestion_state.field_number_mappings = ::YAML.safe_load_file(full_path, aliases: false) + 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..891ae5a2d 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 @@ -21,7 +21,7 @@ module StateExtension def self.extended(state) state.instance_variable_set( :@proto_ingestion_state, - ProtoIngestionState.new(package_name: "elasticgraph") + ProtoIngestionState.new(package_name: "elasticgraph", field_number_mappings: {}) ) 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..8dc56ebef --- /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 next_available_field_number: (::Set[::Integer]) -> ::Integer + def next_available_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..83b4f8b03 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 + @renamed_public_field_names_by_type_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 renamed_public_field_names_by_type_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_manager_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rbs index 4298c1a99..5a7ffb8d1 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 @@ -6,6 +6,8 @@ module ElasticGraph def artifacts_from_schema_def: () -> ::Array[::ElasticGraph::SchemaDefinition::SchemaArtifact[untyped]] def protobuf_schema_definition_results: () -> (::ElasticGraph::SchemaDefinition::Results & ResultsExtension) + def proto_ingestion_state: () -> ProtoIngestionState + def load_existing_proto_field_number_mappings: () -> void 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/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..4f702ed4b 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 @@ -52,6 +53,44 @@ module SchemaDefinition expect(output.lines).to include(a_string_including("already up to date", PROTO_SCHEMA_FILE)) }.to maintain { read_artifact(PROTO_SCHEMA_FILE) } end + + it "can persist and reuse proto field-number mappings from an artifact file" 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 + + run_rake_with_proto("schema_artifacts:dump") + + expect(read_artifact(PROTO_FIELD_NUMBERS_FILE)).not_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 name = 2;") + expect(read_artifact(PROTO_SCHEMA_FILE)).to include("string id = 1;") + end end private @@ -81,6 +120,10 @@ def read_artifact(name) path = File.join("config", "schema", "artifacts", name) File.read(path) if File.exist?(path) end + + def parsed_proto_field_numbers + ::YAML.safe_load(read_artifact(PROTO_FIELD_NUMBERS_FILE)) + 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..95407fdcf 100644 --- a/elasticgraph-proto_ingestion/spec/support/proto_schema_support.rb +++ b/elasticgraph-proto_ingestion/spec/support/proto_schema_support.rb @@ -18,13 +18,23 @@ 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 dumped, mirroring how `schema_artifacts:dump` + # loads the `proto_field_numbers.yaml` artifact from the prior dump. That 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 artifact). + 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..c1e5b6345 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,20 @@ # 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 + it "dumps proto artifacts alongside the base artifacts" do artifact_base_names = artifacts_for(define_indexed_type_schema) - expect(artifact_base_names).to include(PROTO_SCHEMA_FILE) + expect(artifact_base_names).to include(PROTO_SCHEMA_FILE, PROTO_FIELD_NUMBERS_FILE) 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" @@ -36,7 +37,47 @@ module SchemaDefinition artifact_base_names = artifacts_for(results) - expect(artifact_base_names).not_to include(PROTO_SCHEMA_FILE) + expect(artifact_base_names).not_to include(PROTO_SCHEMA_FILE, PROTO_FIELD_NUMBERS_FILE) + end + + it "seeds proto generation with the field-number mappings from a previously dumped artifact" do + ::FileUtils.mkdir_p("artifacts") + ::File.write(::File.join("artifacts", PROTO_FIELD_NUMBERS_FILE), <<~YAML) + messages: + Widget: + fields: + id: 7 + YAML + + results = define_indexed_type_schema + artifacts_for(results) + + expect(results.proto_schema).to include("string id = 7;") + end + + it "seeds proto generation with the enum value numbers from a previously dumped artifact" do + ::FileUtils.mkdir_p("artifacts") + ::File.write(::File.join("artifacts", PROTO_FIELD_NUMBERS_FILE), <<~YAML) + enums: + Status: + values: + INACTIVE: 5 + YAML + + results = define_proto_schema_results 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 + artifacts_for(results) + + expect(results.proto_schema).to include("STATUS_ACTIVE = 1;", "STATUS_INACTIVE = 5;") end def define_indexed_type_schema 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..e8dc7272f 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,63 @@ 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;") + expect(results2.proto_schema).to include("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..f41bfada1 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,123 @@ 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 + + generated = results.proto_schema + expect(generated).to include("string id = 7;") + expect(generated).to include("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 + + generated = results2.proto_schema + expect(generated).to include("string id = 1;") + expect(generated).to include("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 +366,182 @@ 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;") + expect(results2.proto_schema).to include("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