gnodet-bot commented on code in PR #13112:
URL: https://github.com/apache/maven/pull/13112#discussion_r4004677279
##########
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:
💡 **Still not addressed (raised in previous review):** The comment
references `DefaultModelBuilder.getExternalActivationContext()` but the actual
method is `externalActivationContext()` (no `get` prefix).
```suggestion
// In external model builds the caller suppresses user properties
via a sandboxed context
// (see DefaultModelBuilder.externalActivationContext()), so
consumer -D flags cannot
```
##########
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:
⚠️ **Still not addressed (raised in previous review): 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 is a divergence between the two stacks. 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]
