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

Change URI#to_s to handle default ports for more schemes than http/https #5233

Merged
merged 26 commits into from
Jan 2, 2018
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
d844362
Change URI#to_s to handle default ports for more schemes than http/https
lachlan Nov 2, 2017
6077c5b
Change DEFAULT_PORTS to be a constant rather than class variable
lachlan Nov 2, 2017
18a0728
Rename method to default_port? and move nil check back to #to_s
lachlan Nov 2, 2017
b886778
Remove unnecessary return statement from default_port? method
lachlan Nov 3, 2017
23a825e
Add URI.default_ports method, which allows for default ports to
lachlan Nov 4, 2017
5cb8564
Fix URI::DefaultPorts#[]= to downcase scheme on delete
lachlan Nov 4, 2017
57a2936
Add URI.default_port method for returning a scheme's the default port
lachlan Nov 4, 2017
965ccde
Rename URI.default_ports method to URI.default_port, allowing
lachlan Nov 4, 2017
a665fe4
Fix method comments to use back ticks consistently
lachlan Nov 4, 2017
82fa4c4
Change DefaultPort class to use instance variable rather than class
lachlan Nov 4, 2017
1fe8e52
Simplify to use two methods: URI.default_port and URI.set_default_port
lachlan Nov 4, 2017
43df609
Fix the method documentation formatting
lachlan Nov 4, 2017
a43adff
Add more well-known default ports sourced from IANA via Mahmoud's
lachlan Nov 4, 2017
aeffdf4
Fix #default_port? to correctly handle nil scheme
lachlan Nov 4, 2017
5923d8e
Change URI default port API to return a mutable subclass of Hash
lachlan Nov 5, 2017
3afe221
Change DefaultPortHash seeding to use merge! method
lachlan Nov 5, 2017
88c7a5f
Fix default_port? to not call scheme.downcase unnecessarily
lachlan Nov 5, 2017
b64d394
Add spec for URI with nil scheme and non-nil port
lachlan Nov 5, 2017
1684fab
Change DefaultPortHash seeds to be an initialize parameter
lachlan Nov 5, 2017
4c10f9f
Fix DefaultPortHash case insensitive keys for methods other than []= …
lachlan Nov 5, 2017
d8af953
Revert to using two methods: URI.default_port and URI.set_default_port
lachlan Nov 5, 2017
4f8dd3e
Fix typo in URI.default_port comment
lachlan Nov 5, 2017
fa62ae9
Fix typo in URI.set_default_port comment
lachlan Nov 5, 2017
de6329b
Fix URI.set_default_port comment formatting for scheme parameter
lachlan Nov 5, 2017
7dec28c
Fix URI#default_port? to be thread safe
lachlan Nov 24, 2017
bb3f56a
Change URI.set_default_port to always return nil
lachlan Nov 25, 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
45 changes: 44 additions & 1 deletion spec/std/uri_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,53 @@ describe "URI" do
end
it { URI.new("http", "www.example.com", user: "@al:ce", password: "s/cr3t").to_s.should eq("http://%40al%3Ace:s%2Fcr3t@www.example.com") }
it { URI.new("http", "www.example.com", fragment: "top").to_s.should eq("http://www.example.com#top") }
it { URI.new("http", "www.example.com", 1234).to_s.should eq("http://www.example.com:1234") }
it { URI.new("http", "www.example.com", 80, "/hello").to_s.should eq("http://www.example.com/hello") }
it { URI.new("http", "www.example.com", 80, "/hello", "a=1").to_s.should eq("http://www.example.com/hello?a=1") }
it { URI.new("mailto", opaque: "foo@example.com").to_s.should eq("mailto:foo@example.com") }

