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

fix: Use IOUtils toStream and toString #10975

Merged
merged 2 commits into from
May 12, 2021

Conversation

caalador
Copy link
Contributor

@caalador caalador commented May 12, 2021

Use IOUtils toString and toInputStream
to not change any characters in the file content.
Using streamToString adds new lines for each usage,
but this can not be done for a json file that contains
in the string content new lines.

fixes #10893

@caalador caalador added this to Iteration Reviews in OLD Vaadin Flow ongoing work (Vaadin 10+) via automation May 12, 2021
Use IOUtils toString and toInputStream
to not change any characters in the file content.

fixes #10893
@caalador caalador force-pushed the issues/10893_charset_problem branch from 7982bc6 to b8fa115 Compare May 12, 2021 09:02
@@ -650,8 +651,8 @@ private static InputStream getStatsFromClassPath(VaadinService service) {
Stats statistics = service.getContext().getAttribute(Stats.class);

if (statistics != null) {
return new ByteArrayInputStream(
statistics.statsJson.getBytes(StandardCharsets.UTF_8));
return IOUtils.toInputStream(statistics.statsJson,
Copy link
Contributor

Choose a reason for hiding this comment

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

This the implementation which IOUtils.toInputStream delegates to

 public static InputStream toInputStream(final String input, final Charset encoding) {
        return new ByteArrayInputStream(input.getBytes(Charsets.toCharset(encoding)));
    }

It's almost identical.

So this should not fix anything even though it may be considered as a small code improvement .

I guess the reason of changing chars is using UTF-8 as encoding while the json statistics is not in UTF-8 for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just to keep the usage similar and more readable.
The main fix to the issue is to not use streamToString(stream) but IOUtils.toString(stream, StandardCharsets.UTF_8)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK.

Copy link
Contributor

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

But are you sure this fixes anything ?
No way to make a unit test?

@@ -650,8 +651,8 @@ private static InputStream getStatsFromClassPath(VaadinService service) {
Stats statistics = service.getContext().getAttribute(Stats.class);

if (statistics != null) {
return new ByteArrayInputStream(
statistics.statsJson.getBytes(StandardCharsets.UTF_8));
return IOUtils.toInputStream(statistics.statsJson,
Copy link
Contributor

Choose a reason for hiding this comment

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

OK.

@caalador
Copy link
Contributor Author

Added test.

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 4 issues

  1. CRITICAL FrontendUtils.java#L1159: Either log or rethrow this exception. rule
  2. MINOR FrontendUtils.java#L478: Remove this use of "closeQuietly"; it is deprecated. rule
  3. MINOR FrontendUtils.java#L529: Remove this use of "closeQuietly"; it is deprecated. rule
  4. INFO FrontendUtils.java#L167: Do not forget to remove this deprecated code someday. rule

Copy link
Contributor

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

Does the test throw without the change?

Copy link
Contributor

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

OK, I see it fails

@denis-anisimov denis-anisimov merged commit a228eb2 into master May 12, 2021
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Iteration Reviews to Done - pending release May 12, 2021
@denis-anisimov denis-anisimov deleted the issues/10893_charset_problem branch May 12, 2021 12:24
vaadin-bot pushed a commit that referenced this pull request May 12, 2021
* fix: Use IOUtils toStream and toString

Use IOUtils toString and toInputStream
to not change any characters in the file content.

fixes #10893
vaadin-bot pushed a commit that referenced this pull request May 12, 2021
* fix: Use IOUtils toStream and toString

Use IOUtils toString and toInputStream
to not change any characters in the file content.

fixes #10893
vaadin-bot added a commit that referenced this pull request May 12, 2021
* fix: Use IOUtils toStream and toString

Use IOUtils toString and toInputStream
to not change any characters in the file content.

fixes #10893

Co-authored-by: caalador <mikael.grankvist@vaadin.com>
vaadin-bot added a commit that referenced this pull request May 12, 2021
* fix: Use IOUtils toStream and toString

Use IOUtils toString and toInputStream
to not change any characters in the file content.

fixes #10893

Co-authored-by: caalador <mikael.grankvist@vaadin.com>
Artur- pushed a commit that referenced this pull request May 18, 2021
* fix: Use IOUtils toStream and toString

Use IOUtils toString and toInputStream
to not change any characters in the file content.

fixes #10893
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 21.0.0.alpha2. For prerelease versions, it will be included in its final version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
OLD Vaadin Flow ongoing work (Vaadin ...
  
Done - pending release
Development

Successfully merging this pull request may close these issues.

JsonTokenizer fails in production mode with specific dependencies since 14.4.9
3 participants