SDK-354 - Fixing bugs and adding test cases for the AddExclusions goal (

#306)

integration-tests/src/test/java/org/openmrs/maven/plugins/AddExclusionIntegrationTest.java

@@ -1,27 +1,88 @@
 package org.openmrs.maven.plugins;
 
+import org.apache.commons.io.FileUtils;
 import org.junit.Test;
 import org.openmrs.maven.plugins.model.DistroProperties;
 import org.openmrs.maven.plugins.utility.DistroHelper;
 
+import java.io.File;
+
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
 public class AddExclusionIntegrationTest extends AbstractSdkIntegrationTest {
 
+    public DistroProperties getDistroProperties() {
+        return DistroHelper.getDistroPropertiesFromFile(distroFile);
+    }
+
+    @Test
+    public void shouldAddExclusionIfNoParentDefined() throws Exception {
+        includeDistroPropertiesFile("openmrs-distro.properties");
+        assertNotNull(getDistroProperties());
+        assertNull(getDistroProperties().getParentDistroArtifact());
+        assertTrue(getDistroProperties().getExclusions().isEmpty());
+        executeExclusionTask(distroFile, "omod.uicommons");
+        assertNotNull(getDistroProperties());
+        assertTrue(getDistroProperties().getExclusions().contains("omod.uicommons"));
+        assertTrue(getLogContents().contains(AddExclusion.WARNING_NO_PARENT_DISTRO));
+    }
+
     @Test
-    public void shouldAddExclusion() throws Exception {
-        DistroProperties distroProperties = DistroHelper.getDistroPropertiesFromFile(distroFile);
-        assertNotNull(distroProperties);
-        assertFalse(distroProperties.getExclusions().contains("omod.uicommons"));
+    public void shouldAddExclusionForPropertyContainedInParent() throws Exception {
+        includeDistroPropertiesFile("openmrs-distro-parent-as-parent.properties");
+        assertNotNull(getDistroProperties());
+        assertNotNull(getDistroProperties().getParentDistroArtifact());
+        assertTrue(getDistroProperties().getExclusions().isEmpty());
+        executeExclusionTask(distroFile, "omod.uicommons");
+        assertNotNull(getDistroProperties());
+        assertTrue(getDistroProperties().getExclusions().contains("omod.uicommons"));
+        assertFalse(getLogContents().contains(AddExclusion.WARNING_NO_PARENT_DISTRO));
+        assertFalse(getLogContents().contains(AddExclusion.WARNING_PROPERTY_NOT_IN_PARENT));
+    }
 
-        addTaskParam("distro", distroFile.getAbsolutePath());
-        addTaskParam("property", "omod.uicommons");
-        executeTask("exclusion");
+    @Test
+    public void shouldAddExclusionForPropertyContainedInDistro() throws Exception {
+        includeDistroPropertiesFile("openmrs-distro-parent-as-distro.properties");
+        assertNotNull(getDistroProperties());
+        assertNotNull(getDistroProperties().getParentDistroArtifact());
+        assertTrue(getDistroProperties().getExclusions().isEmpty());
+        executeExclusionTask(distroFile, "omod.uicommons");
+        assertNotNull(getDistroProperties());
+        assertTrue(getDistroProperties().getExclusions().contains("omod.uicommons"));
+        assertFalse(getLogContents().contains(AddExclusion.WARNING_NO_PARENT_DISTRO));
+        assertFalse(getLogContents().contains(AddExclusion.WARNING_PROPERTY_NOT_IN_PARENT));
+    }
+
+    @Test
+    public void shouldAddExclusionForPropertyNotContainedInParent() throws Exception {
+        includeDistroPropertiesFile("openmrs-distro-parent-as-parent.properties");
+        assertNotNull(getDistroProperties());
+        assertNotNull(getDistroProperties().getParentDistroArtifact());
+        assertTrue(getDistroProperties().getExclusions().isEmpty());
+        executeExclusionTask(distroFile, "omod.invalidmodulename");
+        assertNotNull(getDistroProperties());
+        assertTrue(getDistroProperties().getExclusions().contains("omod.invalidmodulename"));
+        assertFalse(getLogContents().contains(AddExclusion.WARNING_NO_PARENT_DISTRO));
+        assertTrue(getLogContents().contains(AddExclusion.WARNING_PROPERTY_NOT_IN_PARENT));
+    }
+
+    private String getLogContents() throws Exception {
+        File logFile = new File(testDirectory, "log.txt");
+        assertTrue(logFile.exists());
+        return FileUtils.readFileToString(logFile, "UTF-8");
+    }
 
+    private void executeExclusionTask(File distroFile, String exclusion) throws Exception {
+        if (distroFile != null) {
+            addTaskParam("distro", distroFile.getAbsolutePath());
+        }
+        if (exclusion != null) {
+            addTaskParam("property", exclusion);
+        }
+        executeTask("exclusion");
         assertSuccess();
-        distroProperties = DistroHelper.getDistroPropertiesFromFile(distroFile);
-        assertTrue(distroProperties.getExclusions().contains("omod.uicommons"));
     }
 }

integration-tests/src/test/java/org/openmrs/maven/plugins/SetupIntegrationTest.java

@@ -351,7 +351,7 @@ public void setup_shouldInstallServerWithGivenJavaHomeAndAddJavaHomeToSdkPropert
 
     @Test
     public void setup_shouldInstallWithParentDistroSpecifiedInDistroProperties() throws Exception {
-        includeDistroPropertiesFile("openmrs-distro-parent-distro.properties");
+        includeDistroPropertiesFile("openmrs-distro-parent-as-distro.properties");
         addTaskParam("distro", testDirectory.toString() + File.separator + "openmrs-distro.properties");
         addMockDbSettings();
 
@@ -362,9 +362,9 @@ public void setup_shouldInstallWithParentDistroSpecifiedInDistroProperties() thr
 
         executeTask("setup");
 
-        assertFilePresent( serverId, "openmrs-2.0.1.war");
+        assertFilePresent( serverId, "openmrs-2.0.5.war");
         assertModulesInstalled(serverId, "htmlformentry-3.3.1.omod");
-        assertFileNotPresent(serverId, "modules", "htmlformentry-3.3.0.omod");
+        assertFileNotPresent(serverId, "modules", "htmlformentry-3.10.0.omod");
     }
 
     @Test

integration-tests/src/test/resources/integration-test/openmrs-distro-parent-as-distro.properties

@@ -0,0 +1,5 @@
+name=Spa Artifact Example
+version=2.5
+distro.referenceapplication=2.6.1
+omod.htmlformentry=3.3.1
+db.h2.supported=false

integration-tests/src/test/resources/integration-test/openmrs-distro-parent-as-parent.properties

@@ -0,0 +1,8 @@
+name=Spa Artifact Example
+version=2.5
+parent.groupId=org.openmrs.distro
+parent.artifactId=referenceapplication-package
+parent.version=2.6.1
+parent.type=jar
+omod.htmlformentry=3.3.1
+db.h2.supported=false

integration-tests/src/test/resources/integration-test/openmrs-distro-spa-artifacts.properties

@@ -1,13 +1,9 @@
 name=Spa Artifact Example
 version=1.0.0-SNAPSHOT
-
 war.openmrs=2.6.9
-
 omod.spa=2.0.0
-
 spa.artifactId=openmrs-frontend-zl
 spa.groupId=org.pih.openmrs
 spa.version=1.3.0
 spa.includes=openmrs-frontend-zl-1.3.0
-
 db.h2.supported=false

maven-plugin/src/main/java/org/openmrs/maven/plugins/AddExclusion.java

@@ -15,6 +15,9 @@
 @Mojo(name = "exclusion", requiresProject = false)
 public class AddExclusion extends AbstractTask {
 
+    public static final String WARNING_PROPERTY_NOT_IN_PARENT = "The property is not included in the parent distro";
+    public static final String WARNING_NO_PARENT_DISTRO = "This distro properties file does not contain a valid parent distro";
+
     /**
      * Path to the openmrs-distro.properties file to modify
      */
@@ -51,22 +54,23 @@ public void executeTask() throws MojoExecutionException, MojoFailureException {
             throw new MojoFailureException("Invalid distro file");
         }
 
-        Artifact distroArtifact = originalDistroProperties.getParentArtifact();
-        if (StringUtils.isNotBlank(distroArtifact.getArtifactId()) && StringUtils.isNotBlank(distroArtifact.getGroupId()) && StringUtils.isNotBlank(distroArtifact.getVersion())) {
-            DistroProperties distroProperties = distroHelper.resolveParentArtifact(distroArtifact, new File(distro).getParentFile(), originalDistroProperties, null);
+        Artifact parentDistro = originalDistroProperties.getParentDistroArtifact();
+        if (parentDistro != null) {
+            DistroProperties parentProperties = distroHelper.downloadDistroProperties(new File(distro).getParentFile(), parentDistro);
+            parentProperties = distroHelper.getDistroPropertiesForFullAncestry(parentProperties, new File(distro).getParentFile());
             if (StringUtils.isBlank(property)) {
-                List<String> currentExclusions = distroProperties.getExclusions();
-                List<String> options = distroProperties.getPropertyNames().stream().filter(prop -> !currentExclusions.contains(prop)).collect(Collectors.toList());
-                property = wizard.promptForMissingValueWithOptions("Enter the property you want to exclude",
-                        null, null, options);
-            } else {
-                if (!distroProperties.getPropertyNames().contains(property)) {
-                    wizard.showWarning("The property is not included in the parent distro");
+                List<String> currentExclusions = originalDistroProperties.getExclusions();
+                List<String> options = parentProperties.getPropertyNames().stream().filter(prop -> !currentExclusions.contains(prop)).collect(Collectors.toList());
+                property = wizard.promptForMissingValueWithOptions("Enter the property you want to exclude", null, null, options);
+            }
+            else {
+                if (!parentProperties.getPropertyNames().contains(property)) {
+                    wizard.showWarning(WARNING_PROPERTY_NOT_IN_PARENT);
                 }
             }
-
-        } else {
-            wizard.showWarning("This distro properties file does not contain a valid parent distro");
+        }
+        else {
+            wizard.showWarning(WARNING_NO_PARENT_DISTRO);
             if (StringUtils.isBlank(property)) {
                 property = wizard.promptForValueIfMissing(null, "property to exclude");
                 if (StringUtils.isBlank(property)) {

maven-plugin/src/main/java/org/openmrs/maven/plugins/BuildDistro.java

@@ -135,7 +135,7 @@ public void executeTask() throws MojoExecutionException, MojoFailureException {
 			if (distroFile.exists()) {
 				wizard.showMessage("Building distribution from the distro file at " + distroFile + "...\n");
 				distroProperties = new DistroProperties(distroFile);
-				distroArtifact = distroProperties.getParentArtifact();
+				distroArtifact = distroProperties.getParentDistroArtifact();
 				distroProperties = handleParentDistroProperties(distroArtifact, buildDirectory, distroProperties);
 			} else if (Project.hasProject(userDir)) {
 				Project config = Project.loadProject(userDir);
@@ -156,7 +156,7 @@ public void executeTask() throws MojoExecutionException, MojoFailureException {
 			}
 		} else if (StringUtils.isNotBlank(distro)) {
 			distroProperties = distroHelper.resolveDistroPropertiesForStringSpecifier(distro, versionsHelper);
-			distroArtifact = distroProperties.getParentArtifact();
+			distroArtifact = distroProperties.getParentDistroArtifact();
 			distroProperties = handleParentDistroProperties(distroArtifact, buildDirectory, distroProperties);
 		}
 
@@ -201,7 +201,7 @@ public void executeTask() throws MojoExecutionException, MojoFailureException {
 	}
 
 	private DistroProperties handleParentDistroProperties(Artifact distroArtifact, File buildDirectory, DistroProperties distroProperties) throws MojoExecutionException {
-		if (StringUtils.isNotBlank(distroArtifact.getArtifactId()) && StringUtils.isNotBlank(distroArtifact.getGroupId()) && StringUtils.isNotBlank(distroArtifact.getVersion())) {
+		if (distroArtifact != null) {
 			distroProperties = distroHelper.resolveParentArtifact(distroArtifact, buildDirectory, distroProperties, appShellVersion);
 		}
 		return distroProperties;

maven-plugin/src/main/java/org/openmrs/maven/plugins/Setup.java

@@ -213,8 +213,8 @@ private DistroProperties resolveDistroProperties(Server server) throws MojoExecu
 					break;
 
 				default:  // distro properties from current directory
-					Artifact distroArtifact = distroProperties.getParentArtifact();
-					if (StringUtils.isNotBlank(distroArtifact.getArtifactId()) && StringUtils.isNotBlank(distroArtifact.getGroupId()) && StringUtils.isNotBlank(distroArtifact.getVersion())) {
+					Artifact distroArtifact = distroProperties.getParentDistroArtifact();
+					if (distroArtifact != null) {
 						distroProperties = distroHelper.resolveParentArtifact(distroArtifact, server, distroProperties, appShellVersion);
 					} else {
 						server.setPlatformVersion(

sdk-commons/src/main/java/org/openmrs/maven/plugins/model/BaseSdkProperties.java

@@ -26,6 +26,7 @@ public abstract class BaseSdkProperties {
     public static final String VERSION = "version";
     public static final String TYPE_CONTENT = "content";
     public static final String TYPE_DISTRO = "distro";
+    public static final String TYPE_PARENT = "parent"; // This is an alternative to "distro"
     public static final String TYPE_OWA = "owa";
     public static final String TYPE_SPA = "spa";
     public static final String TYPE_CONFIG = "config";

sdk-commons/src/main/java/org/openmrs/maven/plugins/model/DistroProperties.java

@@ -109,46 +109,59 @@ private String extractPropertyName(String key) {
         }
     }
 
-    public Artifact getDistroArtifact() {
+    /**
+     * Allowed formats for artifactId and version
+     * distro.artifactId=version
+     * distro.artifactId.groupId=
+     * distro.artifactId.type=
+     * -OR-
+     * distro.anything=version
+     * distro.anything.artifactId=
+     * distro.anything.groupId=
+     * distro.anything.type=
+     * -OR-
+     * parent.artifactId=
+     * parent.groupId=
+     * parent.version=
+     * parent.type=
+     */
+    public Artifact getParentDistroArtifact() throws MojoExecutionException {
+        Artifact artifact = null;
         for (Object keyObject: getAllKeys()) {
             String key = keyObject.toString();
             String artifactType = getArtifactType(key);
-            if(artifactType.equals(TYPE_DISTRO)) {
-                return new Artifact(checkIfOverwritten(key, ARTIFACT_ID), getParam(key), checkIfOverwritten(key, GROUP_ID), checkIfOverwritten(key, TYPE), "jar");
+            if (artifactType.equals(TYPE_DISTRO)) {
+                String artifactId = checkIfOverwritten(key, ARTIFACT_ID);
+                String version = getParam(key);
+                String groupId = checkIfOverwritten(key, GROUP_ID);
+                String type = checkIfOverwritten(key, TYPE);
+                if (artifact != null) {
+                    throw new MojoExecutionException("Only a single " + TYPE_DISTRO + " property can be added to indicate the parent distribution");
+                }
+                artifact = new Artifact(artifactId, version, groupId, type);
             }
         }
-        return null;
-    }
-
-    public Artifact getParentArtifact() {
-        String parentArtifactId = getParam("parent.artifactId");
-        String parentGroupId = getParam("parent.groupId");
-        String parentVersion = getParam("parent.version");
-
-        int missingCount = 0;
-        if (StringUtils.isBlank(parentArtifactId)) {
-            missingCount++;
-        }
-        if (StringUtils.isBlank(parentGroupId)) {
-            missingCount++;
-        }
-        if (StringUtils.isBlank(parentVersion)) {
-            missingCount++;
-        }
-
-        // We are only going to throw an error if only one or two parameters are missing
-        if (missingCount > 0 && missingCount < 3) {
-            throw new IllegalArgumentException("Missing arguments for the parent");
+        String artifactId = getParam(TYPE_PARENT + "." + ARTIFACT_ID);
+        String version = getParam(TYPE_PARENT + "." + VERSION);
+        String groupId = getParam(TYPE_PARENT + "." + GROUP_ID);
+        String type = getParam(TYPE_PARENT + "." + TYPE, TYPE_ZIP);
+        if (StringUtils.isNotBlank(artifactId)) {
+            if (artifact != null) {
+                throw new MojoExecutionException("Distro properties cannot define both a " + TYPE_DISTRO + " and a " + TYPE_PARENT + " property");
+            }
+            if (StringUtils.isBlank(version) || StringUtils.isBlank(groupId)) {
+                throw new MojoExecutionException("You must specify a " + TYPE_PARENT + " groupId and version if you specify and artifactId");
+            }
+            artifact = new Artifact(artifactId, version, groupId, type);
         }
-
-        return new Artifact(parentArtifactId, parentVersion, parentGroupId, "zip");
+        return DistroHelper.normalizeArtifact(artifact, null);
     }
 
     public List<Artifact> getModuleArtifacts(DistroHelper distroHelper, File directory) throws MojoExecutionException {
         List<Artifact> childArtifacts = getModuleArtifacts();
         List<Artifact> parentArtifacts = new ArrayList<>();
 
-        Artifact artifact = getDistroArtifact();
+        Artifact artifact = getParentDistroArtifact();
         if (artifact != null) {
             DistroProperties distroProperties = distroHelper.downloadDistroProperties(directory, artifact);
             parentArtifacts.addAll(distroProperties.getModuleArtifacts(distroHelper, directory));
@@ -160,7 +173,7 @@ public List<Artifact> getOwaArtifacts(DistroHelper distroHelper, File directory)
         List<Artifact> childArtifacts = getOwaArtifacts();
         List<Artifact> parentArtifacts = new ArrayList<>();
 
-        Artifact artifact = getDistroArtifact();
+        Artifact artifact = getParentDistroArtifact();
         if (artifact != null) {
             DistroProperties distroProperties = distroHelper.downloadDistroProperties(directory, artifact);
             parentArtifacts.addAll(distroProperties.getOwaArtifacts(distroHelper, directory));
@@ -171,7 +184,7 @@ public List<Artifact> getOwaArtifacts(DistroHelper distroHelper, File directory)
     public Map<String, String> getSpaProperties(DistroHelper distroHelper, File directory) throws MojoExecutionException {
         Map<String, String> spaProperties = getSpaProperties();
 
-        Artifact artifact = getDistroArtifact();
+        Artifact artifact = getParentDistroArtifact();
         if (artifact != null) {
             DistroProperties distroProperties = distroHelper.downloadDistroProperties(directory, artifact);
             spaProperties.putAll(distroProperties.getSpaProperties(distroHelper, directory));
@@ -183,7 +196,7 @@ public List<Artifact> getWarArtifacts(DistroHelper distroHelper, File directory)
         List<Artifact> childArtifacts = getWarArtifacts();
         List<Artifact> parentArtifacts = new ArrayList<>();
 
-        Artifact artifact = getDistroArtifact();
+        Artifact artifact = getParentDistroArtifact();
         if (artifact != null) {
             DistroProperties distroProperties = distroHelper.downloadDistroProperties(directory, artifact);
             parentArtifacts.addAll(distroProperties.getWarArtifacts(distroHelper, directory));
@@ -192,7 +205,7 @@ public List<Artifact> getWarArtifacts(DistroHelper distroHelper, File directory)
     }
 
     public String getPlatformVersion(DistroHelper distroHelper, File directory) throws MojoExecutionException{
-        Artifact artifact = getDistroArtifact();
+        Artifact artifact = getParentDistroArtifact();
         if (artifact != null) {
             DistroProperties distroProperties = distroHelper.downloadDistroProperties(directory, artifact);
             return distroProperties.getPlatformVersion(distroHelper, directory);