8288783: Error messages are confusing when options conflict in -XX:StartFlightRecording

Reviewed-by: egahlin

src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/ArgumentParser.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2021, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -30,6 +30,7 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.StringJoiner;
 
 final class ArgumentParser {
     private final Map<String, Object> options = new HashMap<>();
@@ -42,6 +43,8 @@ final class ArgumentParser {
     private final Argument[] arguments;
     private int position;
 
+    private final List<String> conflictedOptions = new ArrayList<>();
+
     ArgumentParser(Argument[] arguments, String text, char delimiter) {
         this.text = text;
         this.delimiter = delimiter;
@@ -64,10 +67,36 @@ final class ArgumentParser {
             addOption(key, value);
             eatDelimiter();
         }
+        checkConflict();
         checkMandatory();
         return options;
     }
 
+    protected void checkConflict() {
+        if (conflictedOptions.isEmpty()) {
+            return;
+        }
+
+        StringBuilder sb = new StringBuilder();
+        sb.append("Option");
+
+        // If multiple options conflict, the following blocks are executed
+        if (conflictedOptions.size() > 1) {
+            sb.append("s ");
+            StringJoiner sj = new StringJoiner(", ");
+            while (conflictedOptions.size() > 1) {
+                sj.add(conflictedOptions.remove(0));
+            }
+            sb.append(sj);
+            sb.append(" and");
+        }
+
+        sb.append(" ");
+        sb.append(conflictedOptions.remove(0));
+        sb.append(" can only be specified once.");
+        throw new IllegalArgumentException(sb.toString());
+    }
+
     private void checkMandatory() {
         for (Argument arg : arguments) {
             if (!options.containsKey(arg.name())) {
@@ -94,9 +123,12 @@ final class ArgumentParser {
                     }
                 } else {
                     if (options.containsKey(key)) {
-                        throw new IllegalArgumentException("Duplicates in diagnostic command arguments");
+                        if (!conflictedOptions.contains(key)) {
+                            conflictedOptions.add(key);
+                        }
+                    } else {
+                        options.put(key, v);
                     }
-                    options.put(key, v);
                 }
             }
         }

test/jdk/jdk/jfr/jcmd/TestJcmdOptionSpecifiedOnce.java

@@ -0,0 +1,46 @@
+/*
+ * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package jdk.jfr.jcmd;
+
+/**
+ * @test
+ * @summary The test verifies that options can only be specified once with jcmd JFR
+ * @key jfr
+ * @requires vm.hasJFR
+ * @library /test/lib /test/jdk
+ * @modules jdk.jfr/jdk.jfr.internal.dcmd
+ * @run main/othervm jdk.jfr.jcmd.TestJcmdOptionSpecifiedOnce
+ */
+public class TestJcmdOptionSpecifiedOnce {
+
+    public static void main(String[] args) throws Exception {
+
+        testJCmdConflict();
+    }
+
+    private static void testJCmdConflict() {
+        var output= JcmdHelper.jcmd("JFR.start name=hello name=greetings");
+        output.shouldContain("name can only be specified once");
+    }
+}
+

test/jdk/jdk/jfr/startupargs/TestStartupOptionSpecifiedOnce.java

@@ -0,0 +1,56 @@
+/*
+ * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package jdk.jfr.startupargs;
+
+import jdk.test.lib.process.ProcessTools;
+
+/**
+ * @test The test verifies that options can only be specified once with --XX:StartFlightRecording
+ * @key jfr
+ * @requires vm.hasJFR
+ * @library /test/lib /test/jdk
+ * @run main jdk.jfr.startupargs.TestStartupOptionSpecifiedOnce
+ */
+public class TestStartupOptionSpecifiedOnce {
+
+    public static void main(String[] args) throws Exception {
+        testStartFlightRecordingConflict();
+        testConflictThreeOptions();
+        testAbleMultipleOption();
+    }
+
+    private static void testStartFlightRecordingConflict() throws Exception {
+        var output = ProcessTools.executeTestJava("-XX:StartFlightRecording:disk=true,disk=false,name=cat,name=dog");
+        output.shouldContain("disk and name can only be specified once.");
+    }
+
+    private static void testConflictThreeOptions() throws Exception {
+        var output = ProcessTools.executeTestJava("-XX:StartFlightRecording:name=abc,name=def,disk=true,disk=false,delay=1s,delay=2s");
+        output.shouldContain("name, disk and delay can only be specified once.");
+    }
+
+    private static void testAbleMultipleOption() throws Exception {
+        var output = ProcessTools.executeTestJava("-XX:StartFlightRecording:settings=default,settings=profile");
+        output.shouldNotContain("settings can only be specified once");
+    }
+}