Pluggable ingestion serializers with json_ingestion - #1079
Conversation
d29bd5d to
a3cfa5c
Compare
9e6f39a to
c408a50
Compare
myronmarston
left a comment
There was a problem hiding this comment.
This is a great start but I think there's a lot more to do here. If the JSON schema ingestion support is no longer part of elasticgraph-schema_definition (now that it lives in elasticgraph-json_schema) then elasticgraph-json_schema needs to be solely responsible for all aspects of the JSON schema artifact.
Here's a non-exhaustive list of things that are still in elasticgraph-schema_definition that should be moved:
- https://github.com/block/elasticgraph/blob/main/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/json_schema_pruner.rb
- https://github.com/block/elasticgraph/blob/main/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/json_schema_spec.rb
- The
to_json_schemapolymorphic API onindexing/field_typeclasses.
I'm sure there's a lot more than that but that'll get you started. the way I'm thinking about this: when someone uses ElasticGraph with only protobuf ingestion in the future, and doesn't even install the elasticgraph-json_schema gem, their collection of ElasticGraph gems should have no JSON-schema related logic, in a similar fashion to how we're not going to add protobuf directly to elasticgraph-schema_definition--it'll be provided by an extesnion.
A good way to approach this is to grep for json_schema in elasticgraph-schema_definition. Any mention of json_schema in elasticgraph-schema_definition should probably be moved. (Maybe some mentions of json without -schema, too, although there are some legit mentions of JSON that will remain like JSONSafeLong).
c408a50 to
5717334
Compare
23c3ead to
0607029
Compare
0607029 to
40815e1
Compare
myronmarston
left a comment
There was a problem hiding this comment.
Sorry for the delayed review--this is massive! (But also exciting to see...). On top of my inline feedback below, I've got some meta feedback:
- The fact that this reworks both the tests and the implementation makes it hard to have confidence that it preserves JSON schema behavior (and also makes it a very large PR!). I'd like to see this split:
- First, extract the implementation into
elasticgraph-json_ingestionwhile leavingelasticgraph-schema_definition/specchanged as little as possible. Leaving it unchanged gives us confidence that the extraction produces equivalent behavior. You'll probably have to updateelasticgraph-schema_definition/spec/spec_helper.rbto have it (for now) automatically useelasticgraph-json_ingestionin all its tests so that the existing tests can continue to pass. - Secondly, move the tests from
elasticgraph-schema_definitionintoelasticgraph-json_ingestion. - Even with that split into 2 steps, the PRs will still be massive. If you see some further opportunity to split into smaller PRs, please take the opportunity to do that--I'm happy to review a whole stack of PRs.
- First, extract the implementation into
- There's still a ton of
json_schemareferences inelasticgraph_schema_definition. Can you audit these? It would be good to confirm that every reference tojson_schemaneeds to remain. I'll plan to do an audit in a future review as well (once you've let me know that you've finished your review).- ...but obviously the audit can be done at the end on top of the final PR in your (eventual) stack.
0cd8153 to
c2bcd2b
Compare
2d3764a to
63fe627
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
63fe627 to
709e591
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.
Summary
elasticgraph-json_ingestiongem while keeping it loaded by default for backward compatibility.Motivation
Per discussion #1059, protobuf ingestion should be able to plug into the same schema-definition pipeline without keeping JSON ingestion hardcoded in core. This PR narrows that first step to extraction and extension wiring for the existing JSON ingestion behavior.
What moved where
API#json_schema_version,API#json_schema_strictness, built-in JSON schema setupElasticGraph::JSONIngestion::SchemaDefinition::APIExtensionElasticGraph::JSONIngestion::SchemaDefinition::SchemaElements::*ExtensionIndexing::Field,FieldReference,FieldType::*, andIndexJSON schema behaviorElasticGraph::JSONIngestion::SchemaDefinition::Indexing::*ExtensionElasticGraph::JSONIngestion::SchemaDefinition::SchemaArtifactManagerExtensionEventEnvelope,JSONSchemaWithMetadata,JSONSchemaFieldMetadata,JSONSchemaPrunerelasticgraph-json_ingestionWhat stays in core
enforce_json_schema_version, which the JSON ingestion extension still uses.Test plan
script/lintscript/type_checkscript/spellcheckenv -u COVERAGE BUNDLE_GEMFILE=elasticgraph-schema_definition/Gemfile bundle exec rspec elasticgraph-schema_definition/spec/unit --fail-fast --format progressenv -u COVERAGE BUNDLE_GEMFILE=elasticgraph-apollo/Gemfile bundle exec rspec elasticgraph-apollo/spec/unit --fail-fast --format progressscript/run_gem_specs elasticgraph-json_ingestion(skips because the extracted gem intentionally has no spec directory yet)