Source enum values from external proto enums and reference external enum types by jwils · Pull Request #1286 · block/elasticgraph

jwils · 2026-07-01T20:55:30Z

Why

Some schemas already have canonical protobuf enum definitions. Generating duplicate enums in schema.proto makes Java consumers deal with parallel types, and maintaining enum value lists in two places invites drift.

What

Adds external_proto_enum on enum types, per the API inversion suggested in review — configuration lives on the enum type itself instead of a global hash imported from application code:

schema.enum_type "Currency" do |t|
  t.values "USD", "CAD"
  t.external_proto_enum MyApp::Protos::Currency,
    exclusions: [:UNKNOWN_DO_NOT_USE],
    name_transform: ->(name) { name.sub(/\ACURRENCY_/, "") }
end

Commit 1 — value sourcing: the generated proto enum's values come from the proto enum class (anything exposing .enums), honoring exclusions, expected_extras, and name_transform keyword args. Multiple sources may be registered and must agree.
Commit 2 — external references: passing proto: and import: goes further — schema.proto imports the named file and references the existing enum type instead of generating a local duplicate. A referenced enum must have exactly one option-free source whose values match the ElasticGraph enum's values, so transformed or curated enums stay generated locally.

Because the API lives on enum types and options are real keyword arguments, several things from the previous revision disappear entirely: the global-hash normalization and string/symbol key handling, the extension state field, and two validation error cases that are now impossible by construction (an external reference without a value source, and referencing a non-enum type).

Risk Assessment

Low — only affects the unreleased elasticgraph-proto_ingestion extension, and both features are opt-in with eager validation.

References

Stacked on Support proto2 syntax and custom proto file headers #1305 (→ Pin protobuf field and enum value numbers in a sidecar artifact #1304 → Generate a protobuf schema artifact from ElasticGraph schemas #1080)
Addresses Generate a protobuf schema artifact from ElasticGraph schemas #1080 (comment)
Replaces Reference external proto enum types #1265, which was auto-closed when a rebase temporarily collapsed this branch into its base

## Why - continue the pluggable ingestion serializer proposal after pulling JSON Schema into its own gem - revive the earlier protobuf prototype from #1056 on top of the new serializer extension points ## What - fill in the `elasticgraph-proto_ingestion` extension gem with core generation: `schema_artifacts:dump` emits a `proto3` `schema.proto` covering the schema's indexed types - map built-in ElasticGraph scalars to proto types, with `t.protobuf type:` for custom scalars (resolved via `type_ref.with_reverted_override` so built-ins renamed with `type_name_overrides` keep working) - generate messages for object/interface/union types and enums (with a zero-valued `*_UNSPECIFIED` entry), escaping proto reserved words and wrapping lists of lists so the output stays valid - keep `schema.proto` on public GraphQL field names; validate proto package names - hold extension state on a `ProtoIngestionState` container behind a single `proto_ingestion_state` reader (matching #1281) Field and enum value numbers are assigned sequentially in definition order in this PR; the stacked follow-up adds the `proto_field_numbers.yaml` sidecar that keeps them wire-stable across schema evolution. ## Stacked follow-ups 1. this PR — core `schema.proto` generation 2. wire-stable field/enum value numbers via a `proto_field_numbers.yaml` sidecar 3. `syntax: :proto2` support and custom file-level `headers:` 4. #1286 — enum value sourcing from existing proto enums + external proto type references ## Verification - `script/run_gem_specs elasticgraph-proto_ingestion` (100% line + branch coverage at this commit) - `script/type_check`, `script/lint`, `script/spellcheck` - `script/quick_build` green at the stack head (whose tree is identical to the previously reviewed single-PR revision) ## References - #1059 - #1056 - #1079 ## Update — 2026-07-10 - The current stack is #1080 → #1304 → #1306 → #1305 → #1286. - Proto extension state now uses a mutable Struct, and keyword package-name segments are validated without being rewritten. - Lists of lists now raise an actionable schema error instead of generating wrapper messages; this supersedes the earlier wrapping note above. --- ## Update — 2026-07-15 - Interface and union messages now wrap concrete subtype messages in a `oneof`, matching the JSON Schema `oneOf` representation. - Concrete subtype messages omit the redundant `__typename` discriminator. - Proto type rendering is now stateless: the generator selects the reachable type graph up front, then each extended type renders itself without mutating shared traversal state. - No-block extension coverage now completes the definitions so the fixture remains valid under CI GraphQL-schema validation. --- ## Update — 2026-07-19 - Replaced keyword suffixing with fully qualified local message and enum references, preserving source type and field names while disambiguating contextual protobuf words and built-in scalar names. - Removed the now-unnecessary keyword collision tracking and verified a generated schema containing contextual names with `protoc` 35.1.

…num types

myronmarston · 2026-08-20T00:15:22Z

    end

