Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use existing character set in POST body when possible #23603

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -372,7 +372,13 @@ public void onProgress(long bytesWritten, long contentLength, boolean done) {
return;
}
} else {
requestBody = RequestBody.create(contentMediaType, body.getBytes(StandardCharsets.UTF_8));
// Use getBytes() to convert the body into a byte[], preventing okhttp from
// appending the character set to the Content-Type header when otherwise unspecified
// https://github.com/facebook/react-native/issues/8237
Charset charset = contentMediaType == null
? StandardCharsets.UTF_8
: contentMediaType.charset(StandardCharsets.UTF_8);
requestBody = RequestBody.create(contentMediaType, body.getBytes(charset));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hey99xx I updated this line to include your suggestion for null safety. Just for posterity too, this matches how other parts of the code are handling this case:

Charset charset = responseBody.contentType() == null ? StandardCharsets.UTF_8 :
responseBody.contentType().charset(StandardCharsets.UTF_8);

}
} else if (data.hasKey(REQUEST_BODY_KEY_BASE64)) {
if (contentType == null) {
Expand Down
Expand Up @@ -18,6 +18,7 @@
import com.facebook.react.bridge.ReactContext;
import com.facebook.react.bridge.WritableArray;
import com.facebook.react.bridge.WritableMap;
import com.facebook.react.common.StandardCharsets;
import com.facebook.react.common.network.OkHttpCallUtil;
import com.facebook.react.modules.core.DeviceEventManagerModule.RCTDeviceEventEmitter;

Expand Down Expand Up @@ -346,6 +347,94 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
assertThat(argumentCaptor.getValue().body().contentType().toString()).isEqualTo("application/json");
}

@Test
public void testRespectsExistingCharacterSet() throws Exception {
RCTDeviceEventEmitter emitter = mock(RCTDeviceEventEmitter.class);
ReactApplicationContext context = mock(ReactApplicationContext.class);
when(context.getJSModule(any(Class.class))).thenReturn(emitter);

OkHttpClient httpClient = mock(OkHttpClient.class);
when(httpClient.newCall(any(Request.class))).thenAnswer(new Answer<Object>() {
@Override
public Object answer(InvocationOnMock invocation) throws Throwable {
Call callMock = mock(Call.class);
return callMock;
}
});
OkHttpClient.Builder clientBuilder = mock(OkHttpClient.Builder.class);
when(clientBuilder.build()).thenReturn(httpClient);
when(httpClient.newBuilder()).thenReturn(clientBuilder);
NetworkingModule networkingModule = new NetworkingModule(context, "", httpClient);

JavaOnlyMap body = new JavaOnlyMap();
body.putString("string", "Friðjónsson");

mockEvents();

networkingModule.sendRequest(
"POST",
"http://somedomain/bar",
0,
JavaOnlyArray.of(JavaOnlyArray.of("Content-Type", "text/plain; charset=utf-16")),
body,
/* responseType */ "text",
/* useIncrementalUpdates*/ true,
/* timeout */ 0,
/* withCredentials */ false);

ArgumentCaptor<Request> argumentCaptor = ArgumentCaptor.forClass(Request.class);
verify(httpClient).newCall(argumentCaptor.capture());

Buffer contentBuffer = new Buffer();
argumentCaptor.getValue().body().writeTo(contentBuffer);
assertThat(contentBuffer.readString(StandardCharsets.UTF_16)).isEqualTo("Friðjónsson");
}

@Test
public void testGracefullyRecoversFromInvalidContentType() throws Exception {
RCTDeviceEventEmitter emitter = mock(RCTDeviceEventEmitter.class);
ReactApplicationContext context = mock(ReactApplicationContext.class);
when(context.getJSModule(any(Class.class))).thenReturn(emitter);

OkHttpClient httpClient = mock(OkHttpClient.class);
when(httpClient.newCall(any(Request.class))).thenAnswer(new Answer<Object>() {
@Override
public Object answer(InvocationOnMock invocation) throws Throwable {
Call callMock = mock(Call.class);
return callMock;
}
});
OkHttpClient.Builder clientBuilder = mock(OkHttpClient.Builder.class);
when(clientBuilder.build()).thenReturn(httpClient);
when(httpClient.newBuilder()).thenReturn(clientBuilder);
NetworkingModule networkingModule = new NetworkingModule(context, "", httpClient);

JavaOnlyMap body = new JavaOnlyMap();
body.putString("string", "test");

mockEvents();

networkingModule.sendRequest(
"POST",
"http://somedomain/bar",
0,
JavaOnlyArray.of(JavaOnlyArray.of("Content-Type", "invalid")),
body,
/* responseType */ "text",
/* useIncrementalUpdates*/ true,
/* timeout */ 0,
/* withCredentials */ false);

ArgumentCaptor<Request> argumentCaptor = ArgumentCaptor.forClass(Request.class);
verify(httpClient).newCall(argumentCaptor.capture());

Buffer contentBuffer = new Buffer();
argumentCaptor.getValue().body().writeTo(contentBuffer);

assertThat(contentBuffer.readString(StandardCharsets.UTF_8)).isEqualTo("test");
assertThat(argumentCaptor.getValue().header("Content-Type")).isEqualTo("invalid");
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hey99xx I also added this test case to cover the null pointer exception instance on bad content types. It asserts that the content is still parseable as UTF8 and the Content Type header is preserved.


@Test
public void testMultipartPostRequestSimple() throws Exception {
PowerMockito.mockStatic(RequestBodyUtil.class);
Expand Down