-
Notifications
You must be signed in to change notification settings - Fork 144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prohibit operation specific configuration to be used for unsupported operations #275
Comments
Hello - hi! Thanks for the investigation. Not 100% sure what you try to achieve, though. Would you care to elaborate with more context/code samples? In general, the requester sets Sounds like you try to use a custom response queue for RPC, then I would suggest use the configuration in the documentation. In your example above.. what type is |
'consume' in my example is the IConsumerConfigurationBuilder used to configure the consumer part of the RequestAsync. I was trying to figure out how to configure the request operation without reading the documentation so this is why I was having problems.. I think using the recommended configuration from the documentation will solve the problem and I will try that, but also I think the library should try to avoid making available certain configurations that do not work. The ReplyToAddress is a pass-through property that converts between the PublicationAddress object and the ReplyTo string, so the reply_to header should be set in both cases.
|
I agree with you. It is a problem that most operations add extensions to |
As for your comment on ReplyTo. When i developed the RPC feature I got some odd behavior when using ReplyToAddress with direct RPC. This is perhaps changed as of the latest version of the underlying client - have not verified |
Nevertheless, thanks a bunch for the input - it is appreciated! |
Hello again, @MrGlenThomas - I've been giving this some though but have found a good solution to only allow certain operation specific configurations tied to the operation. My best idea so far is to have a custom pipe context for operations, like public interface IRequestPipeContext : IPipeContext { }
internal class RequestPipeContext : PipeContext, IRequestPipeContext
{
internal RequestPipeContext(IPipeContext context)
{
Properties = context?.Properties;
}
} and add extensions method to it public static IRequestPipeContext UseRequestConfiguration(this IRequestPipeContext context, Action<IRequestConfigurationBuilder> configuration)
{
context.Properties.Add(PipeKey.ConfigurationAction, configuration);
return context;
} The problem with this is that it forces you to configure operation specific things first, as if you would do something like Any ideas? |
I think the custom pipe context is the way to go. I have been trying to use RawRabbit v2 with my colleagues and we have spent a lot of time trying to figure out the correct configurations for each scenario we are using. I think it may be ok to need to use the fluent API in a certain order. I'm sure I have seen this with other fluent APIs like EntityFramework and FluentValidation. I expect the end result still functions the same without any problems? |
I have just spent the last hour trying to figure out why my queue name was being ignored when using SubscribeAsync.
The code compiled and ran without any obvious issues, but the queue generated did not have the correct name. It turned out that I had mistakenly used the UseRespondConfiguration instead of UseConsumerConfiguration. It is far too easy to make a mistake. |
I hear you, @MrGlenThomas. For me, working in this code base for a long time, it makes perfect sense that Respond Configuration is intended for the Respond Operation, the Request Configuration is for the Request Operation etc. However, that rule is not valid for Subscribe which uses the basic Consumer Configuration... so I see the problem 😜. You probably already know this, but the code compiles because all the methods on the I'm not 100% thrilled by the idea of operation specific pipe context. Right now I'm considering other ways to make it easier to use the right methods:
await responder.RespondAsync<BasicRequest, BasicResponse>(message =>
Task.FromResult(sent),
ctx => RespondConfig.Create(cfg => cfg.Consume(c => c
.WithRoutingKey("custom_key"))
.FromDeclaredQueue(q => q
.WithName("custom_queue")
.WithAutoDelete())
.OnDeclaredExchange(e => e
.WithName("custom_exchange")
.WithAutoDelete()))
.UseHostnameQueueSuffix(true)
); Feel free to come with more ideas around this 👍 |
I would love option 3. The different methods where very confusing when I started with rawrabbit. Especially that you can combine a method with a not matching rabbit operation is counter intuitive.
Outlook for Android herunterladen
On Mon, Oct 2, 2017 at 8:59 PM +0200, "Pär Dahlman" <[email protected]> wrote:
I hear you, @MrGlenThomas. For me, working in this code base for a long time, it makes perfect sense that Respond Configuration is intended for the Respond Operation, the Request Configuration is for the Request Operation etc. However, that rule is not valid for Subscribe which uses the basic Consumer Configuration... so I see the problem 😜.
You probably already know this, but the code compiles because all the methods on the IContext are extension methods that basically adds entries to an IDictionary<string, object>. There will always be a risk of adding an entry to the dictionary that is not picked up by the middlewares in the pipe, but it is impossible to guard against as the pipe might be manipulated by plugins, or middlewares might be reused in custom pipes etc.
I'm not 100% thrilled by the idea of operation specific pipe context. Right now I'm considering other ways to make it easier to use the right methods:
Putting extension operation specific extension methods in a a namespace containing the operation name. That way you get a hint that something is off if you include RawRabbit.Operations.Respond in a file where you are trying to do a SubscribeAsync
Check naming conventions, e.g. update the method name for configuring the Subscribe operation to UseSubscribeConfiguration and UsePublishConfiguration etc.
Provide a static builder classes for operation configuration (see below)
await responder.RespondAsync<BasicRequest, BasicResponse>(message =>
Task.FromResult(sent),
ctx => RespondConfig.Create(cfg => cfg.Consume(c => c
.WithRoutingKey("custom_key"))
.FromDeclaredQueue(q => q
.WithName("custom_queue")
.WithAutoDelete())
.OnDeclaredExchange(e => e
.WithName("custom_exchange")
.WithAutoDelete()))
.UseHostnameQueueSuffix(true)
);
Feel free to come with more ideas around this 👍
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
I'm not sure that static builder classes is any different to using the extension methods. Renaming the method to UseSubscribeConfiguration would be better, but it would be best if an incorrect configuration didn't compile. |
Formerly named PublisherConfiguration
Formaly know as UseConsumerConfiguration
I've started to look at this. First step is to make sure the configuration methods follow the naming convention. Might be looking at enforcing with specialized context, but before I do I need to investigate some more. |
OK, some major updates related to this ticket:
|
Looks good. I am holiday on the moment and look forward to trying it out when I get home. Thanks for looking into this. |
No worries - enjoy your holiday! 🌞 |
The fix for this has been released in rc1 🎉 |
There are many ways to configure response consumer for RPC requests and I think that many configurations do not function correctly. The header does not match the consumer setup so the response is not received.
The configurations I have tried so far and the resulting headers:
No consumer config
reply_to = amq.rabbitmq.reply-to.g2dkABNyYWJiaXRAOTk4NmQwZGExNTgzAABGtQAAAS8B.K8AGraiuM2eLasK+PQLhMQ==
consume.Consume(c => c.FromQueue("MyQueueName"))
reply_to = topic:///amq.rabbitmq.reply-to
consume.Consume(c => c.FromQueue("MyQueueName").WithRoutingKey("MyRoutingKey"))
reply_to = topic:///MyRoutingKey
consume.Consume(c => c.FromQueue("MyQueueName").OnExchange("MyExchange").WithRoutingKey("MyRoutingKey"))
reply_to = MyQueueName
consume.OnDeclaredExchange(exch => exch.WithName("MyExchange")).Consume(c => c.FromQueue("MyQueueName").WithRoutingKey("MyRoutingKey"))
reply_to = topic://MyExchange/MyRoutingKey
consume.OnDeclaredExchange(ex => ex.WithName("MyExchange")).FromDeclaredQueue(que => que.WithName("MyQueueName")).Consume(c => c.WithRoutingKey("MyRoutingKey")))
reply_to = topic://MyExchange/MyRoutingKey
consume.OnDeclaredExchange(exch => exch.WithName("MyExchange")).FromDeclaredQueue(que => que.WithName("MyQueueName"))
reply_to = topic://MyExchange/amq.rabbitmq.reply-to
Will add some more info when I have done some more testing.
The text was updated successfully, but these errors were encountered: