Migrate JSON ingestion specs into gem#1224
Conversation
9545bf4 to
eee835d
Compare
cd78e96 to
c34ed76
Compare
eee835d to
c0a2045
Compare
c34ed76 to
d4259d8
Compare
c0a2045 to
c1771af
Compare
c1771af to
d6dba0f
Compare
d4259d8 to
5d3b496
Compare
d6dba0f to
ff22ea0
Compare
8c038dc to
7371130
Compare
ef120ae to
f3a3547
Compare
7371130 to
83edb59
Compare
f3a3547 to
12be6cd
Compare
83edb59 to
7fb3c05
Compare
12be6cd to
2e2996c
Compare
7fb3c05 to
e713ed5
Compare
50e8b8c to
e243aa7
Compare
ca4ad1d to
ce59542
Compare
23e1562 to
c95e7cb
Compare
460d40d to
56e06cf
Compare
c95e7cb to
a92fd04
Compare
56e06cf to
9149b33
Compare
a92fd04 to
6b2f76b
Compare
9149b33 to
16ae034
Compare
6b2f76b to
aa68d56
Compare
16ae034 to
253bda5
Compare
aa68d56 to
95d92e6
Compare
253bda5 to
898d197
Compare
95d92e6 to
18b4bc2
Compare
898d197 to
9c881ac
Compare
- Trim the JSON-schema-versioning scenarios from the core `rake_tasks_spec` that are now covered by `schema_artifact_manager_extension_spec` in `elasticgraph-json_ingestion`, and remove the helpers that only those scenarios used. - Replace the core coverage those scenarios provided with focused unit specs: the `deleted_type`/`deleted_field`/`renamed_from` registration DSL, `FieldPath#fully_qualified_path_in_index`, and the test-support behavior when a schema sets its own JSON schema version. - Make `doc_comment` load-bearing in the wrapper equality specs by adding cases that differ only in `doc_comment`. - Assert on observable behavior instead of `singleton_class.ancestors` in the blockless-element extension spec. - Explain why `unresolved_type_ref` uses a stand-in.
18b4bc2 to
e99a7bf
Compare
9c881ac to
81f85dc
Compare
|
Addressed in 3dd2511 on this PR. |
The extension loader raises if the same extension constant is loaded from two different `require_path`s within one process. `scalar_type_extension_spec` loaded `ExampleScalarCoercionAdapter` via an absolute path while `elasticgraph-schema_definition`'s `scalar_types_by_name_spec` used a relative one, so any spec worker that ran both files failed 17 examples with `InvalidExtensionError`. Use the same relative path (with a local copy of the example adapter so the gem's own suite can resolve it).
As noted on #1224, mentions of JSON schema in `elasticgraph-schema_definition/spec` should approach zero now that the JSON schema logic lives in `elasticgraph-json_ingestion`: - The graphql_schema, datastore_config, and runtime_metadata spec supports now run schemas without any extension modules, and the `json_schema` calls that existed only to satisfy the extension's scalar validation are gone. - The scalar `json_schema` requirement test (duplicated by the json_ingestion suite) is deleted, and the `long`/`unsigned_long` placeholder-inference tests that depend on JSON schema bounds moved to the json_ingestion suite (along with the built-in-scalar placeholder map, which differs with the extension loaded). - The reserved-type-name test now exercises the core `reserved_type_names` mechanism directly; the `ElasticGraphEventEnvelope` reservation is already covered by the json_ingestion suite. - `rake_tasks_spec` runs its synthetic schemas without the extension and no longer asserts on JSON schema artifacts (covered by json_ingestion's integration spec). A new short-diff test keeps `truncate_diff` fully covered. The tests that evaluate the repo's own `config/schema.rb` still load the extension, since that schema is a JSON ingestion application. The remaining mentions are the `define_schema` test-support seam (which exists for optional ingestion extensions) and JSON-the-format documentation text.
The whole-repo CI run failed coverage because two identical copies of `ExampleScalarCoercionAdapter` (one per gem suite) shared the same relative require path, so only one of them ever loaded and the other reported 0% coverage. Moving the adapter into `spec_support/lib` leaves a single copy that every suite loads from the same require path, which also preserves the extension loader's same-path guarantee that the prior commit relied on.
myronmarston
left a comment
There was a problem hiding this comment.
Not done reviewing but want to submit my feedback so far...
| super( | ||
| schema_element_name_form: "snake_case", | ||
| extension_modules: [JSONIngestion::SchemaDefinition::APIExtension], | ||
| extension_modules: [APIExtension], |
There was a problem hiding this comment.
We should implement something like
so that the APIExtension is automatically added by all tests in the json-ingestion gem.(Please defer this to a follow up PR).
- Port the JSON-schema tests from `rake_tasks_spec` (version bump enforcement, versioned metadata maintenance, renamed/deleted field/type guidance, conflict and routing/rollover deletion errors) into `schema_artifact_manager_extension_spec` in place of the minimal from-scratch tests, restoring the original coverage. - Rewrite `wrappers_spec` to drive the wrappers through the public schema definition API instead of exercising the internal classes directly. - Fix the value semantics of the stateless field type wrappers (`Scalar`, `Enum`, `Union`): `DelegateClass` defines `==` to unwrap only the left operand, so two wrappers around equal field types never compared equal even though their `hash` values matched, breaking the `eql?`/`hash` contract. A shared `ValueSemantics` module now unwraps both sides, keeping `==`/`eql?`/`hash` consistent.
myronmarston
left a comment
There was a problem hiding this comment.
Beyond the feedback below:
- Can we add
elasticgraph-json_ingestionto the list of gems that run in parallel in? I measuredelasticgraph/script/run_gem_specs
Line 32 in ac2c10e
rspecvsflatware_rspecand found that flatware makes the json-ingestion test suite quite a bit faster. - I audited the remaining mentions of json in
elasticgraph-schema_definitionand here's what I found...
Some RBS signatures remain to migrate (or just remove if already copied over):
sig/elastic_graph/schema_definition/schema_elements/field.rbs:12: type jsonSchema = untyped
sig/elastic_graph/schema_definition/schema_elements/type_with_subfields.rbs:58: # ?json_schema: SchemaElements::Field::jsonSchema,
sig/elastic_graph/schema_definition/test_support.rbs:8: ?json_schema_version: ::Integer,
sig/elastic_graph/schema_definition/test_support.rbs:20: ?json_schema_version: ::Integer,
sig/elastic_graph/schema_definition/api.rbs:5: type jsonSchemaLayer = :nullable | :array
sig/elastic_graph/schema_definition/api.rbs:6: type jsonSchemaLayersArray = ::Array[jsonSchemaLayer]
(Note: these latter 2 are not really related to the PR as they aren't tests, but now that the tests have move this is the first point an audit was possible. Feel free to move those in a follow up PR).
Beyond that, this LGTM!
| RSpec.describe DeprecatedElement do | ||
| include_context "SchemaDefinitionHelpers" | ||
|
|
||
| it "records `deleted_type`, `deleted_field`, and `renamed_from` calls so that schema artifact tooling can consume them" do |
There was a problem hiding this comment.
deleted_type, deleted_field, and renamed_from are really only needed for use with the JSON schema definition APIs. They are used to record additional metadata on the versioned JSON schema artifacts. Since they don't have any use beyond that, it probably makes sense to move them into elasticgraph-json_ingestion in a follow up PR.
| }.to raise_error Errors::SchemaError, a_string_including("BigInt", "lacks `json_schema`") | ||
| end | ||
|
|
||
| it "extends schema elements created without customization blocks" do |
There was a problem hiding this comment.
This test doesn't seem related to ScalarTypeExtension at all. It seems to exist to cover some branches in factory_extension.rb. I'd recommend we move it there and simplify it drastically:
diff --git a/elasticgraph-json_ingestion/lib/elastic_graph/json_ingestion/schema_definition/factory_extension.rb b/elasticgraph-json_ingestion/lib/elastic_graph/json_ingestion/schema_definition/factory_extension.rb
index 93f2ae09..2bff3962 100644
--- a/elasticgraph-json_ingestion/lib/elastic_graph/json_ingestion/schema_definition/factory_extension.rb
+++ b/elasticgraph-json_ingestion/lib/elastic_graph/json_ingestion/schema_definition/factory_extension.rb
@@ -90,7 +90,7 @@ module ElasticGraph
extended_type.json_schema(**options)
end
- yield extended_type if block_given?
+ yield extended_type
extended_type.finalize_json_schema_configuration!
end
end
@@ -104,7 +104,7 @@ module ElasticGraph
def new_type_with_subfields(schema_kind, name, wrapping_type:, field_factory:)
super(schema_kind, name, wrapping_type: wrapping_type, field_factory: field_factory) do |type|
extended_type = type.extend(SchemaElements::TypeWithSubfieldsExtension) # : ::ElasticGraph::SchemaDefinition::SchemaElements::TypeWithSubfields & SchemaElements::TypeWithSubfieldsExtension
- yield extended_type if block_given?
+ yield extended_type
end
end
diff --git a/elasticgraph-json_ingestion/spec/unit/elastic_graph/json_ingestion/schema_definition/factory_extension_spec.rb b/elasticgraph-json_ingestion/spec/unit/elastic_graph/json_ingestion/schema_definition/factory_extension_spec.rb
new file mode 100644
index 00000000..7080b068
--- /dev/null
+++ b/elasticgraph-json_ingestion/spec/unit/elastic_graph/json_ingestion/schema_definition/factory_extension_spec.rb
@@ -0,0 +1,29 @@
+# Copyright 2024 - 2026 Block, Inc.
+#
+# Use of this source code is governed by an MIT-style
+# license that can be found in the LICENSE file or at
+# https://opensource.org/licenses/MIT.
+#
+# frozen_string_literal: true
+
+require "elastic_graph/constants"
+require "elastic_graph/errors"
+require "elastic_graph/json_ingestion/schema_definition/factory_extension"
+require "elastic_graph/spec_support/schema_definition_helpers"
+
+module ElasticGraph
+ module JSONIngestion
+ module SchemaDefinition
+ RSpec.describe FactoryExtension do
+ include_context "SchemaDefinitionHelpers"
+
+ it "allows `enum_type` and `interface_type` to be called without a block" do
+ define_schema(schema_element_name_form: "snake_case") do |schema|
+ schema.enum_type "EmptyEnum"
+ schema.interface_type "EmptyInterface"
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/elasticgraph-json_ingestion/spec/unit/elastic_graph/json_ingestion/schema_definition/schema_elements/scalar_type_extension_spec.rb b/elasticgraph-json_ingestion/spec/unit/elastic_graph/json_ingestion/schema_definition/schema_elements/scalar_type_extension_spec.rb
index 364dfe2e..15199586 100644
--- a/elasticgraph-json_ingestion/spec/unit/elastic_graph/json_ingestion/schema_definition/schema_elements/scalar_type_extension_spec.rb
+++ b/elasticgraph-json_ingestion/spec/unit/elastic_graph/json_ingestion/schema_definition/schema_elements/scalar_type_extension_spec.rb
@@ -28,37 +28,6 @@ module ElasticGraph
}.to raise_error Errors::SchemaError, a_string_including("BigInt", "lacks `json_schema`")
end
- it "extends schema elements created without customization blocks" do
- api = build_api
- api.enum_type "EmptyEnum"
- api.interface_type "EmptyInterface"
- direct_type_with_subfields = api.factory.new_type_with_subfields(
- :object,
- "DirectObject",
- wrapping_type: nil,
- field_factory: api.factory.method(:new_field)
- )
-
- # An enum's derived GraphQL types are built from a derived scalar twin, which can only be
- # built if `EnumTypeExtension` configured the twin's `json_schema`; otherwise building it
- # raises a "lacks `json_schema`" error.
- expect {
- api.state.enum_types_by_name.fetch("EmptyEnum").derived_graphql_types
- }.not_to raise_error
-
- # `json_schema` is only available on types extended with `TypeWithSubfieldsExtension`.
- interface_type = api.state.object_types_by_name.fetch("EmptyInterface")
- interface_type.json_schema minProperties: 1
- expect(interface_type.json_schema_options).to eq({minProperties: 1})
-
- direct_type_with_subfields.json_schema minProperties: 2
- expect(direct_type_with_subfields.json_schema_options).to eq({minProperties: 2})
-
- expect {
- build_api.scalar_type "BigInt"
- }.to raise_error Errors::SchemaError, a_string_including("BigInt", "lacks `json_schema`")
- end
-
it "infers a numeric missing-value placeholder for JSON-safe unsigned_long scalars with custom coercion" do
grouping_missing_value_placeholder = grouping_missing_value_placeholder_for(
"unsigned_long",
@@ -215,16 +184,6 @@ module ElasticGraph
# when one worker runs multiple suites.
"elastic_graph/spec_support/example_extensions/scalar_coercion_adapter"
end
-
- def build_api
- schema_elements = SchemaArtifacts::RuntimeMetadata::SchemaElementNames.new(form: "snake_case")
- ::ElasticGraph::SchemaDefinition::API.new(
- schema_elements,
- true,
- extension_modules: [APIExtension],
- output: log_device
- )
- end
end
end
end
diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/factory.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/factory.rbs
index 0a5c325b..cc55a8a9 100644
--- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/factory.rbs
+++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/factory.rbs
@@ -112,7 +112,7 @@ module ElasticGraph
::String,
wrapping_type: SchemaElements::anyObjectType,
field_factory: ::Method
- ) ?{ (SchemaElements::TypeWithSubfields) -> void } -> SchemaElements::TypeWithSubfields
+ ) { (SchemaElements::TypeWithSubfields) -> void } -> SchemaElements::TypeWithSubfields
@@type_with_subfields_new: ::Method
def new_union_type: (::String) { (SchemaElements::UnionType) -> void } -> SchemaElements::UnionTypeNote that factory.new_type_with_subfields and factory.new_scalar_type both yield unconditionally w/o the json-ingestion extension:
...so there's no need for the json-ingestion extension to support no block for these. I also noticed the type signature was wrong in factory.rbs so my suggested snippet above fixes that, too.
| }.to raise_error Errors::SchemaError, a_string_including("BigInt", "lacks `json_schema`") | ||
| end | ||
|
|
||
| it "infers a numeric missing-value placeholder for JSON-safe unsigned_long scalars with custom coercion" do |
There was a problem hiding this comment.
It could help to wrap all the ones related to grouping_missing_value_placeholder in a describe for organizational purposes.
Why
The JSON ingestion gem should own the JSON-schema tests that cover its extracted implementation so the gem can enforce full coverage without a SimpleCov skip.
What
elasticgraph-json_ingestion.elasticgraph-json_ingestion.Verification
script/run_gem_specs elasticgraph-json_ingestionscript/type_check elasticgraph-json_ingestionBUNDLE_GEMFILE=Gemfile bundle exec rspec --format progress spec/integration/elastic_graph/schema_definition/rake_tasks_spec.rb spec/unit/elastic_graph/schema_definition/runtime_metadata/scalar_types_by_name_spec.rbfromelasticgraph-schema_definition/bundle exec standardrb ...on touched specs/supportscript/run_gem_specs elasticgraph-schema_definitionandscript/run_gem_specs elasticgraph-json_ingestionboth pass with 100% coverage (with the test datastore booted).Follow-ups
module JSONIngestion::SchemaDefinitionform so that git/GitHub detects them as file moves. A follow-up PR (after this stack merges) will convert them to the standard nested module style and re-indent.schema_artifact_manager_extension_spec.rbcovers have been removed from the corerake_tasks_spec.rb; the core lines they exercised are now covered by focused unit specs instead.Stack
Current PR is marked with
->.