Tweak the semantic of various URL related methods of ActionDispatch::Request

8 files changed, 41 insertions, 122 deletions

diff --git a/actionpack/lib/action_controller/test_case.rb b/actionpack/lib/action_controller/test_case.rb
index 64d9bdab2a..7e0a833dfa 100644
--- a/actionpack/lib/action_controller/test_case.rb
+++ b/actionpack/lib/action_controller/test_case.rb
@@ -322,6 +322,8 @@ module ActionController
         @controller ||= klass.new rescue nil
       end
 
+      @request.env.delete('PATH_INFO')
+
       if @controller
         @controller.request = @request
         @controller.params = {}
@@ -333,15 +335,19 @@ module ActionController
       @request.remote_addr = '208.77.188.166' # example.com
     end
 
-    private
-      def build_request_uri(action, parameters)
-        unless @request.env['REQUEST_URI']
-          options = @controller.__send__(:url_options).merge(parameters)
-          options.update(:only_path => true, :action => action)
+  private
+    def build_request_uri(action, parameters)
+      unless @request.env["PATH_INFO"]
+        options = @controller.__send__(:url_options).merge(parameters)
+        options.update(:only_path => true, :action => action, :relative_url_root => nil)
+        rewriter = ActionController::UrlRewriter.new(@request, parameters)
 
-          url = ActionController::UrlRewriter.new(@request, parameters)
-          @request.request_uri = url.rewrite(@router, options)
-        end
+        url, query_string = rewriter.rewrite(@router, options).split("?", 2)
+
+        @request.env["SCRIPT_NAME"] = @controller.config.relative_url_root
+        @request.env["PATH_INFO"] = url
+        @request.env["QUERY_STRING"] = query_string || ""
       end
+    end
   end
 end
diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb
index 42ad19044d..e67d970a23 100644
--- a/actionpack/lib/action_dispatch/http/url.rb
+++ b/actionpack/lib/action_dispatch/http/url.rb
@@ -85,42 +85,23 @@ module ActionDispatch
         subdomains(tld_length).join('.')
       end
 
-      # Returns the query string, accounting for server idiosyncrasies.
+      # The query string must always be present
       def query_string
-        @env['QUERY_STRING'].present? ? @env['QUERY_STRING'] : (@env['REQUEST_URI'].to_s.split('?', 2)[1] || '')
+        @env['QUERY_STRING']
       end
 
       # Returns the request URI, accounting for server idiosyncrasies.
       # WEBrick includes the full URL. IIS leaves REQUEST_URI blank.
       def request_uri
