gnodet-bot commented on code in PR #13114:
URL: https://github.com/apache/maven/pull/13114#discussion_r4018865955
##########
maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java:
##########
@@ -516,16 +518,86 @@ 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) {
+ return new ProfileActivationContext() {
+ @Override
+ public List<String> getActiveProfileIds() {
+ return delegate.getActiveProfileIds();
+ }
+
+ @Override
+ public List<String> getInactiveProfileIds() {
+ return delegate.getInactiveProfileIds();
+ }
+
+ /**
+ * System properties merged with project properties (system wins
on conflict).
+ * This makes POM-declared properties visible to the
PropertyProfileActivator
+ * without modifying the activator's lookup chain for non-external
models.
+ */
+ @Override
+ public Map<String, String> getSystemProperties() {
+ Map<String, String> projectProps =
delegate.getProjectProperties();
+ if (projectProps == null || projectProps.isEmpty()) {
+ return delegate.getSystemProperties();
+ }
+ Map<String, String> merged = new HashMap<>(projectProps);
+ merged.putAll(delegate.getSystemProperties()); // system wins
+ return merged;
Review Comment:
⚠️ **Performance: `getSystemProperties()` allocates a new `HashMap` on every
call.**
`OperatingSystemProfileActivator` calls this method 3× per profile (name,
arch, version). `PropertyProfileActivator` and `JdkVersionProfileActivator`
call it once each. In a project with deep transitive deps and profiles using OS
activation, this is O(deps × profiles × |systemProperties|) heap allocation per
build.
Since the merged map is derived from immutable inputs (system properties
don't change mid-build, project properties don't change mid-build), it can be
computed once and cached:
```suggestion
public Map<String, String> getSystemProperties() {
Map<String, String> projectProps =
delegate.getProjectProperties();
if (projectProps == null || projectProps.isEmpty()) {
return delegate.getSystemProperties();
}
Map<String, String> merged = new HashMap<>(projectProps);
merged.putAll(delegate.getSystemProperties()); // system wins
return Collections.unmodifiableMap(merged);
```
Alternatively, compute the map lazily once and store it in a `final` field
of the anonymous class:
```java
private static ProfileActivationContext
externalActivationContext(ProfileActivationContext delegate) {
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);
merged.putAll(delegate.getSystemProperties());
mergedSystemProps = Collections.unmodifiableMap(merged);
}
return new ProfileActivationContext() {
// ...
@Override
public Map<String, String> getSystemProperties() {
return mergedSystemProps;
}
// ...
};
}
```
The latter approach is preferred: one allocation per external model instead
of one per `getSystemProperties()` call. The `unmodifiableMap` wrapper also
makes the contract explicit — the returned map must not be mutated (currently
nothing mutates it, but there's no enforcement).
--
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]
