Merge pull request OpenLiberty#1779 from cherylking/fixPropertyLoading

Fixes for duplicate jvm options

liberty-maven-plugin/src/it/dev-it/resources/basic-dev-project/pom.xml

@@ -13,6 +13,7 @@
     <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
     <maven.compiler.source>1.8</maven.compiler.source>
     <maven.compiler.target>1.8</maven.compiler.target>
+    <liberty.jvm.minheap>-Xms512m</liberty.jvm.minheap>
     <app.name>LibertyProject</app.name>
     <!-- tag::ports[] -->
     <testServerHttpPort>9080</testServerHttpPort>
@@ -212,6 +213,9 @@
             <default.https.port>${testServerHttpsPort}</default.https.port>
             <com.ibm.ws.logging.message.format>json</com.ibm.ws.logging.message.format>
           </bootstrapProperties>
+          <jvmOptions>
+            <param>-Xms512m</param>
+         </jvmOptions>
           <!-- ADDITIONAL_CONFIGURATION -->
         </configuration>
       </plugin>

liberty-maven-plugin/src/it/dev-it/src/test/java/net/wasdev/wlp/test/dev/it/DevCopyTestDependenciesTest.java

@@ -42,7 +42,7 @@ public static void setUpBeforeClass() throws Exception {
 
       // add new parameter in first argument to skip install features on restart
       // in this case, it should not skip install feature because Liberty was not previously installed
-      startProcess("-DskipInstallFeature=true", true);
+      startProcess("-DskipInstallFeature=true -Dliberty.jvm.minHeapSize=-Xms512m", true);
    }
 
    @AfterClass
@@ -57,10 +57,32 @@ public void copyDependenciesTest() throws Exception {
 
       File f = new File(targetDir, "liberty/wlp/usr/servers/defaultServer/lib/global/postgresql-42.1.1.jar");
       assertFalse(f.exists());
+   }
 
+   @Test
+   public void skipInstallFeatureTest() throws Exception {
       assertTrue("The install-feature goal did not run: "+getLogTail(), verifyLogMessageExists("Running liberty:install-feature", 2000));
       assertFalse("The skip install-feature log message was found unexpectedly: "+getLogTail(), verifyLogMessageExists("Skipping liberty:install-feature", 2000));
-
    }
 
+   @Test
+   public void duplicatJvmOptionsTest() throws Exception {
+        File jvmOptionsFile = new File(tempProj,"/target/liberty/wlp/usr/servers/defaultServer/jvm.options");
+        assertTrue("The jvm.options file not found at path: "+jvmOptionsFile.getAbsolutePath(),jvmOptionsFile.exists());
+
+        String fileContents = org.codehaus.plexus.util.FileUtils.fileRead(jvmOptionsFile.getCanonicalPath()).replaceAll("\r","");
+
+        String[] fileContentsArray = fileContents.split("\\n");
+        assertTrue("fileContents", fileContentsArray.length == 2);
+
+        for (int i=0; i < fileContentsArray.length; i++) {
+            String nextLine = fileContentsArray[i];
+            // verify that a single -Xms512m is the only entry in the jvm.options file.
+            if (i == 0) {
+                assertTrue("comment not found on first line", nextLine.equals("# Generated by liberty-maven-plugin"));
+            } else if (i == 1) {
+                assertTrue("-Xms512m not found on last line", nextLine.equals("-Xms512m"));
+            }
+        }
+    }    
 }

liberty-maven-plugin/src/it/server-param-pom-override-it/invoker.properties

@@ -1,2 +1,2 @@
 # prove that Liberty configuration provided by maven properties can be overridden on cmd line
-invoker.goals.1 = clean install -Dliberty.var.someVariable2=myCmdLineValue -Dliberty.var.new.CmdLine.Var=newCmdLineValue
+invoker.goals.1 = clean install -Dliberty.var.someVariable2=myCmdLineValue -Dliberty.var.new.CmdLine.Var=newCmdLineValue -Dliberty.jvm.minHeap=-Xms512m -Dliberty.jvm.myprop1="-Dmy.jvm.prop=abc" "-Dliberty.jvm.debug=-agentlib:jdwp=transport=dt_socket;server=y;suspend=n;address=7777"

