Allow sneakers to be run without an exchange

[BenTalagan]

This is a basic, minimal draft for implementing this feature : sneakers should (optionally) be able to run on a queue without having to mess at all with the exchange on which the queue is bound. This is important in configurations where the queue already exists and no authorizations are given on the exchange itself. See #399.

Passing explicitly exchange =>nil to the sneakers queue configuration will bypass the exchange handling part (until now, sneakers would crash in that configuration). It will not break the compatibility with configurations where a sneakers exchange is not given at all (it will default to the 'sneakers' exchange just as before). I've also run the tests and nothing's reported broken. I'm not familiar with sneakers source, but haven't found other parts of the source that could be impacted by these changes.

Example of use :

#!/usr/bin/env ruby
require 'sneakers'

class Processor
  include Sneakers::Worker
  from_queue :logs,
             :exchange => nil
  
  def work(msg)
    puts "Got message #{msg}"
    ack!
  end
  
end

[gabrieljoelc]

Thank you for open this PR! The changes look straightforward and the existing specs passing is a great sign of backwards compatibility.

Can you add some specs to queue_spec.rb and worker_spec.rb that explicitly test the nil exchange case(s)? Do you think we should add an example worker as well?

[BenTalagan]

Hi Gabriel, I have added a bit of test in worker_spec.rb ; however I'm a bit lost in the queue_spec tests and would probably need a bit of a help :-)

I'm not sure if an example is really useful, probably a notice in the documentation would be sufficient. But I'm not sure where is the best place for it, the options for the from_queue configuration class method of a worker are not that well documented and need a bit of digging in the doc to be found.

[michaelklishin]

This looks reasonable except for the test. Thank you.

[michaelklishin]

I find this test to be confusing. What does a queue have to do with an exchange? If there is no way to make it clearer, let's drop it.

[BenTalagan]

You're right, after re-reading, this is really confusing. Sorry, I'm not familiar at all with the test logic. I've made a correction for this test which, maybe, makes more sense.

[gabrieljoelc]

thoughts on nil exchange in regards to this line when it's invoked through Worker#enqueue() and also this line right in Worker (see also this spec)?

[gabrieljoelc]

I do think an example would help prove that the consumer behaves the way we expect even when no exchange.

[BenTalagan]

thoughts on nil exchange in regards to this line when it's invoked through Worker#enqueue()?

Ouch, good point. This line is also problematic.

Within the current architecture, we cannot have a publisher on a worker if the queue's exchange is not authorized for writing : this case does not really fit sneakers' architecture where a worker deduces its publisher from its queue's parameters. Not sure what we should do here... raise an exception (seems too restrictive to me, the worker will not be able to have a publisher). Or allow a worker to have an exchange for publishing different from the one the queue is bound to? That may lead to complicated refactoring... One solution I can imagine, though, is instead of passing exchange => nil, have another parameter (name to be defined, like skip_exchange_binding/declaration) to get the wanted behavior. The exchange + exchange_options parameters could then be used for the publisher's declaration, although it's not totally intuitive because the queue could therefore be bound to a totally different exchange.

[gabrieljoelc]

yea, it feels like we need to answer this question:

what does it mean to publish without an exchange?

if publish has no meaning when no exchange, then maybe we have a no-op publisher or even have a separate worker?

it does feel like Worker#publish() should be using Worker#publisher instead of using the exchange directly. We could then have a publisher factory which creates based on the exchange or a custom publisher override.

[BenTalagan]

it does feel like Worker#publish() should be using Worker#publisher instead of using the exchange directly. We could then have a publisher factory which creates based on the exchange or an custom publisher override.

I do agree with you, all the more that this looks like repeated code : publisher vs worker.

yea, it feels like we need to answer this question:
what does it mean to publish without an exchange?
if publish has no meaning when no exchange, then maybe we have a no-op publisher or even have a separate worker?

Indeed. The aim of a worker's publisher is (to my understanding) to have a simple way to answer easily to messages within the worker's handling code ; I've personally never had the use for it since in most cases I use sneakers as an entry point and the "publishing" part is done somewhere else without sneakers. But I understand perfectly the use for it. Within sneakers "convenient" easy-to-use model, an exchange may be needed for replying ; my objection against the current model is, why should it necessarily be the queue's exchange? Probably for keeping the model simple, i.e. sneakers offers a default publisher which is by design bound to the queue's exchange. If one needs another publisher he/she can create it aside and use it for replying. So my proposition is the following :

1. Fix Worker#publish() to use self.class.enqueue which uses the Worker's class publisher.
2. If exchange => nil is given, raise an error in publisher to alert from misuse ("trying to use worker's publisher which is not defined").
3. Keep the old behaviour for the publisher creation, but allow the creation of an alternative publisher with a with_publisher class method on the worker (just like from_queue), that overrides the default publisher parameters deduced from the from_queue arguments.

That way, we should not break anything and keep the compatibility, but be able to create workers on queues without specifying an exchange (with exchange =>nil), and have the possibility to override the default publisher's behavior.

What's your on opinion and do you have alternative ideas? :)

[michaelklishin]

I like the idea of worker "independent of publishing."

[BenTalagan]

Thank you, @michaelklishin, for your feedback on that matter!

@gabrieljoelc are you ok to go for a with_publisher class method for overriding the automatic publisher creation of a worker (which is based on the exchange+exchange_options parameters of the from_queue class method)?

[gabrieljoelc]

@BenTalagan that's a good idea. However, I think the framework should default to the no-op publisher when nil exchange with no configuration for the user. Maybe a factory that's based on the exchange uses the with_publisher by default, but the user can still use with_publisher to override?

[michaelklishin]

Sneakers now has a new home under https://github.com/ruby-amqp/kicks. May I ask you to re-submit this PR there? Thank you 🙏