Skip to content

Commit

Permalink
Simplify Rack 3 support using Rack::Headers
Browse files Browse the repository at this point in the history
The `Rack::Headers` subclass of `Hash` was added in Rack 3, and is meant
to help libraries support both Rack 2 and 3. I was concerned whether
returning `Rack::Headers` instead of `Hash` to the end user was
backwards compatible, but it seems that both Roda and Rack are doing it,
so it should be safe.

We remove Appraisal and for now let Rack 2 be tested implicitly with
Ruby 2.3, as Rack 3 requires Ruby 2.4+. But I don't think we need to be
testing Rack 2.x support, especially now that we've removed duplication,
I would be surprised if it breaks. With `Rack::Headers` we were able to
keep Ruby 2.3 support, as we're not using `Hash#transform_keys` anymore.

I also switched from rack-test_app, which seems to be unmaintained, back
to rack-test, which is the established library for testing Rack apps.
Ever since Jeremy Evans took over maintenance, it's gotten a lot better
and more flexible, so it's completely viable for Shrine's needs. Since
rack-test returns `Rack::Headers`, we were able to remove the header
constants.
  • Loading branch information
janko committed Apr 28, 2024
1 parent 5231c36 commit 54e51c8
Show file tree
Hide file tree
Showing 19 changed files with 314 additions and 421 deletions.
8 changes: 3 additions & 5 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ jobs:
fail-fast: false
matrix:
ruby:
- "2.3"
- "2.4"
- "2.5"
- "2.6"
Expand All @@ -33,9 +34,6 @@ jobs:
- "3.2"
- "3.3"
- "jruby-9.4"
rack:
- "2"
- "3"

steps:
- uses: actions/checkout@v3
Expand All @@ -55,7 +53,7 @@ jobs:
${GITHUB_WORKSPACE}/minio/minio server ${GITHUB_WORKSPACE}/minio/data --address localhost:9000 &>${GITHUB_WORKSPACE}/minio/data/server.log &
- name: Install dependencies
run: bundle exec appraisal install
run: bundle install

- name: Run tests
run: bundle exec appraisal rack-${{ matrix.rack }} rake test
run: bundle exec rake test
7 changes: 0 additions & 7 deletions Appraisals

This file was deleted.

2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ gem "simplecov"

gem "hanna", require: false

gem "activerecord-jdbcsqlite3-adapter", "~> 51.0", platform: :jruby if RUBY_ENGINE == "jruby"
gem "activerecord-jdbcsqlite3-adapter", "~> 70.0", platform: :jruby if RUBY_ENGINE == "jruby"
10 changes: 0 additions & 10 deletions gemfiles/rack_2.gemfile

This file was deleted.

10 changes: 0 additions & 10 deletions gemfiles/rack_3.gemfile

This file was deleted.

60 changes: 18 additions & 42 deletions lib/shrine/plugins/derivation_endpoint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -365,16 +365,9 @@ def call(env)
handle_request(request)
end

headers ||= {}

if Rack.release >= "3"
headers["content-length"] ||= body.respond_to?(:bytesize) ? body.bytesize.to_s :
body.map(&:bytesize).inject(0, :+).to_s
else
headers["Content-Length"] ||= body.map(&:bytesize).inject(0, :+).to_s
end

headers = headers.transform_keys(&:downcase) if Rack.release >= "3"
headers = Rack::Headers[headers]
headers["Content-Length"] ||= body.respond_to?(:bytesize) ? body.bytesize.to_s :
body.map(&:bytesize).inject(0, :+).to_s

[status, headers, body]
end
Expand Down Expand Up @@ -420,8 +413,6 @@ def handle_request(request)
headers["Cache-Control"] = derivation.option(:cache_control)
end

headers = headers.transform_keys(&:downcase) if Rack.release >= "3"

[status, headers, body]
end

Expand Down Expand Up @@ -455,11 +446,7 @@ def expires_in(request)

# Halts the request with the error message.
def error!(status, message)
headers = { "Content-Type" => "text/plain" }

