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

add retry for spdx data #1313

Merged
merged 5 commits into from
May 11, 2024
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
52 changes: 22 additions & 30 deletions frontend/src/pages/plugins/[name].tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ import { PluginPage } from '@/components/PluginPage';
import { DEFAULT_REPO_DATA } from '@/constants/plugin';
import { useLoadingState } from '@/context/loading';
import { PluginStateProvider } from '@/context/plugin';
import { SpdxLicenseData, SpdxLicenseResponse } from '@/store/search/types';
import { SpdxLicenseData } from '@/store/search/types';
import { PluginData } from '@/types';
import { createUrl, fetchRepoData, FetchRepoDataResult, Logger } from '@/utils';
import { getErrorMessage } from '@/utils/error';
import { hubAPI } from '@/utils/HubAPIClient';
import { spdxLicenseDataAPI } from '@/utils/spdx';
import { getSpdxProps } from '@/utils/spdx';
import { getServerSidePropsHandler } from '@/utils/ssr';

/**
Expand All @@ -41,57 +41,49 @@ export const getServerSideProps = getServerSidePropsHandler<Props, Params>({
*/
async getProps({ params }) {
const name = String(params?.name);
const props: Props = {
repo: DEFAULT_REPO_DATA,
};

let codeRepo = '';
let plugin: PluginData | undefined;

try {
const plugin = await hubAPI.getPlugin(name);
plugin = await hubAPI.getPlugin(name);
codeRepo = plugin.code_repository;
props.plugin = plugin;
} catch (err) {
props.error = getErrorMessage(err);
const error = getErrorMessage(err);
logger.error({
message: 'Failed to fetch plugin data',
plugin: name,
error: props.error,
error,
});

return { props };
return {
props: { error },
};
}

const repoData = await fetchRepoData(codeRepo);
Object.assign(props, repoData);

if (props.repoFetchError) {
const logType = inRange(props.repoFetchError.status, 400, 500)
if (repoData.repoFetchError) {
const logType = inRange(repoData.repoFetchError.status, 400, 500)
? 'info'
: 'error';

logger[logType]({
message: 'Failed to fetch repo data',
plugin: name,
error: props.error,
error: repoData.repoFetchError,
});
}

try {
const {
data: { licenses },
} = await spdxLicenseDataAPI.get<SpdxLicenseResponse>('');
props.licenses = licenses;
} catch (err) {
props.error = getErrorMessage(err);
const licenses = await getSpdxProps(logger);

logger.error({
message: 'Failed to fetch spdx license data',
error: props.error,
});
}

return { props };
return {
props: {
plugin,
licenses,
repo: DEFAULT_REPO_DATA,
...repoData,
},
};
},
});

