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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f32578
replace usages of commons.lang3 and commons.text
ampuscas Oct 10, 2022
8532106
use Java11 methods instead of commons-lang3
ampuscas Oct 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,6 @@
</developers>

<dependencies>
<dependency>
<groupId>io.jenkins.plugins</groupId>
<artifactId>commons-lang3-api</artifactId>
</dependency>
<dependency>
<groupId>io.jenkins.plugins</groupId>
<artifactId>commons-text-api</artifactId>
</dependency>
Comment on lines -49 to -56
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jtnord jtnord Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

<dependency>
<groupId>edu.hm.hafner</groupId>
<artifactId>codingstyle</artifactId>
Expand Down
6 changes: 2 additions & 4 deletions src/main/java/io/jenkins/plugins/util/EnvironmentResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
package io.jenkins.plugins.util;

import org.apache.commons.lang3.StringUtils;

import edu.hm.hafner.util.VisibleForTesting;
import edu.umd.cs.findbugs.annotations.CheckForNull;

import org.springframework.util.StringUtils;
import hudson.EnvVars;
import hudson.Util;

Expand Down Expand Up @@ -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++) {
Copy link
Member

Choose a reason for hiding this comment

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

!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)

Copy link

Choose a reason for hiding this comment

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

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

String old = expanded;
expanded = Util.replaceMacro(expanded, environment);
if (old.equals(expanded)) {
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/io/jenkins/plugins/util/JenkinsFacade.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import java.util.Set;
import java.util.stream.Collectors;

import org.apache.commons.lang3.StringUtils;
import org.springframework.util.StringUtils;
import org.springframework.security.access.AccessDeniedException;

import edu.umd.cs.findbugs.annotations.CheckForNull;
Expand Down Expand Up @@ -257,7 +257,7 @@ private String getContextPath() {
if (currentRequest != null) {
return currentRequest.getContextPath();
}
return StringUtils.EMPTY;
return "";
}

/**
Expand All @@ -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, "/"));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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


}

Expand Down
13 changes: 6 additions & 7 deletions src/test/java/io/jenkins/plugins/util/FormValidationAssert.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@

import java.util.Objects;

import org.apache.commons.lang3.StringUtils;
import org.apache.commons.text.StringEscapeUtils;
import org.assertj.core.api.AbstractAssert;

import hudson.util.FormValidation;
import hudson.util.FormValidation.Kind;
import org.springframework.web.util.HtmlUtils;

/**
* Assertions for {@link FormValidation} instances.
Expand Down Expand Up @@ -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());
Copy link
Member

@jtnord jtnord Oct 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

}

return this;
Expand All @@ -110,10 +109,10 @@ public FormValidationAssert hasMessage(final String expectedMessage) {
public FormValidationAssert hasMessageContaining(final String expectedMessagePart) {
isNotNull();

String actualMessage = StringEscapeUtils.unescapeHtml4(actual.getMessage());
if (!StringUtils.contains(actualMessage, expectedMessagePart)) {
String actualMessage = HtmlUtils.htmlUnescape(actual.getMessage());
if (!actualMessage.contains(expectedMessagePart)) {
failWithMessage("%nExpecting %s of:%n <%s>%nto contain:%n <%s>%nbut was:%n <%s>.", "message",
StringEscapeUtils.unescapeHtml4(actual.toString()), expectedMessagePart, actualMessage);
HtmlUtils.htmlUnescape(actual.toString()), expectedMessagePart, actualMessage);
}

return this;
Expand Down