Make the JSON ingestion spec extension fully opt-in#1249
Open
jwils wants to merge 5 commits into
Open
Conversation
jwils
requested review from
BrianSigafoos-SQ,
bsorbo,
jwondrusch and
myronmarston
as code owners
jwils
force-pushed
the
joshuaw/indexer-ingestion-adapters
branch
from
e8e1a67 to
1631628
Compare
jwils
force-pushed
the
joshuaw/lazy-json-ingestion-spec-default
branch
from
81cb325 to
5e11583
Compare
jwils
changed the title
jwils
force-pushed
the
joshuaw/lazy-json-ingestion-spec-default
branch
from
8dc35fb to
39af63e
Compare
jwils
force-pushed
the
joshuaw/indexer-ingestion-adapters
branch
from
1631628 to
4a79422
Compare
Removes the load-time availability probe (`begin/rescue LoadError`) from `spec_support/schema_definition_helpers.rb`, as suggested on #1205. The `require` now happens only when a spec relies on the default extension modules, so the file loads fine in spec bundles that exclude the optional `elasticgraph-json_ingestion` gem (those suites pass `extension_modules:` explicitly), and a suite that relies on the default without the gem gets a clear `LoadError` instead of silently building artifacts without JSON schemas. Spec files that reference the extension constant directly now require it themselves instead of depending on the probe's load-time side effect.
The lazy default now lives in `CommonSpecHelpers` so that `generate_schema_artifacts` (and `build_datastore_core`, via a new `schema_definition_extension_modules` option) can use it too. The `elasticgraph-health_check` and `elasticgraph-query_interceptor` suites — whose bundles exclude `elasticgraph-json_ingestion` — pass `[]` explicitly. Spec files that reference the extension constant directly now require it themselves rather than relying on the deleted probe's load-time side effect.
jwils
force-pushed
the
joshuaw/indexer-ingestion-adapters
branch
from
4a79422 to
b1abdb0
Compare
Nothing needs the defaulting anymore. The shared spec helpers no longer apply the JSON ingestion extension to schema definitions; instead, each place that genuinely needs JSON schemas opts in explicitly: - `generate_schema_artifacts` includes the extension only when loading the repository's main test schema (`config/schema.rb`), which uses the JSON ingestion schema definition DSL. - `build_indexer` includes the extension when it generates artifacts itself, since the indexer can only ingest JSON events today. The require is lazy so bundles without `elasticgraph-json_ingestion` can still build an indexer from an externally built `datastore_core:`. - Specs that index documents with inline schemas (and so need JSON schema validation) pass the extension at their build call sites. - Vestigial `t.json_schema` calls in unit specs that never index are removed, along with a `JsonSafeLong` placeholder assertion that duplicated coverage now living in elasticgraph-json_ingestion.
Every spec in elasticgraph-json_ingestion exercises behavior provided by APIExtension, so a gem-local "JSONIngestionSchemaDefinitionHelpers" context (which extends the shared "SchemaDefinitionHelpers" context) now applies the extension to every schema its specs define, instead of each `define_schema` call site passing `extension_modules:` explicitly.
jwils
force-pushed
the
joshuaw/lazy-json-ingestion-spec-default
branch
from
39af63e to
6a733f1
Compare
With `VALIDATE_GRAPHQL_SCHEMAS=1` (as on CI), the eager SDL validation raised the unresolvable-type error during GraphQL schema generation, before JSON schema generation could exercise the wrapped `FieldReference#resolve` nil return the example exists to cover, leaving that branch uncovered. Tagging the example with `:dont_validate_graphql_schema` (the existing opt-out for intentionally invalid schemas) makes the error surface via the JSON schema path in all environments.
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.
Why
On #1205, @myronmarston suggested that the
DEFAULT_SCHEMA_DEFINITION_EXTENSION_MODULESavailability probe inspec_support/schema_definition_helpers.rb(abegin/rescue LoadErrorevaluated at file load) could go away in a follow-up once the extraction settled. The review here then asked the natural follow-on question: is there anything left that really needs the defaulting, or is this a good time to force tests to opt in?Answer: nothing needs the defaulting anymore. This PR removes it entirely.
What
spec_support/schema_definition_helpers.rbno longer applies any extension modules by default —define_schemain specs builds a plain core schema unless the caller passesextension_modules:.generate_schema_artifactsincludes the extension only when loading the repository's main test schema (config/schema.rb), which uses the JSON ingestion schema definition DSL. Schemas defined via a block get no extensions unless requested.build_indexerincludes the extension when it generates schema artifacts itself, since the indexer can only ingest JSON events today. The require is lazy so spec bundles withoutelasticgraph-json_ingestion(apollo, warehouse, health_check, query_interceptor) can still build an indexer from an externally builtdatastore_core:.schema_definition_extension_modules:/extension_modules:at their build call sites.t.json_schemacalls in unit specs that never index are removed, along with aJsonSafeLongplaceholder assertion inelasticgraph-graphqlthat duplicated coverage now living inelasticgraph-json_ingestion."JSONIngestionSchemaDefinitionHelpers"shared context (modeled onelasticgraph-warehouse'swarehouse_schema_support) now appliesAPIExtensionto every schema defined inelasticgraph-json_ingestion's own specs, so those call sites don't each passextension_modules:explicitly.Verification
script/run_gem_specsforelasticgraph-schema_definition,elasticgraph-json_ingestion,elasticgraph-indexer,elasticgraph-graphql,elasticgraph-datastore_core,elasticgraph-admin,elasticgraph-health_check, andelasticgraph-query_interceptorscript/type_checkscript/quick_buildStack
Current PR is marked with
->.