gnodet-bot commented on code in PR #1774:
URL: https://github.com/apache/maven/pull/1774#discussion_r4087759558
##########
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/ProfileActivationContext.java:
##########
@@ -142,4 +145,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` must have a `default` no-op
implementation.**
`ProfileActivationContext` is a maven-api-spi type that embedders (IntelliJ
m2e, Eclipse m2e, Gradle's Maven import, custom build tools) implement
directly. Adding an abstract method to this interface is a binary-incompatible
change: every existing implementation will fail with `AbstractMethodError` at
runtime when loaded against 4.x (unless they recompile against the new API,
which they often can't do atomically).
The fix is straightforward — provide a `default` no-op that makes the method
opt-in for existing implementations while still allowing the cascade mechanism
to work:
```suggestion
default void addProfileProperties(Collection<Profile> activatedProfiles)
{
// no-op by default; override in implementations that support
cascading
}
```
This is consistent with how other SPI extension points in maven-api-spi
handle backward compatibility.
##########
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:
⚠️ **NPE when `profile` is `null`** — `profile.getSource()` at this line
will throw `NullPointerException` if `profile` is `null`.
This was flagged in the first review (July 10). `ConditionParserTest` calls
`activator.property(null, context, s)` at line 55 of that test, passing `null`
as the profile. The test currently doesn't trigger the model-property fallback
branch (it uses system properties), so the NPE is latent — but it will fire if
any test exercises property resolution against a model with properties when
called via the test harness.
Fix:
```suggestion
if (profile != null &&
Profile.SOURCE_POM.equals(profile.getSource())) {
```
Alternatively, make `ConditionParserTest` pass a real (non-null) profile in
its resolver — that removes the null contract entirely and is arguably the
correct fix, since production code always has a non-null profile.
##########
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);
Review Comment:
⚠️ **Spurious property injection from suppressed `activeByDefault`
profiles.**
`activeByDefault` profiles are placed in `activePomProfilesByDefault` (line
86) but *also* added to `activatedProfiles` (line 92). This means
`context.addProfileProperties(activatedProfiles)` at line 97 injects their
properties into the cascading context — even though these profiles will be
silently dropped at lines 106–109 when any non-default POM profile is present.
**Concrete failure scenario:**
```
Profile A: activeByDefault=true, sets cascade.trigger=on
Profile B: activates on cascade.trigger=on (not activeByDefault)
Profile C: activates on prop set by B
```
When the user triggers any explicit profile (e.g. `-Pexplicit`), A is
supposed to be suppressed. But A's `cascade.trigger=on` is still injected,
activating B, which activates C — all three get injected into the result even
though A was suppressed.
The fix: exclude `activeByDefault`-only profiles from
`addProfileProperties`. They should not inject cascading properties if they'll
ultimately be suppressed:
```suggestion
remainingProfiles.remove(profile);
if (activated || active) {
activatedProfiles.add(profile);
}
```
This ensures only genuinely activated profiles (not `activeByDefault` ones)
contribute to the cascade. There is no test covering this edge case — one
should be added.
##########
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 — no test class loads this POM.**
This file is a valid test fixture, but there is no corresponding test in
`impl/maven-core/src/test/java/` that exercises it (neither in
`DefaultMavenProjectBuilderTest` nor `ProjectBuilderTest`). Without a test,
this resource is dead code and provides no regression protection for the
cascading feature at the integration level.
Either add an integration test that loads this POM with `trigger=start`
system property and asserts `profile1`, `profile2`, `profile3` are active and
`profile4` is not — or remove the file and rely solely on the
`DefaultProfileSelectorTest` unit tests.
--
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]
