fix: default charset to UTF-8 for text/csv if not specified #1423

rohitvvv · 2021-08-05T23:19:34Z

Some servers don't return the charset. This causes german
characters to be encoded incorrectly, since ISO_8859_1 does not
work very well in such cases defaulting to UTF-8 if its missing.

https://www.iana.org/assignments/media-types/text/csv

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #1421 ☕️

Some servers don't return the charset. This causes german characters to be encoded incorrectly, since ISO_8859_1 does not work very well in such cases defaulting to UTF-8 if its missing. https://www.iana.org/assignments/media-types/text/csv

chingor13

LGTM, with 2 super-minor nits.

chingor13 · 2021-08-10T16:55:05Z

google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java

@@ -534,6 +534,10 @@ public Charset getContentCharset() {
        // https://tools.ietf.org/html/rfc4627 - JSON must be encoded with UTF-8
        return StandardCharsets.UTF_8;
      }
+       // fallback to well-kown charset for text/csv


nit:

Suggested change

// fallback to well-kown charset for text/csv

// fallback to well-kown charset for text/csv

chingor13 · 2021-08-10T16:56:14Z

google-http-client/src/main/java/com/google/api/client/http/HttpResponse.java

+      if ("text".equals(mediaType.getType()) && "csv".equals(mediaType.getSubType())) {
+        return StandardCharsets.UTF_8;


nit: can we add a comment with the link to the spec in case someone browsing the code wonders why this is here (without having to do a git blame and track down this PR through history).

chingor13

Also, please run mvn com.coveo:fmt-maven-plugin:format to fix the format/linter.

Fixed review comments. Thanks

rohitvvv · 2021-08-10T17:24:21Z

Thank you for the review. I have updated the code.

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 5, 2021

rohitvvv force-pushed the csvutf8 branch from 765b1a7 to 052fabe Compare August 5, 2021 23:22

rohitvvv marked this pull request as ready for review August 5, 2021 23:25

rohitvvv requested a review from a team as a code owner August 5, 2021 23:25

chingor13 requested changes Aug 10, 2021

View reviewed changes

chingor13 changed the title ~~Defaulting to UTF-8 if charset is missing~~ fix: default charset to UTF-8 for text/csv if not specified Aug 10, 2021

chingor13 reviewed Aug 10, 2021

View reviewed changes

Fix: Defaulting to UTF-8 if charset is missing

d32902f

Fixed review comments. Thanks

chingor13 approved these changes Aug 11, 2021

View reviewed changes

chingor13 added the automerge Merge the pull request once unit tests and other checks pass. label Aug 11, 2021

gcf-merge-on-green bot merged commit 26f3da4 into googleapis:master Aug 11, 2021

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: default charset to UTF-8 for text/csv if not specified #1423

fix: default charset to UTF-8 for text/csv if not specified #1423

rohitvvv commented Aug 5, 2021 •

edited

chingor13 left a comment

chingor13 Aug 10, 2021

rohitvvv Aug 10, 2021

chingor13 Aug 10, 2021

rohitvvv Aug 10, 2021

chingor13 left a comment

rohitvvv commented Aug 10, 2021

	// fallback to well-kown charset for text/csv
	// fallback to well-kown charset for text/csv

		if ("text".equals(mediaType.getType()) && "csv".equals(mediaType.getSubType())) {
		return StandardCharsets.UTF_8;

fix: default charset to UTF-8 for text/csv if not specified #1423

fix: default charset to UTF-8 for text/csv if not specified #1423

Conversation

rohitvvv commented Aug 5, 2021 • edited

chingor13 left a comment

Choose a reason for hiding this comment

chingor13 Aug 10, 2021

Choose a reason for hiding this comment

rohitvvv Aug 10, 2021

Choose a reason for hiding this comment

chingor13 Aug 10, 2021

Choose a reason for hiding this comment

rohitvvv Aug 10, 2021

Choose a reason for hiding this comment

chingor13 left a comment

Choose a reason for hiding this comment

rohitvvv commented Aug 10, 2021

rohitvvv commented Aug 5, 2021 •

edited