headers = headers.transform_keys(&:downcase) if Rack.release >= "3"

throw :halt, [status, headers, [message]]
throw :halt, [status, { "Content-Type" => "text/plain" }, [message]]
end

def secret_key
Expand Down Expand Up @@ -496,33 +483,20 @@ def local_response(env)
# `Content-Type` and `Content-Disposition` response headers from derivation
# options and file extension of the derivation result.
def file_response(file, env)
response = rack_file_response(file.path, env)

status = response[0]

headers = if Rack.release >= "3"
{
"content-type" => type || response[1]["content-type"],
"content-length" => response[1]["content-length"],
"content-disposition" => content_disposition(file),
"content-range" => response[1]["content-range"],
"accept-ranges" => "bytes",
}.compact
else
{
"Content-Type" => type || response[1]["Content-Type"],
"Content-Length" => response[1]["Content-Length"],
"Content-Disposition" => content_disposition(file),
"Content-Range" => response[1]["Content-Range"],
"Accept-Ranges" => "bytes",
}.compact
end
status, headers, body = rack_file_response(file.path, env)

body = Rack::BodyProxy.new(response[2]) { File.delete(file.path) }
headers = Rack::Headers[headers] if Rack.release >= "3"
headers = {
"Content-Type" => type || headers["Content-Type"],
"Content-Length" => headers["Content-Length"],
"Content-Disposition" => content_disposition(file),
"Content-Range" => headers["Content-Range"],
"Accept-Ranges" => "bytes",
}.compact

file.close
body = Rack::BodyProxy.new(body) { File.delete(file.path) }

headers = headers.transform_keys(&:downcase) if Rack.release >= "3"
file.close

[status, headers, body]
end
Expand All @@ -541,8 +515,10 @@ def upload_response(env)

if upload_redirect
redirect_url = uploaded_file.url(**upload_redirect_url_options)
headers = { "Location" => redirect_url }
headers = Rack::Headers[headers] if Rack.release >= "3"

[302, { "Location" => redirect_url }, []]
[302, headers, []]
else
if derivative && File.exist?(derivative.path)
file_response(derivative, env)
Expand Down
17 changes: 4 additions & 13 deletions lib/shrine/plugins/download_endpoint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -113,14 +113,9 @@ def call(env)
handle_request(request)
end

if Rack.release >= "3"
headers["content-length"] ||= body.respond_to?(:bytesize) ? body.bytesize.to_s :
body.map(&:bytesize).inject(0, :+).to_s
else
headers["Content-Length"] ||= body.map(&:bytesize).inject(0, :+).to_s
end

headers = headers.transform_keys(&:downcase) if Rack.release >= "3"
headers = Rack::Headers[headers] if Rack.release >= "3"
headers["Content-Length"] ||= body.respond_to?(:bytesize) ? body.bytesize.to_s :
body.map(&:bytesize).inject(0, :+).to_s

[status, headers, body]
end
Expand Down Expand Up @@ -204,11 +199,7 @@ def bad_request!(message)

# Halts the request with the error message.
def error!(status, message)
headers = { "Content-Type" => "text/plain" }

headers = headers.transform_keys(&:downcase) if Rack.release >= "3"

throw :halt, [status, headers, [message]]
throw :halt, [status, { "Content-Type" => "text/plain" }, [message]]
end
end
end
28 changes: 10 additions & 18 deletions lib/shrine/plugins/presign_endpoint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,9 @@ def call(env)
end
end

if Rack.release >= "3"
headers["content-length"] ||= body.respond_to?(:bytesize) ? body.bytesize.to_s :
body.map(&:bytesize).inject(0, :+).to_s
else
headers["Content-Length"] ||= body.map(&:bytesize).inject(0, :+).to_s
end

headers = headers.transform_keys(&:downcase) if Rack.release >= "3"
headers = Rack::Headers[headers] if Rack.release >= "3"
headers["Content-Length"] ||= body.respond_to?(:bytesize) ? body.bytesize.to_s :
body.map(&:bytesize).inject(0, :+).to_s

