From 8e31fa3b72892f7421b85b5a79d71a2d726ccddd Mon Sep 17 00:00:00 2001 From: schneems Date: Thu, 21 Aug 2014 11:52:25 -0500 Subject: [PATCH] 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 --- .../lib/action_dispatch/middleware/static.rb | 29 ++++++++++++------ actionpack/test/dispatch/static_test.rb | 26 ++++++++++++++-- actionpack/test/fixtures/public/gzip/foo.zoo | 4 +++ .../test/fixtures/public/gzip/foo.zoo.gz | Bin 0 -> 38457 bytes .../\345\205\254\345\205\261/gzip/foo.zoo" | 4 +++ .../\345\205\254\345\205\261/gzip/foo.zoo.gz" | Bin 0 -> 38457 bytes guides/source/4_2_release_notes.md | 7 +++++ 7 files changed, 58 insertions(+), 12 deletions(-) create mode 100644 actionpack/test/fixtures/public/gzip/foo.zoo create mode 100644 actionpack/test/fixtures/public/gzip/foo.zoo.gz create mode 100644 "actionpack/test/fixtures/\345\205\254\345\205\261/gzip/foo.zoo" create mode 100644 "actionpack/test/fixtures/\345\205\254\345\205\261/gzip/foo.zoo.gz" diff --git a/actionpack/lib/action_dispatch/middleware/static.rb b/actionpack/lib/action_dispatch/middleware/static.rb index a3e8126ace..0733132277 100644 --- a/actionpack/lib/action_dispatch/middleware/static.rb +++ b/actionpack/lib/action_dispatch/middleware/static.rb @@ -16,8 +16,7 @@ class FileHandler def initialize(root, cache_control) @root = root.chomp('/') @compiled_root = /^#{Regexp.escape(root)}/ - headers = {} - headers['Cache-Control'] = cache_control if cache_control + headers = cache_control && { 'Cache-Control' => cache_control } @file_server = ::Rack::File.new(@root, headers) end @@ -37,10 +36,11 @@ def match?(path) end def call(env) - path = env['PATH_INFO'] - gzip_file_exists = gzip_file_exists?(path) - if gzip_file_exists && gzip_encoding_accepted?(env) - env['PATH_INFO'] = "#{path}.gz" + path = env['PATH_INFO'] + gzip_path = gzip_file_path(path) + + if gzip_path && gzip_encoding_accepted?(env) + env['PATH_INFO'] = gzip_path status, headers, body = @file_server.call(env) headers['Content-Encoding'] = 'gzip' headers['Content-Type'] = content_type(path) @@ -48,8 +48,11 @@ def call(env) status, headers, body = @file_server.call(env) end - headers['Vary'] = 'Accept-Encoding' if gzip_file_exists + headers['Vary'] = 'Accept-Encoding' if gzip_path + return [status, headers, body] + ensure + env['PATH_INFO'] = path end private @@ -73,11 +76,17 @@ def content_type(path) end def gzip_encoding_accepted?(env) - env['HTTP_ACCEPT_ENCODING'] =~ /\bgzip\b/ + env['HTTP_ACCEPT_ENCODING'] =~ /\bgzip\b/i end - def gzip_file_exists?(path) - File.exist?(File.join(@root, "#{::Rack::Utils.unescape(path)}.gz")) + def gzip_file_path(path) + can_gzip_mime = content_type(path) =~ /\A(?:text\/|application\/javascript)/ + gzip_path = "#{path}.gz" + if can_gzip_mime && File.exist?(File.join(@root, ::Rack::Utils.unescape(gzip_path))) + gzip_path + else + false + end end end 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 @@ def test_serves_gzip_files_when_header_set 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 @@ def assert_gzip(file_name, response) 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 = {}) diff --git a/actionpack/test/fixtures/public/gzip/foo.zoo b/actionpack/test/fixtures/public/gzip/foo.zoo new file mode 100644 index 0000000000..1826a7660e --- /dev/null +++ b/actionpack/test/fixtures/public/gzip/foo.zoo @@ -0,0 +1,4 @@ +!function(e,t){"object"==typeof module&&"object"==typeof module.exports?module.exports=e.document?t(e,!0):function(e){if(!e.document)throw new Error("jQuery requires a window with a document");return t(e)}:t(e)}("undefined"!=typeof window?window:this,function(e,t){function n(e){var t=e.length,n=it.type(e);return"function"===n||it.isWindow(e)?!1:1===e.nodeType&&t?!0:"array"===n||0===t||"number"==typeof t&&t>0&&t-1 in e}function r(e,t,n){if(it.isFunction(t))return it.grep(e,function(e,r){return!!t.call(e,r,e)!==n});if(t.nodeType)return it.grep(e,function(e){return e===t!==n});if("string"==typeof t){if(ft.test(t))return it.filter(t,e,n);t=it.filter(t,e)}return it.grep(e,function(e){return it.inArray(e,t)>=0!==n})}function i(e,t){do e=e[t];while(e&&1!==e.nodeType);return e}function o(e){var t=xt[e]={};return it.each(e.match(bt)||[],function(e,n){t[n]=!0}),t}function a(){ht.addEventListener?(ht.removeEventListener("DOMContentLoaded",s,!1),e.removeEventListener("load",s,!1)):(ht.detachEvent("onreadystatechange",s),e.detachEvent("onload",s))}function s(){(ht.addEventListener||"load"===event.type||"complete"===ht.readyState)&&(a(),it.ready())}function l(e,t,n){if(void 0===n&&1===e.nodeType){var r="data-"+t.replace(Ct,"-$1").toLowerCase();if(n=e.getAttribute(r),"string"==typeof n){try{n="true"===n?!0:"false"===n?!1:"null"===n?null:+n+""===n?+n:kt.test(n)?it.parseJSON(n):n}catch(i){}it.data(e,t,n)}else n=void 0}return n}function u(e){var t;for(t in e)if(("data"!==t||!it.isEmptyObject(e[t]))&&"toJSON"!==t)return!1;return!0}function c(e,t,n,r){if(it.acceptData(e)){var i,o,a=it.expando,s=e.nodeType,l=s?it.cache:e,u=s?e[a]:e[a]&&a;if(u&&l[u]&&(r||l[u].data)||void 0!==n||"string"!=typeof t)return u||(u=s?e[a]=G.pop()||it.guid++:a),l[u]||(l[u]=s?{}:{toJSON:it.noop}),("object"==typeof t||"function"==typeof t)&&(r?l[u]=it.extend(l[u],t):l[u].data=it.extend(l[u].data,t)),o=l[u],r||(o.data||(o.data={}),o=o.data),void 0!==n&&(o[it.camelCase(t)]=n),"string"==typeof t?(i=o[t],null==i&&(i=o[it.camelCase(t)])):i=o,i}}function d(e,t,n){if(it.acceptData(e)){var r,i,o=e.nodeType,a=o?it.cache:e,s=o?e[it.expando]:it.expando;if(a[s]){if(t&&(r=n?a[s]:a[s].data)){it.isArray(t)?t=t.concat(it.map(t,it.camelCase)):t in r?t=[t]:(t=it.camelCase(t),t=t in r?[t]:t.split(" ")),i=t.length;for(;i--;)delete r[t[i]];if(n?!u(r):!it.isEmptyObject(r))return}(n||(delete a[s].data,u(a[s])))&&(o?it.cleanData([e],!0):nt.deleteExpando||a!=a.window?delete a[s]:a[s]=null)}}}function f(){return!0}function p(){return!1}function h(){try{return ht.activeElement}catch(e){}}function m(e){var t=Mt.split("|"),n=e.createDocumentFragment();if(n.createElement)for(;t.length;)n.createElement(t.pop());return n}function g(e,t){var n,r,i=0,o=typeof e.getElementsByTagName!==Et?e.getElementsByTagName(t||"*"):typeof e.querySelectorAll!==Et?e.querySelectorAll(t||"*"):void 0;if(!o)for(o=[],n=e.childNodes||e;null!=(r=n[i]);i++)!t||it.nodeName(r,t)?o.push(r):it.merge(o,g(r,t));return void 0===t||t&&it.nodeName(e,t)?it.merge([e],o):o}function v(e){Dt.test(e.type)&&(e.defaultChecked=e.checked)}function y(e,t){return it.nodeName(e,"table")&&it.nodeName(11!==t.nodeType?t:t.firstChild,"tr")?e.getElementsByTagName("tbody")[0]||e.appendChild(e.ownerDocument.createElement("tbody")):e}function b(e){return e.type=(null!==it.find.attr(e,"type"))+"/"+e.type,e}function x(e){var t=Vt.exec(e.type);return t?e.type=t[1]:e.removeAttribute("type"),e}function w(e,t){for(var n,r=0;null!=(n=e[r]);r++)it._data(n,"globalEval",!t||it._data(t[r],"globalEval"))}function T(e,t){if(1===t.nodeType&&it.hasData(e)){var n,r,i,o=it._data(e),a=it._data(t,o),s=o.events;if(s){delete a.handle,a.events={};for(n in s)for(r=0,i=s[n].length;i>r;r++)it.event.add(t,n,s[n][r])}a.data&&(a.data=it.extend({},a.data))}}function E(e,t){var n,r,i;if(1===t.nodeType){if(n=t.nodeName.toLowerCase(),!nt.noCloneEvent&&t[it.expando]){i=it._data(t);for(r in i.events)it.removeEvent(t,r,i.handle);t.removeAttribute(it.expando)}"script"===n&&t.text!==e.text?(b(t).text=e.text,x(t)):"object"===n?(t.parentNode&&(t.outerHTML=e.outerHTML),nt.html5Clone&&e.innerHTML&&!it.trim(t.innerHTML)&&(t.innerHTML=e.innerHTML)):"input"===n&&Dt.test(e.type)?(t.defaultChecked=t.checked=e.checked,t.value!==e.value&&(t.value=e.value)):"option"===n?t.defaultSelected=t.selected=e.defaultSelected:("input"===n||"textarea"===n)&&(t.defaultValue=e.defaultValue)}}function k(t,n){var r,i=it(n.createElement(t)).appendTo(n.body),o=e.getDefaultComputedStyle&&(r=e.getDefaultComputedStyle(i[0]))?r.display:it.css(i[0],"display");return i.detach(),o}function C(e){var t=ht,n=Zt[e];return n||(n=k(e,t),"none"!==n&&n||(Qt=(Qt||it("