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

fix(@aws-amplify/api): graphql API.cancel fix #9578

Merged
merged 15 commits into from
May 10, 2022
Merged

fix(@aws-amplify/api): graphql API.cancel fix #9578

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f223ab7
fix(@aws-amplify/api): graphql API.cancel fix
jamesaucode Feb 11, 2022
cbce31b
Merge branch 'main' into api/graphql-cancel-fix
jamesaucode Feb 11, 2022
a7d858f
fix(api): only return true if request is cancelled
jamesaucode Feb 11, 2022
1bfe7cd
update unit test
jamesaucode Feb 11, 2022
0c92965
check cancelToken exist before calling cancel
jamesaucode Feb 14, 2022
98c77cb
removed old comment
jamesaucode Feb 14, 2022
2538e4b
Merge branch 'main' into api/graphql-cancel-fix
jamesaucode Apr 27, 2022
a9b4197
test run CI
jamesaucode Apr 27, 2022
9c0d497
fix ci config
jamesaucode Apr 27, 2022
8dc91d8
chore: add geo to `.github/ISSUE_TEMPLATE` (#9847)
TreTuna Apr 28, 2022
52e1e83
chore: keep @babel/traverse on prior version to unblock build (#9859)
svidgen May 2, 2022
95dfe90
circleci config update
jamesaucode May 4, 2022
f7a5f21
Merge branch 'main' into api/graphql-cancel-fix
jamesaucode May 4, 2022
8b36664
add new integ tests to required
jamesaucode May 5, 2022
a6f62b7
Merge branch 'main' into api/graphql-cancel-fix
jamesaucode May 9, 2022
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
25 changes: 19 additions & 6 deletions packages/api-rest/__tests__/RestClient-unit-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,11 @@ describe('RestClient test', () => {
});

describe('Cancel Token', () => {
afterEach(() => {
jest.clearAllMocks();
jest.resetAllMocks();
});

const apiOptions = {
headers: {},
endpoints: [
Expand All @@ -474,16 +479,24 @@ describe('RestClient test', () => {

test('request non existent', () => {
const restClient = new RestClient(apiOptions);
// if the request doesn't exist we can still say it is canceled successfully
expect(
restClient.cancel(
new Promise<any>((req, res) => {})
)
).toBeTruthy();
expect(restClient.cancel(new Promise<any>((req, res) => {}))).toBe(false);
});

test('request exist', () => {
const restClient = new RestClient(apiOptions);
const request = Promise.resolve();
restClient.updateRequestToBeCancellable(
request,
restClient.getCancellableToken()
);
expect(restClient.cancel(request)).toBe(true);
});

test('happy case', () => {
const restClient = new RestClient(apiOptions);
jest
.spyOn(RestClient.prototype, 'ajax')
.mockImplementationOnce(() => Promise.resolve());

const cancellableToken = restClient.getCancellableToken();
const request = restClient.ajax('url', 'method', { cancellableToken });
Expand Down
9 changes: 4 additions & 5 deletions packages/api-rest/src/RestClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,9 @@ export class RestClient {
const source = this._cancelTokenMap.get(request);
if (source) {
source.cancel(message);
return true;
}
return true;
return false;
}

/**
Expand Down Expand Up @@ -366,10 +367,8 @@ export class RestClient {
/** private methods **/

private _signed(params, credentials, isAllResponse, { service, region }) {
const {
signerServiceInfo: signerServiceInfoParams,
...otherParams
} = params;
const { signerServiceInfo: signerServiceInfoParams, ...otherParams } =
params;

const endpoint_region: string =
region || this._region || this._options.region;
Expand Down
28 changes: 28 additions & 0 deletions packages/api/__tests__/API-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,32 @@ describe('API test', () => {
const api = new API(null);
expect(await api.graphql(null)).toBe('grapqhqlResponse');
});

describe('cancel', () => {
test('cancel RestAPI request', async () => {
jest
.spyOn(GraphQLAPIClass.prototype, 'cancel')
.mockImplementation(() => false);
const restAPICancelSpy = jest
.spyOn(RestAPIClass.prototype, 'cancel')
.mockImplementation(() => true);
const api = new API(null);
const request = Promise.resolve();
expect(api.cancel(request)).toBe(true);
expect(restAPICancelSpy).toHaveBeenCalled();
});

test('cancel GraphQLAPI request', async () => {
const graphQLAPICancelSpy = jest
.spyOn(GraphQLAPIClass.prototype, 'cancel')
.mockImplementation(() => true);
jest
.spyOn(RestAPIClass.prototype, 'cancel')
.mockImplementation(() => false);
const api = new API(null);
const request = Promise.resolve();
expect(api.cancel(request)).toBe(true);
expect(graphQLAPICancelSpy).toHaveBeenCalled();
});
});
});
15 changes: 10 additions & 5 deletions packages/api/src/API.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,12 +155,17 @@ export class APIClass {
return this._restApi.isCancel(error);
}
/**
* Cancels an inflight request
* @param {any} request - request to cancel
* @return {boolean} - A boolean indicating if the request was cancelled
* Cancels an inflight request for either a GraphQL request or a Rest API request.
* @param request - request to cancel
* @param [message] - custom error message
* @return If the request was cancelled
*/
cancel(request: Promise<any>, message?: string) {
return this._restApi.cancel(request, message);
cancel(request: Promise<any>, message?: string): boolean {
// it's safe to potentially call both method, as 'request' refers to a reference of a request, which is unique.
return (
this._restApi.cancel(request, message) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not as familiar with this part of the codebase, but wondering if it's possible to conditionally call one or the other, depending on what the API type is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we can check what the API type is because the parameter only takes the request (which can be any promise) and check it against the internal WeakMap. I did add a hasCancelToken method to check before we actually call the .cancel function.

this._graphqlApi.cancel(request, message)
);
}

/**
Expand Down