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

Fix response status usage - convert it with to_i method #1644

Closed
wants to merge 1 commit into from

Conversation

andrykonchin
Copy link
Contributor

Some middleware treat response status as Integer but according to the specification it's just an object with to_i method which returns Integer.

Changes:

  • fixed Rack::ConditionalGet
  • fixed Rack::ETag

@andrykonchin
Copy link
Contributor Author

andrykonchin commented May 4, 2020

Actually I am thinking about general approach in tests to ensure middleware follow the specification and use only declared interface of response status, headers and body.

For instance in the tests all the applications can wrap a response with some helper and convert status, headers and body to objects which respond to declared method e.g. 'to_i' and each:

app = lambda { |env| Rack::Lint::Response.wrap(201, { 'Content-Type' => 'text/plain' }, ["Hello, World!"]) }

module Rack
  class Lint
    module Response
      def self.wrap(status, headers, body)
        [status.to_s, headers.to_enum, body.to_enum]
      end
    end
  end
end

This is the simple implementation and it isn't strict as it could be but it's enough to catch issues with treating status as Integer, headers as Hash and body as Array.

If it's reasonable approach I will be happy to create a PR and update the specs.

@jeremyevans
Copy link
Contributor

I'm not sure how other maintainers feel about this, but I think in Rack 3 we should change SPEC to require the status to be an Integer instead of requiring that it can be converted to an integer. Having all middleware perform conversion to get the status is inefficient, and I can't see a significant advantage in supporting non-Integer status values.

We already have a similar pull request to make the response headers be a hash (#1597).

@ioquatix
Copy link
Member

ioquatix commented May 5, 2020

I agree status should be an integer, I don't know any use case that benefits from this not being the case. Any counter arguments?

@ioquatix
Copy link
Member

ioquatix commented May 5, 2020

@tenderlove can we get your historical context on this?

@tenderlove
Copy link
Member

@tenderlove can we get your historical context on this?

Honestly I think it's legacy. I agree it should just be an integer and we should fix SPEC

@ioquatix
Copy link
Member

@olleolleolle if you are interested to have a go at helping, this one is pretty low hanging fruit (updating the SPEC).

olleolleolle added a commit to olleolleolle/rack that referenced this pull request May 24, 2020
  - rack#1644 refers to the idea "require Status to be an Integer"
  - This implements that change
olleolleolle added a commit to olleolleolle/rack that referenced this pull request May 24, 2020
  - rack#1644 refers to the idea "require Status to be an Integer"
  - This implements that change
olleolleolle added a commit to olleolleolle/rack that referenced this pull request May 24, 2020
  - rack#1644 refers to the idea "require Status to be an Integer"
  - This implements that change
ioquatix pushed a commit that referenced this pull request May 24, 2020
  - #1644 refers to the idea "require Status to be an Integer"
  - This implements that change
@ioquatix
Copy link
Member

Fixed by #1662

@ioquatix ioquatix closed this May 24, 2020
@andrykonchin andrykonchin deleted the fix-status-code-usage branch May 24, 2020 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants