feat: add server param ands support for conditions

[derberg]

Description

• add server param
• add conditionalFiles support instead of custom hook

Related issue(s)
See also asyncapi/generator#358

[Tenischev]

@derberg thank you for the example, but i thought about this and decide that such limitations could be not appropriate in some cases.
Some time ago in Slack @damaru-inc asked about possibilities to set several addresses in one URL, because usually for Kafka you would like to set several addresses. Since, currently it's not possible, this template combine all servers.url with protocol=kafka into one string here https://github.com/asyncapi/java-spring-template/blob/master/template/src/main/resources/application.yml#L50
Another case when server parameter will not work - you have a legacy system, where old applications uses RabbitMQ as message broker and new ones uses Kafka.

My main point - current specification is not limited in paradigm "server per environment" or "one server per use" and i think this is not good approach to create such limitations in the template.

I suggest to rewrite custom hook to conditionalFiles, but without usage of server parameter

[derberg]

@Tenischev

Some time ago in Slack @damaru-inc asked about possibilities to set several addresses in one URL, because usually for Kafka you would like to set several addresses. Since, currently it's not possible, this template combine all servers.url with protocol=kafka into one string here https://github.com/asyncapi/java-spring-template/blob/master/template/src/main/resources/application.yml#L50

I think I recall the discussion but wasn't final advice to use variables instead of multiple servers? Otherwise, I think it will be difficult for you to keep compatibility in this template with other protocols. Most promoted approach to servers is to use them to define different environments, and without server flag template user won't be able to specify it.

Another case when server parameter will not work - you have a legacy system, where old applications uses RabbitMQ as message broker and new ones uses Kafka.

You mean one asyncapi has two servers that use different protocols

server per environment

It is of course not limited, but most common. You know, one thing is specification, and other is best practices 😉

[Tenischev]

I think I recall the discussion but wasn't final advice to use variables instead of multiple servers?

Use variables to distinguishes environments, or also comment from @fmvilas

Also, don’t forget different servers don’t necessarily mean different environments. Your servers could be name like production-1 , production-2, etc. Or something not even related to environments like my-broker .

You mean one asyncapi has two servers that use different protocols

Yes

[derberg]

@Tenischev you did not engage too much in discussion 😄

[Tenischev]

Different points of view regarding usage of several servers in the community... absolute free from spec side...
@derberg my position, AsyncAPI is too young to speak about best practices. So, i would prefer to use conditionFiles instead of custom hook, but avoid usage of server param for now.
But if you would like to introduce it, there other places in the template which must be updated, because currently template generates for all listed servers.
Parameters generation based on server - https://github.com/asyncapi/java-spring-template/blob/master/template/src/main/resources/application.yml
Dependency libs checks supported protocols from server list - https://github.com/asyncapi/java-spring-template/blob/master/template/build.gradle
And this java class - https://github.com/asyncapi/java-spring-template/blob/master/template/src/main/java/com/asyncapi/infrastructure/Config.java
And this java class - https://github.com/asyncapi/java-spring-template/blob/master/template/src/main/java/com/asyncapi/service/PublisherService.java

[derberg]

@Tenischev I don't want to be pushy here, no worries. I'm just pointing out that without server param and how it is used at the moment, it will be hard for you to support other protocols, and approach where servers are split per environment (the approach that is indirectly promoted in documentation as an example) ;) if above can be solved different way, I'm all ears

[Tenischev]

@derberg with the case to support several protocols in one AsyncAPI by one application is not a problem, as far as protocols compatible. I mean e.g. in mqtt we would name channels like statistic\{deviceId}\temperature when in kafka something like statistic.temperature.

Regarding server per environment, as i suggested before people could use variable in the URL to do it. But even if you would like to have a definition like server per environment, i would assume that in this case you will not have a different protocols in these servers 😀. And so, not the hook and condition files must be treated, the real place where server parameter should be took into account is application.yml because now it's generated for all servers in the AsyncAPI file.

[derberg]

@Tenischev in AsyncAPI you should not name channels like statistic.temperature as channel names should be A relative path to an individual channel. The field name MUST be in the form of a RFC 6570 URI template.

I didn't really get the rest of your message. How do you see it working with a single server and environments done with params and variables? You can have default variable sure, but somehow the user will need to pass the custom values for variables unless you're thinking about environment variables support. And no, users will most probably not specify different servers with different protocols, I don't see a use case, but that is not the point here.

now, instead of supporting server param you have to:

• you have to maintain a custom filter like https://github.com/asyncapi/java-spring-template/blob/master/filters/all.js#L35
• and even though you use it here https://github.com/asyncapi/java-spring-template/blob/master/template/src/main/java/com/asyncapi/infrastructure/Config.java#L10-L12 you still need to do ifs like this anyway if server.protocol() == 'mqtt'
• plus maintain a hook mentioned in this PR

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 30 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.
Thank you for your contributions ❤️

[Tenischev]

Hi @derberg ,
I would like to continue discussion regarding channel names.
For mqtt we have a clear requirements for topic names - https://mosquitto.org/man/mqtt-7.html
For kafka I couldn't provide so clear definition, but we have this article - https://riccomini.name/how-paint-bike-shed-kafka-topic-naming-conventions (which is very good from my point of view)
So, when for mqtt it's normal to have topic name like statistic\{deviceId}\temperature for the kafka you definitely should avoid such names.

[derberg]

@Tenischev oh man, you're back! long time

You are right, there is no one standard for different brokers, some have rules, some not, and this is why we should stick to one in the spec and then easily translate it to whatever is needed for a given broker. Spec is pretty clear here

[Tenischev]

@derberg Ok, let's apply this, but please not mandatory.
My statement as previous - it could be the case when you have two different message relay on different protocols, but your application work with both of them.
I understand, that if server is not mandatory we couldn't relay on conditionalFiles, but it's OK.

[derberg]

@Tenischev tricky part here is that if it is not mandatory, then users will have issues that conditionalFiles doesn't work and irrelevant parts are not deleted 🤷‍♂️

[Tenischev]

@derberg i know, because of that this template has a special hook to remove unnecessary files. Thus, conditionalFiles are backed up by the hook.
But thinking about how conditionalFiles are processed, we have a point for improvement. If we have a file for mqtt and in Asyncapi the mqtt protocol is not present at all, there is not much sense was server parameter defined or not, we shouldn't generate this file in any case. What do you think?

[derberg]

If we have a file for mqtt and in Asyncapi the mqtt protocol is not present at all, there is not much sense was server parameter defined or not, we shouldn't generate this file in any case

I think you got me lost here 😄
if you want to generate code from an AsyncAPI file, you need server information there, there is no other way. So if you want to generate app that talks to kafka, AsyncAPI file must specify Kafka, how can we have a file for mqtt and in Asyncapi the mqtt protocol is not present at all?

[derberg]

@Tenischev I think it is fair to say this PR should wait for asyncapi/spec#465. What do you think? I think everything else is just a workaround that replaces another workaround which is not how improvements should be done 😄

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 60 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.
Thank you for your contributions ❤️