[MNG-6802] Fix bug in FileProfileActivator (#347)

* [MNG-6802] FileProfileActivator changes exists

FileProfileActivator changes FileProfileActivator.exists
which lets flattened resolveCiFriendliesOnly depending
fail activating profile.

* [MNG-6802] Add java docs

* [MNG-6802] PR comments fixes

* Make ProfileActivationFilePathInterpolator a class
* call super.tearDown() last

* [MNG-6802] PR comments fixes

* Get rid of reusing local variables
* Rename file-> activationFile
* Move if ( missing ) to try block
* Update javadocs
* Replace string format with concatenation
* Replace FileReader to FileInputStream

* [MNG-6802] Update javadoc

* [MNG-6802] Switch to commons lang3 StringUtils

* [MNG-6802] Switch to commons lang3 StringUtils

* [MNG-6802] Switch to plexus-utils StringUtils back

* [MNG-6802] Switch to plexus-utils StringUtils back

* [MNG-6802] Resolve conflicts

* [MNG-6802] Upgrade tests

* [MNG-6802] Fix for windows

* [MNG-6802] Remove annotation

* [MNG-6802] Create a temp dir

* [MNG-6802] Remove space

* [MNG-6802] Small refactoring

* [MNG-6802] Use JUnit5 tempDir

* [MNG-6802] Use static import

* [MNG-6802] Remove plexus

* [MNG-6802] Remove plexus

* [MNG-6802] Fix checklstyle

* [MNG-6802] Restart build

* [MNG-6802] Restart build

* [MNG-6802] Restart build (last attempt)

* [MNG-6802] Fix PR comments

* [MNG-6802] Refactor try-catch

* [MNG-6802] Fix comments in FileProfileActivatorTest

* [MNG-6802] Use primitive

* [MNG-6802] Push down path

maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java

@@ -50,6 +50,7 @@
 import org.apache.maven.building.Source;
 import org.apache.maven.feature.Features;
 import org.apache.maven.model.Activation;
+import org.apache.maven.model.ActivationFile;
 import org.apache.maven.model.Build;
 import org.apache.maven.model.Dependency;
 import org.apache.maven.model.DependencyManagement;
@@ -74,11 +75,13 @@
 import org.apache.maven.model.normalization.ModelNormalizer;
 import org.apache.maven.model.path.ModelPathTranslator;
 import org.apache.maven.model.path.ModelUrlNormalizer;
+import org.apache.maven.model.path.ProfileActivationFilePathInterpolator;
 import org.apache.maven.model.plugin.LifecycleBindingsInjector;
 import org.apache.maven.model.plugin.PluginConfigurationExpander;
 import org.apache.maven.model.plugin.ReportConfigurationExpander;
 import org.apache.maven.model.plugin.ReportingConverter;
 import org.apache.maven.model.profile.DefaultProfileActivationContext;
+import org.apache.maven.model.profile.ProfileActivationContext;
 import org.apache.maven.model.profile.ProfileInjector;
 import org.apache.maven.model.profile.ProfileSelector;
 import org.apache.maven.model.resolution.InvalidRepositoryException;
@@ -87,6 +90,7 @@
 import org.apache.maven.model.resolution.WorkspaceModelResolver;
 import org.apache.maven.model.superpom.SuperPomProvider;
 import org.apache.maven.model.validation.ModelValidator;
+import org.codehaus.plexus.interpolation.InterpolationException;
 import org.codehaus.plexus.interpolation.MapBasedValueSource;
 import org.codehaus.plexus.interpolation.StringSearchInterpolator;
 import org.eclipse.sisu.Nullable;
@@ -153,6 +157,9 @@ public class DefaultModelBuilder
 
     private ModelMerger modelMerger = new FileToRawModelMerger();
 
+    @Inject
+    private ProfileActivationFilePathInterpolator profileActivationFilePathInterpolator;
+
     public DefaultModelBuilder setModelProcessor( ModelProcessor modelProcessor )
     {
         this.modelProcessor = modelProcessor;
@@ -255,6 +262,13 @@ public DefaultModelBuilder setReportingConverter( ReportingConverter reportingCo
         return this;
     }
 
+    public DefaultModelBuilder setProfileActivationFilePathInterpolator(
+            ProfileActivationFilePathInterpolator profileActivationFilePathInterpolator )
+    {
+        this.profileActivationFilePathInterpolator = profileActivationFilePathInterpolator;
+        return this;
+    }
+
     @Override
     public DefaultTransformerContextBuilder newTransformerContextBuilder()
     {
@@ -401,7 +415,11 @@ private Model readEffectiveModel( final ModelBuildingRequest request, final Defa
             // model normalization
             modelNormalizer.mergeDuplicates( tmpModel, request, problems );
 
-            Map<String, Activation> interpolatedActivations = getProfileActivations( tmpModel, false );
+            profileActivationContext.setProjectProperties( tmpModel.getProperties() );
+
+            Map<String, Activation> interpolatedActivations = getInterpolatedActivations( rawModel,
+                                                                                          profileActivationContext, 
+                                                                                          problems );
             injectProfileActivations( tmpModel, interpolatedActivations );
 
             // profile injection
@@ -482,6 +500,56 @@ else if ( !parentIds.add( parentData.getId() ) )
         return resultModel;
     }
 
+    private Map<String, Activation> getInterpolatedActivations( Model rawModel,
+                                                                DefaultProfileActivationContext context,
+                                                                DefaultModelProblemCollector problems )
+    {
+        Map<String, Activation> interpolatedActivations = getProfileActivations( rawModel, true );
+        for ( Activation activation : interpolatedActivations.values() )
+        {
+            if ( activation.getFile() != null )
+            {
+                replaceWithInterpolatedValue( activation.getFile(), context, problems );
+            }
+        }
+        return interpolatedActivations;
+    }
+
+    private void replaceWithInterpolatedValue( ActivationFile activationFile, ProfileActivationContext context,
+                                               DefaultModelProblemCollector problems  )
+    {
+        try
+        {
+            if ( isNotEmpty( activationFile.getExists() ) )
+            {
+                String path = activationFile.getExists();
+                String absolutePath = profileActivationFilePathInterpolator.interpolate( path, context );
+                activationFile.setExists( absolutePath );
+            }
+            else if ( isNotEmpty( activationFile.getMissing() ) )
+            {
+                String path = activationFile.getMissing();
+                String absolutePath = profileActivationFilePathInterpolator.interpolate( path, context );
+                activationFile.setMissing( absolutePath );
+            }
+        }
+        catch ( InterpolationException e )
+        {
+            String path = isNotEmpty(
+                    activationFile.getExists() ) ? activationFile.getExists() : activationFile.getMissing();
+
+            problems.add( new ModelProblemCollectorRequest( Severity.ERROR, Version.BASE ).setMessage(
+                    "Failed to interpolate file location " + path + ": " + e.getMessage() ).setLocation(
+                    activationFile.getLocation( isNotEmpty( activationFile.getExists() ) ? "exists" : "missing"  ) )
+                    .setException( e ) );
+        }
+    }
+
+    private static boolean isNotEmpty( String string ) 
+    {
+        return string != null && !string.isEmpty();
+    }
+
     @Override
     public ModelBuildingResult build( final ModelBuildingRequest request, final ModelBuildingResult result )
         throws ModelBuildingException

maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilderFactory.java

@@ -43,6 +43,7 @@
 import org.apache.maven.model.path.ModelPathTranslator;
 import org.apache.maven.model.path.ModelUrlNormalizer;
 import org.apache.maven.model.path.PathTranslator;
+import org.apache.maven.model.path.ProfileActivationFilePathInterpolator;
 import org.apache.maven.model.path.UrlNormalizer;
 import org.apache.maven.model.plugin.DefaultPluginConfigurationExpander;
 import org.apache.maven.model.plugin.DefaultReportConfigurationExpander;
@@ -111,7 +112,13 @@ protected ProfileSelector newProfileSelector()
     protected ProfileActivator[] newProfileActivators()
     {
         return new ProfileActivator[] { new JdkVersionProfileActivator(), new OperatingSystemProfileActivator(),
-            new PropertyProfileActivator(), new FileProfileActivator().setPathTranslator( newPathTranslator() ) };
+            new PropertyProfileActivator(), new FileProfileActivator()
+                        .setProfileActivationFilePathInterpolator( newProfileActivationFilePathInterpolator() ) };
+    }
+
+    protected ProfileActivationFilePathInterpolator newProfileActivationFilePathInterpolator()
+    {
+        return new ProfileActivationFilePathInterpolator().setPathTranslator( newPathTranslator() );
     }
 
     protected UrlNormalizer newUrlNormalizer()
@@ -232,6 +239,7 @@ public DefaultModelBuilder newInstance()
         modelBuilder.setPluginConfigurationExpander( newPluginConfigurationExpander() );
         modelBuilder.setReportConfigurationExpander( newReportConfigurationExpander() );
         modelBuilder.setReportingConverter( newReportingConverter() );
+        modelBuilder.setProfileActivationFilePathInterpolator( newProfileActivationFilePathInterpolator() );
 
         return modelBuilder;
     }

maven-model-builder/src/main/java/org/apache/maven/model/path/ProfileActivationFilePathInterpolator.java

@@ -0,0 +1,103 @@
+package org.apache.maven.model.path;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.maven.model.ActivationFile;
+import org.apache.maven.model.profile.ProfileActivationContext;
+import org.codehaus.plexus.interpolation.AbstractValueSource;
+import org.codehaus.plexus.interpolation.InterpolationException;
+import org.codehaus.plexus.interpolation.MapBasedValueSource;
+import org.codehaus.plexus.interpolation.RegexBasedInterpolator;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.io.File;
+
+/**
+ * Finds an absolute path for {@link ActivationFile#getExists()} or {@link ActivationFile#getMissing()}
+ *
+ * @author Ravil Galeyev
+ */
+@Named
+@Singleton
+public class ProfileActivationFilePathInterpolator
+{
+
+    @Inject
+    private PathTranslator pathTranslator;
+
+    public ProfileActivationFilePathInterpolator setPathTranslator( PathTranslator pathTranslator )
+    {
+        this.pathTranslator = pathTranslator;
+        return this;
+    }
+
+    /**
+     * Interpolates given {@code path}.
+     *
+     * @return absolute path or {@code null} if the input was {@code null}
+     */
+    public String interpolate( String path, ProfileActivationContext context ) throws InterpolationException
+    {
+        if ( path == null )
+        {
+            return null;
+        }
+
+        RegexBasedInterpolator interpolator = new RegexBasedInterpolator();
+
+        final File basedir = context.getProjectDirectory();
+
+        if ( basedir != null )
+        {
+            interpolator.addValueSource( new AbstractValueSource( false )
+            {
+                @Override
+                public Object getValue( String expression )
+                {
+                    /*
+                     * We intentionally only support ${basedir} and not ${project.basedir} as the latter form
+                     * would suggest that other project.* expressions can be used which is beyond the design.
+                     */
+                    if ( "basedir".equals( expression ) )
+                    {
+                        return basedir.getAbsolutePath();
+                    }
+                    return null;
+                }
+            } );
+        }
+        else if ( path.contains( "${basedir}" ) )
+        {
+            return null;
+        }
+
+        interpolator.addValueSource( new MapBasedValueSource( context.getProjectProperties() ) );
+
+        interpolator.addValueSource( new MapBasedValueSource( context.getUserProperties() ) );
+
+        interpolator.addValueSource( new MapBasedValueSource( context.getSystemProperties() ) );
+
+        String absolutePath = interpolator.interpolate( path, "" );
+
+        return pathTranslator.alignToBaseDirectory( absolutePath, basedir );
+    }
+}

maven-model-builder/src/main/java/org/apache/maven/model/profile/activation/FileProfileActivator.java

@@ -28,15 +28,13 @@
 import org.apache.maven.model.Activation;
 import org.apache.maven.model.ActivationFile;
 import org.apache.maven.model.Profile;
-import org.apache.maven.model.building.ModelProblemCollector;
 import org.apache.maven.model.building.ModelProblem.Severity;
 import org.apache.maven.model.building.ModelProblem.Version;
+import org.apache.maven.model.building.ModelProblemCollector;
 import org.apache.maven.model.building.ModelProblemCollectorRequest;
-import org.apache.maven.model.path.PathTranslator;
+import org.apache.maven.model.path.ProfileActivationFilePathInterpolator;
 import org.apache.maven.model.profile.ProfileActivationContext;
-import org.codehaus.plexus.interpolation.AbstractValueSource;
+import org.codehaus.plexus.interpolation.InterpolationException;
-import org.codehaus.plexus.interpolation.MapBasedValueSource;
-import org.codehaus.plexus.interpolation.RegexBasedInterpolator;
 import org.codehaus.plexus.util.StringUtils;
 
 /**
@@ -58,11 +56,12 @@ public class FileProfileActivator
 {
 
     @Inject
-    private PathTranslator pathTranslator;
+    private ProfileActivationFilePathInterpolator profileActivationFilePathInterpolator;
 
-    public FileProfileActivator setPathTranslator( PathTranslator pathTranslator )
+    public FileProfileActivator setProfileActivationFilePathInterpolator(
+            ProfileActivationFilePathInterpolator profileActivationFilePathInterpolator )
     {
-        this.pathTranslator = pathTranslator;
+        this.profileActivationFilePathInterpolator = profileActivationFilePathInterpolator;
         return this;
     }
 
@@ -101,64 +100,23 @@ else if ( StringUtils.isNotEmpty( file.getMissing() ) )
             return false;
         }
 
-        RegexBasedInterpolator interpolator = new RegexBasedInterpolator();
-
-        final File basedir = context.getProjectDirectory();
-
-        if ( basedir != null )
-        {
-            interpolator.addValueSource( new AbstractValueSource( false )
-            {
-                @Override
-                public Object getValue( String expression )
-                {
-                    /*
-                     * NOTE: We intentionally only support ${basedir} and not ${project.basedir} as the latter form
-                     * would suggest that other project.* expressions can be used which is however beyond the design.
-                     */
-                    if ( "basedir".equals( expression ) )
-                    {
-                        return basedir.getAbsolutePath();
-                    }
-                    return null;
-                }
-            } );
-        }
-        else if ( path.contains( "${basedir}" ) )
-        {
-            return false;
-        }
-
-        interpolator.addValueSource( new MapBasedValueSource( context.getProjectProperties() ) );
-
-        interpolator.addValueSource( new MapBasedValueSource( context.getUserProperties() ) );
-
-        interpolator.addValueSource( new MapBasedValueSource( context.getSystemProperties() ) );
-
         try
         {
-            path = interpolator.interpolate( path, "" );
+            path = profileActivationFilePathInterpolator.interpolate( path, context );
         }
