From 6d5226c9f806746cb46489a12d1eba8c1be253ba Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Wed, 8 Jul 2026 18:19:37 -0500 Subject: [PATCH 1/4] 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 next available 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). - Mappings are validated: collisions raise clear errors, and non-integer numbers are rejected instead of letting `Integer()` silently truncate. `configure_proto_field_number_mappings` allows tests and advanced callers to seed mappings without an artifact file. --- elasticgraph-proto_ingestion/README.md | 48 ++- .../lib/elastic_graph/proto_ingestion.rb | 3 + .../schema_definition/api_extension.rb | 12 + .../proto_ingestion_state.rb | 2 +- .../schema_definition/results_extension.rb | 30 +- .../schema_definition/schema.rb | 347 ++++++++++++++- .../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 + .../schema_definition/api_extension.rbs | 1 + .../proto_ingestion_state.rbs | 5 +- .../schema_definition/results_extension.rbs | 2 + .../schema_definition/schema.rbs | 44 +- .../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 ++ .../schema_artifact_manager_extension_spec.rb | 49 ++- .../schema_edge_cases_spec.rb | 405 ++++++++++++++++++ .../schema_definition/schema_spec.rb | 332 +++++++++++++- 24 files changed, 1362 insertions(+), 49 deletions(-) diff --git a/elasticgraph-proto_ingestion/README.md b/elasticgraph-proto_ingestion/README.md index ba6ffe1b6..fafefb9ff 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 @@ -93,6 +96,49 @@ ElasticGraph.define_schema do |schema| end ``` +### 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. + +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 +``` + ## Type Mappings The generated `schema.proto` uses these built-in scalar mappings: 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/api_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/api_extension.rb index def52fad7..19a6963ef 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/api_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/api_extension.rb @@ -45,6 +45,18 @@ def proto_schema_artifacts(package_name:) nil end + # Configures proto field-number mappings directly from a hash. + # Useful for tests and advanced use cases where mappings are sourced outside artifacts. + # When artifacts are dumped, mappings from the existing `proto_field_numbers.yaml` artifact + # are loaded automatically; this method does not need to be called in that case. + # + # @param proto_field_number_mappings [Hash] + # @return [void] + def configure_proto_field_number_mappings(proto_field_number_mappings) + proto_ingestion_state.field_number_mappings = proto_field_number_mappings + nil + end + private # Returns this gem's state container. Centralizes the Steep cast that's needed because 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..6e9f3be3d 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 1100ba5d9..cdca68e5c 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 @@ -6,6 +6,7 @@ # # frozen_string_literal: true +require "elastic_graph/errors" 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" @@ -15,13 +16,38 @@ module ProtoIngestion module SchemaDefinition # Builds a `proto3` schema string from an ElasticGraph schema definition. class Schema + # Internal representation of a stored field-number mapping. + # + # @!attribute [r] field_number + # @return [Integer] + # @!attribute [r] name_in_index + # @return [String] + FieldNumberMapping = ::Data.define(:field_number, :name_in_index) + + # The largest field number protobuf allows (2^29 - 1). + MAX_FIELD_NUMBER = 536_870_911 + # Field numbers protobuf reserves for its own implementation; they may not be used as field tags. + RESERVED_FIELD_NUMBER_RANGE = 19_000..19_999 + # The largest enum value number protobuf allows (the int32 maximum). + MAX_ENUM_VALUE_NUMBER = 2_147_483_647 + # @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] + def initialize( + state:, + all_types:, + package_name:, + proto_field_number_mappings: {} + ) @state = state @all_types = all_types @package_name = package_name + @proto_field_number_mappings_by_message = normalize_proto_field_number_mappings(proto_field_number_mappings) + @proto_enum_value_numbers_by_enum = normalize_proto_enum_value_number_mappings(proto_field_number_mappings) + @used_field_numbers_by_message = {} + @used_enum_value_numbers_by_enum = {} end # Renders the schema as a valid `proto3` file. @@ -40,6 +66,88 @@ def to_proto sections.join("\n\n") + "\n" end + # Exposes normalized field-number and enum-value-number mappings for writing to artifact YAML. + # + # @return [Hash] + def field_number_mappings_for_artifact + { + "messages" => @proto_field_number_mappings_by_message + .sort_by { |message_name, _| message_name } + .to_h do |message_name, field_numbers| + [message_name, { + "fields" => field_numbers.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" => @proto_enum_value_numbers_by_enum + .sort_by { |enum_name, _| enum_name } + .to_h do |enum_name, value_numbers| + [enum_name, { + "values" => value_numbers.sort_by { |value_name, number| [number, value_name] }.to_h + }] + end + } + 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:) + mappings_for_message = @proto_field_number_mappings_by_message[message_name] ||= {} + used_numbers = @used_field_numbers_by_message[message_name] ||= ::Set.new(mappings_for_message.values.map(&:field_number)) + + mapping = mappings_for_message.fetch(public_field_name) do + migrate_renamed_field_mapping(mappings_for_message, type_name: type_name, public_field_name: public_field_name) || begin + next_field_number = next_available_field_number(used_numbers) + used_numbers << next_field_number + FieldNumberMapping.new(field_number: next_field_number, name_in_index: name_in_index) + end + end + + mapping = FieldNumberMapping.new(field_number: mapping.field_number, name_in_index: name_in_index) if mapping.name_in_index != name_in_index + mappings_for_message[public_field_name] = mapping + + duplicate_field_name = mappings_for_message.find do |mapped_field_name, mapped_field_number| + mapped_field_name != public_field_name && mapped_field_number.field_number == mapping.field_number + end&.first + + if duplicate_field_name + raise Errors::SchemaError, "Protobuf field-number mapping collision in message `#{message_name}`: " \ + "`#{duplicate_field_name}` and `#{public_field_name}` are both mapped to field number #{mapping.field_number}." + end + + mapping.field_number + end + + # Returns the stable protobuf numbers for an enum's values. + # + # @api private + def enum_value_numbers_for(enum_name, value_names) + value_numbers = @proto_enum_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 + private # Selects the indexed root types and every type transitively referenced by their protobuf @@ -62,9 +170,244 @@ 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 + + def valid_field_number?(number) + number.between?(1, MAX_FIELD_NUMBER) && !RESERVED_FIELD_NUMBER_RANGE.cover?(number) + 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 used_enum_value_numbers_for(enum_name, value_numbers) + @used_enum_value_numbers_by_enum[enum_name] ||= begin + duplicated_numbers = value_numbers.group_by { |_, number| number }.select { |_, entries| entries.size > 1 } + + duplicated_numbers.each_value do |entries| + value_names = entries.map(&:first).sort.map { |name| "`#{name}`" }.join(" and ") + raise Errors::SchemaError, "Protobuf enum value-number mapping collision in enum `#{enum_name}`: " \ + "#{value_names} are both mapped to number #{entries.first.last}." + end + + ::Set.new(value_numbers.values) + end + end + + def migrate_renamed_field_mapping(mappings_for_message, type_name:, public_field_name:) + renames_for_type = renamed_public_field_names_by_type_name.fetch(type_name) { return nil } + old_field_names = renames_for_type.fetch(public_field_name) { return nil } + + old_field_names.each do |old_field_name| + return mappings_for_message.delete(old_field_name) if mappings_for_message.key?(old_field_name) + end + + nil + end + + def normalize_proto_field_number_mappings(raw_mappings) + return {} if raw_mappings.nil? + unless raw_mappings.is_a?(Hash) + raise Errors::SchemaError, "Protobuf field-number mappings must be a Hash, got: #{raw_mappings.class}." + end + + messages_hash = + if raw_mappings.key?("messages") + raw_mappings.fetch("messages") + elsif raw_mappings.key?(:messages) + raw_mappings.fetch(:messages) + elsif raw_mappings.key?("enums") || raw_mappings.key?(:enums) + # The hash uses the sectioned artifact format but only maps enum value numbers. + {} # : ::Hash[untyped, untyped] + else + raw_mappings + end + + unless messages_hash.is_a?(Hash) + raise Errors::SchemaError, "Protobuf field-number mappings must have a `messages` Hash." + end + + normalized = {} # : ::Hash[::String, fieldNumberMappingsByFieldName] + + messages_hash.each do |message_name, field_numbers| + unless field_numbers.is_a?(Hash) + raise Errors::SchemaError, "Field-number mapping for message `#{message_name}` must be a Hash." + end + + normalized_fields = + if field_numbers.key?("fields") + field_numbers.fetch("fields") + elsif field_numbers.key?(:fields) + field_numbers.fetch(:fields) + else + field_numbers + end + + unless normalized_fields.is_a?(Hash) + raise Errors::SchemaError, "Field-number mapping for message `#{message_name}` must contain a `fields` Hash." + end + + normalized_message_name = message_name.to_s + normalized_field_numbers = {} # : fieldNumberMappingsByFieldName + + normalized_fields.each do |field_name, field_number_or_mapping| + normalized_field_name = field_name.to_s + normalized_field_number, normalized_name_in_index = normalize_field_number_mapping_entry( + normalized_message_name, + normalized_field_name, + field_number_or_mapping + ) + + unless valid_field_number?(normalized_field_number) + raise Errors::SchemaError, "Field-number mapping for `#{normalized_message_name}.#{normalized_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_or_mapping.inspect}." + end + + normalized_field_numbers[normalized_field_name] = FieldNumberMapping.new( + field_number: normalized_field_number, + name_in_index: normalized_name_in_index + ) + end + + normalized[normalized_message_name] = normalized_field_numbers + end + + normalized + end + + def normalize_proto_enum_value_number_mappings(raw_mappings) + return {} if raw_mappings.nil? + + enums_hash = + if raw_mappings.is_a?(Hash) && raw_mappings.key?("enums") + raw_mappings.fetch("enums") + elsif raw_mappings.is_a?(Hash) && raw_mappings.key?(:enums) + raw_mappings.fetch(:enums) + else + return {} + end + + unless enums_hash.is_a?(Hash) + raise Errors::SchemaError, "Protobuf enum value-number mappings must have an `enums` Hash." + end + + normalized = {} # : ::Hash[::String, ::Hash[::String, ::Integer]] + + enums_hash.each do |enum_name, value_numbers| + values_hash = + if value_numbers.is_a?(Hash) && value_numbers.key?("values") + value_numbers.fetch("values") + elsif value_numbers.is_a?(Hash) && value_numbers.key?(:values) + value_numbers.fetch(:values) + else + value_numbers + end + + unless values_hash.is_a?(Hash) + raise Errors::SchemaError, "Enum value-number mapping for enum `#{enum_name}` must contain a `values` Hash." + end + + normalized_enum_name = enum_name.to_s + normalized_value_numbers = {} # : ::Hash[::String, ::Integer] + + values_hash.each do |value_name, value_number| + normalized_number = integral_number(value_number) + + if normalized_number.nil? || normalized_number <= 0 || normalized_number > MAX_ENUM_VALUE_NUMBER + raise Errors::SchemaError, "Enum value-number mapping for `#{normalized_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 + + normalized_value_numbers[value_name.to_s] = normalized_number + end + + normalized[normalized_enum_name] = normalized_value_numbers + end + + normalized + end + + def normalize_field_number_mapping_entry(message_name, field_name, field_number_or_mapping) + if field_number_or_mapping.is_a?(Hash) + raw_field_number = + if field_number_or_mapping.key?("field_number") + field_number_or_mapping.fetch("field_number") + elsif field_number_or_mapping.key?(:field_number) + field_number_or_mapping.fetch(:field_number) + else + raise Errors::SchemaError, "Field-number mapping for `#{message_name}.#{field_name}` must include `field_number`." + end + + raw_name_in_index = + if field_number_or_mapping.key?("name_in_index") + field_number_or_mapping.fetch("name_in_index") + elsif field_number_or_mapping.key?(:name_in_index) + field_number_or_mapping.fetch(:name_in_index) + else + field_name + end + + unless raw_name_in_index.is_a?(String) || raw_name_in_index.is_a?(Symbol) + raise Errors::SchemaError, "Field-number mapping for `#{message_name}.#{field_name}` " \ + "must use a String or Symbol `name_in_index`, got: #{raw_name_in_index.inspect}." + end + + [integral_field_number(message_name, field_name, raw_field_number), raw_name_in_index.to_s] + else + [integral_field_number(message_name, field_name, field_number_or_mapping), field_name] + end + end + + def integral_field_number(message_name, field_name, raw_field_number) + number = integral_number(raw_field_number) + + if number.nil? + raise Errors::SchemaError, "Field-number mapping for `#{message_name}.#{field_name}` " \ + "must be an integer, got: #{raw_field_number.inspect}." + end + + number + end + + # Coerces a raw mapping number to an Integer without truncating: only `Integer` values and + # strings that parse as integers are accepted; anything else (including floats, which + # `Kernel#Integer` would silently truncate) returns `nil`. + def integral_number(raw) + case raw + when ::Integer then raw + when ::String then raw.match?(/\A-?\d+\z/) ? Integer(raw) : nil + end + 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.to_h do |type_name, old_to_new| + current_to_old = ::Hash.new { |h, k| h[k] = [] } # : ::Hash[::String, ::Array[::String]] + + old_to_new.each do |old_field_name, renamed_field| + current_to_old[renamed_field.name] << old_field_name + end + + [type_name, current_to_old] + 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..2a0bb322d 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 + protobuf_load_existing_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 protobuf_load_existing_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 e0275a647..a65ebec57 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. @@ -76,13 +76,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 9a9308fde..4ac68ec92 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 @@ -40,7 +40,7 @@ def finalize_protobuf_configuration! # 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/api_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/api_extension.rbs index e7bb15e78..e8ac37ee5 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/api_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/api_extension.rbs @@ -4,6 +4,7 @@ module ElasticGraph module APIExtension: ::ElasticGraph::SchemaDefinition::API def self.extended: (::ElasticGraph::SchemaDefinition::API & APIExtension) -> void def proto_schema_artifacts: (package_name: ::String) -> void + def configure_proto_field_number_mappings: (untyped) -> void private 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..2d5339a10 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: () -> 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 7ff135db9..7e7f15f70 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 @@ -2,22 +2,64 @@ module ElasticGraph module ProtoIngestion module SchemaDefinition class Schema + class FieldNumberMapping + attr_reader field_number: ::Integer + attr_reader name_in_index: ::String + + def self.new: (field_number: ::Integer, name_in_index: ::String) -> instance + end + + type fieldNumberMappingsByFieldName = ::Hash[::String, FieldNumberMapping] + + MAX_FIELD_NUMBER: ::Integer + RESERVED_FIELD_NUMBER_RANGE: ::Range[::Integer] + MAX_ENUM_VALUE_NUMBER: ::Integer + @state: ::ElasticGraph::SchemaDefinition::State @all_types: ::Array[::ElasticGraph::SchemaDefinition::SchemaElements::graphQLType] @package_name: ::String + @proto_field_number_mappings_by_message: ::Hash[::String, fieldNumberMappingsByFieldName] + @proto_enum_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]] + @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 valid_field_number?: (::Integer) -> bool + def next_available_field_number: (::Set[::Integer]) -> ::Integer + def next_available_enum_value_number: (::Set[::Integer]) -> ::Integer + def used_enum_value_numbers_for: (::String, ::Hash[::String, ::Integer]) -> ::Set[::Integer] + def migrate_renamed_field_mapping: ( + fieldNumberMappingsByFieldName, + type_name: ::String, + public_field_name: ::String + ) -> FieldNumberMapping? + def normalize_proto_field_number_mappings: (untyped) -> ::Hash[::String, fieldNumberMappingsByFieldName] + def normalize_proto_enum_value_number_mappings: (untyped) -> ::Hash[::String, ::Hash[::String, ::Integer]] + def normalize_field_number_mapping_entry: (::String, ::String, untyped) -> [::Integer, ::String] + def integral_field_number: (::String, ::String, untyped) -> ::Integer + def integral_number: (untyped) -> ::Integer? + 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..91342e799 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 protobuf_load_existing_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 406f72ab8..f3daa9b7d 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 @@ -11,13 +11,13 @@ module ElasticGraph def value: (::String) ?{ (::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & EnumValueExtension) -> void } -> void 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 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 proto_enum_value_prefix: () -> ::String 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 553990072..4db417d8c 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 @@ -9,7 +9,7 @@ module ElasticGraph def finalize_protobuf_configuration!: () -> 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/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 0cd3434fc..88919c505 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 @@ -109,6 +109,411 @@ 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 on field-number mapping collisions for a message" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + { + "messages" => { + "Account" => { + "id" => 1, + "name" => 1 + } + } + } + ) + + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "name", "String" + t.index "accounts" + end + end + + expect { + results.proto_schema + }.to raise_error(Errors::SchemaError, a_string_including("field-number mapping collision")) + end + + it "normalizes nil field-number mappings to empty hashes" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings nil + end + + expect(results.proto_schema).to eq("") + expect(results.proto_field_number_mappings).to eq({"enums" => {}, "messages" => {}}) + end + + it "validates field-number mapping input type" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings("bad") + + 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 Hash")) + end + + it "validates that `messages` is a hash in field-number mappings" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings({"messages" => "bad"}) + + 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 have a `messages` Hash")) + end + + it "accepts symbol `:messages` key in field-number mappings" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + { + messages: { + "Account" => { + "id" => 7 + } + } + } + ) + + s.object_type "Account" do |t| + t.field "id", "ID" + t.index "accounts" + end + end + + expect(results.proto_schema).to include("string id = 7;") + end + + it "accepts symbol `:fields` and nested symbol keys in field-number mappings" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + { + messages: { + "Account" => { + fields: { + "id" => { + field_number: 7, + name_in_index: :account_id + } + } + } + } + } + ) + + s.object_type "Account" do |t| + t.field "id", "ID", name_in_index: "account_id" + t.index "accounts" + end + end + + expect(results.proto_schema).to include("string id = 7;") + end + + it "validates per-message field-number mapping structure" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings({"messages" => {"Account" => "bad"}}) + + 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 Hash")) + end + + it "validates that nested `fields` is a hash in field-number mappings" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings({"messages" => {"Account" => {"fields" => "bad"}}}) + + 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 contain a `fields` Hash")) + end + + it "validates that mapped field numbers are valid protobuf field tags" do + [0, Schema::MAX_FIELD_NUMBER + 1, 19_000, 19_999].each do |invalid_number| + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings({"messages" => {"Account" => {"id" => invalid_number}}}) + + 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", "excluding the reserved 19000-19999 range", invalid_number.to_s + )) + end + end + + it "accepts the maximum protobuf field number while allowing enum values in the field-reserved range" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + { + "messages" => {"Account" => {"id" => Schema::MAX_FIELD_NUMBER}}, + # Enum value numbers have no protobuf-reserved range, so 19000-19999 is fine here. + "enums" => {"Status" => {"values" => {"ACTIVE" => 19_005}}} + } + ) + + s.enum_type "Status" do |t| + t.values "ACTIVE" + end + + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "status", "Status" + t.index "accounts" + end + end + + generated = results.proto_schema + expect(generated).to include("string id = #{Schema::MAX_FIELD_NUMBER};") + expect(generated).to include("STATUS_ACTIVE = 19005;") + end + + it "skips the protobuf-reserved range when allocating new field numbers" do + legacy_mappings = (1..18_999).to_h { |number| ["legacy_field_#{number}", number] } + + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings({"messages" => {"Account" => legacy_mappings}}) + + s.object_type "Account" do |t| + t.field "id", "ID" + t.index "accounts" + end + end + + expect(results.proto_schema).to include("string id = 20000;") + end + + it "validates that mapped enum value numbers do not exceed the int32 maximum" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + {"enums" => {"Status" => {"values" => {"ACTIVE" => Schema::MAX_ENUM_VALUE_NUMBER + 1}}}} + ) + + 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 positive integer no greater than 2147483647")) + end + + it "validates that mapped field numbers are integers" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings({"messages" => {"Account" => {"id" => "abc"}}}) + + 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 an integer")) + end + + it "accepts integer field numbers given as strings" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings({"messages" => {"Account" => {"id" => "7"}}}) + + s.object_type "Account" do |t| + t.field "id", "ID" + t.index "accounts" + end + end + + expect(results.proto_schema).to include("string id = 7;") + end + + it "accepts symbol `:enums` and `:values` keys and shorthand value hashes in enum value-number mappings" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + { + enums: { + "Status" => {values: {"INACTIVE" => 5}}, + "Color" => {"RED" => 3} + } + } + ) + + s.enum_type "Status" do |t| + t.values "ACTIVE", "INACTIVE" + end + + s.enum_type "Color" do |t| + t.values "RED" + end + + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "status", "Status" + t.field "color", "Color" + t.index "accounts" + end + end + + generated = results.proto_schema + expect(generated).to include("STATUS_ACTIVE = 1;", "STATUS_INACTIVE = 5;", "COLOR_RED = 3;") + end + + it "validates that `enums` is a hash in enum value-number mappings" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings({"enums" => "bad"}) + + 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 have an `enums` Hash")) + end + + it "validates per-enum value-number mapping structure" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings({"enums" => {"Status" => "bad"}}) + + 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 contain a `values` Hash")) + end + + it "validates that mapped enum value numbers are positive integers, since 0 is reserved" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings({"enums" => {"Status" => {"values" => {"ACTIVE" => 0}}}}) + + 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 positive integer", "_UNSPECIFIED")) + end + + it "validates that mapped enum value numbers are integers rather than truncating them" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings({"enums" => {"Status" => {"values" => {"ACTIVE" => 1.5}}}}) + + 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 positive integer", "1.5")) + end + + it "validates that structured mappings include `field_number`" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + {"messages" => {"Account" => {"fields" => {"id" => {"name_in_index" => "account_id"}}}}} + ) + + s.object_type "Account" do |t| + t.field "id", "ID", name_in_index: "account_id" + t.index "accounts" + end + end + + expect { + results.proto_schema + }.to raise_error(Errors::SchemaError, a_string_including("must include `field_number`")) + end + + it "validates that structured mappings use a String or Symbol `name_in_index`" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + {"messages" => {"Account" => {"fields" => {"id" => {"field_number" => 7, "name_in_index" => 123}}}}} + ) + + 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 use a String or Symbol `name_in_index`")) + end + + it "defaults structured mappings without `name_in_index` to the field name" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + {"messages" => {"Account" => {"fields" => {"display_name" => {"field_number" => 7}}}}} + ) + + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "display_name", "String" + t.index "accounts" + end + end + + expect(results.proto_schema).to include("string display_name = 7;") + end + + it "allocates the next available field number when a renamed field has no old mapping entry" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + {"messages" => {"Account" => {"fields" => {"other_name" => 7}}}} + ) + + 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(results.proto_schema).to include("string id = 1;") + expect(results.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..036e1f67b 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,115 @@ 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 "can assign field numbers from configured mappings" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + { + "messages" => { + "Account" => { + "id" => 10, + "name" => 2 + } + } + } + ) + + 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 = 10;") + expect(generated).to include("string name = 2;") + end + + it "assigns new field numbers after mapped values when mappings are partial" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + { + "messages" => { + "Account" => { + "id" => 1 + } + } + } + ) + + 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 name = 2;") + 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 "preserves reserved numbers for removed fields and allocates new numbers above them" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + { + "messages" => { + "Account" => { + "id" => 1, + "legacyField" => 2 + } + } + } + ) + + 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 = 1;") + expect(generated).to include("string name = 3;") + + expect(results.proto_field_number_mappings).to eq({ + "enums" => {}, + "messages" => { + "Account" => { + "fields" => { + "id" => 1, + "legacyField" => 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 +358,224 @@ 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 "preserves a field number across a public field rename" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + { + "messages" => { + "Account" => { + "fields" => { + "full_name" => 7 + } + } + } + } + ) + + 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(results.proto_schema).to include("string id = 1;") + expect(results.proto_schema).to include("string display_name = 7;") + expect(results.proto_field_number_mappings).to eq({ + "enums" => {}, + "messages" => { + "Account" => { + "fields" => { + "id" => 1, + "display_name" => 7 + } + } + } + }) + end + + it "generates the proto schema and assigns field numbers when no renames are declared" do + results = define_proto_schema_results do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "display_name", "String" + t.index "accounts" + end + end + + expect(results.proto_schema).to eq(<<~PROTO) + syntax = "proto3"; + + package elasticgraph; + + message Account { + string id = 1; + string display_name = 2; + } + PROTO + + expect(results.proto_field_number_mappings).to eq({ + "enums" => {}, + "messages" => { + "Account" => { + "fields" => { + "id" => 1, + "display_name" => 2 + } + } + } + }) + end + + it "preserves enum value numbers from configured mappings, reserving removed values' numbers" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + { + "enums" => { + "Status" => { + "values" => { + "ACTIVE" => 1, + "PAUSED" => 2, + "INACTIVE" => 3 + } + } + } + } + ) + + # `PAUSED` has been removed since the mappings were dumped, and `ARCHIVED` has been added. + 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(results.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(results.proto_field_number_mappings.fetch("enums")).to eq({ + "Status" => { + "values" => { + "ACTIVE" => 1, + "PAUSED" => 2, + "INACTIVE" => 3, + "ARCHIVED" => 4 + } + } + }) + end + + it "raises a clear error when the configured mappings assign the same number to two enum values" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + {"enums" => {"Status" => {"values" => {"ACTIVE" => 1, "INACTIVE" => 1}}}} + ) + + s.enum_type "Status" do |t| + t.values "ACTIVE", "INACTIVE" + end + + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "status", "Status" + t.index "accounts" + end + end + + expect { + results.proto_schema + }.to raise_error(Errors::SchemaError, a_string_including( + "enum value-number mapping collision in enum `Status`", + "`ACTIVE` and `INACTIVE`", + "number 1" + )) + end + + it "rejects field numbers that are not integers rather than silently truncating them" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + {"messages" => {"Account" => {"fields" => {"id" => 1.5}}}} + ) + + 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("`Account.id`", "must be an integer", "1.5")) + end + + it "preserves stable field numbers for oneof alternatives" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + { + "messages" => { + "Vehicle" => { + "fields" => { + "removed" => 1, + "car" => 4, + "bike" => 8 + } + } + } + } + ) + + ["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(results.proto_schema, "Vehicle") + expect(vehicle).to include("Car car = 4;", "Bike bike = 8;", "Scooter scooter = 2;") + expect(results.proto_field_number_mappings.fetch("messages").fetch("Vehicle")).to eq({ + "fields" => { + "removed" => 1, + "scooter" => 2, + "car" => 4, + "bike" => 8 + } + }) end it "renders independently each time it is called" do From 178a5865156e7cc02e9bddc13d519392fff2e268 Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Thu, 16 Jul 2026 09:17:13 -0500 Subject: [PATCH 2/4] Cover protobuf numbering boundaries --- .../schema_edge_cases_spec.rb | 12 ++++++---- .../schema_definition/schema_spec.rb | 23 +++++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) 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 88919c505..52c2cf5a8 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 @@ -269,18 +269,21 @@ module SchemaDefinition end end - it "accepts the maximum protobuf field number while allowing enum values in the field-reserved range" do + it "accepts maximum protobuf numbers while allowing enum values in the field-reserved range" do results = define_proto_schema_results do |s| s.configure_proto_field_number_mappings( { "messages" => {"Account" => {"id" => Schema::MAX_FIELD_NUMBER}}, # Enum value numbers have no protobuf-reserved range, so 19000-19999 is fine here. - "enums" => {"Status" => {"values" => {"ACTIVE" => 19_005}}} + "enums" => {"Status" => {"values" => { + "ACTIVE" => Schema::MAX_ENUM_VALUE_NUMBER, + "INACTIVE" => 19_005 + }}} } ) s.enum_type "Status" do |t| - t.values "ACTIVE" + t.values "ACTIVE", "INACTIVE" end s.object_type "Account" do |t| @@ -292,7 +295,8 @@ module SchemaDefinition generated = results.proto_schema expect(generated).to include("string id = #{Schema::MAX_FIELD_NUMBER};") - expect(generated).to include("STATUS_ACTIVE = 19005;") + expect(generated).to include("STATUS_ACTIVE = #{Schema::MAX_ENUM_VALUE_NUMBER};") + expect(generated).to include("STATUS_INACTIVE = 19005;") 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 036e1f67b..c4ef11280 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 @@ -311,6 +311,29 @@ module SchemaDefinition }) end + it "exposes field and enum number mappings in canonical order" do + results = define_proto_schema_results do |s| + s.configure_proto_field_number_mappings( + { + "messages" => { + "ZMessage" => {"fields" => {"second" => 2, "first" => 1}}, + "AMessage" => {"fields" => {"only" => 3}} + }, + "enums" => { + "ZEnum" => {"values" => {"SECOND" => 2, "FIRST" => 1}}, + "AEnum" => {"values" => {"ONLY" => 3}} + } + } + ) + end + + mappings = results.proto_field_number_mappings + expect(mappings.fetch("messages").keys).to eq(["AMessage", "ZMessage"]) + expect(mappings.dig("messages", "ZMessage", "fields").keys).to eq(["first", "second"]) + expect(mappings.fetch("enums").keys).to eq(["AEnum", "ZEnum"]) + expect(mappings.dig("enums", "ZEnum", "values").keys).to eq(["FIRST", "SECOND"]) + end + it "preserves reserved numbers for removed fields and allocates new numbers above them" do results = define_proto_schema_results do |s| s.configure_proto_field_number_mappings( From ddadbc7c874c012dd4429245394d9869ac4f6c36 Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Thu, 9 Jul 2026 10:52:21 -0500 Subject: [PATCH 3/4] Map DateTime to google.protobuf.Timestamp and document temporal string formats `DateTime` fields now use the well-known `google.protobuf.Timestamp` type (with `google/protobuf/timestamp.proto` imported automatically) instead of `string`. A Timestamp cannot be malformed the way a string can, and proto consumers get language-native timestamp types. Note that a Timestamp is a UTC instant, so a publisher's original UTC offset is not preserved; `t.protobuf type: "string"` remains available to override. To support this, `t.protobuf` gains two options usable by any scalar: - `import:` maps a scalar to an externally defined proto type, emitting the needed `import` statement in `schema.proto`. - `comment:` documents the expected format on each generated field, used by the built-in `string`-typed temporal scalars (`Date`, `LocalTime`, `TimeZone`) whose proto type is wider than the ElasticGraph type (e.g. `// ISO 8601 date`). Values are still validated at ingestion time, just as with JSON ingestion. --- elasticgraph-proto_ingestion/README.md | 53 ++++++++++++----- .../schema_definition/factory_extension.rb | 38 +++++++------ .../schema_definition/schema.rb | 9 +++ .../object_interface_and_union_extension.rb | 6 +- .../schema_elements/scalar_type_extension.rb | 16 +++++- .../schema_definition/factory_extension.rbs | 2 +- .../schema_definition/schema.rbs | 1 + .../object_interface_and_union_extension.rbs | 2 +- .../schema_elements/scalar_type_extension.rbs | 4 +- .../schema_definition/api_extension_spec.rb | 9 +-- .../schema_definition/schema_spec.rb | 57 +++++++++++++++++++ 11 files changed, 154 insertions(+), 43 deletions(-) diff --git a/elasticgraph-proto_ingestion/README.md b/elasticgraph-proto_ingestion/README.md index fafefb9ff..d3379d44b 100644 --- a/elasticgraph-proto_ingestion/README.md +++ b/elasticgraph-proto_ingestion/README.md @@ -96,6 +96,22 @@ ElasticGraph.define_schema do |schema| end ``` +A custom scalar can also map to an externally defined proto type by passing `import:` with the +proto file that defines it, and `comment:` documents the expected format on each generated field +(useful when the proto type is wider than the ElasticGraph type): + +```ruby +# in config/schema/phone_number.rb + +ElasticGraph.define_schema do |schema| + schema.scalar_type "PhoneNumber" do |t| + t.mapping type: "keyword" + t.json_schema type: "string" + t.protobuf type: "string", comment: "E.164 phone number" + end +end +``` + ### Stable Field Numbers `schema_artifacts:dump` automatically reads and writes `proto_field_numbers.yaml` @@ -143,23 +159,30 @@ enums: The generated `schema.proto` uses these built-in scalar mappings: -| ElasticGraph Type | Protobuf Type | -|-------------------|------------| -| `Boolean` | `bool` | -| `Cursor` | `string` | -| `Date` | `string` | -| `DateTime` | `string` | -| `Float` | `double` | -| `ID` | `string` | -| `Int` | `int32` | -| `JsonSafeLong` | `int64` | -| `LocalTime` | `string` | -| `LongString` | `int64` | -| `String` | `string` | -| `TimeZone` | `string` | -| `Untyped` | `string` | +| ElasticGraph Type | Protobuf Type | +|-------------------|-----------------------------| +| `Boolean` | `bool` | +| `Cursor` | `string` | +| `Date` | `string` | +| `DateTime` | `google.protobuf.Timestamp` | +| `Float` | `double` | +| `ID` | `string` | +| `Int` | `int32` | +| `JsonSafeLong` | `int64` | +| `LocalTime` | `string` | +| `LongString` | `int64` | +| `String` | `string` | +| `TimeZone` | `string` | +| `Untyped` | `string` | Additionally: +- `DateTime` uses the [well-known `Timestamp` type](https://protobuf.dev/reference/protobuf/google.protobuf/#timestamp); + `schema.proto` imports `google/protobuf/timestamp.proto` automatically. Note that a `Timestamp` + is a UTC instant, so a publisher's original UTC offset is not preserved. +- `string`-typed temporal scalars (`Date`, `LocalTime`, `TimeZone`) are wider than the + ElasticGraph types they carry, so generated fields of these types document the expected format + in a comment (e.g. `// ISO 8601 date, e.g. "2024-11-25"`). Values are validated when events + are ingested, just as with JSON ingestion. - List types become `repeated` fields. - 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. diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/factory_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/factory_extension.rb index 5548535aa..d2ab3df85 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/factory_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/factory_extension.rb @@ -18,21 +18,21 @@ module ProtoIngestion module SchemaDefinition # Extension module applied to Factory to add proto support. module FactoryExtension - # Default protobuf types applied to ElasticGraph's built-in scalar types as they are constructed. - BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME = { - "Boolean" => "bool", - "Cursor" => "string", - "Date" => "string", - "DateTime" => "string", - "Float" => "double", - "ID" => "string", - "Int" => "int32", - "JsonSafeLong" => "int64", - "LocalTime" => "string", - "LongString" => "int64", - "String" => "string", - "TimeZone" => "string", - "Untyped" => "string" + # Default protobuf options applied to ElasticGraph's built-in scalar types as they are constructed. + BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME = { + "Boolean" => {type: "bool"}, + "Cursor" => {type: "string"}, + "Date" => {type: "string", comment: %(ISO 8601 date, e.g. "2024-11-25")}, + "DateTime" => {type: "google.protobuf.Timestamp", import: "google/protobuf/timestamp.proto"}, + "Float" => {type: "double"}, + "ID" => {type: "string"}, + "Int" => {type: "int32"}, + "JsonSafeLong" => {type: "int64"}, + "LocalTime" => {type: "string", comment: %(ISO 8601 local time, e.g. "14:23:12")}, + "LongString" => {type: "int64"}, + "String" => {type: "string"}, + "TimeZone" => {type: "string", comment: %(IANA time zone identifier, e.g. "America/Los_Angeles")}, + "Untyped" => {type: "string"} }.freeze # Creates a new enum type with proto extensions. @@ -93,8 +93,12 @@ def new_scalar_type(name) super(name) do |type| extended_type = type.extend(SchemaElements::ScalarTypeExtension) # : ::ElasticGraph::SchemaDefinition::SchemaElements::ScalarType & SchemaElements::ScalarTypeExtension - if (proto_type = BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME[name.to_s]) - extended_type.protobuf type: proto_type + if (proto_options = BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME[name.to_s]) + extended_type.protobuf( + type: proto_options.fetch(:type), + import: proto_options[:import], + comment: proto_options[:comment] + ) end yield extended_type if block_given? 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 cdca68e5c..ef419c0d2 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 @@ -60,6 +60,7 @@ def to_proto sections = [ %(syntax = "proto3";), "package #{@package_name};", + *render_imports(types), render_definitions(types) ] @@ -174,6 +175,14 @@ def render_definitions(types) .join("\n\n") end + def render_imports(types) + imports = types.filter_map do |type| + type.protobuf_import if type.respond_to?(:protobuf_import) + end.uniq.sort + + imports.empty? ? [] : [imports.map { |import| %(import "#{import}";) }.join("\n")] + end + def valid_field_number?(number) number.between?(1, MAX_FIELD_NUMBER) && !RESERVED_FIELD_NUMBER_RANGE.cover?(number) 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..fd64cf730 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 @@ -74,7 +74,7 @@ def render_proto_message(schema, message_name, package_name) fields = proto_fields 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( + repeated, field_type, type_comment = proto_field_type_for( field.type, package_name: package_name, context_field_name: field.name @@ -87,6 +87,7 @@ def render_proto_message(schema, message_name, package_name) ) label = "repeated " if repeated line = " #{label}#{field_type} #{schema_field.name} = #{field_number};" + line += " // #{type_comment}" if type_comment field_documentation = ProtoDocumentation .comment_lines_for(schema_field.doc_comment, indent: " ") .map { |comment_line| "#{comment_line}\n" } @@ -152,7 +153,8 @@ def proto_field_type_for(type_ref, package_name:, context_field_name:) end proto_type = _ = base_type_ref.resolved - [list_depth == 1, proto_type.proto_type_reference(package_name)] + type_comment = (ScalarTypeExtension === proto_type) ? proto_type.protobuf_comment : nil + [list_depth == 1, proto_type.proto_type_reference(package_name), type_comment] end end 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 4ac68ec92..f0e7f374e 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 @@ -18,12 +18,24 @@ module ScalarTypeExtension # @dynamic protobuf_type attr_reader :protobuf_type + # Proto file to import for the configured protobuf type, if it is externally defined. + # @dynamic protobuf_import + attr_reader :protobuf_import + + # Comment rendered on generated proto fields of this scalar type. + # @dynamic protobuf_comment + attr_reader :protobuf_comment + # Configures the protobuf type for this scalar type. # - # @param type [String] protobuf scalar type name + # @param type [String] protobuf type name + # @param import [String, nil] proto file to import for an externally defined type + # @param comment [String, nil] comment rendered on generated fields of this type # @return [void] - def protobuf(type:) + def protobuf(type:, import: nil, comment: nil) @protobuf_type = type + @protobuf_import = import + @protobuf_comment = comment end # Validates that a protobuf type has been configured on this scalar type. GraphQL-only diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/factory_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/factory_extension.rbs index 620a1664c..7b4d58227 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/factory_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/factory_extension.rbs @@ -2,7 +2,7 @@ module ElasticGraph module ProtoIngestion module SchemaDefinition module FactoryExtension: ::ElasticGraph::SchemaDefinition::Factory - BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME: ::Hash[::String, ::String] + BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME: ::Hash[::String, ::Hash[::Symbol, ::String]] def new_enum_type: (::String) ?{ (::ElasticGraph::SchemaDefinition::SchemaElements::EnumType & SchemaElements::EnumTypeExtension) -> void } -> (::ElasticGraph::SchemaDefinition::SchemaElements::EnumType & SchemaElements::EnumTypeExtension) def new_enum_value: (::String, ::String) ?{ (::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & SchemaElements::EnumValueExtension) -> void } -> (::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & SchemaElements::EnumValueExtension) 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 7e7f15f70..5377adf08 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 @@ -45,6 +45,7 @@ module ElasticGraph def proto_types: () -> ::Array[untyped] def render_definitions: (::Array[untyped] types) -> ::String + def render_imports: (::Array[untyped] types) -> ::Array[::String] def valid_field_number?: (::Integer) -> bool def next_available_field_number: (::Set[::Integer]) -> ::Integer def next_available_enum_value_number: (::Set[::Integer]) -> ::Integer 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 608f27a7d..41503a099 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 @@ -28,7 +28,7 @@ module ElasticGraph ::ElasticGraph::SchemaDefinition::SchemaElements::TypeReference, package_name: ::String, context_field_name: ::String - ) -> [bool, ::String] + ) -> [bool, ::String, ::String?] end end end 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 4db417d8c..f4d76718a 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 @@ -4,8 +4,10 @@ module ElasticGraph module SchemaElements module ScalarTypeExtension: ::ElasticGraph::SchemaDefinition::SchemaElements::ScalarType attr_reader protobuf_type: ::String? + attr_reader protobuf_import: ::String? + attr_reader protobuf_comment: ::String? - def protobuf: (type: ::String) -> void + def protobuf: (type: ::String, ?import: ::String?, ?comment: ::String?) -> void def finalize_protobuf_configuration!: () -> void def proto_name: () -> ::String def proto_type_reference: (::String package_name) -> ::String diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb index 196308eaf..2fc562c4d 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb @@ -35,7 +35,7 @@ module SchemaDefinition field_types = [] proto = define_proto_schema do |s| s.object_type "Widget" do |t| - FactoryExtension::BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME.each_key do |type_name| + FactoryExtension::BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME.each_key do |type_name| t.field type_name.downcase, type_name end field_types = t.graphql_fields_by_name.values.map { |field| field.type.name } @@ -43,9 +43,10 @@ module SchemaDefinition end end - expect(field_types).to match_array(FactoryExtension::BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME.keys) - FactoryExtension::BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME.each.with_index(1) do |(type_name, proto_type), field_number| - expect(proto).to include("#{proto_type} #{type_name.downcase} = #{field_number};") + expect(field_types).to match_array(FactoryExtension::BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME.keys) + expect(proto).to include('import "google/protobuf/timestamp.proto";') + FactoryExtension::BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME.each.with_index(1) do |(type_name, proto_options), field_number| + expect(proto).to include("#{proto_options.fetch(:type)} #{type_name.downcase} = #{field_number};") 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 c4ef11280..332f5cd29 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,6 +242,63 @@ module SchemaDefinition expect(proto_type_def_from(proto, "Event")).to include("int64 occurred_at = 2;") end + it "maps `DateTime` fields to `google.protobuf.Timestamp`, importing its proto file once" do + proto = define_proto_schema do |s| + s.object_type "Event" do |t| + t.field "id", "ID" + t.field "created_at", "DateTime" + t.field "updated_at", "DateTime" + t.index "events" + end + end + + expect(proto).to eq(<<~PROTO) + syntax = "proto3"; + + package elasticgraph; + + import "google/protobuf/timestamp.proto"; + + message Event { + string id = 1; + google.protobuf.Timestamp created_at = 2; + google.protobuf.Timestamp updated_at = 3; + } + PROTO + end + + it "renders format comments on fields whose scalar type documents one" do + proto = define_proto_schema do |s| + s.object_type "Person" do |t| + t.field "id", "ID" + t.field "birth_date", "Date" + t.index "people" + end + end + + expect(proto).to include( + %(string birth_date = 2; // ISO 8601 date, e.g. "2024-11-25") + ) + end + + it "supports `import:` and `comment:` on custom scalar types" do + proto = define_proto_schema do |s| + s.scalar_type "Money" do |t| + t.mapping type: "keyword" + t.protobuf type: "myapp.types.Money", import: "myapp/types/money.proto", comment: "amount + currency" + end + + s.object_type "Order" do |t| + t.field "id", "ID" + t.field "total", "Money" + t.index "orders" + end + end + + expect(proto).to include('import "myapp/types/money.proto";') + expect(proto).to include("myapp.types.Money total = 2; // amount + currency") + end + it "can assign field numbers from configured mappings" do results = define_proto_schema_results do |s| s.configure_proto_field_number_mappings( From d676254103fb40d559fc1b95d2862895ba959031 Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Thu, 16 Jul 2026 09:17:48 -0500 Subject: [PATCH 4/4] Test protobuf import stability --- .../schema_definition/api_extension_spec.rb | 9 +++--- .../schema_definition/schema_spec.rb | 32 +++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb index 2fc562c4d..f717102fe 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb @@ -32,10 +32,11 @@ module SchemaDefinition end it "maps every built-in scalar to a proto field type" do + built_in_scalar_options = FactoryExtension::BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME field_types = [] proto = define_proto_schema do |s| s.object_type "Widget" do |t| - FactoryExtension::BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME.each_key do |type_name| + built_in_scalar_options.each_key do |type_name| t.field type_name.downcase, type_name end field_types = t.graphql_fields_by_name.values.map { |field| field.type.name } @@ -43,10 +44,10 @@ module SchemaDefinition end end - expect(field_types).to match_array(FactoryExtension::BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME.keys) + expect(field_types).to match_array(built_in_scalar_options.keys) expect(proto).to include('import "google/protobuf/timestamp.proto";') - FactoryExtension::BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME.each.with_index(1) do |(type_name, proto_options), field_number| - expect(proto).to include("#{proto_options.fetch(:type)} #{type_name.downcase} = #{field_number};") + built_in_scalar_options.each.with_index(1) do |(type_name, options), field_number| + expect(proto).to include("#{options.fetch(:type)} #{type_name.downcase} = #{field_number};") 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 332f5cd29..c899c0689 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 @@ -267,6 +267,38 @@ module SchemaDefinition PROTO end + it "sorts and de-duplicates imports shared by distinct protobuf types" do + proto = define_proto_schema do |s| + s.scalar_type "FirstZType" do |t| + t.mapping type: "keyword" + t.protobuf type: "example.FirstZType", import: "z/types.proto" + end + + s.scalar_type "SecondZType" do |t| + t.mapping type: "keyword" + t.protobuf type: "example.SecondZType", import: "z/types.proto" + end + + s.scalar_type "AType" do |t| + t.mapping type: "keyword" + t.protobuf type: "example.AType", import: "a/types.proto" + end + + s.object_type "Event" do |t| + t.field "id", "ID" + t.field "first_z", "FirstZType" + t.field "second_z", "SecondZType" + t.field "a", "AType" + t.index "events" + end + end + + expect(proto.lines.grep(/\Aimport /).map(&:chomp)).to eq([ + %(import "a/types.proto";), + %(import "z/types.proto";) + ]) + end + it "renders format comments on fields whose scalar type documents one" do proto = define_proto_schema do |s| s.object_type "Person" do |t|