Further reduce usages of `StringUtils` #9044

BobDu · 2024-03-16T14:13:16Z

like #6270

Testing done

Proposed changelog entries

JENKINS-XXXXX, human-readable text

Proposed upgrade guidelines

N/A

Submitter checklist

Give feedback

The Jira issue, if it exists, is well-described.
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
There is automated testing or an explanation as to why this change has no tests.
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
For dependency updates, there are links to external changelogs and, if possible, full differentials.
For new APIs and extension points, there is a link to at least one consumer.
Options

Desired reviewers

@mention

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

Maintainer checklist

Give feedback

There are at least two (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 is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see 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).
Options

StefanSpieker

I briefly reviewed this PR, I'm not sure if all replacements are equivalent. At least the StringUtils.defaultString should be replaced by the suggestion - not sure that all imports still fit

cli/src/main/java/hudson/cli/CLI.java

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

core/src/main/java/hudson/tasks/BuildTrigger.java

Signed-off-by: Bob Du <[email protected]>

basil · 2024-03-22T18:38:13Z

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

+            if (o.endsWith(removeSuffix2)) {
+                o = o.substring(0, o.length() - removeSuffix2.length());
+            }
+            final String expectedOrigin = o;


The extra o variable seems like an unnecessary layer of indirection for the reader. Why not just name it expectedOrigin to begin with and remove this line.

expectedOrigin is a local variable used in lambda, it must be a final or effectively final.

Of course we can set o as another name for readability, but I think it is not very necessary as a temporary variable.

core/src/main/java/hudson/PluginManager.java

Signed-off-by: Bob Du <[email protected]>

basil

Looks great! Many thanks for this contribution. Just two minor comments and I am ready to approve this PR.

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

core/src/main/java/hudson/search/FixedSet.java

basil

Many thanks for the PR!

MarkEWaite

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.

/label ready-for-merge

jglick · 2024-04-04T17:03:55Z

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

@@ -65,7 +64,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
        HttpServletResponse rsp = (HttpServletResponse) response;
        String authorization = req.getHeader("Authorization");

-        if (StringUtils.startsWithIgnoreCase(authorization, "Basic ")) {
+        if (authorization != null && authorization.toLowerCase().startsWith("Basic ".toLowerCase())) {


While it is unlikely to matter in practice, this is not a sound replacement. If a request was sent with

AUTHORIZATION: BASIC czNjcjN0

then a server running in Turkish locale (e.g., #9066, though here it is the server not client locale which matters) would fail this clause because

"basıc cznjcjn0".startsWith("basic ")

is false.

In general, you always want to pass the Locale argument (almost always Locale.ROOT) to toLowerCase / toUpperCase.

@BobDu Is it worth filing a follow-up PR to address the above feedback? Or do we think the code is fine as-is?

I agree always add Locale.ROOT arg to toLowerCase method is a best practices.
Already make a new PR #9162

MarkEWaite added the skip-changelog Should not be shown in the changelog label Mar 16, 2024

NotMyFault requested a review from a team March 18, 2024 14:58

StefanSpieker reviewed Mar 21, 2024

View reviewed changes

cli/src/main/java/hudson/cli/CLI.java Show resolved Hide resolved

core/src/main/java/hudson/model/Slave.java Outdated Show resolved Hide resolved

core/src/main/java/hudson/tasks/BuildTrigger.java Outdated Show resolved Hide resolved

BobDu added 12 commits March 22, 2024 19:23

Remove usages of StringUtils#containsIgnoreCase

f31afb2

Signed-off-by: Bob Du <[email protected]>

Remove usages of StringUtils#removeStart StringUtils#removeEnd

aa5e0af

Signed-off-by: Bob Du <[email protected]>

Remove usages of StringUtils#defaultString

aeb5060

Signed-off-by: Bob Du <[email protected]>

Remove usages of StringUtils#defaultIfBlank

6434727

Signed-off-by: Bob Du <[email protected]>

Remove usages of StringUtils#isAlphanumeric

b817155

Signed-off-by: Bob Du <[email protected]>

Remove usages of StringUtils.EMPTY

e7ee9bb

Signed-off-by: Bob Du <[email protected]>

Remove usages of StringUtils#split

0fa773e

Signed-off-by: Bob Du <[email protected]>

Remove usages of StringUtils#startsWithIgnoreCase

e942cc1

Signed-off-by: Bob Du <[email protected]>

Remove usages of StringUtils#countMatches

c3f10bd

Signed-off-by: Bob Du <[email protected]>

Remove usages of StringUtils#endsWithAny

99ccb39

Signed-off-by: Bob Du <[email protected]>

fix spotbugs warning

12a0efc

Signed-off-by: Bob Du <[email protected]>

Use Objects.toString

744f3db

Signed-off-by: Bob Du <[email protected]>

BobDu force-pushed the reduce-commons-lang branch from f26ce3d to 744f3db Compare March 22, 2024 11:23

basil reviewed Mar 22, 2024

View reviewed changes

revert unrelated change

9e9e714

Signed-off-by: Bob Du <[email protected]>

basil reviewed Mar 25, 2024

View reviewed changes

core/src/main/java/hudson/model/AutoCompletionCandidates.java Show resolved Hide resolved

core/src/main/java/hudson/search/FixedSet.java Show resolved Hide resolved

basil approved these changes Mar 26, 2024

View reviewed changes

MarkEWaite approved these changes Mar 26, 2024

View reviewed changes

comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Mar 26, 2024

MarkEWaite merged commit 8197702 into jenkinsci:master Mar 27, 2024
17 checks passed

jglick reviewed Apr 4, 2024

View reviewed changes

BobDu mentioned this pull request Apr 12, 2024

Set Locale.ROOT when use String.toLowerCase #9162

Merged

basil mentioned this pull request Apr 30, 2024

Set Locale.ROOT when use String.toLowerCase #9215

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further reduce usages of `StringUtils` #9044

Further reduce usages of `StringUtils` #9044

BobDu commented Mar 16, 2024

Submitter checklist

Maintainer checklist

StefanSpieker left a comment

basil Mar 22, 2024

BobDu Mar 23, 2024

basil left a comment

basil left a comment

MarkEWaite left a comment

jglick Apr 4, 2024 •

edited

Loading

basil Apr 11, 2024

BobDu Apr 12, 2024

Further reduce usages of StringUtils #9044

Further reduce usages of StringUtils #9044

Conversation

BobDu commented Mar 16, 2024

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

StefanSpieker left a comment

Choose a reason for hiding this comment

basil Mar 22, 2024

Choose a reason for hiding this comment

BobDu Mar 23, 2024

Choose a reason for hiding this comment

basil left a comment

Choose a reason for hiding this comment

basil left a comment

Choose a reason for hiding this comment

MarkEWaite left a comment

Choose a reason for hiding this comment

jglick Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

basil Apr 11, 2024

Choose a reason for hiding this comment

BobDu Apr 12, 2024

Choose a reason for hiding this comment

Further reduce usages of `StringUtils` #9044

Further reduce usages of `StringUtils` #9044

jglick Apr 4, 2024 •

edited

Loading