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

Avoid returning [nil] to Rack #2118

Open
stanhu opened this issue Oct 15, 2020 · 8 comments
Open

Avoid returning [nil] to Rack #2118

stanhu opened this issue Oct 15, 2020 · 8 comments
Labels

Comments

@stanhu
Copy link
Contributor

stanhu commented Oct 15, 2020

Prior to Rack v2.1.0, Rack::Response would call to_s on each of the body parts: https://github.com/rack/rack/blob/85684323f8f58409e717af91e446d257d496f8b8/lib/rack/response.rb#L40-L43. That means if Grape returned [nil], Rack would handle this gracefully.

This is no longer the case now with https://github.com/rack/rack/pull/1434/files. It's easy to have an API call that returns nil to Grape, and the body will be set to [nil]:

grape/lib/grape/endpoint.rb

Lines 267 to 277 in 0d5cf40

response_object = execute
end
run_filters afters, :after
cookies.write(header)
# status verifies body presence when DELETE
@body ||= response_object
# The body commonly is an Array of Strings, the application instance itself, or a Stream-like object
response_object = stream || [body]

This causes fits with Rack:

NoMethodError (undefined method `empty?' for nil:NilClass):
rack (2.1.4) lib/rack/etag.rb:70:in `block in digest_body'
rack (2.1.4) lib/rack/body_proxy.rb:34:in `block in each'
rack (2.1.4) lib/rack/body_proxy.rb:34:in `each'
rack (2.1.4) lib/rack/body_proxy.rb:34:in `each'
rack (2.1.4) lib/rack/etag.rb:68:in `digest_body'
rack (2.1.4) lib/rack/etag.rb:31:in `call'
rack (2.1.4) lib/rack/conditional_get.rb:40:in `call'
rack (2.1.4) lib/rack/head.rb:14:in `call'

Maybe Grape should return nil instead of [nil] if there is no body? This might have to be done in the formatters since nil may mean null in JSON.

Rack issue: rack/rack#1712

@stanhu
Copy link
Contributor Author

stanhu commented Oct 15, 2020

I'm going to close this issue because this problem seems to have gone away in Rack v2.2.3, but v2.1.x has this issue. rack/rack#1535 might have helped.

@stanhu stanhu closed this as completed Oct 15, 2020
@stanhu
Copy link
Contributor Author

stanhu commented Oct 15, 2020

Closed too soon. I got this with v2.2.3 as well.

@stanhu stanhu reopened this Oct 15, 2020
@stanhu
Copy link
Contributor Author

stanhu commented Oct 15, 2020

Based on rack/rack#1713 (comment), it sounds like Grape should flag responses that don't contain String values?

@dblock
Copy link
Member

dblock commented Oct 15, 2020

Do we want to raise errors in Grape for this? Feels like that would be a no, because a contract is a contract, but I'm open to it.

@stanhu
Copy link
Contributor Author

stanhu commented Oct 15, 2020

I created this middleware:

class ResponseCoercerMiddleware < ::Grape::Middleware::Base
  def call(env)
    response = super(env)

    status = response[0]
    body = response[2]

    return response if Rack::Utils::STATUS_WITH_NO_ENTITY_BODY[status]
    return response unless body.is_a?(Array)

    body.map! do |part|
      if part.is_a?(String)
        part
      else
        part.to_s
      end
    end

     response
  end
end

In test and development, I modified this to raise an exception when part is not a String.

@stanhu
Copy link
Contributor Author

stanhu commented Oct 15, 2020

I wonder if we want to add some variant of this to the base middleware.

@dblock dblock added the bug? label Dec 15, 2020
@ebenoist
Copy link

Just got hit by this as well. Is there a blessed way to fix this issue?

@dblock
Copy link
Member

dblock commented Mar 1, 2022

@ebenoist Care to dig up whether this is the case un latest Rack and what the Rack API is supposed to be? Seems like there's no conclusion above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants