slackapi / node-slack-sdk

Slack Developer Kit for Node.js
https://slack.dev/node-slack-sdk
MIT License
3.26k stars 656 forks source link

providing a way to disable message content being logged #1786

Closed Parama92 closed 1 month ago

Parama92 commented 2 months ago

Summary

In this PR, we aim to prevent the logging of message content when a request error occurs while attempting to invoke any of Slack's web APIs.

It solves this issue.

The problem at a glace -

The Error type - WebAPIRequestError, contains a property called original which holds the raw error received from axios. The config property within this original is a part of the error data received from axios. This config property might in turn hold a data property, which is the "message" we are trying to opt-out of showing.

Tests conducted -

  1. Error log with the opt-out config - Error with config Note: The original property is missing from the Error object.

  2. Error log without adding the config - Error without config Note: The original property is present in the Error object along with all the details of the request.

Requirements

salesforce-cla[bot] commented 2 months ago

Thanks for the contribution! Before we can merge this, we need @Parama92 to sign the Salesforce Inc. Contributor License Agreement.

Parama92 commented 2 months ago

Thanks for the feedback! I have addressed the comments and have re-requested a review

Parama92 commented 2 months ago

I have addressed the issues that caused the linter to fail. It is running fine on my local machine now. Do you mind running the workflow once again?

filmaj commented 2 months ago

Hmm, seems like the tests are still failing. It looks to me like something is blocking the testing process; see the CI output here. I can also reproduce this behaviour locally on my macbook pro using node v20.

filmaj commented 2 months ago

I believe what is happening is that, because in the tests we return an error / a non-200 HTTP response, web-api will by default attempt to retry issuing the request (see this default option). The default retry mechanism is "10 retries in 30 minutes" - meaning, if the Slack API returns an error result (like the "fake" API behaviour in the new tests you added), web-api will block the process in order to retry the request, blocking the process for up to 30 mins! I believe this is what is causing the tests to hang.

As per our documentation on retries, try setting this option in the tests on WebClient to turn off retries:

{
  retryConfig: { retries: 0 },
}
filmaj commented 2 months ago

It's also a good idea to run npm test locally before pushing any changes to make sure everything works as expected on your machine.

Parama92 commented 1 month ago

Hmm, makes sense. Strange that it seemed to work fine on my local system earlier. Thanks for the heads-up though! I make the necessary changes and ran npm test. I am getting an unrelated error, which didn't show up in any of the test runs here 🤔 - Error

Hopeful that this time it will work out fine! 🤞