replace usages of commons.lang3 and commons.text #217

ampuscas · 2022-10-10T09:41:30Z

Removing dependency of commons-lang3 and replacing with standard JDK11 methods. Keeping the commons-text dependency only for test scope. Also, I override the jenkins baseline in order to be able to use Java11 methods

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

raul-arabaolaza

Please add a description to the pr and why is needed, also see my comments, it seems this changes one dependency for another, are the changed methods really doing the same? There may be subtle differences, also please check if you can just replace with standard JDK methods instead of changing the dependency

raul-arabaolaza · 2022-10-10T09:57:35Z

src/main/java/io/jenkins/plugins/util/EnvironmentResolver.java

@@ -45,7 +43,7 @@ public EnvironmentResolver() {
    public String expandEnvironmentVariables(@CheckForNull final EnvVars environment, final String nonExpandedValue) {
        String expanded = nonExpandedValue;
        if (environment != null && !environment.isEmpty()) {
-            for (int i = 0; i < resolveVariablesDepth && StringUtils.isNotBlank(expanded); i++) {
+            for (int i = 0; i < resolveVariablesDepth && StringUtils.hasText(expanded) && StringUtils.hasLength(expanded); i++) {


I do believe is better to check for the actual code of isNotBlanck. Probably is small enough to replicate here without moving the dependency from commons-lang to spring

raul-arabaolaza · 2022-10-10T09:59:54Z

src/main/java/io/jenkins/plugins/util/JenkinsFacade.java

@@ -269,7 +269,7 @@ private String getContextPath() {
     * @return the absolute URL
     */
    public String getAbsoluteUrl(final String... urlElements) {
-        return getAbsoluteUrl(StringUtils.join(urlElements, "/"));
+        return getAbsoluteUrl(StringUtils.arrayToDelimitedString(urlElements, "/"));


Same as before, or probably you can do this with java Streams

raul-arabaolaza · 2022-10-10T10:02:52Z

src/test/java/io/jenkins/plugins/util/FormValidationAssert.java

        if (!Objects.equals(actualMessage, expectedMessage)) {
            failWithMessage(EXPECTED_BUT_WAS_MESSAGE, "message",
-                    StringEscapeUtils.unescapeHtml4(actual.toString()), expectedMessage, actualMessage);
+                    HtmlUtils.htmlUnescape(actual.toString()), expectedMessage, actualMessage);


Same as before, also, are this methods doing the same thing?

jtnord · 2022-10-10T09:52:44Z

src/main/java/io/jenkins/plugins/util/EnvironmentResolver.java

@@ -45,7 +43,7 @@ public EnvironmentResolver() {
    public String expandEnvironmentVariables(@CheckForNull final EnvVars environment, final String nonExpandedValue) {
        String expanded = nonExpandedValue;
        if (environment != null && !environment.isEmpty()) {
-            for (int i = 0; i < resolveVariablesDepth && StringUtils.isNotBlank(expanded); i++) {
+            for (int i = 0; i < resolveVariablesDepth && StringUtils.hasText(expanded) && StringUtils.hasLength(expanded); i++) {


!String.isBlank() in Java 11 (which if using a recent core this plugin should be able to use (if not should be possible to update it)

jtnord · 2022-10-10T09:53:41Z

src/main/java/io/jenkins/plugins/util/JenkinsFacade.java

@@ -269,7 +269,7 @@ private String getContextPath() {
     * @return the absolute URL
     */
    public String getAbsoluteUrl(final String... urlElements) {
-        return getAbsoluteUrl(StringUtils.join(urlElements, "/"));
+        return getAbsoluteUrl(StringUtils.arrayToDelimitedString(urlElements, "/"));


use inbuilt j2se feature String.join("/", urlElements) ?

jtnord · 2022-10-10T09:54:47Z

src/test/java/io/jenkins/plugins/util/FormValidationAssert.java

@@ -88,10 +87,10 @@ public FormValidationAssert isOk() {
    public FormValidationAssert hasMessage(final String expectedMessage) {
        isNotNull();

-        String actualMessage = StringEscapeUtils.unescapeHtml4(actual.getMessage());
+        String actualMessage = HtmlUtils.htmlUnescape(actual.getMessage());


I don't think we should be using org.springframework.web

As this is a test class - leaving the commons text and using the dependency in test scope should be ok.

Alternatively without checking what happens here - there may well be a DOM class in HTMLUnit that could be used.

Having said that https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringEscapeUtils.html#unescapeHtml4-java.lang.String- is deprecated - so changing it for something else also seems reasonable.

uhafner · 2022-10-10T10:33:59Z

Can you please explain the reasoning for this PR? Both dependencies are used by downstream plugins so it makes no sense to remove the dependency here.

uhafner

As mentioned in my previous comment, I do not understand the reasoning for this PR. StringUtils is a well known, stable and high quality component. Did you encounter any problems with the current code? Or why are you replacing it?

uhafner · 2022-10-10T20:28:57Z

pom.xml

+    <jenkins.baseline>2.361</jenkins.baseline>
+    <jenkins.version>${jenkins.baseline}.2</jenkins.version>


I understand that you want to use some JDK 11 features but these changes in the PR are not worth of such a huge user impact right now (maybe in a couple of weeks).

uhafner · 2022-10-10T20:29:55Z

pom.xml

-    <dependency>
-      <groupId>io.jenkins.plugins</groupId>
-      <artifactId>commons-lang3-api</artifactId>
-    </dependency>
-    <dependency>
-      <groupId>io.jenkins.plugins</groupId>
-      <artifactId>commons-text-api</artifactId>
-    </dependency>


These dependencies are used by all downstream plugins. We cannot remove them without breaking the downstream plugins.

These dependencies are used by all downstream plugins.

They are in use by some of the downstream dependencies, but not all.

We cannot remove them without breaking the downstream plugins.

if they are used by downstream plugins - then it would be a bug in those plugins to not declare a dependency on these plugins in their pom, they need to be fixed, but that is irrespective of this PR.

We cannot remove them without breaking the downstream plugins.

if they are used by downstream plugins - then it would be a bug in those plugins to not declare a dependency on these plugins in their pom, they need to be fixed, but that is irrespective of this PR.

Why is this a bug? This is a feature of Maven. Commons lang (and some other dependencies) are provided by this plugin-util library so that you do not need to explicitly need to declare those dependencies in your plugins. So there is only a single place to change those dependencies.

Why is this a bug? This is a feature of Maven

the feature I think you are talking about is transitive dependencies.

The maven feature will give you transitive dependencies that 3rd parties use in your classpath, this is so when compliling/testing your code it will work when run with the library that needs that code.

Maven avoids the need to discover and specify the libraries that your own dependencies require by including transitive dependencies automatically. (emphasis mine)

However if you also use those dependencies directly you are only getting the dependency via the 3rd party thus if the 3rd party changes you will end up with a broken build because you are using the library and not declaring it, and this is then incorrect.

To see this in action (and why it is incorrect) run mvn dependency:analyze and pay attention to the warning "used but not declared".

If you use something you should always declare it.

(now dependencies coming from Jenkins itself are somewhat special because of the way Jenkins and the hpi plugin themselves abuse some maven constructs - but for library and plugin dependencies you should never rely on transitive dependencies if you directly use something yourself).

See also https://books.sonatype.com/mvnex-book/reference/optimizing-sect-dependency-plugin.html

In general you are right, it is not recommended (while possible) to use transitive dependencies of a library that you are using.

However in the case of the plugin-util API: this is one of the reasons for the existence for this plugin. This plugin provides libraries with a fixed version number for other plugins (like Jenkins does). It contains these libraries as copied classes or as dependency to another API plugin (before the birth of commons-lang-api-plugin these classes were part of the plugin-util as well). This can be done in a similar way by changing the parent pom but it is done via transitive dependencies for the historical reason I mentioned before.

However in the case of the plugin-util API: this is one of the reasons for the existence for this plugin. This plugin provides libraries with a fixed version number for other plugins (like Jenkins does)

Maven provides a BOM for this purpose, and there exists a bom implementation for plugins that does even more than just give a set of versions - it provides a known good tested set of versions.

This can be done in a similar way by changing the parent pom

no need to change any parent pom, as mentioned above. just depenencyManagement with the bom at scope import for the plugin who wishes to depend on other plugins with a known version.

However in the case of the plugin-util API: this is one of the reasons for the existence for this plugin. This plugin provides libraries with a fixed version number for other plugins (like Jenkins does)

Maven provides a BOM for this purpose, and there exists a bom implementation for plugins that does even more than just give a set of versions - it provides a known good tested set of versions.

The BOM just provides the version, not the library. But this is what I want to achieve, a single entry point to define the dependencies of some fundamental libraries that should be used by all plugins.

The BOM just provides the version, not the library

exactly

But this is what I want to achieve, a single entry point to define the dependencies of some fundamental libraries that should be used by all plugins.

but the point is that all the plugins do not need it, so add it where it is required to enable administrators to reduce plugin bloat and work (e.g. if commons-text is not installed as it is not used then it does not need updating and would be one less thing that could have a security issue that requires updating).

as a concrete example just to use [font-awesome](https://plugins.jenkins.io/font-awesome-api we need to have

Plugin Utilities API (this plugin - which is fair enough)

commons-lang3 v3.x Jenkins API (unused by font-awesome and not needed post this PR)

commons-text-api (unused by font-awesome and not needed post this PR)

I am happy to add PRs to the consumers to add the dependency to the commons-text and commons-lang3 where they use it to move this forward if it helps.

uhafner · 2022-10-10T20:30:36Z

pom.xml

+    <dependency>
+      <groupId>io.jenkins.plugins</groupId>
+      <artifactId>commons-text-api</artifactId>
+      <scope>test</scope>
+    </dependency>


Unnecessary since we cannot remove the compile dependency.

andycoates · 2022-10-24T00:35:45Z

Apologies, not a Java/Maven guru, but would this change also prevent org.apache.commons.text being part of the build?

Currently it seems to bundle 1.9 which has vulnerabilities due to recent RCE

    <dependency>
      <groupId>org.apache.commons</groupId>
      <artifactId>commons-text</artifactId>
      <version>1.9</version>
      <scope>provided</scope>
    </dependency>

under

WEB-INF/lib/org/apache/commons/text/

This is from the point of view of security scanners picking up the library. A quick look seems to suggest its only used for testing anyway and doesn't seem to use the vulnerable function - but use of the library version gets detected etc, so was looking for potential updates.

uhafner · 2022-10-24T20:37:32Z

Apologies, not a Java/Maven guru, but would this change also prevent org.apache.commons.text being part of the build?

Currently it seems to bundle 1.9 which has vulnerabilities due to recent RCE
    <dependency>
      <groupId>org.apache.commons</groupId>
      <artifactId>commons-text</artifactId>
      <version>1.9</version>
      <scope>provided</scope>
    </dependency>
under
WEB-INF/lib/org/apache/commons/text/
This is from the point of view of security scanners picking up the library. A quick look seems to suggest its only used for testing anyway and doesn't seem to use the vulnerable function - but use of the library version gets detected etc, so was looking for potential updates.

@andycoates commons-text is not bundled anymore in this plugin, see 35f0064

Are you using an old version of this plugin?

andycoates · 2022-10-24T22:58:18Z

@andycoates commons-text is not bundled anymore in this plugin, see 35f0064

Are you using an old version of this plugin?

Yes we were on 2.17 and upgrading. That's awesome to hear :) So if I understand, it's no longer bundled - but rather now just has a "plugin" dependency on the commons-text-api plugin instead? (which we're upgrading, so sounds like this is ultimately solved with an upgrade from a vuln scanner point of view)

uhafner · 2022-10-25T06:44:31Z

@andycoates commons-text is not bundled anymore in this plugin, see 35f0064
Are you using an old version of this plugin?

Yes we were on 2.17 and upgrading. That's awesome to hear :) So if I understand, it's no longer bundled - but rather now just has a "plugin" dependency on the commons-text-api plugin instead? (which we're upgrading, so sounds like this is ultimately solved with an upgrade from a vuln scanner point of view)

Exactly. Since we now have an official API plugin for commons-text, there is no need to include the classes here. This helps to get faster updates, since commons-text-api plugin has an independent release process.

uhafner · 2022-11-17T21:41:16Z

Obsolete due to #227

replace usages of commons.lang3 and commons.text

7f32578

github-actions bot assigned uhafner Oct 10, 2022

raul-arabaolaza suggested changes Oct 10, 2022

View reviewed changes

jtnord reviewed Oct 10, 2022

View reviewed changes

use Java11 methods instead of commons-lang3

8532106

uhafner reviewed Oct 10, 2022

View reviewed changes

uhafner mentioned this pull request Nov 17, 2022

Add dependencies of commons-lang and commons-text jenkinsci/analysis-pom-plugin#598

Merged

uhafner closed this Nov 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace usages of commons.lang3 and commons.text #217

replace usages of commons.lang3 and commons.text #217

ampuscas commented Oct 10, 2022 •

edited

Loading

raul-arabaolaza left a comment

raul-arabaolaza Oct 10, 2022

raul-arabaolaza Oct 10, 2022

raul-arabaolaza Oct 10, 2022

jtnord Oct 10, 2022

jtnord Oct 10, 2022

jtnord Oct 10, 2022 •

edited

Loading

uhafner commented Oct 10, 2022

uhafner left a comment

uhafner Oct 10, 2022

uhafner Oct 10, 2022

jtnord Oct 13, 2022

uhafner Oct 13, 2022

jtnord Oct 13, 2022

uhafner Oct 13, 2022

jtnord Oct 13, 2022

uhafner Oct 13, 2022

jtnord Nov 4, 2022 •

edited

Loading

uhafner Oct 10, 2022

andycoates commented Oct 24, 2022 •

edited

Loading

uhafner commented Oct 24, 2022

andycoates commented Oct 24, 2022

uhafner commented Oct 25, 2022

uhafner commented Nov 17, 2022

		<jenkins.baseline>2.361</jenkins.baseline>
		<jenkins.version>${jenkins.baseline}.2</jenkins.version>

replace usages of commons.lang3 and commons.text #217

replace usages of commons.lang3 and commons.text #217

Conversation

ampuscas commented Oct 10, 2022 • edited Loading

raul-arabaolaza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

uhafner commented Oct 10, 2022

uhafner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andycoates commented Oct 24, 2022 • edited Loading

uhafner commented Oct 24, 2022

andycoates commented Oct 24, 2022

uhafner commented Oct 25, 2022

uhafner commented Nov 17, 2022

ampuscas commented Oct 10, 2022 •

edited

Loading

jtnord Oct 10, 2022 •

edited

Loading

jtnord Nov 4, 2022 •

edited

Loading

andycoates commented Oct 24, 2022 •

edited

Loading