-        if uri = @env['REQUEST_URI']
-          # Remove domain, which webrick puts into the request_uri.
-          (%r{^\w+\://[^/]+(/.*|$)$} =~ uri) ? $1 : uri
-        else
-          # Construct IIS missing REQUEST_URI from SCRIPT_NAME and PATH_INFO.
-          uri = @env['PATH_INFO'].to_s
-
-          if script_filename = @env['SCRIPT_NAME'].to_s.match(%r{[^/]+$})
-            uri = uri.sub(/#{script_filename}\//, '')
-          end
-
-          env_qs = @env['QUERY_STRING'].to_s
-          uri += "?#{env_qs}" unless env_qs.empty?
-
-          if uri.blank?
-            @env.delete('REQUEST_URI')
-          else
-            @env['REQUEST_URI'] = uri
-          end
-        end
+        uri = "#{@env["SCRIPT_NAME"]}#{@env["PATH_INFO"]}"
+        uri << "?#{@env["QUERY_STRING"]}" if @env["QUERY_STRING"].present?
+        uri
       end
 
       # Returns the interpreted \path to requested resource after all the installation
       # directory of this application was taken into account.
       def path
-        path = request_uri.to_s[/\A[^\?]*/]
-        path.sub!(/\A#{ActionController::Base.relative_url_root}/, '')
-        path
+        @env['PATH_INFO']
       end
 
     private
diff --git a/actionpack/lib/action_view/helpers/url_helper.rb b/actionpack/lib/action_view/helpers/url_helper.rb
index d9607c0095..148f2868e9 100644
--- a/actionpack/lib/action_view/helpers/url_helper.rb
+++ b/actionpack/lib/action_view/helpers/url_helper.rb
@@ -544,10 +544,11 @@ module ActionView
         # submitted url doesn't have any either.  This lets the function
         # work with things like ?order=asc
         if url_string.index("?")
-          request_uri = request.request_uri
+          request_uri = request.fullpath
         else
-          request_uri = request.request_uri.split('?').first
+          request_uri = request.path
         end
+
         if url_string =~ /^\w+:\/\//
           url_string == "#{request.protocol}#{request.host_with_port}#{request_uri}"
         else
diff --git a/actionpack/lib/action_view/test_case.rb b/actionpack/lib/action_view/test_case.rb
index f639700eaf..12142a3728 100644
--- a/actionpack/lib/action_view/test_case.rb
+++ b/actionpack/lib/action_view/test_case.rb
@@ -34,6 +34,8 @@ module ActionView
         @request = ActionController::TestRequest.new
         @response = ActionController::TestResponse.new
 
+        @request.env.delete('PATH_INFO')
+
         @params = {}
       end
     end
diff --git a/actionpack/test/controller/http_digest_authentication_test.rb b/actionpack/test/controller/http_digest_authentication_test.rb
index 22d2e23de1..6f167fe627 100644
--- a/actionpack/test/controller/http_digest_authentication_test.rb
+++ b/actionpack/test/controller/http_digest_authentication_test.rb
@@ -139,7 +139,7 @@ class HttpDigestAuthenticationTest < ActionController::TestCase
 
   test "authentication request with request-uri that doesn't match credentials digest-uri" do
     @request.env['HTTP_AUTHORIZATION'] = encode_credentials(:username => 'pretty', :password => 'please')
-    @request.env['REQUEST_URI'] = "/http_digest_authentication_test/dummy_digest/altered/uri"
+    @request.env['PATH_INFO'] = "/http_digest_authentication_test/dummy_digest/altered/uri"
     get :display
 
     assert_response :unauthorized
@@ -148,7 +148,8 @@ class HttpDigestAuthenticationTest < ActionController::TestCase
 
   test "authentication request with absolute request uri (as in webrick)" do
     @request.env['HTTP_AUTHORIZATION'] = encode_credentials(:username => 'pretty', :password => 'please')
-    @request.env['REQUEST_URI'] = "http://test.host/http_digest_authentication_test/dummy_digest"
+    @request.env["SERVER_NAME"] = "test.host"
+    @request.env['PATH_INFO'] = "/http_digest_authentication_test/dummy_digest"
 
     get :display
 
@@ -171,7 +172,8 @@ class HttpDigestAuthenticationTest < ActionController::TestCase
   test "authentication request with absolute uri in both request and credentials (as in Webrick with IE)" do
     @request.env['HTTP_AUTHORIZATION'] = encode_credentials(:url => "http://test.host/http_digest_authentication_test/dummy_digest",
                                                             :username => 'pretty', :password => 'please')
-    @request.env['REQUEST_URI'] = "http://test.host/http_digest_authentication_test/dummy_digest"
+    @request.env['SERVER_NAME'] = "test.host"
+    @request.env['PATH_INFO'] = "/http_digest_authentication_test/dummy_digest"
 
     get :display
 
@@ -226,7 +228,7 @@ class HttpDigestAuthenticationTest < ActionController::TestCase
 
     credentials = decode_credentials(@response.headers['WWW-Authenticate'])
     credentials.merge!(options)
-    credentials.merge!(:uri => @request.env['REQUEST_URI'].to_s)
+    credentials.merge!(:uri => @request.env['PATH_INFO'].to_s)
     ActionController::HttpAuthentication::Digest.encode_credentials(method, credentials, password, options[:password_is_ha1])
   end
 
diff --git a/actionpack/test/controller/integration_test.rb b/actionpack/test/controller/integration_test.rb
index c38348fa68..814699978d 100644
--- a/actionpack/test/controller/integration_test.rb
+++ b/actionpack/test/controller/integration_test.rb
@@ -346,8 +346,8 @@ class IntegrationProcessTest < ActionController::IntegrationTest
   def test_get_with_parameters
     with_test_route_set do
       get '/get_with_params', :foo => "bar"
-      assert_equal '/get_with_params', request.env["REQUEST_URI"]
-      assert_equal '/get_with_params', request.request_uri
+      assert_equal '/get_with_params', request.env["PATH_INFO"]
+      assert_equal '/get_with_params', request.path_info
       assert_equal 'foo=bar', request.env["QUERY_STRING"]
       assert_equal 'foo=bar', request.query_string
       assert_equal 'bar', request.parameters['foo']
diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb
index 703f03fa96..1dfcdaebfd 100644
--- a/actionpack/test/dispatch/request_test.rb
+++ b/actionpack/test/dispatch/request_test.rb
@@ -144,103 +144,33 @@ class RequestTest < ActiveSupport::TestCase
   end
 
   test "request uri" do
-    request = stub_request 'REQUEST_URI' => "http://www.rubyonrails.org/path/of/some/uri?mapped=1"
+    request = stub_request 'SCRIPT_NAME' => '', 'PATH_INFO' => '/path/of/some/uri', 'QUERY_STRING' => 'mapped=1'
     assert_equal "/path/of/some/uri?mapped=1", request.request_uri
     assert_equal "/path/of/some/uri",          request.path
 
-    request = stub_request 'REQUEST_URI' => "http://www.rubyonrails.org/path/of/some/uri"
+    request = stub_request 'SCRIPT_NAME' => '', 'PATH_INFO' => '/path/of/some/uri'
     assert_equal "/path/of/some/uri", request.request_uri
     assert_equal "/path/of/some/uri", request.path
 
-    request = stub_request 'REQUEST_URI' => "/path/of/some/uri"
-    assert_equal "/path/of/some/uri", request.request_uri
-    assert_equal "/path/of/some/uri", request.path
-
-    request = stub_request 'REQUEST_URI' => "/"
+    request = stub_request 'SCRIPT_NAME' => '', 'PATH_INFO' => '/'
     assert_equal "/", request.request_uri
     assert_equal "/", request.path
 
-    request = stub_request 'REQUEST_URI' => "/?m=b"
+    request = stub_request 'SCRIPT_NAME' => '', 'PATH_INFO' => '/', 'QUERY_STRING' => 'm=b'
     assert_equal "/?m=b", request.request_uri
     assert_equal "/",     request.path
 
-    request = stub_request 'REQUEST_URI' => "/", 'SCRIPT_NAME' => '/dispatch.cgi'
-    assert_equal "/", request.request_uri
-    assert_equal "/", request.path
-
-    ActionController::Base.relative_url_root = "/hieraki"
-    request = stub_request 'REQUEST_URI' => "/hieraki/", 'SCRIPT_NAME' => "/hieraki/dispatch.cgi"
+    request = stub_request 'SCRIPT_NAME' => '/hieraki', 'PATH_INFO' => '/'
     assert_equal "/hieraki/", request.request_uri
     assert_equal "/",         request.path
-    ActionController::Base.relative_url_root = nil
 
-    ActionController::Base.relative_url_root = "/collaboration/hieraki"
-    request = stub_request 'REQUEST_URI' => "/collaboration/hieraki/books/edit/2",
-      'SCRIPT_NAME' => "/collaboration/hieraki/dispatch.cgi"
+    request = stub_request 'SCRIPT_NAME' => '/collaboration/hieraki', 'PATH_INFO' => '/books/edit/2'
     assert_equal "/collaboration/hieraki/books/edit/2", request.request_uri
     assert_equal "/books/edit/2",                       request.path
-    ActionController::Base.relative_url_root = nil
-
-    # The following tests are for when REQUEST_URI is not supplied (as in IIS)
-    request = stub_request 'PATH_INFO'   => "/path/of/some/uri?mapped=1",
-                           'SCRIPT_NAME' => nil,
-                           'REQUEST_URI' => nil
-    assert_equal "/path/of/some/uri?mapped=1", request.request_uri
-    assert_equal "/path/of/some/uri",          request.path
 
-    ActionController::Base.relative_url_root = '/path'
-    request = stub_request 'PATH_INFO'   => "/path/of/some/uri?mapped=1",
-                           'SCRIPT_NAME' => "/path/dispatch.rb",
-                           'REQUEST_URI' => nil
+    request = stub_request 'SCRIPT_NAME' => '/path', 'PATH_INFO' => '/of/some/uri', 'QUERY_STRING' => 'mapped=1'
     assert_equal "/path/of/some/uri?mapped=1", request.request_uri
     assert_equal "/of/some/uri",               request.path
-    ActionController::Base.relative_url_root = nil
-
-    request = stub_request 'PATH_INFO'   => "/path/of/some/uri",
-                           'SCRIPT_NAME' => nil,
-                           'REQUEST_URI' => nil
-    assert_equal "/path/of/some/uri", request.request_uri
-    assert_equal "/path/of/some/uri", request.path
-
-    request = stub_request 'PATH_INFO' => '/', 'REQUEST_URI' => nil
-    assert_equal "/", request.request_uri
-    assert_equal "/", request.path
-
-    request = stub_request 'PATH_INFO' => '/?m=b', 'REQUEST_URI' => nil
-    assert_equal "/?m=b", request.request_uri
-    assert_equal "/",     request.path
-
-    request = stub_request 'PATH_INFO'   => "/",
-                           'SCRIPT_NAME' => "/dispatch.cgi",
-                           'REQUEST_URI' => nil
-    assert_equal "/", request.request_uri
-    assert_equal "/", request.path
-
-    ActionController::Base.relative_url_root = '/hieraki'
-    request = stub_request 'PATH_INFO'   => "/hieraki/",
-                           'SCRIPT_NAME' => "/hieraki/dispatch.cgi",
-                           'REQUEST_URI' => nil
-    assert_equal "/hieraki/", request.request_uri
-    assert_equal "/",         request.path
-    ActionController::Base.relative_url_root = nil
-
-    request = stub_request 'REQUEST_URI' => '/hieraki/dispatch.cgi'
-    ActionController::Base.relative_url_root = '/hieraki'
-    assert_equal "/dispatch.cgi", request.path
-    ActionController::Base.relative_url_root = nil
-
-    request = stub_request 'REQUEST_URI' => '/hieraki/dispatch.cgi'
-    ActionController::Base.relative_url_root = '/foo'
-    assert_equal "/hieraki/dispatch.cgi", request.path
-    ActionController::Base.relative_url_root = nil
-
-    # This test ensures that Rails uses REQUEST_URI over PATH_INFO
-    ActionController::Base.relative_url_root = nil
-    request = stub_request 'REQUEST_URI' => "/some/path",
-                           'PATH_INFO'   => "/another/path",
-                           'SCRIPT_NAME' => "/dispatch.cgi"
-    assert_equal "/some/path", request.request_uri
-    assert_equal "/some/path", request.path
   end
 
 
@@ -498,7 +428,7 @@ class RequestTest < ActiveSupport::TestCase
 
 protected
 
-  def stub_request(env={})
+  def stub_request(env = {})
     ActionDispatch::Request.new(env)
   end
 
diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb
index b047466aaf..afec78ddce 100644
--- a/actionpack/test/template/url_helper_test.rb
+++ b/actionpack/test/template/url_helper_test.rb
@@ -464,8 +464,6 @@ end
 class LinkToUnlessCurrentWithControllerTest < ActionController::TestCase
   def setup
     super
-    @request    = ActionController::TestRequest.new
-    @response   = ActionController::TestResponse.new
     @controller = TasksController.new
   end
 
@@ -565,7 +563,6 @@ end
 class PolymorphicControllerTest < ActionController::TestCase
   def setup
     super
-    @request  = ActionController::TestRequest.new
     @response = ActionController::TestResponse.new
   end