Skip to content

Conversation

@JappyMondo
Copy link

I recently introduced the Blacklist feature (different username @jappyjan , same person :D but this time using my work account ^^)
Tuns out that i accidentally introduced a bug which causes the moleculer broker/service to kill the process instead of just not executing an action.

This PR fixes it by replacing the throwing of an error when a service is blacklisted with a simple return like we do it for the whitelist too.

@jappyjan
Copy link
Contributor

Could someone take a look at this please?

if (alias.action && route.hasBlacklist) {
if (this.checkBlacklist(route, alias.action)) {
this.logger.debug(` The '${alias.action}' action is in the blacklist!`);
throw new ServiceNotFoundError({ action: alias.action });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you should keep the error throwing because we do it in whitelist as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/index.js Outdated
// Blacklist check
if (route.hasBlacklist) {
if (this.checkBlacklist(route, action.name)) {
this.logger.debug(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the log, no log message in whitelist as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@JappyMondo
Copy link
Author

sory @icebob... took me agfain forever to continue here... too much stuff going on at work.
i have no addressed all of your review comments and tested everything again with our setup.
seems to all be fine.
let me know if there is more to change.

@JappyMondo
Copy link
Author

ping. @icebob

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants