Skip to content
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

providing a way to disable message content being logged #1786

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 49 additions & 1 deletion packages/web-api/src/WebClient.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ require('mocha');
const fs = require('fs');
const path = require('path');
const { Agent } = require('https');
const { assert } = require('chai');
const { assert, expect } = require('chai');
const { WebClient, buildThreadTsWarningMessage } = require('./WebClient');
const { ErrorCode } = require('./errors');
const { LogLevel } = require('./logger');
Expand Down Expand Up @@ -1612,6 +1612,54 @@ describe('WebClient', function () {
});
});

describe('has an option to suppress request error from Axios', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding these tests!

One thing we should do is ensure we mock out any calls to the slack.com API. Right now, since your tests are actually invoking e.g. conversations.list() method, this is issuing an HTTP request to slack.com/api. Not great, as we are a) unnecessarily creating traffic for Slack and b) have less control over the outcome of our tests. What if, during a test run, the Slack API goes down? Should the tests fail because of some service degradation? Probably not.

To handle this, we use the nock module to mock out API calls. Take a look at the nock setup and use on lines 150-175 of this file. Using nock, we can finely control a 'fake' response from the Slack API. This way, our tests will avoid both problems a) and b) above!

Additionally, since you have total control over the "Slack API" response using nock, you can not only test for the presence (and absence) of the original property in the test, you can even make sure that its contents are what you expect (since you control the API response).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! I understand. That does make sense.

I appreciate you taking the time to review this, share your thoughts, and explain the importance of each change. I apologize for any misunderstanding or oversight in my initial attempts.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries at all! Zero judgment on our side, we appreciate you taking the time to send the PR and the improvements; the least we could do is help guide you along in the journey as we try to keep the quality and testing level of the project high working together 😄


beforeEach(function () {
this.scope = nock('https://slack.com')
.post(/api/)
.replyWithError('Request failed!!')
})

it('the \'original\' property is attached when the option, attachOriginalToWebAPIRequestError is absent', async () => {
const client = new WebClient(token, {
retryConfig: { retries: 0 },
});

client.apiCall('conversations/list').catch((error) => {
expect(error).to.haveOwnProperty('original')
this.scope.done();
done();
});

});

it('the \'original\' property is attached when the option, attachOriginalToWebAPIRequestError is set to true', async () => {
const client = new WebClient(token, {
attachOriginalToWebAPIRequestError: true,
retryConfig: { retries: 0 },
});

client.apiCall('conversations/list').catch((error) => {
expect(error).to.haveOwnProperty('original')
this.scope.done();
done();
});
});

it('the \'original\' property is not attached when the option, attachOriginalToWebAPIRequestError is set to false', async () => {
const client = new WebClient(token, {
attachOriginalToWebAPIRequestError: false,
retryConfig: { retries: 0 },
});

client.apiCall('conversations/list').catch((error) => {
expect(error).not.to.haveOwnProperty('original')
this.scope.done();
done();
});
});
});

afterEach(function () {
nock.cleanAll();
});
Expand Down
17 changes: 16 additions & 1 deletion packages/web-api/src/WebClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,13 @@ export interface WebClientOptions {
rejectRateLimitedCalls?: boolean;
headers?: Record<string, string>;
teamId?: string;
/**
* Indicates whether to attach the original error to a Web API request error.
* When set to true, the original error object will be attached to the Web API request error.
* @type {boolean}
* @default true
*/
attachOriginalToWebAPIRequestError?: boolean,
}

export type TLSOptions = Pick<SecureContextOptions, 'pfx' | 'key' | 'passphrase' | 'cert' | 'ca'>;
Expand Down Expand Up @@ -167,6 +174,12 @@ export class WebClient extends Methods {
*/
private teamId?: string;

/**
* Configuration to opt-out of attaching the original error
* (obtained from the HTTP client) to WebAPIRequestError.
*/
private attachOriginalToWebAPIRequestError: boolean;

/**
* @param token - An API token to authenticate/authorize with Slack (usually start with `xoxp`, `xoxb`)
*/
Expand All @@ -182,6 +195,7 @@ export class WebClient extends Methods {
rejectRateLimitedCalls = false,
headers = {},
teamId = undefined,
attachOriginalToWebAPIRequestError = true,
filmaj marked this conversation as resolved.
Show resolved Hide resolved
}: WebClientOptions = {}) {
super();

Expand All @@ -195,6 +209,7 @@ export class WebClient extends Methods {
this.tlsConfig = tls !== undefined ? tls : {};
this.rejectRateLimitedCalls = rejectRateLimitedCalls;
this.teamId = teamId;
this.attachOriginalToWebAPIRequestError = attachOriginalToWebAPIRequestError;

// Logging
if (typeof logger !== 'undefined') {
Expand Down Expand Up @@ -613,7 +628,7 @@ export class WebClient extends Methods {
const e = error as any;
this.logger.warn('http request failed', e.message);
if (e.request) {
throw requestErrorWithOriginal(e);
throw requestErrorWithOriginal(e, this.attachOriginalToWebAPIRequestError);
}
throw error;
}
Expand Down
7 changes: 5 additions & 2 deletions packages/web-api/src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,16 @@ export function errorWithCode(error: Error, code: ErrorCode): CodedError {
/**
* A factory to create WebAPIRequestError objects
* @param original - original error
* @param attachOriginal - config indicating if 'original' property should be added on the error object
*/
export function requestErrorWithOriginal(original: Error): WebAPIRequestError {
export function requestErrorWithOriginal(original: Error, attachOriginal: boolean): WebAPIRequestError {
const error = errorWithCode(
new Error(`A request error occurred: ${original.message}`),
ErrorCode.RequestError,
) as Partial<WebAPIRequestError>;
error.original = original;
if (attachOriginal) {
error.original = original;
}
return (error as WebAPIRequestError);
}

Expand Down
Loading