Prevent okhttp from adding ;charset=utf8 to ContentType Header

[nhunzaker]

Summary

Before this commit, fetch() calls append "charset=utf8" to the Content-Type header on Android (and not on iOS). This is because of an implementation detail in the okhttp library. This means that you can make a call on the JavaScript side like:

let body = JSON.stringify({ key: "value" });

let headers = {
  "Content-Type": "application/json"
};

fetch("http://10.0.2.2:3000", { method: "POST", body, headers });

However the resulting request appends the utf8 character:

POST - 13:34:32:
  content-type: application/json; charset=utf-8
  content-length: 15
  host: 10.0.2.2:3000
  connection: Keep-Alive
  accept-encoding: gzip
  user-agent: okhttp/3.12.1

Passing byte array into the RequestBody avoids this, as recommended by a maintainer of okhttp:

square/okhttp#2099 (comment)

Related issues:
#8237

Changelog

[Android][fixed] - Prevent fetch() POST requests on Android from appending charset=utf-8 to Content-Type header.

Test Plan

I setup a test project here: https://github.com/nhunzaker/rn-charset-issue. It includes a simple node server that logs header output from requests. With this change, the server no longer reports the addition of charset=utf-8 in the Content-Type header.

[nhunzaker]

I copied this from previous tests. It's possible more of the mocks below can be removed. Any thoughts?

[hey99xx]

This is actually a pretty simple fix for a long going problem for many 👍
Can we still specify utf8 as the charset for the bytes though?

import com.facebook.react.common.StandardCharsets;

body.getBytes(StandardCharsets.UTF_8)

For unit tests, you're heavily mocking OkHttp. Was the test actually failing as intended when you undo your fix?

[nhunzaker]

@hey99xx good deal, 👍. I wondered about that, or passing in the character set as defined by MediaType.parse.

The new test fails without this change. I get the following failure:

org.junit.ComparisonFailure: expected:<null> but was:<UTF-8>
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)

I'd love to actually assert the headers that OkHTTP is building. This was the closest I could manage to get.

[cpojer]

@nhunzaker I'm not sure I understand your last comment. Can you confirm that @hey99xx's concern is addressed?

[nhunzaker]

Regarding:

For unit tests, you're heavily mocking OkHttp. Was the test actually failing as intended when you undo your fix?

Yes, I believe I have. While the test heavily mocks OkHttp, the important part to test is the stringification of the MediaType class, which is not mocked.

I sent out another commit that I hope simplifies the test and makes that clearer (https://github.com/facebook/react-native/pull/23580/files#diff-0a0a7fd9969f1f5f68d80bf421fcc092R346).

Without my change, this fails, returning "application/json; charset=utf-8"

[cpojer]

@nhunzaker I mean this part: "Can we still specify utf8 as the charset for the bytes though?" – I assume the answer is yes, just wanna hear the confirmation.

[nhunzaker]

@cpojer Oh! Sorry, yep!

This line should do it:
https://github.com/facebook/react-native/pull/23580/files#diff-f8703d50cf5af4c06824e2928dddb500R375

[cpojer]

Awesome, thanks for clarifying. Let's ship it.

[facebook-github-bot]

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[hey99xx]

Not sure if my approval matters after @cpojer 's, but here you go anyways

[cpojer]

Yes it definitely does matter ☺️

[react-native-bot]

@nhunzaker merged commit 4a80776 into facebook:master.