Skip to content
This repository was archived by the owner on Dec 10, 2021. It is now read-only.

feat: add retry to callApi #384

Merged
etr2460 merged 1 commit into from
Apr 21, 2020
Merged

feat: add retry to callApi #384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions packages/superset-ui-connection/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
},
"dependencies": {
"@babel/runtime": "^7.1.2",
"fetch-retry": "^3.1.0",
"whatwg-fetch": "^3.0.0"
},
"publishConfig": {
Expand Down
7 changes: 7 additions & 0 deletions packages/superset-ui-connection/src/SupersetClientClass.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,21 @@ import {
Credentials,
CsrfPromise,
CsrfToken,
FetchRetryOptions,
Headers,
Host,
Mode,
Protocol,
RequestConfig,
SupersetClientResponse,
} from './types';
import { DEFAULT_FETCH_RETRY_OPTIONS } from './constants';

export default class SupersetClientClass {
credentials: Credentials;
csrfToken?: CsrfToken;
csrfPromise?: CsrfPromise;
fetchRetryOptions?: FetchRetryOptions;
protocol: Protocol;
host: Host;
headers: Headers;
Expand All @@ -27,6 +30,7 @@ export default class SupersetClientClass {
protocol = 'http:',
host = 'localhost',
headers = {},
fetchRetryOptions = {},
mode = 'same-origin',
timeout,
credentials = undefined,
Expand All @@ -40,6 +44,7 @@ export default class SupersetClientClass {
this.credentials = credentials;
this.csrfToken = csrfToken;
this.csrfPromise = undefined;
this.fetchRetryOptions = { ...DEFAULT_FETCH_RETRY_OPTIONS, ...fetchRetryOptions };

if (typeof this.csrfToken === 'string') {
this.headers = { ...this.headers, 'X-CSRFToken': this.csrfToken };
Expand Down Expand Up @@ -80,6 +85,7 @@ export default class SupersetClientClass {
body,
credentials,
endpoint,
fetchRetryOptions,
headers,
host,
method,
Expand All @@ -95,6 +101,7 @@ export default class SupersetClientClass {
callApi({
body,
credentials: credentials ?? this.credentials,
fetchRetryOptions,
headers: { ...this.headers, ...headers },
method,
mode: mode ?? this.mode,
Expand Down
8 changes: 6 additions & 2 deletions packages/superset-ui-connection/src/callApi/callApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import 'whatwg-fetch';
import fetchRetry from 'fetch-retry';
import { CallApi } from '../types';
import { CACHE_AVAILABLE, CACHE_KEY, HTTP_STATUS_NOT_MODIFIED, HTTP_STATUS_OK } from '../constants';

Expand All @@ -7,6 +8,7 @@ export default function callApi({
body,
cache = 'default',
credentials = 'same-origin',
fetchRetryOptions,
headers,
method = 'GET',
mode = 'same-origin',
Expand All @@ -16,6 +18,8 @@ export default function callApi({
stringify = true,
url,
Copy link
Copy Markdown
Contributor

@ktmud ktmud Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Can we add the retry options to callApi arguments, too, in case people want to override the defaults?

I'd also argue it's probably safer to make this an opt-in rather than opt-out. Can add an API to superset-ui/connection to change the defaults.

Copy link
Copy Markdown
Contributor Author

@etr2460 etr2460 Apr 21, 2020

Choose a reason for hiding this comment

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

I've added the retry options to the SupersetClient config. However, I think it's fair to make this opt out. I can add a comment in incubator-superset that this was added, but I think the common path should include QOL features like automatic retried on network issues

}: CallApi): Promise<Response> {
const fetchWithRetry = fetchRetry(fetch, fetchRetryOptions);

const request = {
body,
cache,
Expand Down Expand Up @@ -43,7 +47,7 @@ export default function callApi({
request.headers = { ...request.headers, 'If-None-Match': etag };
}

return fetch(url, request);
return fetchWithRetry(url, request);
})
.then(response => {
if (response.status === HTTP_STATUS_NOT_MODIFIED) {
Expand Down Expand Up @@ -82,5 +86,5 @@ export default function callApi({
request.body = formData;
}

return fetch(url, request);
return fetchWithRetry(url, request);
}
8 changes: 8 additions & 0 deletions packages/superset-ui-connection/src/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
import { FetchRetryOptions } from './types';

// HTTP status codes
export const HTTP_STATUS_OK = 200;
export const HTTP_STATUS_NOT_MODIFIED = 304;

// Namespace for Cache API
export const CACHE_AVAILABLE = 'caches' in self;
export const CACHE_KEY = '@SUPERSET-UI/CONNECTION';

export const DEFAULT_FETCH_RETRY_OPTIONS: FetchRetryOptions = {
retries: 3,
retryDelay: 1000,
retryOn: [503],
};
8 changes: 8 additions & 0 deletions packages/superset-ui-connection/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ export type Body = RequestInit['body'];
export type Cache = RequestInit['cache'];
export type Credentials = RequestInit['credentials'];
export type Endpoint = string;
export type FetchRetryOptions = {
retries?: number;
retryDelay?: number | ((attempt: number, error: Error, response: Response) => number);
retryOn?: number[] | ((attempt: number, error: Error, response: Response) => boolean);
};
export type Headers = { [k: string]: string };
export type Host = string;
export type Json = { [k: string]: any };
Expand All @@ -21,6 +26,7 @@ export interface CallApi {
body?: Body;
cache?: Cache;
credentials?: Credentials;
fetchRetryOptions?: FetchRetryOptions;
headers?: Headers;
method?: Method;
mode?: Mode;
Expand All @@ -34,6 +40,7 @@ export interface CallApi {
export interface RequestBase {
body?: Body;
credentials?: Credentials;
fetchRetryOptions?: FetchRetryOptions;
headers?: Headers;
host?: Host;
mode?: Mode;
Expand Down Expand Up @@ -70,6 +77,7 @@ export type Protocol = 'http:' | 'https:';
export interface ClientConfig {
credentials?: Credentials;
csrfToken?: CsrfToken;
fetchRetryOptions?: FetchRetryOptions;
headers?: Headers;
host?: Host;
protocol?: Protocol;
Expand Down
44 changes: 39 additions & 5 deletions packages/superset-ui-connection/test/callApi/callApi.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import * as constants from '../../src/constants';
import { LOGIN_GLOB } from '../fixtures/constants';
import throwIfCalled from '../utils/throwIfCalled';
import { CallApi } from '../../src/types';
import { DEFAULT_FETCH_RETRY_OPTIONS } from '../../src/constants';

describe('callApi()', () => {
beforeAll(() => {
Expand All @@ -20,6 +21,8 @@ describe('callApi()', () => {
const mockPatchUrl = '/mock/patch/url';
const mockCacheUrl = '/mock/cache/url';
const mockNotFound = '/mock/notfound';
const mockErrorUrl = '/mock/error/url';
const mock503 = '/mock/503';

const mockGetPayload = { get: 'payload' };
const mockPostPayload = { post: 'payload' };
Expand All @@ -30,13 +33,16 @@ describe('callApi()', () => {
body: 'BODY',
headers: { Etag: 'etag' },
};
const mockErrorPayload = { status: 500, statusText: 'Internal error' };

fetchMock.get(mockGetUrl, mockGetPayload);
fetchMock.post(mockPostUrl, mockPostPayload);
fetchMock.put(mockPutUrl, mockPutPayload);
fetchMock.patch(mockPatchUrl, mockPatchPayload);
fetchMock.get(mockCacheUrl, mockCachePayload);
fetchMock.get(mockNotFound, { status: 404 });
fetchMock.get(mock503, { status: 503 });
fetchMock.get(mockErrorUrl, () => Promise.reject(mockErrorPayload));

afterEach(fetchMock.reset);

Expand Down Expand Up @@ -437,19 +443,47 @@ describe('callApi()', () => {
});
});

it('rejects if the request throws', () => {
const mockErrorUrl = '/mock/error/url';
const mockErrorPayload = { status: 500, statusText: 'Internal error' };
fetchMock.get(mockErrorUrl, () => Promise.reject(mockErrorPayload));
it('rejects after retrying thrice if the request throws', () => {
Copy link
Copy Markdown
Contributor

@williaster williaster Apr 21, 2020

Choose a reason for hiding this comment

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

could update the tests to match the retry config variables specified if we expose them

Copy link
Copy Markdown
Contributor

@williaster williaster Apr 21, 2020

Choose a reason for hiding this comment

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

this is pretty awesome tho that the throwing is delayed

Copy link
Copy Markdown
Contributor Author

@etr2460 etr2460 Apr 21, 2020

Choose a reason for hiding this comment

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

updated

expect.assertions(3);

return callApi({
fetchRetryOptions: DEFAULT_FETCH_RETRY_OPTIONS,
url: mockErrorUrl,
method: 'GET',
})
.then(throwIfCalled)
.catch(error => {
expect(fetchMock.calls(mockErrorUrl)).toHaveLength(4);
expect(error.status).toBe(mockErrorPayload.status);
expect(error.statusText).toBe(mockErrorPayload.statusText);
});
});

it('rejects without retries if the config is set to 0 retries', () => {
expect.assertions(3);

return callApi({ url: mockErrorUrl, method: 'GET' })
return callApi({
fetchRetryOptions: { retries: 0 },
url: mockErrorUrl,
method: 'GET',
})
.then(throwIfCalled)
.catch(error => {
expect(fetchMock.calls(mockErrorUrl)).toHaveLength(1);
expect(error.status).toBe(mockErrorPayload.status);
expect(error.statusText).toBe(mockErrorPayload.statusText);
});
});

it('rejects after retrying thrice if the request returns a 503', async () => {
const url = mock503;
const response = await callApi({
fetchRetryOptions: DEFAULT_FETCH_RETRY_OPTIONS,
url,
method: 'GET',
});
const calls = fetchMock.calls(url);
expect(calls).toHaveLength(4);
expect(response.status).toEqual(503);
});
});
9 changes: 8 additions & 1 deletion yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8001,7 +8001,7 @@ es5-shim@^4.5.13:
resolved "https://registry.yarnpkg.com/es5-shim/-/es5-shim-4.5.14.tgz#90009e1019d0ea327447cb523deaff8fe45697ef"
integrity sha512-7SwlpL+2JpymWTt8sNLuC2zdhhc+wrfe5cMPI2j0o6WsPdfAiPwmFy2f0AocPB4RQVBOZ9kNTgi5YF7TdhkvEg==

es6-promise@^4.0.3:
es6-promise@^4.0.3, es6-promise@^4.2.8:
version "4.2.8"
resolved "https://registry.yarnpkg.com/es6-promise/-/es6-promise-4.2.8.tgz#4eb21594c972bc40553d276e510539143db53e0a"
integrity sha512-HJDGx5daxeIvxdBxvG2cb9g4tEvwIk3i8+nhX0yGrYmZUzbkdg8QbDevheDB8gd0//uPj4c1EQua8Q+MViT0/w==
Expand Down Expand Up @@ -8679,6 +8679,13 @@ fetch-mock@^7.2.5:
path-to-regexp "^2.2.1"
whatwg-url "^6.5.0"

fetch-retry@^3.1.0:
version "3.1.0"
resolved "https://registry.yarnpkg.com/fetch-retry/-/fetch-retry-3.1.0.tgz#53a25652adf56def526f9b6f6d749a5bebffd319"
integrity sha512-pHCYCq7g854KkebphR3tKb4M7TJK91ZI0K2BU82cWv+vNkFQn0PZZFrQd/mL+Ra/mj2HLZNvzkTRjPEq2Dh/Bg==
dependencies:
es6-promise "^4.2.8"

figgy-pudding@^3.4.1, figgy-pudding@^3.5.1:
version "3.5.2"
resolved "https://registry.yarnpkg.com/figgy-pudding/-/figgy-pudding-3.5.2.tgz#b4eee8148abb01dcf1d1ac34367d59e12fa61d6e"
Expand Down