This is an automated email from the ASF dual-hosted git repository. rfscholte pushed a commit to branch MNG-6656 in repository https://gitbox.apache.org/repos/asf/maven.git
commit 3d453bf25df69fecd645561f1d4b39732d52c5b1 Author: rfscholte <[email protected]> AuthorDate: Mon Dec 30 15:23:16 2019 +0100 [MNG-6656] Introduce custom ModelMerger to keep inputLocationTracker from fileModel in rawModel --- .../maven/model/building/DefaultModelBuilder.java | 196 +++++++++++++++++++-- .../model/building/DefaultModelBuilderFactory.java | 1 + .../model/validation/DefaultModelValidator.java | 35 ++-- .../model/building/FileToRawModelMergerTest.java | 82 +++++++++ .../validation/DefaultModelValidatorTest.java | 2 +- 5 files changed, 279 insertions(+), 37 deletions(-) diff --git a/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java b/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java index 5bfe617..0e2f10a 100644 --- a/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java +++ b/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java @@ -57,15 +57,21 @@ import org.apache.maven.artifact.versioning.VersionRange; import org.apache.maven.feature.Features; import org.apache.maven.model.Activation; import org.apache.maven.model.Build; +import org.apache.maven.model.BuildBase; +import org.apache.maven.model.CiManagement; import org.apache.maven.model.Dependency; import org.apache.maven.model.DependencyManagement; import org.apache.maven.model.InputLocation; import org.apache.maven.model.InputSource; import org.apache.maven.model.Model; +import org.apache.maven.model.ModelBase; import org.apache.maven.model.Parent; import org.apache.maven.model.Plugin; +import org.apache.maven.model.PluginContainer; import org.apache.maven.model.PluginManagement; import org.apache.maven.model.Profile; +import org.apache.maven.model.ReportPlugin; +import org.apache.maven.model.Reporting; import org.apache.maven.model.Repository; import org.apache.maven.model.building.ModelProblem.Severity; import org.apache.maven.model.building.ModelProblem.Version; @@ -75,6 +81,7 @@ import org.apache.maven.model.interpolation.ModelInterpolator; import org.apache.maven.model.io.ModelParseException; import org.apache.maven.model.management.DependencyManagementInjector; import org.apache.maven.model.management.PluginManagementInjector; +import org.apache.maven.model.merge.ModelMerger; import org.apache.maven.model.normalization.ModelNormalizer; import org.apache.maven.model.path.ModelPathTranslator; import org.apache.maven.model.path.ModelUrlNormalizer; @@ -124,7 +131,7 @@ public class DefaultModelBuilder @Inject private ModelUrlNormalizer modelUrlNormalizer; - + @Inject private SuperPomProvider superPomProvider; @@ -164,12 +171,13 @@ public class DefaultModelBuilder private Provider<BuildPomXMLFilterFactory> buildPomXMLFilterFactory; @Inject - @Nullable private ModelCacheManager modelCacheManager; @Inject @Nullable private BuildPomXMLFilterListener xmlFilterListener; + + private ModelMerger modelMerger = new FileToRawModelMerger(); public DefaultModelBuilder setModelProcessor( ModelProcessor modelProcessor ) { @@ -278,6 +286,11 @@ public class DefaultModelBuilder this.buildPomXMLFilterFactory = () -> buildPomXMLFilterFactory; return this; } + + public void setModelCacheManager( ModelCacheManager modelCacheManager ) + { + this.modelCacheManager = modelCacheManager; + } @SuppressWarnings( "checkstyle:methodlength" ) @Override @@ -451,7 +464,7 @@ public class DefaultModelBuilder result.setEffectiveModel( resultModel ); - if ( modelCacheManager != null && request.getPomFile() != null ) + if ( request.getPomFile() != null ) { modelCacheManager.put( request.getPomFile().toPath(), resultModel ); } @@ -664,27 +677,33 @@ public class DefaultModelBuilder modelValidator.validateFileModel( model, request, problems ); request.setFileModel( model ); - modelValidator.validateRawModel( model, request, problems ); - - if ( hasFatalErrors( problems ) ) - { - throw problems.newModelBuildingException(); - } - - if ( !hasModelErrors( problems ) && Features.buildConsumer().isActive() && pomFile != null ) + if ( Features.buildConsumer().isActive() && pomFile != null ) { try { - model = modelProcessor.read( transformData( pomFile.toPath() ), null ); + Model rawModel = modelProcessor.read( transformData( pomFile.toPath() ), null ); model.setPomFile( pomFile ); + + // model with locationTrackers, required for proper feedback during validations + model = request.getFileModel().clone(); + + // Apply enriched data + modelMerger.merge( model, rawModel, false, null ); } catch ( IOException | TransformerException | SAXException | ParserConfigurationException e ) { - problems.add( new ModelProblemCollectorRequest( Severity.FATAL, Version.V37 ).setException( e ) ); + problems.add( new ModelProblemCollectorRequest( Severity.WARNING, Version.V37 ).setException( e ) ); } } + modelValidator.validateRawModel( model, request, problems ); + + if ( hasFatalErrors( problems ) ) + { + throw problems.newModelBuildingException(); + } + return model; } @@ -1537,4 +1556,155 @@ public class DefaultModelBuilder } } + /** + * As long is Maven controls the BuildPomXMLFilter, the entities that need merging is known. + * All others can simply be copied from source to target to restore the locationTracker + * + * @author Robert Scholte + * @since 3.7.0 + */ + class FileToRawModelMerger extends ModelMerger + { + @Override + protected void mergeBuild_Extensions( Build target, Build source, boolean sourceDominant, + Map<Object, Object> context ) + { + // don't merge + } + + + @Override + protected void mergeBuildBase_Resources( BuildBase target, BuildBase source, boolean sourceDominant, + Map<Object, Object> context ) + { + // don't merge + } + + @Override + protected void mergeBuildBase_TestResources( BuildBase target, BuildBase source, boolean sourceDominant, + Map<Object, Object> context ) + { + // don't merge + } + + @Override + protected void mergeCiManagement_Notifiers( CiManagement target, CiManagement source, boolean sourceDominant, + Map<Object, Object> context ) + { + // don't merge + } + + @Override + protected void mergeDependencyManagement_Dependencies( DependencyManagement target, DependencyManagement source, + boolean sourceDominant, Map<Object, Object> context ) + { + Iterator<Dependency> sourceIterator = source.getDependencies().iterator(); + target.getDependencies().stream().forEach( t -> mergeDependency( t, sourceIterator.next(), sourceDominant, + context ) ); + } + + @Override + protected void mergeDependency_Exclusions( Dependency target, Dependency source, boolean sourceDominant, + Map<Object, Object> context ) + { + // don't merge + } + + @Override + protected void mergeModel_Contributors( Model target, Model source, boolean sourceDominant, + Map<Object, Object> context ) + { + // don't merge + } + + @Override + protected void mergeModel_Developers( Model target, Model source, boolean sourceDominant, + Map<Object, Object> context ) + { + // don't merge + } + + @Override + protected void mergeModel_Licenses( Model target, Model source, boolean sourceDominant, + Map<Object, Object> context ) + { + // don't merge + } + + @Override + protected void mergeModel_MailingLists( Model target, Model source, boolean sourceDominant, + Map<Object, Object> context ) + { + // don't merge + } + + @Override + protected void mergeModel_Profiles( Model target, Model source, boolean sourceDominant, + Map<Object, Object> context ) + { + Iterator<Profile> sourceIterator = source.getProfiles().iterator(); + target.getProfiles().stream().forEach( t -> mergeProfile( t, sourceIterator.next(), sourceDominant, + context ) ); + } + + @Override + protected void mergeModelBase_Dependencies( ModelBase target, ModelBase source, boolean sourceDominant, + Map<Object, Object> context ) + { + Iterator<Dependency> sourceIterator = source.getDependencies().iterator(); + target.getDependencies().stream().forEach( t -> mergeDependency( t, sourceIterator.next(), sourceDominant, + context ) ); + } + + @Override + protected void mergeModelBase_PluginRepositories( ModelBase target, ModelBase source, boolean sourceDominant, + Map<Object, Object> context ) + { + target.setPluginRepositories( source.getPluginRepositories() ); + } + + @Override + protected void mergeModelBase_Repositories( ModelBase target, ModelBase source, boolean sourceDominant, + Map<Object, Object> context ) + { + // don't merge + } + + @Override + protected void mergePlugin_Dependencies( Plugin target, Plugin source, boolean sourceDominant, + Map<Object, Object> context ) + { + Iterator<Dependency> sourceIterator = source.getDependencies().iterator(); + target.getDependencies().stream().forEach( t -> mergeDependency( t, sourceIterator.next(), sourceDominant, + context ) ); + } + + @Override + protected void mergePlugin_Executions( Plugin target, Plugin source, boolean sourceDominant, + Map<Object, Object> context ) + { + // don't merge + } + + @Override + protected void mergeReporting_Plugins( Reporting target, Reporting source, boolean sourceDominant, + Map<Object, Object> context ) + { + // don't merge + } + + @Override + protected void mergeReportPlugin_ReportSets( ReportPlugin target, ReportPlugin source, boolean sourceDominant, + Map<Object, Object> context ) + { + // don't merge + } + + @Override + protected void mergePluginContainer_Plugins( PluginContainer target, PluginContainer source, + boolean sourceDominant, Map<Object, Object> context ) + { + // don't merge + } + } } diff --git a/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilderFactory.java b/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilderFactory.java index cd499eb..82cd713 100644 --- a/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilderFactory.java +++ b/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilderFactory.java @@ -227,6 +227,7 @@ public class DefaultModelBuilderFactory modelBuilder.setReportConfigurationExpander( newReportConfigurationExpander() ); modelBuilder.setReportingConverter( newReportingConverter() ); modelBuilder.setBuildPomXMLFilterFactory( new BuildPomXMLFilterFactory() ); + modelBuilder.setModelCacheManager( new DefaultModelCacheManager() ); return modelBuilder; } diff --git a/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java b/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java index 1280a48..f789333 100644 --- a/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java +++ b/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java @@ -19,7 +19,6 @@ package org.apache.maven.model.validation; * under the License. */ -import org.apache.maven.feature.Features; import org.apache.maven.model.Activation; import org.apache.maven.model.ActivationFile; import org.apache.maven.model.Build; @@ -118,6 +117,17 @@ public class DefaultModelValidator if ( request.getValidationLevel() >= ModelBuildingRequest.VALIDATION_LEVEL_MAVEN_2_0 ) { + Set<String> modules = new HashSet<>(); + for ( int i = 0, n = m.getModules().size(); i < n; i++ ) + { + String module = m.getModules().get( i ); + if ( !modules.add( module ) ) + { + addViolation( problems, Severity.ERROR, Version.V20, "modules.module[" + i + "]", null, + "specifies duplicate child module " + module, m.getLocation( "modules" ) ); + } + } + Severity errOn30 = getSeverity( request, ModelBuildingRequest.VALIDATION_LEVEL_MAVEN_3_0 ); // [MNG-6074] Maven should produce an error if no model version has been set in a POM file used to build an @@ -227,18 +237,8 @@ public class DefaultModelValidator if ( parent != null ) { - InputLocationTracker locationTracker; - if ( Features.buildConsumer().isActive() ) - { - locationTracker = request.getFileModel().getParent(); - } - else - { - locationTracker = parent; - } - validateStringNotEmpty( "parent.version", problems, Severity.FATAL, Version.BASE, parent.getVersion(), - locationTracker ); + parent ); } } @@ -396,17 +396,6 @@ public class DefaultModelValidator if ( request.getValidationLevel() >= ModelBuildingRequest.VALIDATION_LEVEL_MAVEN_2_0 ) { - Set<String> modules = new HashSet<>(); - for ( int i = 0, n = m.getModules().size(); i < n; i++ ) - { - String module = m.getModules().get( i ); - if ( !modules.add( module ) ) - { - addViolation( problems, Severity.ERROR, Version.V20, "modules.module[" + i + "]", null, - "specifies duplicate child module " + module, m.getLocation( "modules" ) ); - } - } - Severity errOn31 = getSeverity( request, ModelBuildingRequest.VALIDATION_LEVEL_MAVEN_3_1 ); validateBannedCharacters( EMPTY, "version", problems, errOn31, Version.V20, m.getVersion(), null, m, diff --git a/maven-model-builder/src/test/java/org/apache/maven/model/building/FileToRawModelMergerTest.java b/maven-model-builder/src/test/java/org/apache/maven/model/building/FileToRawModelMergerTest.java new file mode 100644 index 0000000..485dc4c --- /dev/null +++ b/maven-model-builder/src/test/java/org/apache/maven/model/building/FileToRawModelMergerTest.java @@ -0,0 +1,82 @@ +package org.apache.maven.model.building; + +/* + * 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 static org.hamcrest.Matchers.hasItems; +import static org.junit.Assert.assertThat; + +import java.lang.reflect.Method; +import java.lang.reflect.ParameterizedType; +import java.lang.reflect.Type; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import org.apache.maven.model.building.DefaultModelBuilder.FileToRawModelMerger; +import org.apache.maven.model.merge.ModelMerger; +import org.junit.Test; + +public class FileToRawModelMergerTest +{ + + /** + * Ensures that all list-merge methods are overridden + */ + @Test + public void testOverriddenMergeMethods() + { + List<String> methodNames = + Stream.of( ModelMerger.class.getDeclaredMethods() ) + .filter( m -> m.getName().startsWith( "merge" ) ) + .filter( m -> + { + String baseName = m.getName().substring( 5 /* merge */ ); + String entity = baseName.substring( baseName.indexOf( '_' ) + 1 ); + try + { + Type returnType = m.getParameterTypes()[0].getMethod( "get" + entity ).getGenericReturnType(); + if ( returnType instanceof ParameterizedType ) + { + return !( (ParameterizedType) returnType ).getActualTypeArguments()[0].equals( String.class ); + } + else + { + return false; + } + } + catch ( ReflectiveOperationException | SecurityException e ) + { + return false; + } + } ) + .map( Method::getName ) + .collect( Collectors.toList() ); + + List<String> overriddenMethods = + Stream.of( FileToRawModelMerger.class.getDeclaredMethods() ) + .map( Method::getName ) + .filter( m -> m.startsWith( "merge" ) ) + .collect( Collectors.toList() ); + + assertThat( overriddenMethods, hasItems( methodNames.toArray( new String[0] ) ) ); + } + + +} diff --git a/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java b/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java index b90fcc6..d2a9e60 100644 --- a/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java +++ b/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java @@ -376,7 +376,7 @@ public class DefaultModelValidatorTest public void testDuplicateModule() throws Exception { - SimpleProblemCollector result = validate( "duplicate-module.xml" ); + SimpleProblemCollector result = validateRaw( "duplicate-module.xml" ); assertViolations( result, 0, 1, 0 );
