[3.10.x] Fix #13084: sandbox profile activation context for external model builds

[gnodet]

Backport of #13112 to maven-3.10.x.

Cherry-picked and manually adapted from the master commits:

• e58a96a Fix 3.10.x fails on org.jboss.resteasy:resteasy-client:jar:7.0.0.Final #13084: sandbox profile activation context for external model builds
• 8ff7ca6 Address review: fix Javadoc method ref and file-activation asymmetry
• f2bf110 fix: move model property lookup from activator to sandbox context

Conflicts resolved by adapting to the flat module layout of maven-3.10.x:

• compat/maven-model-builder/ → maven-model-builder/ (flat layout)
• impl/maven-impl/ changes have no equivalent on 3.10.x (not present)
• its/core-it-suite/ changes dropped (ITs are in a separate repo on this branch)

See #13112 for the full rationale and design.

[gnodet-bot]

Backport of #13112 to maven-3.10.x. Cherry-pick is clean and the approach is consistent across both compat and impl layers.

Design note: The compat PropertyProfileActivator adds context.getProjectProperties() as a third lookup for all builds (not just external), which differs slightly from #13112 on master (where model properties were merged into system properties only in the sandbox context). This is actually more consistent: the impl layer's PropertyProfileActivator also adds model property lookup unconditionally, and the test coverage confirms the intent. POM-declared property activation now works uniformly in both project and external builds on the compat path.

No issues found. Solid backport.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet]

⚠️ Merge conflict — non-trivial

This branch was created from master instead of maven-3.10.x, resulting in 5837 commits ahead / 707 behind the target branch. The merge produces hundreds of conflicts across restructured modules (maven-model-builder, maven-embedder, maven-resolver-provider, etc.).

The actual fix is only in 2 commits (4c8b837 and a10fb4e), but they cannot be merged as-is due to the divergent branch base.

Recommended fix: Close this PR, create a new branch from maven-3.10.x, and cherry-pick/adapt the fix commits.

This comment was posted by an automated babysit agent.

[gnodet-bot]

Re-review of rebased/squashed commit (previously approved a10fb4e, now f97b62193a).

The squash absorbed the test-fix commit (a10fb4e) into the main commit cleanly. The design is solid: sandboxing via an anonymous ProfileActivationContext rather than modifying PropertyProfileActivator is the right call — it keeps the activator stateless and avoids unintended side effects on project builds.

One performance issue on the getSystemProperties() override.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

⚠️ 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:

Suggested change

	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;
	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:

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).

[gnodet]

Fixed in f80e41d. Pre-computed the merged map before the anonymous class — one allocation per external model instead of one per getSystemProperties() call. Also wrapped the result in Collections.unmodifiableMap() to make the no-mutation contract explicit.

[gnodet-bot]

Re-review of f80e41d33a (performance fix commit).

Prior finding: getSystemProperties() allocated a new HashMap on every call — addressed. The pre-computed mergedSystemProps is captured before the anonymous class construction, wrapped in Collections.unmodifiableMap(), and returned by value on every call. Zero allocation per getSystemProperties() invocation — correct fix.

Test coverage gap (non-blocking): testExternalModelNegatedPropertyDefaultOnIsPreserved tests the "property absent → profile fires" path, but there is no test for the suppression direction — a consumer setting skip.defaults as a system property (not -D) would suppress the dependency's default-on profile, since system properties pass through the sandbox. This is intentional behavior (system props are platform facts), but it's worth a test case to lock it in.

All other findings from prior reviews are addressed. Design is sound.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[github-actions]

@cstamas Please assign appropriate label to PR according to the type of change.