[MNG-6729] StringSearchModelInterpolator introspects objects from Java API #275
Conversation
| @@ -250,7 +250,13 @@ private static void evaluateArray( Object target, InterpolateObjectAction ctx ) | |||
|
|
|||
| private boolean isQualifiedForInterpolation( Class<?> cls ) | |||
| { | |||
| return !cls.getName().startsWith( "java" ); | |||
| Package pkg = cls.getPackage(); | |||
There was a problem hiding this comment.
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) ?
There was a problem hiding this comment.
@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.
| @@ -84,6 +84,11 @@ under the License. | |||
| <artifactId>xmlunit-matchers</artifactId> | |||
| <scope>test</scope> | |||
| </dependency> | |||
| <dependency> | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
In this case there's no reason to introduce powermock, so let's just not do so.
There was a problem hiding this comment.
@rfscholte
Can you show me a concrete example?
There was a problem hiding this comment.
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 ) );
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
The current implementation of
StringSearchModelInterpolatorintrospects Java objects, e.g.java.io.File. This fix stops introspecting such objects. See the unit testtestNotInterpolateFile.We found this issue when our build process failed with this warning on the console output:
Following this checklist to help us incorporate your
contribution quickly and easily:
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
[MNG-XXX] - Fixes bug in ApproximateQuantiles,where you replace
MNG-XXXwith the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean verifyto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
The build is successful: https://builds.apache.org/job/maven-box/job/maven/job/MNG-6729/
If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
[ x] I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.