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.

Sign up for GitHub

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

Impl. size-dependent request body compression #4283

Merged
merged 1 commit into from
Jun 7, 2022
Merged

Impl. size-dependent request body compression #4283

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
50 changes: 0 additions & 50 deletions src/config/config.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,54 +29,4 @@ describe('ServerConfigHelpers', () => {
});
});
});

describe('pairMatchesPath', () => {
it('should match exact match', () => {
const pair: UrlParamPair = { url: '/foo/', params: { a: '1' } };
const ans = pairMatchesPath(pair, '/foo/bar/baz', { a: '1' });

assert.isTrue(ans);
});

it('should match exact match with empty params', () => {
const pair: UrlParamPair = { url: '/foo/', params: {} };
const ans = pairMatchesPath(pair, '/foo/bar/baz', {});

assert.isTrue(ans);
});

it('should match exact match with extra params', () => {
const pair: UrlParamPair = { url: '/foo/', params: { a: '1' } };
const ans = pairMatchesPath(pair, '/foo/bar/baz', {
a: '1',
b: '1',
});

assert.isTrue(ans);
});

it('should not match exact when missing all params', () => {
const pair: UrlParamPair = { url: '/foo/', params: { a: '1' } };
const ans = pairMatchesPath(pair, '/foo/bar/baz', {});

assert.isFalse(ans);
});

it('should not match exact when missing some params', () => {
const pair: UrlParamPair = {
url: '/foo/',
params: { a: '1', b: '1' },
};
const ans = pairMatchesPath(pair, '/foo/bar/baz', { a: '1' });

assert.isFalse(ans);
});

it('should not match when param has different val', () => {
const pair: UrlParamPair = { url: '/foo/', params: { a: '1' } };
const ans = pairMatchesPath(pair, '/foo/bar/baz', { a: '2' });

assert.isFalse(ans);
});
});
});
58 changes: 17 additions & 41 deletions src/config/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ import pako from 'pako';

const win = window as any;

const REQ_BODY_SIZE_CHAR_LIMIT = 10000;

// these should not be exported. they should only be accessed
// via getServerConfig and getLoadConfig
const config: any = { serverConfig: {} };
Expand Down Expand Up @@ -202,49 +204,22 @@ export function initializeAPIClients() {
}

if (getServerConfig().enable_request_body_gzip_compression) {
compressRequestBodies(
CBioPortalAPI,
[
{ url: '/mutations/fetch', params: {} },
{ url: '/structural-variant/fetch', params: {} },
{ url: '/clinical-attributes/counts/fetch', params: {} },
{ url: '/patients/fetch', params: {} },
{ url: '/molecular-data/fetch', params: {} },
{
url: '/clinical-data/fetch',
params: { clinicalDataType: 'SAMPLE' },
},
{ url: '/gene-panel-data/fetch', params: {} },
{
url: '/clinical-data/fetch',
params: { clinicalDataType: 'PATIENT' },
},
],
registerRequestBodyCompression(CBioPortalAPI, getCbioPortalApiUrl());
registerRequestBodyCompression(
CBioPortalAPIInternal,
getCbioPortalApiUrl()
);
}
}

/**
* Compresses the request bodies of POST calls of urls that start with
* domain + urlToCompress, and adds the Content-Encoding header. To do this,
* Compresses the request bodies of POST calls of urls with large
* request body size, and adds the Content-Encoding header. To do this,
* it wraps the api client's request function.
* @param apiClient
* @param urlsToCompress
* @param domain
*/
function compressRequestBodies(
apiClient: any,
urlsToCompress: UrlParamPair[],
domain: string
): void {
urlsToCompress = urlsToCompress.map(pair => {
return {
url: domain + pair.url,
params: pair.params,
};
});

function registerRequestBodyCompression(apiClient: any, domain: string): void {
const oldRequestFunc = apiClient.prototype.request;

const newRequestFunc = (
Expand All @@ -258,14 +233,15 @@ function compressRequestBodies(
resolve: any,
errorHandlers: any[]
) => {
if (
method === 'POST' &&
urlsToCompress.filter(pair =>
pairMatchesPath(pair, url, queryParameters)
).length > 0
) {
headers['Content-Encoding'] = 'gzip';
body = pako.gzip(JSON.stringify(body)).buffer;
if (method === 'POST') {
var bodyString = JSON.stringify(body);
if (bodyString.length > REQ_BODY_SIZE_CHAR_LIMIT) {
Copy link
Member

Choose a reason for hiding this comment

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

How expensive would it be to do new Blob([bodyString]).size? Using char count to estimate size is a bit unreliable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Luke-Sikina it's just a heuristic right? and why is it innacurate?

Copy link
Contributor Author

@pvannierop pvannierop Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@Luke-Sikina I also reasoned that a precise estimate is not needed here. We just need the least resource-intensive estimate of request size.

headers['Content-Encoding'] = 'gzip';
body = pako.gzip(bodyString).buffer;
} else {
// Store stringified body, so that stringify only runs once.
body = bodyString;
}
}

oldRequestFunc(
Expand Down