From 2cdb436340147656444bddc2030ebad70b1267c2 Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Thu, 9 Jul 2026 10:52:21 -0500 Subject: [PATCH 1/3] Map DateTime to google.protobuf.Timestamp and document temporal string formats `DateTime` fields now use the well-known `google.protobuf.Timestamp` type (with `google/protobuf/timestamp.proto` imported automatically) instead of `string`. A Timestamp cannot be malformed the way a string can, and proto consumers get language-native timestamp types. Note that a Timestamp is a UTC instant, so a publisher's original UTC offset is not preserved; `t.protobuf type: "string"` remains available to override. To support this, `t.protobuf` gains two options usable by any scalar: - `import:` maps a scalar to an externally defined proto type, emitting the needed `import` statement in `schema.proto`. - `comment:` documents the expected format on each generated field, used by the built-in `string`-typed temporal scalars (`Date`, `LocalTime`, `TimeZone`) whose proto type is wider than the ElasticGraph type (e.g. `// ISO 8601 date`). Values are still validated at ingestion time, just as with JSON ingestion. --- elasticgraph-proto_ingestion/README.md | 95 ++++++-- .../schema_definition/schema.rb | 8 + .../schema_elements/enum_type_extension.rb | 8 + .../object_interface_and_union_extension.rb | 13 +- .../schema_elements/scalar_type_extension.rb | 71 ++++-- .../schema_definition/schema.rbs | 1 + .../schema_elements/enum_type_extension.rbs | 1 + .../object_interface_and_union_extension.rbs | 3 +- .../schema_elements/scalar_type_extension.rbs | 7 +- .../schema_definition/api_extension_spec.rb | 16 +- .../scalar_proto_types_spec.rb | 207 ++++++++++++++++++ 11 files changed, 385 insertions(+), 45 deletions(-) create mode 100644 elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/scalar_proto_types_spec.rb diff --git a/elasticgraph-proto_ingestion/README.md b/elasticgraph-proto_ingestion/README.md index 8b77d6a2c..aab5d8273 100644 --- a/elasticgraph-proto_ingestion/README.md +++ b/elasticgraph-proto_ingestion/README.md @@ -94,27 +94,92 @@ ElasticGraph.define_schema do |schema| end ``` +A custom scalar can also map to an externally defined proto type. Pass `import:` with the path of +the proto file that defines the type: + +```ruby +# in config/schema/duration.rb + +ElasticGraph.define_schema do |schema| + schema.scalar_type "Duration" do |t| + t.mapping type: "keyword" + t.json_schema type: "string" + t.protobuf type: "google.protobuf.Duration", import: "google/protobuf/duration.proto" + end +end +``` + +The generated `schema.proto` then contains `import "google/protobuf/duration.proto";`. ElasticGraph +emits the import only when a generated message uses the scalar. The `import:` value must be the +path of a `.proto` file. + +`comment:` documents the expected format on each generated field. This is useful when the proto +type is wider than the ElasticGraph type: + +```ruby +# in config/schema/phone_number.rb + +ElasticGraph.define_schema do |schema| + schema.scalar_type "PhoneNumber" do |t| + t.mapping type: "keyword" + t.json_schema type: "string" + t.protobuf type: "string", comment: "E.164 phone number" + end +end +``` + +A `PhoneNumber` field then renders as `string phone_number = 1; // E.164 phone number`. + +`comment:` renders as a trailing `//` comment on the field line, so it must be a single line. +Put multi-line documentation on the field's doc comment instead, which renders as `//` lines +above the field. + +### Overriding a Built-in Scalar + +Use `on_built_in_types` to change the protobuf type of a built-in scalar. For example, map +`DateTime` to `string` to keep the original UTC offset of each event: + +```ruby +# in config/schema/protobuf.rb + +ElasticGraph.define_schema do |schema| + schema.on_built_in_types do |type| + type.protobuf type: "string", comment: "ISO 8601 timestamp" if type.name == "DateTime" + end +end +``` + +The override also clears the built-in `import:` value, so `schema.proto` no longer imports +`google/protobuf/timestamp.proto`. + ## Type Mappings The generated `schema.proto` uses these built-in scalar mappings: -| ElasticGraph Type | Protobuf Type | -|-------------------|------------| -| `Boolean` | `bool` | -| `Cursor` | `string` | -| `Date` | `string` | -| `DateTime` | `string` | -| `Float` | `double` | -| `ID` | `string` | -| `Int` | `int32` | -| `JsonSafeLong` | `int64` | -| `LocalTime` | `string` | -| `LongString` | `int64` | -| `String` | `string` | -| `TimeZone` | `string` | -| `Untyped` | `string` | +| ElasticGraph Type | Protobuf Type | +|-------------------|-----------------------------| +| `Boolean` | `bool` | +| `Cursor` | `string` | +| `Date` | `string` | +| `DateTime` | `google.protobuf.Timestamp` | +| `Float` | `double` | +| `ID` | `string` | +| `Int` | `int32` | +| `JsonSafeLong` | `int64` | +| `LocalTime` | `string` | +| `LongString` | `int64` | +| `String` | `string` | +| `TimeZone` | `string` | +| `Untyped` | `string` | Additionally: +- `DateTime` uses the [well-known `Timestamp` type](https://protobuf.dev/reference/protobuf/google.protobuf/#timestamp); + `schema.proto` imports `google/protobuf/timestamp.proto` automatically. Note that a `Timestamp` + is a UTC instant, so a publisher's original UTC offset is not preserved. +- `string`-typed temporal scalars (`Date`, `LocalTime`, `TimeZone`) are wider than the + ElasticGraph types they carry, so generated fields of these types document the expected format + in a comment (e.g. `// ISO 8601 date, e.g. "2024-11-25"`). Values are validated when events + are ingested, just as with JSON ingestion. - List types become `repeated` fields. - Lists of lists (e.g. `[[Float!]!]!`) are not supported because Protocol Buffers cannot represent them directly. Schema artifact generation raises an error identifying the unsupported field. diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb index 9de3efb84..2fdb1c675 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 @@ -48,6 +48,7 @@ def to_proto sections = [ %(syntax = "proto3";), "package #{@package_name};", + *render_imports(types), render_definitions(types) ] @@ -107,6 +108,13 @@ def render_definitions(types) .join("\n\n") end + # Only scalar types can map to an externally defined proto type, so only they can require an import. + def render_imports(types) + imports = types.grep(SchemaElements::ScalarTypeExtension).filter_map(&:protobuf_import).uniq.sort + + imports.empty? ? [] : [imports.map { |import| %(import "#{import}";) }.join("\n")] + end + def validate_unique_enum_value_prefixes(types) enum_type_by_prefix = {} # : ::Hash[::String, untyped] 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 199e8eaec..590d99961 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 @@ -67,6 +67,14 @@ def proto_type_reference(package_name) ".#{package_name}.#{proto_name}" end + # Enum values are self-describing, so fields of this type get no trailing format comment. + # Only scalar types document a format. + # + # @return [nil] + def protobuf_comment + nil + end + # Returns the package-level prefix applied to this enum's protobuf values. # # @return [String] 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 30dbfc879..345f2a1fb 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 @@ -66,6 +66,14 @@ def proto_type_reference(package_name) ".#{package_name}.#{proto_name}" end + # Messages carry their documentation on the message definition itself, so fields of this + # type get no trailing format comment. Only scalar types document a format. + # + # @return [nil] + def protobuf_comment + nil + end + private def render_proto_message(schema, message_name, package_name) @@ -75,7 +83,7 @@ def render_proto_message(schema, message_name, package_name) active_field_names = fields.map { |schema_field, _| schema_field.name } documentation = ProtoDocumentation.comment_lines_for(doc_comment).map { |line| "#{line}\n" }.join field_definitions = fields.map do |schema_field, field| - repeated, field_type = proto_field_type_for( + repeated, field_type, type_comment = proto_field_type_for( field.type, package_name: package_name, context_field_name: field.name @@ -87,6 +95,7 @@ def render_proto_message(schema, message_name, package_name) ) label = "repeated " if repeated line = " #{label}#{field_type} #{schema_field.name} = #{field_number};" + line += " // #{type_comment}" if type_comment field_documentation = ProtoDocumentation .comment_lines_for(schema_field.doc_comment, indent: " ") .map { |comment_line| "#{comment_line}\n" } @@ -160,7 +169,7 @@ def proto_field_type_for(type_ref, package_name:, context_field_name:) end proto_type = _ = base_type_ref.resolved - [list_depth == 1, proto_type.proto_type_reference(package_name)] + [list_depth == 1, proto_type.proto_type_reference(package_name), proto_type.protobuf_comment] end end end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rb index 404258788..539f9571e 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 @@ -14,33 +14,64 @@ module SchemaDefinition module SchemaElements # Extends ScalarType with proto field type conversion. module ScalarTypeExtension - # Default protobuf types applied to ElasticGraph's built-in scalar types as they are constructed. - BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME = { - "Boolean" => "bool", - "Cursor" => "string", - "Date" => "string", - "DateTime" => "string", - "Float" => "double", - "ID" => "string", - "Int" => "int32", - "JsonSafeLong" => "int64", - "LocalTime" => "string", - "LongString" => "int64", - "String" => "string", - "TimeZone" => "string", - "Untyped" => "string" - }.freeze + # Default protobuf options applied to ElasticGraph's built-in scalar types as they are constructed. + BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME = { + "Boolean" => {type: "bool"}, + "Cursor" => {type: "string"}, + "Date" => {type: "string", comment: %(ISO 8601 date, e.g. "2024-11-25")}, + "DateTime" => {type: "google.protobuf.Timestamp", import: "google/protobuf/timestamp.proto"}, + "Float" => {type: "double"}, + "ID" => {type: "string"}, + "Int" => {type: "int32"}, + "JsonSafeLong" => {type: "int64"}, + "LocalTime" => {type: "string", comment: %(ISO 8601 local time, e.g. "14:23:12")}, + "LongString" => {type: "int64"}, + "String" => {type: "string"}, + "TimeZone" => {type: "string", comment: %(IANA time zone identifier, e.g. "America/Los_Angeles")}, + "Untyped" => {type: "string"} + }.freeze # : ::Hash[::String, {type: ::String, ?import: ::String, ?comment: ::String}] + + # An `import` is rendered as `import "PATH";`, so a quote or newline in the path would + # produce invalid proto. `protoc` also requires the path to name a `.proto` file. + VALID_PROTOBUF_IMPORT_PATH = %r{\A[\w./-]+\.proto\z} # Configured protobuf type (e.g. string, int64, bool). # @dynamic protobuf_type attr_reader :protobuf_type + # Proto file to import for the configured protobuf type, if it is externally defined. + # @dynamic protobuf_import + attr_reader :protobuf_import + + # Comment rendered on generated proto fields of this scalar type. + # @dynamic protobuf_comment + attr_reader :protobuf_comment + # Configures the protobuf type for this scalar type. # - # @param type [String] protobuf scalar type name + # @param type [String] protobuf type name + # @param import [String, nil] proto file to import for an externally defined type + # @param comment [String, nil] single-line comment rendered on generated fields of this type # @return [void] - def protobuf(type:) + # @raise [Errors::SchemaError] when `import` is not a `.proto` file path + # @raise [Errors::SchemaError] when `comment` spans multiple lines + def protobuf(type:, import: nil, comment: nil) + if import && !VALID_PROTOBUF_IMPORT_PATH.match?(import) + raise Errors::SchemaError, "`protobuf` import for `#{name}` must be the path of a `.proto` file, " \ + "but got: #{import.inspect}." + end + + # The comment is rendered as a trailing `// ...` on the field line, so a newline would + # push the remainder onto its own line as bare, invalid proto syntax. Multi-line prose + # belongs on the field's GraphQL doc comment, which renders as `//` lines above the field. + if comment&.include?("\n") + raise Errors::SchemaError, "`protobuf` comment for `#{name}` must be a single line, but got: #{comment.inspect}. " \ + "Use the field's doc comment for multi-line documentation." + end + @protobuf_type = type + @protobuf_import = import + @protobuf_comment = comment end # Applies any built-in protobuf type, yields for further configuration, and validates the result. @@ -50,8 +81,8 @@ def protobuf(type:) # @raise [Errors::SchemaError] when a protobuf type is missing def initialize_proto_extension original_name = type_ref.with_reverted_override.name - if (proto_type = BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME[original_name]) - protobuf type: proto_type + if (proto_options = BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME[original_name]) + protobuf(**proto_options) end yield 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 685c6df63..04bf21d86 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 @@ -34,6 +34,7 @@ module ElasticGraph def proto_types: () -> ::Array[untyped] def render_definitions: (::Array[untyped] types) -> ::String + def render_imports: (::Array[untyped] types) -> ::Array[::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]]] 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 6652855ba..7b356db66 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rbs @@ -11,6 +11,7 @@ module ElasticGraph def value: (::String) ?{ (::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & EnumValueExtension) -> void } -> void def proto_name: () -> ::String def proto_type_reference: (::String package_name) -> ::String + def protobuf_comment: () -> nil def proto_enum_value_prefix: () -> ::String def to_proto: (Schema schema, ::String package_name) -> ::String def referenced_proto_types: () -> ::Array[::ElasticGraph::SchemaDefinition::SchemaElements::graphQLType] diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rbs index 608f27a7d..fca9b6e4e 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,6 +10,7 @@ module ElasticGraph def proto_name: () -> ::String def proto_type_reference: (::String package_name) -> ::String + def protobuf_comment: () -> nil 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: ( @@ -28,7 +29,7 @@ module ElasticGraph ::ElasticGraph::SchemaDefinition::SchemaElements::TypeReference, package_name: ::String, context_field_name: ::String - ) -> [bool, ::String] + ) -> [bool, ::String, ::String?] end end end diff --git a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rbs b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rbs index 94f0dcf15..41de2bd80 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 @@ -3,11 +3,14 @@ module ElasticGraph module SchemaDefinition module SchemaElements module ScalarTypeExtension: ::ElasticGraph::SchemaDefinition::SchemaElements::ScalarType - BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME: ::Hash[::String, ::String] + BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME: ::Hash[::String, {type: ::String, ?import: ::String, ?comment: ::String}] + VALID_PROTOBUF_IMPORT_PATH: ::Regexp attr_reader protobuf_type: ::String? + attr_reader protobuf_import: ::String? + attr_reader protobuf_comment: ::String? - def protobuf: (type: ::String) -> void + def protobuf: (type: ::String, ?import: ::String?, ?comment: ::String?) -> void def initialize_proto_extension: () { () -> void } -> void def proto_name: () -> ::String def proto_type_reference: (::String package_name) -> ::String diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb index 44f0d986c..916a84db0 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb @@ -31,11 +31,12 @@ module SchemaDefinition expect(proto_type_def_from(proto, "Widget")).to include(".proto.package.v1.Address address = 2;") end - it "maps every built-in scalar to a proto field type" do + it "maps every built-in scalar to a proto field type, documenting the format of those that need it" do + built_in_scalar_options = SchemaElements::ScalarTypeExtension::BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME field_types = [] proto = define_proto_schema do |s| s.object_type "Widget" do |t| - SchemaElements::ScalarTypeExtension::BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME.each_key do |type_name| + built_in_scalar_options.each_key do |type_name| t.field type_name.downcase, type_name end field_types = t.graphql_fields_by_name.values.map { |field| field.type.name } @@ -43,9 +44,14 @@ module SchemaDefinition end end - expect(field_types).to match_array(SchemaElements::ScalarTypeExtension::BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME.keys) - SchemaElements::ScalarTypeExtension::BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME.each.with_index(1) do |(type_name, proto_type), field_number| - expect(proto).to include("#{proto_type} #{type_name.downcase} = #{field_number};") + expect(field_types).to match_array(built_in_scalar_options.keys) + + aggregate_failures do + built_in_scalar_options.each.with_index(1) do |(type_name, options), field_number| + field_line = "#{options.fetch(:type)} #{type_name.downcase} = #{field_number};" + comment = options[:comment] + expect(proto).to include(comment ? "#{field_line} // #{comment}" : field_line) + end end end diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/scalar_proto_types_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/scalar_proto_types_spec.rb new file mode 100644 index 000000000..b37d437cd --- /dev/null +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/scalar_proto_types_spec.rb @@ -0,0 +1,207 @@ +# Copyright 2024 - 2026 Block, Inc. +# +# Use of this source code is governed by an MIT-style +# license that can be found in the LICENSE file or at +# https://opensource.org/licenses/MIT. +# +# frozen_string_literal: true + +require "elastic_graph/proto_ingestion/schema_definition/api_extension" + +module ElasticGraph + module ProtoIngestion + module SchemaDefinition + # Covers how scalar types map to proto field types: the `type:`, `import:`, and `comment:` + # options of `protobuf`, and the imports and field comments they produce. + RSpec.describe Schema, "scalar proto types" do + it "maps `DateTime` fields to `google.protobuf.Timestamp`, importing its proto file once" do + proto = define_proto_schema do |s| + s.object_type "Event" do |t| + t.field "id", "ID" + t.field "created_at", "DateTime" + t.field "updated_at", "DateTime" + t.index "events" + end + end + + expect(proto).to eq(<<~PROTO) + syntax = "proto3"; + + package elasticgraph; + + import "google/protobuf/timestamp.proto"; + + message Event { + string id = 1; + google.protobuf.Timestamp created_at = 2; + google.protobuf.Timestamp updated_at = 3; + // Next field number: 4 + } + PROTO + end + + it "lets `on_built_in_types` override a built-in scalar's protobuf type, dropping its import" do + proto = define_proto_schema do |s| + s.on_built_in_types do |type| + type.protobuf type: "string", comment: "ISO 8601 timestamp" if type.name == "DateTime" + end + + s.object_type "Event" do |t| + t.field "id", "ID" + t.field "created_at", "DateTime" + t.index "events" + end + end + + expect(proto).to eq(<<~PROTO) + syntax = "proto3"; + + package elasticgraph; + + message Event { + string id = 1; + string created_at = 2; // ISO 8601 timestamp + // Next field number: 3 + } + PROTO + end + + it "sorts and de-duplicates imports shared by distinct protobuf types" do + proto = define_proto_schema do |s| + s.scalar_type "FirstZType" do |t| + t.mapping type: "keyword" + t.protobuf type: "example.FirstZType", import: "z/types.proto" + end + + s.scalar_type "SecondZType" do |t| + t.mapping type: "keyword" + t.protobuf type: "example.SecondZType", import: "z/types.proto" + end + + s.scalar_type "AType" do |t| + t.mapping type: "keyword" + t.protobuf type: "example.AType", import: "a/types.proto" + end + + s.object_type "Event" do |t| + t.field "id", "ID" + t.field "first_z", "FirstZType" + t.field "second_z", "SecondZType" + t.field "a", "AType" + t.index "events" + end + end + + expect(proto.lines.grep(/\Aimport /).map(&:chomp)).to eq([ + %(import "a/types.proto";), + %(import "z/types.proto";) + ]) + end + + it "imports only the proto files of scalar types the generated messages reference" do + proto = define_proto_schema do |s| + s.scalar_type "UsedType" do |t| + t.mapping type: "keyword" + t.protobuf type: "example.UsedType", import: "used.proto" + end + + s.scalar_type "UnusedType" do |t| + t.mapping type: "keyword" + t.protobuf type: "example.UnusedType", import: "unused.proto" + end + + s.scalar_type "GraphQLOnlyFieldType" do |t| + t.mapping type: "keyword" + t.protobuf type: "example.GraphQLOnlyFieldType", import: "graphql_only_field.proto" + end + + s.object_type "Event" do |t| + t.field "id", "ID" + t.field "used", "UsedType" + t.field "graphql_only", "GraphQLOnlyFieldType", graphql_only: true + t.index "events" + end + end + + expect(proto.lines.grep(/\Aimport /).map(&:chomp)).to eq([%(import "used.proto";)]) + end + + it "renders the format comment after the field number, below any doc comment" do + proto = define_proto_schema do |s| + s.object_type "Person" do |t| + t.field "id", "ID" + t.field "important_dates", "[Date!]!" do |f| + f.documentation "The dates that matter to this person." + end + t.index "people" + end + end + + expect(proto_type_def_from(proto, "Person")).to eq(<<~PROTO.strip) + message Person { + string id = 1; + // The dates that matter to this person. + repeated string important_dates = 2; // ISO 8601 date, e.g. "2024-11-25" + // Next field number: 3 + } + PROTO + end + + it "renders the `import:` and `comment:` configured on a custom scalar type" do + proto = define_proto_schema do |s| + s.scalar_type "Money" do |t| + t.mapping type: "keyword" + t.protobuf type: "myapp.types.Money", import: "my-app/types/v1.money.proto", comment: "amount + currency" + end + + s.object_type "Order" do |t| + t.field "id", "ID" + t.field "total", "Money" + t.index "orders" + end + end + + expect(proto).to include('import "my-app/types/v1.money.proto";') + expect(proto).to include("myapp.types.Money total = 2; // amount + currency") + end + + it "rejects an `import:` that is not the path of a `.proto` file" do + invalid_imports = [ + "", # empty + "myapp/types/money", # no `.proto` extension + "myapp/types/money.protobuf", # extension is not exactly `.proto` + %(myapp/types/money.proto"; import "evil.proto), # closes the rendered `import "...";` + %(myapp/types/money.proto\nimport "evil.proto") # adds a line to the rendered proto + ] + + # `protobuf` raises as soon as it is called, so no indexed type is needed here. + aggregate_failures do + invalid_imports.each do |import| + expect { + define_proto_schema do |s| + s.scalar_type "Money" do |t| + t.mapping type: "keyword" + t.protobuf type: "myapp.types.Money", import: import + end + end + }.to raise_error(Errors::SchemaError, a_string_including( + "`protobuf` import for `Money` must be the path of a `.proto` file" + )) + end + end + end + + it "rejects a multi-line `comment:`, which would emit its later lines as bare proto syntax" do + expect { + define_proto_schema do |s| + s.scalar_type "Money" do |t| + t.mapping type: "keyword" + t.protobuf type: "myapp.types.Money", comment: "amount + currency\nin minor units" + end + end + }.to raise_error(Errors::SchemaError, a_string_including("`protobuf` comment for `Money` must be a single line")) + end + end + end + end +end From cdf5c6ef31bd9a95e8125869c0a001dcb0a72fea Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Tue, 18 Aug 2026 20:12:39 -0500 Subject: [PATCH 2/3] Rename `comment:` to `field_comment:` and render it above the field Addresses review feedback on #1306. Proto compilers attach a field's leading comments to the code they generate for that field, while a trailing comment on the field line is usually discarded. Render the format comment above the field instead, below the field's own doc comment and separated from it by a blank `//` line. This also removes the reason the comment had to be a single line, so a multi-line `field_comment:` is now allowed. Rename the option to `field_comment:`, since a scalar type has no proto representation of its own to comment on. The comment applies to each field of that type. Reword the built-in temporal comments to read well above the field (e.g. `Must be formatted as an ISO 8601 date`). Also: - Document that each `protobuf` call replaces the full configuration, so an override that omits `import:` or `field_comment:` clears the value a prior call set. Cover both directions with specs. - Give enum and object types a `protobuf_import` method that returns `nil`, so rendering imports no longer needs to grep for scalar types and non-scalar types can start requiring an import without a change there. Co-Authored-By: Claude Opus 5 --- elasticgraph-proto_ingestion/README.md | 32 ++++--- .../schema_definition/schema.rb | 6 +- .../schema_elements/enum_type_extension.rb | 11 ++- .../object_interface_and_union_extension.rb | 36 +++++--- .../schema_elements/scalar_type_extension.rb | 33 +++---- .../schema_elements/enum_type_extension.rbs | 3 +- .../object_interface_and_union_extension.rbs | 4 +- .../schema_elements/scalar_type_extension.rbs | 6 +- .../schema_definition/api_extension_spec.rb | 6 +- .../scalar_proto_types_spec.rb | 90 +++++++++++++++---- 10 files changed, 156 insertions(+), 71 deletions(-) diff --git a/elasticgraph-proto_ingestion/README.md b/elasticgraph-proto_ingestion/README.md index aab5d8273..079c657be 100644 --- a/elasticgraph-proto_ingestion/README.md +++ b/elasticgraph-proto_ingestion/README.md @@ -113,8 +113,8 @@ The generated `schema.proto` then contains `import "google/protobuf/duration.pro emits the import only when a generated message uses the scalar. The `import:` value must be the path of a `.proto` file. -`comment:` documents the expected format on each generated field. This is useful when the proto -type is wider than the ElasticGraph type: +`field_comment:` documents the expected format on each generated field. This is useful when the +proto type is wider than the ElasticGraph type: ```ruby # in config/schema/phone_number.rb @@ -123,16 +123,23 @@ ElasticGraph.define_schema do |schema| schema.scalar_type "PhoneNumber" do |t| t.mapping type: "keyword" t.json_schema type: "string" - t.protobuf type: "string", comment: "E.164 phone number" + t.protobuf type: "string", field_comment: "Must be an E.164 phone number." end end ``` -A `PhoneNumber` field then renders as `string phone_number = 1; // E.164 phone number`. +A `PhoneNumber` field then renders as: -`comment:` renders as a trailing `//` comment on the field line, so it must be a single line. -Put multi-line documentation on the field's doc comment instead, which renders as `//` lines -above the field. +```protobuf +// Must be an E.164 phone number. +string phone_number = 1; +``` + +The comment goes above the field, below the field's own doc comment, because proto compilers +attach these leading comments to the code they generate for the field. A `field_comment:` can +span multiple lines. The option is named `field_comment:` rather than `comment:` because a scalar +type has no proto representation of its own to comment on; the comment applies to each field of +that type. ### Overriding a Built-in Scalar @@ -144,13 +151,14 @@ Use `on_built_in_types` to change the protobuf type of a built-in scalar. For ex ElasticGraph.define_schema do |schema| schema.on_built_in_types do |type| - type.protobuf type: "string", comment: "ISO 8601 timestamp" if type.name == "DateTime" + type.protobuf type: "string", field_comment: "Must be formatted as an ISO 8601 timestamp." if type.name == "DateTime" end end ``` -The override also clears the built-in `import:` value, so `schema.proto` no longer imports -`google/protobuf/timestamp.proto`. +Each call to `protobuf` replaces the full protobuf configuration. The override above omits +`import:`, so `schema.proto` no longer imports `google/protobuf/timestamp.proto`. An override that +omits `field_comment:` likewise drops the built-in comment. ## Type Mappings @@ -178,8 +186,8 @@ Additionally: is a UTC instant, so a publisher's original UTC offset is not preserved. - `string`-typed temporal scalars (`Date`, `LocalTime`, `TimeZone`) are wider than the ElasticGraph types they carry, so generated fields of these types document the expected format - in a comment (e.g. `// ISO 8601 date, e.g. "2024-11-25"`). Values are validated when events - are ingested, just as with JSON ingestion. + in a comment above the field (e.g. `// Must be formatted as an ISO 8601 date, e.g. "2024-11-25".`). + Values are validated when events are ingested, just as with JSON ingestion. - List types become `repeated` fields. - Lists of lists (e.g. `[[Float!]!]!`) are not supported because Protocol Buffers cannot represent them directly. Schema artifact generation raises an error identifying the unsupported field. diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema.rb index 2fdb1c675..dca5db8d7 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 @@ -108,9 +108,11 @@ def render_definitions(types) .join("\n\n") end - # Only scalar types can map to an externally defined proto type, so only they can require an import. + # Every type reports the proto file it needs imported, or `nil` when it needs none. Today only + # scalar types map to an externally defined proto type, but enum and object types can start + # requiring an import without any change here. def render_imports(types) - imports = types.grep(SchemaElements::ScalarTypeExtension).filter_map(&:protobuf_import).uniq.sort + imports = types.filter_map(&:protobuf_import).uniq.sort imports.empty? ? [] : [imports.map { |import| %(import "#{import}";) }.join("\n")] 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 590d99961..94b2b1242 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 @@ -67,11 +67,18 @@ def proto_type_reference(package_name) ".#{package_name}.#{proto_name}" end - # Enum values are self-describing, so fields of this type get no trailing format comment. + # Enum types render their own protobuf definition, so they never require an import. + # + # @return [nil] + def protobuf_import + nil + end + + # Enum values are self-describing, so fields of this type get no format comment. # Only scalar types document a format. # # @return [nil] - def protobuf_comment + def protobuf_field_comment nil end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/object_interface_and_union_extension.rb index 345f2a1fb..8ae2a116b 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 @@ -66,11 +66,18 @@ def proto_type_reference(package_name) ".#{package_name}.#{proto_name}" end + # Messages render their own protobuf definition, so they never require an import. + # + # @return [nil] + def protobuf_import + nil + end + # Messages carry their documentation on the message definition itself, so fields of this - # type get no trailing format comment. Only scalar types document a format. + # type get no format comment. Only scalar types document a format. # # @return [nil] - def protobuf_comment + def protobuf_field_comment nil end @@ -83,7 +90,7 @@ def render_proto_message(schema, message_name, package_name) active_field_names = fields.map { |schema_field, _| schema_field.name } documentation = ProtoDocumentation.comment_lines_for(doc_comment).map { |line| "#{line}\n" }.join field_definitions = fields.map do |schema_field, field| - repeated, field_type, type_comment = proto_field_type_for( + repeated, field_type, field_comment = proto_field_type_for( field.type, package_name: package_name, context_field_name: field.name @@ -95,13 +102,9 @@ def render_proto_message(schema, message_name, package_name) ) label = "repeated " if repeated line = " #{label}#{field_type} #{schema_field.name} = #{field_number};" - line += " // #{type_comment}" if type_comment - field_documentation = ProtoDocumentation - .comment_lines_for(schema_field.doc_comment, indent: " ") - .map { |comment_line| "#{comment_line}\n" } - .join + comment_lines = field_comment_lines_for(schema_field.doc_comment, field_comment) - "#{field_documentation}#{line}" + [*comment_lines, line].join("\n") 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}." @@ -159,6 +162,19 @@ def proto_fields end end + # Renders a field's documentation and its type's format comment as the `//` lines that go + # above the field. Proto compilers attach these leading comments to the code they generate + # for the field, whereas a trailing comment on the field line is usually discarded. + def field_comment_lines_for(doc_comment, field_comment) + doc_lines = ProtoDocumentation.comment_lines_for(doc_comment, indent: " ") + return doc_lines unless field_comment + + format_lines = ProtoDocumentation.comment_lines_for(field_comment, indent: " ") + return format_lines if doc_lines.empty? + + doc_lines + [" //"] + format_lines + end + def proto_field_type_for(type_ref, package_name:, context_field_name:) list_depth, base_type_ref = ObjectInterfaceAndUnionExtension.list_depth_and_base_type(type_ref) @@ -169,7 +185,7 @@ def proto_field_type_for(type_ref, package_name:, context_field_name:) end proto_type = _ = base_type_ref.resolved - [list_depth == 1, proto_type.proto_type_reference(package_name), proto_type.protobuf_comment] + [list_depth == 1, proto_type.proto_type_reference(package_name), proto_type.protobuf_field_comment] end end end diff --git a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rb b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rb index 539f9571e..2a0808f4c 100644 --- a/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rb +++ b/elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rb @@ -18,18 +18,18 @@ module ScalarTypeExtension BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME = { "Boolean" => {type: "bool"}, "Cursor" => {type: "string"}, - "Date" => {type: "string", comment: %(ISO 8601 date, e.g. "2024-11-25")}, + "Date" => {type: "string", field_comment: %(Must be formatted as an ISO 8601 date, e.g. "2024-11-25".)}, "DateTime" => {type: "google.protobuf.Timestamp", import: "google/protobuf/timestamp.proto"}, "Float" => {type: "double"}, "ID" => {type: "string"}, "Int" => {type: "int32"}, "JsonSafeLong" => {type: "int64"}, - "LocalTime" => {type: "string", comment: %(ISO 8601 local time, e.g. "14:23:12")}, + "LocalTime" => {type: "string", field_comment: %(Must be formatted as an ISO 8601 local time, e.g. "14:23:12".)}, "LongString" => {type: "int64"}, "String" => {type: "string"}, - "TimeZone" => {type: "string", comment: %(IANA time zone identifier, e.g. "America/Los_Angeles")}, + "TimeZone" => {type: "string", field_comment: %(Must be an IANA time zone identifier, e.g. "America/Los_Angeles".)}, "Untyped" => {type: "string"} - }.freeze # : ::Hash[::String, {type: ::String, ?import: ::String, ?comment: ::String}] + }.freeze # : ::Hash[::String, {type: ::String, ?import: ::String, ?field_comment: ::String}] # An `import` is rendered as `import "PATH";`, so a quote or newline in the path would # produce invalid proto. `protoc` also requires the path to name a `.proto` file. @@ -43,35 +43,28 @@ module ScalarTypeExtension # @dynamic protobuf_import attr_reader :protobuf_import - # Comment rendered on generated proto fields of this scalar type. - # @dynamic protobuf_comment - attr_reader :protobuf_comment + # Comment rendered above each generated proto field of this scalar type. + # @dynamic protobuf_field_comment + attr_reader :protobuf_field_comment - # Configures the protobuf type for this scalar type. + # Configures the protobuf type for this scalar type. Each call replaces the full protobuf + # configuration, so an override that omits `import:` or `field_comment:` clears the value + # configured by a prior call. # # @param type [String] protobuf type name # @param import [String, nil] proto file to import for an externally defined type - # @param comment [String, nil] single-line comment rendered on generated fields of this type + # @param field_comment [String, nil] comment rendered above each generated field of this type # @return [void] # @raise [Errors::SchemaError] when `import` is not a `.proto` file path - # @raise [Errors::SchemaError] when `comment` spans multiple lines - def protobuf(type:, import: nil, comment: nil) + def protobuf(type:, import: nil, field_comment: nil) if import && !VALID_PROTOBUF_IMPORT_PATH.match?(import) raise Errors::SchemaError, "`protobuf` import for `#{name}` must be the path of a `.proto` file, " \ "but got: #{import.inspect}." end - # The comment is rendered as a trailing `// ...` on the field line, so a newline would - # push the remainder onto its own line as bare, invalid proto syntax. Multi-line prose - # belongs on the field's GraphQL doc comment, which renders as `//` lines above the field. - if comment&.include?("\n") - raise Errors::SchemaError, "`protobuf` comment for `#{name}` must be a single line, but got: #{comment.inspect}. " \ - "Use the field's doc comment for multi-line documentation." - end - @protobuf_type = type @protobuf_import = import - @protobuf_comment = comment + @protobuf_field_comment = field_comment end # Applies any built-in protobuf type, yields for further configuration, and validates the result. 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 7b356db66..851195247 100644 --- a/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rbs +++ b/elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/enum_type_extension.rbs @@ -11,7 +11,8 @@ module ElasticGraph def value: (::String) ?{ (::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & EnumValueExtension) -> void } -> void def proto_name: () -> ::String def proto_type_reference: (::String package_name) -> ::String - def protobuf_comment: () -> nil + def protobuf_import: () -> nil + def protobuf_field_comment: () -> nil def proto_enum_value_prefix: () -> ::String def to_proto: (Schema schema, ::String package_name) -> ::String def referenced_proto_types: () -> ::Array[::ElasticGraph::SchemaDefinition::SchemaElements::graphQLType] 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 fca9b6e4e..8c2535187 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,8 @@ module ElasticGraph def proto_name: () -> ::String def proto_type_reference: (::String package_name) -> ::String - def protobuf_comment: () -> nil + def protobuf_import: () -> nil + def protobuf_field_comment: () -> nil 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: ( @@ -25,6 +26,7 @@ module ElasticGraph ::ElasticGraph::SchemaDefinition::SchemaElements::Field, ::ElasticGraph::SchemaDefinition::Indexing::Field ]] + def field_comment_lines_for: (::String? doc_comment, ::String? field_comment) -> ::Array[::String] def proto_field_type_for: ( ::ElasticGraph::SchemaDefinition::SchemaElements::TypeReference, package_name: ::String, 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 41de2bd80..a33bd4e7e 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 @@ -3,14 +3,14 @@ module ElasticGraph module SchemaDefinition module SchemaElements module ScalarTypeExtension: ::ElasticGraph::SchemaDefinition::SchemaElements::ScalarType - BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME: ::Hash[::String, {type: ::String, ?import: ::String, ?comment: ::String}] + BUILT_IN_SCALAR_PROTO_OPTIONS_BY_NAME: ::Hash[::String, {type: ::String, ?import: ::String, ?field_comment: ::String}] VALID_PROTOBUF_IMPORT_PATH: ::Regexp attr_reader protobuf_type: ::String? attr_reader protobuf_import: ::String? - attr_reader protobuf_comment: ::String? + attr_reader protobuf_field_comment: ::String? - def protobuf: (type: ::String, ?import: ::String?, ?comment: ::String?) -> void + def protobuf: (type: ::String, ?import: ::String?, ?field_comment: ::String?) -> void def initialize_proto_extension: () { () -> void } -> void def proto_name: () -> ::String def proto_type_reference: (::String package_name) -> ::String diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb index 916a84db0..96e57ad60 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb @@ -48,9 +48,9 @@ module SchemaDefinition aggregate_failures do built_in_scalar_options.each.with_index(1) do |(type_name, options), field_number| - field_line = "#{options.fetch(:type)} #{type_name.downcase} = #{field_number};" - comment = options[:comment] - expect(proto).to include(comment ? "#{field_line} // #{comment}" : field_line) + field_line = " #{options.fetch(:type)} #{type_name.downcase} = #{field_number};" + field_comment = options[:field_comment] + expect(proto).to include(field_comment ? " // #{field_comment}\n#{field_line}" : field_line) end end end diff --git a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/scalar_proto_types_spec.rb b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/scalar_proto_types_spec.rb index b37d437cd..613f3d419 100644 --- a/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/scalar_proto_types_spec.rb +++ b/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/scalar_proto_types_spec.rb @@ -11,7 +11,7 @@ module ElasticGraph module ProtoIngestion module SchemaDefinition - # Covers how scalar types map to proto field types: the `type:`, `import:`, and `comment:` + # Covers how scalar types map to proto field types: the `type:`, `import:`, and `field_comment:` # options of `protobuf`, and the imports and field comments they produce. RSpec.describe Schema, "scalar proto types" do it "maps `DateTime` fields to `google.protobuf.Timestamp`, importing its proto file once" do @@ -40,10 +40,12 @@ module SchemaDefinition PROTO end - it "lets `on_built_in_types` override a built-in scalar's protobuf type, dropping its import" do + it "lets `on_built_in_types` replace a built-in scalar's protobuf type, import, and field comment" do proto = define_proto_schema do |s| s.on_built_in_types do |type| - type.protobuf type: "string", comment: "ISO 8601 timestamp" if type.name == "DateTime" + # `protobuf` replaces the full configuration, so omitting `import:` here drops the + # built-in `google/protobuf/timestamp.proto` import. + type.protobuf type: "string", field_comment: "Must be formatted as an ISO 8601 timestamp." if type.name == "DateTime" end s.object_type "Event" do |t| @@ -60,7 +62,37 @@ module SchemaDefinition message Event { string id = 1; - string created_at = 2; // ISO 8601 timestamp + // Must be formatted as an ISO 8601 timestamp. + string created_at = 2; + // Next field number: 3 + } + PROTO + end + + it "drops a built-in scalar's field comment when an override omits `field_comment:`" do + proto = define_proto_schema do |s| + s.on_built_in_types do |type| + # `Date` has a built-in `field_comment:` and no `import:`; this override inverts both. + type.protobuf type: "google.type.Date", import: "google/type/date.proto" if type.name == "Date" + end + + s.object_type "Event" do |t| + t.field "id", "ID" + t.field "occurred_on", "Date" + t.index "events" + end + end + + expect(proto).to eq(<<~PROTO) + syntax = "proto3"; + + package elasticgraph; + + import "google/type/date.proto"; + + message Event { + string id = 1; + google.type.Date occurred_on = 2; // Next field number: 3 } PROTO @@ -126,13 +158,14 @@ module SchemaDefinition expect(proto.lines.grep(/\Aimport /).map(&:chomp)).to eq([%(import "used.proto";)]) end - it "renders the format comment after the field number, below any doc comment" do + it "renders the field comment above the field, after any doc comment" do proto = define_proto_schema do |s| s.object_type "Person" do |t| t.field "id", "ID" t.field "important_dates", "[Date!]!" do |f| f.documentation "The dates that matter to this person." end + t.field "time_zone", "TimeZone" t.index "people" end end @@ -141,17 +174,21 @@ module SchemaDefinition message Person { string id = 1; // The dates that matter to this person. - repeated string important_dates = 2; // ISO 8601 date, e.g. "2024-11-25" - // Next field number: 3 + // + // Must be formatted as an ISO 8601 date, e.g. "2024-11-25". + repeated string important_dates = 2; + // Must be an IANA time zone identifier, e.g. "America/Los_Angeles". + string time_zone = 3; + // Next field number: 4 } PROTO end - it "renders the `import:` and `comment:` configured on a custom scalar type" do + it "renders the `import:` and `field_comment:` configured on a custom scalar type" do proto = define_proto_schema do |s| s.scalar_type "Money" do |t| t.mapping type: "keyword" - t.protobuf type: "myapp.types.Money", import: "my-app/types/v1.money.proto", comment: "amount + currency" + t.protobuf type: "myapp.types.Money", import: "my-app/types/v1.money.proto", field_comment: "Amount and currency." end s.object_type "Order" do |t| @@ -162,7 +199,7 @@ module SchemaDefinition end expect(proto).to include('import "my-app/types/v1.money.proto";') - expect(proto).to include("myapp.types.Money total = 2; // amount + currency") + expect(proto).to include(" // Amount and currency.\n myapp.types.Money total = 2;") end it "rejects an `import:` that is not the path of a `.proto` file" do @@ -191,15 +228,34 @@ module SchemaDefinition end end - it "rejects a multi-line `comment:`, which would emit its later lines as bare proto syntax" do - expect { - define_proto_schema do |s| - s.scalar_type "Money" do |t| - t.mapping type: "keyword" - t.protobuf type: "myapp.types.Money", comment: "amount + currency\nin minor units" + it "renders a multi-line `field_comment:` as multiple comment lines" do + proto = define_proto_schema do |s| + s.scalar_type "Money" do |t| + t.mapping type: "keyword" + t.protobuf type: "string", field_comment: "Must be an amount and a currency.\n\nThe amount is in minor units." + end + + s.object_type "Order" do |t| + t.field "id", "ID" + t.field "total", "Money" do |f| + f.documentation "What the customer owes." end + t.index "orders" end - }.to raise_error(Errors::SchemaError, a_string_including("`protobuf` comment for `Money` must be a single line")) + end + + expect(proto_type_def_from(proto, "Order")).to eq(<<~PROTO.strip) + message Order { + string id = 1; + // What the customer owes. + // + // Must be an amount and a currency. + // + // The amount is in minor units. + string total = 2; + // Next field number: 3 + } + PROTO end end end From 996b54ea938e8085a4d2b0337c371c6fe20ce680 Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Tue, 18 Aug 2026 20:50:54 -0500 Subject: [PATCH 3/3] Display protobuf README snippets instead of failing on them `script/validate_readme_snippets` had no validator for the `protobuf` snippet type, so the fallback validator failed the build as soon as `elasticgraph-proto_ingestion/README.md` gained a ```protobuf block. Register a `ProtobufSnippetValidator` that displays the snippet and reports it as unvalidated, the same way `text` snippets are handled. Most protobuf snippets in our READMEs are excerpts of a generated `schema.proto` rather than complete proto files, so `protoc` cannot compile them on their own. Co-Authored-By: Claude Opus 5 --- .../protobuf_snippet_validator.rb | 21 +++++++++++++++++++ .../readme_snippet_validator.rb | 4 +++- 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 script/readme_snippets/protobuf_snippet_validator.rb diff --git a/script/readme_snippets/protobuf_snippet_validator.rb b/script/readme_snippets/protobuf_snippet_validator.rb new file mode 100644 index 000000000..e583124c6 --- /dev/null +++ b/script/readme_snippets/protobuf_snippet_validator.rb @@ -0,0 +1,21 @@ +# 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 + +# Displays protobuf snippets without validating them. Most protobuf snippets in our READMEs are +# excerpts of a generated `schema.proto` (a few fields of one message, say) rather than complete +# proto files, so `protoc` cannot compile them on their own. +class ProtobufSnippetValidator < SnippetValidator + def validate(snippet) + puts " 📝 Protobuf content (unvalidated):" + snippet.content.lines.each_with_index do |line, idx| + puts " #{idx + 1}: #{line}" + end + + ValidationResult.unvalidated("Protobuf snippet displayed (no validation performed)") + end +end diff --git a/script/readme_snippets/readme_snippet_validator.rb b/script/readme_snippets/readme_snippet_validator.rb index 768a15dd9..9d3849bcd 100644 --- a/script/readme_snippets/readme_snippet_validator.rb +++ b/script/readme_snippets/readme_snippet_validator.rb @@ -17,6 +17,7 @@ require_relative "yaml_snippet_validator" require_relative "text_snippet_validator" require_relative "mermaid_snippet_validator" +require_relative "protobuf_snippet_validator" require_relative "fallback_snippet_validator" class ReadmeSnippetValidator @@ -202,7 +203,8 @@ def with_temp_project "bash" => BashSnippetValidator.new(project, verbose_output), "yaml" => YamlSnippetValidator.new(project, verbose_output), "text" => TextSnippetValidator.new(project, verbose_output), - "mermaid" => MermaidSnippetValidator.new(project, verbose_output) + "mermaid" => MermaidSnippetValidator.new(project, verbose_output), + "protobuf" => ProtobufSnippetValidator.new(project, verbose_output) } # Fallback validator for unknown snippet types