Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Do not mutate options in TryStatic.new #53

Merged
merged 3 commits into from

5 participants

@tkareine

If I have a following rackup file:

require 'rack'
require 'rack/contrib/try_static'

use Rack::ShowExceptions

map '/foo' do
  use Rack::TryStatic, :root => File.expand_path('public', File.dirname(__FILE__)), :urls => %w{/}, :try => %w{.html index.html /index.html}
end

run proc { [200, {'Content-Type' => 'text/html', 'Content-Length' => '3'}, ["404"]] }

And I run it with rackup, issuing the following requests:

  1. GET /foo
  2. GET /bar
  3. GET /foo

In step 1, TryStatic.new modifies the options, but returns the static file correctly. In step 3, the same options instance gets passed to TryStatic.new. Then, there is no :try key left, because it was removed in step 1. This causes the static file not be returned.

This patch changes TryStatic#initialize so that it does not change the options given to it.

Also, commit 04ced70 of the patch is possibly related to GH-48.

tkareine added some commits
@tkareine tkareine Add unused env parameter for lambdas (Ruby 1.9 compatibility)
Possibly related to GH-48
rack#48
04ced70
@tkareine tkareine Explicitly build options for tests, will be needed next d127328
@tkareine tkareine Do not mutate options given to TryStatic.new
Otherwise, given the following rackup file

----8<----clip----8<----
require 'rack'
require 'rack/contrib/try_static'

use Rack::ShowExceptions

map '/foo' do
  use Rack::TryStatic, :root => File.expand_path('public', File.dirname(__FILE__)), :urls => %w{/}, :try => %w{.html index.html /index.html}
end

run proc { [200, {'Content-Type' => 'text/html', 'Content-Length' => '3'}, ["404"]] }
----8<----clip----8<----

and the following sequence of requests against running it with
`rackup`:

1. GET /foo
2. GET /bar
3. GET /foo

Causes the options to be different in step 3 than in step 1. There
will be no :try option in step 3.
1c13e9e
@lgierth

Arguments shouldn't be modified, good catch!

@tkareine

maintainer ping

@maccman

Ping @tomayko

@jpmckinney

@rtomayko TryStatic is broken (at least in 1.9) and this pull request fixes it.

@jpmckinney jpmckinney referenced this pull request
Closed

TryStatic #48

@rkh rkh merged commit 84a4bdf into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 4, 2012
  1. @tkareine
  2. @tkareine
  3. @tkareine

    Do not mutate options given to TryStatic.new

    tkareine authored
    Otherwise, given the following rackup file
    
    ----8<----clip----8<----
    require 'rack'
    require 'rack/contrib/try_static'
    
    use Rack::ShowExceptions
    
    map '/foo' do
      use Rack::TryStatic, :root => File.expand_path('public', File.dirname(__FILE__)), :urls => %w{/}, :try => %w{.html index.html /index.html}
    end
    
    run proc { [200, {'Content-Type' => 'text/html', 'Content-Length' => '3'}, ["404"]] }
    ----8<----clip----8<----
    
    and the following sequence of requests against running it with
    `rackup`:
    
    1. GET /foo
    2. GET /bar
    3. GET /foo
    
    Causes the options to be different in step 3 than in step 1. There
    will be no :try option in step 3.
This page is out of date. Refresh to see the latest.
Showing with 21 additions and 9 deletions.
  1. +2 −2 lib/rack/contrib/try_static.rb
  2. +19 −7 test/spec_rack_try_static.rb
View
4 lib/rack/contrib/try_static.rb
@@ -17,9 +17,9 @@ class TryStatic
def initialize(app, options)
@app = app
- @try = ['', *options.delete(:try)]
+ @try = ['', *options[:try]]
@static = ::Rack::Static.new(
- lambda { [404, {}, []] },
+ lambda { |_| [404, {}, []] },
options)
end
View
26 test/spec_rack_try_static.rb
@@ -4,23 +4,25 @@
require 'rack/contrib/try_static'
require 'rack/mock'
-def request(options = {})
- options.merge!({
+def build_options(opts)
+ {
:urls => %w[/],
:root => ::File.expand_path(::File.dirname(__FILE__)),
- })
+ }.merge(opts)
+end
+def request(options = {})
@request =
Rack::MockRequest.new(
Rack::TryStatic.new(
- lambda {[200, {}, ["Hello World"]]},
+ lambda { |_| [200, {}, ["Hello World"]]},
options))
end
describe "Rack::TryStatic" do
context 'when file cannot be found' do
it 'should call call app' do
- res = request(:try => ['html']).get('/documents')
+ res = request(build_options(:try => ['html'])).get('/documents')
res.should.be.ok
res.body.should == "Hello World"
end
@@ -28,7 +30,7 @@ def request(options = {})
context 'when file can be found' do
it 'should serve first found' do
- res = request(:try => ['.html', '/index.html', '/index.htm']).get('/documents')
+ res = request(build_options(:try => ['.html', '/index.html', '/index.htm'])).get('/documents')
res.should.be.ok
res.body.strip.should == "index.html"
end
@@ -36,9 +38,19 @@ def request(options = {})
context 'when path_info maps directly to file' do
it 'should serve existing' do
- res = request(:try => ['/index.html']).get('/documents/existing.html')
+ res = request(build_options(:try => ['/index.html'])).get('/documents/existing.html')
res.should.be.ok
res.body.strip.should == "existing.html"
end
end
+
+ context 'when sharing options' do
+ it 'should not mutate given options' do
+ org_options = build_options :try => ['/index.html']
+ given_options = org_options.dup
+ request(given_options).get('/documents').should.be.ok
+ request(given_options).get('/documents').should.be.ok
+ given_options.should == org_options
+ end
+ end
end
Something went wrong with that request. Please try again.