This repository has been archived by the owner. It is now read-only.

output of url.parse should align with http.request options #1390

Closed
ry opened this Issue Jul 23, 2011 · 11 comments

Comments

Projects
None yet
6 participants
@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Jul 23, 2011

So, it seems like the change would be:

  1. "hostname" becomes "host" (and "host" becomes... what?)
  2. add a "path" member which is pathname+search
  3. maybe if there's an "auth" member then we should add the Authorization: Basic <base64> header?

Would it be easier to update the http.request to take a "url" member, which can be a string or a parsed url? Seems like something like this would be really nice:

http.get(url.parse("http://user:pass@blah.com/bloo?baz"), function (res) { .. })

// or even just:
http.get("http://user:pass@blah.com/bloo?baz", function (res) { .. })

isaacs commented Jul 23, 2011

So, it seems like the change would be:

  1. "hostname" becomes "host" (and "host" becomes... what?)
  2. add a "path" member which is pathname+search
  3. maybe if there's an "auth" member then we should add the Authorization: Basic <base64> header?

Would it be easier to update the http.request to take a "url" member, which can be a string or a parsed url? Seems like something like this would be really nice:

http.get(url.parse("http://user:pass@blah.com/bloo?baz"), function (res) { .. })

// or even just:
http.get("http://user:pass@blah.com/bloo?baz", function (res) { .. })
@mikeal

This comment has been minimized.

Show comment Hide comment
@mikeal

mikeal Jul 28, 2011

Member

is the hostname to host change a breaking change?

taking an url parameter starts to dip in to bike shedding and additional features. it would be convenient but would not work for proxying, which means it's only a matter of time before someone asks for proxy support :)

Member

mikeal commented Jul 28, 2011

is the hostname to host change a breaking change?

taking an url parameter starts to dip in to bike shedding and additional features. it would be convenient but would not work for proxying, which means it's only a matter of time before someone asks for proxy support :)

@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Jul 28, 2011

is the hostname to host change a breaking change?

Yes. url.parse puts the machine name (ie, the thing that DNS knows about) in "hostname", but the either auth+machinename+port into the "host" field.

isaacs commented Jul 28, 2011

is the hostname to host change a breaking change?

Yes. url.parse puts the machine name (ie, the thing that DNS knows about) in "hostname", but the either auth+machinename+port into the "host" field.

@ry

This comment has been minimized.

Show comment Hide comment
@ry

ry Aug 6, 2011

also should be somewhat backwards compatible if possible

ry commented Aug 6, 2011

also should be somewhat backwards compatible if possible

@seebees

This comment has been minimized.

Show comment Hide comment
@seebees

seebees Aug 8, 2011

If I may, I would like to work on this. I would suggest making the code switch on options.hostname If the options object passed to ClientRequest has a hostname property, then I would treat is as a url.parse object otherwise legacy. make this change, wait thought .6 to see what people make of it. Then in .7 either change url.parse or remove .host from http.request

  1. I do not see a .path property as necessary, but if the options object has .hostname, .path, .pathname, .search wackiness could ensue if .pathname + .search != .path. But hey, garbage in garbage out.
  2. If .path is added to url.parse wouldn't it need to be .pathname + .search + .hash?
  3. what about http.request(url.parse('https://www.example.com'));? (I know what I think ;)
  4. http.js or http2.js?

Finally what would acceptance entail?

  1. All current tests pass.
  2. Duplicates of current tests with url.parse(.host + .port + .path)
  3. Wacky objects that mix the two calling types, e.g. .path && .pathname
  4. .auth causes Basic auth header.

And as long as I'm in there..., but maybe these should be individual issues?

  1. .setHeader and .getHeader should use ._headerSent (I think that ._header is just the string of .heasers not if it was sent) regardless there should not be 2 different kinds checks.
  2. There are several checks like asdf === undefined, but undefined = 3; works just fine and will make these checks break.
  3. http:760 _flush loops ret = this.socket.write();, but does not check to make sure ret !== false It should fully implement the write as a stream.

seebees commented Aug 8, 2011

