-
Notifications
You must be signed in to change notification settings - Fork 1
Add unsubscribe method. #2
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
base: master
Are you sure you want to change the base?
Conversation
lib/latest/QueuesRoutingTable.js
Outdated
*/ | ||
getHandlerRefsByProperties({ queue, topic, exchange }) { | ||
try { | ||
const { consumerTag } = Object.values(this.handlers).find(handler => |
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.
can you please put find on a new line?
lib/latest/QueuesRoutingTable.js
Outdated
handler.queue === queue && | ||
handler.topic === topic && | ||
handler.exchange === exchange); | ||
return { consumerTag, index: this.consumerTags[consumerTag] }; |
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.
please split on multiple lines. each property on a line
lib/latest/MQRouter.js
Outdated
try { | ||
({ consumerTag, index } = this.queuesRoutingTable.getHandlerRefsByProperties(mqProps)); | ||
this.queuesRoutingTable.unregister({ index }); | ||
return await this.connector.unsubscribe({ consumerTag }); |
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.
please first unsubscribe and then delete it from queueRoutingTable. it might have undesired side effects between deleting from routing table and actual subscribing; also if it fails to unsubscribe it will remain deleted from routing table
lib/latest/MQRouter.js
Outdated
* @returns {Boolean} returns true if unsubscribed | ||
* @public | ||
*/ | ||
async unsubscribe(mqProps) { |
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.
please use destructuring for function signature
lib/latest/MQRouter.js
Outdated
let consumerTag = null; | ||
let index = null; | ||
try { | ||
({ consumerTag, index } = this.queuesRoutingTable.getHandlerRefsByProperties(mqProps)); |
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.
since they are not reassigned use const instead of let
lib/latest/QueuesRoutingTable.js
Outdated
handler.exchange === exchange); | ||
return { consumerTag, index: this.consumerTags[consumerTag] }; | ||
} catch (cause) { | ||
return null; |
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.
return a meaningful error. you can return index and consumer tag null, but don't rely on construction error of language (undefined as property throws error)
also in case of success (not found it is considered a success not an error, from routing table point of view) response should be consistent.
test/MQRouter/Unsubscribe.js
Outdated
topic: dummyTopic, | ||
exchange, | ||
}) | ||
.should.be.fulfilled |
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.
no need to test the prepare flow. subscribe should be tested before and no need to test it again
test/MQRouter/Unsubscribe.js
Outdated
}) | ||
.should.be.fulfilled | ||
.then((success) => { | ||
expect(success).to.be.eql(true); |
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.
please assert that the actual connector unsubscribe was called
}) | ||
.should.be.rejected | ||
.then((error) => { | ||
expect(error.name).to.be.eql('MQ_ROUTER_UNSUBSCRIBE'); |
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.
assert here that you have as cause the actual error trowed by the stub
done(); | ||
}); | ||
|
||
it('Should return null if queue not registered', (done) => { |
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.
these tests will need to be updated after the changes i pointed out
lib/latest/MQRouter.js
Outdated
topic, | ||
exchange, | ||
}) { | ||
const { consumerTag, index } = this.queuesRoutingTable.getHandlerRefsByProperties({ |
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.
if this fails it will not be caught by catch and will throw a non standard error
index: this.consumerTags[consumerTag], | ||
}; | ||
} catch (cause) { | ||
return { |
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.
please return here a specific error with cause as cause
in try block return null if consumerTag
is undefined (not found)
element not found is a perfect response from this function. for actual case it is an error, but this is valid for the caller of getHandlerRefsByProperties
. there can also be a check where you want to test if there is a handler associated for info provided and if it is not you need to do something else. by throwing error here you need to wrap it and do something on catch block.
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 can throw one error when handler not found and one when index not found by consumerTag:
getHandlerRefsByProperties({
queue,
topic,
exchange,
}) {
const handler = Object.values(this.handlers)
.find(el =>
el.queue === queue &&
el.topic === topic &&
el.exchange === exchange);
if (handler) {
if (handler.consumerTag) {
const index = this.consumerTags[handler.consumerTag];
if (index) {
return {
consumerTag: handler.consumerTag,
index,
};
}
throw Reflect.construct(IError, [{
name: 'MQ_ROUTING_TABLE_UNKNOWN_CONSUMER_TAG',
source: `${this.sourceIdentifier}.getHandlerRefsByProperties`,
}]);
}
return {
consumerTag: null,
index: null,
};
}
throw Reflect.construct(IError, [{
name: 'MQ_ROUTING_TABLE_UNKNOWN_HANDLER',
source: `${this.sourceIdentifier}.getHandlerRefsByProperties`,
}]);
}
}
No description provided.