[MULTIPART-FILE] Content type should always be set

# Context

In here:

https://github.com/kennethreitz/requests/blob/v2.7.0/requests/models.py#L464-L466

The content type is only set if the there isn't one already set. I feel this shouldn't be the case.

As we can see here:  https://github.com/kennethreitz/requests/blob/master/requests/packages/urllib3/filepost.py#L92

The content type string has a boundary generated by this method. This is specific to the body being encoded. Without this boundary and content type, the data being posted is no longer considered `Multipart Form Data`, which is the whole point of specifying `files=` in the post method.

Even raising an exception would be better than silently skipping setting the content type.


Hi @vaibhavkaul,

Thanks for sending a pull request. I'm afraid we likely won't be accepting it. Here's why:
- This is drastically backwards incompatible behaviour. 
- It takes a great deal of freedom away from the user which we have intentionally placed into their hands.
- This code does not just change this for the `multipart/form-data` case.
- This also prevents people from writing API fuzzers which provide incorrect content-type headers for a totally different kind of content and drastically reduces requests flexibility.
- This affects the cases where people provide something to `data=` or `json=` with a custom content-type header where they expect their explicit decision to specify that header to be honored.

We could consider this for version 3.0.0 but I think it would not fit there either for several of the reasons listed above and also mostly due to the fact that we hope to not break things too drastically in 3.0.

I won't close this just yet because I hope @Lukasa and @kennethreitz will weigh in as well.

Cheers,
Ian


@sigmavirus24 I understand. I figured it might be worth pointing out. 

> It takes a great deal of freedom away from the user which we have intentionally placed into their hands.

As long as this is an intentional choice.

> This code does not just change this for the multipart/form-data case.

You are right. I missed that, i will at-least make it specific to the files case, even if we end up not merging this.

The silent nature of the problem can cause a lot of headaches. I don't think a majority of the users using `requests` are going for the over-ridden behavior or API Fuzzers.


> The silent nature of the problem can cause a lot of headaches.

Might I ask why this change is any less of a silent headache though? Let me paint a picture:

``` py
requests.post(url, header={'Content-Type': 'multipart/form-data'}, files=files, data=data)
```

Will make the correct body with an incorrect header. With a little research a user can figure out what they did wrong.

``` py
requests.post(url, headers={'Content-Type': 'application/vnd.github.drax-preview+json'}, json=json_data)
```

Would forcibly reset the header to `application/json` which breaks things for the user doing the _right_ thing.

Sometimes overriding requests default behaviour is correct for user. I'm also of the opinion that special-casing this to just multipart uploads is the wrong thing. That means that one of the cases above prevents the user from doing exactly what they intend to do and introduces a great deal of inconsistency in the API. If any user needs to do something with an alternate content-type header for multipart form-data request, we now punish them by forcing them to use the prepared request flow and overriding the header after preparing the request... just to change one header.

I understand that this can be and has been a foot-gun for several people. Google eagerly returns the StackOverflow questions and answers (there are a few) that explain what is happening and what needs to be fixed to correct the request.

I don't think introducing inconsistencies in the API or silent behaviour is the right thing.


I'm also disinclined to make this change. I can see why we'd do it, but I think if we're worried about requests silently subverting expectations then changing the behaviour to _differently_ silently subvert expectations isn't a great direction.

One option might be to instead fire a warning or a log in this case. That would attract user attention while also allowing users who so choose to override this behaviour. 

In general the requests header dictionary lets people do stupid things. If the user wants to change content-length, they can. If they want to change transfer-encoding, they can. There is only so much we can do to protect users from themselves. 


@kennethreitz thoughts?


@sigmavirus24 I made the change so it doesn't affect other things besides files.

> Sometimes overriding requests default behaviour is correct for user

Yes, which which is why `sometimes` should not dictate default behavior.

> Will make the correct body with an incorrect header. 

I dont think thats true since for the body to be parseable by any standard multi-part library (cgi etc) it would need to have the correct header. Just having the data in the body does not make it correct. I would argue the body is wrong in this case for a multipart request.

My motive right now is not to get this merged in at all. Happy to close the PR. But we should be clear about what the issue is here. Specifically I would like to address things like:

> That means that one of the cases above prevents the user from doing exactly what they intend to do

and 

> then changing the behaviour to differently silently subvert expectations isn't a great direction

