Optionally set encoding of rack.input in MockRequest.env_for.

[ioquatix]

Possible fix for #2115.

[jeremyevans]

I'm against this change. I believe rack.input if present must be IO-like per SPEC, and therefore should respond to set_encoding. The issue in #2115 is the user is providing an invalid/non-IO-like input stream. That is something that should be fixed in their code.

[ioquatix]

#2115 (comment)

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 am against introducing Rack::Lint::InputWrapper#set_encoding and think this PR is a better, less risky, solution.

[tenderlove]

Shouldn't the input always be in binary mode? More importantly it should be in binary mode before Rack ever gets its hands on it. If it isn't in binary mode, then I agree its a bug in the user's code.

Could we just delete this line, or change it to call encoding and verify it's already in binary mode? FWIW it seems like we should ask that encoding is implemented on any incoming IO objects so that we can verify the encoding. Calling set_encoding at all seems unexpected.

[tenderlove]

btw, I recognize that just removing the set_encoding will break a bunch of stuff. I was just thinking "pie in the sky". Basically, I don't like the idea of possibly mutating user's objects (which is what set_encoding does).

I'm not sure that the inputs to MockRequest fall under the purvey of "SPEC". Namely because I think the idea behind this code is "I have a bunch of stuff, and I would like to have a SPEC compliant env please". IOW I wouldn't expect the inputs to necessarily meet SPEC, but I do expect the output to conform to SPEC. For example, we accept strings and automatically convert them to "io like objects" for the user.

I don't think we should force the input to MockRequest to implement set_encoding, but I do think it would be swell if we could only read the encoding and raise an exception if the wrong thing is passed in. I know that would be a major version rev though.

[MSP-Greg]

Maybe internal_encoding or external_encoding? IO and StringIO do not respond to encoding...

[jeremyevans]

I am against introducing Rack::Lint::InputWrapper#set_encoding

If we want to limit the allowed input stream methods to just gets, each, and read (and optionally close), I think we should remove the "IO-like" term from SPEC. If we do that, I'm OK with this change.

[ioquatix]

"IO-like" term from SPEC. If we do that, I'm OK with this change.

If "IO-like" is causing confusion, then yes, let's remove it.

However, my interpretation is, that for the methods that are listed, should behave similarly to the methods as implemented on IO.

In other words, one could supply an IO or similar object (StringIO) as an instance of rack.input. OR an object which implements the required method in a way that's similar to IO. That's my understanding of what "IO-like" means.