Fix Matter command argument list mismatch error (#23721)

* Fix Matter command argument list mismatch error * Address the review comments
project-chip · Oct 27, 2023 · ab0dc98 · ab0dc98
1 parent c857478
commit ab0dc98
Show file tree

Hide file tree

Showing 6 changed files with 160 additions and 82 deletions.
diff --git a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/Argument.java b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/Argument.java
@@ -33,59 +33,78 @@ public final class Argument {
   private final long mMax;
   private final Object mValue;
   private final Optional<String> mDesc;
+  private final boolean mOptional;
 
-  public Argument(String name, IPAddress value) {
+  boolean isOptional() {
+    return mOptional;
+  }
+
+  public Argument(String name, IPAddress value, boolean optional) {
     this.mName = name;
     this.mType = ArgumentType.ADDRESS;
     this.mMin = 0;
     this.mMax = 0;
     this.mValue = value;
     this.mDesc = Optional.empty();
+    this.mOptional = optional;
   }
 
-  public Argument(String name, StringBuffer value, @Nullable String desc) {
+  public Argument(String name, StringBuffer value, @Nullable String desc, boolean optional) {
     this.mName = name;
     this.mType = ArgumentType.STRING;
     this.mMin = 0;
     this.mMax = 0;
     this.mValue = value;
     this.mDesc = Optional.ofNullable(desc);
+    this.mOptional = optional;
   }
 
-  public Argument(String name, AtomicBoolean value, @Nullable String desc) {
+  public Argument(String name, AtomicBoolean value, @Nullable String desc, boolean optional) {
     this.mName = name;
     this.mType = ArgumentType.BOOL;
     this.mMin = 0;
     this.mMax = 0;
     this.mValue = value;
     this.mDesc = Optional.ofNullable(desc);
+    this.mOptional = optional;
   }
 
-  public Argument(String name, short min, short max, AtomicInteger value, @Nullable String desc) {
+  public Argument(
+      String name,
+      short min,
+      short max,
+      AtomicInteger value,
+      @Nullable String desc,
+      boolean optional) {
     this.mName = name;
     this.mType = ArgumentType.NUMBER_INT16;
     this.mMin = min;
     this.mMax = max;
     this.mValue = value;
     this.mDesc = Optional.ofNullable(desc);
+    this.mOptional = optional;
   }
 
-  public Argument(String name, int min, int max, AtomicInteger value, @Nullable String desc) {
+  public Argument(
+      String name, int min, int max, AtomicInteger value, @Nullable String desc, boolean optional) {
     this.mName = name;
     this.mType = ArgumentType.NUMBER_INT32;
     this.mMin = min;
     this.mMax = max;
     this.mValue = value;
     this.mDesc = Optional.ofNullable(desc);
+    this.mOptional = optional;
   }
 
-  public Argument(String name, long min, long max, AtomicLong value, @Nullable String desc) {
+  public Argument(
+      String name, long min, long max, AtomicLong value, @Nullable String desc, boolean optional) {
     this.mName = name;
     this.mType = ArgumentType.NUMBER_INT64;
     this.mMin = min;
     this.mMax = max;
     this.mValue = value;
     this.mDesc = Optional.ofNullable(desc);
+    this.mOptional = optional;
   }
 
   public String getName() {

diff --git a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/Command.java b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/Command.java
@@ -32,6 +32,8 @@
  *     Matter devices from the environment.
  */
 public abstract class Command {
+  private static final String OPTIONAL_ARGUMENT_PREFIX = "--";
+  private static final int OPTIONAL_ARGUMENT_PREFIX_LENGTH = 2;
   private final String mName;
   private final ArrayList<Argument> mArgs = new ArrayList<Argument>();
   private final Optional<String> mHelpText;
@@ -82,17 +84,35 @@ public final Optional<String> getArgumentDescription(int index) {
     return mArgs.get(index).getDesc();
   }
 
+  public final void addArgumentToList(Argument arg, boolean optional) {
+    if (arg.isOptional() || mArgs.isEmpty()) {
+      // Safe to just append to the end of a list.
+      mArgs.add(arg);
+      return;
+    }
+
+    // mandatory arg needs to be inserted before the optional arguments.
+    int index = 0;
+    while (index < mArgs.size() && !mArgs.get(index).isOptional()) {
+      index++;
+    }
+
+    // Insert before the first optional arg.
+    mArgs.add(index, arg);
+  }
+
   /**
    * @brief Add a bool command argument
    * @param name The name that will be displayed in the command help
    * @param out A pointer to a MutableInteger where the argv value will be stored
    * @param desc The description of the argument that will be displayed in the command help
+   * @param optional Indicate if an optional argument
    * @return The number of arguments currently added to the command
    */
-  public final int addArgument(String name, AtomicBoolean out, @Nullable String desc) {
-    Argument arg = new Argument(name, out, desc);
-    mArgs.add(arg);
-    return mArgs.size();
+  public final void addArgument(
+      String name, AtomicBoolean out, @Nullable String desc, boolean optional) {
+    Argument arg = new Argument(name, out, desc, optional);
+    addArgumentToList(arg, optional);
   }
 
   /**
@@ -102,13 +122,18 @@ public final int addArgument(String name, AtomicBoolean out, @Nullable String de
    * @param max The minimum value of the argv value
    * @param out A pointer to a MutableInteger where the argv value will be stored
    * @param desc The description of the argument that will be displayed in the command help
+   * @param optional Indicate if an optional argument
    * @return The number of arguments currently added to the command
    */
-  public final int addArgument(
-      String name, short min, short max, AtomicInteger out, @Nullable String desc) {
-    Argument arg = new Argument(name, min, max, out, desc);
-    mArgs.add(arg);
-    return mArgs.size();
+  public final void addArgument(
+      String name,
+      short min,
+      short max,
+      AtomicInteger out,
+      @Nullable String desc,
+      boolean optional) {
+    Argument arg = new Argument(name, min, max, out, desc, optional);
+    addArgumentToList(arg, optional);
   }
 
   /**
@@ -118,13 +143,13 @@ public final int addArgument(
    * @param max The minimum value of the argv value
    * @param out A pointer to a MutableInteger where the argv value will be stored
    * @param desc The description of the argument that will be displayed in the command help
+   * @param optional Indicate if an optional argument
    * @return The number of arguments currently added to the command
    */
-  public final int addArgument(
-      String name, int min, int max, AtomicInteger out, @Nullable String desc) {
-    Argument arg = new Argument(name, min, max, out, desc);
-    mArgs.add(arg);
-    return mArgs.size();
+  public final void addArgument(
+      String name, int min, int max, AtomicInteger out, @Nullable String desc, boolean optional) {
+    Argument arg = new Argument(name, min, max, out, desc, optional);
+    addArgumentToList(arg, optional);
   }
 
   /**
@@ -134,38 +159,39 @@ public final int addArgument(
    * @param max The minimum value of the argv value
    * @param out A pointer to a MutableInteger where the argv value will be stored
    * @param desc The description of the argument that will be displayed in the command help
+   * @param optional Indicate if an optional argument
    * @return The number of arguments currently added to the command
    */
-  public final int addArgument(
-      String name, long min, long max, AtomicLong out, @Nullable String desc) {
-    Argument arg = new Argument(name, min, max, out, desc);
-    mArgs.add(arg);
-    return mArgs.size();
+  public final void addArgument(
+      String name, long min, long max, AtomicLong out, @Nullable String desc, boolean optional) {
+    Argument arg = new Argument(name, min, max, out, desc, optional);
+    addArgumentToList(arg, optional);
   }
 
   /**
    * @brief Add an IP address command argument
    * @param name The name that will be displayed in the command help
    * @param out A pointer to a IPAddress where the argv value will be stored
+   * @param optional Indicate if an optional argument
    * @return The number of arguments currently added to the command
    */
-  public final int addArgument(String name, IPAddress out) {
-    Argument arg = new Argument(name, out);
-    mArgs.add(arg);
-    return mArgs.size();
+  public final void addArgument(String name, IPAddress out, boolean optional) {
+    Argument arg = new Argument(name, out, optional);
+    addArgumentToList(arg, optional);
   }
 
   /**
    * @brief Add a String command argument
    * @param name The name that will be displayed in the command help
    * @param out A pointer to a StringBuffer where the argv value will be stored
    * @param desc The description of the argument that will be displayed in the command help
+   * @param optional Indicate if an optional argument
    * @return The number of arguments currently added to the command
    */
-  public final int addArgument(String name, StringBuffer out, @Nullable String desc) {
-    Argument arg = new Argument(name, out, desc);
-    mArgs.add(arg);
-    return mArgs.size();
+  public final void addArgument(
+      String name, StringBuffer out, @Nullable String desc, boolean optional) {
+    Argument arg = new Argument(name, out, desc, optional);
+    addArgumentToList(arg, optional);
   }
 
   /**
@@ -174,15 +200,44 @@ public final int addArgument(String name, StringBuffer out, @Nullable String des
    * @param args Supplied command-line arguments as an array of String objects.
    */
   public final void initArguments(int argc, String[] args) {
-    int argsCount = mArgs.size();
+    int mandatoryArgsCount = 0;
+    int currentIndex = 0;
 
-    if (argsCount != argc) {
+    for (Argument arg : mArgs) {
+      if (!arg.isOptional()) {
+        mandatoryArgsCount++;
+      }
+    }
+
+    if (argc < mandatoryArgsCount) {
       throw new IllegalArgumentException(
-          "Wrong arguments number: " + argc + " instead of " + argsCount);
+          "initArguments: Wrong arguments number: " + argc + " instead of " + mandatoryArgsCount);
+    }
+
+    // Initialize mandatory arguments
+    for (int i = 0; i < mandatoryArgsCount; i++) {
+      initArgument(currentIndex++, args[i]);
     }
 
-    for (int i = 0; i < argsCount; i++) {
-      initArgument(i, args[i]);
+    // Initialize optional arguments
+    // Optional arguments expect a name and a value, so i is increased by 2 on every step.
+    for (int i = mandatoryArgsCount; i < argc; i += 2) {
+      // optional arguments starts with OPTIONAL_ARGUMENT_PREFIX
+      if (args[i].length() <= OPTIONAL_ARGUMENT_PREFIX_LENGTH
+          && !args[i].startsWith(OPTIONAL_ARGUMENT_PREFIX)) {
+        throw new IllegalArgumentException("initArguments: Invalid optional argument: " + args[i]);
+      }
+
+      if (args[i]
+          .substring(OPTIONAL_ARGUMENT_PREFIX_LENGTH)
+          .equals(mArgs.get(currentIndex).getName())) {
+        if (i + 1 >= argc) {
+          throw new IllegalArgumentException(
+              "initArguments: Optional argument " + args[i] + " missing value");
+        }
+
+        initArgument(currentIndex++, args[i + 1]);
+      }
     }
   }
 

diff --git a/.../java-matter-controller/java/src/com/matter/controller/commands/common/MatterCommand.java b/.../java-matter-controller/java/src/com/matter/controller/commands/common/MatterCommand.java
@@ -57,40 +57,43 @@ public MatterCommand(
     this.mCredIssuerCmds = Optional.ofNullable(credIssuerCmds);
     this.mChipDeviceController = controller;
 
-    // TODO: Add support to enable the below optional arguments
-    /*
     addArgument(
-        "paa-trust-store-path",
+        "--paa-trust-store-path",
         mPaaTrustStorePath,
         "Path to directory holding PAA certificate information.  Can be absolute or relative to the current working "
-            + "directory.");
+            + "directory.",
+        true);
     addArgument(
-        "cd-trust-store-path",
+        "--cd-trust-store-path",
         mCDTrustStorePath,
         "Path to directory holding CD certificate information.  Can be absolute or relative to the current working "
-            + "directory.");
+            + "directory.",
+        true);
     addArgument(
-        "commissioner-name",
+        "--commissioner-name",
         mCommissionerName,
         "Name of fabric to use. Valid values are \"alpha\", \"beta\", \"gamma\", and integers greater than or equal to "
-            + "4.  The default if not specified is \"alpha\".");
+            + "4.  The default if not specified is \"alpha\".",
+        true);
     addArgument(
-        "commissioner-nodeid",
+        "--commissioner-nodeid",
         0,
         Long.MAX_VALUE,
         mCommissionerNodeId,
-        "The node id to use for chip-tool.  If not provided, kTestControllerNodeId (112233, 0x1B669) will be used.");
+        "The node id to use for chip-tool.  If not provided, kTestControllerNodeId (112233, 0x1B669) will be used.",
+        true);
     addArgument(
-        "use-max-sized-certs",
+        "--use-max-sized-certs",
         mUseMaxSizedCerts,
         "Maximize the size of operational certificates. If not provided or 0 (\"false\"), normally sized operational "
-            + "certificates are generated.");
+            + "certificates are generated.",
+        true);
     addArgument(
-        "only-allow-trusted-cd-keys",
+        "--only-allow-trusted-cd-keys",
         mOnlyAllowTrustedCdKeys,
         "Only allow trusted CD verifying keys (disallow test keys). If not provided or 0 (\"false\"), untrusted CD "
-            + "verifying keys are allowed. If 1 (\"true\"), test keys are disallowed.");
-    */
+            + "verifying keys are allowed. If 1 (\"true\"), test keys are disallowed.",
+        true);
   }
 
   // This method returns the commissioner instance to be used for running the command.

diff --git a/...a-matter-controller/java/src/com/matter/controller/commands/discover/DiscoverCommand.java b/...a-matter-controller/java/src/com/matter/controller/commands/discover/DiscoverCommand.java
@@ -29,8 +29,8 @@ public final class DiscoverCommand extends MatterCommand {
 
   public DiscoverCommand(ChipDeviceController controller, CredentialsIssuer credsIssuer) {
     super(controller, "resolve", credsIssuer);
-    addArgument("nodeid", 0, Long.MAX_VALUE, mNodeId, null);
-    addArgument("fabricid", 0, Long.MAX_VALUE, mFabricId, null);
+    addArgument("nodeid", 0, Long.MAX_VALUE, mNodeId, null, false);
+    addArgument("fabricid", 0, Long.MAX_VALUE, mFabricId, null, false);
   }
 
   @Override

diff --git a/...atter-controller/java/src/com/matter/controller/commands/pairing/CloseSessionCommand.java b/...atter-controller/java/src/com/matter/controller/commands/pairing/CloseSessionCommand.java
@@ -30,13 +30,14 @@ public final class CloseSessionCommand extends MatterCommand {
 
   public CloseSessionCommand(ChipDeviceController controller, CredentialsIssuer credsIssuer) {
     super(controller, "close-session", credsIssuer);
-    addArgument("destination-id", 0, Long.MAX_VALUE, mDestinationId, null);
+    addArgument("destination-id", 0, Long.MAX_VALUE, mDestinationId, null, false);
     addArgument(
         "timeout",
         (short) 0,
         Short.MAX_VALUE,
         mTimeoutSecs,
-        "Time, in seconds, before this command is considered to have timed out.");
+        "Time, in seconds, before this command is considered to have timed out.",
+        false);
   }
 
   @Override