Conversation
f4ff0b9 to
1d5d646
Compare
e1a17d2 to
be6c6cf
Compare
b4c0f08 to
8b93271
Compare
024a086 to
f01cd5c
Compare
f01cd5c to
55e723d
Compare
05e4265 to
6f053fc
Compare
6f053fc to
1fa6418
Compare
1fa6418 to
afdfeec
Compare
…xtension (#1302) ## Why JSON parsing and validation has never conceptually belonged to `elasticgraph-indexer`. It should live in `elasticgraph-json_ingestion` alongside JSON schema generation, while the indexer stays ingestion-format-neutral so other formats can plug in the same way. ## What - Move the JSON ingestion adapter and JSON-schema-based record preparer factory into `elasticgraph-json_ingestion` - Register `JSONIngestion::IngestionAdapter` as `Indexer#ingestion_adapters_by_format["json"]` through the schema artifacts' indexer extension - Keep JSON ingestion automatic, while preserving custom adapters and explicitly injected registries - Remove JSON-specific ingestion logic and record preparation from `elasticgraph-indexer` - Raise a clear `ConfigError` when a schema supplies no ingestion adapters - Move the `be_a_valid_elastic_graph_event` matcher to `elastic_graph/json_ingestion/spec_support/event_matcher` ## Remaining follow-up `Indexer::TestSupport::Converters` still builds JSON-envelope events with `json_schema_version`. Moving it requires changes to `elasticgraph-local`'s fake-data indexing, so a later PR will handle it. ## Verification No manual testing. ## Stack Current PR is marked with `->`. - [#1301 Extract an ingestion adapter seam inside elasticgraph-indexer](#1301) - -> [#1302 Move JSON ingestion into elasticgraph-json_ingestion via an indexer extension](#1302) - [#1220 Add configurable indexing event decoder](#1220) - [#1351 Extract indexing field metadata behind a format-neutral value object](#1351) - [#1284 Keep ingestion schema versions adapter-owned](#1284) - [#1376 Pass transport metadata to event decoders](#1376) - [#1377 Model decoded indexing events as typed values](#1377)
afdfeec to
816359b
Compare
816359b to
a5f8aa8
Compare
## Why `ElasticGraph::Indexer` accepts decoded events and should stay independent of their wire format. Applications may need separate JSON and protobuf indexers, while `elasticgraph-indexer_lambda` still needs ElasticGraph to decode its JSON Lines payloads from SQS. ## What - Add `ElasticGraph::JSONIngestion::Indexer`, which wraps the base indexer with JSON Lines support. - Make `elasticgraph-indexer_lambda` use that JSON-aware wrapper while leaving its SQS payload format unchanged. - Keep payload decoder configuration and extension points out of the base indexer. ## How The wrapper can build its own `ElasticGraph::Indexer` from YAML or wrap an existing instance. Its `process` methods decode JSON Lines and pass the events to the base processor. `SqsProcessor` uses the wrapper to decode each SQS body, adds transport metadata, and sends the combined batch to the base processor. This lets a future `ElasticGraph::ProtoIngestion::Indexer` own protobuf decoding without adding another format-specific hook to `elasticgraph-indexer`. ## Risk Low. The Lambda still accepts JSON Lines and follows the same batching and failure-handling paths. The new wrapper changes internal construction but does not add a configuration migration. ## Testing No manual testing. ## Bigger picture This keeps the Ruby indexer format-neutral while giving applications an ElasticGraph-owned decoder for each supported format. ## Stack Current PR is marked with `->`. - [#1301 Extract an ingestion adapter seam inside elasticgraph-indexer](#1301) (merged) - [#1302 Move JSON ingestion into elasticgraph-json_ingestion via an indexer extension](#1302) (merged) - -> [#1220 Add a JSON-aware indexer wrapper](#1220) - [#1351 Extract indexing field metadata behind a format-neutral value object](#1351) - [#1284 Keep ingestion schema versions adapter-owned](#1284) - [#1376 Pass transport metadata to event decoders](#1376) - [#1377 Model decoded indexing events as typed values](#1377) - [#1384 Reject indexed schemas without an indexer extension](#1384)
463c1a1 to
4c6e339
Compare
8d1f8da to
1d890ba
Compare
e8ace45 to
3451e0d
Compare
| def message_id: () -> untyped | ||
| def latency_timestamps: () -> untyped | ||
| def with_payload: (::Hash[::String, untyped]) -> Event | ||
| def to_h: () -> ::Hash[::String, untyped] |
There was a problem hiding this comment.
There's an awful lot of untyped here, which means steep can't check type-system bugs with how we use these values. For example, we should consider which ones we want to make nilable or not, to force the caller to deal with potential nils where they may show up.
There was a problem hiding this comment.
One of the issues here is that we don't have strong types until validation. Made some progress Let me know what you think of this approach.
|
|
||
| # @return [Object, nil] the requested operation | ||
| def op | ||
| payload["op"] |
There was a problem hiding this comment.
You're using #[] for all these methods which gives less friendly errors than .fetch:
irb(main):001> hash = {}
=> {}
irb(main):002> op = hash["op"]
=> nil
irb(main):003> op.upcase
(irb):3:in '<main>': undefined method 'upcase' for nil (NoMethodError)
from /Users/myron.marston/.asdf/installs/ruby/4.0.0/lib/ruby/gems/4.0.0/gems/irb-1.18.0/exe/irb:9:in '<top (required)>'
from /Users/myron.marston/.asdf/installs/ruby/4.0.0/lib/ruby/site_ruby/4.0.0/rubygems.rb:305:in 'Kernel#load'
from /Users/myron.marston/.asdf/installs/ruby/4.0.0/lib/ruby/site_ruby/4.0.0/rubygems.rb:305:in 'Gem.activate_and_load_bin_path'
from /Users/myron.marston/.asdf/installs/ruby/4.0.0/bin/irb:25:in '<main>'
irb(main):004> op = hash.fetch("op")
(irb):4:in 'Hash#fetch': key not found: "op" (KeyError)
from (irb):4:in '<main>'
from /Users/myron.marston/.asdf/installs/ruby/4.0.0/lib/ruby/gems/4.0.0/gems/irb-1.18.0/exe/irb:9:in '<top (required)>'
from /Users/myron.marston/.asdf/installs/ruby/4.0.0/lib/ruby/site_ruby/4.0.0/rubygems.rb:305:in 'Kernel#load'
from /Users/myron.marston/.asdf/installs/ruby/4.0.0/lib/ruby/site_ruby/4.0.0/rubygems.rb:305:in 'Gem.activate_and_load_bin_path'
from /Users/myron.marston/.asdf/installs/ruby/4.0.0/bin/irb:25:in '<main>'
IMO it's much nicer to be notified of the missing key rather than getting a NoMethod on nil error later.
So, it'd be good to use .fetch in the methods where we expect the key to always be there. (Of course, this is moot if you go with my suggested alternate EVent design from above).
| attr_reader record: ::Hash[::String, untyped]? | ||
| attr_reader op: untyped | ||
| attr_reader type: untyped | ||
| attr_reader version: untyped |
There was a problem hiding this comment.
I'd prefer to keep the stricter types--untyped essentially disables type checking on the returned value.
For the record below we could consider using generics for it. (That could also apply to Event as it also has record.
| expect(Event.from(event)).to equal(event) | ||
| end | ||
|
|
||
| it "returns nil for missing envelope fields so an adapter can report all validation failures" do |
There was a problem hiding this comment.
Oh this is interesting--I was thinking we wouldn't want to return nils for expected fields, but I hadn't thought about how it might turn clear validation errors into exceptions instead.
I'm not sure what's best here. Ideally the validation is handled in just one spot and the rest of the system can assume non-nil values.
There was a problem hiding this comment.
Let me know what you think of the approach now with the validated event type. It relates to a few of the comments on typing. Event::Validated is now strictly typed
| # This is by design, since we're picking a schema based on best-effort, so to avoid that by-design validation error, | ||
| # performing the envelope validation on a "patched" version of the event. | ||
| event_with_patched_envelope = event.merge({JSON_SCHEMA_VERSION_KEY => selected_json_schema_version}) | ||
| event_with_patched_envelope = event.to_h.merge({JSON_SCHEMA_VERSION_KEY => selected_json_schema_version}) |
There was a problem hiding this comment.
It's a little weird to go back to a hash here. You might consider doing something like:
class JSONEvent = ::Data.define(:base_event, :json_schema_version_key) do
extend Forwardable
def_delegatators :base_event, :id, :version, # ...etc
endIIRC, there are some other spots in this PR where you .to_h to deal with JSON_SCHEMA_VERSION_KEY--this approach would let you deal with it as an attribute like any other.
Why
The shared indexing pipeline reads decoded events through string keys. Its latency logger also requires
json_schema_version, so a valid non-JSON event can fail after the datastore write.What
Introduce
Indexer::Eventas a format-neutral value object with named envelope fields, and remove the JSON schema version requirement from shared latency logs.How
Eventstores named envelope fields and can be constructed directly from native decoder values.Event.from_hashpreserves the original hash for format-specific validation.Eventobjects. The processor, operation factory, and remaining indexing pipeline accept only events.Event::Validated. Shared operations then use concreteString,Integer, and timestamp types instead ofuntypedvalues.Risk
This changes the decoder, processor, adapter, operation, and failure-event contracts. Extensions must pass
Eventobjects to the processor and return validated events from adapters.ElasticGraphIndexingLatenciesno longer includesjson_schema_version.Testing
No manual testing. A local full gem run did not start because the test datastore was unavailable. The full datastore suites run in CI.
Bigger picture
Non-hash ingestion decoders can construct
Eventdirectly and keep native records through adapter validation. The protobuf ingestion work in #1388 can use this path instead of manufacturing a JSON-shaped event hash.