Expand Down Expand Up @@ -157,7 +149,7 @@ export default function Plugin({
<PluginStateProvider
licenses={licenses}
plugin={plugin}
repo={repo}
repo={repo ?? DEFAULT_REPO_DATA}
repoFetchError={repoFetchError}
>
<PluginPage />
Expand Down
39 changes: 16 additions & 23 deletions frontend/src/pages/plugins/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ import { ErrorMessage } from '@/components/ErrorMessage';
import { NotFoundPage } from '@/components/NotFoundPage';
import { SearchPage } from '@/components/SearchPage';
import { SearchStoreProvider } from '@/store/search/context';
import { SpdxLicenseData, SpdxLicenseResponse } from '@/store/search/types';
import { SpdxLicenseData } from '@/store/search/types';
import { PluginIndexData } from '@/types';
import { Logger } from '@/utils';
import { getErrorMessage } from '@/utils/error';
import { hubAPI } from '@/utils/HubAPIClient';
import { spdxLicenseDataAPI } from '@/utils/spdx';
import { getSpdxProps as getSpdxLicenses } from '@/utils/spdx';
import { getServerSidePropsHandler } from '@/utils/ssr';

interface Props {
Expand All @@ -24,37 +24,30 @@ const logger = new Logger('pages/plugins/index.tsx');

export const getServerSideProps = getServerSidePropsHandler<Props>({
async getProps() {
const props: Props = {
status: 200,
};
let index: PluginIndexData[];

try {
const index = await hubAPI.getPluginIndex();
props.index = index;
index = await hubAPI.getPluginIndex();
} catch (err) {
props.error = getErrorMessage(err);
const error = getErrorMessage(err);

logger.error({
message: 'Failed to plugin index',
error: props.error,
error,
});
}

try {
const {
data: { licenses },
} = await spdxLicenseDataAPI.get<SpdxLicenseResponse>('');
props.licenses = licenses;
} catch (err) {
props.error = getErrorMessage(err);

logger.error({
message: 'Failed to fetch spdx license data',
error: props.error,
});
return { props: { error } };
}

return { props };
const licenses = await getSpdxLicenses(logger);

return {
props: {
index,
licenses,
status: 200,
},
};
},
});

Expand Down
91 changes: 17 additions & 74 deletions frontend/src/utils/HubAPIClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ import {
PluginSectionType,
} from '@/types';

import { retryAxios } from './async';
import { Logger } from './logger';
import { sleep } from './sleep';
import { getFullPathFromAxios } from './url';
import {
validateMetricsData,
Expand Down Expand Up @@ -72,21 +72,6 @@ function isHubAPIErrorResponse(
return !!(data as HubAPIErrorResponse).errorType;
}

/**
* Max number of times to retry fetching a request.
*/
const MAX_RETRIES = 3;

/**
* Backoff factory for increasing the delay when re-fetching requests.
*/
const RETRY_BACKOFF_FACTOR = 2;

/**
* Initial delay before retrying a request in milliseconds.
*/
const INITIAL_RETRY_DELAY_MS = 1000;

/**
* Class for interacting with the hub API. Each function makes a request to the
* hub API and runs client-side data validation on the data to ensure
Expand All @@ -103,66 +88,24 @@ export class HubAPIClient {
});

private async sendRequest<T>(url: string, config?: AxiosRequestConfig<T>) {
const method = config?.method ?? 'GET';
const path = getFullPathFromAxios(url, config);
let retryDelay = INITIAL_RETRY_DELAY_MS;

for (let retry = 0; retry < MAX_RETRIES; retry += 1) {
try {
const { data, status } = await this.api.request<T>({
url,
params: {
...config?.params,
retryCount: retry,
} as Record<string, unknown>,
...config,
});

if (SERVER) {
logger.info({
path,
method,
url,
status,
});
}

return data;
} catch (err) {
const isRetrying = retry < MAX_RETRIES - 1;

if (axios.isAxiosError(err)) {
const status = err.response?.status;
const level =
isRetrying ||
(status !== undefined && status >= 400 && status < 500)
? 'warn'
: 'error';

logger[level]({
message: 'Error sending request',
error: err.message,
method,
path,
url,
isRetrying,
retry,
...(err.response?.status ? { status: err.response.status } : {}),
});
}

if (isRetrying) {
await sleep(retryDelay);
retryDelay *= RETRY_BACKOFF_FACTOR;
} else {
throw err;
}
}
const { data, status } = await retryAxios<T>({
instance: this.api,
url,
...config,
});

if (SERVER) {
const method = config?.method ?? 'GET';
const path = getFullPathFromAxios(url, config);
logger.info({
path,
method,
url,
status,
});
}

// This edge case should never happen but is required by TypeScript to
// prevent a compile error.
throw new Error('failed to request data');
return data;
}

async getPluginIndex(): Promise<PluginIndexData[]> {
Expand Down
82 changes: 82 additions & 0 deletions frontend/src/utils/async.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import axios from 'axios';

import { retryAsync, retryAxios } from './async';

describe('retryAsync', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it('should retry promise fails', async () => {
const expectedRetryCount = 2;
let actualRetryCount = 0;

async function execute() {
if (actualRetryCount === expectedRetryCount) {
return Promise.resolve();
}

actualRetryCount += 1;
throw new Error('failure');
}

await retryAsync({ execute });
expect(actualRetryCount).toBe(expectedRetryCount);
});

it('should fail when promise fails too many times', async () => {
const expectedRetryCount = 3;
let actualRetryCount = 0;

// eslint-disable-next-line @typescript-eslint/require-await
async function execute() {
actualRetryCount += 1;
throw new Error('failure');
}

await expect(
retryAsync({ execute, retries: expectedRetryCount }),
).rejects.toThrow('failure');
expect(actualRetryCount).toBe(expectedRetryCount);
});
});

describe('retryAxios', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it('should retry fetching a failed request', async () => {
const expectedRetryCount = 2;
let actualRetryCount = 0;
jest.spyOn(axios, 'request').mockImplementation(() => {
if (actualRetryCount === expectedRetryCount) {
return Promise.resolve({
data: 'data',
status: 200,
});
}

actualRetryCount += 1;
throw new Error('failure');
});

const res = await retryAxios({ url: '/test' });
expect(res.data).toBe('data');
expect(actualRetryCount).toBe(expectedRetryCount);
});

it('should fail when the request fails too many times', async () => {
Copy link

Choose a reason for hiding this comment

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

love this test! I was actually checking to see if it throws on error lol

const expectedRetryCount = 3;
let actualRetryCount = 0;
jest.spyOn(axios, 'request').mockImplementation(() => {
actualRetryCount += 1;
throw new Error('failure');
});

await expect(
retryAxios({ url: '/test', retries: expectedRetryCount }),
).rejects.toThrow('failure');
expect(actualRetryCount).toBe(expectedRetryCount);
});
});
Loading
Loading