Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MNG-6729] StringSearchModelInterpolator introspects objects from Java API #275

Merged
merged 1 commit into from
Aug 9, 2019
Merged

[MNG-6729] StringSearchModelInterpolator introspects objects from Java API #275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions maven-model-builder/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ under the License.
<artifactId>xmlunit-matchers</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not use powermock to get access to Fields. I don't want to seduce people to use powermock, in general it means your code is not good testable, but that should be fixed with better code, not with powermock power.
I'm pretty sure we're already using some code for this, e.g. Reflector from Plexus Utils.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rfscholte
Why Robert? Java developers are glad to use PowerMock all around the world. As for instance, I would not be able to grow the coverage in Surefire without using PowerMock.
I am not arguing against. I am just claiming how it is in the Java world.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case there's no reason to introduce powermock, so let's just not do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rfscholte
Can you show me a concrete example?

Copy link
Contributor

Choose a reason for hiding this comment

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

Based org.apache.commons.lang3.reflect.FieldUtils; on commons-lang3

    Map<Class<?>, ?> cache =
        (Map<Class<?>, ?>) FieldUtils.readDeclaredStaticField( StringSearchModelInterpolator.class,
                                                               "CACHED_ENTRIES", true );
    assertThat( ( (Object[]) FieldUtils.readDeclaredField( fileCacheItem, "fields", true ) ).length, is( 0 ) );

Copy link
Contributor Author

@Tibor17 Tibor17 Aug 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Developers know that test code uses a specific set of libraries which are extremely specialized and some of them are Domain Specific in test area.
I have quite a lot of commercial experiences with writing Java tests but we always have used Mockito, Powermock, AssertJ, Spock, etc. But we never mix the domains.
Also we should use modern libraries for testing purposes as the same as Java 8 is juicy for a lot of contributors and me too! There's no difference with the libs, and even more important to make the Maven code attractive for new contributors. This means we wouldn't change the world with our internal coding principals; instead we should adapt to the world, the sooner the better!

Copy link
Contributor

Choose a reason for hiding this comment

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

Tibor, this is becoming a useless discussion. The only thing you wanted to do is verify a private Field, that's pure reflection. You don't need a library for that, but if you already have a library supporting that, just use that one.
Mavens reputation of downloading the internet is because Maven made it maybe too easy too add dependencies. One should be critical for every dependency, hence in this case there's no need for powermock.

Copy link
Contributor Author

@Tibor17 Tibor17 Aug 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We started with mocking MOJO and we improved the JaCoCo coverage of the class which normally can be tested only in the integration tests. So we used PowerMock and the coverage grew; wrong contributions can be easily found within a minute. So this was, I think, extremly good experience. And what is fantastic on such unit tests is the showcase of purpose of the entire class and expected behavior because good unit tests becomes documentation. So but complex MOJOs with private methods and injection would not be testable without mocking in unit tests.

<groupId>org.powermock</groupId>
<artifactId>powermock-reflect</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
56 changes: 33 additions & 23 deletions ...der/src/main/java/org/apache/maven/model/interpolation/StringSearchModelInterpolator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,13 @@ private static class CacheItem

private boolean isQualifiedForInterpolation( Class<?> cls )
{
return !cls.getName().startsWith( "java" );
Package pkg = cls.getPackage();
Copy link
Contributor

Choose a reason for hiding this comment

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

As you have the Class object.
Will it make sense to exclude every system class, the ones loaded from the system classloader (class.getClassloader().getParent() == null) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eolivelli
Hi Enrico,
System classes were excluded before this change as well in another place.
The purpose of this class is to interpolate Maven expressions and resolve them to String.

Given objects which are passed into these routines can be complex objects with delegates and inheritance as well. This algorithm is traversing all the hierarchy of given object, and the leaf of this structure is anyway supposed to be the String.

Finally, the string with the expression is overridden by interpolated string. This works fine without my change.

My change only guarantees that the internal JDK fields of e.g. java.io.File which are not Maven expressions are not introspected further. Anyway it does not make sense to continue introspecting them further.
Reading the internal fields of e.g. File is what Java 12 does not like, Java prints warnings and so we should rather avoid reading them.

There are much more troubles with this class. As for instance we are also reading static fields. There are warnings shown by IDEA saying that two instance variables can be converted to local variables, etc. I would like to post a new PR for these improvements.

if ( pkg == null )
{
return true;
}
String pkgName = pkg.getName();
return !pkgName.startsWith( "java." ) && !pkgName.startsWith( "javax." );
}

