-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat: update aqmp config #310
feat: update aqmp config #310
Conversation
@Tenischev Hi, please have a look. Thanks. |
Hi @VaishnaviNandakumar |
Hi @Tenischev, I am not very familiar with docker images and configs so I didn't want to touch that. |
Hi @Tenischev , any update on this? |
Hi @VaishnaviNandakumar ,sorry I was busy this week, but I'm remember about pr. Will check soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for such long delay
|
||
{% elif asyncapi | isProtocol('amqp') %} | ||
{% for channelName, channel in asyncapi.channels() %} | ||
{%- set schemaName = channel.subscribe().message().payload().uid() | camelCase | upperFirst %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think channel.publish().message()
should be there, not subscribe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding of AMQP, the messages are published into an exchange and listens from a queue. This queue is defined under the subscriber/consumer module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a small logical/conceptual change required here. Will go through it once again and revise it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the messages are published into an exchange and listens from a queue" - agree.
But please have a look to explanation of AyncAPI publish\subscribe meaning - https://www.asyncapi.com/docs/tutorials/getting-started/hello-world
In this example, you only have one channel called hello. The sample application subscribes to this channel to receive hello {name} messages.
asyncapi: 2.6.0
info:
title: Hello world application
version: '0.1.0'
channels:
hello:
publish:
message:
payload:
type: string
pattern: '^hello .+$'
By means, if AsyncAPI define publish, that means that our generated application should be able to receive messages from the publish. And vice-versa, if AsyncAPI define subscribe, that means that our generated application should be able to sned messages to the subscribe.
The MessageHandlerService should be responsible for receiving messages to do so, listeners for the AsyncAPI publish channels should be defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I had earlier referred to this doc from which my understanding of publish/subscribe was the opposite of your explanation. That is, Publish sends messages and Subscriber receives it. Following this, I thought a listener would be most appropriate for a subscriber instead.
- With the upcoming v3 changes to AsyncAPI, publish/subscribe will be replaced by send/receive. Should I hold on to the current changes until then? It will require a rewrite of the template generation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I do not find the doc is opposite to what I wrote.
It is the same meaning:
If in AsyncAPI the publish
defined, then we should generate Subscriber\Consumer\MessageHandler.
Updating to AsyncAPI v3 is a global task and need to be implemented in whole template, not a part.
I would suggest to you to finish current PR for v2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I got where I went wrong. I was trying to validate the doc from a server perspective while it should've been the client instead. Sorry about that. So just to confirm, publish
defines the consumer and subscribe
defines the producer.
In that case for AMQP, queue and routing key should be defined under publish
while exchange should come under subscribe
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case for AMQP, queue and routing key should be defined under publish while exchange should come under subscribe.
Yes, this is also my understanding
Also, AMQP channel binding might be interesting for you in perspective of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Will update accordingly.
{{channelName}}: | ||
exchange: {{channel.publish().binding('amqp').exchange}} | ||
queue: {{channel.subscribe().binding('amqp').queue}} | ||
routingKey: {{channel.subscribe().binding('amqp').routingKey}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that this is a good idea to generate exchange and queue for all channels without taking into account if API really has publish\subscribe for this channel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tenischev So would the best way to go about it be to put in a check to see if the publish/ subscribe has been defined for that channel and then define exchange/queues etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think yes, of course this is not so critical for the configuration file, but it will keep it clean
partials/AmqpConfig.java
Outdated
|
||
{% for channelName, channel in asyncapi.channels() %} | ||
@Value("${amqp.{{- channelName -}}.exchange}") | ||
private String {{channelName}}Exchange; | ||
|
||
{% endif %}{% endfor %} | ||
{% for channelName, channel in asyncapi.channels() %}{% if channel.hasPublish() %} | ||
@Value("${amqp.queue.{{- channelName -}}}") | ||
@Value("${amqp.{{- channelName -}}.queue}") | ||
private String {{channelName}}Queue; | ||
|
||
{% endif %}{% endfor %} | ||
@Value("${amqp.{{- channelName -}}.routingKey}") | ||
private String {{channelName}}RoutingKey; | ||
{% endfor %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that this is a good idea to generate exchange and queue for all channels without taking into account if API really has publish\subscribe for this channel
partials/AmqpConfig.java
Outdated
{{channelName}}_Exchange, | ||
{{channelName}}_Binding | ||
{% else %} | ||
{{channelName}}_Queue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here, you could use {% if not loop.last %},{% endif %}
to add coma after _Binding instead of counting i
@Tenischev Hi, sorry I missed out on these comments. Slightly packed rn but I will try to get back to resolve these comments in a week. Thanks! |
…spring-template into feat/update-aqmp-config
…iNandakumar/java-spring-template into feat/update-aqmp-config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made the below changes in this update
- Restructured config yaml files to fit AMQP bindings as defined here. It wasn't done as per standard before.
- Added a
routingKey
key under bindings insubscriber
. The previous implementation had it hard coded. - Removed
BindingBuilder
fromAmqpConfig
. Initially it binded all the exchanges, queues and routing keys that came under a single channel together which wasn't conceptually right. - Added
hasPublisher()
andhasSubscriber()
checks.
Output Validation
@VaishnaviNandakumar, please check failed tests. |
@Tenischev which editor are you using? I'm working so far on IntelliJ but I am not able to run any tests. Need some help with the setup. |
@VaishnaviNandakumar I also use IntelliJ, but template test requires npm. Just run |
@Tenischev I have updated the PR to fix the failing tests. However, I do not have the bandwidth at the moment to add more for AMQP. Can that be taken up as a separate issue instead? |
@VaishnaviNandakumar sure it could be done in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VaishnaviNandakumar there is one veeeery small typo, could you please fix and i believe we will merge it
template/build.gradle
Outdated
@@ -15,6 +15,7 @@ repositories { | |||
dependencies { | |||
{%- if asyncapi | isProtocol('amqp') %} | |||
implementation('org.springframework.integration:spring-integration-amqp') | |||
implementation('org.springframework.integration:spring-integration-amqp') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it's doubled?
# Conflicts: # template/src/main/java/com/asyncapi/service/MessageHandlerService.java # tests/__snapshots__/kafka.test.js.snap # tests/__snapshots__/mqtt.test.js.snap # tests/__snapshots__/oneOf.test.js.snap # tests/__snapshots__/parameters.test.js.snap
🎉 This PR is included in version 1.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Test
Message passed to RabbitMQ via command line.
Output from App Listener
Message passed to RabbitMQ via command line.
Output from App Listener
Related issue(s)
Resolves #307