fix: send rabbitmq message to exchange and refactor #1872
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
minottic
changed the title
minottic
changed the title
minottic
force-pushed
the
send_to_exchange
branch
5 times, most recently
from
0307a54 to
0072cfb
Compare
4 tasks
minottic
added a commit
that referenced
this pull request
May 6, 2025
<!-- Follow semantic-release guidelines for the PR title, which is used in the changelog. Title should follow the format `<type>(<scope>): <subject>`, where - Type is one of: build|chore|ci|docs|feat|fix|perf|refactor|revert|style|test|BREAKING CHANGE - Scope (optional) describes the place of the change (eg a particular milestone) and is usually omitted - subject should be a non-capitalized one-line description in present imperative tense and not ending with a period See https://github.com/angular/angular.js/blob/master/DEVELOPERS.md#-git-commit-guidelines for more details. --> ## Description <!-- Short description of the pull request --> Remove variables that are not needed in tests. ## Motivation <!-- Background on use case, changes needed --> This will allow to raise error when connection to rabbitmq fails from #1872 ## Fixes <!-- Please provide a list of the issues fixed by this PR --> * Remove variables that are not needed in tests. * ## Changes: <!-- Please provide a list of the changes implemented by this PR --> * changes made ## Tests included - [ ] Included for each change/fix? - [ ] Passing? <!-- Merge will not be approved unless tests pass --> ## Documentation - [ ] swagger documentation updated (required for API changes) - [ ] official documentation updated ### official documentation info <!-- If you have updated the official documentation, please provide PR # and URL of the updated pages -->
totopoloco
pushed a commit
that referenced
this pull request
May 7, 2025
<!-- Follow semantic-release guidelines for the PR title, which is used in the changelog. Title should follow the format `<type>(<scope>): <subject>`, where - Type is one of: build|chore|ci|docs|feat|fix|perf|refactor|revert|style|test|BREAKING CHANGE - Scope (optional) describes the place of the change (eg a particular milestone) and is usually omitted - subject should be a non-capitalized one-line description in present imperative tense and not ending with a period See https://github.com/angular/angular.js/blob/master/DEVELOPERS.md#-git-commit-guidelines for more details. --> ## Description <!-- Short description of the pull request --> Remove variables that are not needed in tests. ## Motivation <!-- Background on use case, changes needed --> This will allow to raise error when connection to rabbitmq fails from #1872 ## Fixes <!-- Please provide a list of the issues fixed by this PR --> * Remove variables that are not needed in tests. * ## Changes: <!-- Please provide a list of the changes implemented by this PR --> * changes made ## Tests included - [ ] Included for each change/fix? - [ ] Passing? <!-- Merge will not be approved unless tests pass --> ## Documentation - [ ] swagger documentation updated (required for API changes) - [ ] official documentation updated ### official documentation info <!-- If you have updated the official documentation, please provide PR # and URL of the updated pages -->
sbliven
approved these changes
May 14, 2025
There was a problem hiding this comment.
This was a bit hard to review because of all the refactoring. If I read it right the new features are
- Single channel for all messages which stays open throughout the nest lifetime
- Use
publishinstead ofsendToQueue, which allows rabbitMQ to do routing.
The OnError decorator is a bit weird. You never use the "log" feature, and passing an error prefix via _error doesn't seem typesafe. I would have done error handling differently, but I guess this works.
|
essentially the only change is the first commit, the rest is refactoring. The decorator param (log) was necessary before #1886 and I was lazy and did not change it after that was merged (which also meant the BE started fine if no connection could be established). The channel stayed open even before since it was created in the constructor, but lifecycle hooks are used here to make sure stuff is initialized before being used. |
despadam
reviewed
May 14, 2025
Junjiequan
reviewed
May 14, 2025
Comment on lines
+93
to
+98
| @OnError() | ||
| private async connect(): Promise<void> { | ||
| this._error = "Cannot connect to RabbitMQ"; | ||
| this.connection = await amqplibConnect(this.connectionOptions); | ||
| this._error = "Channel error in RabbitMQService: "; | ||
| this.channel = await this.connection.createChannel(); |
There was a problem hiding this comment.
In my view, the OnError decorator brings unnecessary complexities. I personally would use try catch or just .catch() here to make it easy to read. But the idea here is to make it DRY I guess.
Comment on lines
-19
to
-30
| constructor(private readonly configService: ConfigService) { | ||
| Logger.log("Initializing RabbitMQService.", "RabbitMQService"); | ||
|
|
||
| const hostname = this.configService.get<string>("rabbitMq.hostname"); | ||
| const port = this.configService.get<number>("rabbitMq.port"); | ||
| const username = this.configService.get<string>("rabbitMq.username"); | ||
| const password = this.configService.get<string>("rabbitMq.password"); | ||
| constructor(private readonly configService: ConfigService) {} | ||
|
|
||
| if (!hostname || !port || !username || !password) { | ||
| Logger.error( | ||
| "RabbitMQ is enabled but missing one or more config variables.", | ||
| "RabbitMQService", |
There was a problem hiding this comment.
I prefer the old code, as you can see the required fields immediately, but I'm not against having it programmatically either
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Send message to channel rather than to queue, which bypassed the bound exchange. Class refactored to use async await.
Motivation
Using the
channel.publishmethod allows sending to the specified exchange and persists the message to the bound queue.Fixes
channel.publishinstead ofchannel.sendToQueueChanges:
Tests included
Documentation
official documentation info