gnodet-bot commented on code in PR #711:
URL:
https://github.com/apache/maven-deploy-plugin/pull/711#discussion_r4107321650
##########
src/main/java/org/apache/maven/plugins/deploy/DeployFileMojo.java:
##########
@@ -593,6 +603,22 @@ private void processModel(Model model) {
}
}
+ void processPomProperties(Properties properties) {
+ if (isUnresolvedProperty(groupId)) {
+ groupId = properties.getProperty("groupId", groupId);
+ }
+ if (isUnresolvedProperty(artifactId)) {
+ artifactId = properties.getProperty("artifactId", artifactId);
+ }
+ if (isUnresolvedProperty(version)) {
+ version = properties.getProperty("version", version);
+ }
+ }
Review Comment:
๐ **Missing Javadoc:** Package-private method used from tests โ should have
Javadoc explaining what it does, when it's called (after `processModel`), and
what "unresolved" means in this context.
Also: if `pom.properties` contains an empty string for a key (e.g.
`version=`), `Properties.getProperty("version", groupId)` returns `""` (not the
default), which silently replaces a `${revision}` placeholder with an empty
string. Consider guarding against blank values.
```suggestion
/**
* Overrides coordinates that are still unresolved property references
(e.g. {@code ${revision}})
* with the concrete values found in the adjacent {@code pom.properties}
inside the JAR.
* Called after {@link #processModel(Model)} so that only genuinely
unresolved placeholders
* are replaced.
*
* @param properties the properties loaded from {@code
META-INF/maven/<groupId>/<artifactId>/pom.properties}
*/
void processPomProperties(Properties properties) {
if (isUnresolvedProperty(groupId)) {
String resolved = properties.getProperty("groupId");
if (resolved != null && !resolved.isEmpty()) {
groupId = resolved;
}
}
if (isUnresolvedProperty(artifactId)) {
String resolved = properties.getProperty("artifactId");
if (resolved != null && !resolved.isEmpty()) {
artifactId = resolved;
}
}
if (isUnresolvedProperty(version)) {
String resolved = properties.getProperty("version");
if (resolved != null && !resolved.isEmpty()) {
version = resolved;
}
}
}
```
##########
src/main/java/org/apache/maven/plugins/deploy/DeployFileMojo.java:
##########
@@ -593,6 +603,22 @@ private void processModel(Model model) {
}
}
+ void processPomProperties(Properties properties) {
+ if (isUnresolvedProperty(groupId)) {
+ groupId = properties.getProperty("groupId", groupId);
+ }
+ if (isUnresolvedProperty(artifactId)) {
+ artifactId = properties.getProperty("artifactId", artifactId);
+ }
+ if (isUnresolvedProperty(version)) {
+ version = properties.getProperty("version", version);
+ }
+ }
+
+ private static boolean isUnresolvedProperty(String value) {
+ return value != null && value.startsWith("${") && value.endsWith("}");
Review Comment:
๐ก **Narrow detection:** This only catches *pure* property references
(`${revision}`) but not compound expressions like `1.0-${changelist}` where the
value contains an unresolved placeholder but doesn't start with `${`.
For the primary CI-friendly use case (`${revision}`, `${sha1}`,
`${changelist}` used standalone), this is sufficient. But it's worth a Javadoc
note documenting the limitation, since the method name implies a broader
contract.
```suggestion
private static boolean isUnresolvedProperty(String value) {
// Matches pure property references like ${revision}; compound
expressions
// such as "1.0-${changelist}" are not detected (out of scope for
now).
return value != null && value.startsWith("${") &&
value.endsWith("}");
}
```
##########
src/test/java/org/apache/maven/plugins/deploy/DeployFileMojoUnitTest.java:
##########
@@ -103,6 +104,23 @@ void processPomFromPomFileWithOverrides() {
checkMojoProperties("groupO", "artifactO", "versionO", "packagingO");
}
+ @Test
+ void processResolvedVersionFromPomProperties() {
+ mojo.setGroupId("${group}");
+ mojo.setArtifactId("${artifact}");
+ mojo.setVersion("${revision}");
+ Properties properties = new Properties();
+ properties.setProperty("groupId", "org.example");
+ properties.setProperty("artifactId", "example");
+ properties.setProperty("version", "1.2.3");
+
+ mojo.processPomProperties(properties);
+
+ assertEquals("org.example", mojo.getGroupId());
+ assertEquals("example", mojo.getArtifactId());
+ assertEquals("1.2.3", mojo.getVersion());
+ }
Review Comment:
๐งช **Missing edge-case coverage.** The happy path is tested (all three
coordinates unresolved โ resolved). Consider adding tests for:
1. **No-op case:** Already-resolved coordinates are NOT overwritten by
`processPomProperties` (e.g. `groupId="org.real"` should not be replaced even
if `pom.properties` has a different `groupId`).
2. **Partial resolution:** Only `version` is `${revision}`, but
`groupId`/`artifactId` are already concrete.
3. **Missing keys:** `pom.properties` doesn't contain `version` โ the
unresolved `${revision}` should be preserved (not set to null).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
