Add ability to describe exchange and routing key for AMQP consumer and producer #69
Conversation
...qp-plugin/src/main/java/io/github/stavshamir/springwolf/producer/SpringwolfAmqpProducer.java
Show resolved
Hide resolved
| * @param payloadType type of the payload | ||
| * @return amqp producer data | ||
| */ | ||
| public static ProducerData createAMQPProducerData(String channelName, |
There was a problem hiding this comment.
Probably shouldn't be placed as a method on ProducerData since this is amqp-specific.
...n/java/io/github/stavshamir/springwolf/asyncapi/scanners/channels/RabbitChannelsScanner.java
Show resolved
Hide resolved
|
I only had time to skim through, but this looks great. I will go over the finer details tomorrow and will probably have some comments, but again this looks great 🙂 |
There was a problem hiding this comment.
Thanks again @DmitriButorchin for providing this so quickly!
I really like the additions and it's great to have someone with the domain specific knowledge contributing to this project.
I have some few comments I would like you to address, and then I will merge the PR.
| /** | ||
| * The class object of the payload published by this producer. | ||
| */ | ||
| private Class<?> payloadType; | ||
|
|
||
| /** | ||
| * The binding of the producer. | ||
| * The operation binding of the producer. |
There was a problem hiding this comment.
We should change the name of this field to operationBinding now that we have channelBinding. It's a breaking change but that's ok.
| @@ -0,0 +1,27 @@ | |||
| package io.github.stavshamir.springwolf.asyncapi.serializers; | |||
There was a problem hiding this comment.
I'm not sure we need this at this point - currently the binding tab in the UI displays only the operation binding, so this will appear only in the document itself. It's ok to have it serialized as it is without this custom serializer.
There was a problem hiding this comment.
I added it because the endpoint wouldn't work, it resulted in a 500 internal error failing to serialize the channelBinding field. Is there other way to fix this problem?
There was a problem hiding this comment.
Hmm ok, so let's keep it, I can figure it out later.
| @@ -26,6 +28,7 @@ void postConstruct() { | |||
|
|
|||
| private void registerKafkaOperationBindingSerializer() { | |||
| SimpleModule module = new SimpleModule(); | |||
| module.addSerializer(KafkaChannelBinding.class, new KafkaChannelBindingSerializer()); | |||
There was a problem hiding this comment.
See comment in KafkaChannelBindingSerializer.
| @Override | ||
| protected Map<String, ? extends ChannelBinding> buildChannelBinding(RabbitListener annotation) { | ||
| AMQPChannelBinding.ExchangeProperties exchangeProperties = new AMQPChannelBinding.ExchangeProperties(); | ||
| String exchangeName = Stream.of(annotation.bindings()) |
There was a problem hiding this comment.
I think lines 79 to 89 should be extracted to another method getExchangeName
| @Override | ||
| protected Map<String, ? extends OperationBinding> buildOperationBinding(RabbitListener annotation) { | ||
| return ImmutableMap.of("amqp", new AMQPOperationBinding()); | ||
| /* | ||
| The routing key is taken from the binding. As the key field in the @QueueBinding can be an empty array, |
There was a problem hiding this comment.
Good comment, it's important to have this explanation here
| Also in the case when there is no binding for the queue present at all, it means it uses the fact that | ||
| RabbitMQ automatically binds default exchange to a queue with queue's name as a routing key. | ||
| */ | ||
| List<String> routingKeys = Stream.of(annotation.bindings()) |
There was a problem hiding this comment.
Like getExchangeName, I think lines 106 to 125 should be extracted to getRoutingKeys
| this.rabbitTemplate = rabbitTemplate; | ||
| public SpringwolfAmqpProducer(ChannelsService channelsService, List<RabbitTemplate> rabbitTemplates) { | ||
| this.channelsService = channelsService; | ||
| this.rabbitTemplate = rabbitTemplates.get(0); |
There was a problem hiding this comment.
Could you please remind me again why it doesn't matter which template is used here?
There was a problem hiding this comment.
The difference between multiple rabbit template beans is preset default exchange and/or routing key. So you could use the overload .send method with just routingKey as param and payload, or just payload.
Because we're getting the exchange name and routing key from the context and we're using the method with all parameters (i.e. exchange, routing key and payload) we don't care which rabbit template to use.
| } | ||
|
|
||
| public void send(String channelName, Map<String, Object> payload) { | ||
| rabbitTemplate.convertAndSend(channelName, payload); | ||
| ChannelItem channelItem = channelsService.getChannels().get(channelName); | ||
| String exchange = ""; |
There was a problem hiding this comment.
Let's extract getExchange and getRoutingKey here as well
| import java.util.Collections; | ||
|
|
||
| @UtilityClass | ||
| public class ProducerDataUtil { |
There was a problem hiding this comment.
I agree we should provide a convenient way to create the producer data, but I have a different idea that I prefer to implement (make ProducerData an interface, and have each plugin implement it with a builder that will map the input to the correct bindings). So please remove this file and I will add this after I merge your PR.
|
Thank you for the comments, I'll update the PR either later today or tomorrow. |
…d producer. Fix crash when having multiple instances of RabbitTemplate. related to #67
|
I have applied the suggested comments :) |
|
Great job! I will add a convenient way to create amqp producer data and try to squeeze in support for class level RabbitListener and create a new release by Thursday. |
related to #67