gnodet-bot commented on code in PR #13112:
URL: https://github.com/apache/maven/pull/13112#discussion_r4004629221
##########
compat/maven-model-builder/src/main/java/org/apache/maven/model/profile/activation/PropertyProfileActivator.java:
##########
@@ -71,10 +71,18 @@ public boolean isActive(Profile profile,
ProfileActivationContext context, Model
return false;
}
+ // Lookup order: user (-D) → system (java.version, os.name, …) →
project <properties>.
+ // In external model builds the caller suppresses user properties via
a sandboxed context
+ // (see DefaultModelBuilder.getExternalActivationContext()), so
consumer -D flags cannot
Review Comment:
💡 **Minor (Javadoc):** The comment references
`DefaultModelBuilder.getExternalActivationContext()` but the actual method is
`externalActivationContext()` (no `get` prefix).
```suggestion
// Lookup order: user (-D) → system (java.version, os.name, …) →
project <properties>.
// In external model builds the caller suppresses user properties
via a sandboxed context
// (see DefaultModelBuilder.externalActivationContext()), so
consumer -D flags cannot
// activate dependency profiles. Project properties are always
consulted because they are
// part of the artifact's published identity.
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultProfileActivationContext.java:
##########
@@ -355,6 +355,90 @@ public DefaultProfileActivationContext setModel(Model
model) {
return this;
}
+ /**
+ * Returns a sandboxed copy of this context suitable for evaluating
profiles in
+ * repository-resolved (external) models — dependency POMs, parent POMs,
and imported BOMs.
+ * <p>
+ * The sandboxed context:
+ * <ul>
+ * <li><b>Preserves system properties</b> ({@code java.version}, {@code
os.name}, …) so
+ * that JDK- and OS-activated profiles continue to work.</li>
+ * <li><b>Preserves model properties</b> (the POM's own {@code
<properties>} section),
+ * because those are part of the artifact's published identity, not
the consumer's
+ * build environment.</li>
+ * <li><b>Suppresses user properties</b> (consumer {@code -D} flags):
they were not
+ * set for the dependency and must not accidentally activate its
profiles.</li>
+ * <li><b>Disables file existence checks</b>: the publisher's file
system paths do not
+ * exist in the consumer's environment, so file-activated profiles
always return
+ * {@code false}.</li>
+ * </ul>
+ *
+ * @return a sandboxed {@link ProfileActivationContext} for external model
evaluation
+ */
+ public ProfileActivationContext withoutUserPropertiesAndFilesystem() {
+ return new ProfileActivationContext() {
+ @Override
+ public boolean isProfileActive(String profileId) {
+ return
DefaultProfileActivationContext.this.isProfileActive(profileId);
+ }
+
+ @Override
+ public boolean isProfileInactive(String profileId) {
+ return
DefaultProfileActivationContext.this.isProfileInactive(profileId);
+ }
+
+ @Override
+ public String getSystemProperty(String key) {
+ return
DefaultProfileActivationContext.this.getSystemProperty(key);
+ }
+
+ /** User properties are suppressed: consumer {@code -D} flags do
not activate dependency profiles. */
+ @Override
+ public String getUserProperty(String key) {
+ return null;
+ }
+
+ @Override
+ public String getModelProperty(String key) {
+ return
DefaultProfileActivationContext.this.getModelProperty(key);
+ }
+
+ @Override
+ public String getModelArtifactId() {
+ return
DefaultProfileActivationContext.this.getModelArtifactId();
+ }
+
+ @Override
+ public String getModelPackaging() {
+ return
DefaultProfileActivationContext.this.getModelPackaging();
+ }
+
+ @Override
+ public String getModelRootDirectory() {
+ return
DefaultProfileActivationContext.this.getModelRootDirectory();
+ }
+
+ @Override
+ public String getModelBaseDirectory() {
+ return
DefaultProfileActivationContext.this.getModelBaseDirectory();
+ }
+
+ @Override
+ public String interpolatePath(String path) throws
InterpolatorException {
+ return
DefaultProfileActivationContext.this.interpolatePath(path);
+ }
+
+ /**
+ * File existence checks are disabled for external models:
publisher paths do not
+ * exist in the consumer's environment, so file-activated profiles
always return false.
+ */
+ @Override
+ public boolean exists(String path, boolean glob) {
+ return false;
+ }
+ };
+ }
+
@Override
public String interpolatePath(String path) throws InterpolatorException {
Review Comment:
⚠️ **Behavioral asymmetry with compat stack on `<file><missing>` profiles.**
Returning `false` from `exists()` means `FileProfileActivator.isActive()`
returns `true` for `<missing>` profiles (line 99: `return missing !=
fileExists` → `true != false` → `true`). So in external builds, the impl stack
will **activate** profiles with `<file><missing>X</missing></file>`, while the
compat stack's `withoutFileActivation()` pre-filter **removes all
file-activated profiles** regardless of `exists` vs `missing`.
This is arguably more semantically correct (the file really _is_ absent on
the consumer), but it's a divergence between the two stacks and the Javadoc
claim that "file-activated profiles always return false" is only true for
`<exists>` profiles, not `<missing>` ones.
If the intent is to match the compat behavior (suppress all file-activated
profiles), the simplest fix would be to also pre-filter file-activated profiles
out of `interpolatedProfiles` before passing to the selector, as the compat
stack does. Otherwise, this divergence should be documented as intentional.
--
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]
