Content-Type forced for all responses

[MOZGIII]

From Sinatra::Base @ https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L915:

    def call!(env) # :nodoc:
      @env      = env
      @request  = Request.new(env)
      @response = Response.new
      template_cache.clear if settings.reload_templates

      @response['Content-Type'] = nil
      invoke { dispatch! }
      invoke { error_block!(response.status) } unless @env['sinatra.error']

      unless @response['Content-Type']
        if Array === body and body[0].respond_to? :content_type
          content_type body[0].content_type
        else
          content_type :html
        end
      end

      @response.finish
    end

Code

      unless @response['Content-Type']
        if Array === body and body[0].respond_to? :content_type
          content_type body[0].content_type
        else
          content_type :html
        end
      end

is problematic, because it forces response to have a Content-Type header. In some cases we don't want that. Can we make a setting to allow disabling this behavior?

My usecase is not limited to 204 response. I have some error codes that should have no content as well.

[MOZGIII]

Simply extracting this "default" Content-Type handler into a separate method would also do the trick for me. I'd just redefine that method to do nothing.

Currently I ended up reimplementing the whole call! in my app.

[kgrz]

Can you instead use a middleware which removes the content type or any other information that needs to be removed? For instance:

class MyMiddleware
  def initialize(app)
    @app = app
  end

  def call(env)
    status, headers, response = @app.call(env)
    headers.delete('Content-Type')
    [status, headers, response]
  end
end

And in config.ru:

require './middleware.rb'

use MyMiddleware
run MyApp

This way you can customise all the routes according to your use case, and avoid overriding any internal method of the library itself. This will save you from unexpected issues if and when this def call! method changes in future versions.

[mwpastore]

If you halt 204 it will not set a Content-Type. Can you share some examples with us that demonstrate the behavior you're trying to work around?

[MOZGIII]

For example error 404. I expect Content-Type to be nil. But it's forced to be text/html.

[mwpastore]

Unfortunately, it looks like Rack::Protection requires that a Content-Type be set most of the time, so it fails with weird errors if we try to work around Sinatra by setting it to nil or anything like that.

Quite frankly, I don't understand why it's so important to you to remove the Content-Type header in these cases. My understanding is that browsers and other user-agents expect this header to be present on most responses, even on 404. The Content-Length and Transfer-Encoding headers are intended to be used to determine whether or not there is content. Just set it to some appropriate default for your intended audience(s) and forget about it.

If you're still determined to do this, I think @kgrz's idea is best.

[MOZGIII]

Best is to reimplement call!, which I already did.
There is no reason why the Content-Type header is needed to be forced for all users. A middleware like @kgrz would require a non-trivial logic to be added to remove a header since I don't need it everywhere but only for a certain codepaths. To add some metadata to env just to remove headers later - it just seems to like a nonsense.
Furthermore, I don't use Rack::Protection at all. Since it's on by default, to maintain backward-compatibility this should be made an option with enabled state by default (force_default_content_type).

The current implementation is based on an assumption that Content-Type is a header that only 204 requests must not have, however it's now always the case (does not work for everyone).

My actual goal is to implement the API spec that I have as strict as possible.

[mwpastore]

I opened #1239 to address this. We'll see what the higher-ups have to say!

[MOZGIII]

It's been a long time now... I wonder if it's still relevant. If the issue is still there - the PR solves it, so why don't we merge it?

[jkowens]

Fixed by #1239.