liberty-maven-plugin/src/it/server-param-pom-override-it/pom.xml

@@ -17,9 +17,11 @@
         <myArgLine>-javaagent:/path/to/some/jar.jar</myArgLine>
         <liberty.jvm.argLine>@{myArgLine}</liberty.jvm.argLine>
         <liberty.jvm.undefined>@{undefined}</liberty.jvm.undefined>
-        <liberty.jvm.minHeap>-Xms512m</liberty.jvm.minHeap>
+        <liberty.jvm.debug>-agentlib:jdwp=transport=dt_socket;server=y;suspend=n;address=7777</liberty.jvm.debug>
+        <liberty.jvm.myprop2>-Dmy.jvm.prop=abc</liberty.jvm.myprop2>
+        <liberty.jvm.minHeap>-Xms1024m</liberty.jvm.minHeap>
         <!-- This maxHeap property should NOT override the one specified below in jvmOptions -->
-        <liberty.jvm.maxHeap>-Xmx1024m</liberty.jvm.maxHeap>
+        <liberty.jvm.maxHeap>-Xmx2056m</liberty.jvm.maxHeap>
         <liberty.bootstrap.location>pom.xml</liberty.bootstrap.location>
         <liberty.env.JAVA_HOME>/opt/ibm/java</liberty.env.JAVA_HOME>
         <liberty.var.someVariable1>someValue1</liberty.var.someVariable1>
@@ -69,7 +71,9 @@
                         <type>zip</type>
                     </assemblyArtifact>
                     <jvmOptions>
-                        <param>-Xmx768m</param>
+                        <param>-Xmx1024m</param>
+                        <param>-agentlib:jdwp=transport=dt_socket;server=y;suspend=n;address=7777</param>
+                        <param>-Dmy.jvm.prop2=This is the value</param>
                     </jvmOptions>
                     <bootstrapProperties>
                         <someBootstrapVar>@{myBootstrapArg}</someBootstrapVar>
@@ -104,6 +108,12 @@
                 <plugin>
                     <groupId>io.openliberty.tools</groupId>
                     <artifactId>liberty-maven-plugin</artifactId>
+                    <configuration>
+                        <!-- This would be more logical to be specified in a parent pom.xml, but putting here for simplicity for testing. -->
+                        <jvmOptions>
+                            <param>-Dmy.jvm.prop2=This is the value</param>
+                        </jvmOptions>
+                    </configuration>
                     <executions>
                         <execution>
                             <id>stop-server-before-clean</id>

liberty-maven-plugin/src/it/server-param-pom-override-it/src/main/liberty/config/jvm.options

@@ -1 +1,2 @@
--Xmx1024m
+-Xmx2056m
+-Xms512m

liberty-maven-plugin/src/it/server-param-pom-override-it/src/test/java/net/wasdev/wlp/maven/test/app/PluginConfigXmlIT.java

