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

Rack::Lint::Error => a header value must be a String or Array of Strings, but the value of 'content-type' is a NilClass #2411

Closed
ericproulx opened this issue Jan 7, 2024 · 2 comments · Fixed by #2414
Closed

Rack::Lint::Error => a header value must be a String or Array of Strings, but the value of 'content-type' is a NilClass #2411

ericproulx opened this issue Jan 7, 2024 · 2 comments · Fixed by #2414

Comments

@ericproulx
Copy link
Contributor

I've been testing Rack::Lint locally and we have a lot of issues in our spec suite. I'll open issues for others but let's start with this one:

Running spec: spec/grape/api_spec.rb:2341.

 Rack::Lint::LintError:
       a header value must be a String or Array of Strings, but the value of 'content-type' is a NilClass

This test is using rack_response directly in a rescue_from like this

subject.rescue_from ApiSpec::APIErrors::ParentError, rescue_subclasses: true do |e|
 rack_response("rescued from #{e.class.name}", 500)
end

Unfortunately, rack_response doesn't act like error! since the last is passing through the formatting part (because of the throw) whereas the rack_response just html escaped if content_type is set totext/html (if content_type is set in the api)

Finally, rack_response is never mentioned in the README but using a plain Rack::Response is. Also, Using rack_response or Rack::Response doesn't pass through the formatting part of the error middleware since its considered a final response.

IMO, there are many ways to solve that but I found an interesting middleware within Rack called [Rack::ContentType] (https://github.com/rack/rack/blob/main/lib/rack/content_type.rb) that will set a default content-type header if needed. Default, is text/html but I think text/plain would be more appropriate since we do not need to escape. I think its a clean way to deal with that issue and we could remove some default behavior in the code.
@ericproulx
Copy link
Contributor Author

I understand what's happening. Before #1922, content_type was taken from the base middleware which default to txt/plain.

# lib/grape/middleware/base.rb

def content_type
  content_type_for(env[Grape::Env::API_FORMAT] || options[:format]) || TEXT_HTML
end

Now, some test are now using rack_response instead of error! and this method will call content-type from the endpoint which will be nil if not set explicitely from header.

# lib/grape/dsl/inside_route.rb

def content_type(val = nil)
  if val
    header(Rack::CONTENT_TYPE, val)
  else
    header[Rack::CONTENT_TYPE]
  end
end

I don't think anyone is using rack_response in a rescue_from since its never been documented and I think we should
1 - remove the method from inside_route
2 - make the function private in lib/grape/middleware/error.rb, even though its not accessible anymore within rescue_from

@dblock
Copy link
Member

dblock commented Jan 20, 2024

Good plan. If no specs break we should be good.

@ericproulx ericproulx mentioned this issue Jan 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix response headers from lint ericproulx/grape
2 participants
@dblock @ericproulx