Add support for non-ascii URIs

http.gemspec

@@ -23,7 +23,8 @@ Gem::Specification.new do |gem|
   gem.version       = HTTP::VERSION
 
   gem.add_runtime_dependency "http_parser.rb", "~> 0.6.0"
-  gem.add_runtime_dependency "http-form_data", "~> 1.0.0"
+  gem.add_runtime_dependency "http-form_data", "~> 1.0.1"
+  gem.add_runtime_dependency "addressable",    "~> 2.3"
 
   gem.add_development_dependency "bundler", "~> 1.0"
 end

lib/http/client.rb

@@ -1,9 +1,11 @@
 require "cgi"
 require "uri"
+
 require "http/form_data"
 require "http/options"
 require "http/connection"
 require "http/redirector"
+require "http/uri"
 
 module HTTP
   # Clients make requests and receive responses
@@ -14,6 +16,8 @@ class Client
     KEEP_ALIVE         = "Keep-Alive".freeze
     CLOSE              = "close".freeze
 
+    HTTP_OR_HTTPS_RE   = %r{^https?://}i
+
     attr_reader :default_options
 
     def initialize(default_options = {})
@@ -109,33 +113,25 @@ def verify_connection!(uri)
 
     # Strips out query/path to give us a consistent way of comparing hosts
     def base_host(uri)
-      base = uri.dup
-      base.query = nil
-      base.path = ""
-      base.to_s
+      uri.omit(:query, :path).to_s
     end
 
     # Merges query params if needed
+    #
+    # @param [#to_s] uri
+    # @return [URI]
     def make_request_uri(uri, options)
-      uri = normalize_uri uri
+      uri = uri.to_s
 
-      if options.params && !options.params.empty?
-        params    = CGI.parse(uri.query.to_s).merge(options.params || {})
-        uri.query = URI.encode_www_form params
+      if default_options.persistent? && uri !~ HTTP_OR_HTTPS_RE
+        uri = "#{default_options.persistent}#{uri}"
       end
 
-      uri
-    end
+      uri = HTTP::URI.parse uri
 
-    # Normalize URI
-    #
-    # @param [#to_s] uri
-    # @return [URI]
-    def normalize_uri(uri)
-      if default_options.persistent? && uri !~ /^http|https/
-        uri = URI("#{default_options.persistent}#{uri}")
-      else
-        uri = URI(uri.to_s)
+      if options.params && !options.params.empty?
+        params    = CGI.parse(uri.query.to_s).merge(options.params || {})
+        uri.query = ::URI.encode_www_form params
       end
 
       # Some proxies (seen on WEBRick) fail if URL has

lib/http/connection.rb

@@ -129,7 +129,7 @@ def expired?
     # @param (see #initialize)
     # @return [void]
     def start_tls(req, options)
-      return unless req.uri.is_a?(URI::HTTPS) && !req.using_proxy?
+      return unless req.uri.https? && !req.using_proxy?
 
       ssl_context = options[:ssl_context]
 

lib/http/request.rb

@@ -1,14 +1,18 @@
+require "forwardable"
+require "base64"
+require "time"
+
 require "http/errors"
 require "http/headers"
 require "http/request/caching"
 require "http/request/writer"
 require "http/version"
-require "base64"
-require "uri"
-require "time"
+require "http/uri"
 
 module HTTP
   class Request
+    extend Forwardable
+
     include HTTP::Headers::Mixin
 
     # The method given was not understood
@@ -70,7 +74,7 @@ def __method__(*args)
     # :nodoc:
     def initialize(verb, uri, headers = {}, proxy = {}, body = nil, version = "1.1") # rubocop:disable ParameterLists
       @verb   = verb.to_s.downcase.to_sym
-      @uri    = uri.is_a?(URI) ? uri : URI(uri.to_s)
+      @uri    = HTTP::URI.parse uri
       @scheme = @uri.scheme && @uri.scheme.to_s.downcase.to_sym
 
       fail(UnsupportedMethodError, "unknown method: #{verb}") unless METHODS.include?(@verb)
@@ -80,14 +84,13 @@ def initialize(verb, uri, headers = {}, proxy = {}, body = nil, version = "1.1")
 
       @headers = HTTP::Headers.coerce(headers || {})
 
-      @headers["Host"]        ||= default_host
+      @headers["Host"]        ||= default_host_header_value
       @headers["User-Agent"]  ||= USER_AGENT
     end
 
     # Returns new Request with updated uri
     def redirect(uri, verb = @verb)
-      uri = @uri.merge uri.to_s
-      req = self.class.new(verb, uri, headers, proxy, body, version)
+      req = self.class.new(verb, @uri.join(uri), headers, proxy, body, version)
       req["Host"] = req.uri.host
       req
     end
@@ -116,17 +119,17 @@ def include_proxy_authorization_header
 
     # Compute HTTP request header for direct or proxy request
     def request_header
-      "#{verb.to_s.upcase} #{path_for_request_header} HTTP/#{version}"
+      "#{verb.to_s.upcase} #{uri.normalize} HTTP/#{version}"
     end
 
     # Host for tcp socket
     def socket_host
-      using_proxy? ? proxy[:proxy_address] : uri.host
+      using_proxy? ? proxy[:proxy_address] : host
     end
 
     # Port for tcp socket
     def socket_port
-      using_proxy? ? proxy[:proxy_port] : uri.port
+      using_proxy? ? proxy[:proxy_port] : port
     end
 
     # @return [HTTP::Request::Caching]
@@ -136,32 +139,19 @@ def caching
 
     private
 
-    def path_for_request_header
-      if using_proxy?
-        uri
-      else
-        uri_path_with_query
-      end
-    end
-
-    def uri_path_with_query
-      path = uri_has_query? ? "#{uri.path}?#{uri.query}" : uri.path
-      path.empty? ? "/" : path
-    end
+    # @!attribute [r] host
+    #   @return [String]
+    def_delegator :@uri, :host
 
-    def uri_has_query?
-      uri.query && !uri.query.empty?
+    # @!attribute [r] port
+    #   @return [Fixnum]
+    def port
+      @uri.port || @uri.default_port
     end
 
-    # Default host (with port if needed) header value.
-    #
-    # @return [String]
-    def default_host
-      if PORTS[@scheme] == @uri.port
-        @uri.host
-      else
-        "#{@uri.host}:#{@uri.port}"
-      end
+    # @return [String] Default host (with port if needed) header value.
+    def default_host_header_value
+      PORTS[@scheme] != port ? "#{host}:#{port}" : host
     end
   end
 end

lib/http/uri.rb

@@ -0,0 +1,23 @@
+require "addressable/uri"
+
+module HTTP
+  class URI < Addressable::URI
+    # HTTP scheme
+    HTTP_SCHEME = "http".freeze
+
+    # HTTPS scheme
+    HTTPS_SCHEME = "https".freeze
+
+    # @return [True] if URI is HTTP
+    # @return [False] otherwise
+    def http?
+      HTTP_SCHEME == scheme
+    end
+
+    # @return [True] if URI is HTTPS
+    # @return [False] otherwise
+    def https?
+      HTTPS_SCHEME == scheme
+    end
+  end
+end

spec/lib/http/client_spec.rb

@@ -1,3 +1,5 @@
+# coding: utf-8
+
 require "support/http_handling_shared"
 require "support/dummy_server"
 require "support/ssl_helper"
@@ -9,15 +11,18 @@
 
   StubbedClient = Class.new(HTTP::Client) do
     def make_request(request, options)
-      stubs.fetch(request.uri.to_s) { super(request, options) }
+      stubs.fetch(request.uri) { super(request, options) }
     end
 
     def stubs
       @stubs ||= {}
     end
 
     def stub(stubs)
-      @stubs = stubs
+      @stubs = stubs.each_with_object({}) do |(k, v), o|
+        o[HTTP::URI.parse k] = v
+      end
+
       self
     end
   end
@@ -73,6 +78,23 @@ def simple_response(body, status = 200)
       expect { client.get("http://example.com/") }
         .to raise_error(HTTP::Redirector::TooManyRedirectsError)
     end
+
+    context "with non-ASCII URLs" do
+      it "theoretically works like a charm" do
+        client = StubbedClient.new(:follow => true).stub(
+          "http://example.com/"      => redirect_response("/könig"),
+          "http://example.com/könig" => simple_response("OK")
+        )
+
+        expect { client.get "http://example.com/könig" }.not_to raise_error
+      end
+
+      it "works like a charm in real world" do
+        url    = "http://git.io/jNeY"
+        client = HTTP.follow
+        expect(client.get(url).to_s).to include "support for non-ascii URIs"
+      end
+    end
   end
 
   describe "caching" do
@@ -154,6 +176,19 @@ def simple_response(body, status = 200)
   end
 
   describe "#request" do
+    context "with non-ASCII URLs" do
+      it "theoretically works like a charm" do
+        client =  described_class.new
+        expect { client.get "#{dummy.endpoint}/könig" }.not_to raise_error
+      end
+
+      it "works like a charm in real world" do
+        url     = "https://github.com/httprb/http.rb/pull/197/ö無"
+        client  = HTTP.follow
+        expect(client.get(url).to_s).to include "support for non-ascii URIs"
+      end
+    end
+
     context "with explicitly given `Host` header" do
       let(:headers) { {"Host" => "another.example.com"} }
       let(:client)  { described_class.new :headers => headers }

spec/lib/http/request_spec.rb

@@ -71,7 +71,7 @@
 
     subject(:redirected) { request.redirect "http://blog.example.com/" }
 
-    its(:uri)     { is_expected.to eq URI.parse "http://blog.example.com/" }
+    its(:uri)     { is_expected.to eq HTTP::URI.parse "http://blog.example.com/" }
 
     its(:verb)    { is_expected.to eq request.verb }
     its(:body)    { is_expected.to eq request.body }
@@ -84,7 +84,7 @@
     context "with schema-less absolute URL given" do
       subject(:redirected) { request.redirect "//another.example.com/blog" }
 
-      its(:uri)     { is_expected.to eq URI.parse "http://another.example.com/blog" }
+      its(:uri)     { is_expected.to eq HTTP::URI.parse "http://another.example.com/blog" }
 
       its(:verb)    { is_expected.to eq request.verb }
       its(:body)    { is_expected.to eq request.body }
@@ -98,7 +98,7 @@
     context "with relative URL given" do
       subject(:redirected) { request.redirect "/blog" }
 
-      its(:uri)     { is_expected.to eq URI.parse "http://example.com/blog" }
+      its(:uri)     { is_expected.to eq HTTP::URI.parse "http://example.com/blog" }
 
       its(:verb)    { is_expected.to eq request.verb }
       its(:body)    { is_expected.to eq request.body }
@@ -110,14 +110,14 @@
 
       context "with original URI having non-standard port" do
         let(:request) { HTTP::Request.new(:post, "http://example.com:8080/", headers, proxy, body) }
-        its(:uri)     { is_expected.to eq URI.parse "http://example.com:8080/blog" }
+        its(:uri)     { is_expected.to eq HTTP::URI.parse "http://example.com:8080/blog" }
       end
     end
 
     context "with relative URL that misses leading slash given" do
       subject(:redirected) { request.redirect "blog" }
 
-      its(:uri)     { is_expected.to eq URI.parse "http://example.com/blog" }
+      its(:uri)     { is_expected.to eq HTTP::URI.parse "http://example.com/blog" }
 
       its(:verb)    { is_expected.to eq request.verb }
       its(:body)    { is_expected.to eq request.body }
@@ -129,7 +129,7 @@
 
       context "with original URI having non-standard port" do
         let(:request) { HTTP::Request.new(:post, "http://example.com:8080/", headers, proxy, body) }
-        its(:uri)     { is_expected.to eq URI.parse "http://example.com:8080/blog" }
+        its(:uri)     { is_expected.to eq HTTP::URI.parse "http://example.com:8080/blog" }
       end
     end
 

spec/lib/http_spec.rb

@@ -14,7 +14,7 @@
 
     context "with URI instance" do
       it "is easy" do
-        response = HTTP.get URI dummy.endpoint
+        response = HTTP.get HTTP::URI.parse dummy.endpoint
         expect(response.to_s).to match(/<!doctype html>/)
       end
     end