@@ -154,37 +154,96 @@ public void testJvmOptionsFileElements() throws Exception {
             String fileContents = FileUtils.fileRead(TARGET_JVM_OPTIONS).replaceAll("\r","");
 
             String[] fileContentsArray = fileContents.split("\\n");
-            assertTrue("fileContents", fileContentsArray.length == 6);
+            assertTrue("fileContentsArray.length="+fileContentsArray.length+" fileContents: "+fileContents, fileContentsArray.length == 9);
 
             boolean myArgLineValueFound = false;
+            boolean myUndefinedVarFound = false;
             boolean myXms512mFound = false;
+            boolean myXms1024mFound = false;    // should NOT be present
             boolean myXmx1024mFound = false;
-            boolean myUndefinedVarFound = false;
+            boolean myXmx2056mFound = false;    // should appear before -Xmx1024m
+            boolean myJvmPropFoundAbc = false;  // should only be found once (had dup values specified)
+            boolean myJvmProp2 = false;         // should only be found once (had dup values specified)
+            boolean myDebugPropFound = false;   // should only be found once, but was specified 3 different ways
+
+            // Verify file contents appear in this overall order (but order within a section is not guaranteed). 
+            // There should be NO duplicates.
+
+            // Maven project properties:
+            // -javaagent:/path/to/some/jar.jar
+            // @{undefined}
+            // -Xmx2056m
+
+            // System properties:
+            // -Xms512m (overrode project property with same name)
+            // -Dmy.jvm.prop=abc (overrode project property that had dup value)
+
+            // jvmOptions
+            // -Xmx1024m (from jvmOptions parameter)
+            // -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=7777. (from jvmOptions parameter that had dup value of both a System prop and Maven project prop)
+            // -Dmy.jvm.prop2=This is the value (from jvmOptions parameter specified in both parent and project pom.xml - should only appear once)
 
             for (int i=0; i < fileContentsArray.length; i++) {
                 String nextLine = fileContentsArray[i];
-                // verify that -Xmx768m is last in the jvm.options file, and that -Xms512m and -Xmx1024m appear before it.
+                // verify that -Xmx1024m comes after -Xmx2056m and -Xms512m in the jvm.options file, and that -Xms1024m is not included.
                 if (i == 0) {
                     assertTrue("comment not found on first line", nextLine.equals("# Generated by liberty-maven-plugin"));
-                } else if (i == 5) {
-                    assertTrue("-Xmx768m not found on last line", nextLine.equals("-Xmx768m"));
-                } else {
+                } else if (i >= 6) {  // verify jvmOptions present
+                    assertTrue("Expected jvmOptions not found: "+nextLine,
+                        nextLine.equals("-Xmx1024m") || 
+                        nextLine.equals("-agentlib:jdwp=transport=dt_socket;server=y;suspend=n;address=7777") ||
+                        nextLine.equals("-Dmy.jvm.prop2=This is the value"));
+
+                    if (nextLine.equals("-Xmx1024m")) {
+                        assertFalse("jvm option found more than once: "+nextLine,myXmx1024mFound);
+                        myXmx1024mFound = true;
+                    } else if (nextLine.equals("-agentlib:jdwp=transport=dt_socket;server=y;suspend=n;address=7777")) {
+                        assertFalse("jvm option found more than once: "+nextLine,myDebugPropFound);
+                        myDebugPropFound = true;
+                    } else {
+                        assertFalse("jvm option found more than once: "+nextLine,myJvmProp2);
+                        myJvmProp2 = true;
+                    }
+                } else if (i >= 4) {  // verify System properties present
+                        assertTrue("Expected System properties not found: "+nextLine,
+                        nextLine.equals("-Xms512m") || 
+                        nextLine.equals("-Dmy.jvm.prop=abc"));
+
                     if (nextLine.equals("-Xms512m")) {
+                        assertFalse("jvm option found more than once: "+nextLine,myXms512mFound);
                         myXms512mFound = true;
-                    } else if (nextLine.equals("-Xmx1024m")) {
-                        myXmx1024mFound = true;
-                    } else if (nextLine.equals("-javaagent:/path/to/some/jar.jar")) {
+                    } else {
+                        assertFalse("jvm option found more than once: "+nextLine,myJvmPropFoundAbc);
+                        myJvmPropFoundAbc = true;
+                    }
+                } else {  // verify Maven project properties present
+                    assertTrue("Expected jvmOptions not found: "+nextLine,
+                        nextLine.equals("-javaagent:/path/to/some/jar.jar") || 
+                        nextLine.equals("@{undefined}") ||
+                        nextLine.equals("-Xmx2056m"));
+
+                    if (nextLine.equals("-javaagent:/path/to/some/jar.jar")) {
+                        assertFalse("jvm option found more than once: "+nextLine,myArgLineValueFound);
                         myArgLineValueFound = true;
                     } else if (nextLine.equals("@{undefined}")) {
+                        assertFalse("jvm option found more than once: "+nextLine,myUndefinedVarFound);
                         myUndefinedVarFound = true;
+                    } else {
+                        assertFalse("jvm option found more than once: "+nextLine,myXmx2056mFound);
+                        myXmx2056mFound = true;
                     }
                 }
             }
+            assertTrue("-Xmx1024m not found", myXmx1024mFound);
+            assertTrue("-Dmy.jvm.prop2=This is the value - not found", myJvmProp2);
+            assertTrue("-agentlib:jdwp=transport=dt_socket;server=y;suspend=n;address=7777 not found", myDebugPropFound);
 
             assertTrue("-Xms512m not found", myXms512mFound);
-            assertTrue("-Xmx1024m not found", myXmx1024mFound);
+            assertTrue("-Dmy.jvm.prop=abc", myJvmPropFoundAbc);
+
             assertTrue("-javaagent:/path/to/some/jar.jar not found", myArgLineValueFound);
             assertTrue("@{undefined} not found", myUndefinedVarFound);
+            assertTrue("-Xmx2056m not found", myXmx2056mFound);
         }
     }
 

