logstash-plugins/logstash-output-elasticsearch

Isolate datastream vs normal indexing decision into test fixture

andsel opened this issue · 0 comments

In fixing issue #1160 the PR #1161 added some testing to cover the case of integration metadata in combination with datastreams.
This led to the the creation of two tests which switched method under-test:

In these tests the method under test switched from event_action_tuple to data_stream_event_action_tuple to test the datastream scenario.
It would mean that abstraction level tested by test isn't right one, ideally the fixture's setup would trigger one path of execution or the other. There shouldn't be knowledge in the test about which method to call, in this context.

This issue is proposing to introduce a change so that the tests haven't knowldege to call event_action_tuple or data_stream_event_action_tuple to proper test the use case, should be exposed a more abstract method which isolate from this decision, and decision is made only based on the data in the fixture.

Possibile solution could be the introduction o action_tuple(event) that uses the @event_mapper (which presently wraps either event_action_tuple or data_stream_event_action_tuple depending on other configuration):

  def action_tuple(event)
    @event_mapper.call(event)
  end

And then changing the current only call-site of @event_mapper.call to go through our new unified boundary:

diff --git a/lib/logstash/outputs/elasticsearch.rb b/lib/logstash/outputs/elasticsearch.rb
index 598265d..55e9a87 100644
--- a/lib/logstash/outputs/elasticsearch.rb
+++ b/lib/logstash/outputs/elasticsearch.rb
@@ -424,7 +424,7 @@ class LogStash::Outputs::ElasticSearch < LogStash::Outputs::Base
     event_mapping_errors = [] # list of FailedEventMapping
     events.each do |event|
       begin
-        successful_events << @event_mapper.call(event)
+        successful_events << action_tuple(event)
       rescue EventMappingError => ie
         event_mapping_errors << FailedEventMapping.new(event, ie.message)
       end

We could then test action_tuple directly for a given plugin config without needing to know whether we should be testing event_action_tuple or data_stream_event_action_tuple.

Another possible way would be to pull the datastream-specific and legacy-specific behaviour out into a self-contained Modules so that setup_mapper_and_target wouldn't need to store procs-that-call-specific-methods.