gnodet commented on code in PR #13406:
URL: https://github.com/apache/maven/pull/13406#discussion_r4241430850
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultProfileInjector.java:
##########
@@ -103,6 +104,69 @@ public void mergeBuildBase(BuildBase.Builder builder,
BuildBase target, BuildBas
mergeBuildBase(builder, target, source, true,
Collections.emptyMap());
}
+ @Override
+ protected void mergeModelBase_Properties(
+ ModelBase.Builder builder,
+ ModelBase target,
+ ModelBase source,
+ boolean sourceDominant,
+ Map<Object, Object> context) {
+ Map<String, String> src = source.getProperties();
+ Map<String, String> tgt = target.getProperties();
+ Map<String, String> merged = new LinkedHashMap<>(tgt.size() +
src.size() * 2);
+ if (sourceDominant) {
+ merged.putAll(tgt);
+ merged.putAll(src);
+ } else {
+ merged.putAll(src);
+ merged.putAll(tgt);
+ }
+
+ if (src.isEmpty()) {
+ builder.properties(merged);
+ builder.location(
+ "properties",
+ InputLocation.merge(
+ target.getLocation("properties"),
source.getLocation("properties"), sourceDominant));
+ return;
+ }
+
+ InputLocation propsLoc = source.getLocation("properties");
+ if (source instanceof Profile profile) {
+ String profileId = profile.getId();
+ if (profileId != null && !profileId.isEmpty()) {
+ String prefix = "profile." + profileId + ".";
+ // Note: Profile IDs containing dots (e.g., "foo.bar") may
cause key collisions
+ // if another profile (e.g., "foo") defines a property
that shares the same prefix
+ // (e.g., "bar.myprop"). In such cases, last-write-wins
applies. Profile IDs without
+ // dots or using hyphens are recommended.
+ for (Map.Entry<String, String> entry : src.entrySet()) {
+ String key = entry.getKey();
+ String value = entry.getValue();
+ // Do not re-prefix properties that already start with
"profile." to avoid redundant double-scoping
+ // (e.g. "profile.custom" defined in profile
"myprofile" remains as "profile.custom" rather than
+ // generating "profile.myprofile.profile.custom").
+ if (!key.startsWith("profile.")) {
+ String scopedKey = prefix + key;
+ merged.put(scopedKey, value);
+ if (propsLoc != null) {
+ InputLocation loc = propsLoc.getLocation(key);
+ if (loc != null) {
+ builder.location(scopedKey, loc);
Review Comment:
🐛 **Wrong location level — scoped property `InputLocation` lost on
inheritance merge.**
`builder.location(scopedKey, loc)` stores the location in
`ModelBase.locations[scopedKey]` (top level). But property locations live
nested inside `model.getLocation("properties")`. When
`DefaultInheritanceAssembler.mergeModelBase_Properties` runs, it only merges
the sub-locations of the `"properties"` `InputLocation` — it does not propagate
model-level entries. After parent-child inheritance assembly, these scoped
locations are silently dropped.
Accumulate `scopedKey → loc` into a local map and merge them into the
`InputLocation` you build for `"properties"` at the end of the method instead.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultProfileInjector.java:
##########
@@ -103,6 +104,69 @@ public void mergeBuildBase(BuildBase.Builder builder,
BuildBase target, BuildBas
mergeBuildBase(builder, target, source, true,
Collections.emptyMap());
}
+ @Override
+ protected void mergeModelBase_Properties(
+ ModelBase.Builder builder,
+ ModelBase target,
+ ModelBase source,
+ boolean sourceDominant,
+ Map<Object, Object> context) {
+ Map<String, String> src = source.getProperties();
+ Map<String, String> tgt = target.getProperties();
+ Map<String, String> merged = new LinkedHashMap<>(tgt.size() +
src.size() * 2);
+ if (sourceDominant) {
+ merged.putAll(tgt);
+ merged.putAll(src);
+ } else {
+ merged.putAll(src);
+ merged.putAll(tgt);
+ }
+
+ if (src.isEmpty()) {
+ builder.properties(merged);
+ builder.location(
+ "properties",
+ InputLocation.merge(
+ target.getLocation("properties"),
source.getLocation("properties"), sourceDominant));
+ return;
+ }
+
+ InputLocation propsLoc = source.getLocation("properties");
+ if (source instanceof Profile profile) {
+ String profileId = profile.getId();
+ if (profileId != null && !profileId.isEmpty()) {
+ String prefix = "profile." + profileId + ".";
+ // Note: Profile IDs containing dots (e.g., "foo.bar") may
cause key collisions
+ // if another profile (e.g., "foo") defines a property
that shares the same prefix
+ // (e.g., "bar.myprop"). In such cases, last-write-wins
applies. Profile IDs without
+ // dots or using hyphens are recommended.
+ for (Map.Entry<String, String> entry : src.entrySet()) {
+ String key = entry.getKey();
+ String value = entry.getValue();
+ // Do not re-prefix properties that already start with
"profile." to avoid redundant double-scoping
+ // (e.g. "profile.custom" defined in profile
"myprofile" remains as "profile.custom" rather than
+ // generating "profile.myprofile.profile.custom").
+ if (!key.startsWith("profile.")) {
+ String scopedKey = prefix + key;
+ merged.put(scopedKey, value);
Review Comment:
⚠️ **Silent property collision.** If `tgt` already contains `scopedKey`
(e.g., the user explicitly declared
`<profile.dev.server.port>9090</profile.dev.server.port>` in the model's
`<properties>`), this silently overwrites it with the profile's value. There is
no warning, no model problem reported. At minimum, check
`tgt.containsKey(scopedKey)` and log a debug-level note or document that
`profile.<id>.*` property names are reserved by this feature.
--
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]