[status, headers, body]
end
Expand Down Expand Up @@ -165,25 +160,22 @@ def generate_presign(location, options, request)
# headers, and a body enumerable. If `:rack_response` option is given,
# calls that instead.
def make_response(object, request)
if @rack_response
response = @rack_response.call(object, request)
status, headers, body = if @rack_response
@rack_response.call(object, request)
else
response = [200, { "Content-Type" => CONTENT_TYPE_JSON }, [object.to_json]]
[200, { "Content-Type" => CONTENT_TYPE_JSON }, [object.to_json]]
end

headers = Rack::Headers[headers] if Rack.release >= "3"
# prevent browsers from caching the response
response[1]["Cache-Control"] = "no-store" unless response[1].key?("Cache-Control")
headers["Cache-Control"] = "no-store" unless headers.key?("Cache-Control")

response
[status, headers, body]
end

# Used for early returning an error response.
def error!(status, message)
headers = { "Content-Type" => CONTENT_TYPE_TEXT }

headers = headers.transform_keys(&:downcase) if Rack.release >= "3"

throw :halt, [status, headers, [message]]
throw :halt, [status, { "Content-Type" => CONTENT_TYPE_TEXT }, [message]]
end

# Returns the uploader around the specified storage.
Expand Down
8 changes: 3 additions & 5 deletions lib/shrine/plugins/rack_response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,9 @@ def call(**options)
headers = rack_headers(**options)
body = rack_body(**options)

if Rack.release >= "3"
[status, headers.transform_keys(&:downcase), body]
else
[status, headers, body]
end
headers = Rack::Headers[headers] if Rack.release >= "3"

[status, headers, body]
end

private
Expand Down
17 changes: 4 additions & 13 deletions lib/shrine/plugins/upload_endpoint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,9 @@ def call(env)
handle_request(request)
end

if Rack.release >= "3"
headers["content-length"] ||= body.respond_to?(:bytesize) ? body.bytesize.to_s :
body.map(&:bytesize).inject(0, :+).to_s
else
headers["Content-Length"] ||= body.map(&:bytesize).inject(0, :+).to_s
end

headers = headers.transform_keys(&:downcase) if Rack.release >= "3"
headers = Rack::Headers[headers] if Rack.release >= "3"
headers["Content-Length"] ||= body.respond_to?(:bytesize) ? body.bytesize.to_s :
body.map(&:bytesize).inject(0, :+).to_s

[status, headers, body]
end
Expand Down Expand Up @@ -217,11 +212,7 @@ def verify_checksum!(file, request)

# Used for early returning an error response.
def error!(status, message)
headers = { "Content-Type" => CONTENT_TYPE_TEXT }

headers = headers.transform_keys(&:downcase) if Rack.release >= "3"

throw :halt, [status, headers, [message]]
throw :halt, [status, { "Content-Type" => CONTENT_TYPE_TEXT }, [message]]
end

# Returns the uploader around the specified storage.
Expand Down
3 changes: 1 addition & 2 deletions shrine.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,14 @@ direct uploads for fully asynchronous user experience.
gem.add_dependency "content_disposition", "~> 1.0"

# general testing helpers
gem.add_development_dependency "appraisal", "~> 2.5"
gem.add_development_dependency "rake", ">= 11.1"
gem.add_development_dependency "minitest", "~> 5.8"
gem.add_development_dependency "mocha", "~> 1.11"

# for endpoint plugins
gem.add_development_dependency "rack", ">= 2", "< 4"
gem.add_development_dependency "http-form_data", "~> 2.2"
gem.add_development_dependency "rack-test_app"
gem.add_development_dependency "rack-test", "~> 2.1"

# for determine_mime_type plugin
gem.add_development_dependency "mimemagic", ">= 0.3.2"
Expand Down
Loading

0 comments on commit 54e51c8

Please sign in to comment.