feat: add RabbitMQ publisher #137

anandsunderraman · 2022-03-08T15:32:06Z

Description

Creating a amqp publisher from asyncapi file

Related issue(s)
#132

Summary of Changes

Added functionality for amqp publishers under components/Handlers.js
Added functionality to create amqpPublisher under components/Publisher.js
Added util functions under components/common.js
Added a util to convert any model to watermill message under template/asyncapi/payload.js
Refactored code to produce the main.go file in template/index.js
Added a bunch of unit tests

smoya · 2022-03-14T14:43:47Z

components/Handlers.js

@@ -15,28 +15,81 @@ func ${operation}(msg *message.Message) error {
 }
 `;

-function SubscriptionHandlers({ channels }) {
+export function SubscriptionHandlers({ channels }) {
  return Object.entries(channels)
    .map(([channelName, channel]) => {
      if (channel.hasPublish()) {
        const operation = pascalCase(channel.publish().id());


operationId is not mandatory so can be empty. I didn't realize the fact we were using it for generating the method names.

I think we should generate one if it's empty. Maybe channel name + operation type.

Exactly! Either:

Some method (handler function) name is auto generated (and have it documented). Currently, nothing is included, and ofc, we have a syntax error in two (declaration and usage) places.

Or the docs of this generator should explicit that operationId should be used to properly define your handler name.

For a good UX, I would prefer to output the fact that some generated names where included, since no explicit operationId exists in the spec.

And there is also $id property of payload, based on which the proper Go struct naming is done.
That's another useful and optional thing (without it you get a name, but it's something like AnonymousSchema which is not that not that great. It's ok that it is generated, no syntax errors, but the naming can be improved).

@smoya @dxps thanks for noticing this.
I am tempted to generate a method name based if operationId is not present.
But as @dxps mentions we would have to make sure there are no duplicate names.
Another thing I notice is that channel names can have special characters. So we would have to remove all special characters.
I like @dxps's suggestion to mandate an operationId for this spec. Meanwhile I can also check other generators on how they handle this scenario

@anandsunderraman I reviewed the code again and I noticed you didn't add a guard checking if the operationId is present, forcing the user to set it or throwing an error otherwise. You have an example in nodejs-ws-template.

Also, I would mention this in the README file as for example java-spring-template does.

Good catch! Forgot about operationId, although I mentioned it initially.

The other part that could get improved is related to the $id property of payload, based on which the proper Go struct naming is done. That's another useful and optional thing, without specifying it you get a name like AnonymousSchema which is not that great or useful.

components/Handlers.js

dxps · 2022-03-24T21:15:02Z

@anandsunderraman Could you please react on the provided feedback?
Meanwhile, I will play with this new contribution and get back with feedback and contribute.
Thanks!

anandsunderraman · 2022-03-24T21:16:28Z

@anandsunderraman Could you please react on the provided feedback? Meanwhile, I will play with this new part as well. Thanks!

@dxps I was waiting for all reviewers to provide feedback, so that I can make my changes all at once, preventing multiple back and forth

dxps · 2022-03-26T15:13:30Z

@anandsunderraman While just testing it with the spec mentioned here, which the spec does not yet declare any subscribe operation at the channel level, there are two (small) issues:

In handlers.go, "context" is imported but not used.
In subscribers.go, there is no GetAMQPPublisher generated function, thus in main.go there is the corresponding error.

Used ag --debug /path/to/spec.yaml https://github.com/anandsunderraman/go-watermill-template/tree/rabbit-publisher -o gen_go for testing it.

template/index.js

dxps

Just a heads-up for now (for the 1st one, I'll look into it myself as well, to see how harmful it can be):

In payloads.go, there is this warning:
In main.go, there is this warning:

This can easily be solved by checking for that error, by introducing these after that call:
```
 if err != nil {
 	return err
 }