-        catch ( Exception e )
+        catch ( InterpolationException e )
         {
             problems.add( new ModelProblemCollectorRequest( Severity.ERROR, Version.BASE )
                     .setMessage( "Failed to interpolate file location " + path + " for profile " + profile.getId()
-                                 + ": " + e.getMessage() )
+                            + ": " + e.getMessage() )
                     .setLocation( file.getLocation( missing ? "missing" : "exists" ) )
                     .setException( e ) );
             return false;
         }
 
-        path = pathTranslator.alignToBaseDirectory( path, basedir );
+        if ( path == null )
-
-        // replace activation value with interpolated value
-        if ( missing )
         {
-            file.setMissing( path );
+            return false;
-        }
-        else
-        {
-            file.setExists( path );
         }
 
         File f = new File( path );

maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderFactoryTest.java

@@ -20,10 +20,15 @@
  */
 
 import java.io.File;
+import java.io.FileInputStream;
+import java.nio.file.Paths;
 
+import org.apache.maven.model.Model;
+import org.apache.maven.model.io.xpp3.MavenXpp3Reader;
 import org.codehaus.plexus.util.xml.Xpp3Dom;
 import org.junit.jupiter.api.Test;
 
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 
@@ -33,9 +38,11 @@
 public class DefaultModelBuilderFactoryTest
 {
 
+    private static final String BASE_DIR = Paths.get( "src", "test", "resources", "poms", "factory" ).toString();
+
     private File getPom( String name )
     {
-        return new File( "src/test/resources/poms/factory/" + name + ".xml" ).getAbsoluteFile();
+        return new File( Paths.get( BASE_DIR, name + ".xml"  ).toString() ).getAbsoluteFile();
     }
 
     @Test
@@ -58,4 +65,30 @@ public void testCompleteWiring()
         assertEquals( "  1.5  ", conf.getChild( "target" ).getValue() );
     }
 
