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

Return 'null' on status 404 #57

Merged
merged 4 commits into from
Jan 31, 2018
Merged

Return 'null' on status 404 #57

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4fa0f3a
Add a null type to IRestResponse.result since it might be null
Jan 16, 2018
72df730
Improve initialization of default result object
Jan 16, 2018
308cec6
Adjust rest tests
Jan 16, 2018
77fac00
Rename variable in RestClient._processResponse()
Jan 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 7 additions & 4 deletions lib/RestClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import util = require("./Util");

export interface IRestResponse<T> {
statusCode: number,
result: T
result: T | null
}

export interface IRequestOptions {
Expand Down Expand Up @@ -174,9 +174,12 @@ export class RestClient {

private async _processResponse<T>(res: httpm.HttpClientResponse, options: IRequestOptions): Promise<IRestResponse<T>> {
return new Promise<IRestResponse<T>>(async (resolve, reject) => {
let rres: IRestResponse<T> = <IRestResponse<T>>{};
let statusCode: number = res.message.statusCode;
rres.statusCode = statusCode;
const statusCode: number = res.message.statusCode;

const rres: IRestResponse<T> = {
Copy link
Member

Choose a reason for hiding this comment

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

Can we rename this to response or something more readable?

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 retained the original variable name, but yes, this should be renamed.

statusCode: statusCode,
result: null,
};

// not found leads to null obj returned
if (statusCode == httpm.HttpCodes.NotFound) {
Expand Down
92 changes: 46 additions & 46 deletions test/resttests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ describe('Rest Tests', function () {

it('constructs', () => {
this.timeout(1000);

let rest: restm.RestClient = new restm.RestClient('typed-test-client-tests');
assert(rest, 'rest client should not be null');
})
Expand All @@ -38,75 +38,75 @@ describe('Rest Tests', function () {

let restRes: restm.IRestResponse<HttpBinData> = await _rest.get<HttpBinData>('https://httpbin.org/get');
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/get');
assert(restRes.result && restRes.result.url === 'https://httpbin.org/get');
Copy link
Member

Choose a reason for hiding this comment

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

For all of these, I am not sure it adds value to have the restRes.result check before doing an equality check. It fails either way and since this is just a test I don't think we need it. Makes it harder to read too.

Copy link
Member

Choose a reason for hiding this comment

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

Same comment for all places this has been added.

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 added these checks because I usually compile code with "strict" type checking enabled. If this option is disabled, then it would produces no error message, but in my eye, it is actually semantically wrong.

});

it('gets a resource with baseUrl', async() => {
let restRes: restm.IRestResponse<HttpBinData> = await _restBin.get<HttpBinData>('get');
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/get');
assert(restRes.result && restRes.result.url === 'https://httpbin.org/get');
});

it('creates a resource', async() => {
let res: any = { name: 'foo' };
let restRes: restm.IRestResponse<HttpBinData> = await _rest.create<HttpBinData>('https://httpbin.org/post', res);
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/post');
assert(restRes.result.json.name === 'foo');
assert(restRes.result && restRes.result.url === 'https://httpbin.org/post');
assert(restRes.result && restRes.result.json.name === 'foo');
});

it('creates a resource with a baseUrl', async() => {
let res: any = { name: 'foo' };
let restRes: restm.IRestResponse<HttpBinData> = await _restBin.create<HttpBinData>('post', res);
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/post');
assert(restRes.result.json.name === 'foo');
});
assert(restRes.result && restRes.result.url === 'https://httpbin.org/post');
assert(restRes.result && restRes.result.json.name === 'foo');
});

it('replaces a resource', async() => {
let res: any = { name: 'foo' };
let restRes: restm.IRestResponse<HttpBinData> = await _rest.replace<HttpBinData>('https://httpbin.org/put', res);
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/put');
assert(restRes.result.json.name === 'foo');
assert(restRes.result && restRes.result.url === 'https://httpbin.org/put');
assert(restRes.result && restRes.result.json.name === 'foo');
});

it('replaces a resource with a baseUrl', async() => {
let res: any = { name: 'foo' };
let restRes: restm.IRestResponse<HttpBinData> = await _restBin.replace<HttpBinData>('put', res);
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/put');
assert(restRes.result.json.name === 'foo');
assert(restRes.result && restRes.result.url === 'https://httpbin.org/put');
assert(restRes.result && restRes.result.json.name === 'foo');
});

it('updates a resource', async() => {
let res: any = { name: 'foo' };
let restRes: restm.IRestResponse<HttpBinData> = await _rest.update<HttpBinData>('https://httpbin.org/patch', res);
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/patch');
assert(restRes.result.json.name === 'foo');
assert(restRes.result && restRes.result.url === 'https://httpbin.org/patch');
assert(restRes.result && restRes.result.json.name === 'foo');
});

it('updates a resource with a baseUrl', async() => {
let res: any = { name: 'foo' };
let restRes: restm.IRestResponse<HttpBinData> = await _restBin.update<HttpBinData>('patch', res);
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/patch');
assert(restRes.result.json.name === 'foo');
});
assert(restRes.result && restRes.result.url === 'https://httpbin.org/patch');
assert(restRes.result && restRes.result.json.name === 'foo');
});

it('deletes a resource', async() => {
let restRes: restm.IRestResponse<HttpBinData> = await _rest.del<HttpBinData>('https://httpbin.org/delete');
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/delete');
assert(restRes.result && restRes.result.url === 'https://httpbin.org/delete');
});

it('deletes a resource with a baseUrl', async() => {
let restRes: restm.IRestResponse<HttpBinData> = await _restBin.del<HttpBinData>('delete');
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/delete');
});
assert(restRes.result && restRes.result.url === 'https://httpbin.org/delete');
});

it('does an options request', async() => {
let restRes: restm.IRestResponse<HttpBinData> = await _rest.options<HttpBinData>('https://httpbin.org');
assert(restRes.statusCode == 200, "statusCode should be 200");
Expand All @@ -115,7 +115,7 @@ describe('Rest Tests', function () {
it('does an options request with baseUrl', async() => {
let restRes: restm.IRestResponse<HttpBinData> = await _restBin.options<HttpBinData>('');
assert(restRes.statusCode == 200, "statusCode should be 200");
});
});

//----------------------------------------------
// Get Error Cases
Expand All @@ -128,9 +128,9 @@ describe('Rest Tests', function () {
it('gets a non-existant resource (404)', async() => {
try {
let restRes: restm.IRestResponse<HttpBinData> = await _rest.get<HttpBinData>('https://httpbin.org/status/404');

assert(restRes.statusCode == 404, "statusCode should be 404");
assert(restRes.result == null, "object should be null");
assert(restRes.result === null, "object should be null");
Copy link
Member

Choose a reason for hiding this comment

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

result object should be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to your documentation in https://github.com/Microsoft/typed-rest-client/blob/master/README.md a 404 should return null.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep - in the REST layer since it's job is to return a restful object, a 404 should return a null. However the http layer will return body.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm fine with these lines / changes

Copy link
Member

Choose a reason for hiding this comment

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

I did a poor job of writing that comment. I meant that the string could be changed to be more expressive.

}
catch(err) {
assert(false, "should not throw");
Expand All @@ -141,7 +141,7 @@ describe('Rest Tests', function () {
// Unauthorized (401)
// should throw and attach statusCode to the Error object
// err.message is message proerty of resourceful error object or if not supplied, a generic error message
//
//
it('gets and handles unauthorized (401)', async() => {
try {
let restRes: restm.IRestResponse<HttpBinData> = await _rest.get<HttpBinData>('https://httpbin.org/status/401');
Expand All @@ -151,13 +151,13 @@ describe('Rest Tests', function () {
assert(err['statusCode'] == 401, "statusCode should be 401");
assert(err.message && err.message.length > 0, "should have error message");
}
});
});

//
// Internal Server Error
// should throw and attach statusCode to the Error object
// err.message is message proerty of resourceful error object or if not supplied, a generic error message
//
//
it('gets and handles a server error (500)', async() => {
try {
let restRes: restm.IRestResponse<HttpBinData> = await _rest.get<HttpBinData>('https://httpbin.org/status/500');
Expand All @@ -183,7 +183,7 @@ describe('Rest Tests', function () {

// Assert
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/anything/anythingextra');
assert(restRes.result && restRes.result.url === 'https://httpbin.org/anything/anythingextra');
});

it('maintains the path from the base url with no slashes', async() => {
Expand All @@ -195,7 +195,7 @@ describe('Rest Tests', function () {

// Assert
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/anything/anythingextra');
assert(restRes.result && restRes.result.url === 'https://httpbin.org/anything/anythingextra');
});

it('maintains the path from the base url with double slashes', async() => {
Expand All @@ -207,7 +207,7 @@ describe('Rest Tests', function () {

// Assert
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/anything/anythingextra');
assert(restRes.result && restRes.result.url === 'https://httpbin.org/anything/anythingextra');
});

it('maintains the path from the base url with multiple parts', async() => {
Expand All @@ -219,7 +219,7 @@ describe('Rest Tests', function () {

// Assert
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/anything/extrapart/anythingextra');
assert(restRes.result && restRes.result.url === 'https://httpbin.org/anything/extrapart/anythingextra');
});

it('maintains the path from the base url where request has multiple parts', async() => {
Expand All @@ -231,7 +231,7 @@ describe('Rest Tests', function () {

// Assert
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/anything/anythingextra/moreparts');
assert(restRes.result && restRes.result.url === 'https://httpbin.org/anything/anythingextra/moreparts');
});

it('maintains the path from the base url where both have multiple parts', async() => {
Expand All @@ -243,7 +243,7 @@ describe('Rest Tests', function () {

// Assert
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/anything/multiple/anythingextra/moreparts');
assert(restRes.result && restRes.result.url === 'https://httpbin.org/anything/multiple/anythingextra/moreparts');
});

it('maintains the path from the base url where request has query parameters', async() => {
Expand All @@ -255,9 +255,9 @@ describe('Rest Tests', function () {

// Assert
assert(restRes.statusCode == 200, "statusCode should be 200");
assert(restRes.result.url === 'https://httpbin.org/anything/multiple/anythingextra/moreparts?foo=bar&baz=top');
assert(restRes.result.args.foo === 'bar');
assert(restRes.result.args.baz === 'top');
assert(restRes.result && restRes.result.url === 'https://httpbin.org/anything/multiple/anythingextra/moreparts?foo=bar&baz=top');
assert(restRes.result && restRes.result.args.foo === 'bar');
assert(restRes.result && restRes.result.args.baz === 'top');
});

//
Expand All @@ -272,11 +272,11 @@ describe('Rest Tests', function () {
let res: string = util.getUrl('', 'http://httpbin.org');
assert(res === 'http://httpbin.org', "should be http://httpbin.org");
});

it('resolves a null resource with baseUrl', async() => {
let res: string = util.getUrl(null, 'http://httpbin.org');
assert(res === 'http://httpbin.org', "should be http://httpbin.org");
});
});

it('resolves a full resource and no baseUrl', async() => {
let res: string = util.getUrl('http://httpbin.org/get?x=y&a=b');
Expand All @@ -291,7 +291,7 @@ describe('Rest Tests', function () {
it('resolves a relative path resource with host baseUrl', async() => {
let res: string = util.getUrl('get/foo', 'http://httpbin.org');
assert(res === 'http://httpbin.org/get/foo', `should be http://httpbin.org/get/foo but is ${res}`);
});
});

it('resolves a rooted path resource with pathed baseUrl', async() => {
let res: string = util.getUrl('/get/foo', 'http://httpbin.org/bar');
Expand Down