diff --git a/config/site/support/doctest_helper.rb b/config/site/support/doctest_helper.rb index 70c2c9ca2..4163dc566 100644 --- a/config/site/support/doctest_helper.rb +++ b/config/site/support/doctest_helper.rb @@ -103,6 +103,21 @@ module ElasticGraph end end + doctest.before "ElasticGraph::ProtoIngestion::SchemaDefinition::SchemaElements::EnumTypeExtension#external_proto_enum" do + extend ::RSpec::Mocks::ExampleMethods + + # The examples source enum values from an app-defined proto enum class; provide one here. + proto_enum_entry = ::Data.define(:name, :number) + currency_proto_enum = ::Class.new + currency_proto_enum.define_singleton_method(:enums) do + [:CURRENCY_UNKNOWN_DO_NOT_USE, :CURRENCY_USD, :CURRENCY_CAD].each_with_index.map do |name, number| + proto_enum_entry.new(name: name, number: number) + end + end + + stub_const("MyApp::Protos::Currency", currency_proto_enum) + end + doctest.before "ElasticGraph::SchemaDefinition::SchemaElements::ScalarType#coerce_with" do ::FileUtils.mkdir_p "coercion_adapters" ::File.write("coercion_adapters/phone_number.rb", <<~EOS) diff --git a/elasticgraph-proto_ingestion/README.md b/elasticgraph-proto_ingestion/README.md index 2e1c3dce1..f31247e4e 100644 --- a/elasticgraph-proto_ingestion/README.md +++ b/elasticgraph-proto_ingestion/README.md @@ -205,6 +205,84 @@ enums: BLUE: 2 ``` +### Sourcing Enum Values From Existing Protobuf Enums + +If your project already has canonical protobuf enum definitions, you can source an enum's +generated proto values from them instead of maintaining the value list in two places. Call +`external_proto_enum` on the enum type with a proto enum class (anything exposing `.enums`). +In the examples below, this stand-in plays the role of your app's generated proto enum class: + +```ruby +# in config/schema/app_protos.rb + +# Stands in for a proto enum class generated by your app's protobuf tooling. +module MyApp + module Protos + class Currency + EnumEntry = ::Data.define(:name, :number) + + def self.enums + [ + EnumEntry.new(name: :CURRENCY_UNKNOWN_DO_NOT_USE, number: 0), + EnumEntry.new(name: :CURRENCY_USD, number: 1), + EnumEntry.new(name: :CURRENCY_CAD, number: 2) + ] + end + end + end +end +``` + +Optional per-source options curate and transform the sourced values: + +```ruby +# in config/schema/currency.rb + +ElasticGraph.define_schema do |schema| + schema.enum_type "Currency" do |t| + t.values "USD", "CAD" + t.external_proto_enum MyApp::Protos::Currency, + # Proto values to omit from the generated enum. + exclusions: [:UNKNOWN_DO_NOT_USE], + # Values expected in the generated enum that the proto enum lacks. + expected_extras: [:LEGACY], + # Optional transform applied to each proto value name. + name_transform: ->(name) { name.sub(/\ACURRENCY_/, "") } + end +end +``` + +When an enum has one or more external sources, `elasticgraph-proto_ingestion` uses them +as the source of the generated enum's values. When multiple sources are registered for the +same enum, they must all resolve to the same value set. + +### Referencing Existing Protobuf Types + +For enums that exactly match a canonical proto enum, you can go further and reference the +existing proto type instead of generating a duplicate local enum. Pass `proto:` and `import:` +to `external_proto_enum`, and `schema.proto` will import the named file and use the external +type name directly: + +```ruby +# in config/schema/currency.rb + +ElasticGraph.define_schema do |schema| + schema.enum_type "Currency" do |t| + t.values "USD", "CAD" + t.external_proto_enum MyApp::Protos::Currency, + proto: "myapp.types.Currency", + import: "myapp/types/currency.proto" + end +end +``` + +A referenced enum must have exactly one option-free source whose values match the +ElasticGraph enum's values; transformed, curated, or multi-source enums stay generated +locally so value curation remains explicit. The source's enum entries must also expose +`.number`, and those numbers must agree with any numbers previously pinned in +`proto_field_numbers.yaml` — otherwise switching to the external type would silently +reinterpret existing wire data. + ## Type Mappings The generated `schema.proto` uses these built-in scalar mappings: 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 e89e5e685..74b371287 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 @@ -157,6 +157,13 @@ def enum_value_numbers_for(enum_name, value_names) end end + # Returns previously pinned numbers for a protobuf enum. + # + # @api private + def pinned_enum_value_numbers(enum_name) + @proto_enum_value_numbers_by_enum[enum_name] || {} + end + # Returns the label for a protobuf field under the configured syntax. # # @api private 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 2652b66e5..16b67addf 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 @@ -18,6 +18,58 @@ module SchemaDefinition module SchemaElements # Extends EnumType with proto field type conversion. module EnumTypeExtension + # Describes an external proto enum registered as the source of this enum's generated values. + ExternalProtoEnumSource = ::Data.define(:proto_enum, :exclusions, :expected_extras, :name_transform) + + # Describes an existing proto enum type referenced instead of generating a local definition. + ExternalProtoEnumReference = ::Data.define(:proto_name, :import) + + # Sources this enum's generated proto values from an existing proto enum class. Passing + # `proto:` and `import:` references that enum directly instead of generating a local enum. + # + # @return [void] + def external_proto_enum(proto_enum, exclusions: [], expected_extras: [], name_transform: nil, proto: nil, import: nil) + unless proto_enum.respond_to?(:enums) + raise Errors::SchemaError, "`external_proto_enum` on `#{name}` must be given a proto enum class with `.enums`, " \ + "but got: #{proto_enum.inspect}." + end + + if proto || import + unless proto.is_a?(String) && !proto.empty? && import.is_a?(String) && !import.empty? + raise Errors::SchemaError, "`external_proto_enum` on `#{name}` must be given both `proto` and `import` " \ + "as non-empty Strings to reference an external proto enum type." + end + unless exclusions.empty? && expected_extras.empty? && name_transform.nil? + raise Errors::SchemaError, "`external_proto_enum` on `#{name}` cannot combine `proto`/`import` with " \ + "`exclusions`, `expected_extras`, or `name_transform`; transformed or curated enums must stay generated locally." + end + + @external_proto_reference = ExternalProtoEnumReference.new(proto_name: proto, import: import) + @proto_name = nil + end + + external_proto_enum_sources << ExternalProtoEnumSource.new( + proto_enum: proto_enum, + exclusions: exclusions.map(&:to_s), + expected_extras: expected_extras.map(&:to_s), + name_transform: name_transform + ) + nil + end + + # External proto enums registered via {#external_proto_enum}. + # + # @return [Array] + def external_proto_enum_sources + @external_proto_enum_sources ||= [] + end + + # The external proto enum type referenced instead of generating a local enum, if configured. + # + # @dynamic external_proto_reference + # @return [ExternalProtoEnumReference, nil] + attr_reader :external_proto_reference + # Defines an enum value and immediately validates its protobuf name. # # @return [void] @@ -49,6 +101,11 @@ def value(value_name, &block) # # @return [String] def to_proto(schema) + if external_proto_reference + validate_external_proto_reference(schema) + return nil + end + render_proto_enum(schema) end @@ -70,7 +127,14 @@ def referenced_proto_types # # @return [String] def proto_name - @proto_name ||= Identifier.enum_name(name) + @proto_name ||= external_proto_reference&.proto_name || Identifier.enum_name(name) + end + + # Returns the proto file imported for an externally referenced enum. + # + # @return [String, nil] + def protobuf_import + external_proto_reference&.import end # @private @@ -83,20 +147,33 @@ def configure_derived_scalar_type(scalar_type) private def render_proto_enum(schema) - values = values_by_name.values - value_numbers = schema.enum_value_numbers_for(proto_name, values_by_name.keys) + source_value_names = proto_enum_value_names + proto_value_names_by_source_name = source_value_names.to_h do |source_name| + [source_name, proto_enum_value_name(source_name)] + end + duplicate_names = proto_value_names_by_source_name.values.tally.select { |_, count| count > 1 } + if duplicate_names.any? + raise Errors::SchemaError, "Enum `#{name}` maps to duplicate proto enum value names: #{duplicate_names.keys.sort.join(", ")}." + end + value_numbers = schema.enum_value_numbers_for(proto_name, source_value_names) zero_value_name = "#{enum_value_prefix}_UNSPECIFIED" + if (source_name = proto_value_names_by_source_name.key(zero_value_name)) + raise Errors::SchemaError, "Enum `#{name}` value `#{source_name}` maps to proto enum value name " \ + "`#{zero_value_name}`, which conflicts with the generated zero value `#{zero_value_name}`." + end lines = ProtoDocumentation.comment_lines_for(doc_comment) lines << "enum #{proto_name} {" lines << " // The default value when no enum value has been explicitly set. Do not use this value." lines << " // See https://protobuf.dev/programming-guides/proto3/#enum-default." lines << " #{zero_value_name} = 0;" - values.each do |raw_value| - value = raw_value # : ::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & EnumValueExtension - lines.concat(ProtoDocumentation.comment_lines_for(value.doc_comment, indent: " ")) - lines << " #{value.proto_name(enum_value_prefix)} = #{value_numbers.fetch(value.name)};" + proto_value_names_by_source_name.each do |source_name, proto_value_name| + if (raw_value = values_by_name[source_name]) + value = raw_value # : ::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & EnumValueExtension + lines.concat(ProtoDocumentation.comment_lines_for(value.doc_comment, indent: " ")) + end + lines << " #{proto_value_name} = #{value_numbers.fetch(source_name)};" end lines << "}" lines.join("\n") @@ -105,6 +182,82 @@ def render_proto_enum(schema) def enum_value_prefix Support::Casing.to_upper_snake(name) end + + def proto_enum_value_name(value_name) + if (raw_value = values_by_name[value_name]) + value = raw_value # : ::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & EnumValueExtension + value.proto_name(enum_value_prefix) + else + Identifier.enum_value_name("#{enum_value_prefix}_#{Support::Casing.to_upper_snake(value_name)}") + end + end + + def proto_enum_value_names + return values_by_name.keys if external_proto_enum_sources.empty? + + values_by_source = external_proto_enum_sources.map { |source| enum_value_names_from_source(source) } + canonical_values = values_by_source.first + canonical_set = canonical_values.uniq.sort + if values_by_source.drop(1).any? { |source_values| source_values.uniq.sort != canonical_set } + raise Errors::SchemaError, "External proto enums for `#{name}` produce inconsistent value sets. " \ + "Ensure each `external_proto_enum` source (with exclusions/expected_extras/name_transform) resolves to the same values." + end + canonical_values + end + + def enum_value_names_from_source(source) + name_transform = source.name_transform || :itself.to_proc + mapped_values = source.proto_enum.enums.map { |entry| name_transform.call(entry.name.to_s).to_s } + (mapped_values - source.exclusions + source.expected_extras).uniq + rescue => e + raise Errors::SchemaError, "Failed loading external proto enum values for `#{name}` from `#{source.proto_enum}`: #{e.message}" + end + + def validate_external_proto_reference(schema) + unless external_proto_enum_sources.one? + raise Errors::SchemaError, "External proto enum `#{name}` must use exactly one `external_proto_enum` " \ + "source; multi-source enums cannot be safely referenced externally." + end + + numbers_by_value_name = enum_value_numbers_from_source(external_proto_enum_sources.first) + external_names = numbers_by_value_name.keys.sort + elasticgraph_names = values_by_name.keys.map(&:to_s).uniq.sort + if external_names != elasticgraph_names + raise Errors::SchemaError, "External proto enum `#{name}` values do not match the ElasticGraph enum values. " \ + "External values: #{external_names.join(", ")}. ElasticGraph values: #{elasticgraph_names.join(", ")}." + end + + schema.pinned_enum_value_numbers(Identifier.enum_name(name)).each do |value_name, pinned_number| + external_number = numbers_by_value_name[value_name] + if external_number && external_number != pinned_number + raise Errors::SchemaError, "External proto enum `#{name}` assigns `#{value_name}` the number " \ + "#{external_number}, but previously dumped artifacts pin it to #{pinned_number}; referencing it would " \ + "silently reinterpret existing wire data." + end + + conflicting_name = numbers_by_value_name.find do |external_name, number| + external_name != value_name && number == pinned_number + end&.first + if conflicting_name + raise Errors::SchemaError, "External proto enum `#{name}` assigns `#{conflicting_name}` the number " \ + "#{pinned_number}, which previously dumped artifacts pin to `#{value_name}`; referencing it would " \ + "silently reinterpret existing wire data." + end + end + end + + def enum_value_numbers_from_source(source) + entries = source.proto_enum.enums + unless entries.all? { |entry| entry.respond_to?(:number) } + raise Errors::SchemaError, "External proto enum `#{name}` cannot be referenced: its enum entries " \ + "must expose `.number` so its values can be verified against previously pinned numbers." + end + entries.to_h { |entry| [entry.name.to_s, entry.number] } + rescue Errors::SchemaError + raise + rescue => e + raise Errors::SchemaError, "Failed loading external proto enum values for `#{name}` from `#{source.proto_enum}`: #{e.message}" + end 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 f786d1464..3da341d9b 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 name_in_index: ::String ) -> ::Integer def enum_value_numbers_for: (::String, ::Array[::String]) -> ::Hash[::String, ::Integer] + def pinned_enum_value_numbers: (::String) -> ::Hash[::String, ::Integer] def field_label: (bool) -> ::String? private 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 5e47f4c57..64e1517f4 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 @@ -3,11 +3,38 @@ module ElasticGraph module SchemaDefinition module SchemaElements module EnumTypeExtension: ::ElasticGraph::SchemaDefinition::SchemaElements::EnumType + class ExternalProtoEnumSource + attr_reader proto_enum: untyped + attr_reader exclusions: ::Array[::String] + attr_reader expected_extras: ::Array[::String] + attr_reader name_transform: untyped + + def self.new: ( + proto_enum: untyped, + exclusions: ::Array[::String], + expected_extras: ::Array[::String], + name_transform: untyped + ) -> instance + end + + class ExternalProtoEnumReference + attr_reader proto_name: ::String + attr_reader import: ::String + + def self.new: (proto_name: ::String, import: ::String) -> instance + end + @proto_name: ::String? + @external_proto_enum_sources: ::Array[ExternalProtoEnumSource]? + @external_proto_reference: ExternalProtoEnumReference? + def external_proto_enum: (untyped, ?exclusions: ::Array[::String | ::Symbol], ?expected_extras: ::Array[::String | ::Symbol], ?name_transform: untyped, ?proto: ::String?, ?import: ::String?) -> void + def external_proto_enum_sources: () -> ::Array[ExternalProtoEnumSource] + attr_reader external_proto_reference: ExternalProtoEnumReference? def value: (::String) ?{ (::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & EnumValueExtension) -> void } -> void def proto_name: () -> ::String - def to_proto: (Schema) -> ::String + def protobuf_import: () -> ::String? + def to_proto: (Schema) -> ::String? def proto_definition_kind: () -> :enum def referenced_proto_types: () -> ::Array[::ElasticGraph::SchemaDefinition::SchemaElements::graphQLType] def configure_derived_scalar_type: (::ElasticGraph::SchemaDefinition::SchemaElements::ScalarType) -> void @@ -16,6 +43,11 @@ module ElasticGraph def render_proto_enum: (Schema) -> ::String def enum_value_prefix: () -> ::String + def proto_enum_value_name: (::String) -> ::String + def proto_enum_value_names: () -> ::Array[::String] + def enum_value_names_from_source: (ExternalProtoEnumSource) -> ::Array[::String] + def validate_external_proto_reference: (Schema) -> void + def enum_value_numbers_from_source: (ExternalProtoEnumSource) -> ::Hash[::String, ::Integer] end end end diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/factory_extension_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/factory_extension_spec.rb index 1b10b5fc1..38b12ecce 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/factory_extension_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/factory_extension_spec.rb @@ -54,6 +54,31 @@ module SchemaDefinition end }.to raise_error(Errors::SchemaError, a_string_including("both map to the same proto type name `package_`")) end + + it "detects proto type name collisions after enum configuration blocks run" do + proto_status = ::Data.define(:enums).new(enums: []) + + expect { + define_proto_schema do |s| + s.enum_type "CurrentStatus" do |t| + t.value "ACTIVE" + t.external_proto_enum proto_status, + proto: "myapp.types.Status", + import: "myapp/types/status.proto" + end + + s.enum_type "PreviousStatus" do |t| + t.value "ACTIVE" + t.external_proto_enum proto_status, + proto: "myapp.types.Status", + import: "myapp/types/status.proto" + end + end + }.to raise_error(Errors::SchemaError, a_string_including( + "Type names `CurrentStatus` and `PreviousStatus`", + "same proto type name `myapp.types.Status`" + )) + end 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 320cf2479..72ed9f743 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 @@ -36,6 +36,64 @@ module SchemaDefinition )) end + it "raises when externally sourced enum values map to duplicate proto value names" do + proto_status = ::Class.new do + def self.enums + [ + ::Data.define(:name).new(name: :option), + ::Data.define(:name).new(name: :OPTION) + ] + end + end + + results = define_proto_schema_results do |s| + s.enum_type "Status" do |t| + t.value "ACTIVE" + t.external_proto_enum proto_status + 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 `Status` maps to duplicate proto enum value names: STATUS_OPTION" + )) + end + + it "raises when an externally sourced enum value conflicts with the generated zero value" do + proto_status = ::Class.new do + def self.enums + [::Data.define(:name).new(name: :UNSPECIFIED)] + end + end + + results = define_proto_schema_results do |s| + s.enum_type "Status" do |t| + t.value "ACTIVE" + t.external_proto_enum proto_status + 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 `Status` value `UNSPECIFIED`", + "conflicts with the generated zero value `STATUS_UNSPECIFIED`" + )) + end + it "raises when an enum value conflicts with the generated zero value" do expect { define_proto_schema do |s| @@ -85,6 +143,264 @@ module SchemaDefinition expect(proto_type_def_from(proto, "Account")).to include("Status status = 2;") expect(proto_type_def_from(proto, "User")).to include("Status status = 2;") end + + it "raises when `external_proto_enum` is given an object without `.enums`" do + expect { + define_proto_schema do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE" + t.external_proto_enum ::Object.new + end + end + }.to raise_error(Errors::SchemaError, a_string_including( + "`external_proto_enum` on `Status`", "must be given a proto enum class with `.enums`" + )) + end + + it "wraps unexpected exceptions from external proto enum sources" do + proto_status = ::Class.new do + def self.enums + [::Data.define(:name).new(name: :ACTIVE)] + end + end + + results = define_proto_schema_results do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE" + t.external_proto_enum proto_status, name_transform: ->(_name) { raise "boom" } + 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("Failed loading external proto enum values for `Status`")) + end + + it "accepts multiple external proto enum sources when they resolve to the same values" do + proto_status_a = ::Class.new do + def self.enums + [::Data.define(:name).new(name: :ACTIVE)] + end + end + + proto_status_b = ::Class.new do + def self.enums + [::Data.define(:name).new(name: :ACTIVE)] + end + end + + results = define_proto_schema_results do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE" + t.external_proto_enum proto_status_a + t.external_proto_enum proto_status_b + 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 include("STATUS_ACTIVE = 1;") + end + + it "requires both `proto` and `import` as non-empty Strings to reference an external proto enum type" do + # The eager validation raises before `.enums` is ever consulted. + proto_status = ::Object.new + proto_status.define_singleton_method(:enums) { [] } + + [ + {proto: "myapp.types.Status"}, + {import: "myapp/types/status.proto"}, + {proto: "", import: "myapp/types/status.proto"}, + {proto: :"myapp.types.Status", import: "myapp/types/status.proto"} + ].each do |reference_options| + expect { + define_proto_schema do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE" + t.external_proto_enum proto_status, **reference_options + end + end + }.to raise_error(Errors::SchemaError, a_string_including( + "`external_proto_enum` on `Status`", "must be given both `proto` and `import` as non-empty Strings" + )) + end + end + + it "raises when an external reference is combined with transform options" do + # The eager validation raises before `.enums` is ever consulted. + proto_status = ::Object.new + proto_status.define_singleton_method(:enums) { [] } + + expect { + define_proto_schema do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE" + t.external_proto_enum proto_status, + exclusions: [:LEGACY], + proto: "myapp.types.Status", + import: "myapp/types/status.proto" + end + end + }.to raise_error(Errors::SchemaError, a_string_including( + "cannot combine `proto`/`import` with `exclusions`, `expected_extras`, or `name_transform`", + "must stay generated locally" + )) + end + + it "raises when a referenced enum has multiple sources" do + # The multi-source validation raises before `.enums` is ever consulted. + proto_status_a = ::Object.new + proto_status_a.define_singleton_method(:enums) { [] } + proto_status_b = ::Object.new + proto_status_b.define_singleton_method(:enums) { [] } + + results = define_proto_schema_results do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE" + t.external_proto_enum proto_status_a, + proto: "myapp.types.Status", + import: "myapp/types/status.proto" + t.external_proto_enum proto_status_b + 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( + "must use exactly one `external_proto_enum` source", + "cannot be safely referenced externally" + )) + end + + it "raises when a referenced enum's external values do not match the ElasticGraph enum values" do + proto_status = ::Class.new do + def self.enums + [ + ::Data.define(:name, :number).new(name: :ACTIVE, number: 1), + ::Data.define(:name, :number).new(name: :PENDING, number: 2) + ] + end + end + + results = define_proto_schema_results do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE", "INACTIVE" + t.external_proto_enum proto_status, + proto: "myapp.types.Status", + import: "myapp/types/status.proto" + 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( + "External proto enum `Status` values do not match the ElasticGraph enum values", + "External values: ACTIVE, PENDING", + "ElasticGraph values: ACTIVE, INACTIVE" + )) + end + + it "raises when a referenced enum's entries do not expose value numbers" do + proto_status = ::Class.new do + def self.enums + [::Data.define(:name).new(name: :ACTIVE)] + end + end + + results = define_referenced_status_schema(proto_status, values: ["ACTIVE"]) + + expect { + results.proto_schema + }.to raise_error(Errors::SchemaError, a_string_including( + "must expose `.number` so its values can be verified against previously pinned numbers" + )) + end + + it "raises when a referenced enum changes a pinned value's number" do + proto_status = ::Class.new do + def self.enums + [ + ::Data.define(:name, :number).new(name: :ACTIVE, number: 2), + ::Data.define(:name, :number).new(name: :INACTIVE, number: 1) + ] + end + end + + results = define_referenced_status_schema( + proto_status, + values: ["ACTIVE", "INACTIVE"], + pinned_value_numbers: {"ACTIVE" => 1, "INACTIVE" => 2} + ) + + expect { + results.proto_schema + }.to raise_error(Errors::SchemaError, a_string_including( + "assigns `ACTIVE` the number 2, but previously dumped artifacts pin it to 1", + "silently reinterpret existing wire data" + )) + end + + it "raises when a referenced enum reuses a pinned number for a different value" do + proto_status = ::Class.new do + def self.enums + [ + ::Data.define(:name, :number).new(name: :ACTIVE, number: 1), + ::Data.define(:name, :number).new(name: :LEGACY, number: 2) + ] + end + end + + # `PAUSED` was removed from the enum; its number stays reserved in the artifact. + results = define_referenced_status_schema( + proto_status, + values: ["ACTIVE", "LEGACY"], + pinned_value_numbers: {"ACTIVE" => 1, "PAUSED" => 2} + ) + + expect { + results.proto_schema + }.to raise_error(Errors::SchemaError, a_string_including( + "assigns `LEGACY` the number 2, which previously dumped artifacts pin to `PAUSED`", + "silently reinterpret existing wire data" + )) + end + + it "wraps unexpected exceptions from referenced proto enum sources" do + proto_status = ::Class.new do + def self.enums + raise "boom" + end + end + + results = define_referenced_status_schema(proto_status, values: ["ACTIVE"]) + + expect { + results.proto_schema + }.to raise_error(Errors::SchemaError, a_string_including("Failed loading external proto enum values for `Status`")) + 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( @@ -494,6 +810,29 @@ module SchemaDefinition expect(results.proto_schema).to include("string id = 1;") expect(results.proto_schema).to include("string display_name = 2;") end + + private + + def define_referenced_status_schema(proto_status, values:, pinned_value_numbers: nil) + define_proto_schema_results do |s| + if pinned_value_numbers + s.configure_proto_field_number_mappings({"enums" => {"Status" => {"values" => pinned_value_numbers}}}) + end + + s.enum_type "Status" do |t| + t.values(*values) + t.external_proto_enum proto_status, + proto: "myapp.types.Status", + import: "myapp/types/status.proto" + end + + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "status", "Status" + t.index "accounts" + end + end + 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 610f91387..39460d6f0 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 @@ -780,6 +780,184 @@ module SchemaDefinition expect(proto_type_def_from(proto, "Command")).to include("COMMAND_OPTION = 1;", "COMMAND_STREAM = 2;") expect(proto_type_def_from(proto, "Request")).to include("string package_ = 2; // source name: package") end + + it "sources enum values from an external proto enum" do + proto_status = ::Class.new do + def self.enums + [ + ::Data.define(:name).new(name: :UNKNOWN_DO_NOT_USE), + ::Data.define(:name).new(name: :ACTIVE), + ::Data.define(:name).new(name: :INACTIVE) + ] + end + end + + proto = define_proto_schema do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE", "INACTIVE", "OBSOLETE" + t.external_proto_enum proto_status, + exclusions: [:UNKNOWN_DO_NOT_USE], + expected_extras: [:LEGACY] + end + + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "status", "Status" + t.index "accounts" + end + end + + generated = proto + expect(generated).to include("STATUS_ACTIVE = 1;") + expect(generated).to include("STATUS_INACTIVE = 2;") + expect(generated).to include("STATUS_LEGACY = 3;") + expect(generated).not_to include("OBSOLETE") + end + + it "applies `name_transform` to external proto enum value names" do + proto_currency = ::Class.new do + def self.enums + [ + ::Data.define(:name).new(name: :CURRENCY_USD), + ::Data.define(:name).new(name: :CURRENCY_CAD) + ] + end + end + + proto = define_proto_schema do |s| + s.enum_type "Currency" do |t| + t.values "USD", "CAD" + t.external_proto_enum proto_currency, name_transform: ->(name) { name.sub(/\ACURRENCY_/, "") } + end + + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "currency", "Currency" + t.index "accounts" + end + end + + generated = proto + expect(generated).to include("CURRENCY_USD = 1;") + expect(generated).to include("CURRENCY_CAD = 2;") + expect(generated).not_to include("CURRENCY_CURRENCY_") + end + + it "raises when external proto enum sources produce inconsistent values" do + proto_status_a = ::Class.new do + def self.enums + [ + ::Data.define(:name).new(name: :ACTIVE), + ::Data.define(:name).new(name: :INACTIVE) + ] + end + end + + proto_status_b = ::Class.new do + def self.enums + [ + ::Data.define(:name).new(name: :ACTIVE), + ::Data.define(:name).new(name: :PENDING) + ] + end + end + + proto_status_c = ::Class.new do + def self.enums + [ + ::Data.define(:name).new(name: :ACTIVE), + ::Data.define(:name).new(name: :INACTIVE) + ] + end + end + + results = define_proto_schema_results do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE", "INACTIVE" + t.external_proto_enum proto_status_a + t.external_proto_enum proto_status_b + t.external_proto_enum proto_status_c + 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( + "External proto enums for `Status` produce inconsistent value sets" + )) + end + + it "references an external proto enum type instead of generating a local enum" do + proto_status = ::Class.new do + def self.enums + [ + ::Data.define(:name, :number).new(name: :ACTIVE, number: 1), + ::Data.define(:name, :number).new(name: :INACTIVE, number: 2) + ] + end + end + + proto = define_proto_schema do |s| + s.enum_type "Status" do |t| + t.values "ACTIVE", "INACTIVE" + expect(t.proto_name).to eq("Status") + t.external_proto_enum proto_status, + proto: "myapp.types.Status", + import: "myapp/types/status.proto" + end + + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "status", "Status" + t.field "previous_status", "Status" + t.index "accounts" + end + end + + generated = proto + expect(generated.scan('import "myapp/types/status.proto";').size).to eq(1) + expect(generated).to include("myapp.types.Status status = 2;") + expect(generated).to include("myapp.types.Status previous_status = 3;") + expect(generated).not_to include("enum Status") + end + + it "accepts a referenced enum whose numbers match previously pinned enum value numbers" do + proto_status = ::Class.new do + def self.enums + [ + ::Data.define(:name, :number).new(name: :ACTIVE, number: 1), + ::Data.define(:name, :number).new(name: :INACTIVE, number: 2) + ] + end + end + + proto = define_proto_schema do |s| + s.configure_proto_field_number_mappings( + {"enums" => {"Status" => {"values" => {"ACTIVE" => 1, "INACTIVE" => 2}}}} + ) + + s.enum_type "Status" do |t| + t.values "ACTIVE", "INACTIVE" + t.external_proto_enum proto_status, + proto: "myapp.types.Status", + import: "myapp/types/status.proto" + end + + s.object_type "Account" do |t| + t.field "id", "ID" + t.field "status", "Status" + t.index "accounts" + end + end + + expect(proto).to include("myapp.types.Status status = 2;") + end end end end