Pin protobuf field and enum value numbers in a sidecar artifact - #1304
Pin protobuf field and enum value numbers in a sidecar artifact#1304jwils wants to merge 5 commits into
Conversation
67b8198 to
f62a859
Compare
90e2af5 to
616dbfd
Compare
616dbfd to
74011e5
Compare
075e749 to
8d6e03f
Compare
8d6e03f to
0a476c1
Compare
7a57c87 to
26fe805
Compare
26fe805 to
8803f5e
Compare
## 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.
dc9a3cb to
156369c
Compare
|
|
||
| it "uses public field names in schema.proto when `name_in_index` differs" do | ||
| proto = define_proto_schema do |s| | ||
| it "assigns the lowest unused field number to a new field, rather than the number after the maximum used one" do |
There was a problem hiding this comment.
I'm concerned about this--it has the potential to re-use an old field number that used to be used by another field that has been deleted.
If the logic grabs the lowest unused field number, what is the mechanism to prevent "recycling" old field numbers (which causes problems when deserializing old messages)?
Related to this, one thing that's unclear to me is what happens to deleted fields. Do they stick around in the artifact forever?
There was a problem hiding this comment.
Yes, right now they stick around in the artifact forever so it should prevent reusing numbers from deleted artifacts.
I imagine we could improve how it works some, want me to explore that? For example today deleted fields are stored in the yaml file but wouldn't be output to the proto. We could output them as reserved numbers.
There was a problem hiding this comment.
Ah another catch, re-added field with the same name as a deleted one silently reclaims its old number for potentially different data.
There was a problem hiding this comment.
Ah another catch, re-added field with the same name as a deleted one silently reclaims its old number for potentially different data.
I think it's just as likely that a field was mistakenly removed and then the removal got reverted. I'd expect that to be more common than an old field name getting reused for some entirely new piece of data.
I don't think that EG has a way to know what the user intends for this case. Ultimately, the user can control it through hand-editing the proto numbers file (and the field reuse will be visible in the .proto file diff). I lean towards EG re-using the old field number because it's probably the more common case and is hopefully simpler.
That said, there's one specific case where reuse of an old field name could cause a problem: when the field type changes in a way that's incompatible with the old field type. https://protobuf.dev/programming-guides/proto3/#conditionally-safe-changes discusses what sort of type changes are compatible vs incompatible...but I don't think we want to maintain logic that understands that. Also, a type change isn't just a problem when an old, removed field name gets re-used; someone can directly change a field from one type to another, and it could be incompatible. A couple options:
- We could store the field type metadata in the field numbers file and each time a field changes type assign it a new number.
- If the user knows its a compatible change, they can hand-edit the numbers file and re-dump to preserve the original number. Or if they are still prototyping they ca just delete the numbers file and redump to get a fresh set of numbers.
- We could use the
bufCLI to detect breaking changes.
I think we should use buf at some point anyway (we can use it as the basis for forcing a proto_schema_version to be bumped, kinda like how json_schema_version works today...). For now, I'd like to avoid overcomplicating things. And I definitely don't want to make this PR bigger with a bunch of new logic. Maybe punt on this for now?
156369c to
ec29566
Compare
ec29566 to
e8b37a2
Compare
Today, every schema artifact is a pure function of the schema definition: the schema artifacts directory can safely be deleted and regenerated at any time. In #1304 (comment), @myronmarston pointed out that `proto_field_numbers.yaml` doesn't fit that model--it's an _input_ to `schema.proto` generation, not a pure output--and suggested treating it as part of the schema definition, stored as a sibling of `path_to_schema`, rather than as a schema artifact. For an extension to maintain a file there, schema definition state needs to know where the schema definition lives. `RakeTasks` already knows (it's how the schema gets loaded) but never passed it into the API. This PR stores `path_to_schema` on `SchemaDefinition::State`, allowing extensions to access it without expanding `SchemaArtifactManager`'s dependencies. Nothing in core uses the value yet, so there are no behavior changes to the core artifacts. #1304 builds on this to relocate `proto_field_numbers.yaml`. ## Update — 2026-07-29 After review, the path is carried by schema definition state rather than an extension-only `SchemaArtifactManager` instance variable. The manager continues to accept the narrower schema-definition results object.
4b5acdf to
bbe754f
Compare
myronmarston
left a comment
myronmarston
left a comment
There was a problem hiding this comment.
Not done reviewing but wanted to submit my feedback so far.
| end | ||
| end | ||
|
|
||
| expect(results2.proto_schema).to include("string id = 1;", "string name = 3;") |
There was a problem hiding this comment.
Should we explicitly reserve field numbers?
https://protobuf.dev/programming-guides/proto3/#fieldreserved
| }) | ||
| end | ||
|
|
||
| it "uses public field names in schema.proto and stores name_in_index overrides in the mapping artifact" do |
There was a problem hiding this comment.
These tests demonstrate storing name_in_index in the mapping, but how is that used later? If it's used when evolving the schema as part of dumping the proto, can we demonstrate it in a test rather than just asserting that it's dumped?
(OTOH, if it's dumped for use at ingestion time, it's fine to just assert on its presence in the dumped mapping).
bbe754f to
92bff82
Compare
myronmarston
left a comment
myronmarston
left a comment
There was a problem hiding this comment.
not done but submitting my feedback so far
| # messages and enums sorted by name and their fields and values sorted by number. | ||
| # | ||
| # @return [Hash<String, Object>] | ||
| def to_artifact |
There was a problem hiding this comment.
Elsewhere in this codebase we call this to_dumpable_hash. Mind renaming to match?
https://github.com/search?q=repo%3Ablock%2Felasticgraph+to_dumpable_hash&type=code
|
|
||
| [enum_name, parsed_values] | ||
| end | ||
| end |
There was a problem hiding this comment.
There's a ton of validation code. Instead of writing it by hand can you just define a JSON schema for it and use that? We migrated our config classes fro hand-validation to JSON schema and it worked quite well.
(And once you do that you can probably remove much of the tests for the various invalid cases--once the schema is declared and there's not hand-written validation logic, a single "validates against the JSON schema" test is sufficient").
Field and enum value numbers were assigned sequentially in definition order, so reordering or removing a field renumbered everything after it — breaking wire compatibility with previously serialized data. `schema_artifacts:dump` now reads and writes a `proto_field_numbers.yaml` sidecar artifact: - Existing numbers stay fixed even if field order changes; new fields get the lowest unused numbers. - `schema.proto` keeps the public GraphQL field names while the sidecar stores private `name_in_index` overrides. - A field renamed with `field.renamed_from` reuses its existing number under the new public name. - Enum value numbers are pinned in an `enums` section; removed values keep their numbers reserved so they are never reused (`0` remains the generated `*_UNSPECIFIED` value). - The sidecar is safe to hand-edit but strictly validated: unknown keys, non-integer numbers, out-of-range numbers, and collisions raise clear errors instead of silently reassigning numbers. The parsed mappings are modeled by a `FieldNumberMappings` class that owns conversion to and from the dumped artifact format.
Protobuf field-number state only needs public wire names and stable\nnumbers. Leave public-to-index name translation to record preparation,\nso every field mapping has the same simple integer representation.
Store a per-message allocation cursor beside the stable field mappings.\nNew fields advance from that cursor instead of reclaiming gaps that may\nhave belonged to fields absent from a hand-edited mapping.
0ddfe16 to
11fc5c1
Compare
2 participants
Why
With #1080's sequential numbering, reordering or removing a field renumbers everything after it — breaking wire compatibility with previously serialized data. Protobuf field numbers must stay stable across schema evolution.
What
schema_artifacts:dumpnow reads and writes aproto_field_numbers.yamlsidecar artifact: existing numbers stay fixed even if field order changes, and new fields allocate from each message's storednext_numberwithout backfilling earlier gapsschema.protoand the sidecar both use public GraphQL field names;name_in_indexremains an indexing-time concernfield.renamed_fromreuses its existing number under the new public nameenumssection; removed values keep their numbers reserved so they are never reused (0remains the generated*_UNSPECIFIEDvalue)Integer()silently truncateRisk Assessment
Low — only affects the unreleased
elasticgraph-proto_ingestionextension from #1080.References
Update — 2026-07-15
oneofalternatives now use the same stable message-field map: existing and removed subtype tags remain reserved, while new subtypes receive the next available tag.elasticgraph-proto_ingestionexamples pass with 100% line and branch coverage; GraphQL-schema validation, lint, spelling, and Steep are green.Update — 2026-07-29
path_to_schemato schema definition state #1316, which exposespath_to_schemathrough schema definition state without expandingSchemaArtifactManager's dependencies.elasticgraph-proto_ingestionexamples pass with 100% line and branch coverage; lint and Steep are green.Update — 2026-08-07
public_field_name: number, with noname_in_indexmetadata.next_numbercursor; new fields advance from that cursor instead of filling gaps, and older mapping files derive the cursor from their maximum mapped number.Indexer::RecordPreparer, which owns recursive public-to-index name translation and scalar preparation.elasticgraph-proto_ingestionexamples pass with 100% line and branch coverage; lint, spelling, and Steep are green.Update Aug 8, 12:35 CDT: Addressed the current review feedback.
next_numbercursors; cursor-less entries are invalid.name_in_indexfrom protobuf wire mappings, reserve removed fields and enum values in generated protos, and validate the sidecar with JSON Schema.from_parsed_yaml,FromYamlFile, andto_dumpable_hash, reduced allocation mutation, and grouped the mapping specs by behavior.elasticgraph-proto_ingestionexamples pass with 100% line and branch coverage; all 1,583elasticgraph-schema_definitionexamples pass with 100% coverage; lint and Steep are green.Update Aug 8, 12:59 CDT: Simplified JSON Schema validation coverage.
Update Aug 8, 13:57 CDT: Rebased the full four-PR stack onto main at 3256b4c.