Fix multipart to set its header even when other headers are provided

[jirutka]

@raw_request.initialize_http_header resets the headers, so when options contained :headers, it always erased the Content-Type header set for multipart.

Now the Content-Type header is always set to "multipart/form-data" when body is detected as multipart. This is IMHO not correct, but that's how the original implementation was designed…

Related to: #569
Notify: @TheSmartnik

[jirutka]

This is nasty copypaste code, but unfortunately that’s how the rest of the tests in this file is written, so to be consistent…

[TheSmartnik]

This is IMHO not correct, but that's how the original implementation was designed

Why do you think this isn't correct? What would be a correct behaviour in your opinion?

[jordanbyron]

@TheSmartnik you can re-produce the bug by running this slightly modified version of the specs for checking the headers. Only change I made here was to include headers: {} when calling @klass.post:

context 'when posting file' do
  let(:boundary) { '------------------------c772861a5109d5ef' }
  let(:headers) do
    { 'Content-Type'=>"multipart/form-data; boundary=#{boundary}" }
  end

  before do
    expect(HTTParty::Request::MultipartBoundary).to receive(:generate).and_return(boundary)
  end

  it 'changes content-type headers to multipart/form-data' do
    stub_request(:post, "http://example.com/").with(headers: headers)

    @klass.post('http://example.com', body: { file: File.open('spec/fixtures/tiny.gif')}, headers: {})
  end
end

Adding any headers to the post call wipes out the Content-Type header set here:

# lib/httparty/request.rb
#
# Assuming *options* has a multipart body AND headers
#
def setup_raw_request
  # snip ...

  if options[:body] # <= This gets called
    if body.multipart?
      content_type = "multipart/form-data; boundary=#{body.boundary}"
      @raw_request.initialize_http_header('Content-Type' => content_type)  # <= This gets called
    end
    @raw_request.body = body.call
  end

  if options[:headers].respond_to?(:to_hash)  # <= This gets called
    headers_hash = options[:headers].to_hash

    # This gets called, clearing the Content-Type header set above
    @raw_request.initialize_http_header(headers_hash) 
    # $=> @raw_request.to_hash 
    # #=> {}
    #  Missing Content-Type, breaking multipart support :(
    # snip ...
    end
  end

  # snip ....
end

[jnunemaker]

I think this makes sense. It seems like we'd want the multi part header regardless of any other headers passed in.

@TheSmartnik any feels to the contrary?

[jordanbyron]

@jnunemaker @TheSmartnik hey gents! Anything I can do to help move this along? We've got our httparty locked at the pre-0.16 version until this gets resolved. Thanks so much!

[TheSmartnik]

Hi, everyone. Sorry for a long reply.

I think this makes sense. It seems like we'd want the multi part header regardless of any other headers passed in.

@jnunemaker I agree. That's why I asked, why @jirutka said

This is IMHO not correct

[TheSmartnik]

Tested the fix. Working good. Thanks @jirutka

[jnunemaker]

0.16.1 is out.