dehasi commented on a change in pull request #347: URL: https://github.com/apache/maven/pull/347#discussion_r430181500
########## File path: maven-model-builder/src/test/java/org/apache/maven/model/profile/activation/FileProfileActivatorTest.java ########## @@ -0,0 +1,145 @@ +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.building.DefaultModelBuilder; +import org.apache.maven.model.path.DefaultPathTranslator; +import org.apache.maven.model.path.DefaultProfileActivationFilePathInterpolator; +import org.apache.maven.model.profile.DefaultProfileActivationContext; + +import java.io.File; + +/** + * Tests {@link FileProfileActivator}. + * + * @author Ravil Galeyev + */ +public class FileProfileActivatorTest extends AbstractProfileActivatorTest<FileProfileActivator> +{ + private static final String PATH = "src/test/resources/"; + private static final String FILE = "file.txt"; + + private final DefaultProfileActivationContext context = new DefaultProfileActivationContext(); + + public FileProfileActivatorTest() + { + super( FileProfileActivator.class ); + } + + @Override + protected void setUp() throws Exception + { + super.setUp(); + activator.setProfileActivationFilePathInterpolator( + new DefaultProfileActivationFilePathInterpolator().setPathTranslator( new DefaultPathTranslator() ) ); + + context.setProjectDirectory( new File( PATH ) ); + + File file = new File( PATH + FILE ); + if ( !file.exists() ) + { + if ( !file.createNewFile() ) + { + throw new RuntimeException( String.format( "Can't create %s", file ) ); + } + } + } + + + public void test_isActive_noFile() + { + 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 ); + } + + public void test_isActiveExists_fileExists() + { + assertActivation( true, newExistsProfile( FILE ), context ); + assertActivation( true, newExistsProfile( "${basedir}" ), context ); + assertActivation( true, newExistsProfile( "${basedir}/" + FILE ), context ); + + assertActivation( false, newMissingProfile( FILE ), context ); + assertActivation( false, newMissingProfile( "${basedir}" ), context ); + assertActivation( false, newMissingProfile( "${basedir}/" + FILE ), context ); + } + + public void test_isActiveExists_leavesFileUnchanged() + { + Profile profile = newExistsProfile( FILE ); + assertEquals( profile.getActivation().getFile().getExists(), FILE ); + + assertActivation( true, profile, context ); + + assertEquals( profile.getActivation().getFile().getExists(), FILE ); + } + + private Profile newExistsProfile( String filePath ) + { + return newProfile( filePath, true ); + } + + private Profile newMissingProfile( String filePath ) + { + return newProfile( filePath, false ); + } + + private Profile newProfile( String filePath, boolean exists ) + { + ActivationFile activationFile = new ActivationFile(); + if ( exists ) + { + activationFile.setExists( filePath ); + } + else + { + activationFile.setMissing( filePath ); + } + + Activation activation = new Activation(); + activation.setFile( activationFile ); + + Profile profile = new Profile(); + profile.setActivation( activation ); + + return profile; + } + + @Override + protected void tearDown() throws Exception + { + super.tearDown(); Review comment: I moved it down. Thank you for pointing. ########## File path: maven-model-builder/src/main/java/org/apache/maven/model/path/ProfileActivationFilePathInterpolator.java ########## @@ -0,0 +1,38 @@ +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.profile.ProfileActivationContext; +import org.codehaus.plexus.interpolation.InterpolationException; + +/** + * Interpolates path for {@link org.apache.maven.model.ActivationFile}. + * + * @author Ravil Galeyev + */ +public interface ProfileActivationFilePathInterpolator Review comment: I tried to keep the same code style. I took `UrlNormalizer` as an example. Anyway, I made `ProfileActivationFilePathInterpolator` a class. If an interface looks better, let me know, I'll revert :) ########## File path: maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java ########## @@ -444,6 +461,64 @@ else if ( !parentIds.add( parentData.getId() ) ) return result; } + 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 file, ProfileActivationContext context, + DefaultModelProblemCollector problems ) + { + String path; + boolean missing; + + if ( StringUtils.isNotEmpty( file.getExists() ) ) Review comment: I rename `file` -> `activationFile` to make it clear. Therefore `activationFile.getExists()` or `activationFile.getMissing()` return `String`. `StringUtils.isNotEmpty` checks if the string is `null` or empty. @elharo Does it make sense for you now? ########## File path: maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java ########## @@ -444,6 +461,64 @@ else if ( !parentIds.add( parentData.getId() ) ) return result; } + 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 file, ProfileActivationContext context, + DefaultModelProblemCollector problems ) + { + String path; + boolean missing; + + if ( StringUtils.isNotEmpty( file.getExists() ) ) + { + path = file.getExists(); + missing = false; + } + else if ( StringUtils.isNotEmpty( file.getMissing() ) ) + { + path = file.getMissing(); + missing = true; + } + else + { + return; + } + + try + { + path = profileActivationFilePathInterpolator.interpolate( path, context ); Review comment: Good point. I introduced `absolutePath` ########## File path: maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java ########## @@ -444,6 +461,64 @@ else if ( !parentIds.add( parentData.getId() ) ) return result; } + 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 file, ProfileActivationContext context, + DefaultModelProblemCollector problems ) + { + String path; + boolean missing; + + if ( StringUtils.isNotEmpty( file.getExists() ) ) + { + path = file.getExists(); + missing = false; + } + else if ( StringUtils.isNotEmpty( file.getMissing() ) ) + { + path = file.getMissing(); + missing = true; + } + else + { + return; + } + + try + { + path = profileActivationFilePathInterpolator.interpolate( path, context ); + } + catch ( InterpolationException e ) + { + problems.add( new ModelProblemCollectorRequest( Severity.ERROR, Version.BASE ).setMessage( + "Failed to interpolate file location " + path + ": " + e.getMessage() ).setLocation( + file.getLocation( missing ? "missing" : "exists" ) ).setException( e ) ); + return; + } + + if ( missing ) Review comment: Moved into `try` block ########## File path: maven-model-builder/src/test/java/org/apache/maven/model/profile/activation/FileProfileActivatorTest.java ########## @@ -0,0 +1,144 @@ +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 java.io.File; + +/** + * Tests {@link FileProfileActivator}. + * + * @author Ravil Galeyev + */ +public class FileProfileActivatorTest extends AbstractProfileActivatorTest<FileProfileActivator> +{ + private static final String PATH = "src/test/resources/"; + private static final String FILE = "file.txt"; + + private final DefaultProfileActivationContext context = new DefaultProfileActivationContext(); + + public FileProfileActivatorTest() + { + super( FileProfileActivator.class ); + } + + @Override + protected void setUp() throws Exception + { + super.setUp(); + activator.setProfileActivationFilePathInterpolator( + new ProfileActivationFilePathInterpolator().setPathTranslator( new DefaultPathTranslator() ) ); + + context.setProjectDirectory( new File( PATH ) ); + + File file = new File( PATH + FILE ); + if ( !file.exists() ) + { + if ( !file.createNewFile() ) + { + throw new RuntimeException( String.format( "Can't create %s", file ) ); + } + } + } + + + public void test_isActive_noFile() + { + 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 ); + } + + public void test_isActiveExists_fileExists() + { + assertActivation( true, newExistsProfile( FILE ), context ); + assertActivation( true, newExistsProfile( "${basedir}" ), context ); + assertActivation( true, newExistsProfile( "${basedir}/" + FILE ), context ); + + assertActivation( false, newMissingProfile( FILE ), context ); + assertActivation( false, newMissingProfile( "${basedir}" ), context ); + assertActivation( false, newMissingProfile( "${basedir}/" + FILE ), context ); + } + + public void test_isActiveExists_leavesFileUnchanged() + { + Profile profile = newExistsProfile( FILE ); + assertEquals( profile.getActivation().getFile().getExists(), FILE ); + + assertActivation( true, profile, context ); + + assertEquals( profile.getActivation().getFile().getExists(), FILE ); + } + + private Profile newExistsProfile( String filePath ) + { + return newProfile( filePath, true ); + } + + private Profile newMissingProfile( String filePath ) + { + return newProfile( filePath, false ); + } + + private Profile newProfile( String filePath, boolean exists ) + { + ActivationFile activationFile = new ActivationFile(); + if ( exists ) + { + activationFile.setExists( filePath ); + } + else + { + activationFile.setMissing( filePath ); + } + + Activation activation = new Activation(); + activation.setFile( activationFile ); + + Profile profile = new Profile(); + profile.setActivation( activation ); + + return profile; + } + + @Override + protected void tearDown() throws Exception + { + File file = new File( PATH + FILE ); + if ( file.exists() ) + { + if ( !file.delete() ) + { + throw new RuntimeException( String.format( "Can't delete %s", file ) ); Review comment: I think it's a rare case when I can create a file before a test and can't remove it after. But when it happens, I'd like to attract attention. I replaced `RuntimeException` with `IOException`. ########## File path: maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderFactoryTest.java ########## @@ -56,4 +59,29 @@ public void testCompleteWiring() assertEquals( " 1.5 ", conf.getChild( "target" ).getValue() ); } + public void test_pom_changes() 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 resultedExists = result.getRawModel().getProfiles().get( 1 ).getActivation().getFile().getExists(); + + assertEquals( originalExists, resultedExists ); + assertTrue( result.getEffectiveModel().getProfiles().get( 1 ).getActivation().getFile().getExists() + .contains( "src/test/resources/poms/factory/" ) ); + } + + private static Model readPom( File file ) throws Exception + { + MavenXpp3Reader reader = new MavenXpp3Reader(); + + return reader.read( new FileReader( file ) ); Review comment: Thank you for the pointing! It's good to know. I replaced with `FileInputSream`. If you have better solution, please, let me know. ########## File path: maven-model-builder/src/main/java/org/apache/maven/model/path/ProfileActivationFilePathInterpolator.java ########## @@ -0,0 +1,102 @@ +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.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; + +/** + * Interpolates path for {@link org.apache.maven.model.ActivationFile}. Review comment: I replaced it with simpler `Finds an absolute path...`. Because that's actually what it does. The goal is to "interpolate" `activationFile` => replace its `exists` or `missing` with the absulute path. ########## File path: maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java ########## @@ -444,6 +461,64 @@ else if ( !parentIds.add( parentData.getId() ) ) return result; } + 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 file, ProfileActivationContext context, + DefaultModelProblemCollector problems ) + { + String path; + boolean missing; + + if ( StringUtils.isNotEmpty( file.getExists() ) ) Review comment: @elharo I can replace it with ``` if ( activationFile.getExists() != null && !activationFile.getExists().isEmpty() ) ``` If you wish, but for me `isNotEmpty()` is more cleaner ########## File path: maven-model-builder/src/main/java/org/apache/maven/model/path/ProfileActivationFilePathInterpolator.java ########## @@ -0,0 +1,102 @@ +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.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; + +/** + * Interpolates path for {@link org.apache.maven.model.ActivationFile}. Review comment: I replaced it with simpler `Finds an absolute path...`. Because that's actually what it does. The goal is to "interpolate" `activationFile` => replace its `exists` or `missing` with the absolute path. If there is a better name, please let me know. Probably it's even worth to rename the class. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
