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.

Sign up for GitHub

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

Jump to bottom

Correct Rack::MockRequest/Rack::Lint::Wrapper::InputWrapper re #set_encoding #2115

Closed
wants to merge 1 commit into from
Closed

Correct Rack::MockRequest/Rack::Lint::Wrapper::InputWrapper re #set_encoding #2115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/rack/lint.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,11 @@ def each(*args)
def close(*args)
@input.close(*args)
end

## * +set_encoding+ just passes through to the underlying object.
def set_encoding(*args)
@input.set_encoding(*args) if @input.respond_to?(:set_encoding)
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? Surely this is on the mock request to deal with - i.e. we shouldn't change the interface requirements of the input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I encountered this situation when appropriating Rack::MockRequest.env_for to generate the elements for (what are effectively) subrequests within an app, what happened was rack.input was an instance of Rack::Lint::Wrapper::InputWrapper wrapping an instance of Puma::NullIO and neither respond to #set_encoding, so it would crash.

I genuinely believe it's more clement behaviour for Rack::MockRequest.env_for to test if the input object responds to #set_encoding before trying to call it (just like it does for #size directly below).

I'm not married to that pass-through method in Rack::Lint::Wrapper::InputWrapper but the motivation there was Postel's law. In case the underlying input does respond to #set_encoding, perform the expected behaviour.

Copy link
Member

@ioquatix ioquatix Aug 27, 2023 •
edited

Choose a reason for hiding this comment

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

What you are asking, is for every implementation of the input body to optionally adopt this interface. But the expectation is, the input should always be binary. So, I'm not sure whether this is a good idea. If the input is not binary, that sounds like a bug in whatever is generating the body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am doing nothing more than ask for things not to crash when they don't need to. Prior to this patch, the implementation of Rack::MockRequest.env_for does exactly that: crash because it already assumes whatever handed to it responds to #set_encoding. Do you have an issue with that too, or just the pass-through method in Rack::Lint::Wrapper::InputWrapper?

end
end

##
Expand Down
3 changes: 2 additions & 1 deletion lib/rack/mock_request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,8 @@ def self.env_for(uri = "", opts = {})
end

if rack_input
rack_input.set_encoding(Encoding::BINARY)
# input is not guaranteed to respond to set_encoding
rack_input.set_encoding(Encoding::BINARY) if rack_input.respond_to?(:set_encoding)
env[RACK_INPUT] = rack_input

env["CONTENT_LENGTH"] ||= env[RACK_INPUT].size.to_s if env[RACK_INPUT].respond_to?(:size)
Expand Down
4 changes: 4 additions & 0 deletions test/spec_mock_request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,10 @@
called.must_equal true
end

it "doesn't complain if the input can't #set_encoding" do
env = Rack::MockRequest.env_for('/foo', input: Object.new)
end

it "defaults encoding to ASCII 8BIT" do
req = Rack::MockRequest.env_for("/foo")

Expand Down