Fixing mojohaus#654: NPE in SetMojo when a dependency version is null

andrzejj0 · andrzejj0 · commit 7dad7a9a2595 · 2022-08-27T16:36:34.000+02:00
diff --git a/pom.xml b/pom.xml
@@ -81,6 +81,9 @@
       <email>antoon.johansson@gmail.com</email>
       <timezone>+1</timezone>
     </contributor>
+    <contributor>
+      <name>Andrzej Jarmoniuk</name>
+    </contributor>
   </contributors>
 
   <prerequisites>
diff --git a/src/main/java/org/codehaus/mojo/versions/SetMojo.java b/src/main/java/org/codehaus/mojo/versions/SetMojo.java
@@ -433,10 +433,12 @@ private void applyChange( MavenProject project, SortedMap<String, Model> reactor
         addChange( groupId, artifactId, oldVersion, newVersion );
         // now fake out the triggering change
 
-        final Map.Entry<String, Model> current = PomHelper.getModelEntry( reactor, groupId, artifactId );
-        current.getValue().setVersion( newVersion );
-
-        addFile( files, project, current.getKey() );
+        Map.Entry<String, Model> current = PomHelper.getModelEntry( reactor, groupId, artifactId );
+        if ( current != null )
+        {
+            current.getValue().setVersion( newVersion );
+            addFile( files, project, current.getKey() );
+        }
 
         for ( Map.Entry<String, Model> sourceEntry : reactor.entrySet() )
         {
diff --git a/src/main/java/org/codehaus/mojo/versions/api/PomHelper.java b/src/main/java/org/codehaus/mojo/versions/api/PomHelper.java
@@ -816,7 +816,7 @@ else if ( inMatchScope && matchTargetRegex.matcher( path ).matches() )
                 {
                     if ( "groupId".equals( elementName ) )
                     {
-                        haveGroupId = groupId.equals( pom.getElementText().trim() );
+                        haveGroupId =  pom.getElementText().trim().equals( groupId );
                         path = stack.pop();
                     }
                     else if ( "artifactId".equals( elementName ) )
@@ -1539,12 +1539,14 @@ public static Map<String, Model> getChildModels( Map<String, Model> reactor, Str
      */
     public static Model getModel( Map<String, Model> reactor, String groupId, String artifactId )
     {
-        Map.Entry<String, Model> entry = getModelEntry( reactor, groupId, artifactId );
-        return entry == null ? null : entry.getValue();
+        return reactor.values().stream().filter(
+                model -> ( groupId == null || groupId.equals( getGroupId( model ) ) ) && artifactId.equals(
+                        getArtifactId( model ) ) ).findAny().orElse( null );
     }
 
     /**
-     * Returns the model that has the specified groupId and artifactId or <code>null</code> if no such model exists.
+     * Returns the model that has the specified groupId (if specified)
+     * and artifactId or <code>null</code> if no such model exists.
      *
      * @param reactor    The map of models keyed by path.
      * @param groupId    The groupId to match.
@@ -1554,15 +1556,9 @@ public static Model getModel( Map<String, Model> reactor, String groupId, String
     public static Map.Entry<String, Model> getModelEntry( Map<String, Model> reactor, String groupId,
                                                           String artifactId )
     {
-        for ( Map.Entry<String, Model> entry : reactor.entrySet() )
-        {
-            Model model = entry.getValue();
-            if ( groupId.equals( getGroupId( model ) ) && artifactId.equals( getArtifactId( model ) ) )
-            {
-                return entry;
-            }
-        }
-        return null;
+        return reactor.entrySet().stream().filter(
+                e -> ( groupId == null || groupId.equals( PomHelper.getGroupId( e.getValue() ) ) ) && artifactId.equals(
+                        PomHelper.getArtifactId( e.getValue() ) ) ).findAny().orElse( null );
     }
 
     /**
@@ -1578,15 +1574,12 @@ public static int getReactorParentCount( Map<String, Model> reactor, Model model
         {
             return 0;
         }
-        else
+        Model parentModel = getModel( reactor, model.getParent().getGroupId(), model.getParent().getArtifactId() );
+        if ( parentModel == null )
         {
-            Model parentModel = getModel( reactor, model.getParent().getGroupId(), model.getParent().getArtifactId() );
-            if ( parentModel != null )
-            {
-                return getReactorParentCount( reactor, parentModel ) + 1;
-            }
             return 0;
         }
+        return getReactorParentCount( reactor, parentModel ) + 1;
     }
 
     /**
diff --git a/src/main/java/org/codehaus/mojo/versions/utils/RegexUtils.java b/src/main/java/org/codehaus/mojo/versions/utils/RegexUtils.java
@@ -97,16 +97,18 @@ public static String quote( String s )
     public static int getWildcardScore( String wildcardRule )
     {
         int score = 0;
-        for ( int i = 0; i < wildcardRule.length(); i++ )
+        if ( wildcardRule != null )
         {
-            char c = wildcardRule.charAt( i );
-            if ( c == '?' )
-            {
-                score++;
-            }
-            else if ( c == '*' )
+            for ( char c : wildcardRule.toCharArray() )
             {
-                score += 1000;
+                if ( c == '?' )
+                {
+                    score++;
+                }
+                else if ( c == '*' )
+                {
+                    score += 1000;
+                }
             }
         }
         return score;
diff --git a/src/test/java/org/codehaus/mojo/versions/SetMojoTest.java b/src/test/java/org/codehaus/mojo/versions/SetMojoTest.java
@@ -4,16 +4,14 @@
 import org.apache.maven.plugin.MojoExecutionException;
 import org.apache.maven.plugin.MojoFailureException;
 import org.apache.maven.project.MavenProject;
-import org.junit.Test;
+import org.codehaus.mojo.versions.utils.BaseMojoTestCase;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.is;
-import static org.junit.Assert.fail;
 
-public class SetMojoTest
+public class SetMojoTest extends BaseMojoTestCase
 {
-    @Test
     public void testGetIncrementedVersion() throws MojoExecutionException
     {
         new SetMojo()
@@ -28,7 +26,6 @@ public void testGetIncrementedVersion() throws MojoExecutionException
         };
     }
 
-    @Test
     public void testNextSnapshotIndexLowerBound()
     {
         new SetMojo()
@@ -48,7 +45,6 @@ public void testNextSnapshotIndexLowerBound()
         };
     }
 
-    @Test
     public void testNextSnapshotIndexUpperBound()
     {
         new SetMojo()
@@ -68,7 +64,6 @@ public void testNextSnapshotIndexUpperBound()
         };
     }
 
-    @Test
     public void testNextSnapshotIndexWithoutNextSnapshot() throws MojoFailureException
     {
         try
@@ -92,4 +87,9 @@ public void testNextSnapshotIndexWithoutNextSnapshot() throws MojoFailureExcepti
         }
     }
 
+    public void testVersionlessDependency() throws Exception
+    {
+        SetMojo myMojo = createMojo( "set", "src/test/resources/org/codehaus/mojo/set/versionless-01/pom.xml" );
+        myMojo.execute();
+    }
 }
diff --git a/src/test/resources/org/codehaus/mojo/set/versionless-01/pom.xml b/src/test/resources/org/codehaus/mojo/set/versionless-01/pom.xml
@@ -0,0 +1,38 @@
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+    <modelVersion>4.0.0</modelVersion>
+    <groupId>default-group</groupId>
+    <artifactId>default-artifact</artifactId>
+    <version>1.0</version>
+    <packaging>pom</packaging>
+
+    <dependencyManagement>
+        <dependencies>
+            <dependency>
+                <groupId>localhost</groupId>
+                <artifactId>dummy-api</artifactId>
+                <version>1.1</version>
+            </dependency>
+        </dependencies>
+    </dependencyManagement>
+
+    <dependencies>
+        <dependency>
+            <groupId>localhost</groupId>
+            <artifactId>dummy-api</artifactId>
+        </dependency>
+    </dependencies>
+
+    <build>
+        <plugins>
+            <plugin>
+                <artifactId>versions-maven-plugin</artifactId>
+                <configuration>
+                    <updateBuildOutputTimestampPolicy>onchange</updateBuildOutputTimestampPolicy>
+                    <artifactId>dummy-api</artifactId>
+                    <newVersion>2.0</newVersion>
+                </configuration>
+            </plugin>
+        </plugins>
+    </build>
+</project>

Original file line number	Diff line number	Diff line change
`@@ -816,7 +816,7 @@ else if ( inMatchScope && matchTargetRegex.matcher( path ).matches() )`
`816`	`816`	`{`
`817`	`817`	`if ( "groupId".equals( elementName ) )`
`818`	`818`	`{`
`819`		`- haveGroupId = groupId.equals( pom.getElementText().trim() );`
	`819`	`+ haveGroupId = pom.getElementText().trim().equals( groupId );`
`820`	`820`	`path = stack.pop();`
`821`	`821`	`}`
`822`	`822`	`else if ( "artifactId".equals( elementName ) )`
`@@ -1539,12 +1539,14 @@ public static Map<String, Model> getChildModels( Map<String, Model> reactor, Str`
`1539`	`1539`	`*/`
`1540`	`1540`	`public static Model getModel( Map<String, Model> reactor, String groupId, String artifactId )`
`1541`	`1541`	`{`
`1542`		`- Map.Entry<String, Model> entry = getModelEntry( reactor, groupId, artifactId );`
`1543`		`- return entry == null ? null : entry.getValue();`
	`1542`	`+ return reactor.values().stream().filter(`
	`1543`	`+ model -> ( groupId == null \|\| groupId.equals( getGroupId( model ) ) ) && artifactId.equals(`
	`1544`	`+ getArtifactId( model ) ) ).findAny().orElse( null );`
`1544`	`1545`	`}`
`1545`	`1546`
`1546`	`1547`	`/**`
`1547`		`- * Returns the model that has the specified groupId and artifactId or <code>null</code> if no such model exists.`
	`1548`	`+ * Returns the model that has the specified groupId (if specified)`
	`1549`	`+ * and artifactId or <code>null</code> if no such model exists.`
`1548`	`1550`	`*`
`1549`	`1551`	`* @param reactor The map of models keyed by path.`
`1550`	`1552`	`* @param groupId The groupId to match.`
`@@ -1554,15 +1556,9 @@ public static Model getModel( Map<String, Model> reactor, String groupId, String`
`1554`	`1556`	`public static Map.Entry<String, Model> getModelEntry( Map<String, Model> reactor, String groupId,`
`1555`	`1557`	`String artifactId )`
`1556`	`1558`	`{`
`1557`		`- for ( Map.Entry<String, Model> entry : reactor.entrySet() )`
`1558`		`- {`
`1559`		`- Model model = entry.getValue();`
`1560`		`- if ( groupId.equals( getGroupId( model ) ) && artifactId.equals( getArtifactId( model ) ) )`
`1561`		`- {`
`1562`		`- return entry;`
`1563`		`- }`
`1564`		`- }`
`1565`		`- return null;`
	`1559`	`+ return reactor.entrySet().stream().filter(`
	`1560`	`+ e -> ( groupId == null \|\| groupId.equals( PomHelper.getGroupId( e.getValue() ) ) ) && artifactId.equals(`
	`1561`	`+ PomHelper.getArtifactId( e.getValue() ) ) ).findAny().orElse( null );`
`1566`	`1562`	`}`
`1567`	`1563`
`1568`	`1564`	`/**`
`@@ -1578,15 +1574,12 @@ public static int getReactorParentCount( Map<String, Model> reactor, Model model`
`1578`	`1574`	`{`
`1579`	`1575`	`return 0;`
`1580`	`1576`	`}`
`1581`		`- else`
	`1577`	`+ Model parentModel = getModel( reactor, model.getParent().getGroupId(), model.getParent().getArtifactId() );`
	`1578`	`+ if ( parentModel == null )`
`1582`	`1579`	`{`
`1583`		`- Model parentModel = getModel( reactor, model.getParent().getGroupId(), model.getParent().getArtifactId() );`
`1584`		`- if ( parentModel != null )`
`1585`		`- {`
`1586`		`- return getReactorParentCount( reactor, parentModel ) + 1;`
`1587`		`- }`
`1588`	`1580`	`return 0;`
`1589`	`1581`	`}`
	`1582`	`+ return getReactorParentCount( reactor, parentModel ) + 1;`
`1590`	`1583`	`}`
`1591`	`1584`
`1592`	`1585`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -97,16 +97,18 @@ public static String quote( String s )`
`97`	`97`	`public static int getWildcardScore( String wildcardRule )`
`98`	`98`	`{`
`99`	`99`	`int score = 0;`
`100`		`- for ( int i = 0; i < wildcardRule.length(); i++ )`
	`100`	`+ if ( wildcardRule != null )`
`101`	`101`	`{`
`102`		`- char c = wildcardRule.charAt( i );`
`103`		`- if ( c == '?' )`
`104`		`- {`
`105`		`- score++;`
`106`		`- }`
`107`		`- else if ( c == '*' )`
	`102`	`+ for ( char c : wildcardRule.toCharArray() )`
`108`	`103`	`{`
`109`		`- score += 1000;`
	`104`	`+ if ( c == '?' )`
	`105`	`+ {`
	`106`	`+ score++;`
	`107`	`+ }`
	`108`	`+ else if ( c == '*' )`
	`109`	`+ {`
	`110`	`+ score += 1000;`
	`111`	`+ }`
`110`	`112`	`}`
`111`	`113`	`}`
`112`	`114`	`return score;`
Original file line number	Diff line number	Diff line change
`@@ -4,16 +4,14 @@`
`4`	`4`	`import org.apache.maven.plugin.MojoExecutionException;`
`5`	`5`	`import org.apache.maven.plugin.MojoFailureException;`
`6`	`6`	`import org.apache.maven.project.MavenProject;`
`7`		`-import org.junit.Test;`
	`7`	`+import org.codehaus.mojo.versions.utils.BaseMojoTestCase;`
`8`	`8`
`9`	`9`	`import static org.hamcrest.MatcherAssert.assertThat;`
`10`	`10`	`import static org.hamcrest.Matchers.containsString;`
`11`	`11`	`import static org.hamcrest.Matchers.is;`
`12`		`-import static org.junit.Assert.fail;`
`13`	`12`
`14`		`-public class SetMojoTest`
	`13`	`+public class SetMojoTest extends BaseMojoTestCase`
`15`	`14`	`{`
`16`		`- @Test`
`17`	`15`	`public void testGetIncrementedVersion() throws MojoExecutionException`
`18`	`16`	`{`
`19`	`17`	`new SetMojo()`
`@@ -28,7 +26,6 @@ public void testGetIncrementedVersion() throws MojoExecutionException`
`28`	`26`	`};`
`29`	`27`	`}`
`30`	`28`
`31`		`- @Test`
`32`	`29`	`public void testNextSnapshotIndexLowerBound()`
`33`	`30`	`{`
`34`	`31`	`new SetMojo()`
`@@ -48,7 +45,6 @@ public void testNextSnapshotIndexLowerBound()`
`48`	`45`	`};`
`49`	`46`	`}`
`50`	`47`
`51`		`- @Test`
`52`	`48`	`public void testNextSnapshotIndexUpperBound()`
`53`	`49`	`{`
`54`	`50`	`new SetMojo()`
`@@ -68,7 +64,6 @@ public void testNextSnapshotIndexUpperBound()`
`68`	`64`	`};`
`69`	`65`	`}`
`70`	`66`
`71`		`- @Test`
`72`	`67`	`public void testNextSnapshotIndexWithoutNextSnapshot() throws MojoFailureException`
`73`	`68`	`{`
`74`	`69`	`try`
`@@ -92,4 +87,9 @@ public void testNextSnapshotIndexWithoutNextSnapshot() throws MojoFailureExcepti`
`92`	`87`	`}`
`93`	`88`	`}`
`94`	`89`
	`90`	`+ public void testVersionlessDependency() throws Exception`
	`91`	`+ {`
	`92`	`+ SetMojo myMojo = createMojo( "set", "src/test/resources/org/codehaus/mojo/set/versionless-01/pom.xml" );`
	`93`	`+ myMojo.execute();`
	`94`	`+ }`
`95`	`95`	`}`