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.

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

omit default ports by default (probably remove option also) #357

Closed
geemus opened this issue Jan 9, 2014 · 9 comments · May be fixed by #448
Closed

omit default ports by default (probably remove option also) #357

geemus opened this issue Jan 9, 2014 · 9 comments · May be fixed by #448
Labels
Milestone

Comments

@geemus
Copy link
Contributor

geemus commented Jan 9, 2014

This will require some fixes in fog (around signing for AWS). So will have to coordinate releases. That said, omitting the ports would simplify (and prevents some gnarly issues that occasionally creep up).

@dentarg
Copy link
Contributor

dentarg commented Jun 9, 2014

I can add that omit_default_port: true helped me when getting http://www.jigsaw-online.com/products/tribal-geo-trousers-9056, I got 404 with excon without it. Chrome, curl, wget all had no problems getting to the content. I think it would be good to change the defaults.

@dolzenko
Copy link

+1 for omitting by default. I think I was pretty much about to go insane because of this option. Using Google APIs with Excon failed with OAUTH_TOKEN_INVALID error while every other client worked (console curl, net/http).

PS. According to spec it can be indeed omitted http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.23

@sigmavirus24
Copy link

According to RFC 7230 the Host header should be generated from the requested URI. Ports only belong there if the requested URI is something like https://example.org:442/some/resource. So this should default to true or the option should be renamed to include_default_port and be set to false.

@geemus
Copy link
Contributor Author

geemus commented Dec 5, 2014

Yeah, the difficulty comes from Ruby's URI implementation. When it does URI.parse on something it ALWAYS returns an integer port, regardless of whether there was one or not. So it isn't as easy as it could (or perhaps should) be to omit it unless it is included from the outset.

@mathias
Copy link
Contributor

mathias commented Dec 5, 2014

I would like feedback from excon users (particularly those in this thread that have looked at the omit_default_port functionality) over in PR #448 -- it'd be good to know what people think about introducing a breaking change for this. Thanks!

@sigmavirus24
Copy link

@geemus so I forgot that #port is always populated but here's my toying around in irb a bit:

1.9.3-p448 :001 > require 'uri'
 => true
1.9.3-p448 :002 > u = URI('https://api.github.com/user')
 => #<URI::HTTPS:0x007fcf39a427d8 URL:https://api.github.com/user>
1.9.3-p448 :003 > u.default_port, u.port
=> [443, 443]
1.9.3-p448 :004 > v = URI('https://api.github.com:444/user')
 => #<URI::HTTPS:0x007fcf39a536a0 URL:https://api.github.com:444/user>
1.9.3-p448 :005 > v.default_port
 => 443
1.9.3-p448 :006 > v.port
 => 444

So the easy thing to do is check if parsed.default_port == parsed.port to provide a reliable default. If users then need to include the port they should use an option (like include_default_port) to force it to be included. I found it surprising that I had to use omit_default_port to prevent the port from being included when every other User-Agent I've used has behaved as if omit_default_port were always true.

@geemus
Copy link
Contributor Author

geemus commented Dec 8, 2014

Yeah. Everything I read at the time I originally implemented things seemed to indicate that there was no harm in either including or omitting the default port. So I thought it would be simpler to not have the exceptional case around defaults, instead simply always including it. And it was simpler, certainly, but has lead to some weird edge cases since though it is technically correct, there are cases where in usage it does not follow this technicality closely.

@stale
Copy link

stale bot commented Jul 30, 2018

This issue has been automatically marked stale due to inactivity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 30, 2018
@geemus
Copy link
Contributor Author

geemus commented Jul 30, 2018

Closing as duplicate of #448

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants