Refactor and improve URI implementation #6323

straight-shoota · 2018-07-03T20:32:23Z

This PR applies a ton of mostly small improvements to URI and URI::Parser.

The type of URI#path is changed to String. It is no longer nilable. The semantic is: every URI has a path, but it may be empty. This avoids ambiguity between empty path and nil path.
The macro URI::Parser.step is removed. It was previously used to spec internals of URI::Parser but it no longer works because the struct can't be inherited. It doesn't make much sense to spec the internal API anyway, having unit tests for URI.parse is totally fine. Consequently, I have removed the specs for URI::Parser and integrated them into the specs for URI.parse.
Most notable is probably the removal of the property URI#opaque. It is simply not necessary to have an extra ivar for this. It is replaced by URI#path which contains both a path in the hierarchical part as well as a non-hierarchical, opaque path. URI#opaque? answers if the URI is considered opaque.
The spec suite has been reorganized and I added a bunch of new examples, borrows from gloang and akka.
During that process I discovered and fixed a few issues in the parser and formatter:
- #to_s no longer hides the port if it is the default port. A default port can now be stripped using #normalize.
- #parse downcases the scheme to make it case-insensitive. This is recommended by RFC 3986.
- #parse decodes escape sequences in host, and #to_s applies them.
- #parse correctly parses an empty port. Previously, scheme://host:/ would set port to 0, now it's nil.
- #parse parses a string starting with three slashes as empty host. Previously, host was nil.
- #to_s inserts a slash if path does not start with one and follows an authority. Paths starting with double slash // are prefixed by /. if it does not follow an authority. Otherwise the first path component would be interpreted as authority.

yxhuvud · 2018-07-03T21:10:13Z

spec/std/uri_spec.cr

+  describe ".parse" do
+    # scheme
+    assert_uri("http:", scheme: "http")
+    it { URI.parse("HttP:").should eq(URI.new(scheme: "http")) }


you are missing a spec of what the result of #scheme on a missing scheme is. Some tests below lacks scheme but none tests what #scheme is in that case.

I'm not sure what you mean. There are many specs for no-scheme URIs. For example assert_uri("/foo", path: "/foo"). It ensures that the URI parsed from "/foo" has a path of /foo and all other properties at the default value. This means scheme should be nil.

yxhuvud · 2018-07-03T21:15:32Z

The test failures make me wonder if eq should handle default ports.

straight-shoota · 2018-07-03T21:33:58Z

Nah, the specs are only failing because of the previous incorrect behavior of ommitting the port if it was specified but equal to the default.

Comparing two URIs should be strict. If a port is specified, even if it's the default, there are chances this is on purpose. http://example.com and http://example.com:80 are not identical. You can easily use #normalize to remove default ports.

asterite · 2018-07-03T21:38:19Z

$ irb
irb(main):001:0> require "uri"
=> true
irb(main):002:0> u1 = URI.parse("http://foo.com:80")
=> #<URI::HTTP http://foo.com>
irb(main):003:0> u2 = URI.parse("http://foo.com")
=> #<URI::HTTP http://foo.com>
irb(main):004:0> u1 == u2
=> true

asterite · 2018-07-03T21:39:10Z

I'm not saying we should copy Ruby... but specifying a default port when it's the default one, and keeping it around... I don't know... there's no difference, really.

straight-shoota · 2018-07-03T21:57:55Z

@asterite Yeah, Ruby does that. But it also uses a scheme-specific type URI::HTTP. Maybe that can serve as an excuse...

Besides, just a few counter examples:

u, err := url.Parse("http://foo.org:80")
fmt.Println(u.Port()) // => 80

System.out.println(new URI("http://foo.com:80").getPort()); // => 80

urlparse('http://foo.com:80').port # => 80

straight-shoota · 2018-07-03T22:05:22Z

there's no difference, really.

For well-known schemes, probably not. But when you're using a custom scheme, it might not be given that the next consumer of a URI has the same default port configured.

URI should be a simple data container and serialize to and from string representation without any further processing. If you want to get rid of unnecessary default ports, use #normalize.

URI.parse(string).to_s == string should hold for any well-formed URI.

asterite · 2018-07-03T23:40:57Z

Sounds good

src/uri.cr

straight-shoota · 2018-10-05T13:19:20Z

🏓 A second review would be great!

src/uri.cr

straight-shoota · 2018-11-27T22:16:00Z

@bcardiff 🏓

straight-shoota · 2019-01-07T21:18:42Z

I'd really like to get this merged for 0.28.0. It's so annoying to have to work around URI bugs I fixed six months ago.

bararchy · 2019-01-07T21:29:59Z

I'm with @straight-shoota please let's get this merged

