diff --git a/config/site/support/doctest_helper.rb b/config/site/support/doctest_helper.rb index 70c2c9ca2..6a7e55c22 100644 --- a/config/site/support/doctest_helper.rb +++ b/config/site/support/doctest_helper.rb @@ -64,6 +64,7 @@ module ElasticGraph @api = SchemaDefinition::API.new( SchemaArtifacts::RuntimeMetadata::SchemaElementNames.new(form: :camelCase, overrides: {}), true, + path_to_schema: "#{@tmp_dir}/schema.rb", extension_modules: extension_modules ) diff --git a/elasticgraph-proto_ingestion/README.md b/elasticgraph-proto_ingestion/README.md index ba6ffe1b6..8b77d6a2c 100644 --- a/elasticgraph-proto_ingestion/README.md +++ b/elasticgraph-proto_ingestion/README.md @@ -72,7 +72,8 @@ ElasticGraph.define_schema do |schema| end ``` -After running `bundle exec rake schema_artifacts:dump`, ElasticGraph will generate `schema.proto`. +After running `bundle exec rake schema_artifacts:dump`, ElasticGraph will generate a `schema.proto` +schema artifact, and will maintain a `proto_field_numbers.yaml` file alongside your schema definition. ## Schema Definition API @@ -118,3 +119,67 @@ Additionally: - Lists of lists (e.g. `[[Float!]!]!`) are not supported because Protocol Buffers cannot represent them directly. Schema artifact generation raises an error identifying the unsupported field. - Enum types generate `enum` definitions whose values are prefixed with the enum type name in `UPPER_SNAKE_CASE`, including a zero-valued `*_UNSPECIFIED` entry. + +## Stable Field Numbers + +`schema_artifacts:dump` automatically reads and writes `proto_field_numbers.yaml`, +stored alongside your schema definition (as a sibling of the file `path_to_schema` +points to). Existing numbers stay fixed even if field or enum value order changes. New fields, +`oneof` alternatives, and enum values use their type's stored `next_number`, so gaps below that +cursor are never filled: + +```yaml +messages: + Widget: + fields: + id: 1 + display_name: 2 + next_number: 3 +``` + +Unlike the schema artifacts--which are safe to delete and regenerate at any time--this +file is part of your schema definition: it is an input to `schema.proto` generation. +While prototyping, you may delete it and regenerate it to reset the number assignments. +Once generated protos have been used to serialize data or consumed by another codebase, +however, you must not delete and regenerate it or the original number assignments will be +lost. Commit it to version control alongside your schema definition; `schema_artifacts:check` +reports it as out of date when your schema has changed but the file has not been dumped, +so CI will catch a forgotten dump. + +The file is safe to hand-edit (e.g. when resolving a merge conflict), but it is strictly +validated: unknown keys, non-integer numbers, out-of-range numbers, and duplicate numbers +are all rejected at dump time rather than silently reassigning numbers. + +Alternatives inside generated interface and union `oneof` blocks use the same stable +message-field mappings, so adding or removing a concrete subtype does not renumber the +remaining alternatives. + +Removed fields and `oneof` alternatives remain in the sidecar. Their numbers are explicitly +reserved in `schema.proto`, with comments recording the prior names, while every generated +message includes a comment identifying its next field number. If a removed field or alternative +is restored under the same name, it reuses its original number and is no longer reserved. + +Both `schema.proto` and the sidecar use public GraphQL field names. Index field names, +including `name_in_index` overrides, are not part of the protobuf wire schema or its +stable-numbering state. + +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 claim the stored +`next_number`, and removed values keep their numbers reserved so they are never reused +(number `0` is always the generated `*_UNSPECIFIED` value). `schema.proto` explicitly reserves +each removed value number, includes a comment recording its prior name, and identifies the next +value number for each enum: + +```yaml +enums: + WidgetColor: + values: + RED: 1 + BLUE: 2 + next_number: 3 +``` diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion.rb index e48ad46e2..df5f14429 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion.rb @@ -11,5 +11,8 @@ module ElasticGraph module ProtoIngestion # The name of the generated Protocol Buffers schema file. PROTO_SCHEMA_FILE = "schema.proto" + + # The name of the generated proto field-number mapping file. + PROTO_FIELD_NUMBERS_FILE = "proto_field_numbers.yaml" end end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb new file mode 100644 index 000000000..b151162d2 --- /dev/null +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rb @@ -0,0 +1,363 @@ +# Copyright 2024 - 2026 Block, Inc. +# +# Use of this source code is governed by an MIT-style +# license that can be found in the LICENSE file or at +# https://opensource.org/licenses/MIT. +# +# frozen_string_literal: true + +require "elastic_graph/errors" +require "elastic_graph/support/from_yaml_file" +require "elastic_graph/support/json_schema/validator_factory" + +module ElasticGraph + module ProtoIngestion + module SchemaDefinition + # Registry of the protobuf field and enum value numbers assigned to an ElasticGraph schema. + # Parses and validates the numbers stored in the `proto_field_numbers.yaml` artifact, hands + # out the next available numbers for new fields and enum values, and serializes the updated + # mappings for the next artifact dump so that numbers stay stable over time. + class FieldNumberMappings + extend Support::FromYamlFile + + # Stored field numbers and allocation cursor for a single protobuf message. + # + # @!attribute [r] field_numbers_by_name + # @return [Hash] + # @!attribute [r] next_number + # @return [Integer] + MessageMapping = ::Data.define(:field_numbers_by_name, :next_number) + private_constant :MessageMapping + + # Stored value numbers and allocation cursor for a single protobuf enum. + # + # @!attribute [r] value_numbers_by_name + # @return [Hash] + # @!attribute [r] next_number + # @return [Integer] + EnumMapping = ::Data.define(:value_numbers_by_name, :next_number) + private_constant :EnumMapping + + # The largest field number protobuf allows (2^29 - 1), per + # https://protobuf.dev/programming-guides/proto3/#assigning. + MAX_FIELD_NUMBER = 536_870_911 + # Field numbers protobuf reserves for its own implementation; they may not be used as + # field tags, per https://protobuf.dev/programming-guides/proto3/#assigning. + RESERVED_FIELD_NUMBER_RANGE = 19_000..19_999 + # The largest enum value number protobuf allows (the int32 maximum), per + # https://protobuf.dev/programming-guides/proto3/#enum. + MAX_ENUM_VALUE_NUMBER = 2_147_483_647 + + # JSON schema for the `proto_field_numbers.yaml` artifact. + JSON_SCHEMA = { + "$schema" => "http://json-schema.org/draft-07/schema#", + "definitions" => { + "field_number" => { + "type" => "integer", + "minimum" => 1, + "maximum" => MAX_FIELD_NUMBER, + "not" => { + "minimum" => RESERVED_FIELD_NUMBER_RANGE.begin, + "maximum" => RESERVED_FIELD_NUMBER_RANGE.end + } + }, + "next_field_number" => { + "type" => "integer", + "minimum" => 1, + "maximum" => MAX_FIELD_NUMBER + 1, + "not" => { + "minimum" => RESERVED_FIELD_NUMBER_RANGE.begin, + "maximum" => RESERVED_FIELD_NUMBER_RANGE.end + } + }, + "enum_value_number" => { + "type" => "integer", + "minimum" => 1, + "maximum" => MAX_ENUM_VALUE_NUMBER + }, + "next_enum_value_number" => { + "type" => "integer", + "minimum" => 1, + "maximum" => MAX_ENUM_VALUE_NUMBER + 1 + } + }, + "type" => "object", + "properties" => { + "messages" => { + "type" => "object", + "additionalProperties" => { + "type" => "object", + "properties" => { + "fields" => { + "type" => "object", + "additionalProperties" => {"$ref" => "#/definitions/field_number"} + }, + "next_number" => {"$ref" => "#/definitions/next_field_number"} + }, + "required" => ["fields", "next_number"], + "additionalProperties" => false + } + }, + "enums" => { + "type" => "object", + "additionalProperties" => { + "type" => "object", + "properties" => { + "values" => { + "type" => "object", + "additionalProperties" => {"$ref" => "#/definitions/enum_value_number"} + }, + "next_number" => {"$ref" => "#/definitions/next_enum_value_number"} + }, + "required" => ["values", "next_number"], + "additionalProperties" => false + } + } + }, + "additionalProperties" => false + } + + VALIDATOR = Support::JSONSchema::Validator.new( + schema: Support::JSONSchema::ValidatorFactory.new( + schema: JSON_SCHEMA, + sanitize_pii: false + ).root_schema, + sanitize_pii: false + ) + private_constant :VALIDATOR + + # Builds an instance from parsed `proto_field_numbers.yaml`, validating its structure and + # every mapped number. + # + # @param parsed_yaml [Hash, nil] parsed contents of the artifact (or a hash in the same format) + # @return [FieldNumberMappings] + # @raise [Errors::SchemaError] if the mappings deviate from the artifact format or contain invalid numbers + def self.from_parsed_yaml(parsed_yaml) + parsed_yaml ||= {} # : ::Hash[::String, untyped] + if (validation_error = VALIDATOR.validate_with_error_message(parsed_yaml)) + raise Errors::SchemaError, "Invalid protobuf field-number mappings:\n\n#{validation_error}" + end + + empty_section = {} # : ::Hash[untyped, untyped] + + new( + message_mappings_by_name: parse_messages(parsed_yaml.fetch("messages", empty_section)), + enum_mappings_by_name: parse_enums(parsed_yaml.fetch("enums", empty_section)) + ) + end + + # @param message_mappings_by_name [Hash] validated message mappings + # @param enum_mappings_by_name [Hash] validated enum mappings + # @api private + def initialize(message_mappings_by_name:, enum_mappings_by_name:) + @message_mappings_by_name = message_mappings_by_name + @enum_mappings_by_name = enum_mappings_by_name + end + + # Returns the stable protobuf number for a message field, assigning the message's stored + # `next_number` if the field has no mapping. When the field was renamed, the mapping + # stored under one of its `previous_field_names` (and its number) carries over. + # + # @param message_name [String] + # @param public_field_name [String] + # @param previous_field_names [Array] old public names of the field, if renamed + # @return [Integer] + def field_number_for(message_name:, public_field_name:, previous_field_names:) + message_mapping = @message_mappings_by_name.fetch(message_name) do + MessageMapping.new(field_numbers_by_name: {}, next_number: 1) + end + field_numbers = message_mapping.field_numbers_by_name + + return field_numbers.fetch(public_field_name) if field_numbers.key?(public_field_name) + + old_field_name = previous_field_names.find { |field_name| field_numbers.key?(field_name) } + updated_mapping = + if old_field_name + message_mapping.with( + field_numbers_by_name: field_numbers + .except(old_field_name) + .merge(public_field_name => field_numbers.fetch(old_field_name)) + ) + else + allocate_field_number(message_name, public_field_name, message_mapping) + end + + @message_mappings_by_name = @message_mappings_by_name.merge(message_name => updated_mapping) + updated_mapping.field_numbers_by_name.fetch(public_field_name) + end + + # Returns the next field number that will be assigned for the given message. + # + # @param message_name [String] + # @return [Integer] + def next_field_number_for(message_name) + @message_mappings_by_name[message_name]&.next_number || 1 + end + + # Returns field names and numbers retained in the mappings but absent from the message. + # + # @param message_name [String] + # @param active_field_names [Array] + # @return [Hash] + def reserved_field_numbers_for(message_name, active_field_names) + field_numbers = @message_mappings_by_name[message_name]&.field_numbers_by_name || {} + reserved_numbers_by_name(field_numbers, active_field_names) + end + + # Returns the stable protobuf numbers for an enum's values, assigning the next available + # numbers to values that have no stored mapping. + # + # @param enum_name [String] + # @param value_names [Array] + # @return [Hash] + def enum_value_numbers_for(enum_name, value_names) + enum_mapping = @enum_mappings_by_name.fetch(enum_name) do + EnumMapping.new(value_numbers_by_name: {}, next_number: 1) + end + value_numbers = enum_mapping.value_numbers_by_name + new_value_names = value_names - value_numbers.keys + first_new_number = enum_mapping.next_number + last_new_number = first_new_number + new_value_names.size - 1 + + if new_value_names.any? && last_new_number > MAX_ENUM_VALUE_NUMBER + raise Errors::SchemaError, "Cannot allocate another protobuf enum value number for enum `#{enum_name}`: " \ + "the maximum enum value number (#{MAX_ENUM_VALUE_NUMBER}) has been reached." + end + + new_value_numbers = new_value_names.each_with_index.to_h do |value_name, index| + [value_name, first_new_number + index] + end + updated_value_numbers = value_numbers.merge(new_value_numbers) + updated_mapping = enum_mapping.with( + value_numbers_by_name: updated_value_numbers, + next_number: last_new_number + 1 + ) + @enum_mappings_by_name = @enum_mappings_by_name.merge(enum_name => updated_mapping) + + value_names.to_h do |value_name| + [value_name, updated_value_numbers.fetch(value_name)] + end + end + + # Returns the next value number that will be assigned for the given enum. + # + # @param enum_name [String] + # @return [Integer] + def next_enum_value_number_for(enum_name) + @enum_mappings_by_name[enum_name]&.next_number || 1 + end + + # Returns value names and numbers retained in the mappings but absent from the enum. + # + # @param enum_name [String] + # @param active_value_names [Array] + # @return [Hash] + def reserved_enum_value_numbers_for(enum_name, active_value_names) + value_numbers = @enum_mappings_by_name[enum_name]&.value_numbers_by_name || {} + reserved_numbers_by_name(value_numbers, active_value_names) + end + + # Serializes the mappings back to the `proto_field_numbers.yaml` artifact format, with + # messages and enums sorted by name and their fields and values sorted by number. + # + # @return [Hash] + def to_dumpable_hash + { + "messages" => @message_mappings_by_name + .sort_by(&:first) + .to_h do |message_name, message_mapping| + [message_name, { + "fields" => message_mapping.field_numbers_by_name.sort_by { |field_name, number| [number, field_name] }.to_h, + "next_number" => message_mapping.next_number + }] + end, + "enums" => @enum_mappings_by_name + .sort_by(&:first) + .to_h do |enum_name, enum_mapping| + [enum_name, { + "values" => enum_mapping.value_numbers_by_name.sort_by { |value_name, number| [number, value_name] }.to_h, + "next_number" => enum_mapping.next_number + }] + end + } + end + + private + + def reserved_numbers_by_name(numbers_by_name, active_names) + numbers_by_name + .except(*active_names) + .sort_by { |name, number| [number, name] } + .to_h + end + + # Returns a message mapping with the stored allocation cursor assigned to `field_name` and + # advanced past the claimed number and protobuf's reserved 19000..19999 range. + def allocate_field_number(message_name, field_name, message_mapping) + field_number = message_mapping.next_number + if field_number > MAX_FIELD_NUMBER + raise Errors::SchemaError, "Cannot allocate another protobuf field number for message `#{message_name}`: " \ + "the maximum field number (#{MAX_FIELD_NUMBER}) has been reached." + end + + next_number = field_number + 1 + if RESERVED_FIELD_NUMBER_RANGE.cover?(next_number) + next_number = RESERVED_FIELD_NUMBER_RANGE.end + 1 + end + + message_mapping.with( + field_numbers_by_name: message_mapping.field_numbers_by_name.merge(field_name => field_number), + next_number: next_number + ) + end + + private_class_method def self.parse_messages(messages_section) + messages_section.to_h do |message_name, message_entry| + fields = message_entry.fetch("fields") # : ::Hash[::String, ::Integer] + + verify_no_number_collisions( + fields, + "field-number mapping collision in message `#{message_name}`" + ) + + next_number = message_entry.fetch("next_number") # : ::Integer + + verify_next_number_is_after_mapped_numbers("message `#{message_name}`", next_number, fields) + [message_name, MessageMapping.new(field_numbers_by_name: fields, next_number: next_number)] + end + end + + private_class_method def self.parse_enums(enums_section) + enums_section.to_h do |enum_name, enum_entry| + values = enum_entry.fetch("values") # : ::Hash[::String, ::Integer] + verify_no_number_collisions(values, "enum value-number mapping collision in enum `#{enum_name}`") + + next_number = enum_entry.fetch("next_number") # : ::Integer + verify_next_number_is_after_mapped_numbers("enum `#{enum_name}`", next_number, values) + [enum_name, EnumMapping.new(value_numbers_by_name: values, next_number: next_number)] + end + end + + private_class_method def self.verify_next_number_is_after_mapped_numbers(mapping_description, next_number, numbers) + max_number = numbers.values.max + if max_number && next_number <= max_number + raise Errors::SchemaError, "Protobuf `next_number` for #{mapping_description} must be greater than " \ + "every mapped number (maximum: #{max_number}), got: #{next_number}." + end + + nil + end + + private_class_method def self.verify_no_number_collisions(numbers_by_name, collision_description) + numbers_by_name.group_by(&:last).each_value do |entries| + next if entries.size < 2 + + names = entries.map(&:first).sort.map { |name| "`#{name}`" }.join(" and ") + raise Errors::SchemaError, "Protobuf #{collision_description}: " \ + "#{names} are both mapped to number #{entries.first.last}." + end + end + end + end + end +end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rb index 22d33d749..207d7d35f 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rb @@ -12,7 +12,7 @@ module SchemaDefinition # Holds the proto ingestion extension's schema definition state. # # @private - class ProtoIngestionState < ::Struct.new(:package_name) + class ProtoIngestionState < ::Struct.new(:package_name, :field_number_mappings) end end end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/results_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/results_extension.rb index 50fa81213..b2bd60084 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/results_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/results_extension.rb @@ -20,19 +20,31 @@ def proto_schema @proto_schema ||= protobuf_schema_generator.to_proto end + # Returns proto field-number mappings suitable for artifact storage. + # + # @return [Hash] + def proto_field_number_mappings + # Numbers get assigned as `schema.proto` renders, so we must render before reading them. + proto_schema + protobuf_schema_generator.field_number_mappings_for_artifact + end + private def protobuf_schema_generator - # The cast is needed because Steep can't see the `extend(StateExtension)` applied at - # runtime in {APIExtension.extended}. - extension_state = state # : ElasticGraph::SchemaDefinition::State & StateExtension - ingestion_state = extension_state.proto_ingestion_state + @protobuf_schema_generator ||= begin + # The cast is needed because Steep can't see the `extend(StateExtension)` applied at + # runtime in {APIExtension.extended}. + extension_state = state # : ElasticGraph::SchemaDefinition::State & StateExtension + ingestion_state = extension_state.proto_ingestion_state - Schema.new( - state: extension_state, - all_types: all_types, - package_name: ingestion_state.package_name - ) + Schema.new( + state: extension_state, + all_types: all_types, + package_name: ingestion_state.package_name, + proto_field_number_mappings: ingestion_state.field_number_mappings + ) + end end end end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb index 2458b982a..baf636fc4 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb @@ -7,6 +7,7 @@ # frozen_string_literal: true require "elastic_graph/errors" +require "elastic_graph/proto_ingestion/schema_definition/field_number_mappings" require "elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension" require "elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension" require "elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension" @@ -19,10 +20,17 @@ class Schema # @param state [ElasticGraph::SchemaDefinition::State] # @param all_types [Array] # @param package_name [String] - def initialize(state:, all_types:, package_name:) + # @param proto_field_number_mappings [Hash, nil] mappings in the `proto_field_numbers.yaml` artifact format + def initialize( + state:, + all_types:, + package_name:, + proto_field_number_mappings: {} + ) @state = state @all_types = all_types @package_name = package_name + @field_number_mappings = FieldNumberMappings.from_parsed_yaml(proto_field_number_mappings) end # Renders the schema as a valid `proto3` file. @@ -43,6 +51,59 @@ def to_proto sections.join("\n\n") + "\n" end + # Exposes the field-number and enum-value-number mappings for writing to artifact YAML. + # + # @return [Hash] + def field_number_mappings_for_artifact + @field_number_mappings.to_dumpable_hash + end + + # Returns the stable protobuf number for a message field. + # + # @api private + def field_number_for(message_name:, type_name:, public_field_name:) + @field_number_mappings.field_number_for( + message_name: message_name, + public_field_name: public_field_name, + previous_field_names: previous_field_names_for(type_name, public_field_name) + ) + end + + # Returns the next protobuf field number that will be assigned for a message. + # + # @api private + def next_field_number_for(message_name) + @field_number_mappings.next_field_number_for(message_name) + end + + # Returns field names and numbers that must be reserved in a protobuf message. + # + # @api private + def reserved_field_numbers_for(message_name, active_field_names) + @field_number_mappings.reserved_field_numbers_for(message_name, active_field_names) + end + + # Returns the stable protobuf numbers for an enum's values. + # + # @api private + def enum_value_numbers_for(enum_name, value_names) + @field_number_mappings.enum_value_numbers_for(enum_name, value_names) + end + + # Returns the next protobuf value number that will be assigned for an enum. + # + # @api private + def next_enum_value_number_for(enum_name) + @field_number_mappings.next_enum_value_number_for(enum_name) + end + + # Returns value names and numbers that must be reserved in a protobuf enum. + # + # @api private + def reserved_enum_value_numbers_for(enum_name, active_value_names) + @field_number_mappings.reserved_enum_value_numbers_for(enum_name, active_value_names) + end + private # Selects the indexed root types and every type transitively referenced by their protobuf @@ -65,7 +126,7 @@ def proto_types def render_definitions(types) types .sort_by(&:proto_name) - .filter_map { |type| type.to_proto(@package_name) } + .filter_map { |type| type.to_proto(self, @package_name) } .join("\n\n") end @@ -81,6 +142,20 @@ def validate_unique_enum_value_prefixes(types) enum_type_by_prefix[type.proto_enum_value_prefix] = type end end + + def previous_field_names_for(type_name, public_field_name) + previous_field_names_by_type_name_and_field_name.dig(type_name, public_field_name) || [] + end + + # Inverts the state's `old_field_name => renamed_field` index into the form we need here: + # the old public names a field's current public name was renamed from. + def previous_field_names_by_type_name_and_field_name + @previous_field_names_by_type_name_and_field_name ||= @state.renamed_fields_by_type_name_and_old_field_name.transform_values do |old_to_new| + old_to_new + .group_by { |_, renamed_field| renamed_field.name } + .transform_values { |renames| renames.map(&:first) } + end + end end end end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rb index 352c2df84..c78c88268 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rb @@ -6,6 +6,7 @@ # # frozen_string_literal: true +require "elastic_graph/errors" require "elastic_graph/proto_ingestion" module ElasticGraph @@ -16,6 +17,20 @@ module SchemaDefinition # # @private module SchemaArtifactManagerExtension + PROTO_FIELD_NUMBERS_COMMENT_PREAMBLE_LINES = [ + "This file is part of your schema definition--not a regenerable schema artifact. It is an", + "input to `schema.proto` generation: ElasticGraph reads it to keep protobuf field and enum", + "value numbers stable as your schema evolves, and `rake schema_artifacts:dump` maintains it", + "for you.", + "", + "You may update it by hand (e.g. to assign a specific number). While prototyping, you may", + "delete this file and regenerate it to reset the number assignments.", + "", + "Once generated protos have been used to serialize data or consumed by another codebase,", + "you must NOT delete this file and regenerate it. The original number assignments would be", + "lost, and previously serialized protobuf messages would be misread." + ].freeze + private # Overrides the base `artifacts_from_schema_def` method to add proto artifacts. @@ -25,16 +40,37 @@ def artifacts_from_schema_def return base_artifacts if proto_schema.empty? base_artifacts + [ + proto_field_numbers_artifact, new_raw_artifact(PROTO_SCHEMA_FILE, proto_schema.chomp, comment_prefix: "//") ] end + # Builds the `proto_field_numbers.yaml` artifact. The file is part of the schema definition + # rather than a proper schema artifact--it's an input to `schema.proto` generation--so it + # lives alongside `path_to_schema` instead of in the schema artifacts directory. + def proto_field_numbers_artifact + new_yaml_artifact(PROTO_FIELD_NUMBERS_FILE, protobuf_schema_definition_results.proto_field_number_mappings) + .with( + file_name: proto_field_numbers_path, + comment_preamble_lines: PROTO_FIELD_NUMBERS_COMMENT_PREAMBLE_LINES + ) + end + + def proto_field_numbers_path + proto_ingestion_schema_definition_state.proto_field_numbers_path || + raise(Errors::SchemaError, "Cannot dump `#{PROTO_FIELD_NUMBERS_FILE}` without a configured `path_to_schema`.") + end + # Returns the wrapped {ElasticGraph::SchemaDefinition::Results} narrowed to include this # gem's `ResultsExtension`. Centralizes the Steep cast that's needed because Steep can't # see the `extend(ResultsExtension)` applied at runtime. def protobuf_schema_definition_results schema_definition_results # : ElasticGraph::SchemaDefinition::Results & ResultsExtension end + + def proto_ingestion_schema_definition_state + protobuf_schema_definition_results.state # : ElasticGraph::SchemaDefinition::State & StateExtension + end end end end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rb index da0884fd5..199e8eaec 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rb @@ -42,8 +42,8 @@ def value(value_name) # Renders this enum's protobuf definition. # # @return [String] - def to_proto(_package_name) - render_proto_enum + def to_proto(schema, _package_name) + render_proto_enum(schema) end # Returns the schema types referenced by this definition. @@ -83,13 +83,19 @@ 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) + value.to_proto(value_numbers.fetch(value.name), proto_enum_value_prefix: proto_enum_value_prefix) + end) + schema.reserved_enum_value_numbers_for(proto_name, values_by_name.keys).each do |value_name, value_number| + value_definitions << " reserved #{value_number}; // Previously used by #{value_name}." end + value_definitions << " // Next value number: #{schema.next_enum_value_number_for(proto_name)}" <<~PROTO.chomp #{documentation}enum #{proto_name} { diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb index d7267f332..30dbfc879 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb @@ -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 + active_field_names = fields.map { |schema_field, _| schema_field.name } documentation = ProtoDocumentation.comment_lines_for(doc_comment).map { |line| "#{line}\n" }.join - field_definitions = fields.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 + ) 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" } @@ -88,6 +94,10 @@ def render_proto_message(message_name, package_name) "#{field_documentation}#{line}" end + schema.reserved_field_numbers_for(message_name, active_field_names).each do |field_name, field_number| + field_definitions << " reserved #{field_number}; // Previously used by #{field_name}." + end + field_definitions << " // Next field number: #{schema.next_field_number_for(message_name)}" <<~PROTO.chomp #{documentation}message #{message_name} { @@ -96,21 +106,31 @@ 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| + active_field_names = [] # : ::Array[::String] + alternatives = abstract_type.recursively_resolve_subtypes.map do |subtype| proto_subtype = _ = subtype field_name = Support::Casing.to_upper_snake(proto_subtype.proto_name).downcase + active_field_names << field_name + field_number = schema.field_number_for( + message_name: message_name, + type_name: name, + public_field_name: field_name + ) " #{proto_subtype.proto_type_reference(package_name)} #{field_name} = #{field_number};" end + body_lines = [" oneof value {", *alternatives, " }"] + schema.reserved_field_numbers_for(message_name, active_field_names).each do |field_name, field_number| + body_lines << " reserved #{field_number}; // Previously used by #{field_name}." + end + body_lines << " // Next field number: #{schema.next_field_number_for(message_name)}" <<~PROTO.chomp #{documentation}message #{message_name} { - oneof value { - #{alternatives.join("\n")} - } + #{body_lines.join("\n")} } PROTO end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rb index f3dfaf898..404258788 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rb @@ -64,7 +64,7 @@ def initialize_proto_extension # Scalars map to protobuf field types and do not render standalone definitions. # # @return [nil] - def to_proto(_package_name) + def to_proto(_schema, _package_name) nil end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/state_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/state_extension.rb index f59e96343..d5061e85e 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/state_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/state_extension.rb @@ -6,6 +6,8 @@ # # frozen_string_literal: true +require "elastic_graph/proto_ingestion" +require "elastic_graph/proto_ingestion/schema_definition/field_number_mappings" require "elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state" module ElasticGraph @@ -19,11 +21,26 @@ module StateExtension attr_reader :proto_ingestion_state def self.extended(state) + field_number_mappings = + if (path = state.proto_field_numbers_path) && ::File.exist?(path) + FieldNumberMappings.from_yaml_file(path).to_dumpable_hash + else + {} # : ::Hash[::String, untyped] + end + state.instance_variable_set( :@proto_ingestion_state, - ProtoIngestionState.new(package_name: "elasticgraph") + ProtoIngestionState.new(package_name: "elasticgraph", field_number_mappings: field_number_mappings) ) end + + def proto_field_numbers_path + return unless (path = path_to_schema) + + # The `./` prefix is dropped so that a schema definition at the root yields a path that + # reads like the other artifact paths when the rake tasks report on it. + ::File.join(::File.dirname(path), PROTO_FIELD_NUMBERS_FILE).delete_prefix("./") + end end end end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion.rbs index 7cb944875..81cc74611 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion.rbs @@ -1,5 +1,6 @@ module ElasticGraph module ProtoIngestion PROTO_SCHEMA_FILE: ::String + PROTO_FIELD_NUMBERS_FILE: ::String end end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs new file mode 100644 index 000000000..92471ced6 --- /dev/null +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/field_number_mappings.rbs @@ -0,0 +1,64 @@ +module ElasticGraph + module ProtoIngestion + module SchemaDefinition + class FieldNumberMappings + extend Support::FromYamlFile[FieldNumberMappings] + + type fieldNumbersByName = ::Hash[::String, ::Integer] + + class MessageMapping + attr_reader field_numbers_by_name: fieldNumbersByName + attr_reader next_number: ::Integer + + def self.new: (field_numbers_by_name: fieldNumbersByName, next_number: ::Integer) -> instance + def with: (?field_numbers_by_name: fieldNumbersByName, ?next_number: ::Integer) -> instance + end + + class EnumMapping + attr_reader value_numbers_by_name: fieldNumbersByName + attr_reader next_number: ::Integer + + def self.new: (value_numbers_by_name: fieldNumbersByName, next_number: ::Integer) -> instance + def with: (?value_numbers_by_name: fieldNumbersByName, ?next_number: ::Integer) -> instance + end + + MAX_FIELD_NUMBER: ::Integer + RESERVED_FIELD_NUMBER_RANGE: ::Range[::Integer] + MAX_ENUM_VALUE_NUMBER: ::Integer + JSON_SCHEMA: ::Hash[::String, untyped] + VALIDATOR: Support::JSONSchema::Validator + + @message_mappings_by_name: ::Hash[::String, MessageMapping] + @enum_mappings_by_name: ::Hash[::String, EnumMapping] + + def self.from_parsed_yaml: (untyped) ?{ (untyped) -> void } -> instance + def self.parse_messages: (untyped) -> ::Hash[::String, MessageMapping] + def self.parse_enums: (untyped) -> ::Hash[::String, EnumMapping] + def self.verify_next_number_is_after_mapped_numbers: (::String, ::Integer, fieldNumbersByName) -> void + def self.verify_no_number_collisions: (::Hash[::String, ::Integer], ::String) -> void + + def initialize: ( + message_mappings_by_name: ::Hash[::String, MessageMapping], + enum_mappings_by_name: ::Hash[::String, EnumMapping] + ) -> void + + def field_number_for: ( + message_name: ::String, + public_field_name: ::String, + previous_field_names: ::Array[::String] + ) -> ::Integer + def next_field_number_for: (::String) -> ::Integer + def reserved_field_numbers_for: (::String, ::Array[::String]) -> fieldNumbersByName + def enum_value_numbers_for: (::String, ::Array[::String]) -> ::Hash[::String, ::Integer] + def next_enum_value_number_for: (::String) -> ::Integer + def reserved_enum_value_numbers_for: (::String, ::Array[::String]) -> ::Hash[::String, ::Integer] + def to_dumpable_hash: () -> ::Hash[::String, untyped] + + private + + def reserved_numbers_by_name: (fieldNumbersByName, ::Array[::String]) -> fieldNumbersByName + def allocate_field_number: (::String, ::String, MessageMapping) -> MessageMapping + end + end + end +end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rbs index c66c5d97d..377ead2f9 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/proto_ingestion_state.rbs @@ -3,10 +3,9 @@ module ElasticGraph module SchemaDefinition class ProtoIngestionStateSupertype attr_accessor package_name: ::String + attr_accessor field_number_mappings: untyped - def initialize: ( - package_name: ::String - ) -> void + def initialize: (package_name: ::String, field_number_mappings: untyped) -> void end class ProtoIngestionState < ProtoIngestionStateSupertype diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/results_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/results_extension.rbs index 96de3ae03..1d769a23e 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/results_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/results_extension.rbs @@ -3,12 +3,14 @@ module ElasticGraph module SchemaDefinition module ResultsExtension : ::ElasticGraph::SchemaDefinition::Results def proto_schema: () -> ::String + def proto_field_number_mappings: () -> ::Hash[::String, untyped] private def protobuf_schema_generator: () -> Schema @proto_schema: ::String? + @protobuf_schema_generator: Schema? end end end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs index ce7a1cdf7..9c8226f97 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema.rbs @@ -5,20 +5,36 @@ module ElasticGraph @state: ::ElasticGraph::SchemaDefinition::State @all_types: ::Array[::ElasticGraph::SchemaDefinition::SchemaElements::graphQLType] @package_name: ::String + @field_number_mappings: FieldNumberMappings + @previous_field_names_by_type_name_and_field_name: ::Hash[::String, ::Hash[::String, ::Array[::String]]]? def initialize: ( state: ::ElasticGraph::SchemaDefinition::State, all_types: ::Array[::ElasticGraph::SchemaDefinition::SchemaElements::graphQLType], - package_name: ::String + package_name: ::String, + ?proto_field_number_mappings: untyped ) -> void def to_proto: () -> ::String + def field_number_mappings_for_artifact: () -> ::Hash[::String, untyped] + def field_number_for: ( + message_name: ::String, + type_name: ::String, + public_field_name: ::String + ) -> ::Integer + def next_field_number_for: (::String) -> ::Integer + def reserved_field_numbers_for: (::String, ::Array[::String]) -> ::Hash[::String, ::Integer] + def enum_value_numbers_for: (::String, ::Array[::String]) -> ::Hash[::String, ::Integer] + def next_enum_value_number_for: (::String) -> ::Integer + def reserved_enum_value_numbers_for: (::String, ::Array[::String]) -> ::Hash[::String, ::Integer] private def proto_types: () -> ::Array[untyped] def render_definitions: (::Array[untyped] types) -> ::String def validate_unique_enum_value_prefixes: (::Array[untyped] types) -> void + def previous_field_names_for: (::String, ::String) -> ::Array[::String] + def previous_field_names_by_type_name_and_field_name: () -> ::Hash[::String, ::Hash[::String, ::Array[::String]]] end end end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rbs index 4298c1a99..120aa8962 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension.rbs @@ -2,10 +2,15 @@ module ElasticGraph module ProtoIngestion module SchemaDefinition module SchemaArtifactManagerExtension : ::ElasticGraph::SchemaDefinition::SchemaArtifactManager + PROTO_FIELD_NUMBERS_COMMENT_PREAMBLE_LINES: ::Array[::String] + private def artifacts_from_schema_def: () -> ::Array[::ElasticGraph::SchemaDefinition::SchemaArtifact[untyped]] + def proto_field_numbers_artifact: () -> ::ElasticGraph::SchemaDefinition::SchemaArtifact[::Hash[::String, untyped]] + def proto_field_numbers_path: () -> ::String def protobuf_schema_definition_results: () -> (::ElasticGraph::SchemaDefinition::Results & ResultsExtension) + def proto_ingestion_schema_definition_state: () -> (::ElasticGraph::SchemaDefinition::State & StateExtension) end end end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rbs index 4987ddf7e..6652855ba 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rbs @@ -12,13 +12,13 @@ module ElasticGraph def proto_name: () -> ::String def proto_type_reference: (::String package_name) -> ::String def proto_enum_value_prefix: () -> ::String - def to_proto: (::String package_name) -> ::String + def to_proto: (Schema schema, ::String package_name) -> ::String def referenced_proto_types: () -> ::Array[::ElasticGraph::SchemaDefinition::SchemaElements::graphQLType] def configure_derived_scalar_type: (::ElasticGraph::SchemaDefinition::SchemaElements::ScalarType) -> void private - def render_proto_enum: () -> ::String + def render_proto_enum: (Schema schema) -> ::String def proto_zero_value: () -> (::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & EnumValueExtension) def proto_zero_value_name: () -> ::String def values_by_proto_name: () -> ::Hash[::String, ::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & EnumValueExtension] diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rbs index 4265583ee..608f27a7d 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rbs @@ -10,7 +10,7 @@ module ElasticGraph def proto_name: () -> ::String def proto_type_reference: (::String package_name) -> ::String - def to_proto: (::String package_name) -> ::String + def to_proto: (Schema schema, ::String package_name) -> ::String def referenced_proto_types: () -> ::Array[::ElasticGraph::SchemaDefinition::SchemaElements::graphQLType] def self.list_depth_and_base_type: ( ::ElasticGraph::SchemaDefinition::SchemaElements::TypeReference @@ -18,8 +18,8 @@ module ElasticGraph private - def render_proto_message: (::String message_name, ::String package_name) -> ::String - def render_proto_oneof: (::String message_name, ::String package_name) -> ::String + def render_proto_message: (Schema schema, ::String message_name, ::String package_name) -> ::String + def render_proto_oneof: (Schema schema, ::String message_name, ::String package_name) -> ::String def proto_fields: () -> ::Array[[ ::ElasticGraph::SchemaDefinition::SchemaElements::Field, ::ElasticGraph::SchemaDefinition::Indexing::Field diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rbs index f5032bd25..94f0dcf15 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rbs @@ -11,7 +11,7 @@ module ElasticGraph def initialize_proto_extension: () { () -> void } -> void def proto_name: () -> ::String def proto_type_reference: (::String package_name) -> ::String - def to_proto: (::String package_name) -> nil + def to_proto: (Schema schema, ::String package_name) -> nil def referenced_proto_types: () -> ::Array[::ElasticGraph::SchemaDefinition::SchemaElements::graphQLType] end end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/state_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/state_extension.rbs index 3b29bc557..60dca722f 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/state_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/state_extension.rbs @@ -5,6 +5,7 @@ module ElasticGraph attr_reader proto_ingestion_state: ProtoIngestionState def self.extended: (::ElasticGraph::SchemaDefinition::State & StateExtension) -> void + def proto_field_numbers_path: () -> ::String? end end end diff --git a/elasticgraph-proto_ingestion/spec/integration/elastic_graph/proto_ingestion/schema_definition/rake_tasks_spec.rb b/elasticgraph-proto_ingestion/spec/integration/elastic_graph/proto_ingestion/schema_definition/rake_tasks_spec.rb index 66fae144e..fa686a45a 100644 --- a/elasticgraph-proto_ingestion/spec/integration/elastic_graph/proto_ingestion/schema_definition/rake_tasks_spec.rb +++ b/elasticgraph-proto_ingestion/spec/integration/elastic_graph/proto_ingestion/schema_definition/rake_tasks_spec.rb @@ -9,6 +9,7 @@ require "elastic_graph/proto_ingestion" require "elastic_graph/proto_ingestion/schema_definition/api_extension" require "elastic_graph/schema_definition/rake_tasks" +require "yaml" module ElasticGraph module ProtoIngestion @@ -50,7 +51,71 @@ module SchemaDefinition expect { output = run_rake_with_proto("schema_artifacts:dump") expect(output.lines).to include(a_string_including("already up to date", PROTO_SCHEMA_FILE)) - }.to maintain { read_artifact(PROTO_SCHEMA_FILE) } + }.to maintain { read_artifact(PROTO_SCHEMA_FILE) }.and maintain { read_proto_field_numbers } + end + + it "persists proto field-number mappings and reuses them on the next dump" do + write_proto_schema(table_defs: <<~EOS) + s.object_type "Product" do |t| + t.field "id", "ID" + t.field "name", "String" + t.index "products" + end + EOS + + expect { + run_rake_with_proto("schema_artifacts:dump") + }.to change { read_proto_field_numbers } + .from(nil) + .to(a_string_starting_with("# This file is part of your schema definition")) + + # The file is maintained alongside the schema definition, not in the schema artifacts directory. + expect(read_artifact(PROTO_FIELD_NUMBERS_FILE)).to be_nil + expect(parsed_proto_field_numbers).to eq({ + "enums" => {}, + "messages" => { + "Product" => { + "fields" => { + "id" => 1, + "name" => 2 + }, + "next_number" => 3 + } + } + }) + + write_proto_schema(table_defs: <<~EOS) + s.object_type "Product" do |t| + t.field "name", "String" + t.field "id", "ID" + t.index "products" + end + EOS + + run_rake_with_proto("schema_artifacts:dump") + + expect(read_artifact(PROTO_SCHEMA_FILE)).to include("string id = 1;", "string name = 2;") + end + end + + describe "schema_artifacts:check" do + it "reports `#{PROTO_FIELD_NUMBERS_FILE}` until it has been dumped alongside the schema definition" do + write_proto_schema(table_defs: <<~EOS) + s.object_type "Product" do |t| + t.field "id", "ID" + t.index "products" + end + EOS + + # The leading space verifies the file is reported at the root (alongside the schema + # definition) rather than nested in the schema artifacts directory. + expect { + run_rake_with_proto("schema_artifacts:check") + }.to abort_with a_string_including(" #{PROTO_FIELD_NUMBERS_FILE} (file does not exist)") + + run_rake_with_proto("schema_artifacts:dump") + + expect(run_rake_with_proto("schema_artifacts:check")).to include(PROTO_FIELD_NUMBERS_FILE, "up to date") end end @@ -81,6 +146,15 @@ def read_artifact(name) path = File.join("config", "schema", "artifacts", name) File.read(path) if File.exist?(path) end + + # The field-numbers file is dumped as a sibling of `path_to_schema` (`schema.rb`, above). + def read_proto_field_numbers + File.read(PROTO_FIELD_NUMBERS_FILE) if File.exist?(PROTO_FIELD_NUMBERS_FILE) + end + + def parsed_proto_field_numbers + ::YAML.safe_load(read_proto_field_numbers) + end end end end diff --git a/elasticgraph-proto_ingestion/spec/support/proto_schema_support.rb b/elasticgraph-proto_ingestion/spec/support/proto_schema_support.rb index 20aa71487..81a2dc80d 100644 --- a/elasticgraph-proto_ingestion/spec/support/proto_schema_support.rb +++ b/elasticgraph-proto_ingestion/spec/support/proto_schema_support.rb @@ -18,13 +18,25 @@ def define_proto_schema(**options, &block) define_proto_schema_results(**options, &block).proto_schema end - def define_proto_schema_results(**options, &block) + # Defines a schema and returns its `Results`. Pass the results of a previous + # `define_proto_schema_results` call as `prior_results` to seed the new schema with the + # field-number mappings the previous one generated, standing in for the + # `proto_field_numbers.yaml` file that `schema_artifacts:dump` would have written between + # the two schema definitions. (Loading that file is covered by + # `schema_artifact_manager_extension_spec` and `rake_tasks_spec`.) `prior_results` is the + # standard way to test mapping behavior; pass raw `proto_field_number_mappings:` only for + # scenarios a prior dump cannot produce (such as a hand-edited or invalid file). + def define_proto_schema_results(prior_results = nil, proto_field_number_mappings: nil, **options, &block) + mappings = proto_field_number_mappings || prior_results&.proto_field_number_mappings + define_schema( schema_element_name_form: :snake_case, extension_modules: [SchemaDefinition::APIExtension], - **options, - &block - ) + **options + ) do |schema| + schema.state.proto_ingestion_state.field_number_mappings = mappings if mappings + block.call(schema) + end end def proto_type_def_from(proto, type) diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb new file mode 100644 index 000000000..318159090 --- /dev/null +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/field_number_mappings_spec.rb @@ -0,0 +1,236 @@ +# Copyright 2024 - 2026 Block, Inc. +# +# Use of this source code is governed by an MIT-style +# license that can be found in the LICENSE file or at +# https://opensource.org/licenses/MIT. +# +# frozen_string_literal: true + +require "elastic_graph/proto_ingestion/schema_definition/field_number_mappings" +require "elastic_graph/support/json_schema/meta_schema_validator" + +module ElasticGraph + module ProtoIngestion + module SchemaDefinition + RSpec.describe FieldNumberMappings do + describe ".from_parsed_yaml" do + it "returns empty mappings for `nil`, as parsing an empty artifact file yields" do + mappings = FieldNumberMappings.from_parsed_yaml(nil) + + expect(mappings.to_dumpable_hash).to eq({"enums" => {}, "messages" => {}}) + end + + describe "artifact structure validation" do + it "validates mappings against a valid JSON schema" do + expect(Support::JSONSchema.strict_meta_schema_validator.valid?(FieldNumberMappings::JSON_SCHEMA)).to be(true) + + expect { + FieldNumberMappings.from_parsed_yaml({ + "enums" => {"Status" => {"values" => {"ACTIVE" => 1}}} + }) + }.to raise_error(Errors::SchemaError, a_string_including( + "Invalid protobuf field-number mappings", "Validation errors" + )) + end + end + + describe "mapping consistency validation" do + it "raises clear errors when fields or enum values collide" do + expect { + FieldNumberMappings.from_parsed_yaml({ + "messages" => {"Account" => {"fields" => {"id" => 1, "name" => 1}, "next_number" => 2}} + }) + }.to raise_error(Errors::SchemaError, a_string_including( + "field-number mapping collision in message `Account`", "`id` and `name`", "number 1" + )) + + expect { + FieldNumberMappings.from_parsed_yaml({ + "enums" => {"Status" => {"values" => {"ACTIVE" => 1, "INACTIVE" => 1}, "next_number" => 2}} + }) + }.to raise_error(Errors::SchemaError, a_string_including( + "enum value-number mapping collision in enum `Status`", "`ACTIVE` and `INACTIVE`", "number 1" + )) + end + + it "validates that each `next_number` is greater than every mapped number" do + expect { + FieldNumberMappings.from_parsed_yaml({ + "messages" => {"Account" => {"fields" => {"id" => 7}, "next_number" => 7}} + }) + }.to raise_error(Errors::SchemaError, a_string_including( + "`next_number` for message `Account`", "greater than every mapped number", "maximum: 7", "got: 7" + )) + + expect { + FieldNumberMappings.from_parsed_yaml({ + "enums" => {"Status" => {"values" => {"ACTIVE" => 7}, "next_number" => 7}} + }) + }.to raise_error(Errors::SchemaError, a_string_including( + "`next_number` for enum `Status`", "greater than every mapped number", "maximum: 7", "got: 7" + )) + end + end + + describe "protobuf number boundaries" do + it "accepts maximum numbers while allowing enum values in the field-reserved range" do + artifact = { + "messages" => {"Account" => { + "fields" => {"id" => FieldNumberMappings::MAX_FIELD_NUMBER}, + "next_number" => FieldNumberMappings::MAX_FIELD_NUMBER + 1 + }}, + # Enum value numbers have no protobuf-reserved range, so 19000-19999 is fine here. + "enums" => {"Status" => {"values" => { + "ACTIVE" => FieldNumberMappings::MAX_ENUM_VALUE_NUMBER, + "INACTIVE" => 19_005 + }, "next_number" => FieldNumberMappings::MAX_ENUM_VALUE_NUMBER + 1}} + } + + expect(FieldNumberMappings.from_parsed_yaml(artifact).to_dumpable_hash).to eq(artifact) + end + end + end + + describe ".from_yaml_file" do + it "loads mappings through `FromYamlFile`", :in_temp_dir do + ::File.write("proto_field_numbers.yaml", <<~YAML) + messages: + Account: + fields: + id: 7 + next_number: 8 + enums: + Status: + values: + ACTIVE: 3 + next_number: 8 + YAML + + mappings = FieldNumberMappings.from_yaml_file("proto_field_numbers.yaml") + + expect(mappings.to_dumpable_hash).to eq({ + "messages" => {"Account" => {"fields" => {"id" => 7}, "next_number" => 8}}, + "enums" => {"Status" => {"values" => {"ACTIVE" => 3}, "next_number" => 8}} + }) + end + end + + describe "#field_number_for" do + it "raises a clear error when the field-number range has been exhausted" do + mappings = FieldNumberMappings.from_parsed_yaml({ + "messages" => {"Account" => { + "fields" => {"id" => FieldNumberMappings::MAX_FIELD_NUMBER}, + "next_number" => FieldNumberMappings::MAX_FIELD_NUMBER + 1 + }} + }) + + expect { + mappings.field_number_for(message_name: "Account", public_field_name: "name", previous_field_names: []) + }.to raise_error(Errors::SchemaError, a_string_including( + "Cannot allocate another protobuf field number for message `Account`", + "maximum field number (#{FieldNumberMappings::MAX_FIELD_NUMBER}) has been reached" + )) + end + end + + describe "allocation cursor readers" do + it "returns stored cursors, defaulting to 1 for unmapped messages and enums" do + mappings = FieldNumberMappings.from_parsed_yaml({ + "messages" => {"Account" => {"fields" => {"id" => 7}, "next_number" => 10}}, + "enums" => {"Status" => {"values" => {"ACTIVE" => 3}, "next_number" => 8}} + }) + + expect(mappings.next_field_number_for("Account")).to eq(10) + expect(mappings.next_field_number_for("UnmappedMessage")).to eq(1) + expect(mappings.next_enum_value_number_for("Status")).to eq(8) + expect(mappings.next_enum_value_number_for("UnmappedEnum")).to eq(1) + end + end + + describe "#enum_value_numbers_for" do + it "allocates from the saved cursor without filling gaps" do + mappings = FieldNumberMappings.from_parsed_yaml({ + "enums" => {"Status" => {"values" => {"ACTIVE" => 3}, "next_number" => 10}} + }) + + expect(mappings.enum_value_numbers_for("Status", ["ARCHIVED", "ACTIVE", "DELETED"])).to eq({ + "ARCHIVED" => 10, + "ACTIVE" => 3, + "DELETED" => 11 + }) + expect(mappings.to_dumpable_hash.dig("enums", "Status", "values")).to eq({ + "ACTIVE" => 3, + "ARCHIVED" => 10, + "DELETED" => 11 + }) + expect(mappings.next_enum_value_number_for("Status")).to eq(12) + end + + it "raises a clear error when the enum value-number range has been exhausted" do + mappings = FieldNumberMappings.from_parsed_yaml({ + "enums" => {"Status" => { + "values" => {"ACTIVE" => FieldNumberMappings::MAX_ENUM_VALUE_NUMBER}, + "next_number" => FieldNumberMappings::MAX_ENUM_VALUE_NUMBER + 1 + }} + }) + + expect { + mappings.enum_value_numbers_for("Status", ["ACTIVE", "INACTIVE"]) + }.to raise_error(Errors::SchemaError, a_string_including( + "Cannot allocate another protobuf enum value number for enum `Status`", + "maximum enum value number (#{FieldNumberMappings::MAX_ENUM_VALUE_NUMBER}) has been reached" + )) + end + end + + describe "reserved number readers" do + it "returns mapped names that are not active, ordered by number" do + mappings = FieldNumberMappings.from_parsed_yaml({ + "messages" => {"Account" => { + "fields" => {"name" => 3, "legacy_id" => 1, "id" => 2}, + "next_number" => 4 + }}, + "enums" => {"Status" => {"values" => {"PAUSED" => 2, "ACTIVE" => 1}, "next_number" => 3}} + }) + + reserved_field_numbers = mappings.reserved_field_numbers_for("Account", ["id"]) + expect(reserved_field_numbers).to eq({ + "legacy_id" => 1, + "name" => 3 + }) + expect(reserved_field_numbers.keys).to eq(["legacy_id", "name"]) + expect(mappings.reserved_enum_value_numbers_for("Status", ["ACTIVE"])).to eq({"PAUSED" => 2}) + expect(mappings.reserved_field_numbers_for("MissingMessage", [])).to eq({}) + expect(mappings.reserved_enum_value_numbers_for("MissingEnum", [])).to eq({}) + end + end + + describe "#to_dumpable_hash" do + it "sorts messages and enums by name, and their fields and values by number" do + mappings = FieldNumberMappings.from_parsed_yaml( + { + "messages" => { + "ZMessage" => {"fields" => {"first" => 2, "second" => 1}, "next_number" => 3}, + "AMessage" => {"fields" => {"only" => 3}, "next_number" => 4} + }, + "enums" => { + "ZEnum" => {"values" => {"FIRST" => 2, "SECOND" => 1}, "next_number" => 3}, + "AEnum" => {"values" => {"ONLY" => 3}, "next_number" => 4} + } + } + ) + + artifact = mappings.to_dumpable_hash + expect(artifact.fetch("messages").keys).to eq(["AMessage", "ZMessage"]) + expect(artifact.dig("messages", "ZMessage", "fields").keys).to eq(["second", "first"]) + expect(artifact.dig("messages", "ZMessage", "next_number")).to eq(3) + expect(artifact.fetch("enums").keys).to eq(["AEnum", "ZEnum"]) + expect(artifact.dig("enums", "ZEnum", "values").keys).to eq(["SECOND", "FIRST"]) + expect(artifact.dig("enums", "ZEnum", "next_number")).to eq(3) + expect(artifact.dig("enums", "AEnum", "next_number")).to eq(4) + end + end + end + 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..9b07dbeb4 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension_spec.rb @@ -7,19 +7,40 @@ # frozen_string_literal: true require "elastic_graph/proto_ingestion/schema_definition/schema_artifact_manager_extension" +require "fileutils" require "stringio" module ElasticGraph module ProtoIngestion module SchemaDefinition RSpec.describe SchemaArtifactManagerExtension, :in_temp_dir do - it "dumps the proto schema artifact alongside the base artifacts" do - artifact_base_names = artifacts_for(define_indexed_type_schema) + it "locates `#{PROTO_FIELD_NUMBERS_FILE}` alongside the schema definition since it is an input to schema generation, unlike the proper schema artifacts" do + artifacts_by_file_name = artifacts_for(define_indexed_type_schema).to_h do |artifact| + [artifact.file_name, artifact] + end + + expect(artifacts_by_file_name.keys).to include( + ::File.join("config", PROTO_FIELD_NUMBERS_FILE), + ::File.join("artifacts", PROTO_SCHEMA_FILE) + ) + end - expect(artifact_base_names).to include(PROTO_SCHEMA_FILE) + it "documents the file's hand-editable input-file semantics instead of the standard `DO NOT EDIT BY HAND` preamble" do + artifact = artifacts_for(define_indexed_type_schema).find do |candidate| + ::File.basename(candidate.file_name) == PROTO_FIELD_NUMBERS_FILE + end + + artifact.dump(::StringIO.new) + contents = ::File.read(::File.join("config", PROTO_FIELD_NUMBERS_FILE)) + + expect(contents).to start_with("# This file is part of your schema definition") + expect(contents).to include("While prototyping, you may") + expect(contents).to include("Once generated protos have been used") + expect(contents).to include("you must NOT delete this file and regenerate it") + expect(contents).not_to include("DO NOT EDIT BY HAND") end - it "omits the proto schema artifact when the schema defines no indexed types" do + it "omits proto artifacts when the schema defines no indexed types" do results = define_proto_schema_results do |s| s.object_type "Point" do |t| t.field "x", "Float" @@ -34,13 +55,62 @@ module SchemaDefinition end end - artifact_base_names = artifacts_for(results) + artifact_base_names = artifact_base_names_for(results) + + expect(artifact_base_names).not_to include(PROTO_SCHEMA_FILE, PROTO_FIELD_NUMBERS_FILE) + end + + it "seeds proto generation with previously dumped field and enum numbers before an artifact manager is constructed" do + ::FileUtils.mkdir_p("config") + ::File.write(::File.join("config", PROTO_FIELD_NUMBERS_FILE), <<~YAML) + messages: + Widget: + fields: + id: 7 + next_number: 8 + enums: + Status: + values: + INACTIVE: 5 + next_number: 8 + YAML + + results = define_proto_schema_results(path_to_schema: ::File.join("config", "schema.rb")) do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE", "INACTIVE" + end + + s.object_type "Widget" do |t| + t.field "id", "ID" + t.field "status", "Status" + t.index "widgets" + end + end + + expect(results.proto_schema).to include( + "string id = 7;", + ".elasticgraph.Status status = 8;", + "STATUS_ACTIVE = 8;", + "STATUS_INACTIVE = 5;", + "// Next value number: 9" + ) + end - expect(artifact_base_names).not_to include(PROTO_SCHEMA_FILE) + it "raises a clear error when dumping proto artifacts without a configured `path_to_schema`" do + results = define_proto_schema_results do |s| + s.object_type "Widget" do |t| + t.field "id", "ID" + t.index "widgets" + end + end + + expect { + artifacts_for(results) + }.to raise_error Errors::SchemaError, a_string_including("without a configured `path_to_schema`") end def define_indexed_type_schema - define_proto_schema_results do |s| + define_proto_schema_results(path_to_schema: ::File.join("config", "schema.rb")) do |s| s.object_type "Widget" do |t| t.field "id", "ID" t.index "widgets" @@ -55,7 +125,11 @@ def artifacts_for(results) output: ::StringIO.new ) - manager.send(:artifacts_from_schema_def).map { |artifact| ::File.basename(artifact.file_name) } + manager.send(:artifacts_from_schema_def) + end + + def artifact_base_names_for(results) + artifacts_for(results).map { |artifact| ::File.basename(artifact.file_name) } end end end diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb index ca265658c..63d4d6694 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_edge_cases_spec.rb @@ -154,6 +154,65 @@ module SchemaDefinition expect(proto_type_def_from(proto, "Account")).to include(".elasticgraph.Status status = 2;") expect(proto_type_def_from(proto, "User")).to include(".elasticgraph.Status status = 2;") end + + it "raises when a hand-edited mappings artifact is invalid" do + # An invalid artifact can only arise from hand-editing (a prior dump is always valid), + # so this test must seed raw mappings instead of results from a prior dump. + results = define_proto_schema_results(proto_field_number_mappings: { + "messages" => {"Account" => {"fields" => {"id" => 0}}} + }) do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.index "accounts" + end + end + + expect { + results.proto_schema + }.to raise_error(Errors::SchemaError, a_string_including( + "Invalid protobuf field-number mappings", "/messages/Account/fields/id", "less than: 1" + )) + end + + it "skips the protobuf-reserved range when allocating new field numbers" do + # Reaching the real reserved range (19000-19999) would require ~19,000 fields, so we + # stub it to a small range to verify the allocator respects the constant. + stub_const("ElasticGraph::ProtoIngestion::SchemaDefinition::FieldNumberMappings::RESERVED_FIELD_NUMBER_RANGE", 3..4) + + results = define_proto_schema_results do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "name", "String" + t.field "email", "String" + t.index "accounts" + end + end + + expect(results.proto_schema).to include("string id = 1;", "string name = 2;", "string email = 5;") + expect(results.proto_field_number_mappings.dig("messages", "Account", "next_number")).to eq(6) + end + + it "allocates the next available field number when a renamed field has no old mapping entry" do + results1 = define_proto_schema_results do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.index "accounts" + end + end + + # `display_name` declares a rename from `full_name`, but no `full_name` mapping was ever dumped. + results2 = define_proto_schema_results(results1) do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "display_name", "String" do |f| + f.renamed_from "full_name" + end + t.index "accounts" + end + end + + expect(results2.proto_schema).to include("string id = 1;", "string display_name = 2;") + end end end end diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb index 5333d859c..51f98dbc1 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb @@ -52,11 +52,13 @@ module SchemaDefinition .elasticgraph.Status status = 2; .elasticgraph.Address address = 3; repeated string tags = 4; + // Next field number: 5 } message Address { string street = 1; string city = 2; + // Next field number: 3 } // The status of an account. @@ -69,6 +71,7 @@ module SchemaDefinition // The account is active. STATUS_ACTIVE = 1; STATUS_INACTIVE = 2; + // Next value number: 3 } PROTO end @@ -140,6 +143,7 @@ module SchemaDefinition .elasticgraph.Car car = 1; .elasticgraph.Bike bike = 2; } + // Next field number: 3 } PROTO expect(proto_type_def_from(proto, "Inventor")).to eq(<<~PROTO.strip) @@ -148,6 +152,7 @@ module SchemaDefinition .elasticgraph.Person person = 1; .elasticgraph.Company company = 2; } + // Next field number: 3 } PROTO expect(proto_type_def_from(proto, "Car")).to include("string id = 1;", "int32 doors = 2;") @@ -181,6 +186,7 @@ module SchemaDefinition oneof value { .elasticgraph.Car car = 1; } + // Next field number: 2 } PROTO expect(proto_type_def_from(proto, "Car")).to include("string id = 1;") @@ -205,6 +211,7 @@ module SchemaDefinition oneof value { .elasticgraph.DeliveryVehicle delivery_vehicle = 1; } + // Next field number: 2 } PROTO end @@ -242,18 +249,307 @@ module SchemaDefinition expect(proto_type_def_from(proto, "Event")).to include("int64 occurred_at = 2;") end - it "uses public field names in schema.proto when `name_in_index` differs" do - proto = define_proto_schema do |s| + it "assigns a new field the stored `next_number` rather than filling an earlier gap" do + # A cursor with gaps below it can only arise from a hand-edited artifact, so this test + # must seed raw mappings instead of results from a prior dump. + results = define_proto_schema_results(proto_field_number_mappings: { + "messages" => { + "Account" => { + "fields" => { + "id" => 7 + }, + "next_number" => 10 + } + } + }) do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "name", "String" + t.index "accounts" + end + end + + expect(results.proto_schema).to include("string id = 7;", "string name = 10;") + expect(proto_type_def_from(results.proto_schema, "Account")).to include("// Next field number: 11") + expect(results.proto_field_number_mappings.dig("messages", "Account", "next_number")).to eq(11) + end + + it "preserves proto field numbers when fields are re-ordered" do + results1 = define_proto_schema_results do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "name", "String" + t.field "age", "Int" + t.index "accounts" + end + end + + expect(proto_type_def_from(results1.proto_schema, "Account")).to include( + "string id = 1;", "string name = 2;", "int32 age = 3;" + ) + + results2 = define_proto_schema_results(results1) do |s| + s.object_type "Account" do |t| + t.field "age", "Int" + t.field "id", "ID" + t.field "name", "String" + t.index "accounts" + end + end + + expect(proto_type_def_from(results2.proto_schema, "Account")).to include( + "string id = 1;", "string name = 2;", "int32 age = 3;" + ) + expect(results2.proto_field_number_mappings).to eq(results1.proto_field_number_mappings) + end + + it "exposes generated field-number mappings as an artifact hash" do + results = define_proto_schema_results do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "name", "String" + t.index "accounts" + end + end + + expect(results.proto_field_number_mappings).to eq({ + "enums" => {}, + "messages" => { + "Account" => { + "fields" => { + "id" => 1, + "name" => 2 + }, + "next_number" => 3 + } + } + }) + end + + it "keeps a removed field's number reserved and restores it if the field is re-added" do + results1 = define_proto_schema_results do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "legacy_field", "String" + t.index "accounts" + end + end + + expect(results1.proto_schema).to include("string legacy_field = 2;") + + # `legacy_field` has been removed and `name` added since the mappings were dumped. + results2 = define_proto_schema_results(results1) do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "name", "String" + t.index "accounts" + end + end + + expect(results2.proto_schema).to include("string id = 1;", "string name = 3;") + expect(results2.proto_schema).to include("reserved 2; // Previously used by legacy_field.") + + # `legacy_field` keeps its number reserved in the artifact so it is never reused. + expect(results2.proto_field_number_mappings).to eq({ + "enums" => {}, + "messages" => { + "Account" => { + "fields" => { + "id" => 1, + "legacy_field" => 2, + "name" => 3 + }, + "next_number" => 4 + } + } + }) + + # `legacy_field` is restored after the intermediate artifact has been dumped. + results3 = define_proto_schema_results(results2) do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "legacy_field", "String" + t.field "name", "String" + t.index "accounts" + end + end + + expect(results3.proto_schema).to include( + "string id = 1;", "string legacy_field = 2;", "string name = 3;", "// Next field number: 4" + ) + expect(results3.proto_schema).not_to include("reserved 2;") + expect(results3.proto_field_number_mappings).to eq(results2.proto_field_number_mappings) + end + + it "keeps index field names out of the protobuf schema and field-number mappings" do + results1 = define_proto_schema_results do |s| + s.object_type "Widget" do |t| + t.field "id", "ID" + t.field "display_name", "String", name_in_index: "old_index_name" + t.index "widgets" + end + end + + results2 = define_proto_schema_results(results1) do |s| s.object_type "Widget" do |t| t.field "id", "ID" - t.field "display_name", "String", name_in_index: "display_name_in_index" + t.field "display_name", "String", name_in_index: "new_index_name" t.index "widgets" 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(results1.proto_schema).to include("string display_name = 2;") + expect(results1.proto_schema).not_to include("old_index_name") + expect(results2.proto_schema).to eq(results1.proto_schema) + + expect(results2.proto_field_number_mappings.dig("messages", "Widget", "fields")).to eq({ + "id" => 1, + "display_name" => 2 + }) + end + + it "preserves a field number across a public field rename" do + results1 = define_proto_schema_results do |s| + s.object_type "Account" do |t| + t.field "full_name", "String" + t.field "id", "ID" + t.index "accounts" + end + end + + expect(results1.proto_schema).to include("string full_name = 1;") + + results2 = define_proto_schema_results(results1) do |s| + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "display_name", "String" do |f| + f.renamed_from "full_name" + end + t.index "accounts" + end + end + + expect(results2.proto_schema).to include("string id = 2;", "string display_name = 1;") + expect(results2.proto_schema).not_to include("reserved 1;") + expect(results2.proto_field_number_mappings).to eq({ + "enums" => {}, + "messages" => { + "Account" => { + "fields" => { + "id" => 2, + "display_name" => 1 + }, + "next_number" => 3 + } + } + }) + end + + it "preserves enum value numbers as the enum evolves, reserving removed values' numbers" do + results1 = define_proto_schema_results do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE", "PAUSED", "INACTIVE" + end + + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "status", "Status" + t.index "accounts" + end + end + + expect(proto_type_def_from(results1.proto_schema, "Status")).to include( + "STATUS_ACTIVE = 1;", + "STATUS_PAUSED = 2;", + "STATUS_INACTIVE = 3;", + "// Next value number: 4" + ) + + # `PAUSED` has been removed and `ARCHIVED` added since the mappings were dumped. + results2 = define_proto_schema_results(results1) do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE", "INACTIVE", "ARCHIVED" + end + + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "status", "Status" + t.index "accounts" + end + end + + expect(proto_type_def_from(results2.proto_schema, "Status")).to include( + "STATUS_ACTIVE = 1;", + "STATUS_INACTIVE = 3;", + "STATUS_ARCHIVED = 4;", + "reserved 2; // Previously used by PAUSED.", + "// Next value number: 5" + ) + + # `PAUSED` keeps its number reserved in the artifact so it is never reused for a new value. + expect(results2.proto_field_number_mappings.fetch("enums")).to eq({ + "Status" => { + "values" => { + "ACTIVE" => 1, + "PAUSED" => 2, + "INACTIVE" => 3, + "ARCHIVED" => 4 + }, + "next_number" => 5 + } + }) + end + + it "preserves stable field numbers for oneof alternatives as subtypes are added and removed" do + results1 = define_proto_schema_results do |s| + ["Truck", "Car", "Bike"].each do |type_name| + s.object_type type_name do |t| + t.field "id", "ID" + end + end + + s.union_type "Vehicle" do |t| + t.subtypes "Truck", "Car", "Bike" + t.index "vehicles" + end + end + + expect(proto_type_def_from(results1.proto_schema, "Vehicle")).to include( + "Truck truck = 1;", "Car car = 2;", "Bike bike = 3;" + ) + + # `Truck` has been removed and `Scooter` added since the mappings were dumped. + results2 = define_proto_schema_results(results1) do |s| + ["Car", "Bike", "Scooter"].each do |type_name| + s.object_type type_name do |t| + t.field "id", "ID" + end + end + + s.union_type "Vehicle" do |t| + t.subtypes "Car", "Bike", "Scooter" + t.index "vehicles" + end + end + + vehicle = proto_type_def_from(results2.proto_schema, "Vehicle") + expect(vehicle).to include( + "Car car = 2;", + "Bike bike = 3;", + "Scooter scooter = 4;", + "reserved 1; // Previously used by truck." + ) + + # `truck` keeps its number reserved in the artifact so it is never reused. + expect(results2.proto_field_number_mappings.fetch("messages").fetch("Vehicle")).to eq({ + "fields" => { + "truck" => 1, + "car" => 2, + "bike" => 3, + "scooter" => 4 + }, + "next_number" => 5 + }) end it "renders independently each time it is called" do diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_artifact_manager.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_artifact_manager.rb index 10c29023b..9e78ff6b2 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_artifact_manager.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_artifact_manager.rb @@ -179,23 +179,25 @@ def truncate_diff(diff, lines) def new_yaml_artifact(file_name, desired_contents, extra_comment_lines: []) SchemaArtifact.new( - ::File.join(@schema_artifacts_directory, file_name), - desired_contents, - ->(hash) { ::YAML.dump(hash) }, - ->(string) { ::YAML.safe_load(string) }, - extra_comment_lines, - "#" + file_name: ::File.join(@schema_artifacts_directory, file_name), + desired_contents: desired_contents, + dumper: ->(hash) { ::YAML.dump(hash) }, + loader: ->(string) { ::YAML.safe_load(string) }, + extra_comment_lines: extra_comment_lines, + comment_prefix: "#", + comment_preamble_lines: SchemaArtifact::COMMENT_PREAMBLE_LINES ) end def new_raw_artifact(file_name, desired_contents, comment_prefix: "#") SchemaArtifact.new( - ::File.join(@schema_artifacts_directory, file_name), - desired_contents, - _ = :itself.to_proc, - _ = :itself.to_proc, - [], - comment_prefix + file_name: ::File.join(@schema_artifacts_directory, file_name), + desired_contents: desired_contents, + dumper: _ = :itself.to_proc, + loader: _ = :itself.to_proc, + extra_comment_lines: [], + comment_prefix: comment_prefix, + comment_preamble_lines: SchemaArtifact::COMMENT_PREAMBLE_LINES ) end @@ -217,7 +219,20 @@ def pruned_runtime_metadata(graphql_schema_string) end # @private - class SchemaArtifact < Support::MemoizableData.define(:file_name, :desired_contents, :dumper, :loader, :extra_comment_lines, :comment_prefix) + class SchemaArtifact < Support::MemoizableData.define( + :file_name, + :desired_contents, + :dumper, + :loader, + :extra_comment_lines, + :comment_prefix, + :comment_preamble_lines + ) + COMMENT_PREAMBLE_LINES = [ + "Generated by `bundle exec rake schema_artifacts:dump`.", + "DO NOT EDIT BY HAND. Any edits will be lost the next time the rake task is run." + ].freeze + def dump(output) if out_of_date? dirname = File.dirname(file_name) @@ -237,8 +252,7 @@ def out_of_date? def existing_dumped_contents return nil unless exists? - # We drop the first 2 lines because it is the comment block containing dynamic elements. - file_contents = ::File.read(file_name).split("\n").drop(2).join("\n") + file_contents = ::File.read(file_name).split("\n").drop(comment_preamble_lines_with_extras.size).join("\n") loader.call(file_contents) end @@ -267,13 +281,13 @@ def dumped_contents end def comment_preamble - lines = [ - "Generated by `bundle exec rake schema_artifacts:dump`.", - "DO NOT EDIT BY HAND. Any edits will be lost the next time the rake task is run." - ] + comment_preamble_lines_with_extras.map { |line| "#{comment_prefix} #{line}".rstrip }.join("\n") + end + + def comment_preamble_lines_with_extras + return comment_preamble_lines if extra_comment_lines.empty? - lines = extra_comment_lines + [""] + lines unless extra_comment_lines.empty? - lines.map { |line| "#{comment_prefix} #{line}".rstrip }.join("\n") + extra_comment_lines + [""] + comment_preamble_lines end end end diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/test_support.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/test_support.rb index 92c4a0595..8c977a857 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/test_support.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/test_support.rb @@ -22,6 +22,7 @@ def define_schema( schema_element_name_form:, schema_element_name_overrides: {}, index_document_sizes: true, + path_to_schema: nil, extension_modules: [], derived_type_name_formats: {}, type_name_overrides: {}, @@ -37,6 +38,7 @@ def define_schema( define_schema_with_schema_elements( schema_elements, index_document_sizes: index_document_sizes, + path_to_schema: path_to_schema, extension_modules: extension_modules, derived_type_name_formats: derived_type_name_formats, type_name_overrides: type_name_overrides, @@ -49,6 +51,7 @@ def define_schema( def define_schema_with_schema_elements( schema_elements, index_document_sizes: true, + path_to_schema: nil, extension_modules: [], derived_type_name_formats: {}, type_name_overrides: {}, @@ -58,6 +61,7 @@ def define_schema_with_schema_elements( api = API.new( schema_elements, index_document_sizes, + path_to_schema: path_to_schema, extension_modules: extension_modules, derived_type_name_formats: derived_type_name_formats, type_name_overrides: type_name_overrides, diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_artifact_manager.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_artifact_manager.rbs index 88c63edb6..e01d9b3ed 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_artifact_manager.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/schema_artifact_manager.rbs @@ -49,17 +49,32 @@ module ElasticGraph attr_reader loader: ^(::String) -> T attr_reader extra_comment_lines: ::Array[::String] attr_reader comment_prefix: ::String + attr_reader comment_preamble_lines: ::Array[::String] def initialize: ( - ::String, - T, - ^(T) -> ::String, - ^(::String) -> T, - ::Array[::String], - ::String) -> void + file_name: ::String, + desired_contents: T, + dumper: ^(T) -> ::String, + loader: ^(::String) -> T, + extra_comment_lines: ::Array[::String], + comment_prefix: ::String, + comment_preamble_lines: ::Array[::String] + ) -> void + + def with: ( + ?file_name: ::String, + ?desired_contents: T, + ?dumper: ^(T) -> ::String, + ?loader: ^(::String) -> T, + ?extra_comment_lines: ::Array[::String], + ?comment_prefix: ::String, + ?comment_preamble_lines: ::Array[::String] + ) -> self end class SchemaArtifact[T] < SchemaArtifactSupertype[T] + COMMENT_PREAMBLE_LINES: ::Array[::String] + def dump: (io) -> void def out_of_date?: () -> bool def existing_dumped_contents: () -> T? @@ -73,6 +88,7 @@ module ElasticGraph @dumped_contents: ::String? def dumped_contents: () -> ::String def comment_preamble: () -> ::String + def comment_preamble_lines_with_extras: () -> ::Array[::String] end end end diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/test_support.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/test_support.rbs index 18dcc688f..03caf1699 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/test_support.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/test_support.rbs @@ -5,6 +5,7 @@ module ElasticGraph schema_element_name_form: SchemaArtifacts::RuntimeMetadata::SchemaElementNames::form, ?schema_element_name_overrides: ::Hash[::Symbol, ::String], ?index_document_sizes: bool, + ?path_to_schema: ::String?, ?extension_modules: ::Array[::Module], ?derived_type_name_formats: ::Hash[::Symbol, ::String], ?type_name_overrides: ::Hash[::Symbol, ::String], @@ -15,6 +16,7 @@ module ElasticGraph def define_schema_with_schema_elements: ( SchemaArtifacts::RuntimeMetadata::SchemaElementNames, ?index_document_sizes: bool, + ?path_to_schema: ::String?, ?extension_modules: ::Array[::Module], ?derived_type_name_formats: ::Hash[::Symbol, ::String], ?type_name_overrides: ::Hash[::Symbol, ::String], diff --git a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/schema_artifact_spec.rb b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/schema_artifact_spec.rb index f071d0600..7defad81f 100644 --- a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/schema_artifact_spec.rb +++ b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/schema_artifact_spec.rb @@ -14,12 +14,13 @@ module SchemaDefinition RSpec.describe SchemaArtifact, :in_temp_dir do it "renders the comment preamble using the configured comment prefix so artifacts can use their format's comment syntax" do artifact = SchemaArtifact.new( - "widgets.proto", - "message Widget {}", - :itself.to_proc, - :itself.to_proc, - [], - "//" + file_name: "widgets.proto", + desired_contents: "message Widget {}", + dumper: :itself.to_proc, + loader: :itself.to_proc, + extra_comment_lines: [], + comment_prefix: "//", + comment_preamble_lines: SchemaArtifact::COMMENT_PREAMBLE_LINES ) artifact.dump(::StringIO.new) @@ -30,6 +31,30 @@ module SchemaDefinition message Widget {} EOS end + + it "renders configured comment preamble lines" do + artifact = SchemaArtifact.new( + file_name: "widgets.proto", + desired_contents: "message Widget {}", + dumper: :itself.to_proc, + loader: :itself.to_proc, + extra_comment_lines: ["Extra context."], + comment_prefix: "//", + comment_preamble_lines: ["Custom preamble."] + ) + + artifact.dump(::StringIO.new) + + expect(::File.read("widgets.proto")).to eq(<<~EOS.strip) + // Extra context. + // + // Custom preamble. + message Widget {} + EOS + + identical_artifact = artifact.with + expect(identical_artifact).not_to be_out_of_date + end end end end