You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Jordan Ly <jo...@gmail.com> on 2018/02/22 23:23:38 UTC
Review Request 65761: Use StandardCharset instead of Charset.forName
in ApiModule
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65761/
-----------------------------------------------------------
Review request for Aurora and Renan DelValle.
Repository: aurora
Description
-------
Minor nit, use the StandardCharset constant for UTF-8 as opposed to creating it ourselves.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 09950a77427c7ed94090a036ae26dae2939aa2c1
Diff: https://reviews.apache.org/r/65761/diff/1/
Testing
-------
`./gradlew test`
Manually verified Firefox, Chrome, and Safari.
Thanks,
Jordan Ly
Re: Review Request 65761: Use StandardCharset instead of
Charset.forName in ApiModule
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65761/#review198151
-----------------------------------------------------------
Master (d95fc2f) is green with this patch.
./build-support/jenkins/build.sh
However, it appears that it might lack test coverage.
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Feb. 22, 2018, 11:23 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65761/
> -----------------------------------------------------------
>
> (Updated Feb. 22, 2018, 11:23 p.m.)
>
>
> Review request for Aurora and Renan DelValle.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Minor nit, use the StandardCharset constant for UTF-8 as opposed to creating it ourselves.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 09950a77427c7ed94090a036ae26dae2939aa2c1
>
>
> Diff: https://reviews.apache.org/r/65761/diff/1/
>
>
> Testing
> -------
>
> `./gradlew test`
>
> Manually verified Firefox, Chrome, and Safari.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 65761: Use StandardCharset instead of
Charset.forName in ApiModule
Posted by Renan DelValle <re...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65761/#review198375
-----------------------------------------------------------
Tested on incognito window in Chrome 64, private window in Firefox 58, and private window in Safari 11.0.3. Used incognito/private windows to avoid javascript caches influencing outcome.
Only thing I would change in this patch is modifying `testThriftJsonUtf8Accepted` in `org/apache/aurora/scheduler/http/api/ApiIT.java` to something like:
```
String upperUTF8 = StandardCharsets.UTF_8.name().toUpperCase();
String lowerUTF8 = StandardCharsets.UTF_8.name().toLowerCase();
// We also want to ensure charset parsing is case-insensitive because different browsers have
// different default behaviors (Chrome and Safari will change charset to all uppercase, while
// Firefox may leave it lowercase.
ClientResponse upperCaseUTF = getPlainRequestBuilder(ApiModule.API_PATH)
.type("application/vnd.apache.thrift.json; charset=" + upperUTF8)
.accept("application/vnd.apache.thrift.json; charset=" + upperUTF8)
.post(ClientResponse.class, JSON_FIXTURE);
assertEquals(SC_OK, upperCaseUTF.getStatus());
assertEquals(
"application/vnd.apache.thrift.json",
upperCaseUTF.getHeaders().getFirst(CONTENT_TYPE));
ClientResponse lowerCaseUTF = getPlainRequestBuilder(ApiModule.API_PATH)
.type("application/vnd.apache.thrift.json; charset=" + lowerUTF8)
.accept("application/vnd.apache.thrift.json; charset=" + lowerUTF8)
```
just so we are basing our test on something we depend on in the main source.
Other than that looks good to me!
- Renan DelValle
On Feb. 22, 2018, 3:23 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65761/
> -----------------------------------------------------------
>
> (Updated Feb. 22, 2018, 3:23 p.m.)
>
>
> Review request for Aurora and Renan DelValle.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Minor nit, use the StandardCharset constant for UTF-8 as opposed to creating it ourselves.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 09950a77427c7ed94090a036ae26dae2939aa2c1
>
>
> Diff: https://reviews.apache.org/r/65761/diff/1/
>
>
> Testing
> -------
>
> `./gradlew test`
>
> Manually verified Firefox, Chrome, and Safari.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 65761: Use StandardCharset instead of
Charset.forName in ApiModule
Posted by Renan DelValle <re...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65761/#review198150
-----------------------------------------------------------
Ship it!
Ship It!
- Renan DelValle
On Feb. 22, 2018, 3:23 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65761/
> -----------------------------------------------------------
>
> (Updated Feb. 22, 2018, 3:23 p.m.)
>
>
> Review request for Aurora and Renan DelValle.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Minor nit, use the StandardCharset constant for UTF-8 as opposed to creating it ourselves.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 09950a77427c7ed94090a036ae26dae2939aa2c1
>
>
> Diff: https://reviews.apache.org/r/65761/diff/1/
>
>
> Testing
> -------
>
> `./gradlew test`
>
> Manually verified Firefox, Chrome, and Safari.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 65761: Use StandardCharset instead of
Charset.forName in ApiModule
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65761/#review198398
-----------------------------------------------------------
Ship it!
Master (e3f496a) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Feb. 28, 2018, 7:13 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65761/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2018, 7:13 p.m.)
>
>
> Review request for Aurora and Renan DelValle.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Minor nit, use the StandardCharset constant for UTF-8 as opposed to creating it ourselves.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 09950a77427c7ed94090a036ae26dae2939aa2c1
> src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java 910af28eef27bc4fe98f17158b2ec545d5b93518
>
>
> Diff: https://reviews.apache.org/r/65761/diff/2/
>
>
> Testing
> -------
>
> `./gradlew test`
>
> Manually verified Firefox, Chrome, and Safari.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 65761: Use StandardCharset instead of
Charset.forName in ApiModule
Posted by Jordan Ly <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65761/
-----------------------------------------------------------
(Updated Feb. 28, 2018, 6:13 p.m.)
Review request for Aurora and Renan DelValle.
Changes
-------
Use StandardCharset in tests.
Repository: aurora
Description
-------
Minor nit, use the StandardCharset constant for UTF-8 as opposed to creating it ourselves.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 09950a77427c7ed94090a036ae26dae2939aa2c1
src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java 910af28eef27bc4fe98f17158b2ec545d5b93518
Diff: https://reviews.apache.org/r/65761/diff/2/
Changes: https://reviews.apache.org/r/65761/diff/1-2/
Testing
-------
`./gradlew test`
Manually verified Firefox, Chrome, and Safari.
Thanks,
Jordan Ly