private boolean isQualifiedForInterpolation( Field field, Class<?> fieldType )
Expand Down Expand Up @@ -278,33 +284,37 @@ private boolean isQualifiedForInterpolation( Field field, Class<?> fieldType )
this.isQualifiedForInterpolation = isQualifiedForInterpolation( clazz );
this.isArray = clazz.isArray();
List<CacheField> fields = new ArrayList<>();
for ( Field currentField : clazz.getDeclaredFields() )
if ( isQualifiedForInterpolation )
{
Class<?> type = currentField.getType();
if ( isQualifiedForInterpolation( currentField, type ) )
for ( Field currentField : clazz.getDeclaredFields() )
{
if ( String.class == type )
Class<?> type = currentField.getType();
if ( isQualifiedForInterpolation( currentField, type ) )
{
if ( !Modifier.isFinal( currentField.getModifiers() ) )
if ( String.class == type )
{
fields.add( new StringField( currentField ) );
if ( !Modifier.isFinal( currentField.getModifiers() ) )
{
fields.add( new StringField( currentField ) );
}
}
else if ( List.class.isAssignableFrom( type ) )
{
fields.add( new ListField( currentField ) );
}
else if ( Collection.class.isAssignableFrom( type ) )
{
throw new RuntimeException(
"We dont interpolate into collections, use a list instead" );
}
else if ( Map.class.isAssignableFrom( type ) )
{
fields.add( new MapField( currentField ) );
}
else
{
fields.add( new ObjectField( currentField ) );
}
}
else if ( List.class.isAssignableFrom( type ) )
{
fields.add( new ListField( currentField ) );
}
else if ( Collection.class.isAssignableFrom( type ) )
{
throw new RuntimeException( "We dont interpolate into collections, use a list instead" );
}
else if ( Map.class.isAssignableFrom( type ) )
{
fields.add( new MapField( currentField ) );
}
else
{
fields.add( new ObjectField( currentField ) );
}
}
}
Expand Down
110 changes: 110 additions & 0 deletions ...src/test/java/org/apache/maven/model/interpolation/StringSearchModelInterpolatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@
import java.util.concurrent.Future;
import java.util.concurrent.FutureTask;

import static org.hamcrest.CoreMatchers.anyOf;
import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertThat;
import static org.powermock.reflect.Whitebox.getField;
import static org.powermock.reflect.Whitebox.getInternalState;

