Improvements after review feedback

Author: gnodet

impl/maven-core/src/test/java/org/apache/maven/project/DefaultMavenProjectBuilderTest.java

@@ -414,17 +414,15 @@ void testActivatedDefaultProfileBySource(String fsName, Configuration fsConfig,
             String actualLocation = location.getSource().getLocation();
             String expectedPath = "pom-with-profiles" + separator + "pom.xml";
 
-            // Log the actual vs expected for debugging
-            System.out.println("=== Cross-Platform Path Test [" + fsName + "] ===");
-            System.out.println("Expected path pattern: " + expectedPath);
-            System.out.println("Actual location: " + actualLocation);
-            System.out.println("Contains expected pattern: " + actualLocation.contains(expectedPath));
-            System.out.println("File.separator on this system: '" + File.separator + "'");
-
             // The test will pass with File.separator but this shows the platform differences
             assertThat(
                     "Location should contain path with proper separators for " + fsName + " (actual: " + actualLocation
-                            + ")",
+                            + ")\n"
+                            + "=== Cross-Platform Path Test [" + fsName + "] ===\n"
+                            + "Expected path pattern: " + expectedPath + "\n"
+                            + "Actual location: " + actualLocation + "\n"
+                            + "Contains expected pattern: " + actualLocation.contains(expectedPath) + "\n"
+                            + "File.separator on this system: '" + File.separator + "'",
                     actualLocation,
                     containsString("pom-with-profiles/pom.xml"));
         }