If I may, I would like to work on this. I would suggest making the code switch on options.hostname If the options object passed to ClientRequest has a hostname property, then I would treat is as a url.parse object otherwise legacy. make this change, wait thought .6 to see what people make of it. Then in .7 either change url.parse or remove .host from http.request

  1. I do not see a .path property as necessary, but if the options object has .hostname, .path, .pathname, .search wackiness could ensue if .pathname + .search != .path. But hey, garbage in garbage out.
  2. If .path is added to url.parse wouldn't it need to be .pathname + .search + .hash?
  3. what about http.request(url.parse('https://www.example.com'));? (I know what I think ;)
  4. http.js or http2.js?

Finally what would acceptance entail?

  1. All current tests pass.
  2. Duplicates of current tests with url.parse(.host + .port + .path)
  3. Wacky objects that mix the two calling types, e.g. .path && .pathname
  4. .auth causes Basic auth header.

And as long as I'm in there..., but maybe these should be individual issues?

  1. .setHeader and .getHeader should use ._headerSent (I think that ._header is just the string of .heasers not if it was sent) regardless there should not be 2 different kinds checks.
  2. There are several checks like asdf === undefined, but undefined = 3; works just fine and will make these checks break.
  3. http:760 _flush loops ret = this.socket.write();, but does not check to make sure ret !== false It should fully implement the write as a stream.
@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Aug 8, 2011

@seebees Yes! That'd be awesome. Please sign the CLA if you haven't already: http://nodejs.org/cla.html

I do not see a .path property as necessary, but if the options object has .hostname, .path, .pathname, .search wackiness could ensue if .pathname + .search != .path. But hey, garbage in garbage out.

Easiest way would be to add a "path" field to the url.parse output. obj.path = obj.pathname + obj.search. Lots of busywork updating the tests, but I think that's the best way to do it.

wouldn't it need to be .pathname + .search + .hash?

No. The hash is not typically sent in an HTTP request, it's client-only.

http.js or http2.js?

All new stuff should be in http2.js

All current tests pass.

Yep. ./configure --debug && make test-all. (make test is faster, but you should do test-all every so often as well.)

