Conversation
Preserve effective inherited string properties when creating immutable Properties snapshots. Keep defaults outside the direct map and avoid retaining mutable source references. Add regression coverage for nested defaults, overrides, non-string shadows, and source mutation.
| if (props == null) { | ||
| return null; | ||
| } | ||
| Properties defaults = null; |
There was a problem hiding this comment.
I don't follow why you don't just create defaults here. Lazy initialization is not obviously needed. What's the reasoning? Why return null when the aergument is not null?
There was a problem hiding this comment.
+1 on @elharo's point. The lazy init avoids an allocation when the source Properties has no defaults chain (the common case), but it trades clarity for a marginal optimization.
A simpler version would be:
private static Properties copyDefaults(Properties props) {
if (props == null) {
return null;
}
Properties defaults = new Properties();
for (String name : props.stringPropertyNames()) {
if (!(props.get(name) instanceof String)) {
defaults.setProperty(name, props.getProperty(name));
}
}
return defaults.isEmpty() ? null : defaults;
}Or even just always return a (possibly empty) Properties and drop the ternary — super(emptyProperties) is harmless.
That said, the current code is correct as-is — just a readability nit.
There was a problem hiding this comment.
right @elharo @gnodet . The lazy initialization was intended to avoid allocating an empty Properties when no inherited defaults existed, but the added complexity might not be justified here. The API copy now initializes it eagerly, and I aligned the internal XML implementation and MDO template with the same behavior. Let me know thanks!
gnodet
left a comment
There was a problem hiding this comment.
Nice work — this is the right approach. Compared to the alternative (our #12906, now closed), this PR:
- Preserves the
Propertiescontract —containsKey()returnsfalsefor defaults-only keys, whilegetProperty()still finds them through the chain. The flattening approach in #12906 lost this distinction. - Handles non-String values —
entrySet()copies direct non-String values intact, whilecopyDefaults()reconstructs the string fallback chain. #12906 silently dropped non-String entries. - Fixes all three copies (api, impl, mdo template) — #12906 only touched the api copy.
The test is more thorough than ours too — exercises multi-level defaults, non-String shadow values, and containsKey() vs getProperty() semantics.
One minor nit (echoed on @elharo's comment): the lazy init of defaults in copyDefaults() trades clarity for a marginal optimization. Consider the simpler eager-init version. But it's correct either way — not blocking on it.
LGTM ✅
Apply the eager defaults snapshot initialization to the internal XML implementation and MDO template, matching the API implementation updated during review.
Fixes #12602
Summary
ROPropertiescopied only the direct entries from the sourcePropertiesinstance. Any values inherited through its defaults chain were therefore lost, causinggetPropertyto returnnullfor inherited properties.This change snapshots the effective inherited string properties into a private defaults object before copying the direct entries. This preserves standard
Propertiesbehavior while:The API implementation, internal XML implementation, and MDO generation template use the same behavior.
Tests
Added regression coverage for:
Verification:
mvn clean verifymvn -pl api/maven-api-xml -am -Dtest=ImmutableCollectionsTest -Dsurefire.failIfNoSpecifiedTests=false testmvn -Prun-its clean installwas also attempted. The Core IT suite did not complete locally because its extracted Maven distribution directory disappeared during execution, resulting in cascading unrelated missing-file failures. Tests executed before that point passed after the matching4.0.xIT support artifacts were installed.Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.