You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "efegokdemir (via GitHub)" <gi...@apache.org> on 2026/09/24 07:55:33 UTC
[PR] fix: handle missing property values [maven-filtering]
efegokdemir opened a new pull request, #391:
URL: https://github.com/apache/maven-filtering/pull/391
## Summary
Prevent `PropertyUtils.getPropertyValue()` from dereferencing a missing property value.
## Changes
- Return `null` immediately when the requested key is absent.
- Add regression coverage for the missing-key path.
## Testing
- `mvn -Dspotless.skip=true -DskipITs -Dinvoker.skip=true verify` — passed (76 tests)
- `git diff --check` — passed
- Standard `mvn verify` is blocked by the local Palantir formatter/JDK incompatibility (`NoSuchFieldError` for `JCTree$JCCompilationUnit.endPositions`).
Fixes #358
- [x] I hereby declare this contribution to be licenced under the Apache License 2.0, January 2004.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: handle missing property values [maven-filtering]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: handle missing property values [maven-filtering]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #391:
URL: https://github.com/apache/maven-filtering/pull/391
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org