Fix `DM_DEFAULT_ENCODING` SpotBugs violations #6050

basil · 2021-12-11T23:19:53Z

Fix DM_DEFAULT_ENCODING SpotBugs violations by explicitly passing in the default character set where possible (or StandardCharsets.UTF_8 for Unix-specific functionality).

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

(If applicable) Jira issue is well described
Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
- Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
Appropriate autotests or explanation to why this change has no tests
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

There are at least 2 approvals for the pull request and no outstanding requests for change
Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
Changelog entries in the PR title and/or Proposed changelog entries are correct
Proper changelog labels are set so that the changelog can be generated automatically
If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

basil · 2021-12-12T23:19:05Z

core/src/main/java/hudson/model/AbstractBuild.java

@@ -668,7 +669,7 @@ public void defaultCheckout() throws IOException, InterruptedException {
                    }
                } catch (AbortException e) {
                    listener.error(e.getMessage());
-                } catch (InterruptedIOException e) {
+                } catch (ClosedByInterruptException | InterruptedIOException e) {


The change to SCM.java elsewhere in this PR (from traditional Java I/O to NIO.2) means that cancelling a job while SCM checkout is taking place can now throw ClosedByInterruptException as well as InterruptedException and InterruptedIOException. Higher-level code marks the build as ABORTED for InterruptedException and FAILURE for other I/O exceptions, so we convert the ClosedByInterruptException to an InterruptedException here (just as the existing code does for InterruptedIOException) so that the build will be shown as ABORTED (as with the pre-NIO.2 logic).

oleg-nenashev

All changes look reasonable. PEMs files should always be UTF-8, same as Unix process files IIRC

jtnord

At least one spotbugs warning was masking a real issue.

I have looked at the other ones and can not say for certain that all of them are correct but they appear to be. Given this change would then mask the reports of potential issues it may be better to have a few more eyes on this than normal.
To be clear - this change does not introduce any bug - but as the spotbugs warnings are fixed, there is no longer a report to look into something.

cli/src/main/java/hudson/cli/CLIConnectionFactory.java

Co-authored-by: James Nord <[email protected]>

jtnord

I can not say for certain that the changes are all that all correct but they appear to be. Given this change would then mask the reports of potential issues it may be better to have a few more eyes on this than normal.

To be clear - this change does not introduce any bug - but as the spotbugs warnings are fixed, there is no longer a report to look into something.

basil · 2021-12-20T19:11:57Z

it may be better to have a few more eyes on this than normal.

Any other takers?

jglick

A number of places here where default charset is specified but I think UTF-8 would be more correct. I understand that you are actually making the more conservative change here—retaining existing behavior—but by making the warning go away and adding an explicit call to defaultCharset it makes it look like this was intentional, when generally it was not, thus making it less likely that the code would be corrected in the future compared to just leaving it alone (or adding a suppression annotation).

core/src/main/java/hudson/cli/CLICommand.java

core/src/main/java/hudson/console/ConsoleNote.java

core/src/main/java/hudson/security/csrf/DefaultCrumbIssuer.java

core/src/main/java/hudson/util/CompressedFile.java

core/src/main/java/hudson/util/ConsistentHash.java

core/src/main/java/hudson/util/LogTaskListener.java

core/src/main/java/jenkins/security/DefaultConfidentialStore.java

jglick

OK I think.

basil · 2021-12-20T22:06:05Z

OK I think.

Thanks. I'll run this through BOM testing before merging for good measure.

jglick · 2021-12-20T22:13:53Z

run this through BOM testing

Would not hurt, though I doubt PCT would catch interesting regressions here since all the CI machines should be using UTF-8 system encoding anyway.

basil · 2021-12-20T22:19:56Z

Would not hurt, though I doubt PCT would catch interesting regressions here since all the CI machines should be using UTF-8 system encoding anyway.

But re-running the tests after I merge the Windows CI PR should cover that.

jglick · 2021-12-21T16:20:11Z

Yes that should help. I suspect any actual regressions would be in unusual user configurations not covered by tests.

basil · 2021-12-22T00:17:27Z

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!

jglick · 2022-12-15T14:58:53Z

places here where default charset is specified but I think UTF-8 would be more correct

OTOH as of https://openjdk.org/jeps/400 it should not matter typically, so choice of Charset.defaultCharset() vs. StandardCharsets.UTF-8 vs. (where permitted without deprecation warning!) omitting a Charset or String charset argument would be a matter of style and legibility.

Fix DM_DEFAULT_ENCODING SpotBugs violations

a029cb7

basil added the skip-changelog Should not be shown in the changelog label Dec 11, 2021

basil added 3 commits December 12, 2021 09:27

Revert changes to SCM

67063e7

Restore changes to SCM; fix test bugs

ebb6700

Not a test bug after all

dbecf0a

basil commented Dec 12, 2021

View reviewed changes

basil added 2 commits December 16, 2021 06:55

Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING

703e597

Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING

a0d59f6

basil requested a review from a team December 17, 2021 16:09

oleg-nenashev approved these changes Dec 17, 2021

View reviewed changes

oleg-nenashev requested a review from a team December 17, 2021 22:16

basil added 2 commits December 18, 2021 14:18

Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING

76ef55a

Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING

a2661d8

basil requested a review from a team December 20, 2021 17:06

jtnord reviewed Dec 20, 2021

View reviewed changes

cli/src/main/java/hudson/cli/CLIConnectionFactory.java Outdated Show resolved Hide resolved

cli/src/main/java/hudson/cli/CLIConnectionFactory.java Outdated Show resolved Hide resolved

basil and others added 2 commits December 20, 2021 10:21

Update cli/src/main/java/hudson/cli/CLIConnectionFactory.java

032a3fa

Co-authored-by: James Nord <[email protected]>

Update cli/src/main/java/hudson/cli/CLIConnectionFactory.java

181119a

Co-authored-by: James Nord <[email protected]>

basil requested a review from a team December 20, 2021 18:43

basil added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Dec 20, 2021

jtnord approved these changes Dec 20, 2021

View reviewed changes

basil requested a review from a team December 20, 2021 19:11

jglick requested changes Dec 20, 2021

View reviewed changes

basil added 7 commits December 20, 2021 13:34

Use getClientCharset() in CLICommand

5876b03

Use UTF-8 in ConsoleNote

f7628b3

Let us use ASCII here perhaps

0684c9c

Use UTF-8 in HudsonAuthenticationEntryPoint

7e51696

Use UTF-8 in DefaultCrumbIssuer

265558f

Salt is a hex-string, so US_ASCII works

d6904a2

Deprecate CompressedFile#loadAsString

7d47fa1

basil added 4 commits December 20, 2021 13:47

Use UTF-8 in ConsistentHash

b14c624

Use UTF-8 in LogTaskListener

ebbc692

Use ASCII for DefaultConfidentialStore

ae5a020

Missed one

af9dccc

basil requested a review from jglick December 20, 2021 21:57

jglick approved these changes Dec 20, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING

9da3b4a

Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING

53a4a6e

basil added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 22, 2021

basil added 2 commits December 22, 2021 06:16

Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING

6d5d610

Get a new build

a7ff6ad

basil merged commit 8a5e309 into jenkinsci:master Dec 23, 2021

basil deleted the DM_DEFAULT_ENCODING branch December 23, 2021 04:34

basil mentioned this pull request Mar 13, 2022

Restore original method signature of CLICommand#getClientCharset #6370

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `DM_DEFAULT_ENCODING` SpotBugs violations #6050

Fix `DM_DEFAULT_ENCODING` SpotBugs violations #6050

basil commented Dec 11, 2021

basil Dec 12, 2021

oleg-nenashev left a comment

jtnord left a comment

jtnord left a comment

basil commented Dec 20, 2021

jglick left a comment •

edited

Loading

jglick left a comment

basil commented Dec 20, 2021

jglick commented Dec 20, 2021

basil commented Dec 20, 2021

jglick commented Dec 21, 2021

basil commented Dec 22, 2021

jglick commented Dec 15, 2022

Fix DM_DEFAULT_ENCODING SpotBugs violations #6050

Fix DM_DEFAULT_ENCODING SpotBugs violations #6050

Conversation

basil commented Dec 11, 2021

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

basil Dec 12, 2021

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

jtnord left a comment

Choose a reason for hiding this comment

jtnord left a comment

Choose a reason for hiding this comment

basil commented Dec 20, 2021

jglick left a comment • edited Loading

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

basil commented Dec 20, 2021

jglick commented Dec 20, 2021

basil commented Dec 20, 2021

jglick commented Dec 21, 2021

basil commented Dec 22, 2021

jglick commented Dec 15, 2022

Fix `DM_DEFAULT_ENCODING` SpotBugs violations #6050

Fix `DM_DEFAULT_ENCODING` SpotBugs violations #6050

jglick left a comment •

edited

Loading