+    @Test
+    public void testPomChanges() throws Exception
+    {
+        ModelBuilder builder = new DefaultModelBuilderFactory().newInstance();
+        assertNotNull( builder );
+        File pom = getPom( "simple" );
+
+        String originalExists = readPom( pom ).getProfiles().get( 1 ).getActivation().getFile().getExists();
+
+        DefaultModelBuildingRequest request = new DefaultModelBuildingRequest();
+        request.setProcessPlugins( true );
+        request.setPomFile( pom );
+        ModelBuildingResult result = builder.build( request );
+        String resultExists = result.getRawModel().getProfiles().get( 1 ).getActivation().getFile().getExists();
+
+        assertEquals( originalExists, resultExists );
+        assertTrue( result.getEffectiveModel().getProfiles().get( 1 ).getActivation().getFile().getExists()
+                .contains( BASE_DIR ) );
+    }
+
+    private static Model readPom( File file ) throws Exception
+    {
+        MavenXpp3Reader reader = new MavenXpp3Reader();
+
+        return reader.read( new FileInputStream( file ) );
+    }
 }

maven-model-builder/src/test/java/org/apache/maven/model/profile/activation/FileProfileActivatorTest.java

@@ -0,0 +1,131 @@
+package org.apache.maven.model.profile.activation;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.maven.model.Activation;
+import org.apache.maven.model.ActivationFile;
+import org.apache.maven.model.Profile;
+import org.apache.maven.model.path.DefaultPathTranslator;
+import org.apache.maven.model.path.ProfileActivationFilePathInterpolator;
+import org.apache.maven.model.profile.DefaultProfileActivationContext;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Tests {@link FileProfileActivator}.
+ *
+ * @author Ravil Galeyev
+ */
+public class FileProfileActivatorTest extends AbstractProfileActivatorTest<FileProfileActivator>
+{
+
+    @TempDir
+    Path tempDir;
+
+    private final DefaultProfileActivationContext context = new DefaultProfileActivationContext();
+
+    public FileProfileActivatorTest()
+    {
+        super( FileProfileActivator.class );
+    }
+
+    @BeforeEach
+    public void setUp() throws Exception
+    {
+        super.setUp();
+        activator.setProfileActivationFilePathInterpolator(
+                new ProfileActivationFilePathInterpolator().setPathTranslator( new DefaultPathTranslator() ) );
+
+        context.setProjectDirectory( new File( tempDir.toString() ) );
+
+        File file = new File( tempDir.resolve( "file.txt" ).toString() );
+        if ( !file.createNewFile() )
+        {
+            throw new IOException( "Can't create " + file );
+        }
+    }
+
+    @Test
+    public void testIsActiveNoFile()
+    {
+        assertActivation( false, newExistsProfile( null ), context );
+        assertActivation( false, newExistsProfile( "someFile.txt" ), context );
+        assertActivation( false, newExistsProfile( "${basedir}/someFile.txt" ), context );
+
+        assertActivation( false, newMissingProfile( null ), context );
+        assertActivation( true, newMissingProfile( "someFile.txt" ), context );
+        assertActivation( true, newMissingProfile( "${basedir}/someFile.txt" ), context );
+    }
+
+    @Test
+    public void testIsActiveExistsFileExists()
+    {
+        assertActivation( true, newExistsProfile( "file.txt" ), context );
+        assertActivation( true, newExistsProfile( "${basedir}" ), context );
+        assertActivation( true, newExistsProfile( "${basedir}/" + "file.txt" ), context );
+
+        assertActivation( false, newMissingProfile( "file.txt" ), context );
+        assertActivation( false, newMissingProfile( "${basedir}" ), context );
+        assertActivation( false, newMissingProfile( "${basedir}/" + "file.txt" ), context );
+    }
+
+    @Test
+    public void testIsActiveExistsLeavesFileUnchanged()
+    {
+        Profile profile = newExistsProfile( "file.txt" );
+        assertEquals( "file.txt", profile.getActivation().getFile().getExists() );
+
+        assertActivation( true, profile, context );
+
+        assertEquals( "file.txt", profile.getActivation().getFile().getExists() );
+    }
+
+    private Profile newExistsProfile( String filePath )
+    {
+        ActivationFile activationFile = new ActivationFile();
+        activationFile.setExists( filePath );
+        return newProfile( activationFile );
+    }
+
+    private Profile newMissingProfile( String filePath )
+    {
+        ActivationFile activationFile = new ActivationFile();
+        activationFile.setMissing( filePath );
+        return newProfile( activationFile );
+    }
+
+    private Profile newProfile( ActivationFile activationFile )
+    {
+        Activation activation = new Activation();
+        activation.setFile( activationFile );
+
+        Profile profile = new Profile();
+        profile.setActivation( activation );
+
+        return profile;
+    }
+}