From 2403272a9e77cd579c91d0f7b0a0886df200ccb6 Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Tue, 29 Nov 2022 04:28:10 -0800 Subject: [PATCH] Fix Matter command argument list mismatch error (#23721) * Fix Matter command argument list mismatch error * Address the review comments --- .../controller/commands/common/Argument.java | 31 ++++- .../controller/commands/common/Command.java | 119 +++++++++++++----- .../commands/common/MatterCommand.java | 33 ++--- .../commands/discover/DiscoverCommand.java | 4 +- .../commands/pairing/CloseSessionCommand.java | 5 +- .../commands/pairing/PairingCommand.java | 50 ++++---- 6 files changed, 160 insertions(+), 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 index 559c244834c231..c9d911bdc7f347 100644 --- 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 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 index 13ed473f43285f..16c3b30b914aaa 100644 --- 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 mArgs = new ArrayList(); private final Optional mHelpText; @@ -82,17 +84,35 @@ public final Optional 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,25 +159,25 @@ 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); } /** @@ -160,12 +185,13 @@ public final int addArgument(String name, IPAddress out) { * @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/examples/java-matter-controller/java/src/com/matter/controller/commands/common/MatterCommand.java b/examples/java-matter-controller/java/src/com/matter/controller/commands/common/MatterCommand.java index 9aefec5d81b63a..d78a7bb90b9337 100644 --- a/examples/java-matter-controller/java/src/com/matter/controller/commands/common/MatterCommand.java +++ b/examples/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/examples/java-matter-controller/java/src/com/matter/controller/commands/discover/DiscoverCommand.java b/examples/java-matter-controller/java/src/com/matter/controller/commands/discover/DiscoverCommand.java index 3d0bf2518e9098..988e97bb23c3b8 100644 --- a/examples/java-matter-controller/java/src/com/matter/controller/commands/discover/DiscoverCommand.java +++ b/examples/java-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/examples/java-matter-controller/java/src/com/matter/controller/commands/pairing/CloseSessionCommand.java b/examples/java-matter-controller/java/src/com/matter/controller/commands/pairing/CloseSessionCommand.java index ce3e263665866a..f8b911217594ec 100644 --- a/examples/java-matter-controller/java/src/com/matter/controller/commands/pairing/CloseSessionCommand.java +++ b/examples/java-matter-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 diff --git a/examples/java-matter-controller/java/src/com/matter/controller/commands/pairing/PairingCommand.java b/examples/java-matter-controller/java/src/com/matter/controller/commands/pairing/PairingCommand.java index 323c3bd2cb12fa..2d4a6bbfae234d 100644 --- a/examples/java-matter-controller/java/src/com/matter/controller/commands/pairing/PairingCommand.java +++ b/examples/java-matter-controller/java/src/com/matter/controller/commands/pairing/PairingCommand.java @@ -163,18 +163,18 @@ public PairingCommand( throw new RuntimeException(e); } - addArgument("node-id", 0, Long.MAX_VALUE, mNodeId, null); + addArgument("node-id", 0, Long.MAX_VALUE, mNodeId, null, false); switch (networkType) { case NONE: case ETHERNET: break; case WIFI: - addArgument("ssid", mSSID, null); - addArgument("password", mPassword, null); + addArgument("ssid", mSSID, null, false); + addArgument("password", mPassword, null, false); break; case THREAD: - addArgument("operationalDataset", mOperationalDataset, null); + addArgument("operationalDataset", mOperationalDataset, null, false); break; } @@ -184,29 +184,29 @@ public PairingCommand( case CODE: case CODE_PASE_ONLY: Only: - addArgument("payload", mOnboardingPayload, null); - addArgument("discover-once", mDiscoverOnce, null); - addArgument("use-only-onnetwork-discovery", mUseOnlyOnNetworkDiscovery, null); + addArgument("payload", mOnboardingPayload, null, false); + addArgument("discover-once", mDiscoverOnce, null, false); + addArgument("use-only-onnetwork-discovery", mUseOnlyOnNetworkDiscovery, null, false); break; case BLE: - addArgument("setup-pin-code", 0, 134217727, mSetupPINCode, null); - addArgument("discriminator", (short) 0, (short) 4096, mDiscriminator, null); + addArgument("setup-pin-code", 0, 134217727, mSetupPINCode, null, false); + addArgument("discriminator", (short) 0, (short) 4096, mDiscriminator, null, false); break; case ON_NETWORK: - addArgument("setup-pin-code", 0, 134217727, mSetupPINCode, null); + addArgument("setup-pin-code", 0, 134217727, mSetupPINCode, null, false); break; case SOFT_AP: AP: - addArgument("setup-pin-code", 0, 134217727, mSetupPINCode, null); - addArgument("discriminator", (short) 0, (short) 4096, mDiscriminator, null); - addArgument("device-remote-ip", mRemoteAddr); - addArgument("device-remote-port", (short) 0, Short.MAX_VALUE, mRemotePort, null); + addArgument("setup-pin-code", 0, 134217727, mSetupPINCode, null, false); + addArgument("discriminator", (short) 0, (short) 4096, mDiscriminator, null, false); + addArgument("device-remote-ip", mRemoteAddr, false); + addArgument("device-remote-port", (short) 0, Short.MAX_VALUE, mRemotePort, null, false); break; case ETHERNET: - addArgument("setup-pin-code", 0, 134217727, mSetupPINCode, null); - addArgument("discriminator", (short) 0, (short) 4096, mDiscriminator, null); - addArgument("device-remote-ip", mRemoteAddr); - addArgument("device-remote-port", (short) 0, Short.MAX_VALUE, mRemotePort, null); + addArgument("setup-pin-code", 0, 134217727, mSetupPINCode, null, false); + addArgument("discriminator", (short) 0, (short) 4096, mDiscriminator, null, false); + addArgument("device-remote-ip", mRemoteAddr, false); + addArgument("device-remote-port", (short) 0, Short.MAX_VALUE, mRemotePort, null, false); break; } @@ -214,28 +214,28 @@ public PairingCommand( case NONE: break; case SHORT_DISCRIMINATOR: - addArgument("discriminator", (short) 0, (short) 4096, mDiscriminator, null); + addArgument("discriminator", (short) 0, (short) 4096, mDiscriminator, null, false); break; case LONG_DISCRIMINATOR: - addArgument("discriminator", (short) 0, (short) 4096, mDiscriminator, null); + addArgument("discriminator", (short) 0, (short) 4096, mDiscriminator, null, false); break; case VENDOR_ID: - addArgument("vendor-id", (short) 0, Short.MAX_VALUE, mDiscoveryFilterCode, null); + addArgument("vendor-id", (short) 0, Short.MAX_VALUE, mDiscoveryFilterCode, null, false); break; case COMPRESSED_FABRIC_ID: - addArgument("fabric-id", 0, Long.MAX_VALUE, mDiscoveryFilterCode, null); + addArgument("fabric-id", 0, Long.MAX_VALUE, mDiscoveryFilterCode, null, false); break; case COMMISSIONING_MODE: case COMMISSIONER: break; case DEVICE_TYPE: - addArgument("device-type", (short) 0, Short.MAX_VALUE, mDiscoveryFilterCode, null); + addArgument("device-type", (short) 0, Short.MAX_VALUE, mDiscoveryFilterCode, null, false); break; case INSTANCE_NAME: - addArgument("name", mDiscoveryFilterInstanceName, null); + addArgument("name", mDiscoveryFilterInstanceName, null, false); break; } - addArgument("timeout", (long) 0, Long.MAX_VALUE, mTimeoutMillis, null); + addArgument("timeout", (long) 0, Long.MAX_VALUE, mTimeoutMillis, null, false); } }