Skip to content

Refactor indexer lambda event decoding#1082

Open
jwils wants to merge 1 commit intomainfrom
joshuaw/indexer-runtime-event-decoder
Open

Refactor indexer lambda event decoding#1082
jwils wants to merge 1 commit intomainfrom
joshuaw/indexer-runtime-event-decoder

Conversation

@jwils
Copy link
Copy Markdown
Collaborator

@jwils jwils commented Mar 23, 2026

Summary

  • extract SQS event decoding behind a dedicated decoder object in elasticgraph-indexer_lambda
  • keep JSON Lines as the default path while documenting the injected decoder contract on SqsProcessor
  • add the missing JSONLDecoder / SqsProcessor RBS coverage and unit coverage for happy-path and empty-body decoding

Why

  • prepares the runtime ingestion path for alternate payload formats without changing current JSONL behavior
  • keeps the refactor narrowly scoped to the SQS/Lambda entrypoint on main

@jwils jwils force-pushed the joshuaw/indexer-runtime-event-decoder branch from 327b53f to cd70e31 Compare March 23, 2026 04:10
Copy link
Copy Markdown
Collaborator

@myronmarston myronmarston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general; left a few minor suggestions.

Can you explain how the protobuf ingestion support will build on this?

# `#decode_events(sqs_record:, body:)` contract and return event hashes.
#
# @private
class JSONLDecoder
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is stateless, so we could define this as a module and do def self.decode_events below so that there's no garbage instance created for no reason.

# @param body [String] resolved SQS message body
# @return [Array<Hash>] decoded ElasticGraph events
def decode_events(sqs_record:, body:)
_ = sqs_record
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this line? It looks like it doesn't do anything...

More generally, how do you expect sqs_record to get used?

# @return [Array<Hash>] decoded ElasticGraph events
def decode_events(sqs_record:, body:)
_ = sqs_record
body.split("\n").map { |event| JSON.parse(event) }
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
body.split("\n").map { |event| JSON.parse(event) }
body.split("\n").map { |event| ::JSON.parse(event) }

Comment on lines +4 to +7
def decode_events: (
sqs_record: ::Hash[::String, untyped],
body: ::String
) -> ::Array[::Hash[::String, untyped]]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def decode_events: (
sqs_record: ::Hash[::String, untyped],
body: ::String
) -> ::Array[::Hash[::String, untyped]]
extend _EventPayloadDecoder

This is the RBS equivalent of "implements the interface at the class level".

module ElasticGraph
module IndexerLambda
class SqsProcessor
interface _EventPayloadDecoder
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably move out of SqsProcessor so it can be referenced without the SqsProcessor:: prefix. Maybe moved into the jsonl_decoder file?

module ElasticGraph
module IndexerLambda
RSpec.describe JSONLDecoder do
describe "#decode_events" do
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
describe "#decode_events" do
describe ".decode_events" do

(If you adopt my suggestion to make it callable directly on JSONLDecoder with no instance needed...).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants