Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rack.response_finished to Rack::Lint #1802

Closed
wants to merge 9 commits into from
Closed
6 changes: 6 additions & 0 deletions SPEC.rdoc
Expand Up @@ -137,6 +137,12 @@ There are the following restrictions:
set. <tt>PATH_INFO</tt> should be <tt>/</tt> if
<tt>SCRIPT_NAME</tt> is empty.
<tt>SCRIPT_NAME</tt> never should be <tt>/</tt>, but instead be empty.
<tt>rack.response_finished</tt>:: An array of objects responding to #call with one argument, the env hash for the request, that will be called after the HTTP response has been sent to the client.
The callables are called directly after the HTTP response has been sent to the
client. The callables should be called sequentially and synchronously in the
same execution context as the the response. If an exception is raised, it will
be ignored and will not impact the execution of the other callables.


=== The Input Stream

Expand Down
1 change: 1 addition & 0 deletions lib/rack/constants.rb
Expand Up @@ -52,6 +52,7 @@ module Rack
RACK_RECURSIVE_INCLUDE = 'rack.recursive.include'
RACK_MULTIPART_BUFFER_SIZE = 'rack.multipart.buffer_size'
RACK_MULTIPART_TEMPFILE_FACTORY = 'rack.multipart.tempfile_factory'
RACK_RESPONSE_FINISHED = 'rack.response_finished'
Copy link
Member

@ioquatix ioquatix Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BlakeWilliams @tenderlove looking at other keys, should this be rack.response.finished or rack.response_finished? We don't use rack.request_form_input but rack.request.form_input.

Is there any precedent for appending _callbacks to the name i.e. rack.response.finished_callbacks or something? I don't mind either way, just calling it out for further discussion.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of it being "namespaced", like rack.response.finished. I'll update that real quick, but I also like the idea of specifying that it is some kind of array of callbacks. I'll defer to other folks on naming, but I'm 👍 on rack.response.finished_callbacks.

RACK_REQUEST_FORM_INPUT = 'rack.request.form_input'
RACK_REQUEST_FORM_HASH = 'rack.request.form_hash'
RACK_REQUEST_FORM_VARS = 'rack.request.form_vars'
Expand Down
17 changes: 17 additions & 0 deletions lib/rack/lint.rb
Expand Up @@ -359,6 +359,23 @@ def check_environment(env)
unless env[SCRIPT_NAME] != "/"
raise LintError, "SCRIPT_NAME cannot be '/', make it '' and PATH_INFO '/'"
end

## <tt>rack.response_finished</tt>:: An array of callables run after the HTTP response has been sent.
BlakeWilliams marked this conversation as resolved.
Show resolved Hide resolved
if callables = env[RACK_RESPONSE_FINISHED]
raise LintError, "rack.response_finished must be an array of callable objects" unless callables.is_a?(Array)

callables.each do |callable|
raise LintError, "rack.response_finished values must respond to call" unless callable.respond_to?(:call)
BlakeWilliams marked this conversation as resolved.
Show resolved Hide resolved

arity = if callable.respond_to?(:arity)
callable.arity
else
callable.method(:call).arity
end

raise LintError, "rack.response_finished values must accept an env argument" unless arity == 1
end
end
end

##
Expand Down
36 changes: 36 additions & 0 deletions test/spec_lint.rb
Expand Up @@ -209,6 +209,31 @@ def obj.error(*) end
Rack::Lint.new(nil).call(env("SCRIPT_NAME" => "/"))
}.must_raise(Rack::Lint::LintError).
message.must_match(/cannot be .* make it ''/)

lambda {
Rack::Lint.new(nil).call(env("rack.response_finished" => "not a callable"))
}.must_raise(Rack::Lint::LintError).
message.must_match(/rack.response_finished must be an array of callable objects/)

lambda {
Rack::Lint.new(nil).call(env("rack.response_finished" => [-> (env) {}, "not a callable"]))
}.must_raise(Rack::Lint::LintError).
message.must_match(/rack.response_finished values must respond to call/)

lambda {
Rack::Lint.new(nil).call(env("rack.response_finished" => [-> () {}]))
}.must_raise(Rack::Lint::LintError).
message.must_match(/rack.response_finished values must accept an env argument/)

callable_object = Class.new do
def call
end
end.new

lambda {
Rack::Lint.new(nil).call(env("rack.response_finished" => [callable_object]))
}.must_raise(Rack::Lint::LintError).
message.must_match(/rack.response_finished values must accept an env argument/)
end

it "notice input errors" do
Expand Down Expand Up @@ -708,4 +733,15 @@ def assert_lint(*args)
}).call(env({}))[1]['rack.hijack'].call(StringIO.new).read.must_equal ''
end

it "pass valid rack.response_finished" do
callable_object = Class.new do
def call(env)
end
end.new

Rack::Lint.new(lambda { |env|
[200, {}, ["foo"]]
}).call(env({ "rack.response_finished" => [-> (env) {}, lambda { |env| }, callable_object], "content-length" => "3" })).first.must_equal 200
end

end