+    doctest.before "ElasticGraph::ProtoIngestion::SchemaDefinition::SchemaElements::EnumTypeExtension#external_proto_enum" do
+      extend ::RSpec::Mocks::ExampleMethods


It's not safe to just include the RSpec mocks methods--you also have to call its setup/teardown so that it cleans up mocks/stubs properly:

https://rspec.info/features/3-12/rspec-mocks/outside-rspec/any-test-framework/

In particular, your use of stub_const below leaks.

Done in 2b141e7.

myronmarston · 2026-08-20T05:14:47Z

+
+ElasticGraph.define_schema do |schema|
+  schema.enum_type "Currency" do |t|
+    t.values "USD", "CAD"


Above you say:

you can source an enum's generated proto values from them instead of maintaining the value list in two places.

Based on that, I thought the point of this was to not need to define the values--instead, it would get them from the externally defined proto.

Am I misunderstanding what this is for? Or should the t.values be removed here?

myronmarston · 2026-08-20T05:15:41Z

+      # 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_/, "") }


Suggested change

name_transform: ->(name) { name.sub(/\ACURRENCY_/, "") }

name_transform: ->(name) { name.delete_prefix("CURRENCY_") }

(A bit more readable, IMO).

Done in 2b141e7.

myronmarston · 2026-08-20T05:17:34Z

+      # 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],


expected_extras makes since in a test validation context but I'm not sure it makes sense here. If someone wants extra enum values that aren't defined on the proto...can't they just call t.value? Why do we need expected_extras? And what does it even do?

myronmarston · 2026-08-20T05:18:27Z

+
+`name_transform` runs first, and `exclusions` and `expected_extras` then apply to the
+transformed names. In the example above the exclusion is therefore `UNKNOWN_DO_NOT_USE`, the
+name left after the transform strips `CURRENCY_`, and not `CURRENCY_UNKNOWN_DO_NOT_USE`.


Suggested change

name left after the transform strips `CURRENCY_`, and not `CURRENCY_UNKNOWN_DO_NOT_USE`.

name left after the transform strips `CURRENCY_`, rather than `CURRENCY_UNKNOWN_DO_NOT_USE`.

Done in 2b141e7.

myronmarston · 2026-08-20T05:20:53Z

+
+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. Note that `MyApp::Protos::Currency` from the


value curation remains explicit

I don't know what this means.

Done in 2b141e7.

myronmarston · 2026-08-20T05:23:11Z

+ElasticGraph enum's values; transformed, curated, or multi-source enums stay generated
+locally so value curation remains explicit. Note that `MyApp::Protos::Currency` from the
+previous section cannot be referenced this way: its `CURRENCY_`-prefixed names only match
+after a transform, and referenced enums allow no transform.


I don't understand this last sentence.

Also, I thought that EG natively generates enum values with a prefix like CURRENCY_. For example, see:

elasticgraph/elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/schema_spec.rb

Lines 67 to 74 in b98f719