spec/std/uri_spec.cr

Sija · 2019-01-07T21:53:07Z

spec/std/uri_spec.cr

-    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


Why not normalize ports from default?

There are no generic default ports in the URI specification. It depends solely on configuration which port is considered "default".

When no port is specified, the resolver is entrusted to choose whichever the port it seems fit. But when a port is explicitly specified, the resolver is asked to use that specific port, regardless of it's configuration for default ports.

Thus removing a port (even if it is considered default) mutates the URI. It looses information.
A well-formed URI should not be automatically stripped of anything unless explicitly asked (for example using #normalize).

asterite · 2019-01-08T10:30:12Z

I think it makes sense to keep the port if the URI has it set explicitly, even if it's the default port for that scheme.

src/uri.cr

straight-shoota · 2019-02-11T14:32:37Z

Rebased and ready to ship.

jkthorne · 2019-02-11T22:45:04Z

@straight-shoota should all the parse_ methods in the src/uri/uri_parser.cr file be private?

jkthorne · 2019-02-12T19:48:05Z

I just realized that might have been out of scope for this refactor but thanks for moving the parse methods to private.

src/uri.cr

straight-shoota · 2019-02-12T20:13:53Z

@wontruefree Yeah I thought so at first. But then, what the heck... it's a simple thing and fits to the set of changes in this PR.

bcardiff

We need a rebase and fix some conflict and we can merge this.

Every URI has a path, but it may be empty. Making the ivar non-nilable avoids any ambiguity whether an empty path is represented by an empty string (`""`) or `nil`.

This macro was previously used to spec internals of URI::Parser, but it no longer works because the struct can't be inherited. It doesn't make much sense to spec the internal API anyway. Having unit tests for URI.parse is sufficient. The specs for URI::Parser integrated into the specs for URI.parse.

Previously, `#to_s` would omit the port if it matches the default port, even if it is explicitly specified. This ommitance changes the URI and must be avoided, unless explicitly asked to normalize. Changes `#normalize` to remove the port if matches the schema's default port.

It is not necessary to have an extra ivar for an opaque path, URI#path is sufficient and can hold the path of both opaque and hiearchicyl URIs. Adds URI#opaque? to tell whether the URI is considered opaque.

URI schemes are case-insensitive, this ensures that. This approach is recommended by RFC 3986.

URI.prase decodes escape sequences in host, URI#to_s applies them where necessary. This is already some implicity normalization but results in the same URI, although in a differen string representation.

An empty port was previously interpreted as port `0`. Now it's `nil`.

An URI starting with three slashes (`///`) has an empty authority but it's still a hierarchical URI. `host` must be the empty string, not `nil`.

`#to_s` inserts a slash if `path` does not start with one and follows a non-nil authority. Paths starting with double slash (`"//"`) are prefixed by ("/.`) if it does not follow an authority. Otherwise the URI would be considered relative to the schema and the first path component interpreted as authority.

straight-shoota force-pushed the jm/fix/uri-refactor branch from 1afc004 to 165ae8d Compare July 3, 2018 21:05

yxhuvud reviewed Jul 3, 2018

View reviewed changes

straight-shoota force-pushed the jm/fix/uri-refactor branch from 0a1513e to dd2d8bc Compare August 15, 2018 12:56

RX14 reviewed Aug 17, 2018

View reviewed changes

src/uri.cr Show resolved Hide resolved

straight-shoota force-pushed the jm/fix/uri-refactor branch from 6cd19ee to ddda611 Compare August 19, 2018 20:20

RX14 approved these changes Aug 20, 2018

View reviewed changes

straight-shoota mentioned this pull request Sep 16, 2018

Using uri.path instead of String brakes HTTP::Client#get #6729

Closed

RX14 requested a review from bcardiff October 6, 2018 12:01

jkthorne reviewed Oct 9, 2018

View reviewed changes

src/uri.cr Show resolved Hide resolved

straight-shoota added kind:bug topic:stdlib kind:refactor breaking-change kind:specs labels Jan 7, 2019

straight-shoota added this to the 0.28.0 milestone Jan 7, 2019

Sija reviewed Jan 7, 2019

View reviewed changes

spec/std/uri_spec.cr Show resolved Hide resolved

Sija reviewed Jan 7, 2019

View reviewed changes

straight-shoota force-pushed the jm/fix/uri-refactor branch 2 times, most recently from 8f1be0b to a88776b Compare January 7, 2019 22:16

Sija reviewed Jan 8, 2019

View reviewed changes

src/uri.cr Show resolved Hide resolved

straight-shoota force-pushed the jm/fix/uri-refactor branch from a88776b to 19be7a1 Compare February 10, 2019 16:53

j8r reviewed Feb 12, 2019

View reviewed changes

src/uri.cr Show resolved Hide resolved

bcardiff approved these changes Mar 27, 2019

View reviewed changes

straight-shoota added 13 commits March 27, 2019 17:03

Refactor URI#path to be non-nilable

c488efe

Every URI has a path, but it may be empty. Making the ivar non-nilable avoids any ambiguity whether an empty path is represented by an empty string (`""`) or `nil`.

Refactor URI spec simplifying spec examples

6af4fcf

Remove URI#opaque

2aac648

It is not necessary to have an extra ivar for an opaque path, URI#path is sufficient and can hold the path of both opaque and hiearchicyl URIs. Adds URI#opaque? to tell whether the URI is considered opaque.

Refactor and add URI specs

2257692

Change URI.parse to downcase scheme

173619c

URI schemes are case-insensitive, this ensures that. This approach is recommended by RFC 3986.

Fix host with escape sequences in URI.parse/URI#to_s

dddfa6c

URI.prase decodes escape sequences in host, URI#to_s applies them where necessary. This is already some implicity normalization but results in the same URI, although in a differen string representation.

Fix URI.parse with empty port

e6cc855

An empty port was previously interpreted as port `0`. Now it's `nil`.

Fix URI.parse triple slash path

a05296a

An URI starting with three slashes (`///`) has an empty authority but it's still a hierarchical URI. `host` must be the empty string, not `nil`.

Add return types to URI methods

7e74b84

Refactor URI::Parser making helper methods private

b9acfe7

straight-shoota force-pushed the jm/fix/uri-refactor branch from 05e186b to b9acfe7 Compare March 27, 2019 16:07

bcardiff merged commit b2a1357 into crystal-lang:master Apr 5, 2019

straight-shoota deleted the jm/fix/uri-refactor branch April 5, 2019 17:07

bcardiff added topic:stdlib:networking and removed topic:stdlib labels Apr 11, 2019

straight-shoota mentioned this pull request Apr 12, 2019

[RFC] Deprecation tool #7655

Open

yorci mentioned this pull request Apr 19, 2019

Crystal 0.28.0 Adaptation icyleaf/halite#62

Merged

straight-shoota mentioned this pull request Nov 18, 2019

URI components are not unescaped when accessed #2098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and improve URI implementation #6323

Refactor and improve URI implementation #6323

straight-shoota commented Jul 3, 2018 •

edited

yxhuvud Jul 3, 2018

straight-shoota Jul 3, 2018

yxhuvud commented Jul 3, 2018

straight-shoota commented Jul 3, 2018

asterite commented Jul 3, 2018

asterite commented Jul 3, 2018

straight-shoota commented Jul 3, 2018 •

edited

straight-shoota commented Jul 3, 2018

asterite commented Jul 3, 2018

straight-shoota commented Oct 5, 2018

straight-shoota commented Nov 27, 2018

straight-shoota commented Jan 7, 2019

bararchy commented Jan 7, 2019

Sija Jan 7, 2019

straight-shoota Jan 7, 2019 •

edited

asterite commented Jan 8, 2019

straight-shoota commented Feb 11, 2019

jkthorne commented Feb 11, 2019

jkthorne commented Feb 12, 2019

straight-shoota commented Feb 12, 2019

bcardiff left a comment

Refactor and improve URI implementation #6323

Refactor and improve URI implementation #6323

Conversation

straight-shoota commented Jul 3, 2018 • edited

yxhuvud Jul 3, 2018

Choose a reason for hiding this comment

straight-shoota Jul 3, 2018

Choose a reason for hiding this comment

yxhuvud commented Jul 3, 2018

straight-shoota commented Jul 3, 2018

asterite commented Jul 3, 2018

asterite commented Jul 3, 2018

straight-shoota commented Jul 3, 2018 • edited

straight-shoota commented Jul 3, 2018

asterite commented Jul 3, 2018

straight-shoota commented Oct 5, 2018

straight-shoota commented Nov 27, 2018

straight-shoota commented Jan 7, 2019

bararchy commented Jan 7, 2019

Sija Jan 7, 2019

Choose a reason for hiding this comment

straight-shoota Jan 7, 2019 • edited

Choose a reason for hiding this comment

asterite commented Jan 8, 2019

straight-shoota commented Feb 11, 2019

jkthorne commented Feb 11, 2019

jkthorne commented Feb 12, 2019

straight-shoota commented Feb 12, 2019

bcardiff left a comment

Choose a reason for hiding this comment

straight-shoota commented Jul 3, 2018 •

edited

straight-shoota commented Jul 3, 2018 •

edited

straight-shoota Jan 7, 2019 •

edited