Move deprecated schema markers to JSON ingestion - #1259
Merged
Conversation
jwils
marked this pull request as ready for review
June 29, 2026 17:05
jwils
requested review from
BrianSigafoos-SQ,
bsorbo,
ellisandrews-toast,
jwondrusch,
marcdaniels-toast,
myronmarston and
rossroberts-toast
as code owners
June 29, 2026 17:05
myronmarston
approved these changes
Jun 30, 2026
myronmarston
left a comment
myronmarston
left a comment
Collaborator
There was a problem hiding this comment.
LGTM! Left one suggestion that is probably best addressed in a separate PR if we decide to do it.
jwils
enabled auto-merge (squash)
June 30, 2026 21:52
jwils
added a commit
that referenced
this pull request
Jul 7, 2026
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>
jwils
added a commit
that referenced
this pull request
Jul 7, 2026
…on (#1294) ## Why `elasticgraph-proto_ingestion` (#1080) is a second consumer of the schema-evolution declarations: it needs `field.renamed_from` metadata to keep protobuf field numbers stable across public field renames, and `type.deleted_field` maps directly onto protobuf `reserved` field numbers as a natural follow-up. With the markers living inside `elasticgraph-json_ingestion` (where #1259 moved them), any other consumer has to duck-type against state that may or may not be present (`state.respond_to?(:renamed_fields_by_type_name_and_old_field_name)`), and proto-only projects can't declare renames at all. These declarations record serializer-neutral facts about how a schema has evolved, so this PR restores them to core `elasticgraph-schema_definition` (where they lived prior to #1259), partially reverting #1259's placement — the situation has changed now that two serializers consume them. This also addresses [the concern raised in review of #1259](#1259 (comment)) about `elasticgraph-json_ingestion` bolting 9 fields onto `::ElasticGraph::SchemaDefinition::State` with `State & StateExtension` casts: its `StateExtension` now adds only the 5 genuinely JSON-specific fields, and the four deprecation registries are ordinary typed `State` members that need no casts. ## What - Move `DeprecatedElement` to `ElasticGraph::SchemaDefinition::SchemaElements::DeprecatedElement`. - Add the four registries (`renamed_types_by_old_name`, `deleted_types_by_old_name`, `renamed_fields_by_type_name_and_old_field_name`, `deleted_fields_by_type_name_and_old_field_name`) and their `register_*` methods to core `State`, always initialized. - Move the DSL — `field.renamed_from`, `type.renamed_from`, `type.deleted_field`, `schema.deleted_type` — onto core `Field`/`TypeWithSubfields`/`API`, with docs reworded to be serializer-neutral. - `elasticgraph-json_ingestion` keeps everything actually JSON-specific (schema version enforcement, merge reporting and "no longer needed" warnings, pruning), reading the core registries. - Relocate the `DeprecatedElement` spec to `elasticgraph-schema_definition`, where it now also proves the DSL works with no extension active and that `defined_at` reports the user's schema definition location through the `DelegateClass` indirection. No user-facing behavior changes: `f.renamed_from` etc. work exactly as before for json_ingestion users; they now also work without json_ingestion. ## Stacked PRs #1080 rebases on this to drop its `respond_to?` probe and spec shim. ## Verification - `script/quick_build` (all green, including `script/run_gem_specs` for both changed gems at 100% line + branch coverage) - `script/type_check`, `script/lint`, `script/spellcheck` ## References - #1259 (moved the markers to json_ingestion; this partially reverts that placement) - #1080 (the proto_ingestion consumer) - #1059 --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
deleted_type,deleted_field, andrenamed_fromfrom core schema definition into the JSON ingestion schema definition extension.elasticgraph-json_ingestion.Validation
script/lintscript/type_checkscript/run_gem_specs elasticgraph-json_ingestionbundle exec rspec elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/factory_spec.rbNote:
script/run_gem_specs elasticgraph-schema_definitionwas not run to completion locally because the test datastore was not running (http://localhost:9234).