Should metadata and payload be instance variables on the handler?

[michberr]

Motivation

The payload and the listener metadata should be accessible in an intuitive way from any handler methods.

Background

Current implementation:

def process_message(payload)
 ...
 handler = @listener.handler_class.new
 preprocessed_payload = handler.before_consume(payload)

 @listener.handler_class.around_consume(preprocessed_payload, @metadata) do
    handler.consume(preprocessed_payload, @metadata)
  end
end

• Currently, the listener metadata is not available in #before_consume, which poses problems for logging and custom configuration.
• Does it make sense to be passing both payload and metadata as args to every handler method?

Option 1: pass the payload and metadata as arguments to handler methods

handler.before_consume(payload, @metadata)

Pros:

• Only 1 breaking change
• It is obvious which arguments are available to each method

Cons:

• Clunky to pass in args repeatedly
• #before_consume may be deprecated in the future (see #84)

Option 2: Initialize the handler with both the payload and metadata

 handler = @listener.handler_class.new(@metadata, payload)
 preprocessed_payload = handler.before_consume

 handler.around_consume(preprocessed_payload) do |around_consume_payload|
    handler.consume(around_consume_payload)
  end
end

Pros

• Full encapsulation of payload and metadata within the handler instance
• Allows retainment of original payload from any handler method. My team has one particular situation where we need to grab some metadata from the original payload and have that accessible in our around_consume logic. This would fix that problem (but we could also set our own instance var once around_consume is an instance method)

Cons:

• While i DO think it makes sense to store metadata as an instance variable because it is usually static data that is just being passed to every instance method, I'm not sure how generally useful it is to store the original payload as a variable since we expect it to be mutated in #before_consume and perhaps #around_consume (if the implementation changes as suggested in #84).

Option 3: Compromise of 1 and 2

 handler = @listener.handler_class.new(@metadata)
 preprocessed_payload = handler.before_consume(payload)

 handler.around_consume(preprocessed_payload) do |around_consume_payload|
    handler.consume(around_consume_payload)
  end

Since we are hoping to migrate .around_consume to an instance method in #82, the metadata should now be available from any handler instance.

Pros:

• This looks like proper Ruby object-oriented code to me. The metadata is data that is available when we create the handler instance and should be accessible to all methods on that instance.

Cons:

• fairly large breaking change, impacting all handler instance methods
• Potentially less observability into the origin of the metadata <- I actually disagree with this. If we keep metadata as an instance var, any experienced ruby programmer should not be surprised to see it pop-up.

Related Links

#82
#84

[rstratton]

I feel like option 1 provides the cleanest interface to Phobos users, since it clearly states what data their methods are supposed/"allowed" to act on. Exposing instance variables of a library to the users of the library feels a little funny to me, though maybe I'm wrong to feel that way.

[klippx]

Can you please update the description with the latest changes to master, as before_consume now actually does accept metadata? In regard to how the issue is put forward this leaves only "Does it make sense to be passing both payload and metadata as args to every handler method?" remaining as an argument. And to put in another way, Option 1 is currently how it works in Phobos (since yesterday - pending release of course).

Pros Option 2 and Option 3
While I like the idea of from an "object oriented" point of view, I wonder why the suggestion is not to go all in and instantiate both metadata and payload in the handler object and work on them, wouldn't that be even more object oriented? However that would be shooting ourselves in the foot since we know that payload needs to be decoded, uncompressed, whatnot.

Cons Option 2 and Option 3
I am not too fond of the idea from a "functional" point of view. I kind of like that Option 1 provides the ability to pass in "modified" versions of payload and metadata without mutating the instance variables of the handler in between cycles of before_consume, around_consume and consume.

[dorner]

Yep. In general IMO instance variables are not the way to go here since it makes it less visible as to how things are happening. OO makes sense if you are modeling data and operations on that data. In this case, Handler is basically just a container for some functions, so the functional paradigm makes much more sense.

[klippx]

I believe that Option1 should be good enough for you. Please reopen this if you disagree.