@@ -487,17 +485,15 @@ void testActivatedExternalProfileBySource(String fsName, Configuration fsConfig,
             String actualLocation = location.getSource().getLocation();
             String expectedPath = "pom-with-profiles" + separator + "pom.xml";
 
-            // Log the actual vs expected for debugging
-            System.out.println("=== Cross-Platform Path Test [" + fsName + "] - External Profile ===");
-            System.out.println("Expected path pattern: " + expectedPath);
-            System.out.println("Actual location: " + actualLocation);
-            System.out.println("Contains expected pattern: " + actualLocation.contains(expectedPath));
-            System.out.println("File.separator on this system: '" + File.separator + "'");
-
             // The test will pass with File.separator but this shows the platform differences
             assertThat(
                     "Location should contain path with proper separators for " + fsName + " (actual: " + actualLocation
-                            + ")",
+                            + ")\n"
+                            + "=== Cross-Platform Path Test [" + fsName + "] - External Profile ===\n"
+                            + "Expected path pattern: " + expectedPath + "\n"
+                            + "Actual location: " + actualLocation + "\n"
+                            + "Contains expected pattern: " + actualLocation.contains(expectedPath) + "\n"
+                            + "File.separator on this system: '" + File.separator + "'",
                     actualLocation,
                     containsString("pom-with-profiles/pom.xml"));
 

impl/maven-impl/src/main/java/org/apache/maven/impl/cache/DefaultRequestCache.java

@@ -88,7 +88,7 @@ public DefaultRequestCache() {
      * @param stats the cache statistics to format
      * @return a formatted string containing cache statistics
      */
-    private static String formatCacheStatistics(CacheStatistics stats) {
+    static String formatCacheStatistics(CacheStatistics stats) {
         StringBuilder sb = new StringBuilder();
         sb.append("Request Cache Statistics:\n");
         sb.append("  Total requests: ").append(stats.getTotalRequests()).append("\n");

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelObjectPool.java

@@ -30,6 +30,8 @@
 import org.apache.maven.api.model.Dependency;
 import org.apache.maven.api.model.ModelObjectProcessor;
 import org.apache.maven.impl.cache.Cache;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Default implementation of ModelObjectProcessor that provides memory optimization
@@ -55,6 +57,8 @@ public class DefaultModelObjectPool implements ModelObjectProcessor {
     private static final Map<Class<?>, AtomicLong> CACHE_HITS = new ConcurrentHashMap<>();
     private static final Map<Class<?>, AtomicLong> CACHE_MISSES = new ConcurrentHashMap<>();
 
+    private static final Logger LOGGER = LoggerFactory.getLogger(DefaultModelObjectPool.class);
+
     @Override
     @SuppressWarnings("unchecked")
     public <T> T process(T object) {
@@ -111,7 +115,7 @@ private static Cache.ReferenceType getReferenceTypeForClass(Class<?> objectType)
             try {
                 return Cache.ReferenceType.valueOf(perTypeValue.toUpperCase());
             } catch (IllegalArgumentException e) {
-                System.err.println("Unknown reference type for " + className + ": " + perTypeValue + ", using default");
+                LOGGER.warn("Unknown reference type for " + className + ": " + perTypeValue + ", using default");
             }
         }
 
@@ -128,7 +132,7 @@ private static Cache.ReferenceType getDefaultReferenceType() {
                     System.getProperty(Constants.MAVEN_MODEL_PROCESSOR_REFERENCE_TYPE, Cache.ReferenceType.HARD.name());
             return Cache.ReferenceType.valueOf(referenceTypeProperty.toUpperCase());
         } catch (IllegalArgumentException e) {
-            System.err.println("Unknown default reference type, using HARD");
+            LOGGER.warn("Unknown default reference type, using HARD");
             return Cache.ReferenceType.HARD;
         }
     }

impl/maven-impl/src/test/java/org/apache/maven/impl/cache/ReferenceTypeStatisticsIntegrationTest.java

@@ -93,4 +93,75 @@ void shouldHandleEmptyStatistics() {
         var refTypeStats = statistics.getReferenceTypeStatistics();
         assertTrue(refTypeStats.isEmpty());
     }
+
+    @Test
+    void shouldDisplayReferenceTypeStatisticsInOutput() {
+        CacheStatistics statistics = new CacheStatistics();
+
+        // Simulate cache usage with different reference types
+        statistics.recordCacheCreation("HARD", "HARD", CacheRetention.SESSION_SCOPED);
+        statistics.recordCacheCreation("SOFT", "WEAK", CacheRetention.REQUEST_SCOPED);
+        statistics.recordCacheCreation("WEAK", "SOFT", CacheRetention.PERSISTENT);
+
+        // Simulate cache accesses
+        statistics.recordCacheAccess("HARD", "HARD", true);
+        statistics.recordCacheAccess("HARD", "HARD", true);
+        statistics.recordCacheAccess("HARD", "HARD", false);
+
+        statistics.recordCacheAccess("SOFT", "WEAK", true);
+        statistics.recordCacheAccess("SOFT", "WEAK", false);
+        statistics.recordCacheAccess("SOFT", "WEAK", false);
+
+        statistics.recordCacheAccess("WEAK", "SOFT", false);
+
+        // Simulate some regular cache statistics
+        statistics.recordHit("TestRequest", CacheRetention.SESSION_SCOPED);
+        statistics.recordMiss("TestRequest", CacheRetention.SESSION_SCOPED);
+
+        // Capture the formatted output (not used in this test, but could be useful for future enhancements)
+
+        String output = DefaultRequestCache.formatCacheStatistics(statistics);
+
+        // Verify that reference type information is included
+        assertTrue(output.contains("Reference type usage:"), "Should contain reference type section\n" + output);
+        assertTrue(output.contains("HARD/HARD:"), "Should show HARD/HARD reference type\n" + output);
+        assertTrue(output.contains("SOFT/WEAK:"), "Should show SOFT/WEAK reference type\n" + output);
+        assertTrue(output.contains("WEAK/SOFT:"), "Should show WEAK/SOFT reference type\n" + output);
+        assertTrue(output.contains("caches"), "Should show cache creation count\n" + output);
+        assertTrue(output.contains("accesses"), "Should show access count\n" + output);
+        assertTrue(output.contains("hit ratio"), "Should show hit ratio\n" + output);
+
+        // Verify that different hit ratios are shown correctly
+        assertTrue(output.contains("66.7%") || output.contains("66.6%"), "Should show HARD/HARD hit ratio (~66.7%)");
+        assertTrue(output.contains("33.3%"), "Should show SOFT/WEAK hit ratio (33.3%)");
+        assertTrue(output.contains("0.0%"), "Should show WEAK/SOFT hit ratio (0.0%)");
+    }
+
+    @Test
+    void shouldShowMemoryPressureIndicators() {
+        CacheStatistics statistics = new CacheStatistics();
+
+        // Create scenario that might indicate memory pressure
+        statistics.recordCacheCreation("HARD", "HARD", CacheRetention.SESSION_SCOPED);
+        statistics.recordCacheCreation("SOFT", "SOFT", CacheRetention.SESSION_SCOPED);
+
+        // Simulate many cache accesses with hard references (potential OOM risk)
+        for (int i = 0; i < 1000; i++) {
+            statistics.recordCacheAccess("HARD", "HARD", true);
+        }
+
+        // Simulate some soft reference usage
+        for (int i = 0; i < 100; i++) {
+            statistics.recordCacheAccess("SOFT", "SOFT", i % 2 == 0);
+        }
+
+        String output = DefaultRequestCache.formatCacheStatistics(statistics);
+
+        System.out.println("=== Memory Pressure Analysis ===");
+        System.out.println(output);
+
+        // Should show high usage of hard references
+        assertTrue(output.contains("HARD/HARD:"), "Should show hard reference usage");
+        assertTrue(output.contains("1000 accesses"), "Should show high access count for hard references");
+    }
 }

impl/maven-impl/src/test/java/org/apache/maven/impl/cache/ReferenceTypeStatisticsTest.java

@@ -22,23 +22,18 @@ This document describes the enhanced cache configuration functionality in Maven'
 
 ## Overview
 
-The DefaultRequestCache has been enhanced to support configurable reference types and cache scopes through user-defined selectors. This allows fine-grained control over caching behavior for different request types.
+The DefaultRequestCache supports configurable reference types and cache scopes through user-defined selectors. This allows fine-grained control over caching behavior for different request types.
 
 ## Key Features
 
 ### 1. Early Return for ProtoSession
-- The `doCache` method now returns early when the request session is not a `Session` instance (e.g., `ProtoSession`)
-- This prevents caching attempts for non-session contexts
+- The `doCache` method now returns early when the request session is not a `Session` instance (e.g., `ProtoSession`).
+- This prevents caching attempts for non-session contexts.
 
 ### 2. Configurable Cache Behavior
-- Cache scope and reference type can be configured via session user properties
-- Configuration uses CSS-like selectors to match request types
-- Supports parent-child request relationships
-
-### 3. Backward Compatibility
-- Existing `CacheMetadata` interface still supported
-- Default behavior unchanged when no configuration provided
-- Legacy hardcoded behavior maintained as fallback
+- Cache scope and reference type can be configured via session user properties.
+- Configuration uses CSS-like selectors to match request types.
+- Supports parent-child request relationships.
 
 ## Configuration Syntax
 
@@ -59,9 +54,9 @@ Where:
 - `ref`: Reference type for cache entries (optional)
 
 **Note**:
-- You can specify only `scope` or only `ref` - missing values will be merged from less specific selectors or use defaults
-- Selectors match against all interfaces implemented by the request class, not just the class name
-- This allows matching against `ModelBuilderRequest` interface even if the actual class is `DefaultModelBuilderRequest`
+- You can specify only `scope` or only `ref` - missing values will be merged from less specific selectors or use defaults.
+- Selectors match against all interfaces implemented by the request class, not just the class name.
+- This allows matching against `ModelBuilderRequest` interface even if the actual class is `DefaultModelBuilderRequest`.
 
 ### Available Values
 
@@ -139,39 +134,6 @@ For a `ModelBuilderRequest` with `ModelBuildRequest` parent:
 
 ## Implementation Details
 
-### New Classes
-- `CacheConfig`: Record holding complete scope and reference type configuration
-- `PartialCacheConfig`: Record holding partial configuration (allows null values)
-- `CacheSelector`: Represents a selector rule with matching logic
-- `CacheSelectorParser`: Parses configuration strings into selectors
-- `CacheConfigurationResolver`: Resolves and merges configuration for requests
-
-### Modified Classes
-- `DefaultRequestCache.doCache()`: Enhanced with configurable behavior
-- `CacheSelector.matches()`: Enhanced to match against all implemented interfaces
-- Early return for non-Session requests
-- Removed hardcoded reference types
-- Integrated configuration resolution
-
-## Migration Guide
-
-### For Users
-- No changes required for existing builds
-- New configuration is opt-in via user properties
-- Existing behavior preserved when no configuration provided
-
-### For Developers
-- `CacheMetadata` interface still supported for backward compatibility
-- New configuration takes precedence over `CacheMetadata`
-- Default reference types changed to SOFT for consistency
-
-## Testing
-
-The implementation includes comprehensive tests:
-- `CacheConfigurationTest`: Unit tests for configuration parsing and resolution
-- Integration tests for selector matching and priority
-- Backward compatibility tests
-
 ## Performance Considerations
 
 - Configuration parsing is cached per session to avoid re-parsing