Skip to content

Commit

Permalink
Additional changes for handling duplicate values and names on properties
Browse files Browse the repository at this point in the history
  • Loading branch information
@cherylking
cherylking committed Dec 12, 2023
1 parent bd15ef6 commit 67c15c6
Show file tree
Hide file tree
Showing 5 changed files with 133 additions and 36 deletions.
2 changes: 1 addition & 1 deletion liberty-maven-plugin/src/it/server-param-pom-override-it/invoker.properties
View file
Original file line number Diff line number Diff line change
@@ -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 -Dliberty.jvm.minHeapSize=-Xms512m
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"
16 changes: 13 additions & 3 deletions liberty-maven-plugin/src/it/server-param-pom-override-it/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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>
Expand Down
80 changes: 67 additions & 13 deletions ...-param-pom-override-it/src/test/java/net/wasdev/wlp/maven/test/app/PluginConfigXmlIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,42 +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")) {
assertFalse("jvm option found more than once: "+nextLine,myXmx1024mFound);
myXmx1024mFound = true;
} else if (nextLine.equals("-Xmx2056m")) {
fail("jvm option found that should not be present: "+nextLine);
} 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);
}
}

Expand Down
3 changes: 2 additions & 1 deletion ...rty-maven-plugin/src/main/java/io/openliberty/tools/maven/server/PluginConfigSupport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
68 changes: 50 additions & 18 deletions ...y-maven-plugin/src/main/java/io/openliberty/tools/maven/server/StartDebugMojoSupport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>();
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>();

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -820,14 +821,21 @@ private void loadLibertyConfigFromProperties(Properties props) {

getLog().debug("Processing Liberty configuration from property with key "+key+" and value "+value);
switch (propType) {
case ENV: envMavenProps.put(suffix, value);
break;
case BOOTSTRAP: bootstrapMavenProps.put(suffix, value);
break;
case JVM: if (!jvmMavenProps.contains(value)) { jvmMavenProps.add(value); } // avoid exact duplicates
break;
case VAR: varMavenProps.put(suffix, value);
break;
case ENV: envMavenProps.put(suffix, value);
break;
case BOOTSTRAP: bootstrapMavenProps.put(suffix, value);
break;
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);
jvmMavenPropValues.add(value);
break;
case VAR: varMavenProps.put(suffix, value);
break;
case DEFAULTVAR: defaultVarMavenProps.put(suffix, value);
break;
}
Expand Down Expand Up @@ -943,20 +951,44 @@ private void writeServerEnvProperties(File file, Map<String, String> mavenProper
}
}

// Remove any duplicate entries in the passed in List
protected List<String> getUniqueValues(List<String> options) {
List<String> uniqueOptions = new ArrayList<String> ();
if (options == null) {
return uniqueOptions;
}
if (options.isEmpty()) {
return options;
}

for (String nextOption : options) {
// by removing the option first, it ensures this one is last and not a duplicate - nothing happens if the option is not there
if (uniqueOptions.contains(nextOption)) {
getLog().debug("Remove duplicate option: "+nextOption+" at position: "+uniqueOptions.indexOf(nextOption));
}
uniqueOptions.remove(nextOption);
uniqueOptions.add(nextOption);
}
return uniqueOptions;
}

// 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.removeAll(options); // remove any exact duplicates before adding all the jvmOptions
combinedJvmOptions.addAll(options);
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);
Expand Down

0 comments on commit 67c15c6

Please sign in to comment.