[MRELEASE-1109] Snapshot detection and support for versions like ${revision}${sha1}${changelist}

maven-release-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractCheckPomPhase.java

@@ -31,6 +31,7 @@
 import org.apache.maven.shared.release.env.ReleaseEnvironment;
 import org.apache.maven.shared.release.scm.ReleaseScmRepositoryException;
 import org.apache.maven.shared.release.scm.ScmRepositoryConfigurator;
+import org.apache.maven.shared.release.util.MavenExpression;
 import org.codehaus.plexus.util.StringUtils;
 
 import static java.util.Objects.requireNonNull;
@@ -89,7 +90,8 @@ public ReleaseResult execute(
         boolean containsSnapshotProjects = false;
 
         for (MavenProject project : reactorProjects) {
-            if (ArtifactUtils.isSnapshot(project.getVersion())) {
+            String projectVersion = MavenExpression.evaluate(project.getVersion(), project.getProperties());
+            if (ArtifactUtils.isSnapshot(projectVersion)) {
                 containsSnapshotProjects = true;
 
                 break;

maven-release-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractRewritePomsPhase.java

@@ -96,7 +96,7 @@ public abstract class AbstractRewritePomsPhase extends AbstractReleasePhase impl
      * Regular expression pattern matching Maven expressions (i.e. references to Maven properties).
      * The first group selects the property name the expression refers to.
      */
-    private static final Pattern EXPRESSION_PATTERN = Pattern.compile("\\$\\{(.+)\\}");
+    private static final Pattern EXPRESSION_PATTERN = Pattern.compile("\\$\\{(.+?)\\}");
 
     /**
      * All Maven properties allowed to be referenced in parent versions via expressions
@@ -462,7 +462,7 @@ private void rewriteVersion(
      */
     public static String extractPropertyFromExpression(String expression) {
         Matcher matcher = EXPRESSION_PATTERN.matcher(expression);
-        if (!matcher.matches()) {
+        if (!matcher.find()) {
             return null;
         }
         return matcher.group(1);
@@ -565,6 +565,7 @@ private void rewriteArtifactVersions(
                     coordinate.setVersion(mappedVersion);
                 } else {
                     String property = extractPropertyFromExpression(rawVersion);
+                    logInfo(result, "CI Friendly property " + property + " and rawVersion is " + rawVersion);
                     if (property != null) {
                         if (property.startsWith("project.")
                                 || property.startsWith("pom.")
@@ -610,6 +611,8 @@ private void rewriteArtifactVersions(
                                 }
                             } else {
                                 if (CI_FRIENDLY_PROPERTIES.contains(property)) {
+                                    // the parent's pom revision is set inside
+                                    // org.apache.maven.shared.release.transform.jdom2.JDomModel.setVersion
                                     logInfo(
                                             result,
                                             "  Ignoring artifact version update for CI friendly expression "
@@ -629,6 +632,10 @@ private void rewriteArtifactVersions(
                         }
                     } else {
                         // different/previous version not related to current release
+                        // this is the only place where the returned null from `extractPropertyFromExpression` is
+                        // supposed to be handled.
+                        // And the unit test RewritePomsForBranchPhaseTest depends on null result from
+                        // `extractPropertyFromExpression`.
                     }
                 }
             } else if (resolvedSnapshotVersion != null) {

maven-release-manager/src/main/java/org/apache/maven/shared/release/transform/jdom2/JDomModel.java

@@ -23,6 +23,7 @@
 import java.util.List;
 import java.util.Properties;
 
+import org.apache.maven.artifact.ArtifactUtils;
 import org.apache.maven.model.Build;
 import org.apache.maven.model.Dependency;
 import org.apache.maven.model.DependencyManagement;
@@ -198,7 +199,17 @@ public void setVersion(String version) {
                         AbstractRewritePomsPhase.extractPropertyFromExpression(versionElement.getTextNormalize());
                 Properties properties = getProperties();
                 if (properties != null) {
-                    properties.computeIfPresent(ciFriendlyPropertyName, (k, v) -> version);
+                    String sha1 = properties.getProperty("sha1", "");
+                    String changelist = properties.getProperty("changelist", "");
+                    properties.setProperty(
+                            ciFriendlyPropertyName,
+                            // assume that everybody follows the example and properties are simply chained
+                            version.replaceAll(sha1, "").replaceAll(changelist, ""));
+                    if (ArtifactUtils.isSnapshot(version)) {
+                        properties.setProperty("changelist", changelist);
+                    } else {
+                        properties.setProperty("changelist", "");
+                    }
                 }
             } else {
                 JDomUtils.rewriteValue(versionElement, version);

maven-release-manager/src/main/java/org/apache/maven/shared/release/transform/jdom2/JDomProperties.java

@@ -54,6 +54,9 @@ public JDomProperties(Element properties) {
     public synchronized Object setProperty(String key, String value) {
         Element property = properties.getChild(key, properties.getNamespace());
 
+        if (property == null) {
+            property = new Element(key, properties.getNamespace());
+        }
         JDomUtils.rewriteValue(property, value);
 
         // todo follow specs of Hashtable.put
@@ -114,7 +117,9 @@ public String getProperty(String key) {
 
     @Override
     public String getProperty(String key, String defaultValue) {
-        throw new UnsupportedOperationException();
+        String property = getProperty(key);
+
+        return property == null ? defaultValue : property;
     }
 
     @Override

maven-release-manager/src/main/java/org/apache/maven/shared/release/util/MavenExpression.java

@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.shared.release.util;
+
+import java.util.Map;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * @author <a href="mailto:[email protected]">Mikhail Kolesnikov</a>
+ */
+public class MavenExpression {
+    public static final Pattern EXPRESSION_PATTERN = Pattern.compile("\\$\\{(.+?)\\}");
+
+    private MavenExpression() {}
+
+    public static String evaluate(String expression, Map<?, ?> properties) {
+        StringBuilder result = new StringBuilder(expression);
+        Matcher matcher = EXPRESSION_PATTERN.matcher(result);
+        while (matcher.find()) {
+            String propertyName = matcher.group(1);
+            Object propertyValue = properties.get(propertyName);
+            result.replace(matcher.start(), matcher.end(), String.valueOf(propertyValue));
+            matcher.reset();
+        }
+        return result.toString();
+    }
+}

maven-release-manager/src/test/java/org/apache/maven/shared/release/phase/RewritePomsForDevelopmentPhaseTest.java

@@ -398,4 +398,22 @@ public void testRewritePomDependenciesWithoutDependenciesVersionUpdate() throws
 
         assertTrue(comparePomFiles(reactorProjects));
     }
+
+    @Test
+    public void testRewritePomWithCiFriendlyReactor() throws Exception {
+        List<MavenProject> reactorProjects = createReactorProjects("pom-with-parent-and-cifriendly-expressions");
+
+        ReleaseDescriptorBuilder builder =
+                createDescriptorFromProjects(reactorProjects, "pom-with-parent-and-cifriendly-expressions");
+        builder.addReleaseVersion("groupId:artifactId", RELEASE_VERSION);
+        builder.addDevelopmentVersion("groupId:artifactId", NEXT_VERSION);
+        builder.addReleaseVersion("groupId:subproject1", RELEASE_VERSION);
+        builder.addDevelopmentVersion("groupId:subproject1", NEXT_VERSION);
+
+        mapScm(builder);
+
+        phase.execute(ReleaseUtils.buildReleaseDescriptor(builder), new DefaultReleaseEnvironment(), reactorProjects);
+
+        assertTrue(comparePomFiles(reactorProjects));
+    }
 }

maven-release-manager/src/test/java/org/apache/maven/shared/release/phase/RewritePomsForReleasePhaseTest.java

@@ -334,6 +334,19 @@ public void testRewritePomWithCiFriendlyReactor() throws Exception {
         assertTrue(comparePomFiles(reactorProjects));
     }
 
+    @Test
+    public void testRewritePomWithCiFriendlyReactorWithOnlyRevision() throws Exception {
+        List<MavenProject> reactorProjects = createReactorProjects("pom-with-parent-and-cifriendly-revision");
+
+        ReleaseDescriptorBuilder builder =
+                createDescriptorFromProjects(reactorProjects, "pom-with-parent-and-cifriendly-revision");
+        builder.addReleaseVersion("groupId:artifactId", NEXT_VERSION);
+        builder.addReleaseVersion("groupId:subproject1", NEXT_VERSION);
+        phase.execute(ReleaseUtils.buildReleaseDescriptor(builder), new DefaultReleaseEnvironment(), reactorProjects);
+
+        assertTrue(comparePomFiles(reactorProjects));
+    }
+
     // MRELEASE-311
     @Test
     public void testRewritePomWithDependencyPropertyCoordinate() throws Exception {

maven-release-manager/src/test/java/org/apache/maven/shared/release/transform/jdom2/JDomModelTest.java

@@ -69,7 +69,7 @@ public void testSetVersion() throws Exception {
         assertNull(model.getVersion());
 
         // inherit from parent via CI friendly
-        content = "<project><parent><version>${revision}</version></parent></project>";
+        content = "<project><parent><version>${revision}${changelist}</version></parent></project>";
         projectElm = builder.build(new StringReader(content)).getRootElement();
         model = new JDomModel(projectElm);
         assertNull(model.getVersion());

maven-release-manager/src/test/java/org/apache/maven/shared/release/transform/jdom2/JDomPropertiesTest.java

@@ -101,9 +101,12 @@ public void testGetProperty() throws Exception {
         assertEquals("VALUE", new JDomProperties(propertiesElm).getProperty("KEY"));
     }
 
-    @Test(expected = UnsupportedOperationException.class)
-    public void testGetPropertyDefault() {
-        new JDomProperties(null).getProperty(null, null);
+    @Test
+    public void testGetPropertyDefault() throws Exception {
+        String content = "<properties></properties>";
+        Element propertiesElm = builder.build(new StringReader(content)).getRootElement();
+        assertNull(new JDomProperties(propertiesElm).getProperty("KEY", null));
+        assertEquals("", new JDomProperties(propertiesElm).getProperty("KEY", ""));
     }
 
     @Test(expected = UnsupportedOperationException.class)

maven-release-manager/src/test/java/org/apache/maven/shared/release/util/MavenExpressionTest.java

@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.shared.release.util;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Properties;
+
+import junit.framework.TestCase;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+/**
+ * @author <a href="mailto:[email protected]">Mikhail Kolesnikov</a>
+ */
+@RunWith(Parameterized.class)
+public class MavenExpressionTest extends TestCase {
+
+    private final String expected;
+    private final String expression;
+    private final Properties properties = new Properties();
+
+    public MavenExpressionTest(String expected, String expression) {
+        this.expected = expected;
+        this.expression = expression;
+        properties.setProperty("revision", "12");
+        properties.setProperty("sha1", "34");
+        properties.setProperty("changelist", "56");
+    }
+
+    @Parameters(name = "expected result {0} for expression {1}")
+    public static Collection<Object[]> parameters() {
+        return Arrays.asList(
+                new Object[] {"123456", "${revision}${sha1}${changelist}"},
+                new Object[] {"12-34-56", "${revision}-${sha1}-${changelist}"},
+                new Object[] {"12-null-56", "${revision}-${unknown}-${changelist}"});
+    }
+
+    @Test
+    public void testEvaluate() {
+        assertEquals(expected, MavenExpression.evaluate(expression, properties));
+    }
+}

maven-release-manager/src/test/resources/projects/rewrite-for-development/pom-with-parent-and-cifriendly-expressions/expected-pom.xml

@@ -0,0 +1,41 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<!--
+  ~ Copyright 2005-2006 The Apache Software Foundation.
+  ~
+  ~ Licensed under the Apache License, Version 2.0 (the "License");
+  ~ you may not use this file except in compliance with the License.
+  ~ You may obtain a copy of the License at
+  ~
+  ~      http://www.apache.org/licenses/LICENSE-2.0
+  ~
+  ~ Unless required by applicable law or agreed to in writing, software
+  ~ distributed under the License is distributed on an "AS IS" BASIS,
+  ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  ~ See the License for the specific language governing permissions and
+  ~ limitations under the License.
+  -->
+
+<project>
+  <modelVersion>4.0.0</modelVersion>
+  <groupId>groupId</groupId>
+  <artifactId>artifactId</artifactId>
+  <version>${revision}${sha1}${changelist}</version>
+  <packaging>pom</packaging>
+
+  <scm>
+    <connection>scm:svn:file://localhost/tmp/scm-repo/trunk</connection>
+    <developerConnection>scm:svn:file://localhost/tmp/scm-repo/trunk</developerConnection>
+    <url>file://localhost/tmp/scm-repo/trunk</url>
+  </scm>
+
+  <properties>
+    <revision>1.1</revision>
+    <sha1>.123</sha1>
+    <changelist>-SNAPSHOT</changelist>
+  </properties>
+
+  <modules>
+    <module>subproject1</module>
+  </modules>
+</project>

maven-release-manager/src/test/resources/projects/rewrite-for-development/pom-with-parent-and-cifriendly-expressions/pom.xml

@@ -0,0 +1,41 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<!--
+  ~ Copyright 2005-2006 The Apache Software Foundation.
+  ~
+  ~ Licensed under the Apache License, Version 2.0 (the "License");
+  ~ you may not use this file except in compliance with the License.
+  ~ You may obtain a copy of the License at
+  ~
+  ~      http://www.apache.org/licenses/LICENSE-2.0
+  ~
+  ~ Unless required by applicable law or agreed to in writing, software
+  ~ distributed under the License is distributed on an "AS IS" BASIS,
+  ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  ~ See the License for the specific language governing permissions and
+  ~ limitations under the License.
+  -->
+
+<project>
+  <modelVersion>4.0.0</modelVersion>
+  <groupId>groupId</groupId>
+  <artifactId>artifactId</artifactId>
+  <version>${revision}${sha1}${changelist}</version>
+  <packaging>pom</packaging>
+
+  <scm>
+    <connection>scm:svn:file://localhost/tmp/scm-repo/tags/release-label</connection>
+    <developerConnection>scm:svn:file://localhost/tmp/scm-repo/tags/release-label</developerConnection>
+    <url>file://localhost/tmp/scm-repo/tags/release-label</url>
+  </scm>
+
+  <properties>
+    <revision>1.0</revision>
+    <sha1>.123</sha1>
+    <changelist>-SNAPSHOT</changelist>
+  </properties>
+
+  <modules>
+    <module>subproject1</module>
+  </modules>
+</project>

maven-release-manager/src/test/resources/projects/rewrite-for-development/pom-with-parent-and-cifriendly-expressions/subproject1/expected-pom.xml

@@ -0,0 +1,28 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<!--
+  ~ Copyright 2005-2006 The Apache Software Foundation.
+  ~
+  ~ Licensed under the Apache License, Version 2.0 (the "License");
+  ~ you may not use this file except in compliance with the License.
+  ~ You may obtain a copy of the License at
+  ~
+  ~      http://www.apache.org/licenses/LICENSE-2.0
+  ~
+  ~ Unless required by applicable law or agreed to in writing, software
+  ~ distributed under the License is distributed on an "AS IS" BASIS,
+  ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  ~ See the License for the specific language governing permissions and
+  ~ limitations under the License.
+  -->
+
+<project>
+  <modelVersion>4.0.0</modelVersion>
+  <parent>
+    <groupId>groupId</groupId>
+    <artifactId>artifactId</artifactId>
+    <version>${revision}${sha1}${changelist}</version>
+  </parent>
+
+  <artifactId>subproject1</artifactId>
+</project>