Skip to content

Commit

Permalink
Do not use the ReactorModelCache to store the mapping between GA<->So…
Browse files Browse the repository at this point in the history 
…urce
  • Loading branch information
@gnodet
gnodet committed Nov 28, 2020
1 parent b6f0b12 commit 497e7fe
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 73 deletions.
28 changes: 0 additions & 28 deletions maven-core/src/main/java/org/apache/maven/project/ReactorModelCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,8 @@
import org.apache.maven.building.Source;
import org.apache.maven.model.building.ModelCache;

import java.util.HashSet;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;

/**
Expand All @@ -39,8 +37,6 @@ class ReactorModelCache

private final Map<Object, Object> models = new ConcurrentHashMap<>( 256 );

private final Map<GACacheKey, Set<Source>> mappedSources = new ConcurrentHashMap<>( 64 );

@Override
public Object get( String groupId, String artifactId, String version, String tag )
{
Expand All @@ -52,30 +48,6 @@ public void put( String groupId, String artifactId, String version, String tag,
{
models.put( new GavCacheKey( groupId, artifactId, version, tag ), data );
}

@Override
public Source get( String groupId, String artifactId )
{
Set<Source> sources = mappedSources.get( new GACacheKey( groupId, artifactId ) );
if ( sources == null )
{
return null;
}
else
{
return sources.stream().reduce( ( a, b ) ->
{
throw new IllegalStateException( "No unique Source for " + groupId + ':' + artifactId
+ ": " + a.getLocation() + " and " + b.getLocation() );
} ).orElse( null );
}
}

@Override
public void put( String groupId, String artifactId, Source source )
{
mappedSources.computeIfAbsent( new GACacheKey( groupId, artifactId ), k -> new HashSet<>() ).add( source );
}

@Override
public Object get( Source source, String tag )
Expand Down
27 changes: 7 additions & 20 deletions maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -696,7 +696,12 @@ private Model readFileModel( ModelBuildingRequest request,
intoCache( request.getModelCache(), modelSource, ModelCacheTag.FILE, model );
if ( modelSource instanceof FileModelSource )
{
intoCache( request.getModelCache(), getGroupId( model ), model.getArtifactId(), modelSource );
if ( request.getTransformerContextBuilder() instanceof DefaultTransformerContextBuilder )
{
DefaultTransformerContextBuilder contextBuilder =

This comment has been minimized.

Sign in to view

Copy link
@rfscholte

rfscholte Nov 28, 2020

this is something I wanted to avoid: casting to get access to certain methods. But I think I had that issue too.

This comment has been minimized.

Sign in to view

Copy link
@gnodet

gnodet Nov 29, 2020

Author Owner

The needed methods can be added to the interface if that makes more sense.
Also, I'm not sure why the TransformerContextBuilder would have to be on the ModelBuildingRequest, as it's only used once inside the DefaultModelBuilder. The code could be simplified a bit by simply creating the TranformerContext directly instead of the following lines: https://github.com/gnodet/maven/blob/MNG-6957/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java#L728-L730.
It does not seem that the TransformerContextBuilder is really meant to be used elsewhere...

This comment has been minimized.

Sign in to view

Copy link
@rfscholte

rfscholte Nov 29, 2020

https://youtu.be/KDAmlNKZJto explains it a bit. The issue is reverse ordered modules. In needs to get a specific rawModel from the transformContext, but that hasn't been built yet. In that case it must use the modelbuildingrequest to turn the fileModel (that should be there, as they are all collected in phase 1) into a raw model.
The projectBuilder is responsible for calling the modelBuilder per pom, so the projectbuilder is responsible for creating the transformcontextbuilder.

This comment has been minimized.

Sign in to view

Copy link
@gnodet

gnodet Nov 29, 2020

Author Owner

Yeah, the video hints about the problem, but not much detail.
Anyway, I think the ModelBuildingRequest could only contain the fact that we need a TranformerContext to be build, and the final resulting context (the immutable one) could be set on the ModelBuildingResult. The in-process TransformerContext could still be created the same way and used during the model transformation, there's nothing to change here. It's just that TransformerContextBuilder can be avoided completely.
Let me try a follow-up commit...

This comment has been minimized.

Sign in to view

Copy link
@gnodet

gnodet Nov 29, 2020

Author Owner

I misread, forget my previous comment. However, the DefaultTransformerContext is supposed to me immutable, so I'll push a better commit which moves the putSource / getSource to the DefaultTransformerContextBuilder which actually makes more sense.
(DefaultTransformerContextBuilder) request.getTransformerContextBuilder();
contextBuilder.context.putSource( getGroupId( model ), model.getArtifactId(), modelSource );
}
}

return model;
Expand Down Expand Up @@ -1543,14 +1548,6 @@ private <T> void intoCache( ModelCache modelCache, String groupId, String artifa
}
}

private <T> void intoCache( ModelCache modelCache, String groupId, String artifactId, Source source )
{
if ( modelCache != null )
{
modelCache.put( groupId, artifactId, source );
}
}

private <T> void intoCache( ModelCache modelCache, Source source, ModelCacheTag<T> tag, T data )
{
if ( modelCache != null )
Expand All @@ -1572,16 +1569,6 @@ private static <T> T fromCache( ModelCache modelCache, String groupId, String ar
}
return null;
}

private static Source fromCache( ModelCache modelCache, String groupId, String artifactId )
{
if ( modelCache != null )
{
return modelCache.get( groupId, artifactId );
}
return null;
}


private static <T> T fromCache( ModelCache modelCache, Source source, ModelCacheTag<T> tag )
{
Expand Down Expand Up @@ -1845,7 +1832,7 @@ public Model getRawModel( Path path )

private Model findRawModel( String groupId, String artifactId )
{
Source source = fromCache( request.getModelCache(), groupId, artifactId );
Source source = context.getSource( groupId, artifactId );
if ( source != null )
{
try
Expand Down
27 changes: 26 additions & 1 deletion ...odel-builder/src/main/java/org/apache/maven/model/building/DefaultTransformerContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,13 @@

import java.nio.file.Path;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;

import org.apache.maven.building.Source;
import org.apache.maven.model.Model;

/**
Expand All @@ -38,7 +42,9 @@ class DefaultTransformerContext implements TransformerContext
final Map<Path, Model> modelByPath = new HashMap<>();

final Map<GAKey, Model> modelByGA = new HashMap<>();


final Map<GAKey, Set<Source>> mappedSources = new ConcurrentHashMap<>( 64 );

@Override
public String getUserProperty( String key )
{
Expand All @@ -56,6 +62,25 @@ public Model getRawModel( String groupId, String artifactId )
{
return modelByGA.get( new GAKey( groupId, artifactId ) );
}

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
+ ": " + a.getLocation() + " and " + b.getLocation() );
} ).orElse( null );
}

public void putSource( String groupId, String artifactId, Source source )
{
mappedSources.computeIfAbsent( new GAKey( groupId, artifactId ), k -> new HashSet<>() ).add( source );
}

static class GAKey
{
Expand Down
24 changes: 0 additions & 24 deletions maven-model-builder/src/main/java/org/apache/maven/model/building/ModelCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,30 +59,6 @@ default Object get( Source path, String tag )
// only useful for ReactorModelCache
return null;
}

/**
*
* @param groupId The groupId, must not be {@code null}.
* @param artifactId The artifactId, must not be {@code null}.
* @param source the source matching
* @throws IllegalArgumentException if the groupId + artifactId was already added before
*/
default void put( String groupId, String artifactId, Source source )
{
// only useful for ReactorModelCache
}

/**
*
* @param groupId The groupId, must not be {@code null}.
* @param artifactId The artifactId, must not be {@code null}.
* @return The requested source or {@code null} if none was present in the cache.
*/
default Source get( String groupId, String artifactId )
{
// only useful for ReactorModelCache
return null;
}

/**
* Puts the specified data into the cache.
Expand Down

1 comment on commit 497e7fe

@rfscholte
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I see some code movements, so I'm not so worried. I agree that the cache looks again more as intended.

Please sign in to comment.