gnodet-bot commented on code in PR #13158:
URL: https://github.com/apache/maven/pull/13158#discussion_r4029935575
##########
compat/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java:
##########
@@ -515,16 +517,93 @@ private static boolean
isExternalModelBuildingRequest(ModelBuildingRequest reque
}
/**
- * Returns the profiles from the given list whose activation does not
depend on a file or a
- * property. Profiles activated by JDK version, operating system, or marked
- * {@code activeByDefault} are unaffected, since those conditions are a
function of the build
- * platform rather than of the model content.
+ * Returns a sandboxed {@link ProfileActivationContext} for evaluating
profiles in
+ * repository-resolved (external) models — dependency POMs, parent POMs,
and imported BOMs.
+ * <p>
+ * The sandboxed context preserves system properties (so JDK/OS activation
works) and
+ * merges the POM's own {@code <properties>} into the system properties
map so that
+ * property-activated profiles that depend on POM-declared values still
work.
+ * User properties (consumer {@code -D} flags) are suppressed because they
were not
+ * set for the dependency and must not accidentally activate its profiles.
+ * File-based profiles are pre-filtered via {@link
#withoutFileActivation(List)} before
+ * reaching this context, so {@code getProjectDirectory()} is not relied
on for file checks.
+ * <p>
+ * Model properties are merged into system properties (with system
properties taking
+ * precedence) rather than changing the {@code PropertyProfileActivator}
lookup chain,
+ * because changing the activator would affect ALL profile evaluations —
including the
+ * build's own project — which can cause unintended profile activation
when a POM declares
+ * a property that matches a profile's activation condition.
+ *
+ * @param delegate the original full context for this model build
+ * @return a sandboxed context suitable for external model profile
activation
*/
- private static List<Profile>
withoutFileAndPropertyActivation(List<Profile> profiles) {
+ private static ProfileActivationContext
externalActivationContext(ProfileActivationContext delegate) {
+ // Pre-compute the merged system+project properties once per external
model.
+ // getSystemProperties() may be called multiple times per profile
(e.g. OperatingSystemProfileActivator
+ // calls it 3× for name/arch/version), so allocating a new HashMap on
every call is O(deps × profiles ×
+ // |systemProperties|). Computing it eagerly here keeps the anonymous
class allocation-free.
+ final Map<String, String> mergedSystemProps;
+ Map<String, String> projectProps = delegate.getProjectProperties();
+ if (projectProps == null || projectProps.isEmpty()) {
+ mergedSystemProps = delegate.getSystemProperties();
+ } else {
+ Map<String, String> merged = new HashMap<>(projectProps);
Review Comment:
Warning: Mutable map returned when project properties are absent
When `projectProps` is null or empty, `mergedSystemProps` is assigned the
raw `delegate.getSystemProperties()` map (backed by the caller's mutable
`Properties`). The anonymous class then returns this reference directly. A
caller who mutates the underlying system-properties map after creating the
sandbox would see those changes inside, silently violating the "system wins
over model properties" invariant.
The non-empty branch already wraps with `Collections.unmodifiableMap`. The
fix is to apply the same defensiveness on the empty branch:
```suggestion
mergedSystemProps =
Collections.unmodifiableMap(delegate.getSystemProperties());
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -1696,25 +1697,38 @@ private void addActivePomProfiles(String modelId,
List<Profile> activePomProfile
private List<Profile> getActiveProfiles(
Review Comment:
This `.toList()` ends the `nonFileProfiles` filter that only excludes
`activation.file != null`. Condition profiles whose expression calls
`missing(path)` pass this filter and enter the sandbox where `context.exists()`
always returns `false` — so `missing()` fires unconditionally. See general
comment for the full analysis and suggested fix.
--
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]
