From cfaaacd9763642e91761de54c90669a88d772e5a Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 11 Aug 2014 13:29:25 -0500 Subject: Enable gzip compression by default If someone is using ActionDispatch::Static to serve assets and makes it past the `match?` then the file exists on disk and it will be served. This PR adds in logic that checks to see if the file being served is already compressed (via gzip) and on disk, if it is it will be served as long as the client can handle gzip encoding. If not, then a non gzip file will be served. This additional logic slows down an individual asset request but should speed up the consumer experience as compressed files are served and production applications should be delivered with a CDN. This PR allows a CDN to cache a gzip file by setting the `Vary` header appropriately. In net this should speed up a production application that are using Rails as an origin for a CDN. Non-asset request speed is not affected in this PR. --- actionpack/test/dispatch/static_test.rb | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) (limited to 'actionpack/test/dispatch') diff --git a/actionpack/test/dispatch/static_test.rb b/actionpack/test/dispatch/static_test.rb index afdda70748..f4b3a8cb93 100644 --- a/actionpack/test/dispatch/static_test.rb +++ b/actionpack/test/dispatch/static_test.rb @@ -1,6 +1,7 @@ # encoding: utf-8 require 'abstract_unit' require 'rbconfig' +require 'zlib' module StaticTests def test_serves_dynamic_content @@ -106,6 +107,18 @@ module StaticTests end end + def test_serves_gzip_files_when_header_set + file_name = "/gzip/application-a71b3024f80aea3181c09774ca17e712.js" + response = get(file_name, 'HTTP_ACCEPT_ENCODING' => 'gzip') + assert_gzip file_name, response + assert_equal 'application/javascript', response.headers['Content-Type'] + assert_equal 'Accept-Encoding', response.headers["Vary"] + assert_equal 'gzip', response.headers["Content-Encoding"] + + response = get(file_name, 'HTTP_ACCEPT_ENCODING' => '') + refute_equal 'gzip', response.headers["Content-Encoding"] + end + # Windows doesn't allow \ / : * ? " < > | in filenames unless RbConfig::CONFIG['host_os'] =~ /mswin|mingw/ def test_serves_static_file_with_colon @@ -125,13 +138,20 @@ module StaticTests private + def assert_gzip(file_name, response) + expected = File.read("#{FIXTURE_LOAD_PATH}/#{public_path}" + file_name) + actual = Zlib::GzipReader.new(StringIO.new(response.body)).read + assert_equal expected, actual + end + def assert_html(body, response) assert_equal body, response.body assert_equal "text/html", response.headers["Content-Type"] + refute response.headers.key?("Vary") end - def get(path) - Rack::MockRequest.new(@app).request("GET", path) + def get(path, headers = {}) + Rack::MockRequest.new(@app).request("GET", path, headers) end def with_static_file(file) -- cgit v1.2.3 From d3eb92d95a694905a80668dcde1fb49e2d08f388 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Thu, 3 Jul 2014 21:32:14 -0700 Subject: Avoid duplicating routes for HEAD requests. Follow up to rails#15321 Instead of duplicating the routes, we will first match the HEAD request to HEAD routes. If no match is found, we will then map the HEAD request to GET routes. --- actionpack/test/dispatch/request_test.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'actionpack/test/dispatch') diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb index 84bd392fd9..00b80c7357 100644 --- a/actionpack/test/dispatch/request_test.rb +++ b/actionpack/test/dispatch/request_test.rb @@ -650,6 +650,18 @@ class RequestMethod < BaseRequestTest end end + test "allow request method hacking" do + request = stub_request('REQUEST_METHOD' => 'POST') + + assert_equal 'POST', request.request_method + assert_equal 'POST', request.env["REQUEST_METHOD"] + + request.request_method = 'GET' + + assert_equal 'GET', request.request_method + assert_equal 'GET', request.env["REQUEST_METHOD"] + end + test "invalid http method raises exception" do assert_raise(ActionController::UnknownHttpMethod) do stub_request('REQUEST_METHOD' => 'RANDOM_METHOD').request_method -- cgit v1.2.3 From 8e31fa3b72892f7421b85b5a79d71a2d726ccddd Mon Sep 17 00:00:00 2001 From: schneems Date: Thu, 21 Aug 2014 11:52:25 -0500 Subject: Address comments on Gzip implementation - don't mutate PATH_INFO in env, test - test fallback content type matches Rack::File - change assertion style - make HTTP_ACCEPT_ENCODING comparison case insensitive - return gzip path from method instead of true/false so we don't have to assume later - don't allocate un-needed hash. Original comments: https://github.com/rails/rails/commit/ cfaaacd9763642e91761de54c90669a88d772e5a#commitcomment-7468728 cc @jeremy --- actionpack/test/dispatch/static_test.rb | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) (limited to 'actionpack/test/dispatch') diff --git a/actionpack/test/dispatch/static_test.rb b/actionpack/test/dispatch/static_test.rb index f4b3a8cb93..97affc7b21 100644 --- a/actionpack/test/dispatch/static_test.rb +++ b/actionpack/test/dispatch/static_test.rb @@ -115,8 +115,30 @@ module StaticTests assert_equal 'Accept-Encoding', response.headers["Vary"] assert_equal 'gzip', response.headers["Content-Encoding"] + response = get(file_name, 'HTTP_ACCEPT_ENCODING' => 'Gzip') + assert_gzip file_name, response + + response = get(file_name, 'HTTP_ACCEPT_ENCODING' => 'GZIP') + assert_gzip file_name, response + response = get(file_name, 'HTTP_ACCEPT_ENCODING' => '') - refute_equal 'gzip', response.headers["Content-Encoding"] + assert_not_equal 'gzip', response.headers["Content-Encoding"] + end + + def test_does_not_modify_path_info + file_name = "/gzip/application-a71b3024f80aea3181c09774ca17e712.js" + env = {'PATH_INFO' => file_name, 'HTTP_ACCEPT_ENCODING' => 'gzip'} + @app.call(env) + assert_equal file_name, env['PATH_INFO'] + end + + def test_serves_gzip_with_propper_content_type_fallback + file_name = "/gzip/foo.zoo" + response = get(file_name, 'HTTP_ACCEPT_ENCODING' => 'gzip') + assert_gzip file_name, response + + default_response = get(file_name) # no gzip + assert_equal default_response.headers['Content-Type'], response.headers['Content-Type'] end # Windows doesn't allow \ / : * ? " < > | in filenames @@ -147,7 +169,7 @@ module StaticTests def assert_html(body, response) assert_equal body, response.body assert_equal "text/html", response.headers["Content-Type"] - refute response.headers.key?("Vary") + assert_nil response.headers["Vary"] end def get(path, headers = {}) -- cgit v1.2.3 From d78f3f0ec33d57e78249d5b96f38401a1c239410 Mon Sep 17 00:00:00 2001 From: Agis- Date: Fri, 22 Aug 2014 16:52:48 +0300 Subject: Don't ignore constraints in redirect routes https://github.com/rails/rails/commit/402c2af55053c2f29319091ad21fd6fa6b90ee89 introduced a regression that caused any constraints added to redirect routes to be ignored. Fixes #16605 --- actionpack/test/dispatch/routing_test.rb | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) (limited to 'actionpack/test/dispatch') diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index b8e20c52a0..d141210ad9 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -14,6 +14,12 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest end end + class GrumpyRestrictor + def self.matches?(request) + false + end + end + class YoutubeFavoritesRedirector def self.call(params, request) "http://www.youtube.com/watch?v=#{params[:youtube_id]}" @@ -89,6 +95,24 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest verify_redirect 'http://www.example.com/private/index' end + def test_redirect_with_failing_constraint + draw do + get 'hi', to: redirect("/foo"), constraints: ::TestRoutingMapper::GrumpyRestrictor + end + + get '/hi' + assert_equal 404, status + end + + def test_redirect_with_passing_constraint + draw do + get 'hi', to: redirect("/foo"), constraints: ->(req) { true } + end + + get '/hi' + assert_equal 301, status + end + def test_namespace_with_controller_segment assert_raise(ArgumentError) do draw do -- cgit v1.2.3 From 03e660e768fdd05474471b5718d278c123a8d75c Mon Sep 17 00:00:00 2001 From: Peter Suschlik Date: Wed, 27 Aug 2014 11:34:17 +0200 Subject: Use less iterations for KeyGenerator in tests This commit improves performance of cookie tests: Ruby | After | Before ----- | --------:| --------: MRI | 5.03s | 9.28s JRuby | 25.45s | 1648.23s Please note the improvement for JRuby. --- actionpack/test/dispatch/cookies_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'actionpack/test/dispatch') diff --git a/actionpack/test/dispatch/cookies_test.rb b/actionpack/test/dispatch/cookies_test.rb index f62e194ca9..7dc6c37522 100644 --- a/actionpack/test/dispatch/cookies_test.rb +++ b/actionpack/test/dispatch/cookies_test.rb @@ -206,7 +206,7 @@ class CookiesTest < ActionController::TestCase def setup super - @request.env["action_dispatch.key_generator"] = ActiveSupport::KeyGenerator.new("b3c631c314c0bbca50c1b2843150fe33") + @request.env["action_dispatch.key_generator"] = ActiveSupport::KeyGenerator.new("b3c631c314c0bbca50c1b2843150fe33", iterations: 2) @request.env["action_dispatch.signed_cookie_salt"] = "b3c631c314c0bbca50c1b2843150fe33" @request.env["action_dispatch.encrypted_cookie_salt"] = "b3c631c314c0bbca50c1b2843150fe33" @request.env["action_dispatch.encrypted_signed_cookie_salt"] = "b3c631c314c0bbca50c1b2843150fe33" -- cgit v1.2.3 From 0b1a87f73cca3da23b65f3dfb19daeac436ab2ee Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 18 Aug 2014 11:20:06 -0500 Subject: Refactor out Dir.glob from ActionDispatch::Static Dir.glob can be a security concern. The original use was to provide logic of fallback files. Example a request to `/` should render the file from `/public/index.html`. We can replace the dir glob with the specific logic it represents. The glob {,index,index.html} will look for the current path, then in the directory of the path with index file and then in the directory of the path with index.html. This PR replaces the glob logic by manually checking each potential match. Best case scenario this results in one less file API request, worst case, this has one more file API request. Related to #16464 Update: added a test for when a file of a given name (`public/bar.html` and a directory `public/bar` both exist in the same root directory. Changed logic to accommodate this scenario. --- actionpack/test/dispatch/static_test.rb | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'actionpack/test/dispatch') diff --git a/actionpack/test/dispatch/static_test.rb b/actionpack/test/dispatch/static_test.rb index 97affc7b21..6f7373201c 100644 --- a/actionpack/test/dispatch/static_test.rb +++ b/actionpack/test/dispatch/static_test.rb @@ -37,6 +37,10 @@ module StaticTests assert_html "/foo/index.html", get("/foo") end + def test_serves_file_with_same_name_before_index_in_directory + assert_html "/bar.html", get("/bar") + end + def test_served_static_file_with_non_english_filename jruby_skip "Stop skipping if following bug gets fixed: " \ "http://jira.codehaus.org/browse/JRUBY-7192" -- cgit v1.2.3