Generate a protobuf schema artifact from ElasticGraph schemas#1080
Generate a protobuf schema artifact from ElasticGraph schemas#1080jwils wants to merge 1 commit into
Conversation
2e0e4a1 to
53ea492
Compare
a3cfa5c to
9e6f39a
Compare
4325d51 to
fd8dd47
Compare
9e6f39a to
c408a50
Compare
fd8dd47 to
bdce7fb
Compare
c408a50 to
5717334
Compare
0607029 to
40815e1
Compare
6591c29 to
751db75
Compare
b2e8d34 to
6969f84
Compare
070849e to
ce625bc
Compare
e9902f1 to
f069179
Compare
5efeb79 to
7d428d5
Compare
7d428d5 to
9acfed5
Compare
9acfed5 to
4fe8938
Compare
4fe8938 to
474788d
Compare
474788d to
bcc30c1
Compare
Moves `field.renamed_from`, `type.renamed_from`, `type.deleted_field`, and `schema.deleted_type` — along with the state registries and `DeprecatedElement` record they populate — from `elasticgraph-json_ingestion` back into the core schema definition gem, partially reverting the placement chosen in #1259. These declarations record serializer-neutral facts about how a schema has evolved, and we now have a second consumer: `elasticgraph-proto_ingestion` (#1080) needs rename metadata to keep protobuf field numbers stable across renames (with `reserved` field numbers for deletions as a natural follow-up). Keeping the markers inside one serializer forced other consumers to duck-type against state that may or may not be present; with the declarations in core, every consumer reads typed `State` members directly. This also addresses the concern raised in review of #1259 about `elasticgraph-json_ingestion` bolting 9 fields onto `::ElasticGraph::SchemaDefinition::State`: its `StateExtension` now adds only the 5 genuinely JSON-specific fields, and the four deprecation registries are ordinary typed `State` members that need no `State & StateExtension` casts. All JSON-specific behavior — schema version enforcement, merge reporting, pruning, and the warnings that flag no-longer-needed declarations — stays in `elasticgraph-json_ingestion`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
98f82ce to
683be9a
Compare
myronmarston
left a comment
There was a problem hiding this comment.
Not done reviewing but wanted to submit what I have so far.
| ``` | ||
|
|
||
| When a mapping exists for an enum, `elasticgraph-proto_ingestion` uses the mapped proto enum(s) | ||
| as the source of enum values (respecting `exclusions`, `expected_extras`, and `name_transform`). |
There was a problem hiding this comment.
EG today doesn't have any concept of proto enum mappings, and doesn't have any logic to support exclusions, expected_extras or name_transform. Instead of importing an existing data structure designed for tests within Block internal code, can we invert this?
- Design and implement whatever features make sense for the
elasticgraph-proto_ingestiongem as a stand-alone, open source library. - Update the internal testing support inside Block to leverage what
elasticgraph-proto_ingestionprovides.
For example, instead of it being provided as a global data structure, maybe we can do something like:
schema.enum_type "Currency" do |t|
t.external_proto_enum "v1.money.Currency", # ... plus other options as needed like `exclusions`, etc.
endWith that said, I don't yet understand how this gem uses this mapping...I haven't gotten that far.
There was a problem hiding this comment.
Makes sense. Going to move enums out of this pr so we can review this independently.
| RSpec.configure do |config| | ||
| config.when_first_matching_example_defined(:proto_schema) do | ||
| require "support/proto_schema_support" | ||
| end |
There was a problem hiding this comment.
You can also think about applying this tag automatically to specs under elasticgraph-proto_ingestion/spec, using this technique:
elasticgraph/elasticgraph-json_ingestion/spec/spec_helper.rb
Lines 16 to 18 in 4cdc065
| | `Boolean` | `bool` | | ||
| | `Cursor` | `string` | | ||
| | `Date` | `string` | | ||
| | `DateTime` | `string` | |
There was a problem hiding this comment.
Should we use https://protobuf.dev/reference/protobuf/google.protobuf/#timestamp for this?
| |-------------------|------------| | ||
| | `Boolean` | `bool` | | ||
| | `Cursor` | `string` | | ||
| | `Date` | `string` | |
There was a problem hiding this comment.
One potential concern is that string is a much wider type than Date. Something like 0.0001% of valid strings are valid ISO8601 Date strings.
Have you given thought to how invalid values will get handled? With JSON schema we could specify validations for this kind of thing.
|
|
||
| Additionally: | ||
| - List types become `repeated` fields. | ||
| - Nested list types generate wrapper messages so the output remains valid `proto3`. |
There was a problem hiding this comment.
I'm not following this.
schema.object_type "Team" do |t|
t.field "players", "[Player!]!" do |f|
f.mapping type: "nested"
end
t.field "seasons", "[Season!]!" do |f|
f.mapping type: "object"
end
endIs this saying that a wrapper message type will be generated for the players field but not the seasons field? Why would that be? And what would the proto look like?
There was a problem hiding this comment.
nested as in the list of lists not the mapping type, so the issue exists for both player and seasons. you can't do repeated repeated for a field. Updating test.
Fills in the `elasticgraph-proto_ingestion` gem with the core Protocol Buffers schema generation logic. Running `schema_artifacts:dump` emits a `proto3` `schema.proto` artifact covering the schema's indexed types. Capabilities: - Maps built-in ElasticGraph scalars to proto types, with `protobuf` to configure custom scalars (resolved via `type_ref.with_reverted_override` so built-in scalars renamed with `type_name_overrides` keep working). - Generates 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. - `schema.proto` uses the public GraphQL field names, and validates proto package names (each dot-separated segment must be a valid identifier). Field and enum value numbers are assigned sequentially in definition order; keeping them wire-stable across schema evolution comes in a stacked follow-up that adds a `proto_field_numbers.yaml` sidecar artifact. The extension's schema definition state lives on a ProtoIngestionState container exposed via a single `proto_ingestion_state` reader, mirroring the JSONIngestionState approach from #1281.
Note
First of a four-PR stack. Supporting changes merged earlier (#1296 artifact comment prefixes, #1297 gem scaffold), so this diff is contained entirely to the
elasticgraph-proto_ingestiondirectory. Per review feedback the original PR has been split: wire-stable field/enum-value numbering, proto2 + custom headers, and enum sourcing/external references are now stacked follow-ups.Why
What
elasticgraph-proto_ingestionextension gem with core generation:schema_artifacts:dumpemits aproto3schema.protocovering the schema's indexed typest.protobuf type:for custom scalars (resolved viatype_ref.with_reverted_overrideso built-ins renamed withtype_name_overrideskeep working)*_UNSPECIFIEDentry), escaping proto reserved words and wrapping lists of lists so the output stays validschema.protoon public GraphQL field names; validate proto package namesProtoIngestionStatecontainer behind a singleproto_ingestion_statereader (matching Group JSON ingestion schema state #1281)Field and enum value numbers are assigned sequentially in definition order in this PR; the stacked follow-up adds the
proto_field_numbers.yamlsidecar that keeps them wire-stable across schema evolution.Stacked follow-ups
schema.protogenerationproto_field_numbers.yamlsidecarsyntax: :proto2support and custom file-levelheaders:Verification
script/run_gem_specs elasticgraph-proto_ingestion(100% line + branch coverage at this commit)script/type_check,script/lint,script/spellcheckscript/quick_buildgreen at the stack head (whose tree is identical to the previously reviewed single-PR revision)References