Skip to content

fix: Use UTF-8 as default charset for HttpJson requests #1477

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

Merged
merged 12 commits into from
Mar 20, 2023
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ HttpRequest createHttpRequest() throws IOException {
jsonFactory.createJsonParser(requestBody).parse(tokenRequest);
jsonHttpContent =
new JsonHttpContent(jsonFactory, tokenRequest)
.setMediaType((new HttpMediaType("application/json")));
.setMediaType((new HttpMediaType("application/json; charset=utf-8")));
Copy link
Collaborator

@blakeli0 blakeli0 Mar 13, 2023

Choose a reason for hiding this comment

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

The fix looks good to me! This makes me think though, we are currently hardcoding all content type to application/json, which is fine for now, but it will not work for services like storage in the future. It might be one of the reasons that storage has been a handwritten library. CC @meltsufin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For my curiosity: What content-type does Storage use? Wondering if it's as easy as setting the charset or having to create/ modify the HttpMediaType class.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It depends on the use case, in their example, it is set to text/plain. We probably need to make it a parameter in the future and set it differently based on the scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll create an issue to track this. Might be something that a future GAPIC library needs. Also, don't the handwritten libraries use java-core instead of gax?

Copy link
Collaborator

Choose a reason for hiding this comment

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

They use both, also we are thinking about reducing the need of handwritten libraries(including java-core), so this could be one of places. I wouldn't create an issue yet as we don't have the need yet, but something to keep in mind.

} else {
// Force underlying HTTP lib to set Content-Length header to avoid 411s.
// See EmptyContent.java.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
import com.google.longrunning.ListOperationsRequest;
import com.google.protobuf.Empty;
import com.google.protobuf.Field;
import com.google.protobuf.util.JsonFormat;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.util.Arrays;
import java.util.HashMap;
Expand Down Expand Up @@ -177,4 +179,67 @@ public void testRequestUrlUnnormalizedPatch() throws IOException {
Truth.assertThat(httpRequest.getRequestMethod()).isEqualTo("POST");
Truth.assertThat(httpRequest.getHeaders().get("X-HTTP-Method-Override")).isEqualTo("PATCH");
}

/*
We use a separate RequestFormatter because formatting the body requests is what sets the charset to be UTF-8.
The other tests above do not have a set a body request and instead send an EmptyContent (null Type/ CharSet)
*/
@Test
public void testUnicodeValuesInBody() throws IOException {
HttpRequestFormatter<Field> bodyRequestFormatter =
ProtoMessageRequestFormatter.<Field>newBuilder()
.setPath(
"/name/{name=*}",
request -> {
Map<String, String> fields = new HashMap<>();
ProtoRestSerializer<Field> serializer = ProtoRestSerializer.create();
serializer.putPathParam(fields, "name", request.getName());
return fields;
})
.setQueryParamsExtractor(request -> new HashMap<>())
.setRequestBodyExtractor(
request ->
ProtoRestSerializer.create().toBody("*", request.toBuilder().build(), true))
.build();

Field bodyRequestMessage =
Field.newBuilder()
.setName("feline ☺ → ←")
.setNumber(2)
.setDefaultValue("bird ☺ → ←")
.setJsonName("mouse ☺ → ←")
.setTypeUrl("small ☺ → ←")
.build();

ApiMethodDescriptor<Field, Empty> methodDescriptor =
ApiMethodDescriptor.<Field, Empty>newBuilder()
.setFullMethodName("house.cat.get")
.setHttpMethod("PUT")
.setRequestFormatter(bodyRequestFormatter)
.setResponseParser(responseParser)
.build();

HttpRequestRunnable<Field, Empty> httpRequestRunnable =
new HttpRequestRunnable<>(
bodyRequestMessage,
methodDescriptor,
"www.googleapis.com/animals/v1/projects",
HttpJsonCallOptions.newBuilder().build(),
new MockHttpTransport(),
HttpJsonMetadata.newBuilder().build(),
(result) -> {});

HttpRequest httpRequest = httpRequestRunnable.createHttpRequest();
Truth.assertThat(httpRequest.getContent().getType())
.isEqualTo("application/json; charset=utf-8");
try (ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream()) {
// writeTo() uses the Charset when writing to the stream
httpRequest.getContent().writeTo(byteArrayOutputStream);
String output = byteArrayOutputStream.toString();
Field.Builder expectedBuilder = Field.newBuilder();
JsonFormat.parser().merge(output, expectedBuilder);
Field result = expectedBuilder.build();
Truth.assertThat(result).isEqualTo(bodyRequestMessage);
}
}
}