```
In handlers.go, context package is being used, but not included in the import section.
This is another easy to solve one.

Besides these, I tested the generated code and is functionally working.
So, thank you for doing this! 🙏

dxps · 2022-03-26T17:47:54Z

Ok, I understand the 1st warning (copylocks) and it's quite problematic.

To solve it, the code should be slightly updated as follows:

In payloads.go file, payloadToMessage function should return a pointer to the message instead of a copy
(that does a copy of an included Mutex, that's why the warning). Basically, it should look like this:

func PayloadToMessage(i interface{}) (*message.Message, error) {
     var m message.Message

     b, err := json.Marshal(i)
     if err != nil {
             return &m, nil
     }
     m.Payload = b

     return &m, nil
}

And just adapt the handlers in handlers.go file, to call the Publish(...) method accordingly, that is:
```
return a.Publish("{topicName}", m)
```

dxps · 2022-03-26T18:08:36Z

Last but not least, some small improvements that can be included in main.go:

Close the connection at the end.
Although it's a generated code, it should guide others how to do it properly.
Ofc, without it, the sample still works, and you could just see RabbitMQ's log complaining about it
(with client unexpectedly closed TCP connection). The code to include right before the last return nil line:
```
if err := amqpPub.Close(); err != nil {
	log.Printf("Error while closing AMQP connection: %v\n", err)
}
```
I might be too picky here, but startAMQPPublishers(_) function is not actually starting something (that should eventually be stopped), but actually it showcases how publishing should be done. Therefore, I'd rename it as doAMQPPublishing() or something like that. And ofc, the error message being logged in main(_) can be error publishing: ....

derberg

I did not look into logic code as I see others already did it.

I only left one comment about one script name. Also please change PR title to feat: add RabbitMQ publisher. I also think it would be worth extending the readme overview with some more info on what users can generate with this template. Also good to add development section and explain the dev script.

One curiosity question: why did you add dependency related to babel?

package.json

anandsunderraman · 2022-04-04T12:05:19Z

here

@dxps again thanks for finding this issue I will look to address it

anandsunderraman · 2022-04-04T13:32:21Z

Close the connection at the end.
Although it's a generated code, it should guide others how to do it properly.
Ofc, without it, the sample still works, and you could just see RabbitMQ's log complaining about it
(with client unexpectedly closed TCP connection). The code to include right before the last return nil line:

@dxps Completely agree with this suggestion. However I feel this needs more investigation to be done in the watermill.io fashion. I will create a separate issue to handle this

anandsunderraman · 2022-04-08T18:00:31Z

One curiosity question: why did you add dependency related to babel?
@derberg I introduced a change in this PR at: https://github.com/asyncapi/go-watermill-template/pull/137/files#diff-4bab0aa7cca57d1ac8ae38749f9f50f6c2c91b81eb46e28ad46ebcf0e375d1ceR41

const result = render(<SubscriptionHandlers channels={doc.channels()} />)

If I did not add babel I was getting the error:

Add @babel/preset-react (https://git.io/JfeDR) to the 'presets' section of your Babel config to enable transformation.
    If you want to leave it as-is, add @babel/plugin-syntax-jsx (https://git.io/vb4yA) to the 'plugins' section to enable parsing.

dxps · 2022-04-10T20:14:07Z

With these recently introduced updates, the generated code for the publisher looks good and it works.
LGTM 👍 Thanks!

test/asyncapi.yaml

test/files/docWithAMQPSubscriber.yml

smoya

Pasting here as the conversation was marked as solved (now I unresolved).

https://github.com/asyncapi/go-watermill-template/pull/137/files#r847178655

@anandsunderraman I reviewed the code again and I noticed you didn't add a guard checking if the operationId is present, forcing the user to set it or throwing an error otherwise. You have an example in nodejs-ws-template.

Also, I would mention this in the README file as for example java-spring-template does.

anandsunderraman · 2022-04-12T14:05:59Z

Pasting here as the conversation was marked as solved (now I unresolved).

https://github.com/asyncapi/go-watermill-template/pull/137/files#r847178655

@anandsunderraman I reviewed the code again and I noticed you didn't add a guard checking if the operationId is present, forcing the user to set it or throwing an error otherwise. You have an example in nodejs-ws-template.

Also, I would mention this in the README file as for example java-spring-template does.

@smoya apologies. Thanks for pointing out to the other generator examples.
I have addressed them now and added unit tests.

smoya

Superb work @anandsunderraman! Thanks for such an effort; It has been a pleasure to review this PR :)

LGTM!🚀 🌔

anandsunderraman · 2022-04-12T21:27:10Z

thanks @smoya !!

asyncapi-bot · 2022-04-12T21:31:06Z

🎉 This PR is included in version 0.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

anandsunderraman requested review from smoya and asyncapi-bot-eve as code owners March 8, 2022 15:32

anandsunderraman changed the title ~~feat:rabbit-publisher~~ feat: rabbit-publisher Mar 11, 2022

anandsunderraman requested a review from derberg March 11, 2022 12:46

smoya reviewed Mar 14, 2022

View reviewed changes

components/Handlers.js Outdated Show resolved Hide resolved

smoya reviewed Mar 14, 2022

View reviewed changes

components/Handlers.js Outdated Show resolved Hide resolved

dxps reviewed Mar 26, 2022

View reviewed changes

template/index.js Outdated Show resolved Hide resolved

dxps reviewed Mar 26, 2022

View reviewed changes

derberg reviewed Mar 28, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

anandsunderraman force-pushed the rabbit-publisher branch from 18be24d to abe6d3b Compare April 4, 2022 13:33

anandsunderraman changed the title ~~feat: rabbit-publisher~~ feat: add RabbitMQ publisher Apr 8, 2022

anandsunderraman force-pushed the rabbit-publisher branch from abe6d3b to cacb656 Compare April 8, 2022 19:37

anandsunderraman requested review from derberg and smoya April 8, 2022 19:55

smoya reviewed Apr 11, 2022

View reviewed changes

test/asyncapi.yaml Outdated Show resolved Hide resolved

smoya reviewed Apr 11, 2022

View reviewed changes

test/files/docWithAMQPSubscriber.yml Outdated Show resolved Hide resolved

derberg previously approved these changes Apr 11, 2022

View reviewed changes

smoya requested changes Apr 11, 2022

View reviewed changes

anandsunderraman dismissed derberg’s stale review via 1e21991 April 12, 2022 14:04

anandsunderraman force-pushed the rabbit-publisher branch from abe94d8 to 1e21991 Compare April 12, 2022 14:04

anandsunderraman added 21 commits April 12, 2022 10:06

adding rabbit publish handler

bc50ba9

feat: handling model name

989fc61

feat: adding converter for payloads

d5974e6

feat:rabbit-publisher add unit tests and handlers

e612114

feat:rabbit-publisher fixing imports

78e6906

feat:rabbit-publisher add more unit tests

8cc9ee8

feat:rabbit-publisher cleaning up code

97fe4af

feat:rabbit-publisher cleaning up payload

4826a71

feat:rabbit-publisher adding publisher code

fa531ea

feat:rabbit-publisher fixing lint checks

c82cf7a

feat:rabbit-publisher fixing tests

8e0f7cf

feat: rabbi-publisher cleanup code

2ed2fc8

feat: rabbit-publisher adding husky

d498580

feat: addressing issues in PR

a3ff9d3

feat: addressing code review comments

bcc5d1e

feat: addressing code review comments

c6587ec

feat: formatting changes

ae798ac

feat: contribution guide

2feee2b

feat: corrections to readme

313870d

feat: formatting read me

c362ffe

feat: add guard for missing operationId

d0b9812

anandsunderraman force-pushed the rabbit-publisher branch from 1e21991 to d0b9812 Compare April 12, 2022 14:06

anandsunderraman requested a review from smoya April 12, 2022 14:06

smoya approved these changes Apr 12, 2022

View reviewed changes

anandsunderraman merged commit 7d9587d into asyncapi:master Apr 12, 2022

asyncapi-bot added the released label Apr 12, 2022

anandsunderraman mentioned this pull request Apr 15, 2022

Rabbit publisher #132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add RabbitMQ publisher #137

feat: add RabbitMQ publisher #137

anandsunderraman commented Mar 8, 2022 •

edited

Loading

smoya Mar 14, 2022 •

edited

Loading

dxps Mar 26, 2022 •

edited

Loading

dxps Mar 26, 2022

anandsunderraman Apr 4, 2022

smoya Apr 11, 2022 •

edited

Loading

dxps Apr 11, 2022 •

edited

Loading

dxps commented Mar 24, 2022 •

edited

Loading

anandsunderraman commented Mar 24, 2022

dxps commented Mar 26, 2022 •

edited

Loading

dxps left a comment •

edited

Loading

dxps commented Mar 26, 2022

dxps commented Mar 26, 2022

derberg left a comment

anandsunderraman commented Apr 4, 2022

anandsunderraman commented Apr 4, 2022

anandsunderraman commented Apr 8, 2022

dxps commented Apr 10, 2022 •

edited

Loading

smoya left a comment •

edited

Loading

anandsunderraman commented Apr 12, 2022

smoya left a comment

anandsunderraman commented Apr 12, 2022

asyncapi-bot commented Apr 12, 2022

feat: add RabbitMQ publisher #137

feat: add RabbitMQ publisher #137

Conversation

anandsunderraman commented Mar 8, 2022 • edited Loading

smoya Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

dxps Mar 26, 2022 • edited Loading

Choose a reason for hiding this comment

dxps Mar 26, 2022

Choose a reason for hiding this comment

anandsunderraman Apr 4, 2022

Choose a reason for hiding this comment

smoya Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

dxps Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

dxps commented Mar 24, 2022 • edited Loading

anandsunderraman commented Mar 24, 2022

dxps commented Mar 26, 2022 • edited Loading

dxps left a comment • edited Loading

Choose a reason for hiding this comment

dxps commented Mar 26, 2022

dxps commented Mar 26, 2022

derberg left a comment

Choose a reason for hiding this comment

anandsunderraman commented Apr 4, 2022

anandsunderraman commented Apr 4, 2022

anandsunderraman commented Apr 8, 2022

dxps commented Apr 10, 2022 • edited Loading

smoya left a comment • edited Loading

Choose a reason for hiding this comment

anandsunderraman commented Apr 12, 2022

smoya left a comment

Choose a reason for hiding this comment

anandsunderraman commented Apr 12, 2022

asyncapi-bot commented Apr 12, 2022

anandsunderraman commented Mar 8, 2022 •

edited

Loading

smoya Mar 14, 2022 •

edited

Loading

dxps Mar 26, 2022 •

edited

Loading

smoya Apr 11, 2022 •

edited

Loading

dxps Apr 11, 2022 •

edited

Loading

dxps commented Mar 24, 2022 •

edited

Loading

dxps commented Mar 26, 2022 •

edited

Loading

dxps left a comment •

edited

Loading

dxps commented Apr 10, 2022 •

edited

Loading

smoya left a comment •

edited

Loading