it "removes default port" do
URI.new("http", "www.example.com", 80).to_s.should eq("http://www.example.com")
URI.new("https", "www.example.com", 443).to_s.should eq("https://www.example.com")
URI.new("ftp", "www.example.com", 21).to_s.should eq("ftp://www.example.com")
URI.new("sftp", "www.example.com", 22).to_s.should eq("sftp://www.example.com")
URI.new("ldap", "www.example.com", 389).to_s.should eq("ldap://www.example.com")
URI.new("ldaps", "www.example.com", 636).to_s.should eq("ldaps://www.example.com")
end

it "preserves non-default port" do
URI.new("http", "www.example.com", 1234).to_s.should eq("http://www.example.com:1234")
URI.new("https", "www.example.com", 1234).to_s.should eq("https://www.example.com:1234")
URI.new("ftp", "www.example.com", 1234).to_s.should eq("ftp://www.example.com:1234")
URI.new("sftp", "www.example.com", 1234).to_s.should eq("sftp://www.example.com:1234")
URI.new("ldap", "www.example.com", 1234).to_s.should eq("ldap://www.example.com:1234")
URI.new("ldaps", "www.example.com", 1234).to_s.should eq("ldaps://www.example.com:1234")
end

it "preserves port for unknown scheme" do
URI.new("xyz", "www.example.com").to_s.should eq("xyz://www.example.com")
URI.new("xyz", "www.example.com", 1234).to_s.should eq("xyz://www.example.com:1234")
end
end

describe ".default_port" do
it "returns default port for well known schemes" do
URI.default_port("http").should eq(80)
URI.default_port("https").should eq(443)
end

it "returns nil for unknown schemes" do
URI.default_port("xyz").should eq(nil)
end

it "registers port for scheme" do
URI.set_default_port("ponzi", 9999)
URI.default_port("ponzi").should eq(9999)
end

it "unregisters port for scheme" do
URI.set_default_port("ftp", nil)
URI.default_port("ftp").should eq(nil)
end
end

describe ".unescape" do
Expand Down
50 changes: 49 additions & 1 deletion src/uri.cr
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ class URI
if host
io << host
end
if port && !((scheme == "http" && port == 80) || (scheme == "https" && port == 443))
unless port.nil? || default_port?
io << ':'
io << port
end
Expand Down Expand Up @@ -426,4 +426,52 @@ class URI
URI.escape(password, io)
end
end

# A map of schemes and their respective default ports, seeded
# with some well-known schemes.
@@default_ports = {
"ftp" => 21,
"ftps" => 990,
"gopher" => 70,
"http" => 80,
"https" => 443,
"ldap" => 389,
"ldaps" => 636,
"nntp" => 119,
"scp" => 22,
"sftp" => 22,
"ssh" => 22,
"telnet" => 23,
}

# Returns the default port for the given scheme if known, otherwise
# returns nil.
#
# ```
# URI.default_port "http" # => 80
# ```
def self.default_port(scheme : String) : Int32?
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 the URI.default_port method can return nil, should it be suffixed with a question mark (URI.default_port?) as per other methods such as Hash#first_key? that can return nil?

Copy link
Member

Choose a reason for hiding this comment

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

No, this is only used when there is also a variant of the method which does not return nil (usually raises instead). This makes no sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, thanks very much for the explanation!

@@default_ports[scheme.downcase]?
end

# Registers the default port for the given scheme.
#
# If port is nil, the existing default port for the scheme, if any,
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 the doc formatting should be If *port* is `nil`, ...

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!

# will be unregistered.
#
# ```
# URI.set_default_port "ponzi" = 9999
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: = -> ,

# ```
def self.set_default_port(scheme : String, port : Int32?)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

URI.set_default_port is (unintentionally) returning either the previous default port when unregistering, or the new default port when registering.

Should I change it to always return nil, or maybe to return the previous default port value that was replaced or unregistered if any?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just restrict it to : Nil in the declaration and the last line of the method will be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

if port
@@default_ports[scheme.downcase] = port
else
@@default_ports.delete scheme.downcase
end
end

# Returns true if this URI's port is the default port for its scheme.
private def default_port?
port == @@default_ports[scheme]?
end
end