gnodet-bot commented on code in PR #1774:
URL: https://github.com/apache/maven/pull/1774#discussion_r4291692589
##########
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/ProfileActivationContext.java:
##########
@@ -141,4 +144,12 @@ public interface ProfileActivationContext {
* @throws InterpolatorException if an error occurs during interpolation
*/
boolean exists(@Nullable String path, boolean glob);
+
+ /**
+ * Inject properties from newly activated profiles in order to trigger the
cascading mechanism.
+ * This method allows profiles to contribute properties that can trigger
the activation of other profiles.
+ *
+ * @param activatedProfiles The collection of profiles that have been
activated that may trigger the cascading effect.
+ */
+ void addProfileProperties(Collection<Profile> activatedProfiles);
}
Review Comment:
⚠️ **SPI break:** `addProfileProperties` is added to the interface without a
`default` implementation. Every existing custom implementation of
`ProfileActivationContext` (embedders, IDE integrations) will fail to compile.
Add a no-op default:
```suggestion
default void addProfileProperties(Collection<Profile> activatedProfiles)
{}
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultProfileSelector.java:
##########
@@ -63,32 +63,54 @@ public DefaultProfileSelector
addProfileActivator(ProfileActivator profileActiva
@Override
public List<Profile> getActiveProfiles(
Collection<Profile> profiles, ProfileActivationContext context,
ModelProblemCollector problems) {
- List<Profile> activeProfiles = new ArrayList<>(profiles.size());
+
+ List<Profile> activeSettingsProfiles = new ArrayList<>();
+ List<Profile> activePomProfiles = new ArrayList<>();
List<Profile> activePomProfilesByDefault = new ArrayList<>();
- boolean activatedPomProfileNotByDefault = false;
- for (Profile profile : profiles) {
- if (!context.isProfileInactive(profile.getId())) {
- if (context.isProfileActive(profile.getId()) ||
isActive(profile, context, problems)) {
- activeProfiles.add(profile);
- if (Profile.SOURCE_POM.equals(profile.getSource())) {
- activatedPomProfileNotByDefault = true;
- }
- } else if (isActiveByDefault(profile)) {
- if (Profile.SOURCE_POM.equals(profile.getSource())) {
- activePomProfilesByDefault.add(profile);
- } else {
- activeProfiles.add(profile);
+ // Cascading mode: iterate until no more profiles are activated
+ List<Profile> remainingProfiles = new ArrayList<>(profiles);
+ List<Profile> activatedProfiles;
+ do {
+ activatedProfiles = new ArrayList<>();
+ for (Profile profile : List.copyOf(remainingProfiles)) {
+ if (!context.isProfileInactive(profile.getId())) {
+ boolean activated =
context.isProfileActive(profile.getId());
+ boolean active = isActive(profile, context, problems);
+ boolean activeByDefault = isActiveByDefault(profile);
+ if (activated || active || activeByDefault) {
+ if (Profile.SOURCE_POM.equals(profile.getSource())) {
+ if (activated || active) {
+ activePomProfiles.add(profile);
+ } else {
+ activePomProfilesByDefault.add(profile);
+ }
+ } else {
+ activeSettingsProfiles.add(profile);
+ }
+ remainingProfiles.remove(profile);
+ activatedProfiles.add(profile);
}
}
}
+ // Add profile properties for cascading activation
+ context.addProfileProperties(activatedProfiles);
+ } while (!activatedProfiles.isEmpty());
Review Comment:
⚠️ **Spurious property injection from `activeByDefault` profiles:**
`activatedProfiles` in this loop includes profiles activated `activeByDefault`,
but those may be dropped later (line 366–367: they are only included in the
final result when `activePomProfiles.isEmpty()`). If a non-default profile was
activated in the same iteration, the `activeByDefault` ones are excluded from
the result yet their properties have already been injected into the cascade —
triggering further profile activations based on properties that won't actually
be contributed.
The `activatedProfiles` list passed to `context.addProfileProperties()`
should contain only profiles that will definitely appear in the final result.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultProfileSelector.java:
##########
@@ -63,32 +63,54 @@ public DefaultProfileSelector
addProfileActivator(ProfileActivator profileActiva
@Override
public List<Profile> getActiveProfiles(
Collection<Profile> profiles, ProfileActivationContext context,
ModelProblemCollector problems) {
- List<Profile> activeProfiles = new ArrayList<>(profiles.size());
+
+ List<Profile> activeSettingsProfiles = new ArrayList<>();
+ List<Profile> activePomProfiles = new ArrayList<>();
List<Profile> activePomProfilesByDefault = new ArrayList<>();
- boolean activatedPomProfileNotByDefault = false;
- for (Profile profile : profiles) {
- if (!context.isProfileInactive(profile.getId())) {
- if (context.isProfileActive(profile.getId()) ||
isActive(profile, context, problems)) {
- activeProfiles.add(profile);
- if (Profile.SOURCE_POM.equals(profile.getSource())) {
- activatedPomProfileNotByDefault = true;
- }
- } else if (isActiveByDefault(profile)) {
- if (Profile.SOURCE_POM.equals(profile.getSource())) {
- activePomProfilesByDefault.add(profile);
- } else {
- activeProfiles.add(profile);
+ // Cascading mode: iterate until no more profiles are activated
+ List<Profile> remainingProfiles = new ArrayList<>(profiles);
+ List<Profile> activatedProfiles;
+ do {
+ activatedProfiles = new ArrayList<>();
+ for (Profile profile : List.copyOf(remainingProfiles)) {
+ if (!context.isProfileInactive(profile.getId())) {
+ boolean activated =
context.isProfileActive(profile.getId());
+ boolean active = isActive(profile, context, problems);
+ boolean activeByDefault = isActiveByDefault(profile);
+ if (activated || active || activeByDefault) {
+ if (Profile.SOURCE_POM.equals(profile.getSource())) {
+ if (activated || active) {
+ activePomProfiles.add(profile);
+ } else {
+ activePomProfilesByDefault.add(profile);
+ }
+ } else {
+ activeSettingsProfiles.add(profile);
+ }
+ remainingProfiles.remove(profile);
+ activatedProfiles.add(profile);
}
}
}
+ // Add profile properties for cascading activation
+ context.addProfileProperties(activatedProfiles);
+ } while (!activatedProfiles.isEmpty());
+
+ // Clear cascaded properties to prevent them from leaking into
subsequent uses of the context
+ if (context instanceof
org.apache.maven.impl.model.DefaultProfileActivationContext dctx) {
+ dctx.clearCascadedProfileProperties();
Review Comment:
🔧 **`instanceof` coupling — breaks non-default implementations:**
`clearCascadedProfileProperties()` is only called for
`DefaultProfileActivationContext`. Any custom `ProfileActivationContext` that
implements the new `addProfileProperties` SPI accumulates cascaded state across
invocations and never clears it.
The `clearCascadedProfileProperties` lifecycle should either be part of the
`ProfileActivationContext` SPI (e.g. a `clearCascadedProfileProperties` default
method), or the cascading state should be managed entirely within
`DefaultProfileSelector` rather than stored on the context.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/profile/ConditionProfileActivator.java:
##########
@@ -182,16 +182,23 @@ static String doGetProperty(ProfileActivationContext
context, String name) {
// Check user properties
String v = context.getUserProperty(name);
- if (v == null) {
- // Check project properties
- // TODO: this may leads to instability between file model
activation and effective model activation
- // as the effective model properties may be different from
the file model
- v = context.getModelProperty(name);
- }
if (v == null) {
// Check system properties
v = context.getSystemProperty(name);
}
+ if (v == null) {
+ // Check project properties (with cascading support if available)
+ // ONLY for POM profiles - settings profiles should not use model
properties
+ // TODO: this may leads to instability between file model
activation and effective model activation
+ // as the effective model properties may be different from
the file model
+ if (Profile.SOURCE_POM.equals(profile.getSource())) {
+ if (context instanceof
org.apache.maven.impl.model.DefaultProfileActivationContext dctx) {
+ v = dctx.getModelPropertyForActivation(name);
Review Comment:
🔧 **`instanceof` coupling — breaks non-default implementations:** Cascaded
property lookup silently does nothing for any `ProfileActivationContext` that
is not `DefaultProfileActivationContext`. The cascading mechanism becomes a
no-op for custom implementations without any warning.
Move `getCascadedProfileProperty` to the `ProfileActivationContext` SPI as a
`default` method that returns `null`, and call it directly on the context.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/profile/ConditionProfileActivator.java:
##########
@@ -182,16 +182,23 @@ static String doGetProperty(ProfileActivationContext
context, String name) {
// Check user properties
String v = context.getUserProperty(name);
- if (v == null) {
- // Check project properties
- // TODO: this may leads to instability between file model
activation and effective model activation
- // as the effective model properties may be different from
the file model
- v = context.getModelProperty(name);
- }
if (v == null) {
Review Comment:
📝 **Undocumented property lookup order change:** The previous order was user
→ model → system. The new order is user → system → model (cascaded). This is a
**behavioral change** for projects that relied on model properties overriding
system properties in condition-based activators.
Please document this semantic shift explicitly — either in the code comment
or in the PR description — so maintainers can evaluate the compatibility impact.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/profile/ConditionProfileActivator.java:
##########
@@ -182,16 +182,23 @@ static String doGetProperty(ProfileActivationContext
context, String name) {
// Check user properties
String v = context.getUserProperty(name);
- if (v == null) {
- // Check project properties
- // TODO: this may leads to instability between file model
activation and effective model activation
- // as the effective model properties may be different from
the file model
- v = context.getModelProperty(name);
- }
if (v == null) {
// Check system properties
v = context.getSystemProperty(name);
}
+ if (v == null) {
+ // Check project properties (with cascading support if available)
+ // ONLY for POM profiles - settings profiles should not use model
properties
+ // TODO: this may leads to instability between file model
activation and effective model activation
+ // as the effective model properties may be different from
the file model
+ if (Profile.SOURCE_POM.equals(profile.getSource())) {
+ if (context instanceof
org.apache.maven.impl.model.DefaultProfileActivationContext dctx) {
Review Comment:
🐛 **NPE when `profile` is null:** `ConditionParserTest` already calls
`activator.property(null, context, s)` (see `ConditionParserTest.java` line
903). If `profile` is null, `Profile.SOURCE_POM.equals(profile.getSource())`
will throw `NullPointerException`.
Add a null guard:
```suggestion
if (profile != null &&
Profile.SOURCE_POM.equals(profile.getSource())) {
```
##########
impl/maven-core/src/test/resources-project-builder/cascading-profile-activation/pom.xml:
##########
@@ -0,0 +1,145 @@
+<?xml version="1.0" encoding="UTF-8"?>
Review Comment:
🗑️ **Orphaned test resource:** This POM is placed under `maven-core`'s
`resources-project-builder` directory, but no test class in `maven-core` (e.g.
`ProjectBuilderTest`, `DefaultProjectBuilderTest`) references
`"cascading-profile-activation"`. Either add an integration test that exercises
this POM, or move it to `maven-impl` alongside `DefaultProfileSelectorTest`.
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultProfileSelectorTest.java:
##########
@@ -0,0 +1,430 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.impl.model;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.maven.api.model.Activation;
+import org.apache.maven.api.model.ActivationProperty;
+import org.apache.maven.api.model.Model;
+import org.apache.maven.api.model.Profile;
+import org.apache.maven.api.services.model.ProfileActivationContext;
+import org.apache.maven.api.services.model.ProfileActivator;
+import org.apache.maven.impl.model.profile.PropertyProfileActivator;
+import org.apache.maven.impl.model.profile.SimpleProblemCollector;
+import org.apache.maven.impl.model.rootlocator.DefaultRootLocator;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests {@link DefaultProfileSelector} with focus on cascading activation
behavior.
+ */
+public class DefaultProfileSelectorTest {
+
+ private DefaultProfileSelector selector;
+ private SimpleProblemCollector problems;
+
+ @BeforeEach
+ void setUp() {
+ selector = new DefaultProfileSelector();
+ // Add a simple property-based activator for testing
+ selector.addProfileActivator(new PropertyProfileActivator());
+ problems = new SimpleProblemCollector();
+ }
+
+ @Test
+ void testNonCascadingActivation() {
+ // Create profiles with property-based activation
+ Profile profile1 = createProfile("profile1", "prop1", "value1",
Profile.SOURCE_POM);
+ Profile profile2 = createProfile("profile2", "prop2", "value2",
Profile.SOURCE_POM);
+
+ List<Profile> profiles = Arrays.asList(profile1, profile2);
+
+ // Create context with prop1 set
+ DefaultProfileActivationContext context =
+ new DefaultProfileActivationContext(new DefaultRootLocator(),
new DefaultInterpolator());
+ context.setModel(Model.newInstance());
+ context.setSystemProperties(Map.of("prop1", "value1"));
+
+ // Test cascading mode (current implementation only supports cascading)
+ List<Profile> activeProfiles = selector.getActiveProfiles(profiles,
context, problems);
+
+ assertEquals(1, activeProfiles.size());
+ assertEquals("profile1", activeProfiles.get(0).getId());
+ assertTrue(problems.getErrors().isEmpty());
+ }
+
+ @Test
+ void testCascadingActivation() {
+ // Create profiles where one activates another through properties
+ Profile profile1 = createProfileWithProperties(
+ "profile1", "prop1", "value1", Map.of("prop2", "value2"),
Profile.SOURCE_POM);
+ Profile profile2 = createProfile("profile2", "prop2", "value2",
Profile.SOURCE_POM);
+
+ List<Profile> profiles = Arrays.asList(profile1, profile2);
+
+ // Create context with prop1 set (should activate profile1, which sets
prop2, which activates profile2)
+ DefaultProfileActivationContext context =
+ new DefaultProfileActivationContext(new DefaultRootLocator(),
new DefaultInterpolator());
+ context.setSystemProperties(Map.of("prop1", "value1"));
+ context.setModel(Model.newInstance()); // Set a model for property
injection
+
+ // Test cascading mode
+ List<Profile> activeProfiles = selector.getActiveProfiles(profiles,
context, problems);
+
+ assertEquals(2, activeProfiles.size());
+ assertTrue(activeProfiles.stream().anyMatch(p ->
"profile1".equals(p.getId())));
+ assertTrue(activeProfiles.stream().anyMatch(p ->
"profile2".equals(p.getId())));
+ assertTrue(problems.getErrors().isEmpty());
+ }
+
+ @Test
+ void testCascadingVsNonCascadingDifference() {
+ // Create profiles where cascading would activate more profiles
+ Profile profile1 = createProfileWithProperties(
+ "profile1", "prop1", "value1", Map.of("prop2", "value2"),
Profile.SOURCE_POM);
+ Profile profile2 = createProfile("profile2", "prop2", "value2",
Profile.SOURCE_POM);
+
+ List<Profile> profiles = Arrays.asList(profile1, profile2);
+
+ DefaultProfileActivationContext context =
+ new DefaultProfileActivationContext(new DefaultRootLocator(),
new DefaultInterpolator());
+ context.setSystemProperties(Map.of("prop1", "value1"));
+ context.setModel(Model.newInstance()); // Set a model for property
injection
+
+ // Cascading should activate both profile1 and profile2
+ List<Profile> cascading = selector.getActiveProfiles(profiles,
context, problems);
+ assertEquals(2, cascading.size());
+ }
+
+ @Test
+ void testActiveByDefaultProfiles() {
+ Profile defaultProfile =
createActiveByDefaultProfile("default-profile", Profile.SOURCE_POM);
+ Profile conditionalProfile = createProfile("conditional", "prop1",
"value1", Profile.SOURCE_POM);
+
+ List<Profile> profiles = Arrays.asList(defaultProfile,
conditionalProfile);
+
+ DefaultProfileActivationContext context =
+ new DefaultProfileActivationContext(new DefaultRootLocator(),
new DefaultInterpolator());
+ context.setModel(Model.newInstance());
+
+ // Should activate default profile when no conditions are met
+ List<Profile> activeProfiles = selector.getActiveProfiles(profiles,
context, problems);
+ assertEquals(1, activeProfiles.size());
+ assertEquals("default-profile", activeProfiles.get(0).getId());
+
+ // Should not activate default profile when conditional profile is
active
+ context.setSystemProperties(Map.of("prop1", "value1"));
+ activeProfiles = selector.getActiveProfiles(profiles, context,
problems);
+ assertEquals(1, activeProfiles.size());
+ assertEquals("conditional", activeProfiles.get(0).getId());
+ }
+
+ @Test
+ void testMixedSourceProfiles() {
+ Profile pomProfile = createProfile("pom-profile", "prop1", "value1",
Profile.SOURCE_POM);
+ Profile settingsProfile = createProfile("settings-profile", "prop2",
"value2", Profile.SOURCE_SETTINGS);
+
+ List<Profile> profiles = Arrays.asList(pomProfile, settingsProfile);
+
+ DefaultProfileActivationContext context =
+ new DefaultProfileActivationContext(new DefaultRootLocator(),
new DefaultInterpolator());
+ context.setModel(Model.newInstance());
+ context.setSystemProperties(Map.of("prop1", "value1", "prop2",
"value2"));
+
+ List<Profile> activeProfiles = selector.getActiveProfiles(profiles,
context, problems);
+ assertEquals(2, activeProfiles.size());
+
+ // Settings profiles should come after POM profiles in the result
+ assertEquals("pom-profile", activeProfiles.get(0).getId());
+ assertEquals("settings-profile", activeProfiles.get(1).getId());
+ }
+
+ @Test
+ void testEmptyProfilesList() {
+ List<Profile> profiles = Collections.emptyList();
+ DefaultProfileActivationContext context =
+ new DefaultProfileActivationContext(new DefaultRootLocator(),
new DefaultInterpolator());
+ context.setModel(Model.newInstance());
+
+ List<Profile> activeProfiles = selector.getActiveProfiles(profiles,
context, problems);
+ assertTrue(activeProfiles.isEmpty());
+ }
+
+ @Test
+ void testExplicitlyActivatedProfiles() {
+ Profile profile1 = createProfile("profile1", "nonexistent", "value",
Profile.SOURCE_POM);
+ Profile profile2 = createProfile("profile2", "prop2", "value2",
Profile.SOURCE_POM);
+
+ List<Profile> profiles = Arrays.asList(profile1, profile2);
+
+ DefaultProfileActivationContext context = new
DefaultProfileActivationContext(
+ new DefaultRootLocator(),
+ new DefaultInterpolator(),
+ List.of("profile1"),
+ List.of(),
+ Map.of("prop2", "value2"),
+ Map.of(),
+ Model.newInstance());
+
+ List<Profile> activeProfiles = selector.getActiveProfiles(profiles,
context, problems);
+ assertEquals(2, activeProfiles.size());
+ assertTrue(activeProfiles.stream().anyMatch(p ->
"profile1".equals(p.getId())));
+ assertTrue(activeProfiles.stream().anyMatch(p ->
"profile2".equals(p.getId())));
+ }
+
+ @Test
+ void testCascadingActivationChain() {
+ // Create a chain of profiles: profile1 -> profile2 -> profile3
+ Profile profile1 = createProfileWithProperties(
+ "profile1", "prop1", "value1", Map.of("prop2", "value2"),
Profile.SOURCE_POM);
+ Profile profile2 = createProfileWithProperties(
+ "profile2", "prop2", "value2", Map.of("prop3", "value3"),
Profile.SOURCE_POM);
+ Profile profile3 = createProfile("profile3", "prop3", "value3",
Profile.SOURCE_POM);
+
+ List<Profile> profiles = Arrays.asList(profile1, profile2, profile3);
+
+ // Create context with prop1 set
+ DefaultProfileActivationContext context =
+ new DefaultProfileActivationContext(new DefaultRootLocator(),
new DefaultInterpolator());
+ context.setSystemProperties(Map.of("prop1", "value1"));
+ context.setModel(Model.newInstance());
+
+ // Test cascading mode
+ List<Profile> activeProfiles = selector.getActiveProfiles(profiles,
context, problems);
+
+ // All three profiles should be activated through cascading
+ assertEquals(3, activeProfiles.size());
+ assertTrue(activeProfiles.stream().anyMatch(p ->
"profile1".equals(p.getId())));
+ assertTrue(activeProfiles.stream().anyMatch(p ->
"profile2".equals(p.getId())));
+ assertTrue(activeProfiles.stream().anyMatch(p ->
"profile3".equals(p.getId())));
+ assertTrue(problems.getErrors().isEmpty());
+ }
+
+ @Test
+ void testCascadingStopCondition() {
+ // Test that cascading stops when no more profiles can be activated
+ Profile profile1 = createProfileWithProperties(
+ "profile1", "prop1", "value1", Map.of("prop2", "value2"),
Profile.SOURCE_POM);
+ Profile profile2 = createProfileWithProperties(
+ "profile2", "prop2", "value2", Map.of("prop3", "value3"),
Profile.SOURCE_POM);
+ // profile3 requires prop4 which is never set, so cascading should stop
+ Profile profile3 = createProfile("profile3", "prop4", "value4",
Profile.SOURCE_POM);
+
+ List<Profile> profiles = Arrays.asList(profile1, profile2, profile3);
+
+ // Create context with prop1 set
+ DefaultProfileActivationContext context =
+ new DefaultProfileActivationContext(new DefaultRootLocator(),
new DefaultInterpolator());
+ context.setSystemProperties(Map.of("prop1", "value1"));
+ context.setModel(Model.newInstance());
+
+ // Test cascading mode
+ List<Profile> activeProfiles = selector.getActiveProfiles(profiles,
context, problems);
+
+ // Only profile1 and profile2 should be activated, profile3 should not
+ assertEquals(2, activeProfiles.size());
+ assertTrue(activeProfiles.stream().anyMatch(p ->
"profile1".equals(p.getId())));
+ assertTrue(activeProfiles.stream().anyMatch(p ->
"profile2".equals(p.getId())));
+ assertTrue(activeProfiles.stream().noneMatch(p ->
"profile3".equals(p.getId())));
+ assertTrue(problems.getErrors().isEmpty());
+ }
+
+ @Test
+ void testCascadingWithCircularDependency() {
+ // Test that cascading handles circular dependencies gracefully
+ Profile profile1 = createProfileWithProperties(
+ "profile1", "prop1", "value1", Map.of("prop2", "value2"),
Profile.SOURCE_POM);
+ Profile profile2 = createProfileWithProperties(
+ "profile2", "prop2", "value2", Map.of("prop1", "value1"),
Profile.SOURCE_POM);
+
+ List<Profile> profiles = Arrays.asList(profile1, profile2);
+
+ // Create context with prop1 set
+ DefaultProfileActivationContext context =
+ new DefaultProfileActivationContext(new DefaultRootLocator(),
new DefaultInterpolator());
+ context.setSystemProperties(Map.of("prop1", "value1"));
+ context.setModel(Model.newInstance());
+
+ // Test cascading mode
+ List<Profile> activeProfiles = selector.getActiveProfiles(profiles,
context, problems);
+
+ // Both profiles should be activated, but cascading should stop after
first iteration
+ assertEquals(2, activeProfiles.size());
+ assertTrue(activeProfiles.stream().anyMatch(p ->
"profile1".equals(p.getId())));
+ assertTrue(activeProfiles.stream().anyMatch(p ->
"profile2".equals(p.getId())));
+ assertTrue(problems.getErrors().isEmpty());
+ }
+
+ @Test
+ void testCascadingWithInactiveProfile() {
+ // Create profiles where one would activate another, but the second is
explicitly deactivated
+ Profile profile1 = createProfileWithProperties(
+ "profile1", "prop1", "value1", Map.of("prop2", "value2"),
Profile.SOURCE_POM);
+ Profile profile2 = createProfile("profile2", "prop2", "value2",
Profile.SOURCE_POM);
+
+ List<Profile> profiles = Arrays.asList(profile1, profile2);
+
+ // Create context with prop1 set and profile2 explicitly deactivated
+ DefaultProfileActivationContext context = new
DefaultProfileActivationContext(
+ new DefaultRootLocator(),
+ new DefaultInterpolator(),
+ List.of(),
+ List.of("profile2"),
+ Map.of("prop1", "value1"),
+ Map.of(),
+ Model.newInstance());
+
+ // Test cascading mode
+ List<Profile> activeProfiles = selector.getActiveProfiles(profiles,
context, problems);
+
+ // Only profile1 should be activated, profile2 should be deactivated
despite cascading
+ assertEquals(1, activeProfiles.size());
+ assertTrue(activeProfiles.stream().anyMatch(p ->
"profile1".equals(p.getId())));
+ assertTrue(activeProfiles.stream().noneMatch(p ->
"profile2".equals(p.getId())));
+ assertTrue(problems.getErrors().isEmpty());
+ }
+
+ @Test
+ void testCascadingWithRecordImmutability() {
+ // Test that profile records remain immutable during cascading
+ Profile originalProfile1 = createProfileWithProperties(
+ "profile1", "prop1", "value1", Map.of("prop2", "value2"),
Profile.SOURCE_POM);
+ Profile originalProfile2 = createProfile("profile2", "prop2",
"value2", Profile.SOURCE_POM);
+
+ List<Profile> profiles = Arrays.asList(originalProfile1,
originalProfile2);
+
+ // Create context with prop1 set
+ DefaultProfileActivationContext context =
+ new DefaultProfileActivationContext(new DefaultRootLocator(),
new DefaultInterpolator());
+ context.setSystemProperties(Map.of("prop1", "value1"));
+ context.setModel(Model.newInstance());
+
+ // Test cascading mode
+ List<Profile> activeProfiles = selector.getActiveProfiles(profiles,
context, problems);
+
+ // Verify that original profiles are unchanged (immutable records)
+ assertEquals("profile1", originalProfile1.getId());
+ assertEquals("profile2", originalProfile2.getId());
+ assertEquals(Map.of("prop2", "value2"),
originalProfile1.getProperties());
+ assertEquals(Map.of(), originalProfile2.getProperties());
+
+ // Verify activation worked
+ assertEquals(2, activeProfiles.size());
+ assertTrue(problems.getErrors().isEmpty());
+ }
+
+ // Helper methods for creating test profiles
+
+ private Profile createProfile(String id, String propName, String
propValue, String source) {
+ Profile profile = Profile.newBuilder()
+ .id(id)
+ .activation(Activation.newBuilder()
+ .property(ActivationProperty.newBuilder()
+ .name(propName)
+ .value(propValue)
+ .build())
+ .build())
+ .build();
+ profile.setSource(source);
+ return profile;
+ }
+
+ private Profile createProfileWithProperties(
+ String id, String propName, String propValue, Map<String, String>
profileProperties, String source) {
+ Profile profile = Profile.newBuilder()
+ .id(id)
+ .activation(Activation.newBuilder()
+ .property(ActivationProperty.newBuilder()
+ .name(propName)
+ .value(propValue)
+ .build())
+ .build())
+ .properties(profileProperties)
+ .build();
+ profile.setSource(source);
+ return profile;
+ }
+
+ private Profile createActiveByDefaultProfile(String id, String source) {
+ Profile profile = Profile.newBuilder()
+ .id(id)
+
.activation(Activation.newBuilder().activeByDefault(true).build())
+ .build();
+ profile.setSource(source);
+ return profile;
+ }
+
+ /**
+ * Simple property-based profile activator for testing.
+ */
+ private static class PropertyProfileActivator implements ProfileActivator {
+ @Override
+ public boolean isActive(
+ Profile profile,
+ ProfileActivationContext context,
+ org.apache.maven.api.services.ModelProblemCollector problems) {
+ Activation activation = profile.getActivation();
+ if (activation == null || activation.getProperty() == null) {
+ return false;
+ }
+
+ ActivationProperty property = activation.getProperty();
+ String name = property.getName();
+ String expectedValue = property.getValue();
+
+ if (name == null) {
+ return false;
+ }
+
+ // Lookup order: user (-D) → system → cascaded profile properties.
+ // Only cascaded profile properties are checked (not all model
properties),
+ // matching the real PropertyProfileActivator behavior.
+ String actualValue = context.getUserProperty(name);
+ if (actualValue == null) {
+ actualValue = context.getSystemProperty(name);
+ }
+ if (actualValue == null) {
+ if (context instanceof DefaultProfileActivationContext dctx) {
+ actualValue = dctx.getCascadedProfileProperty(name);
Review Comment:
🔧 **Test inner class re-introduces the `instanceof` coupling:** The private
`PropertyProfileActivator` inner class duplicates the `instanceof
DefaultProfileActivationContext` pattern from the production code. If the SPI
is fixed to expose `getCascadedProfileProperty` directly, this test class
should use the real `PropertyProfileActivator` (already imported at line 495)
rather than an inner reimplementation.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/profile/PropertyProfileActivator.java:
##########
@@ -99,6 +99,15 @@ public boolean isActive(Profile profile,
ProfileActivationContext context, Model
if (sysValue == null) {
sysValue = context.getSystemProperty(name);
}
+ // Check cascaded profile properties last (for cascading profile
activation).
+ // Only cascaded properties (from previously-activated profiles) are
checked — the
+ // POM's own <properties> must NOT self-activate profiles via
PropertyProfileActivator.
+ // ONLY for POM profiles - settings profiles should not use cascaded
properties.
+ if (sysValue == null &&
Profile.SOURCE_POM.equals(profile.getSource())) {
+ if (context instanceof
org.apache.maven.impl.model.DefaultProfileActivationContext dctx) {
+ sysValue = dctx.getCascadedProfileProperty(name);
Review Comment:
🔧 **`instanceof` coupling — same issue as in `ConditionProfileActivator`:**
Custom `ProfileActivationContext` implementations silently skip cascaded
property lookup here too.
Same fix applies: expose `getCascadedProfileProperty` on the SPI with a
`default` returning `null`.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultProfileActivationContext.java:
##########
@@ -484,7 +521,7 @@ private String interpolate(String path) throws
InterpolatorException {
if ("project.rootDirectory".equals(s)) {
return getModelRootDirectory();
}
- String r = getModelProperty(s);
+ String r = getModelPropertyForActivation(s);
if (r == null) {
Review Comment:
⚠️ **`interpolatePath` now resolves paths against cascaded profile
properties:** `interpolatePath` is used for file-existence activation (e.g.
`<exists>${some.path}</exists>`). By switching to
`getModelPropertyForActivation` it now also consults
`cascadedProfileProperties`. This means a file-existence activator can now be
triggered by a property injected from another profile's cascading — likely
unintentional, and not covered by any test.
If cascading should apply to file-existence activation, document it
explicitly. If not, revert this line to `getModelProperty(s)`.
--
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]
