Skip to content
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
29 changes: 23 additions & 6 deletions packages/superagent-wrapper/src/request.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as h from '@api-ts/io-ts-http';
import * as E from 'fp-ts/Either';
import * as t from 'io-ts';
import * as PathReporter from 'io-ts/lib/PathReporter';
import { URL } from 'url';
import { pipe } from 'fp-ts/function';

Expand All @@ -13,15 +14,24 @@ type SuccessfulResponses<Route extends h.HttpRoute> = {
};
}[keyof Route['response']];

type DecodedResponse<Route extends h.HttpRoute> =
export type DecodedResponse<Route extends h.HttpRoute> =
| SuccessfulResponses<Route>
| {
status: 'decodeError';
error: unknown;
error: string;
body: unknown;
original: Response;
};

export class DecodeError extends Error {
readonly decodedResponse: DecodedResponse<h.HttpRoute>;

constructor(message: string, decodedResponse: DecodedResponse<h.HttpRoute>) {
super(message);
this.decodedResponse = decodedResponse;
}
}

const decodedResponse = <Route extends h.HttpRoute>(res: DecodedResponse<Route>) => res;

type ExpectedDecodedResponse<
Expand Down Expand Up @@ -134,7 +144,7 @@ const patchRequest = <
// DISCUSS: what's this non-standard HTTP status code?
decodedResponse<Route>({
status: 'decodeError',
error,
error: PathReporter.failure(error).join('\n'),
body: res.body,
original: res,
}),
Expand All @@ -146,9 +156,16 @@ const patchRequest = <
status: StatusCode,
) =>
patchedReq.decode().then((res) => {
if (res.status !== status) {
const error = res.error ?? `Unexpected status code ${String(res.status)}`;
throw new Error(JSON.stringify(error));
if (res.original.status !== status) {
const error = `Unexpected response ${String(
res.original.status,
)}: ${JSON.stringify(res.original.body)}`;
Comment on lines +160 to +162
Copy link
Contributor

Choose a reason for hiding this comment

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

Is res.original.status not already coerced to a string by the backticks?

Suggested change
const error = `Unexpected response ${String(
res.original.status,
)}: ${JSON.stringify(res.original.body)}`;
const error = `Unexpected response ${res.original.status}: ${JSON.stringify(res.original.body)}`;

Copy link
Contributor

Choose a reason for hiding this comment

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

throw new DecodeError(error, res as DecodedResponse<h.HttpRoute>);
} else if (res.status === 'decodeError') {
const error = `Could not decode response ${String(
res.original.status,
)}: ${JSON.stringify(res.original.body)}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that JSON.stringify can also throw 😭 I believe when a circular reference is encountered.

fp-ts provides a wrapper that returns an Either, which we may want to use

Copy link
Contributor

Choose a reason for hiding this comment

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

hurm how would you want to handle this case, @bitgopatmcl? It's pretty gnarly

I was about to open a ticket to track and then I had the thought -- is it even possible for res.original.body to contain a circular reference here? Meaning, was res.original.body recently JSON.parsed? If so, then I suppose there's no cause for concern (though I'll leave a note indicating that we've considered the unsafe use of JSON.stringify and determined that we're in the clear)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about if the server returns a non-application/json response? We should probably consider that case too.

throw new DecodeError(error, res as DecodedResponse<h.HttpRoute>);
} else {
return res as ExpectedDecodedResponse<Route, StatusCode>;
}
Expand Down
46 changes: 34 additions & 12 deletions packages/superagent-wrapper/test/request.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@ import superagent from 'superagent';
import supertest from 'supertest';
import { URL } from 'url';

import { superagentRequestFactory, supertestRequestFactory } from '../src/request';
import {
DecodeError,
superagentRequestFactory,
supertestRequestFactory,
} from '../src/request';
import { buildApiClient } from '../src/routes';

const PostTestRoute = h.httpRoute({
Expand All @@ -33,6 +37,9 @@ const PostTestRoute = h.httpRoute({
bar: t.number,
baz: t.boolean,
}),
401: t.type({
message: t.string,
}),
},
});

Expand Down Expand Up @@ -75,11 +82,16 @@ testApp.post('/test/:id', (req, res) => {
res.send({
invalid: 'response',
});
} else if (req.headers['x-send-unexpected-status-code']) {
} else if (req.headers['x-send-unknown-status-code']) {
res.status(400);
res.send({
error: 'bad request',
});
} else if (req.headers['x-send-unexpected-status-code']) {
res.status(401);
res.send({
message: 'unauthorized',
});
} else {
const response = PostTestRoute.response[200].encode({
...params,
Expand Down Expand Up @@ -129,10 +141,10 @@ describe('request', () => {
});
});

it('gracefully handles unexpected status codes', async () => {
it('gracefully handles unknown status codes', async () => {
const response = await apiClient['api.v1.test']
.post({ id: 1337, foo: 'test', bar: 42 })
.set('x-send-unexpected-status-code', 'true')
.set('x-send-unknown-status-code', 'true')
.decode();

assert.equal(response.status, 'decodeError');
Expand Down Expand Up @@ -169,21 +181,32 @@ describe('request', () => {
.post({ id: 1337, foo: 'test', bar: 42 })
.set('x-send-unexpected-status-code', 'true')
.decodeExpecting(200)
.then(() => false)
.catch(() => true);
.then(() => '')
.catch((err) => (err instanceof DecodeError ? err.message : ''));

assert.deepEqual(result, 'Unexpected response 401: {"message":"unauthorized"}');
});

it('throws for unknown responses', async () => {
const result = await apiClient['api.v1.test']
.post({ id: 1337, foo: 'test', bar: 42 })
.set('x-send-unknown-status-code', 'true')
.decodeExpecting(200)
.then(() => '')
.catch((err) => (err instanceof DecodeError ? err.message : ''));

assert.isTrue(result);
assert.deepEqual(result, 'Unexpected response 400: {"error":"bad request"}');
});

it('throws for decode errors', async () => {
const result = await apiClient['api.v1.test']
.post({ id: 1337, foo: 'test', bar: 42 })
.set('x-send-invalid-response-body', 'true')
.decodeExpecting(200)
.then(() => false)
.catch(() => true);
.then(() => '')
.catch((err) => (err instanceof DecodeError ? err.message : ''));

assert.isTrue(result);
assert.deepEqual(result, 'Could not decode response 200: {"invalid":"response"}');
Copy link
Contributor

Choose a reason for hiding this comment

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

deepEqual an odd choice here!

Copy link
Contributor

Choose a reason for hiding this comment

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

});
});

Expand All @@ -210,8 +233,7 @@ describe('request', () => {
.set('x-send-unexpected-status-code', 'true')
.decode();

assert.equal(req.status, 'decodeError');
assert.equal(req.original.status, 400);
assert.equal(req.status, 401);
});
});
});