enum Status {

// The default value when no enum value has been explicitly set. Do not use this value.

// See https://protobuf.dev/programming-guides/proto3/#enum-default.

STATUS_UNSPECIFIED = 0;

// The account is active.

STATUS_ACTIVE = 1;

STATUS_INACTIVE = 2;

// Next value number: 3

Given that, why is the CURRENCY_ prefixing a problem?

myronmarston · 2026-08-20T05:24:51Z

+after a transform, and referenced enums allow no transform.
+
+The source's enum entries must also expose `.number`. Those numbers are recorded in
+`proto_field_numbers.yaml`, and must agree with any numbers already pinned there — otherwise


If we're referencing an existing proto enum, then we should treat it as the canonical source of truth for the enum value numbers. Recording them in proto_field_numbers.yaml feels like it could let them conflict.

myronmarston · 2026-08-20T05:26:07Z

+          proto_status = ::Class.new do
+            def self.enums
+              [
+                ::Data.define(:name, :number).new(name: :ACTIVE, number: 1),


::Data.define(:name, :number) defines a new class. It's wasteful to define a new class every time we need an object with name and number.

Instead, we should define ProtoEnumValue = ::Data.define(...) somewhere (maybe in support?) and then we can use it from all these places.

Done in 2b141e7.

myronmarston · 2026-08-20T05:27:01Z

+                ::Data.define(:name).new(name: :INACTIVE)
+              ]
+            end
+          end


Instead of dynamically defining a class for these protos can we define a Data class in support for them? e.g. ProtoEnumType = ::Data.define(:enums).

Done in 2b141e7.

…prove README wording, extract shared proto enum Data classes - doctest_helper.rb: Replace stub_const with real module definitions and an after hook that calls remove_const, eliminating the rspec-mocks lifecycle from doctests entirely (fixes stub_const leak flagged by Myron) - README + enum_type_extension.rb + schema_spec.rb: Replace sub(/\ACURRENCY_/, ) with delete_prefix("CURRENCY_") in all 4 copies for consistency - README: Rewrite "one option-free source" as the concrete "exactly one external_proto_enum call that passes no exclusions:, expected_extras:, or name_transform:" - README: Cut "value curation remains explicit" - README: Improve wording: "rather than" instead of "and not" - proto_schema_support.rb: Define ProtoEnumEntry, ProtoEnumEntryNameOnly, and ProtoEnum as shared Data classes for use across all proto_ingestion specs - schema_spec.rb + schema_edge_cases_spec.rb: Replace all inline ::Data.define(:name) / ::Data.define(:name, :number) and ::Class.new blocks with the shared constants, reducing class allocation churn

jwils requested review from BrianSigafoos-SQ, bsorbo, ellisandrews-toast, jwondrusch, marcdaniels-toast, myronmarston and rossroberts-toast as code owners July 1, 2026 20:55

jwils mentioned this pull request Jul 1, 2026

Generate a protobuf schema artifact from ElasticGraph schemas #1080

Merged

jwils force-pushed the joshuaw/protobuf-external-types branch from 723577d to 362dde9 Compare July 5, 2026 15:44

jwils force-pushed the joshuaw/protobuf-external-types branch from 362dde9 to b140cc6 Compare July 7, 2026 15:14

jwils force-pushed the joshuaw/protobuf-external-types branch from b140cc6 to 61d2c86 Compare July 7, 2026 15:27

jwils force-pushed the joshuaw/protobuf-external-types branch from 61d2c86 to 06b8a2b Compare July 7, 2026 16:22

jwils force-pushed the joshuaw/protobuf-external-types branch from 06b8a2b to a2e1ff0 Compare July 7, 2026 16:23

jwils force-pushed the joshuaw/protobuf-external-types branch from a2e1ff0 to cd2b49f Compare July 7, 2026 16:50

jwils force-pushed the joshuaw/protobuf-external-types branch from cd2b49f to f3f5f66 Compare July 7, 2026 18:02

jwils force-pushed the joshuaw/protobuf-external-types branch from f3f5f66 to 8cd283f Compare July 8, 2026 22:59

jwils changed the title ~~Reference external proto enum types~~ Source enum values from existing proto enums and reference external enum types Jul 8, 2026

jwils force-pushed the joshuaw/protobuf-external-types branch from dd523aa to 593342e Compare July 11, 2026 13:40

jwils force-pushed the joshuaw/protobuf-external-types branch from 593342e to ec4819c Compare July 11, 2026 13:54

jwils force-pushed the joshuaw/protobuf-external-types branch from ec4819c to 7a122a7 Compare July 11, 2026 14:00

jwils force-pushed the joshuaw/protobuf-external-types branch 2 times, most recently from 4d94fd9 to 529dcb5 Compare July 11, 2026 14:24

jwils force-pushed the joshuaw/protobuf-external-types branch 2 times, most recently from 170323f to 5f29c5a Compare July 16, 2026 14:21

jwils force-pushed the joshuaw/protobuf-external-types branch from 5f29c5a to 6fc64b7 Compare July 17, 2026 15:05

jwils force-pushed the joshuaw/protobuf-external-types branch from 6fc64b7 to 2b39fb5 Compare July 31, 2026 03:46

jwils force-pushed the joshuaw/protobuf-external-types branch from 2b39fb5 to b565b3b Compare July 31, 2026 13:31

jwils force-pushed the joshuaw/protobuf-external-types branch from b565b3b to 9fb4336 Compare August 3, 2026 15:52

jwils force-pushed the joshuaw/protobuf-external-types branch from 9fb4336 to 0521685 Compare August 3, 2026 15:57

This was referenced Aug 15, 2026

Use Buf to guard protobuf schema compatibility #1352

Draft

Split the proto_ingestion schema spec into cohesive files #1358

Open

Source enum values from external proto enums and reference external e…

0d0fcbf

…num types

myronmarston previously requested changes Aug 20, 2026

View reviewed changes

	name_transform: ->(name) { name.sub(/\ACURRENCY_/, "") }
	name_transform: ->(name) { name.delete_prefix("CURRENCY_") }

	name left after the transform strips `CURRENCY_`, and not `CURRENCY_UNKNOWN_DO_NOT_USE`.
	name left after the transform strips `CURRENCY_`, rather than `CURRENCY_UNKNOWN_DO_NOT_USE`.

	enum Status {
	// The default value when no enum value has been explicitly set. Do not use this value.
	// See https://protobuf.dev/programming-guides/proto3/#enum-default.
	STATUS_UNSPECIFIED = 0;
	// The account is active.
	STATUS_ACTIVE = 1;
	STATUS_INACTIVE = 2;
	// Next value number: 3

Uh oh!

Conversation

jwils commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What

Risk Assessment

References

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jwils commented Jul 1, 2026 •

edited

Loading