Prevent okhttp from adding ;charset=utf8 to ContentType Header #23580

nhunzaker · 2019-02-21T22:51:13Z

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.

Before this commit, fetch() calls include "charset=utf8" on Android. This is because of an implementation detail in the okhttp library. Passing byte array into the RequestBody avoids this, as recommended by a maintainer of okhttp: square/okhttp#2099 (comment) Related issues: facebook#8237

nhunzaker · 2019-02-21T22:53:16Z

ReactAndroid/src/test/java/com/facebook/react/modules/network/NetworkingModuleTest.java

@@ -309,6 +309,55 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
    assertThat(requestHeaders.get("User-Agent")).isEqualTo("React test agent/1.0");
  }

+  @Test
+  public void testPostJsonContentTypeHeader() throws Exception {


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

hey99xx · 2019-02-21T23:05:32Z

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 · 2019-02-21T23:10:37Z

@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 · 2019-02-22T02:42:54Z

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

I was able to drop mocks for events and I limited the test assertions strictly to testing the content type.

nhunzaker · 2019-02-22T04:56:13Z

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 · 2019-02-22T05:00:36Z

@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 · 2019-02-22T05:18:44Z

@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 · 2019-02-22T06:36:03Z

Yes it definitely does matter ☺️

react-native-bot · 2019-02-22T06:40:05Z

@nhunzaker merged commit 4a80776 into facebook:master.

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: ```javascript 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 [Android][fixed] - Prevent fetch() POST requests on Android from appending `charset=utf-8` to `Content-Type` header. Pull Request resolved: #23580 Differential Revision: D14180849 Pulled By: cpojer fbshipit-source-id: b84cadf83361331a9f64d1ff5f2e6399a55527a6

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 21, 2019

nhunzaker mentioned this pull request Feb 21, 2019

Fetch adds charset=utf-8 on Android but not on iOS #8237

Closed

nhunzaker commented Feb 21, 2019

View reviewed changes

react-native-bot added the 🌐Networking Related to a networking API. label Feb 21, 2019

Pass standard utf8 character set into request body.getBytes()

85c6be4

Simplify content type test

7fb5278

I was able to drop mocks for events and I limited the test assertions strictly to testing the content type.

Clean up comment

f1636cc

cpojer approved these changes Feb 22, 2019

View reviewed changes

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 22, 2019

facebook-github-bot approved these changes Feb 22, 2019

View reviewed changes

hey99xx approved these changes Feb 22, 2019

View reviewed changes

facebook-github-bot closed this in 4a80776 Feb 22, 2019

facebook locked as resolved and limited conversation to collaborators Feb 22, 2019

react-native-bot added the Merged This PR has been merged. label Feb 22, 2019

hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent okhttp from adding ;charset=utf8 to ContentType Header #23580

Prevent okhttp from adding ;charset=utf8 to ContentType Header #23580

nhunzaker commented Feb 21, 2019 •

edited

Loading

nhunzaker Feb 21, 2019 •

edited

Loading

hey99xx commented Feb 21, 2019

nhunzaker commented Feb 21, 2019 •

edited

Loading

cpojer commented Feb 22, 2019

nhunzaker commented Feb 22, 2019

cpojer commented Feb 22, 2019

nhunzaker commented Feb 22, 2019

cpojer left a comment

facebook-github-bot left a comment

hey99xx left a comment

cpojer commented Feb 22, 2019

react-native-bot commented Feb 22, 2019

Prevent okhttp from adding ;charset=utf8 to ContentType Header #23580

Prevent okhttp from adding ;charset=utf8 to ContentType Header #23580

Conversation

nhunzaker commented Feb 21, 2019 • edited Loading

Summary

Changelog

Test Plan

nhunzaker Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

hey99xx commented Feb 21, 2019

nhunzaker commented Feb 21, 2019 • edited Loading

cpojer commented Feb 22, 2019

nhunzaker commented Feb 22, 2019

cpojer commented Feb 22, 2019

nhunzaker commented Feb 22, 2019

cpojer left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

hey99xx left a comment

Choose a reason for hiding this comment

cpojer commented Feb 22, 2019

react-native-bot commented Feb 22, 2019

nhunzaker commented Feb 21, 2019 •

edited

Loading

nhunzaker Feb 21, 2019 •

edited

Loading

nhunzaker commented Feb 21, 2019 •

edited

Loading