whichdigital / active-rest-client

ActiveRestClient API Client
https://rubygems.org/gems/active_rest_client
MIT License
385 stars 44 forks source link

Set content type #80

Closed tiagotex closed 9 years ago

tiagotex commented 9 years ago

Set Content-Type for default and JSON.

tiagotex commented 9 years ago

resolve #79

tiagotex commented 9 years ago

I'm thinking we should also enforce the default charset to be UTF-8 for JSON.

@andyjeffries What's your opinion on changing Content-type for JSON from application/json to application/json; charset=utf-8

andyjeffries commented 9 years ago

I agree with this in principle, however my only concern is - if that's the default, how will users change it?

tiagotex commented 9 years ago

In request.rb:

def call(explicit_parameters=nil)
  (...)
  if object_is_class?
    @object.send(:_filter_request, :before, @method[:name], self)
  else
    @object.class.send(:_filter_request, :before, @method[:name], self)
  end

  append_get_parameters
  prepare_request_body_type
  (...)
end

Move append_get_parameters and prepare_request_body_type before if object_is_class?, this way the header is defined but can be overridden using before_request:

# In model
before_request :change_content_type

def change_content_type(name, request)
  if name == :save
      request.headers["Content-Type"] = "application/json"
    end
end

Do you agree?

andyjeffries commented 9 years ago

Not such a fan of that - as before_request may well be used by lots of people to mess around with the request params before the body is generated. A better solution may be to only set the Content-Type header if it's not already set? That way people can still tweak it in a before_request.

andyjeffries commented 9 years ago

P.S. I take it you saw the two failing tests now? ;-)

tiagotex commented 9 years ago

Yes, it was my mistake.

Setting up content-type type only if not defined seems to be a perfect solution.

andyjeffries commented 9 years ago

Thanks for this, merged in.