gnodet-bot commented on code in PR #391:
URL: https://github.com/apache/maven-filtering/pull/391#discussion_r4094608442
##########
src/test/java/org/apache/maven/shared/filtering/PropertyUtilsTest.java:
##########
@@ -90,6 +92,39 @@ void exception() {
assertThrows(Exception.class, () ->
PropertyUtils.loadPropertyFile(nonExistent, true, false));
}
+ @Test
+ void missingPropertyValue() throws Exception {
+ Method getPropertyValue =
+ PropertyUtils.class.getDeclaredMethod("getPropertyValue",
String.class, Properties.class, Logger.class);
+ getPropertyValue.setAccessible(true);
+
+ assertNull(getPropertyValue.invoke(null, "missing", new Properties(),
null));
+ }
+
+ /**
+ * Regression test for the loadPropertyFile caller path: null baseProps
must be handled
+ * gracefully (treated as empty), and a property key whose value
references an unresolved
+ * placeholder must not NPE but leave the placeholder as-is.
+ */
+ @Test
+ void missingKeyInCombinedPropsDoesNotNPE() throws Exception {
+ Path propFile = TEST_DIRECTORY.resolve("missing-key.properties");
+ Files.deleteIfExists(propFile);
+
+ try (Writer writer = Files.newBufferedWriter(propFile)) {
+ // "derived" references "base" which is not defined anywhere
+ writer.write("derived=${base}\n");
+ writer.write("present=hello\n");
+ writer.flush();
+ }
+
+ // null baseProps must not NPE — treated as empty
+ Properties result = PropertyUtils.loadPropertyFile(propFile,
(Properties) null);
+ // unresolved placeholders are left as-is
+ assertEquals("${base}", result.getProperty("derived"));
+ assertEquals("hello", result.getProperty("present"));
Review Comment:
⚠️ **Pre-fix validation: this test does NOT exercise the NPE path and passes
both before and after the fix.**
`combinedProps` is built by `putAll(baseProps) + putAll(fileProps)`, so
every key from `fileProps.keySet()` — including `"derived"` — is always present
in `combinedProps`. When `getPropertyValue("derived", combinedProps, ...)`
runs, `v = p.getProperty("derived")` returns `"${base}"` (non-null), so the new
null-guard never fires. The `assertEquals("${base}", ...)` assertion also
passes without the fix because the existing code already handles unresolved
placeholders by leaving them as-is.
What this test actually validates is pre-existing placeholder preservation
behavior — not the NPE regression. To test the actual fix via the public API,
you'd need a scenario where `getPropertyValue` is called with a key absent from
`p`, which can't happen through `loadPropertyFile` given the current
construction of `combinedProps`.
The test should either be removed (it's covered by the existing
unresolved-placeholder behavior), or its Javadoc updated to accurately describe
what it's testing.
##########
src/main/java/org/apache/maven/shared/filtering/PropertyUtils.java:
##########
@@ -101,7 +101,9 @@ public static Properties loadPropertyFile(Path propFile,
Properties baseProps, L
for (Object o : fileProps.keySet()) {
final String k = (String) o;
final String propValue = getPropertyValue(k, combinedProps,
logger);
- fileProps.setProperty(k, propValue);
+ if (propValue != null) {
+ fileProps.setProperty(k, propValue);
+ }
Review Comment:
⚠️ **Dead guard — `propValue` can never be null here.**
`k` comes from `fileProps.keySet()`, and `combinedProps` is built with
`combinedProps.putAll(fileProps)` a few lines above. So `k` is guaranteed to be
in `combinedProps`, and `getPropertyValue(k, combinedProps, ...)` will find `v
= p.getProperty(k)` non-null on the very first line — the new null-return path
never triggers for this call.
The guard is harmless but misleading: it implies there's a reachable code
path where a file key has no value in `combinedProps`, which there isn't.
Consider removing it to keep the code honest, or document explicitly why it's a
defensive belt-and-suspenders check.
```suggestion
fileProps.setProperty(k, propValue);
```
--
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]
