Restore schema-evolution declarations to elasticgraph-schema_definition#1294
Restore schema-evolution declarations to elasticgraph-schema_definition#1294jwils wants to merge 1 commit into
Conversation
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>
|
|
||
| # Registers an old name that this field used to have in a prior version of the schema. Extensions | ||
| # use this to deal with schema evolution — for example, `elasticgraph-json_ingestion` uses it to | ||
| # migrate data ingested under the old JSON schema version. |
There was a problem hiding this comment.
| # migrate data ingested under the old JSON schema version. | |
| # ingest events that contain old field names that are no longer in the current schema. |
| # use this to deal with schema evolution — for example, `elasticgraph-json_ingestion` uses it to | ||
| # migrate data ingested under the old JSON schema version. | ||
| # | ||
| # @note When `elasticgraph-json_ingestion` is in use and this API applies, ElasticGraph will give you an error message |
There was a problem hiding this comment.
| # @note When `elasticgraph-json_ingestion` is in use and this API applies, ElasticGraph will give you an error message | |
| # @note In situations where this API is needed, ElasticGraph will give you an error message |
|
|
||
| # Registers the name of a field that existed in a prior version of the schema but has been deleted. | ||
| # | ||
| # @note When `elasticgraph-json_ingestion` is in use and this API applies, ElasticGraph will give you an error message |
There was a problem hiding this comment.
| # @note When `elasticgraph-json_ingestion` is in use and this API applies, ElasticGraph will give you an error message | |
| # @note In situations where this API is needed, ElasticGraph will give you an error message |
|
|
||
| # Registers an old name that this type used to have in a prior version of the schema. | ||
| # | ||
| # @note When `elasticgraph-json_ingestion` is in use and this API applies, ElasticGraph will give you an error message |
There was a problem hiding this comment.
| # @note When `elasticgraph-json_ingestion` is in use and this API applies, ElasticGraph will give you an error message | |
| # @note In situations where this API is needed, ElasticGraph will give you an error message |
|
|
||
| # Registers the name of a type that existed in a prior version of the schema but has been deleted. | ||
| # | ||
| # @note When `elasticgraph-json_ingestion` is in use and this API applies, ElasticGraph will give you an error message |
There was a problem hiding this comment.
| # @note When `elasticgraph-json_ingestion` is in use and this API applies, ElasticGraph will give you an error message | |
| # @note In situations where this API is needed, ElasticGraph will give you an error message |
| end | ||
|
|
||
| def register_renamed_type(type_name, from:, defined_at:, defined_via:) | ||
| renamed_types_by_old_name[from] = SchemaElements::DeprecatedElement.new( |
There was a problem hiding this comment.
Instead of manually instantiating DeprecatedElement can we define a new_deprecated_element factory method and use that? That's what we had before #1259 and it's a pattern we want to consistently follow so that extensions can hook in as needed.
| state = define_schema(schema_element_name_form: "snake_case") do |schema| | ||
| schema.object_type "Widget" do |t| | ||
| t.renamed_from "OldWidget" | ||
| t.field "id", "ID!" | ||
| end | ||
| end.state | ||
|
|
||
| expect(state.renamed_types_by_old_name.fetch("OldWidget").defined_at.path).to eq __FILE__ |
There was a problem hiding this comment.
| state = define_schema(schema_element_name_form: "snake_case") do |schema| | |
| schema.object_type "Widget" do |t| | |
| t.renamed_from "OldWidget" | |
| t.field "id", "ID!" | |
| end | |
| end.state | |
| expect(state.renamed_types_by_old_name.fetch("OldWidget").defined_at.path).to eq __FILE__ | |
| renamed_from_callsite = nil | |
| state = define_schema(schema_element_name_form: "snake_case") do |schema| | |
| schema.object_type "Widget" do |t| | |
| renamed_from_callsite = __LINE__ + 1 | |
| t.renamed_from "OldWidget" | |
| t.field "id", "ID!" | |
| end | |
| end.state | |
| defined_at = state.renamed_types_by_old_name.fetch("OldWidget").defined_at | |
| expect(defined_at.path).to eq __FILE__ | |
| expect(defined_at.lineno).to eq renamed_from_callsite |
If we're going to verify the file is correct, we should verify the line number is correct, too--there are other stack frames from the same file in the callstack (such as the schema.object_type call) which would be the wrong line to report.
Also: can you expand this test (or add additional tests) to cover all of the renamed_from/deleted_xyz APIs? They each have defined_at: ... logic which could be wrong so it'd be good to verify they are all correct.
Why
elasticgraph-proto_ingestion(#1080) is a second consumer of the schema-evolution declarations: it needsfield.renamed_frommetadata to keep protobuf field numbers stable across public field renames, andtype.deleted_fieldmaps directly onto protobufreservedfield numbers as a natural follow-up. With the markers living insideelasticgraph-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 about
elasticgraph-json_ingestionbolting 9 fields onto::ElasticGraph::SchemaDefinition::StatewithState & StateExtensioncasts: itsStateExtensionnow adds only the 5 genuinely JSON-specific fields, and the four deprecation registries are ordinary typedStatemembers that need no casts.What
DeprecatedElementtoElasticGraph::SchemaDefinition::SchemaElements::DeprecatedElement.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 theirregister_*methods to coreState, always initialized.field.renamed_from,type.renamed_from,type.deleted_field,schema.deleted_type— onto coreField/TypeWithSubfields/API, with docs reworded to be serializer-neutral.elasticgraph-json_ingestionkeeps everything actually JSON-specific (schema version enforcement, merge reporting and "no longer needed" warnings, pruning), reading the core registries.DeprecatedElementspec toelasticgraph-schema_definition, where it now also proves the DSL works with no extension active and thatdefined_atreports the user's schema definition location through theDelegateClassindirection.No user-facing behavior changes:
f.renamed_frometc. 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, includingscript/run_gem_specsfor both changed gems at 100% line + branch coverage)script/type_check,script/lint,script/spellcheckReferences