You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/12/13 14:34:04 UTC

[GitHub] [maven] michael-o commented on a change in pull request #391: Mng 6957: Support Build Consumer while defining modules in reverse order

michael-o commented on a change in pull request #391:
URL: https://github.com/apache/maven/pull/391#discussion_r541933983



##########
File path: maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java
##########
@@ -263,21 +264,14 @@ private DependencyResolutionResult resolveDependencies( MavenProject project, Re
 
     private List<String> getProfileIds( List<Profile> profiles )
     {
-        List<String> ids = new ArrayList<>( profiles.size() );
-
-        for ( Profile profile : profiles )
-        {
-            ids.add( profile.getId() );
-        }
-
-        return ids;
+        return profiles.stream().map( Profile::getId ).collect( Collectors.toList() );
     }
 
     private ModelBuildingRequest getModelBuildingRequest( InternalConfig config )
     {
         ProjectBuildingRequest configuration = config.request;
 
-        ModelBuildingRequest request = new DefaultModelBuildingRequest();
+        DefaultModelBuildingRequest request = new DefaultModelBuildingRequest();

Review comment:
       This change is necessary to pass the builder?

##########
File path: maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java
##########
@@ -386,44 +381,22 @@ private ModelSource createStubModelSource( Artifact artifact )
         ReactorModelPool.Builder poolBuilder = new ReactorModelPool.Builder();
         final ReactorModelPool modelPool = poolBuilder.build();
         
-        if ( Features.buildConsumer().isActive() )
-        {
-            final TransformerContext context = new TransformerContext()
-            {
-                @Override
-                public String getUserProperty( String key )
-                {
-                    return request.getUserProperties().getProperty( key );
-                }
-    
-                @Override
-                public Model getRawModel( Path p )
-                {
-                    return modelPool.get( p );
-                }
-    
-                @Override
-                public Model getRawModel( String groupId, String artifactId )
-                {
-                    return modelPool.get( groupId, artifactId, null );
-                }
-            };
-            request.getRepositorySession().getData().set( TransformerContext.KEY, context );
-        }
-
-        InternalConfig config = new InternalConfig( request, modelPool,
-                useGlobalModelCache() ? getModelCache() : new ReactorModelCache() );
+        InternalConfig config =
+            new InternalConfig( request, modelPool, useGlobalModelCache() ? getModelCache() : new ReactorModelCache(),
+                        modelBuilder.newTransformerContextBuilder() );
 
-        Map<String, MavenProject> projectIndex = new HashMap<>( 256 );
+        Map<File, MavenProject> projectIndex = new HashMap<>( 256 );
 
+        // phase 1: get file Models from the reactor.
         boolean noErrors =
             build( results, interimResults, projectIndex, pomFiles, new LinkedHashSet<>(), true, recursive,
                    config, poolBuilder );
-
+        
         ClassLoader oldContextClassLoader = Thread.currentThread().getContextClassLoader();
 
         try
         {
+            // Phase 2: get effective from the reactor

Review comment:
       get effective what?

##########
File path: maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java
##########
@@ -640,12 +605,12 @@ private boolean build( List<ProjectBuildingResult> results, List<InterimResult>
     }
 
     private boolean build( List<ProjectBuildingResult> results, List<MavenProject> projects,
-                           Map<String, MavenProject> projectIndex, List<InterimResult> interimResults,
+                           Map<File, MavenProject> projectIndex, List<InterimResult> interimResults,
                            ProjectBuildingRequest request, Map<File, Boolean> profilesXmls,
                            RepositorySystemSession session )
     {
         boolean noErrors = true;
-
+        

Review comment:
       Trailing WS

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java
##########
@@ -1707,4 +1786,131 @@ protected void mergePluginContainer_Plugins( PluginContainer target, PluginConta
             // don't merge
         }
     }
+    
+    /**
+     * Builds up the transformer context.
+     * After the buildplan is ready, the build()-method returns the immutable context useful during distribution.
+     * This is an inner class, as it must be able to call readRawModel() 
+     * 
+     * @author Robert Scholte
+     * @since 3.7.0
+     */
+    private class DefaultTransformerContextBuilder implements TransformerContextBuilder
+    {
+        private final DefaultTransformerContext context = new DefaultTransformerContext();
+
+        private final Map<DefaultTransformerContext.GAKey, Set<Source>> mappedSources
+                = new ConcurrentHashMap<>( 64 );
+
+        /**
+         * If an interface could be extracted, DefaultModelProblemCollector should be ModelProblemCollectorExt
+         * 
+         * @param request
+         * @param collector
+         * @return
+         */
+        @Override
+        public TransformerContext initialize( ModelBuildingRequest request, ModelProblemCollector collector )
+        {
+            // We must assume the TransformerContext was created using this.newTransformerContextBuilder()
+            DefaultModelProblemCollector problems = (DefaultModelProblemCollector) collector;
+            return new TransformerContext()
+            {
+                @Override
+                public String getUserProperty( String key )
+                {
+                    return context.userProperties.computeIfAbsent( key,
+                                                           k -> request.getUserProperties().getProperty( key ) );
+                }
+                
+                @Override
+                public Model getRawModel( String gId, String aId )
+                {
+                    return context.modelByGA.computeIfAbsent( new DefaultTransformerContext.GAKey( gId, aId ),
+                                                              k -> findRawModel( gId, aId ) );
+                }
+                
+                @Override
+                public Model getRawModel( Path path )
+                {
+                    return context.modelByPath.computeIfAbsent( path, k -> findRawModel( path ) );
+                }
+
+                private Model findRawModel( String groupId, String artifactId )
+                {
+                    Source source = getSource( groupId, artifactId );
+                    if ( source != null )
+                    {
+                        try
+                        {
+                            ModelBuildingRequest gaBuildingRequest = new FilterModelBuildingRequest( request ) 
+                            {
+                                @Override
+                                public ModelSource getModelSource()
+                                {
+                                    return (ModelSource) source;
+                                }
+                                
+                            };
+                            return readRawModel( gaBuildingRequest, problems );
+                        }
+                        catch ( ModelBuildingException e )
+                        {
+                            // gathered with problem collector
+                        }
+                    }
+                    return null;
+                }
+                
+                private Model findRawModel( Path p )
+                {
+                    if ( !Files.isRegularFile( p ) )
+                    {
+                        throw new IllegalArgumentException( "Not a regular file" + p );

Review comment:
       Makes it either with a colon or use `'{}'`

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/BuildModelSourceTransformer.java
##########
@@ -58,8 +58,8 @@ protected AbstractSAXFilter getSAXFilter( Path pomFile,
         throws TransformerConfigurationException, SAXException, ParserConfigurationException
     {
         BuildPomXMLFilterFactory buildPomXMLFilterFactory =
-            new DefaultBuildPomXMLFilterFactory( context, lexicalHandlerConsumer );
-
+            new DefaultBuildPomXMLFilterFactory( context, lexicalHandlerConsumer, false );
+        

Review comment:
       Trailing WS

##########
File path: maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java
##########
@@ -505,32 +483,17 @@ private boolean build( List<ProjectBuildingResult> results, List<InterimResult>
             noErrors = false;
         }
 
-        Model model = result.getEffectiveModel();
+        Model model = result.getFileModel().clone();
         
-        poolBuilder.put( model.getPomFile().toPath(),  result.getRawModel() );
+        poolBuilder.put( model.getPomFile().toPath(),  model );
         
-        try
-        {
-            // first pass: build without building parent.
-            initProject( project, projectIndex, false, result, new HashMap<>( 0 ), config.request );
-        }
-        catch ( InvalidArtifactRTException iarte )
-        {
-            result.getProblems().add( new DefaultModelProblem( null, ModelProblem.Severity.ERROR, null, model, -1, -1,
-                  iarte ) );
-        }
-
-        projectIndex.put( result.getModelIds().get( 0 ), project );
-
         InterimResult interimResult = new InterimResult( pomFile, request, result, listener, isRoot );
         interimResults.add( interimResult );
-
-        if ( recursive && !model.getModules().isEmpty() )
+        

Review comment:
       Trailing WS

##########
File path: maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java
##########
@@ -386,44 +381,22 @@ private ModelSource createStubModelSource( Artifact artifact )
         ReactorModelPool.Builder poolBuilder = new ReactorModelPool.Builder();
         final ReactorModelPool modelPool = poolBuilder.build();
         
-        if ( Features.buildConsumer().isActive() )
-        {
-            final TransformerContext context = new TransformerContext()
-            {
-                @Override
-                public String getUserProperty( String key )
-                {
-                    return request.getUserProperties().getProperty( key );
-                }
-    
-                @Override
-                public Model getRawModel( Path p )
-                {
-                    return modelPool.get( p );
-                }
-    
-                @Override
-                public Model getRawModel( String groupId, String artifactId )
-                {
-                    return modelPool.get( groupId, artifactId, null );
-                }
-            };
-            request.getRepositorySession().getData().set( TransformerContext.KEY, context );
-        }
-
-        InternalConfig config = new InternalConfig( request, modelPool,
-                useGlobalModelCache() ? getModelCache() : new ReactorModelCache() );
+        InternalConfig config =
+            new InternalConfig( request, modelPool, useGlobalModelCache() ? getModelCache() : new ReactorModelCache(),
+                        modelBuilder.newTransformerContextBuilder() );
 
-        Map<String, MavenProject> projectIndex = new HashMap<>( 256 );
+        Map<File, MavenProject> projectIndex = new HashMap<>( 256 );
 
+        // phase 1: get file Models from the reactor.
         boolean noErrors =
             build( results, interimResults, projectIndex, pomFiles, new LinkedHashSet<>(), true, recursive,
                    config, poolBuilder );
-
+        

Review comment:
       Trailing WS

##########
File path: maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java
##########
@@ -386,44 +381,22 @@ private ModelSource createStubModelSource( Artifact artifact )
         ReactorModelPool.Builder poolBuilder = new ReactorModelPool.Builder();
         final ReactorModelPool modelPool = poolBuilder.build();
         
-        if ( Features.buildConsumer().isActive() )
-        {
-            final TransformerContext context = new TransformerContext()
-            {
-                @Override
-                public String getUserProperty( String key )
-                {
-                    return request.getUserProperties().getProperty( key );
-                }
-    
-                @Override
-                public Model getRawModel( Path p )
-                {
-                    return modelPool.get( p );
-                }
-    
-                @Override
-                public Model getRawModel( String groupId, String artifactId )
-                {
-                    return modelPool.get( groupId, artifactId, null );
-                }
-            };
-            request.getRepositorySession().getData().set( TransformerContext.KEY, context );
-        }
-
-        InternalConfig config = new InternalConfig( request, modelPool,
-                useGlobalModelCache() ? getModelCache() : new ReactorModelCache() );
+        InternalConfig config =

Review comment:
       Please explain why the inner class with the feature flag is not necessary anymore.

##########
File path: maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java
##########
@@ -703,15 +671,14 @@ private boolean build( List<ProjectBuildingResult> results, List<MavenProject> p
     }
 
     @SuppressWarnings( "checkstyle:methodlength" )
-    private void initProject( MavenProject project, Map<String, MavenProject> projects,
+    private void initProject( MavenProject project, Map<File, MavenProject> projects,
                               boolean buildParentIfNotExisting, ModelBuildingResult result,
                               Map<File, Boolean> profilesXmls, ProjectBuildingRequest projectBuildingRequest )
     {
         Model model = result.getEffectiveModel();
 
         project.setModel( model );
-        project.setOriginalModel( result.getRawModel() );
-        project.setFile( model.getPomFile() );

Review comment:
       Why is this not required anymore?

##########
File path: maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java
##########
@@ -685,12 +650,15 @@ private boolean build( List<ProjectBuildingResult> results, List<MavenProject> p
             catch ( ModelBuildingException e )
             {
                 DefaultProjectBuildingResult result = null;
-                if ( project == null )
+                if ( project == null || interimResult.result.getEffectiveModel() == null )
                 {
                     result = new DefaultProjectBuildingResult( e.getModelId(), interimResult.pomFile, e.getProblems() );
                 }
                 else
                 {
+                    // building effective model includes validation, make sure effectiveModel is available 

Review comment:
       use consistent comment on effective model

##########
File path: maven-core/src/main/java/org/apache/maven/project/ReactorModelCache.java
##########
@@ -36,7 +36,7 @@
 {
 
     private final Map<Object, Object> models = new ConcurrentHashMap<>( 256 );
-
+    

Review comment:
       Trailing WS

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java
##########
@@ -1707,4 +1786,131 @@ protected void mergePluginContainer_Plugins( PluginContainer target, PluginConta
             // don't merge
         }
     }
+    
+    /**
+     * Builds up the transformer context.
+     * After the buildplan is ready, the build()-method returns the immutable context useful during distribution.
+     * This is an inner class, as it must be able to call readRawModel() 
+     * 
+     * @author Robert Scholte
+     * @since 3.7.0

Review comment:
       not valid anymore

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java
##########
@@ -649,64 +682,101 @@ private Model readModel( ModelSource modelSource, File pomFile, ModelBuildingReq
                 .setMessage( "Non-readable POM " + modelSource.getLocation() + ": " + msg ).setException( e ) );
             throw problems.newModelBuildingException();
         }
-        if ( pomFile != null )
-        {
-            model.setPomFile( pomFile );
-        }
-        else if ( modelSource instanceof FileModelSource )
+        
+        if ( modelSource instanceof FileModelSource )
         {
             model.setPomFile( ( (FileModelSource) modelSource ).getFile() );
         }
         problems.setSource( model );
 
         modelValidator.validateFileModel( model, request, problems );
-        request.setFileModel( model );
+
+        if ( hasFatalErrors( problems ) )
+        {
+            throw problems.newModelBuildingException();
+        }
+
+        intoCache( request.getModelCache(), modelSource, ModelCacheTag.FILE, model );
+        if ( modelSource instanceof FileModelSource )
+        {
+            if ( request.getTransformerContextBuilder() instanceof DefaultTransformerContextBuilder )
+            {
+                DefaultTransformerContextBuilder contextBuilder =
+                        (DefaultTransformerContextBuilder) request.getTransformerContextBuilder();
+                contextBuilder.putSource( getGroupId( model ), model.getArtifactId(), modelSource );
+            }
+        }
+
+        return model;
+    }
+
+    private Model readRawModel( ModelBuildingRequest request, DefaultModelProblemCollector problems )
+        throws ModelBuildingException
+    {
+        ModelSource modelSource = request.getModelSource();
         
-        if ( Features.buildConsumer().isActive() && pomFile != null )
+        ModelData cachedData = fromCache( request.getModelCache(), modelSource, ModelCacheTag.RAW );
+        if ( cachedData != null )
         {
+          return cachedData.getModel();    
+        }
+        
+        Model rawModel;
+        if ( Features.buildConsumer().isActive() && modelSource instanceof FileModelSource )
+        {
+            rawModel = readFileModel( request, problems );
+            File pomFile = ( (FileModelSource) modelSource ).getFile();
+            
+            TransformerContext context = null;
+            if ( request.getTransformerContextBuilder() != null )
+            {
+                context = request.getTransformerContextBuilder().initialize( request, problems );   
+            }
+            
             try
             {
-                Model rawModel =
-                    modelProcessor.read( pomFile,
-                               Collections.singletonMap( "transformerContext", request.getTransformerContext() ) );
+                // must implement TransformContext, but should use request to access system properties/modelcache
+                Model transformedFileModel = modelProcessor.read( pomFile,
+                   Collections.singletonMap( ModelReader.TRANSFORMER_CONTEXT, context ) );
 
-                model.setPomFile( pomFile );
-                
-                // model with locationTrackers, required for proper feedback during validations
-                model = request.getFileModel().clone();
+                // rawModel with locationTrackers, required for proper feedback during validations
                 
                 // Apply enriched data
-                modelMerger.merge( model, rawModel, false, null );
+                modelMerger.merge( rawModel, transformedFileModel, false, null );
             }
             catch ( IOException e )
             {
                 problems.add( new ModelProblemCollectorRequest( Severity.FATAL, Version.V37 ).setException( e ) );
             }
         }
+        else if ( request.getFileModel() == null )
+        {
+            rawModel = readFileModel( request, problems );
+        }
+        else
+        {
+            rawModel = request.getFileModel().clone();
+        }
 
-        modelValidator.validateRawModel( model, request, problems );
+        modelValidator.validateRawModel( rawModel, request, problems );
 
         if ( hasFatalErrors( problems ) )
         {
             throw problems.newModelBuildingException();
         }
 
-        if ( pomFile != null )
-        {
-            intoCache( request.getModelCache(), modelSource, ModelCacheTag.FILEMODEL, model );
-        }
-
-        String groupId = getGroupId( model );
-        String artifactId = model.getArtifactId();
-        String version = getVersion( model );
+        String groupId = getGroupId( rawModel );
+        String artifactId = rawModel.getArtifactId();
+        String version = getVersion( rawModel );
 
-        ModelData modelData = new ModelData( modelSource, model, groupId, artifactId, version );
+        ModelData modelData = new ModelData( modelSource, rawModel, groupId, artifactId, version );
         intoCache( request.getModelCache(), groupId, artifactId, version, ModelCacheTag.RAW, modelData );
-
-        return model;
+        intoCache( request.getModelCache(), modelSource, ModelCacheTag.RAW, modelData );
+ 
+        return rawModel;
     }
 
-    private Model getModelFromCache( ModelSource modelSource, ModelCache cache )
+    private Model getModelFromCache( Source modelSource, ModelCache cache )

Review comment:
       Should the arg name change too?

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java
##########
@@ -114,7 +120,7 @@
 
     @Inject
     private ModelPathTranslator modelPathTranslator;
-
+    

Review comment:
       Trailing WS

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java
##########
@@ -1707,4 +1786,131 @@ protected void mergePluginContainer_Plugins( PluginContainer target, PluginConta
             // don't merge
         }
     }
+    
+    /**
+     * Builds up the transformer context.
+     * After the buildplan is ready, the build()-method returns the immutable context useful during distribution.
+     * This is an inner class, as it must be able to call readRawModel() 
+     * 
+     * @author Robert Scholte
+     * @since 3.7.0
+     */
+    private class DefaultTransformerContextBuilder implements TransformerContextBuilder
+    {
+        private final DefaultTransformerContext context = new DefaultTransformerContext();
+
+        private final Map<DefaultTransformerContext.GAKey, Set<Source>> mappedSources
+                = new ConcurrentHashMap<>( 64 );
+
+        /**
+         * If an interface could be extracted, DefaultModelProblemCollector should be ModelProblemCollectorExt
+         * 
+         * @param request
+         * @param collector
+         * @return
+         */
+        @Override
+        public TransformerContext initialize( ModelBuildingRequest request, ModelProblemCollector collector )
+        {
+            // We must assume the TransformerContext was created using this.newTransformerContextBuilder()
+            DefaultModelProblemCollector problems = (DefaultModelProblemCollector) collector;
+            return new TransformerContext()
+            {
+                @Override
+                public String getUserProperty( String key )
+                {
+                    return context.userProperties.computeIfAbsent( key,
+                                                           k -> request.getUserProperties().getProperty( key ) );
+                }
+                
+                @Override
+                public Model getRawModel( String gId, String aId )
+                {
+                    return context.modelByGA.computeIfAbsent( new DefaultTransformerContext.GAKey( gId, aId ),
+                                                              k -> findRawModel( gId, aId ) );
+                }
+                
+                @Override
+                public Model getRawModel( Path path )
+                {
+                    return context.modelByPath.computeIfAbsent( path, k -> findRawModel( path ) );
+                }
+
+                private Model findRawModel( String groupId, String artifactId )
+                {
+                    Source source = getSource( groupId, artifactId );
+                    if ( source != null )
+                    {
+                        try
+                        {
+                            ModelBuildingRequest gaBuildingRequest = new FilterModelBuildingRequest( request ) 
+                            {
+                                @Override
+                                public ModelSource getModelSource()
+                                {
+                                    return (ModelSource) source;
+                                }
+                                
+                            };
+                            return readRawModel( gaBuildingRequest, problems );
+                        }
+                        catch ( ModelBuildingException e )
+                        {
+                            // gathered with problem collector
+                        }
+                    }
+                    return null;
+                }
+                
+                private Model findRawModel( Path p )
+                {
+                    if ( !Files.isRegularFile( p ) )
+                    {
+                        throw new IllegalArgumentException( "Not a regular file" + p );
+                    }
+                    
+                    DefaultModelBuildingRequest req = new DefaultModelBuildingRequest( request )
+                                    .setPomFile( p.toFile() )
+                                    .setModelSource( new FileModelSource( p.toFile() ) );
+                    
+                    try
+                    {
+                        return readRawModel( req, problems );
+                    }
+                    catch ( ModelBuildingException e )
+                    {
+                        // gathered with problem collector
+                    }
+                    return null;
+                }
+            };
+        }
+
+        @Override
+        public TransformerContext build()
+        {
+            return context;
+        }
+
+        public Source getSource( String groupId, String artifactId )
+        {
+            Set<Source> sources = mappedSources.get( new DefaultTransformerContext.GAKey( groupId, artifactId ) );
+            if ( sources == null )
+            {
+                return null;
+            }
+            return sources.stream().reduce( ( a, b ) ->
+            {
+                throw new IllegalStateException( "No unique Source for " + groupId + ':' + artifactId

Review comment:
       String format?

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultTransformerContext.java
##########
@@ -0,0 +1,90 @@
+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 java.nio.file.Path;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+
+import org.apache.maven.model.Model;
+
+/**
+ * 
+ * @author Robert Scholte
+ * @since 3.7.0

Review comment:
       Not valid anymore

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultBuildPomXMLFilterFactory.java
##########
@@ -32,26 +32,29 @@
 import org.xml.sax.ext.LexicalHandler;
 
 /**
- * 
+ * A BuildPomXMLFilterFactory which is context aware
+ *
  * @author Robert Scholte
  * @since 4.0.0
  */
 public class DefaultBuildPomXMLFilterFactory extends BuildPomXMLFilterFactory
 {
     private final TransformerContext context;
     
+    /**
+     *
+     * @param context a set of data to extract values from as required for the build pom
+     * @param lexicalHandlerConsumer the lexical handler consumer
+     * @param consume true is this factory used for creating the consumer pom, otherwise false

Review comment:
       is => if. Use @code for boolean literals

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java
##########
@@ -386,28 +385,27 @@ public DefaultModelBuildingRequest setModelCache( ModelCache modelCache )
     }
 
     @Override
-    public Model getFileModel()
+    public Model getRawModel()

Review comment:
       Why did you change position of these methods?

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/ModelBuildingRequest.java
##########
@@ -63,34 +63,35 @@
      * Denotes strict validation as recommended by the current Maven version.
      */
     int VALIDATION_LEVEL_STRICT = VALIDATION_LEVEL_MAVEN_3_0;
-    
+
     /**
-     * 
-     * @return the file model
+     * Gets the file model to build (with profile activatio). 
+     * If not set, model source will be used to load file model.
+     *
+     * @return The file model to build or {@code null} if not set.
      * @since 4.0.0
      */
     Model getFileModel();
-    
+
     /**
-     * 
+     * Set the file model with profile activation
+     *
      * @param fileModel
      * @return This request, never {@code null}.
      * @since 4.0.0
      */
     ModelBuildingRequest setFileModel( Model fileModel );
-
+    
     /**
-     * Gets the raw model to build. If not set, model source will be used to load raw model.
-     *
-     * @return The raw model to build or {@code null} if not set.
+     * @deprecated rawModel is never set, instead the fileModel is set 

Review comment:
       Why deprecate in a major version?

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/ModelCacheTag.java
##########
@@ -129,12 +129,16 @@ public DependencyManagement fromCache( DependencyManagement data )
 
     };
 
-    ModelCacheTag<Model> FILEMODEL = new ModelCacheTag<Model>() 
+    /**
+     * The tag used for the file model without profile activation
+     * @since 3.7.0

Review comment:
       Not valid anymore

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/FilterModelBuildingRequest.java
##########
@@ -255,30 +255,30 @@ public FilterModelBuildingRequest setModelCache( ModelCache modelCache )
 
         return this;
     }
-
+    
     @Override
-    public Model getFileModel()
+    public Model getRawModel()

Review comment:
       Same here

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/FilterModelBuildingRequest.java
##########
@@ -255,30 +255,30 @@ public FilterModelBuildingRequest setModelCache( ModelCache modelCache )
 
         return this;
     }
-
+    

Review comment:
       Trailing WS

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/io/ModelReader.java
##########
@@ -47,6 +47,12 @@
      * location tracking.
      */
     String INPUT_SOURCE = "org.apache.maven.model.io.inputSource";
+    
+    /**
+     * The key for the option to provide a transformer context, which can be used to transform the input while reading
+     * to get an advanced version of the model.
+     */
+    String TRANSFORMER_CONTEXT = "transformerContext";

Review comment:
       Interesting that those aren't `static final`

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/ModelBuildingRequest.java
##########
@@ -63,34 +63,35 @@
      * Denotes strict validation as recommended by the current Maven version.
      */
     int VALIDATION_LEVEL_STRICT = VALIDATION_LEVEL_MAVEN_3_0;
-    
+
     /**
-     * 
-     * @return the file model
+     * Gets the file model to build (with profile activatio). 
+     * If not set, model source will be used to load file model.
+     *
+     * @return The file model to build or {@code null} if not set.
      * @since 4.0.0
      */
     Model getFileModel();
-    
+
     /**
-     * 
+     * Set the file model with profile activation
+     *
      * @param fileModel
      * @return This request, never {@code null}.
      * @since 4.0.0
      */
     ModelBuildingRequest setFileModel( Model fileModel );
-
+    

Review comment:
       Trailing WS

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultTransformerContext.java
##########
@@ -0,0 +1,90 @@
+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 java.nio.file.Path;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+
+import org.apache.maven.model.Model;
+
+/**
+ * 
+ * @author Robert Scholte
+ * @since 3.7.0
+ */
+class DefaultTransformerContext implements TransformerContext
+{
+    final Map<String, String> userProperties = new HashMap<>();
+    
+    final Map<Path, Model> modelByPath = new HashMap<>();
+
+    final Map<GAKey, Model> modelByGA = new HashMap<>();
+    
+    @Override
+    public String getUserProperty( String key )
+    {
+        return userProperties.get( key );
+    }
+
+    @Override
+    public Model getRawModel( Path p )
+    {
+        return modelByPath.get( p );
+    }
+
+    @Override
+    public Model getRawModel( String groupId, String artifactId )
+    {
+        return modelByGA.get( new GAKey( groupId, artifactId ) );
+    }
+    
+    static class GAKey
+    {
+        private final String groupId;
+        private final String artifactId;
+        private final int hashCode;
+        
+        GAKey( String groupId, String artifactId )
+        {
+            this.groupId = groupId;
+            this.artifactId = artifactId;
+            this.hashCode = Objects.hash( groupId, artifactId );
+        }
+
+        @Override
+        public int hashCode()
+        {
+            return hashCode;
+        }
+
+        @Override
+        public boolean equals( Object obj )
+        {
+            if ( this == obj )
+            {
+                return true;
+            }
+            GAKey other = (GAKey) obj;

Review comment:
       What about instance of?

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/ModelBuildingRequest.java
##########
@@ -63,34 +63,35 @@
      * Denotes strict validation as recommended by the current Maven version.
      */
     int VALIDATION_LEVEL_STRICT = VALIDATION_LEVEL_MAVEN_3_0;
-    
+
     /**
-     * 
-     * @return the file model
+     * Gets the file model to build (with profile activatio). 

Review comment:
       Typo

##########
File path: maven-xml/src/test/java/org/apache/maven/xml/sax/filter/ParentXMLFilterTest.java
##########
@@ -89,6 +89,31 @@ public void testDefaultRelativePath() throws Exception
         assertEquals( expected, actual );
     }
 
+    /**
+     * An empty relative path means it must downloaded from a repository.
+     * That implies that the version cannot be solved (if missing, Maven should complain)
+     * 
+     * @throws Exception
+     */
+    @Test
+    public void testEmptyRelativePathNoVersion() throws Exception
+    {
+        String input = "<parent>"
+            + "<groupId>GROUPID</groupId>"
+            + "<artifactId>ARTIFACTID</artifactId>"
+            + "<relativePath></relativePath>"
+            + "</parent>";
+        String expected = "<parent>"
+                        + "<groupId>GROUPID</groupId>"
+                        + "<artifactId>ARTIFACTID</artifactId>"
+                        + "<relativePath/>" // SAX optimization, however "" != null ...
+                        + "</parent>";;

Review comment:
       Redundant semicolon

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/ModelBuildingResult.java
##########
@@ -41,6 +41,13 @@
      * @return The model identifiers from the lineage of models, never {@code null}.
      */
     List<String> getModelIds();
+    
+    /**
+     * 
+     * @return the file model
+     * @since 3.7.0

Review comment:
       Not valid anymore

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/TransformerContextBuilder.java
##########
@@ -0,0 +1,46 @@
+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.
+ */
+
+/**
+ * The transformerContextBuilder is responsible for initializing the TransformerContext.
+ * In case rawModels are missing, it could do new buildingRequests on the ModelBuilder.
+ * 
+ * @author Robert Scholte
+ * @since 3.7.0

Review comment:
       Not valid anymore




----------------------------------------------------------------
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:
users@infra.apache.org