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

Support path in the base url #23

Merged
merged 9 commits into from
Dec 2, 2017
Merged

Support path in the base url #23

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
55d8ca1
Add optional parameter to preserve path from base url
Nov 21, 2017
b53c4bb
Use path.join instead of concatenation
Nov 21, 2017
18fdc88
Remove forgotten console log
Nov 21, 2017
053191c
Remove parameter to toggle preservePath
Nov 22, 2017
42f98e9
relpaths
bryanmacfarlane Dec 1, 2017
bfa8753
Merge branch 'master' into master
Dec 1, 2017
0957f1c
Merge relpaths branch
Dec 1, 2017
f41b1b5
Use path.posix.join to force forward slash
Dec 1, 2017
149eb43
Fix unit tests
Dec 1, 2017
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
21 changes: 13 additions & 8 deletions lib/RestClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,18 +33,23 @@ export class RestClient {
* @param {string} baseUrl - (Optional) If not specified, use full urls per request. If supplied and a function passes a relative url, it will be appended to this
* @param {ifm.IRequestHandler[]} handlers - handlers are typically auth handlers (basic, bearer, ntlm supplied)
* @param {ifm.IRequestOptions} requestOptions - options for each http requests (http proxy setting, socket timeout)
* @param {boolean} preservePath -
*/
constructor(userAgent: string,
baseUrl?: string,
handlers?: ifm.IRequestHandler[],
requestOptions?: ifm.IRequestOptions) {
requestOptions?: ifm.IRequestOptions,
preservePath?: boolean) {
this.client = new httpm.HttpClient(userAgent, handlers, requestOptions);
if (baseUrl) {
this._baseUrl = baseUrl;
}

this._preservePath = preservePath || false;
}

private _baseUrl: string;
private _preservePath: boolean;

/**
* Gets a resource from an endpoint
Expand All @@ -55,7 +60,7 @@ export class RestClient {
public async options<T>(requestUrl: string,
options?: IRequestOptions): Promise<IRestResponse<T>> {

let url: string = util.getUrl(requestUrl, this._baseUrl);
let url: string = util.getUrl(requestUrl, this._baseUrl, this._preservePath);
let res: httpm.HttpClientResponse = await this.client.options(url,
this._headersFromOptions(options));
return this._processResponse<T>(res, options);
Expand All @@ -70,7 +75,7 @@ export class RestClient {
public async get<T>(requestUrl: string,
options?: IRequestOptions): Promise<IRestResponse<T>> {

let url: string = util.getUrl(requestUrl, this._baseUrl);
let url: string = util.getUrl(requestUrl, this._baseUrl, this._preservePath);
let res: httpm.HttpClientResponse = await this.client.get(url,
this._headersFromOptions(options));
return this._processResponse<T>(res, options);
Expand All @@ -85,7 +90,7 @@ export class RestClient {
public async del<T>(requestUrl: string,
options?: IRequestOptions): Promise<IRestResponse<T>> {

let url: string = util.getUrl(requestUrl, this._baseUrl);
let url: string = util.getUrl(requestUrl, this._baseUrl, this._preservePath);
let res: httpm.HttpClientResponse = await this.client.del(url,
this._headersFromOptions(options));
return this._processResponse<T>(res, options);
Expand All @@ -102,7 +107,7 @@ export class RestClient {
resources: any,
options?: IRequestOptions): Promise<IRestResponse<T>> {

let url: string = util.getUrl(requestUrl, this._baseUrl);
let url: string = util.getUrl(requestUrl, this._baseUrl, this._preservePath);
let headers: ifm.IHeaders = this._headersFromOptions(options, true);

let data: string = JSON.stringify(resources, null, 2);
Expand All @@ -121,7 +126,7 @@ export class RestClient {
resources: any,
options?: IRequestOptions): Promise<IRestResponse<T>> {

let url: string = util.getUrl(requestUrl, this._baseUrl);
let url: string = util.getUrl(requestUrl, this._baseUrl, this._preservePath);
let headers: ifm.IHeaders = this._headersFromOptions(options, true);

let data: string = JSON.stringify(resources, null, 2);
Expand All @@ -140,7 +145,7 @@ export class RestClient {
resources: any,
options?: IRequestOptions): Promise<IRestResponse<T>> {

let url: string = util.getUrl(requestUrl, this._baseUrl);
let url: string = util.getUrl(requestUrl, this._baseUrl, this._preservePath);
let headers: ifm.IHeaders = this._headersFromOptions(options, true);

let data: string = JSON.stringify(resources, null, 2);
Expand All @@ -153,7 +158,7 @@ export class RestClient {
stream: NodeJS.ReadableStream,
options?: IRequestOptions): Promise<IRestResponse<T>> {

let url: string = util.getUrl(requestUrl, this._baseUrl);
let url: string = util.getUrl(requestUrl, this._baseUrl, this._preservePath);
let headers: ifm.IHeaders = this._headersFromOptions(options, true);

let res: httpm.HttpClientResponse = await this.client.sendStream(verb, url, stream, headers);
Expand Down
10 changes: 8 additions & 2 deletions lib/Util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ import * as url from 'url';
* creates an url from a request url and optional base url (http://server:8080)
* @param {string} requestUrl - a fully qualified url or relative url
* @param {string} baseUrl - an optional baseUrl (http://server:8080)
* @param {boolean} preservePath - an optional baseUrl (http://server:8080)
* @return {string} - resultant url
*/
export function getUrl(requestUrl: string, baseUrl?: string): string {
export function getUrl(requestUrl: string, baseUrl?: string, preservePath?: boolean): string {
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 struggling with whether this should be the default. In the normal case of just providing the hostname portion (no path) it does the right thing. In the case where the base url has a relative path, it does the right thing. I need to think about it.

This is pre-release about to go to 1.0 and this change should bump the minor version (so ^ with patch level won't drift into this behavior) and we can doc.

The other thought was flip to opt in to chop the relative path but if that's your intent, just provide the hostname portion. No need for another optional arg ...hmmmm

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is still prerelease, my preference would be to never strip the relative path. Reason being similar to your thought about flipping to opt in. If the use case comes up that the base URL also has a path, then it would make sense to explicitly state it in the baseUrl parameter, and always expect it to be there. At that point the optional parameter can be completely removed. That would make this a breaking change though...

Copy link
Member

Choose a reason for hiding this comment

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

@kaeedo Could you add some examples of usage for this feature? Then we can speak to the explicit use cases. It often helps think things through and make the "right" choice more apparent. (based on the perspective of a consumer of the library). As well as what the consumer would have to do if we didn't have this feature.

It's tricky to balance where we draw the line of what's included and what isn't.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kaeedo - I agree on your points. Bump the minor version with this change. That mitigates the pre-release breaking change since folks default to sliding on patch versions. And it's only a change when you're doing something non mainstream (passing base url with path but then expecting it not to be honored). Let's move forward without the arg. Add the test cases on trailing and leading slashes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stephenmichaelf
For the use case in my office, we have a reverse proxy configured to point to different docker containers:

https://www.contoso.com/firstcontainer
https://www.contoso.com/secondcontainer
https://www.contoso.com/thirdcontainer

I would like to create an instance of a RestClient such that https://www.contoso.com/firstcontainer is the baseUrl instead of just https://www.contoso.com and then having to prepend firstcontainer to every rest request

if (!baseUrl) {
return requestUrl;
}
Expand All @@ -18,8 +19,13 @@ export function getUrl(requestUrl: string, baseUrl?: string): string {
combined.protocol = combined.protocol || base.protocol;
combined.auth = combined.auth || base.auth;
combined.host = combined.host || base.host;
// path from requestUrl always wins

if (preservePath) {
combined.pathname = base.pathname + combined.pathname;
Copy link
Contributor

Choose a reason for hiding this comment

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

path.join? I think there is also interesting test cases with base not / having trailing slash and pathname not/having leading slash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using path.join now. Also fixes the trailing and leading slash problems

}

// path from requestUrl always wins
let res: string = url.format(combined);

return res;
}
2 changes: 1 addition & 1 deletion package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
{
"name": "typed-rest-client",
"version": "0.14.2",
"version": "0.15.0",
"description": "Node Rest and Http Clients for use with TypeScript",
"main": "./RestClient.js",
"scripts": {
"build": "node make.js build",
"test": "node make.js test",
"samples": "node make.js samples"
},
"engines": {
"node": ">=8.9.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

node 8.9 / npm 5.1 is only a developer build time dependency. At runtime this should and needs to work on 6.x LTS and even back to node 5.10. I would remove this from the PR and make another issue to define runtime req (I want our CI to multi plex and validate on ideally >= node 4.x LTS but at least node >=6.x LTS)

"npm": ">=5.5.1"
},
"repository": {
"type": "git",
"url": "git+https://github.com/Microsoft/typed-rest-client.git"
Expand Down
91 changes: 89 additions & 2 deletions test/resttests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import * as path from 'path';
export interface HttpBinData {
url: string;
data: any;
args?: any
}

describe('Rest Tests', function () {
Expand Down Expand Up @@ -84,7 +85,93 @@ describe('Rest Tests', function () {
assert(err['statusCode'] == 500, "statusCode should be 500");
assert(err.message && err.message.length > 0, "should have error message");
}
});
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Add divider

//--------------------------------------------------------
// Path tests
//--------------------------------------------------------

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

it('removes the path from the base url', async() => {
// Arrange
let rest = new restm.RestClient('typed-rest-client-tests', 'https://httpbin.org/pathtoremove');

// Act
let restRes: restm.IRestResponse<HttpBinData> = await rest.get<HttpBinData>('/get');

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

it('removes the path from the base url when excplicit', async() => {
// Arrange
let rest = new restm.RestClient('typed-rest-client-tests', 'https://httpbin.org/pathtoremove', null, null, false);

// Act
let restRes: restm.IRestResponse<HttpBinData> = await rest.get<HttpBinData>('/get');

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

it('maintains the path from the base url', async() => {
// Arrange
let rest = new restm.RestClient('typed-rest-client-tests', 'https://httpbin.org/anything', null, null, true);

// Act
let restRes: restm.IRestResponse<HttpBinData> = await rest.get<HttpBinData>('/anythingextra');

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

it('maintains the path from the base url with multiple parts', async() => {
// Arrange
let rest = new restm.RestClient('typed-rest-client-tests', 'https://httpbin.org/anything/extrapart', null, null, true);

// Act
let restRes: restm.IRestResponse<HttpBinData> = await rest.get<HttpBinData>('/anythingextra');

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

it('maintains the path from the base url where request has multiple parts', async() => {
// Arrange
let rest = new restm.RestClient('typed-rest-client-tests', 'https://httpbin.org/anything', null, null, true);

// Act
let restRes: restm.IRestResponse<HttpBinData> = await rest.get<HttpBinData>('/anythingextra/moreparts');

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

it('maintains the path from the base url where both have multiple parts', async() => {
// Arrange
let rest = new restm.RestClient('typed-rest-client-tests', 'https://httpbin.org/anything/multiple', null, null, true);

// Act
let restRes: restm.IRestResponse<HttpBinData> = await rest.get<HttpBinData>('/anythingextra/moreparts');

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

it('maintains the path from the base url where request has query parameters', async() => {
// Arrange
let rest = new restm.RestClient('typed-rest-client-tests', 'https://httpbin.org/anything/multiple', null, null, true);

// Act
let restRes: restm.IRestResponse<HttpBinData> = await rest.get<HttpBinData>('/anythingextra/moreparts?foo=bar&baz=top');

// 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');
});

// TODO: more tests here
});
});