The specific part of code I am referring to deals [exclusively with Multipart Files](https://github.com/kennethreitz/requests/blob/master/requests/models.py#L445-L447). The whole [idea of adding a `file=` param](http://docs.python-requests.org/en/master/user/quickstart/#post-a-multipart-encoded-file) is to encode the request as a multi-part request. `Most` developers using that code path clearly want the request to be posted as multipart.

Multipart requests must [have a boundary specified in the content-type](https://www.w3.org/Protocols/rfc1341/7_2_Multipart.html), if this is missing, the whole purpose for passing a `files=` param is defeated. This is no longer a multipart request. This is not a `correct body with the wrong header`. Clearly the `requests` framework [does not allow a user to pass their own boundary](https://github.com/kennethreitz/requests/blob/master/requests/models.py#L156) (notice the missing boundary kwarg) and use that. It `always` relies on a auto generated boundary string. We should either give people the ability to be in control of the `content-type` (by letting them specify a boundary string) or do the right think in the library.

> In general the requests header dictionary lets people do stupid things. If the user wants to change content-length, they can. If they want to change transfer-encoding, they can

I agree. Devs do stupid things. But then why make [this](https://github.com/kennethreitz/requests/blob/master/requests/models.py#L434-L438) raise an exception. There should at least be a warning somewhere about the request not being encoded as a multi part request if the headers being passed include a `content-type`, even if that content type is `multipart/form-data`.

I completely agree that we should not make this change if you feel this breaks the existing API, so maybe this should be considered for a future release or maybe not. I'll leave it up to @kennethreitz.


FWIW, do you guys think a better solution would be to allow the user to pass a boundary string and use that instead?


> FWIW, do you guys think a better solution would be to allow the user to pass a boundary string and use that instead?

You already can with the toolbelt's [`MultipartEncoder`](https://toolbelt.readthedocs.org/en/latest/uploading-data.html#streaming-multipart-data-encoder) and since 99% of our users don't need that in requests itself, it makes perfect sense to have something that lives elsewhere to do that.

> But then why make [this](https://github.com/kennethreitz/requests/blob/46184236dc177fb68c7863445609149d0ac243ea/requests/models.py#L434-L438) raise an exception.

Because one is exceptional. We could issue a warning when overriding the content-type header when a user specifies both `data=` and `files=` but I think that will only make a lot of people very angry with us.

> The specific part of code I am referring to deals exclusively with Multipart Files.

Right you're special casing **silent** behaviour for one very narrow case of something that should either be the case for everything or not. APIs cease being for humans when you have to say to yourself "Wait, will requests overwrite this header for me if I'm doing this or is it only in this other case?" every time they go back to do something. That is not a human API, that is an awful API design.

> > Sometimes overriding requests default behaviour is correct for user
> 
> Yes, which which is why sometimes should not dictate default behavior.

The default behaviour of requests is not wrong. The default behaviour is to trust the user that they know what they're doing.

> Most developers using that code path clearly want the request to be posted as multipart.

And they will get a body that is correct for a `multipart/form-data` request. If they're intentionally changing the header and the server they're talking to does not understand the header, they'll figure that out. People using requests are generally rather intelligent. Most have read the documentation and understand what they need to do when creating a valid `multipart/form-data` request. It is often the people who do not read the documentation that run into this, go and read the documentation, and fix it themselves.

> I completely agree that we should not make this change if you feel this breaks the existing API.

It's not a matter of _feeling_ that this is backwards incompatible. It is backwards incompatible.

And let me just strongly reinforce a point @Lukasa and I seem to be unable to convey appropriately:

This is taking documented and expected behaviour that you consider to be _silently_ broken and replacing it with behaviour that will be undocumented, unexpected, and will _silently_ break users' code. Your justification is to fix _silent_ behaviour but you're replacing it with inconsistent _silent_ behaviour.


@sigmavirus24 

> It's not a matter of feeling that this is backwards incompatible. It is backwards incompatible.

Yes, that was a bad choice of words. My Bad.

> People using requests are generally rather intelligent. Most have read the documentation and understand what they need to do when creating a valid multipart/form-data request. 

Thats great!

> It is often the people who do not read the documentation that run into this, go and read the documentation, and fix it themselves.

Can you point to to where in the documentation I would have seen the `content-type` behavior. I was able to find it because I looked at the code.


So to be clear, I think we all agree that this is problematic: the only disagreement is whether we believe the problem is leaving the header alone, or the silence of the behaviour.

I'm in favour of the current behaviour, possibly with a warning or a log.


> I'm in favour of the current behaviour, possibly with a warning or a log.

I think just something in the docs should suffice.

Something on the same lines as:

![image](https://cloud.githubusercontent.com/assets/1840162/13537247/f6556f3c-e211-11e5-9513-a4ce614aee74.png)


I was prepared to go further, with a runtime warning, but I'm totally happy to make a documentation change _right now_ to add warnings. We may actually want a whole section about headers that users generally shouldn't provide: `Host` is another.


If you explicitly provide something, we send it. You know better than us. This is a big design flaw (imo) in other HTTP libraries. 


FWIW, this becomes a much bigger problem when you are trying to Proxy requests and want to maintain most of the original headers. 

Having a list of headers that would potentially cause issues would be great for documentation.

@Lukasa big :+1: 


@Lukasa likely shouldn't provide _but are certainly able to, if needed_. This is a feature, not a bug.