gnodet-bot commented on code in PR #414:
URL: https://github.com/apache/maven-archiver/pull/414#discussion_r4110661213
##########
src/main/java/org/apache/maven/shared/archiver/MavenArchiver.java:
##########
@@ -632,7 +632,7 @@ private static Properties loadOptionalProperties(final
InputStream inputStream)
try (InputStream in = inputStream) {
properties.load(in);
} catch (IllegalArgumentException | IOException ex) {
- // ignore and return empty properties
+ throw new IllegalStateException("Unable to read Maven Archiver
properties", ex);
Review Comment:
🔴 **Disproportionate failure mode.** This `IllegalStateException` propagates
uncaught through `getCreatedByVersion()` → `createdBy()` →
`handleDefaultEntries()` → `getManifest()` / `createArchive()`, killing the
entire build.
The `Created-By` manifest entry is purely cosmetic metadata. A corrupt
`pom.properties` (which is a classpath resource shipped with the library
itself, not user-supplied) should not prevent JAR creation.
The issue (#372) asks for corruption to be *reported*, not for the build to
*fail*. `slf4j-api` is already a compile dependency — logging a warning and
falling back to empty properties would satisfy the issue without the collateral
damage:
```suggestion
LOG.warn("Unable to read Maven Archiver properties", ex);
```
This would require adding a `Logger` field to `MavenArchiver` (the class
currently has none, but slf4j-api is already on the compile classpath).
Alternatively, if introducing a logger is undesirable, `System.err.println` or
re-throwing a checked `MavenArchiverException` (caught upstream) would both be
less destructive than an unchecked `IllegalStateException` that bypasses the
project's exception hierarchy.
##########
src/test/java/org/apache/maven/shared/archiver/MavenArchiverTest.java:
##########
@@ -98,6 +102,17 @@ void setup() {
when(dependencyResolverResult.getDependencies()).thenReturn(dependencies);
}
+ @Test
+ void malformedPomPropertiesAreReported() throws Exception {
+ Method method =
MavenArchiver.class.getDeclaredMethod("loadOptionalProperties",
InputStream.class);
+ method.setAccessible(true);
Review Comment:
💡 **Reflection on private methods is fragile.** `setAccessible(true)`
couples the test to the internal method signature — a rename or parameter
change breaks this test silently.
If the decision is to throw (see main comment), consider testing via the
public surface: e.g., mock the classpath to provide a corrupt `pom.properties`
and verify that `getManifest()` / `createArchive()` propagates the expected
exception. If the decision is to log a warning instead, the test should verify
the warning was emitted (using a SLF4J test appender).
--
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]
