Conversation
jglick
left a comment
There was a problem hiding this comment.
Seems to be incorrect in the first two places I checked. Was this produced by some automated rewrite tool?
| try { | ||
| Properties props = new Properties(); | ||
| boolean hasCustomEndpoint = StringUtils.isNotBlank(getConfiguration().getResolvedCustomEndpoint()); | ||
| boolean hasCustomEndpoint = !(getConfiguration().getResolvedCustomEndpoint() != null && getConfiguration().getResolvedCustomEndpoint().isBlank()); |
There was a problem hiding this comment.
Wait, what? This is equivalent to
| boolean hasCustomEndpoint = !(getConfiguration().getResolvedCustomEndpoint() != null && getConfiguration().getResolvedCustomEndpoint().isBlank()); | |
| boolean hasCustomEndpoint = getConfiguration().getResolvedCustomEndpoint() == null || !getConfiguration().getResolvedCustomEndpoint().isBlank(); |
A null value was clearly supposed to be no endpoint.
| boolean hasCustomEndpoint = !(getConfiguration().getResolvedCustomEndpoint() != null && getConfiguration().getResolvedCustomEndpoint().isBlank()); | ||
|
|
||
| if(StringUtils.isNotBlank(getRegion())) { | ||
| if(!getRegion().isBlank()) { |
There was a problem hiding this comment.
Also an incorrect translation of the original code, though in this case the original code does not seem to have made any sense; from my reading, it could never have been null or blank.
No it was not. Only used an apparently unfocussed brain. |
| Properties props = new Properties(); | ||
| boolean hasCustomEndpoint = StringUtils.isNotBlank(getConfiguration().getResolvedCustomEndpoint()); | ||
| String resolvedCustomEndpoint = getConfiguration().getResolvedCustomEndpoint(); | ||
| boolean hasCustomEndpoint = resolvedCustomEndpoint == null || !resolvedCustomEndpoint.isBlank(); |
There was a problem hiding this comment.
This still looks wrong. It has a custom endpoint if the resolved custom endpoint is not null and is not blank. Right?
There was a problem hiding this comment.
Pull request overview
This PR removes usage of Apache commons-lang v2 utilities from the S3 artifact manager plugin, replacing them with JDK equivalents and enabling an explicit build-time ban on commons-lang 2.
Changes:
- Replace
StringUtilsblank/abbreviate helpers with JDKString::isBlank+ manual truncation logic. - Replace
RandomStringUtilsusage in tests withUUID. - Enable
ban-commons-lang-2enforcement via a Maven property.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/test/java/io/jenkins/plugins/artifact_manager_jclouds/s3/S3AbstractTest.java |
Switches unique test prefix generation from RandomStringUtils to UUID (with truncation). |
src/test/java/io/jenkins/plugins/artifact_manager_jclouds/s3/LocalStackIntegrationTest.java |
Replaces StringUtils.isBlank with a null-safe isBlank check for region selection. |
src/main/java/io/jenkins/plugins/artifact_manager_jclouds/s3/S3BlobStoreConfig.java |
Replaces StringUtils usages in endpoint/validation logic and error message abbreviation. |
src/main/java/io/jenkins/plugins/artifact_manager_jclouds/s3/S3BlobStore.java |
Replaces StringUtils usages in endpoint detection, presigner configuration, and descriptor config check. |
pom.xml |
Adds property to enforce banning commons-lang 2. |
Comments suppressed due to low confidence (1)
src/main/java/io/jenkins/plugins/artifact_manager_jclouds/s3/S3BlobStoreConfig.java:265
getResolvedCustomEndpoint()currently enters the block whencustomEndpointis null because the condition iscustomEndpoint == null || !customEndpoint.isBlank(). This causes the method to return values likehttps://nulland treats a missing endpoint as configured. The condition should require a non-null, non-blankcustomEndpoint(and ideally trim it) before constructing the URL.
public String getResolvedCustomEndpoint() {
if(customEndpoint == null || !customEndpoint.isBlank()) {
String protocol;
if(getUseHttp()) {
protocol = "http";
} else {
protocol = "https";
}
return protocol + "://" + customEndpoint;
}
return null;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -368,7 +368,7 @@ public FormValidation doCheckContainer(@QueryParameter String container){ | |||
|
|
|||
| public FormValidation doCheckPrefix(@QueryParameter String prefix){ | |||
| FormValidation ret; | |||
| if (StringUtils.isBlank(prefix)) { | |||
| if (prefix.isBlank()) { | |||
| ret = FormValidation.ok("Artifacts will be stored in the root folder of the S3 Bucket."); | |||
| } else if (prefix.endsWith("/")) { | |||
| public FormValidation doCheckCustomSigningRegion(@QueryParameter String customSigningRegion) { | ||
| FormValidation ret; | ||
| if (StringUtils.isBlank(customSigningRegion) && StringUtils.isNotBlank(customEndpoint)) { | ||
| if (customSigningRegion != null && customSigningRegion.isBlank() && (customEndpoint == null || !customEndpoint.isBlank())) { |
| public FormValidation doCheckCustomEndpoint(@QueryParameter String customEndpoint) { | ||
| FormValidation ret = FormValidation.ok(); | ||
| if (!StringUtils.isBlank(customEndpoint) && !endPointPattern.matcher(customEndpoint).matches()) { | ||
| if (!customEndpoint.isBlank() && !endPointPattern.matcher(customEndpoint).matches()) { |
| @@ -141,7 +141,7 @@ public BlobStoreContext getContext() throws IOException { | |||
| .overrides(props); | |||
|
|
|||
| if (hasCustomEndpoint) { | |||
| builder = builder.endpoint(getConfiguration().getResolvedCustomEndpoint()); | |||
| builder = builder.endpoint(resolvedCustomEndpoint); | |||
| } | |||
| String customRegion = getConfiguration().getCustomSigningRegion(); | ||
| if(StringUtils.isBlank(customRegion)) { | ||
| if(customRegion.isBlank()) { | ||
| customRegion = getConfiguration().getRegion().id(); | ||
| } | ||
| if(StringUtils.isNotBlank(customRegion)) { | ||
| if(!customRegion.isBlank()) { | ||
| presignerBuilder.region(Region.of(customRegion)); | ||
| } |
| */ | ||
| public boolean isConfigured(){ | ||
| return StringUtils.isNotBlank(S3BlobStoreConfig.get().getContainer()); | ||
| return !S3BlobStoreConfig.get().getContainer().isBlank(); |
| public static String generateUniquePrefix() { | ||
| return String.format("%s%s-%s/", S3_DIR, ZonedDateTime.now().format(DateTimeFormatter.ISO_INSTANT), | ||
| RandomStringUtils.randomAlphabetic(4).toLowerCase()); | ||
| UUID.randomUUID().toString().substring(0, 4).toLowerCase()); |
This removes all usage of Commons Lang library.
Testing done
Ran
mvn verifylocally.Submitter checklist