Run make jslint should not get any worse with your changes. (I usually run it and grep for the file I'm editing, just to make sure.)

Duplicates of current tests with url.parse(.host + .port + .path)

Sure. Whatever seems reasonable to make sure that the new code is being exercised.

.setHeader, undefined, ret !== false, etc.

Yeah, those are separate issues. Leave as-is for now. (Using void 0 instead of undefined has come up before, and got a solid "meh". The world breaks if you set undefined to something, so it's not as if it'd go unnoticed.)

Other stuff:

  1. If the protocol is anything other than "https:", then https.request should refuse it, and same for http.request and "http:" protocol.
  2. If the method isn't specified, I think it should just do whatever it currently does. (Default to GET or something?) I'd rather not have url.parse return a method member.
  3. Definitely only set an authorization: "Basic "+base64(auth) header if there's not already an authorization header.

Thanks!

isaacs commented Aug 8, 2011

@seebees Yes! That'd be awesome. Please sign the CLA if you haven't already: http://nodejs.org/cla.html

I do not see a .path property as necessary, but if the options object has .hostname, .path, .pathname, .search wackiness could ensue if .pathname + .search != .path. But hey, garbage in garbage out.

Easiest way would be to add a "path" field to the url.parse output. obj.path = obj.pathname + obj.search. Lots of busywork updating the tests, but I think that's the best way to do it.

wouldn't it need to be .pathname + .search + .hash?

No. The hash is not typically sent in an HTTP request, it's client-only.

http.js or http2.js?

All new stuff should be in http2.js

All current tests pass.

Yep. ./configure --debug && make test-all. (make test is faster, but you should do test-all every so often as well.)

Run make jslint should not get any worse with your changes. (I usually run it and grep for the file I'm editing, just to make sure.)

Duplicates of current tests with url.parse(.host + .port + .path)

Sure. Whatever seems reasonable to make sure that the new code is being exercised.

.setHeader, undefined, ret !== false, etc.

Yeah, those are separate issues. Leave as-is for now. (Using void 0 instead of undefined has come up before, and got a solid "meh". The world breaks if you set undefined to something, so it's not as if it'd go unnoticed.)

Other stuff:

  1. If the protocol is anything other than "https:", then https.request should refuse it, and same for http.request and "http:" protocol.
  2. If the method isn't specified, I think it should just do whatever it currently does. (Default to GET or something?) I'd rather not have url.parse return a method member.
  3. Definitely only set an authorization: "Basic "+base64(auth) header if there's not already an authorization header.

Thanks!

@mikeal

This comment has been minimized.

Show comment Hide comment
@mikeal

mikeal Aug 8, 2011

Member

not all the tests in master pass for me right now. all the HTTP ones do though.

make sure you branch for this patch, then you can compare the failed tests with master to see if you broke any new ones.

Member

mikeal commented Aug 8, 2011

not all the tests in master pass for me right now. all the HTTP ones do though.

make sure you branch for this patch, then you can compare the failed tests with master to see if you broke any new ones.

@seebees

This comment has been minimized.

Show comment Hide comment
@seebees

seebees Aug 11, 2011

http.request('http://www.example'); may be a bad idea for a number of reasons. But what about http.get? Not a problem to do it, just want to know. At that rate you may end up sucking all the methods into core? It was not clear if that is a yes or no. All and none are easy, some is hard...

I implemented in ClientRequest so http.request collapses and http.request(url.parse('https://www.example.com')) works fine. This means that I lied, really I'm switching on options.protocol and I just prefer options.hostname over options.host

I asked in #1462, but I wanted to be clear on who is making url.parse().path work? And should I put that work here or in #1462 (along with a whole lot of test changes...) if I'm doing it.

seebees commented Aug 11, 2011

http.request('http://www.example'); may be a bad idea for a number of reasons. But what about http.get? Not a problem to do it, just want to know. At that rate you may end up sucking all the methods into core? It was not clear if that is a yes or no. All and none are easy, some is hard...

I implemented in ClientRequest so http.request collapses and http.request(url.parse('https://www.example.com')) works fine. This means that I lied, really I'm switching on options.protocol and I just prefer options.hostname over options.host

I asked in #1462, but I wanted to be clear on who is making url.parse().path work? And should I put that work here or in #1462 (along with a whole lot of test changes...) if I'm doing it.

@seebees

This comment has been minimized.

Show comment Hide comment
@seebees

seebees Aug 17, 2011

So how do I say this is done? Close it? I still want to make url.resolveObject work but I was unsure on how to ask to have the code reviewed...

seebees commented Aug 17, 2011

So how do I say this is done? Close it? I still want to make url.resolveObject work but I was unsure on how to ask to have the code reviewed...

@bnoordhuis

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Aug 17, 2011

Member

@seebees Is 9055a64 your fix? If so, summon @isaacs for a review.

Member

bnoordhuis commented Aug 17, 2011

@seebees Is 9055a64 your fix? If so, summon @isaacs for a review.

@seebees

This comment has been minimized.

Show comment Hide comment
@seebees

seebees Aug 18, 2011

I summon @isaacs, O benevolent reviewer or code. Look not askance of my syntax, you who who have drawn near to my commit, who at your coming have come home. Guard us from the buffer overflow and the underrun, and guard us from the general protection fault, O @isaacs, and so be good for us to pass.

seebees commented Aug 18, 2011

I summon @isaacs, O benevolent reviewer or code. Look not askance of my syntax, you who who have drawn near to my commit, who at your coming have come home. Guard us from the buffer overflow and the underrun, and guard us from the general protection fault, O @isaacs, and so be good for us to pass.

seebees added a commit to seebees/node-1 that referenced this issue Oct 15, 2011

http.request(url.parse(x))
http2.js

protocols object to store defaults for http and https, and use as a switch for supported protocols.
options.hostname > options.host > 'localhost'
if I have an options.auth element and I do not have an Authorization header, I do basic auth.
http.request collapses to new ClientRequest since the defaults are handled by the protocol object

test-http-url.parse*

Fixes #1390

Conflicts:

	lib/http2.js

@koichik koichik closed this in 005d607 Oct 22, 2011

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.