/**
* @author jdcasey
* @author Benjamin Bentmann
Expand Down Expand Up @@ -337,6 +343,100 @@ public void testInterpolateObjectWithStringToStringArrayMapField()
assertEquals( "value4", ( (String[]) obj.values.get( "key2" ) )[1] );
}

public void testInterpolateObjectWithPomFile()
throws Exception
{
Model model = new Model();
model.setPomFile( new File( System.getProperty( "user.dir" ), "pom.xml" ) );
File baseDir = model.getProjectDirectory();

Properties p = new Properties();

Map<String, String> values = new HashMap<>();
values.put( "key", "${project.basedir}" + File.separator + "target" );

ObjectWithMapField obj = new ObjectWithMapField( values );

StringSearchModelInterpolator interpolator = (StringSearchModelInterpolator) createInterpolator();

ModelBuildingRequest config = createModelBuildingRequest( p );

SimpleProblemCollector collector = new SimpleProblemCollector();
interpolator.interpolateObject( obj, model, new File( "." ), config, collector );
assertProblemFree( collector );

assertThat( baseDir.getCanonicalPath(), is( System.getProperty( "user.dir" ) ) );
assertThat( obj.values.size(), is( 1 ) );
assertThat( (String) obj.values.get( "key" ), is( anyOf(
is( System.getProperty( "user.dir" ) + File.separator + "target" ),
// TODO why MVN adds dot /./ in paths???
is( System.getProperty( "user.dir" ) + File.separator + '.' + File.separator + "target" )
) ) );
}

public void testNotInterpolateObjectWithFile()
throws Exception
{
Model model = new Model();

File baseDir = new File( System.getProperty( "user.dir" ) );

Properties p = new Properties();

ObjectWithNotInterpolatedFile obj = new ObjectWithNotInterpolatedFile( baseDir );

StringSearchModelInterpolator interpolator = (StringSearchModelInterpolator) createInterpolator();

ModelBuildingRequest config = createModelBuildingRequest( p );

SimpleProblemCollector collector = new SimpleProblemCollector();
interpolator.interpolateObject( obj, model, new File( "." ), config, collector );
assertProblemFree( collector );

//noinspection unchecked
Map<Class<?>, ?> cache =
(Map<Class<?>, ?>) getField( StringSearchModelInterpolator.class, "CACHED_ENTRIES" )
.get( null );

Object objCacheItem = cache.get( Object.class );
Object fileCacheItem = cache.get( File.class );

assertNotNull( objCacheItem );
assertNotNull( fileCacheItem );

assertThat( ( (Object[]) getInternalState( objCacheItem, "fields" ) ).length, is( 0 ) );
assertThat( ( (Object[]) getInternalState( fileCacheItem, "fields" ) ).length, is( 0 ) );
}

public void testNotInterpolateFile()
throws Exception
{
Model model = new Model();

File baseDir = new File( System.getProperty( "user.dir" ) );

Properties p = new Properties();

StringSearchModelInterpolator interpolator = (StringSearchModelInterpolator) createInterpolator();

ModelBuildingRequest config = createModelBuildingRequest( p );

SimpleProblemCollector collector = new SimpleProblemCollector();
interpolator.interpolateObject( baseDir, model, new File( "." ), config, collector );
assertProblemFree( collector );

//noinspection unchecked
Map<Class<?>, ?> cache =
(Map<Class<?>, ?>) getField( StringSearchModelInterpolator.class, "CACHED_ENTRIES" )
.get( null );

Object fileCacheItem = cache.get( File.class );

assertNotNull( fileCacheItem );

assertThat( ( (Object[]) getInternalState( fileCacheItem, "fields" ) ).length, is( 0 ) );
}


public void testConcurrentInterpolation()
throws Exception
Expand Down Expand Up @@ -432,6 +532,16 @@ public ObjectWithMapField( Map<?, ?> values )
}
}

private static final class ObjectWithNotInterpolatedFile
{
private final File f;

ObjectWithNotInterpolatedFile( File f )
{
this.f = f;
}
}

@SuppressWarnings( "unused" )
private static final class ObjectWithMixedProtection
{
Expand Down
6 changes: 6 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ under the License.
<resolverVersion>1.3.3</resolverVersion>
<slf4jVersion>1.7.25</slf4jVersion>
<xmlunitVersion>2.2.1</xmlunitVersion>
<powermockVersion>1.7.4</powermockVersion>
<maven.test.redirectTestOutputToFile>true</maven.test.redirectTestOutputToFile>
<!-- Control the name of the distribution and information output by mvn -->
<distributionId>apache-maven</distributionId>
Expand Down Expand Up @@ -418,6 +419,11 @@ under the License.
<version>${xmlunitVersion}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.powermock</groupId>
<artifactId>powermock-reflect</artifactId>
<version>${powermockVersion}</version>
</dependency>
</dependencies>
<!--bootstrap-start-comment-->
</dependencyManagement>
Expand Down