liberty-maven-plugin/src/main/java/io/openliberty/tools/maven/BasicSupport.java

@@ -718,14 +718,16 @@ protected Map<String,File> getLibertyDirectoryPropertyFiles() {
 
     protected void setContainerEngine(AbstractContainerSupportUtil util) throws PluginExecutionException {
         String LIBERTY_DEV_PODMAN = "liberty.dev.podman";
-        Properties mergedProperties = project.getProperties();
-        mergedProperties.putAll(System.getProperties()); //System/command line properties will take precedence
-        if (!mergedProperties.isEmpty() && mergedProperties.containsKey(LIBERTY_DEV_PODMAN)) {
-            Object isPodman = mergedProperties.get(LIBERTY_DEV_PODMAN);
-            if (isPodman != null) {
-                util.setIsDocker(!(Boolean.parseBoolean(isPodman.toString())));
-                getLog().debug("liberty.dev.podman was set to: " + (Boolean.parseBoolean(isPodman.toString())));
-            }
+        Object podmanPropValue = null;
+        if (System.getProperties().containsKey(LIBERTY_DEV_PODMAN)) {
+            podmanPropValue = System.getProperties().get(LIBERTY_DEV_PODMAN);
+        } else if (project.getProperties().containsKey(LIBERTY_DEV_PODMAN)) {
+            podmanPropValue = project.getProperties().get(LIBERTY_DEV_PODMAN);
+        }
+
+        if (podmanPropValue != null) {
+            util.setIsDocker(!(Boolean.parseBoolean(podmanPropValue.toString())));
+            getLog().debug("liberty.dev.podman was set to: " + (Boolean.parseBoolean(podmanPropValue.toString())));
         }
     }
 }

liberty-maven-plugin/src/main/java/io/openliberty/tools/maven/server/PluginConfigSupport.java

@@ -137,7 +137,8 @@ protected File exportParametersToXml() throws IOException, ParserConfigurationEx
             if (jvmOptionsResolved == null) {
                 jvmOptionsResolved = handleLatePropertyResolution(jvmOptions);
             }
-            configDocument.createElement("jvmOptions", jvmOptionsResolved);
+            List<String> uniqueOptions = getUniqueValues(jvmOptionsResolved);
+            configDocument.createElement("jvmOptions", uniqueOptions);
         } else {
             configFile = findConfigFile("jvm.options", jvmOptionsFile);
             if (configFile != null) {

liberty-maven-plugin/src/main/java/io/openliberty/tools/maven/server/StartDebugMojoSupport.java

@@ -92,7 +92,8 @@ public abstract class StartDebugMojoSupport extends ServerFeatureSupport {
 
     protected Map<String,String> bootstrapMavenProps = new HashMap<String,String>();  
     protected Map<String,String> envMavenProps = new HashMap<String,String>();  
-    protected List<String> jvmMavenProps = new ArrayList<String>();  
+    protected List<String> jvmMavenPropNames = new ArrayList<String>();  // only used for tracking overriding properties - not included in the generated jvm.options file
+    protected List<String> jvmMavenPropValues = new ArrayList<String>();  
     protected Map<String,String> varMavenProps = new HashMap<String,String>();  
     protected Map<String,String> defaultVarMavenProps = new HashMap<String,String>();  
 
@@ -566,12 +567,12 @@ protected void copyConfigFiles() throws IOException, MojoExecutionException {
                 optionsFile.delete();
             }
         }
-        if (jvmOptions != null || !jvmMavenProps.isEmpty()) {
+        if (jvmOptions != null || !jvmMavenPropValues.isEmpty()) {
             if (jvmOptionsPath != null) {
                 getLog().warn("The " + jvmOptionsPath + " file is overwritten by inlined configuration.");
             }
             jvmOptionsResolved = handleLatePropertyResolution(jvmOptions);
-            writeJvmOptions(optionsFile, jvmOptionsResolved, jvmMavenProps);
+            writeJvmOptions(optionsFile, jvmOptionsResolved, jvmMavenPropValues);
             jvmOptionsPath = "inlined configuration";
         } else if (jvmOptionsFile != null && jvmOptionsFile.exists()) {
             if (jvmOptionsPath != null) {
@@ -824,7 +825,14 @@ private void loadLibertyConfigFromProperties(Properties props) {
                                      break;
                     case BOOTSTRAP:  bootstrapMavenProps.put(suffix, value);
                                      break;
-                    case JVM:        jvmMavenProps.add(value);
+                    case JVM:        if (jvmMavenPropNames.contains(suffix)) {
+                                        int index = jvmMavenPropNames.indexOf(suffix);
+                                        getLog().debug("Remove duplicate property with name: "+suffix+" at position: "+index);
+                                        jvmMavenPropNames.remove(index);
+                                        jvmMavenPropValues.remove(index);
+                                     }
+                                     jvmMavenPropNames.add(suffix);  // need to keep track of names so that a system prop can override a project prop
+                                     jvmMavenPropValues.add(value);
                                      break;
                     case VAR:        varMavenProps.put(suffix, value);
                                      break;
@@ -943,19 +951,42 @@ private void writeServerEnvProperties(File file, Map<String, String> mavenProper
         }
     }
 
+    // Remove any duplicate entries in the passed in List
+    protected List<String> getUniqueValues(List<String> values) {
+        List<String> uniqueValues = new ArrayList<String> ();
+        if (values == null) {
+            return uniqueValues;
+        }
+
+        for (String nextValue : values) {
+            // by removing a matching existing value, it ensures there will not be a duplicate and that this current one will appear later in the List
+            if (uniqueValues.contains(nextValue)) {
+                getLog().debug("Remove duplicate value: "+nextValue+" at position: "+uniqueValues.indexOf(nextValue));
+            }
+            uniqueValues.remove(nextValue); // has no effect if the value is not present
+            uniqueValues.add(nextValue);
+        }
+        return uniqueValues;
+    }
+
     // One of the passed in Lists must be not null and not empty
     private void writeJvmOptions(File file, List<String> options, List<String> mavenProperties) throws IOException {
-        if (!mavenProperties.isEmpty()) {
-            if (options == null) {
-                combinedJvmOptions = mavenProperties;
+        List<String> uniqueOptions = getUniqueValues(options);
+        List<String> uniqueMavenProps = getUniqueValues(mavenProperties);
+
+        if (!uniqueMavenProps.isEmpty()) {
+            if (uniqueOptions.isEmpty()) {
+                combinedJvmOptions = uniqueMavenProps;
             } else {
                 combinedJvmOptions = new ArrayList<String> ();
-                // add the maven properties first so that they do not take precedence over the options specified with jvmOptions
-                combinedJvmOptions.addAll(mavenProperties);
-                combinedJvmOptions.addAll(options);
+                // add the maven properties (which consist of both project properties and system properties) first,
+                // so that they do not take precedence over the options specified with jvmOptions config parameter
+                combinedJvmOptions.addAll(uniqueMavenProps);
+                combinedJvmOptions.removeAll(uniqueOptions); // remove any exact duplicates before adding all the jvmOptions
+                combinedJvmOptions.addAll(uniqueOptions);
             }
         } else {
-            combinedJvmOptions = options;
+            combinedJvmOptions = uniqueOptions;
         }
 
         makeParentDirectory(file);