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

Jump to bottom

fix: UnhandledPromiseRejection errors #782

Merged
merged 4 commits into from
May 21, 2021
Merged

fix: UnhandledPromiseRejection errors #782

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
38362b3
fix: UnhandledPromiseRejection errors
fboucquez May 19, 2021
5b62638
fixed lint
fboucquez May 19, 2021
b242a7e
Feedback fixes
fboucquez May 21, 2021
db14d91
Merge branch 'dev' into unhandled_exception_fix
rg911 May 21, 2021
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
6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@
"scripts": {
"pretest": "npm run build",
"prepack": "npm run build",
"test": "mocha --ui bdd --recursive ./dist/test --timeout 90000",
"test": "mocha --ui bdd --recursive ./dist/test --timeout 90000 --unhandled-rejections=strict ",
"e2econfigcopy": "gulp",
"test:e2e": "npm run build && mocha --ui bdd --recursive ./dist/e2e --timeout 90000",
"test:e2e": "npm run build && mocha --ui bdd --recursive ./dist/e2e --timeout 90000 --unhandled-rejections=strict",
"e2e": "npm run bootstrap-stop && npm run bootstrap-start-detached && npm run test:e2e && npm run bootstrap-stop",
"test:all": "mocha --ui bdd --recursive ./dist/ --timeout 90000",
"test:all": "mocha --ui bdd --recursive ./dist/ --timeout 90000 --unhandled-rejections=strict",
"build": "shx rm -rf dist/ && tsc && npm run e2econfigcopy",
"test:cov": "nyc --reporter=lcov --reporter=text-summary npm t",
"test:coveralls": "npm run test:cov | coveralls",
Expand Down
65 changes: 28 additions & 37 deletions src/infrastructure/Http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,9 @@
*/

import fetch from 'node-fetch';
import { from as observableFrom, Observable, of as observableOf, of, throwError } from 'rxjs';
import { catchError, flatMap, map } from 'rxjs/operators';
import { defer, from as observableFrom, Observable, of as observableOf } from 'rxjs';
import { Configuration, NodeRoutesApi, Pagination, querystring } from 'symbol-openapi-typescript-fetch-client';
import { NetworkType } from '../model/network/NetworkType';
import { NetworkType } from '../model/network';
import { Page } from './Page';
import { RepositoryCallError } from './RepositoryCallError';

Expand All @@ -33,9 +32,9 @@ export abstract class Http {
*/
protected constructor(protected readonly url: string, protected readonly fetchApi?: any) {}

public static errorHandling(error: any): Observable<never> {
public static async errorHandling(error: any): Promise<Error> {
if (error instanceof Error) {
return throwError(error);
return error;
}
const statusCode: number = parseInt(error?.status || error?.statusCode || error?.response?.statusCode || 0);
const statusMessage: string = (
Expand All @@ -55,29 +54,23 @@ export abstract class Http {
return JSON.stringify(body);
};

const getBody = (error: any): Observable<string> => {
const getBody = async (error: any): Promise<string> => {
const body = error?.response?.body;
if (body) {
return of(toString(body));
return toString(body);
}
if (error.text) {
return observableFrom(error.text()).pipe(
map(toString),
catchError(() => of('')),
);
return toString(await error.text());
}
return of('');
return '';
};
return getBody(error).pipe(
flatMap((body: string) => {
const formattedError: RepositoryCallError = {
statusCode,
statusMessage,
body,
};
return throwError(new Error(JSON.stringify(formattedError)));
}),
);

const formattedError: RepositoryCallError = {
statusCode,
statusMessage,
body: await getBody(error),
};
return new Error(JSON.stringify(formattedError));
}

createNetworkTypeObservable(networkType?: NetworkType | Observable<NetworkType>): Observable<NetworkType> {
Expand All @@ -86,7 +79,7 @@ export abstract class Http {
} else if (networkType) {
return observableOf(networkType as NetworkType);
} else {
return this.call(new NodeRoutesApi(this.config()).getNodeInfo(), (body) => body.networkIdentifier);
return defer(() => this.call(new NodeRoutesApi(this.config()).getNodeInfo(), (body) => body.networkIdentifier));
}
}

Expand All @@ -102,20 +95,18 @@ export abstract class Http {
*/
protected call<D, M>(remoteCall: Promise<D>, mapper: (value: D) => M): Observable<M> {
return observableFrom(
remoteCall.catch((e) => {
if (e instanceof Error) {
return Promise.resolve(e);
}
return Promise.reject(e);
}),
).pipe(
map((body) => {
if (body instanceof Error) {
throw body;
}
return mapper(body);
}),
catchError(Http.errorHandling),
new Promise<M>((resolve, reject) =>
remoteCall.then(
async (a) => {
try {
resolve(mapper(a));
} catch (e) {
reject(await Http.errorHandling(e));
}
},
async (e) => reject(await Http.errorHandling(e)),
),
),
);
}

Expand Down
6 changes: 3 additions & 3 deletions src/infrastructure/RepositoryFactoryHttp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,9 @@ export class RepositoryFactoryHttp implements RepositoryFactory {
this.generationHash = observableOf(configs.generationHash);
this.nodePublicKey = observableOf(configs.nodePublicKey);
} else {
const nodeInfoObservable = this.createNodeRepository().getNodeInfo();
this.generationHash = this.cache(() => nodeInfoObservable.pipe(map((b) => b.networkGenerationHashSeed)));
this.nodePublicKey = this.cache(() => nodeInfoObservable.pipe(map((b) => b.nodePublicKey)));
const nodeInfoObservable = this.cache(() => this.createNodeRepository().getNodeInfo());
this.generationHash = defer(() => nodeInfoObservable.pipe(map((b) => b.networkGenerationHashSeed)));
this.nodePublicKey = defer(() => nodeInfoObservable.pipe(map((b) => b.nodePublicKey)));
}
this.websocketUrl = configs?.websocketUrl ? configs?.websocketUrl : `${url.replace(/\/$/, '')}/ws`;
this.websocketInjected = configs?.websocketInjected;
Expand Down
65 changes: 39 additions & 26 deletions test/infrastructure/RepositoryFactory.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,32 +18,34 @@ import { of as observableOf, of } from 'rxjs';
import { catchError, map } from 'rxjs/operators';
import { NetworkConfigurationDTO } from 'symbol-openapi-typescript-fetch-client';
import { instance, mock, when } from 'ts-mockito';
import { AccountHttp } from '../../src/infrastructure/AccountHttp';
import { BlockHttp } from '../../src/infrastructure/BlockHttp';
import { ChainHttp } from '../../src/infrastructure/ChainHttp';
import { FinalizationHttp } from '../../src/infrastructure/FinalizationHttp';
import { HashLockHttp } from '../../src/infrastructure/HashLockHttp';
import { Listener } from '../../src/infrastructure/Listener';
import { MetadataHttp } from '../../src/infrastructure/MetadataHttp';
import { MosaicHttp } from '../../src/infrastructure/MosaicHttp';
import { MultisigHttp } from '../../src/infrastructure/MultisigHttp';
import { NamespaceHttp } from '../../src/infrastructure/NamespaceHttp';
import { NamespaceRepository } from '../../src/infrastructure/NamespaceRepository';
import { NetworkHttp } from '../../src/infrastructure/NetworkHttp';
import { NetworkRepository } from '../../src/infrastructure/NetworkRepository';
import { NodeHttp } from '../../src/infrastructure/NodeHttp';
import { NodeRepository } from '../../src/infrastructure/NodeRepository';
import { ReceiptHttp } from '../../src/infrastructure/ReceiptHttp';
import { RepositoryFactoryHttp } from '../../src/infrastructure/RepositoryFactoryHttp';
import { RestrictionAccountHttp } from '../../src/infrastructure/RestrictionAccountHttp';
import { RestrictionMosaicHttp } from '../../src/infrastructure/RestrictionMosaicHttp';
import { SecretLockHttp } from '../../src/infrastructure/SecretLockHttp';
import { TransactionGroup } from '../../src/infrastructure/TransactionGroup';
import { TransactionHttp } from '../../src/infrastructure/TransactionHttp';
import { TransactionStatusHttp } from '../../src/infrastructure/TransactionStatusHttp';
import { NetworkCurrencies } from '../../src/model/mosaic/NetworkCurrencies';
import { NetworkType } from '../../src/model/network/NetworkType';
import { NodeInfo } from '../../src/model/node/NodeInfo';
import {
AccountHttp,
BlockHttp,
ChainHttp,
FinalizationHttp,
HashLockHttp,
Listener,
MetadataHttp,
MosaicHttp,
MultisigHttp,
NamespaceHttp,
NamespaceRepository,
NetworkHttp,
NetworkRepository,
NodeHttp,
NodeRepository,
ReceiptHttp,
RepositoryFactoryHttp,
RestrictionAccountHttp,
RestrictionMosaicHttp,
SecretLockHttp,
TransactionGroup,
TransactionHttp,
TransactionStatusHttp,
} from '../../src/infrastructure';
import { NetworkCurrencies } from '../../src/model/mosaic';
import { NetworkType } from '../../src/model/network';
import { NodeInfo } from '../../src/model/node';

describe('RepositoryFactory', () => {
it('Should create repositories', () => {
Expand All @@ -70,6 +72,17 @@ describe('RepositoryFactory', () => {
expect(repositoryFactory.createFinalizationRepository()).to.be.not.null;
});

it('Raise error without unhandled-rejections', async () => {
const url = 'https://www.google.com';
Copy link
Contributor

Choose a reason for hiding this comment

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

Any other way to achieve this without relying on a remote call? I think we can use sinon to stub the getNodeHealthmethod to throw an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TS SDK uses ts-mockito. I've mocked the low-level generated NodeRoutesApi reflecting how it rejects responses with the current mock lib. The (nodeRepository as any).nodeRoutesApi doesn't look nice, but well.

const repositoryFactory = new RepositoryFactoryHttp(url);
try {
await repositoryFactory.createNodeRepository().getNodeHealth().toPromise();
expect(true).to.be.false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use chai-as-promised or a similar promise assertion library as a better practice in general for promise assertions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! chai as promises looks nice. Although the try-catch could be more end-user-style assertion? And I wouldn't add another lib just yet. Let's consider it for the future.

} catch (e) {
expect(e.message).contains('{"statusCode":404,"statusMessage":"Not Found","body":"<!DOCTYPE html>');
}
});

it('Should get GenerationHash from cache', (done) => {
let counter = 0;
const repositoryMock: NodeRepository = mock();
Expand Down