NCLSUP-377: Guard against null plugin version

[dwalluck]

We have a Map<ProjectVersionRef, Plugin> where plugin.getVersion() is null. As seen below, the plugin value has no version specified in the POM. However, the key projectVersionRef has a version specified (org.apache.maven.plugins:maven-surefire-plugin:2.12.4).

            <pluginManagement>
                <plugin>
                    <groupId>org.apache.maven.plugins</groupId>
                    <artifactId>maven-surefire-plugin</artifactId>
                    <configuration>
                        <forkMode>once</forkMode>
                        <argLine>-Djava.awt.headless=true ${surefire.memory.settings}</argLine>
                        <runOrder>alphabetical</runOrder>
                    </configuration>
                </plugin>
            </pluginMangement>

[dwalluck]

Is this all that is needed or is something else wrong? This same pattern of startsWith without any null check is used in many other places in the code, and we didn't git a null until now. Also, what is the relationship of the plugin version of null to the dependency version of "2.12.4" here?

[codecov]

Codecov Report

Merging #866 (e4e9ee9) into master (9646dfb) will increase coverage by 0.25%.
The diff coverage is 76.47%.

@@             Coverage Diff              @@
##             master     #866      +/-   ##
============================================
+ Coverage     83.57%   83.83%   +0.25%     
- Complexity     1866     1869       +3     
============================================
  Files           115      115              
  Lines          6906     6908       +2     
  Branches       1198     1198              
============================================
+ Hits           5772     5791      +19     
+ Misses          662      648      -14     
+ Partials        472      469       -3     

Impacted Files	Coverage Δ
...ommonjava/maven/ext/io/rest/DefaultTranslator.java	91.71% <50.00%> (ø)
...en/ext/core/impl/ProjectVersioningManipulator.java	75.00% <57.14%> (ø)
...main/java/org/commonjava/maven/ext/io/ModelIO.java	81.96% <66.66%> (+1.63%)	⬆️
...ava/maven/ext/core/impl/DependencyManipulator.java	86.36% <75.00%> (ø)
...ommonjava/maven/ext/core/util/PropertiesUtils.java	87.56% <87.50%> (+1.49%)	⬆️
...ava/maven/ext/core/impl/BaseGroovyManipulator.java	67.18% <100.00%> (ø)
...monjava/maven/ext/core/impl/PluginManipulator.java	77.82% <100.00%> (+5.89%)	⬆️
.../core/impl/RepoAndReportingRemovalManipulator.java	85.04% <100.00%> (ø)
...a/maven/ext/io/resolver/MavenLocationExpander.java	75.45% <0.00%> (-0.91%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9646dfb...e4e9ee9. Read the comment docs.

[rnc]

I think that while the getResolvedPlugins / getResolvedManagedPlugins should have handled this there are some corner cases being exposed now especially with the managed plugins. I don't think we have any integration tests covering this (I suspect adding a similar block to what Keycloak has in their pluginMangement) would replicate it the problem. There may also be an issue on line 395 in PluginManipulator but I haven't replicated that. Before this can be merged it needs a test - I suggest adapting an existing test to add a parent pom e.g. jboss-parent and a versionless plugin.

[rnc]

I have a potentially simpler fix within resolvePlugins ; whether they should be combined is another question.

[dwalluck]

I have a potentially simpler fix within resolvePlugins ; whether they should be combined is another question.

That would make this change unnecessary? In any case, I was going to at a testcase to this pull request.

[rnc]

Lets discuss offline.

[rnc]

Looks good, I would just like a test as per our meeting