From 443932d66538fa28dd5a1479468f9bd33501619c Mon Sep 17 00:00:00 2001 From: Jon Sequeira Date: Thu, 13 Jan 2022 18:36:05 -0800 Subject: [PATCH 1/2] CommandLineConfiguration.ThrowIfInvalid --- ...ommandLine_api_is_not_changed.approved.txt | 3 + .../CommandLineConfigurationTests.cs | 267 ++++++++++++++++++ .../CommandLineConfiguration.cs | 58 ++-- .../CommandLineConfigurationException.cs | 15 + src/System.CommandLine/IdentifierSymbol.cs | 2 - src/System.CommandLine/Symbol.cs | 4 +- 6 files changed, 310 insertions(+), 39 deletions(-) create mode 100644 src/System.CommandLine.Tests/CommandLineConfigurationTests.cs create mode 100644 src/System.CommandLine/CommandLineConfigurationException.cs diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index fce37361d3..76b2eba2d5 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -78,6 +78,9 @@ public LocalizationResources LocalizationResources { get; } public Command RootCommand { get; } public System.CommandLine.Collections.SymbolSet Symbols { get; } + public System.Void ThrowIfInvalid() + public class CommandLineConfigurationException : System.Exception, System.Runtime.Serialization.ISerializable + .ctor(System.String message) public static class CompletionSourceExtensions public static System.Void Add(this CompletionSourceList completionSources, System.Func> complete) public static System.Void Add(this CompletionSourceList completionSources, System.CommandLine.Completions.CompletionDelegate complete) diff --git a/src/System.CommandLine.Tests/CommandLineConfigurationTests.cs b/src/System.CommandLine.Tests/CommandLineConfigurationTests.cs new file mode 100644 index 0000000000..47378d5759 --- /dev/null +++ b/src/System.CommandLine.Tests/CommandLineConfigurationTests.cs @@ -0,0 +1,267 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using FluentAssertions; +using Xunit; + +namespace System.CommandLine.Tests; + +public class CommandLineConfigurationTests +{ + [Fact] + public void ThrowIfInvalid_throws_if_there_are_duplicate_sibling_option_aliases_on_the_root_command() + { + var option1 = new Option("--dupe"); + var option2 = new Option("-y"); + option2.AddAlias("--dupe"); + + var command = new RootCommand + { + option1, + option2 + }; + + var config = new CommandLineConfiguration(command); + + var validate = () => config.ThrowIfInvalid(); + + validate.Should() + .Throw() + .Which + .Message + .Should() + .Be($"Duplicate alias '--dupe' found on command '{command.Name}'."); + } + + [Fact] + public void ThrowIfInvalid_throws_if_there_are_duplicate_sibling_option_aliases_on_a_subcommand() + { + var option1 = new Option("--dupe"); + var option2 = new Option("--ok"); + option2.AddAlias("--dupe"); + + var command = new RootCommand + { + new Command("subcommand") + { + option1, + option2 + } + }; + + var config = new CommandLineConfiguration(command); + + var validate = () => config.ThrowIfInvalid(); + + validate.Should() + .Throw() + .Which + .Message + .Should() + .Be("Duplicate alias '--dupe' found on command 'subcommand'."); + } + + [Fact] + public void ThrowIfInvalid_throws_if_there_are_duplicate_sibling_subcommand_aliases_on_the_root_command() + { + var command1 = new Command("dupe"); + var command2 = new Command("not-a-dupe"); + command2.AddAlias("dupe"); + + var rootCommand = new RootCommand + { + command1, + command2 + }; + + var config = new CommandLineConfiguration(rootCommand); + + var validate = () => config.ThrowIfInvalid(); + + validate.Should() + .Throw() + .Which + .Message + .Should() + .Be($"Duplicate alias 'dupe' found on command '{rootCommand.Name}'."); + } + + [Fact] + public void ThrowIfInvalid_throws_if_there_are_duplicate_sibling_subcommand_aliases_on_a_subcommand() + { + var command1 = new Command("dupe"); + var command2 = new Command("not-a-dupe"); + command2.AddAlias("dupe"); + + var command = new RootCommand + { + new Command("subcommand") + { + command1, + command2 + } + }; + + var config = new CommandLineConfiguration(command); + + var validate = () => config.ThrowIfInvalid(); + + validate.Should() + .Throw() + .Which + .Message + .Should() + .Be("Duplicate alias 'dupe' found on command 'subcommand'."); + } + + [Fact] + public void ThrowIfInvalid_throws_if_sibling_command_and_option_aliases_collide_on_the_root_command() + { + var option = new Option("dupe"); + var command = new Command("not-a-dupe"); + command.AddAlias("dupe"); + + var rootCommand = new RootCommand + { + option, + command + }; + + var config = new CommandLineConfiguration(rootCommand); + + var validate = () => config.ThrowIfInvalid(); + + validate.Should() + .Throw() + .Which + .Message + .Should() + .Be($"Duplicate alias 'dupe' found on command '{rootCommand.Name}'."); + } + + [Fact] + public void ThrowIfInvalid_throws_if_sibling_command_and_option_aliases_collide_on_a_subcommand() + { + var option = new Option("dupe"); + var command = new Command("not-a-dupe"); + command.AddAlias("dupe"); + + var rootCommand = new RootCommand + { + new Command("subcommand") + { + option, + command + } + }; + + var config = new CommandLineConfiguration(rootCommand); + + var validate = () => config.ThrowIfInvalid(); + + validate.Should() + .Throw() + .Which + .Message + .Should() + .Be("Duplicate alias 'dupe' found on command 'subcommand'."); + } + + [Fact] + public void ThrowIfInvalid_throws_if_there_are_duplicate_sibling_global_option_aliases_on_the_root_command() + { + var option1 = new Option("--dupe"); + var option2 = new Option("-y"); + option2.AddAlias("--dupe"); + + var command = new RootCommand(); + command.AddGlobalOption(option1); + command.AddGlobalOption(option2); + + var config = new CommandLineConfiguration(command); + + var validate = () => config.ThrowIfInvalid(); + + validate.Should() + .Throw() + .Which + .Message + .Should() + .Be($"Duplicate alias '--dupe' found on command '{command.Name}'."); + } + + [Fact] + public void ThrowIfInvalid_does_not_throw_if_global_option_alias_is_the_same_as_local_option_alias() + { + var rootCommand = new RootCommand + { + new Command("subcommand") + { + new Option("--dupe") + } + }; + rootCommand.AddGlobalOption(new Option("--dupe")); + + var config = new CommandLineConfiguration(rootCommand); + + var validate = () => config.ThrowIfInvalid(); + + validate.Should().NotThrow(); + } + + [Fact] + public void ThrowIfInvalid_does_not_throw_if_global_option_alias_is_the_same_as_subcommand_alias() + { + var rootCommand = new RootCommand + { + new Command("subcommand") + { + new Command("--dupe") + } + }; + rootCommand.AddGlobalOption(new Option("--dupe")); + + var config = new CommandLineConfiguration(rootCommand); + + var validate = () => config.ThrowIfInvalid(); + + validate.Should().NotThrow(); + } + + [Fact] + public void ThrowIfInvalid_throws_if_a_command_is_its_own_parent() + { + var command = new RootCommand(); + command.Add(command); + + var config = new CommandLineConfiguration(command); + + var validate = () => config.ThrowIfInvalid(); + + validate.Should() + .Throw() + .Which + .Message + .Should() + .Be($"Cycle detected in command tree. Command '{command.Name}' is its own ancestor."); + } + + [Fact] + public void ThrowIfInvalid_throws_if_a_parentage_cycle_is_detected() + { + var command = new Command("command"); + var rootCommand = new RootCommand { command }; + command.Add(rootCommand); + + var config = new CommandLineConfiguration(rootCommand); + + var validate = () => config.ThrowIfInvalid(); + + validate.Should() + .Throw() + .Which + .Message + .Should() + .Be($"Cycle detected in command tree. Command '{rootCommand.Name}' is its own ancestor."); + } +} \ No newline at end of file diff --git a/src/System.CommandLine/CommandLineConfiguration.cs b/src/System.CommandLine/CommandLineConfiguration.cs index be55897f8e..7306cd9556 100644 --- a/src/System.CommandLine/CommandLineConfiguration.cs +++ b/src/System.CommandLine/CommandLineConfiguration.cs @@ -8,6 +8,7 @@ using System.CommandLine.Invocation; using System.CommandLine.IO; using System.CommandLine.Parsing; +using System.Linq; namespace System.CommandLine { @@ -53,7 +54,7 @@ public CommandLineConfiguration( internal static HelpBuilder DefaultHelpBuilderFactory(BindingContext context, int? requestedMaxWidth = null) { - int maxWidth = requestedMaxWidth ?? int.MaxValue; + int maxWidth = requestedMaxWidth ?? int.MaxValue; if (context.Console is SystemConsole systemConsole) { maxWidth = systemConsole.GetWindowWidth(); @@ -102,58 +103,47 @@ internal static HelpBuilder DefaultHelpBuilderFactory(BindingContext context, in internal ResponseFileHandling ResponseFileHandling { get; } /// - /// Validates all symbols including the child hierarchy. + /// Throws an exception if the parser configuration is ambiguous or otherwise not valid. /// - /// Due to the performance impact of this method, it's recommended to create - /// a Unit Test that calls this method to verify the RootCommand of every application. - internal void ThrowIfInvalid() + /// Due to the performance cost of this method, it is recommended to be used in unit testing or in scenarios where the parser is configured dynamically at runtime. + /// Thrown if the configuration is found to be invalid. + public void ThrowIfInvalid() { ThrowIfInvalid(RootCommand); static void ThrowIfInvalid(Command command) { - for (int i = 0; i < command.Children.Count; i++) + if (command.Parents.FlattenBreadthFirst(c => c.Parents).Any(ancestor => ancestor == command)) { - for (int j = 1; j < command.Children.Count; j++) + throw new CommandLineConfigurationException($"Cycle detected in command tree. Command '{command.Name}' is its own ancestor."); + } + + for (var i = 0; i < command.Children.Count; i++) + { + if (command.Children[i] is IdentifierSymbol symbol1AsIdentifier) { - if (command.Children[j] is IdentifierSymbol identifierSymbol) + for (var j = i + 1; j < command.Children.Count; j++) { - foreach (string alias in identifierSymbol.Aliases) + if (command.Children[j] is IdentifierSymbol symbol2AsIdentifier) { - if (command.Children[i].Matches(alias)) + foreach (var symbol2Alias in symbol2AsIdentifier.Aliases) { - throw new ArgumentException($"Alias '{alias}' is already in use."); + if (symbol1AsIdentifier.Name.Equals(symbol2Alias, StringComparison.Ordinal) || + symbol1AsIdentifier.Aliases.Contains(symbol2Alias)) + { + throw new CommandLineConfigurationException($"Duplicate alias '{symbol2Alias}' found on command '{command.Name}'."); + } } } - - if (identifierSymbol is Command childCommand) - { - if (ReferenceEquals(command, childCommand)) - { - throw new ArgumentException("Parent can't be it's own child."); - } - - ThrowIfInvalid(childCommand); - } } - if (command.Children[i].Matches(command.Children[j].Name)) + if (symbol1AsIdentifier is Command childCommand) { - throw new ArgumentException($"Alias '{command.Children[j].Name}' is already in use."); + ThrowIfInvalid(childCommand); } } - - if (command.Children.Count == 1 && command.Children[0] is Command singleChild) - { - if (ReferenceEquals(command, singleChild)) - { - throw new ArgumentException("Parent can't be it's own child."); - } - - ThrowIfInvalid(singleChild); - } } } } } -} +} \ No newline at end of file diff --git a/src/System.CommandLine/CommandLineConfigurationException.cs b/src/System.CommandLine/CommandLineConfigurationException.cs new file mode 100644 index 0000000000..35a49e88a6 --- /dev/null +++ b/src/System.CommandLine/CommandLineConfigurationException.cs @@ -0,0 +1,15 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace System.CommandLine; + +/// +/// Indicates that a command line configuration is invalid. +/// +public class CommandLineConfigurationException : Exception +{ + /// + public CommandLineConfigurationException(string message) : base(message) + { + } +} \ No newline at end of file diff --git a/src/System.CommandLine/IdentifierSymbol.cs b/src/System.CommandLine/IdentifierSymbol.cs index 2e365352c6..6230e7a60d 100644 --- a/src/System.CommandLine/IdentifierSymbol.cs +++ b/src/System.CommandLine/IdentifierSymbol.cs @@ -59,8 +59,6 @@ public override string Name } } - internal override bool Matches(string name) => string.Equals(name, Name, StringComparison.Ordinal) || _aliases.Contains(name); - /// /// Adds an alias. Multiple aliases can be added, most often used to provide a shorthand alternative. /// diff --git a/src/System.CommandLine/Symbol.cs b/src/System.CommandLine/Symbol.cs index 69358b50e0..2b76c9d308 100644 --- a/src/System.CommandLine/Symbol.cs +++ b/src/System.CommandLine/Symbol.cs @@ -41,9 +41,7 @@ public virtual string Name /// Represents the first parent node. /// internal ParentNode? FirstParent => _firstParent; - - internal virtual bool Matches(string name) => string.Equals(name, _name, StringComparison.Ordinal); - + internal void AddParent(Symbol symbol) { if (_firstParent == null) From 3f7dec2f1fb0e81df1b2279536e9c85f1f5b9c54 Mon Sep 17 00:00:00 2001 From: Jon Sequeira Date: Fri, 14 Jan 2022 13:26:09 -0800 Subject: [PATCH 2/2] add exception constructors: --- ...CommandLine_api_is_not_changed.approved.txt | 2 ++ .../CommandLineConfigurationException.cs | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index 76b2eba2d5..822edc2fbc 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -81,6 +81,8 @@ public System.Void ThrowIfInvalid() public class CommandLineConfigurationException : System.Exception, System.Runtime.Serialization.ISerializable .ctor(System.String message) + .ctor() + .ctor(System.String message, System.Exception innerException) public static class CompletionSourceExtensions public static System.Void Add(this CompletionSourceList completionSources, System.Func> complete) public static System.Void Add(this CompletionSourceList completionSources, System.CommandLine.Completions.CompletionDelegate complete) diff --git a/src/System.CommandLine/CommandLineConfigurationException.cs b/src/System.CommandLine/CommandLineConfigurationException.cs index 35a49e88a6..1263a5535d 100644 --- a/src/System.CommandLine/CommandLineConfigurationException.cs +++ b/src/System.CommandLine/CommandLineConfigurationException.cs @@ -1,15 +1,33 @@ // Copyright (c) .NET Foundation and contributors. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using System.Runtime.Serialization; + namespace System.CommandLine; /// /// Indicates that a command line configuration is invalid. /// +[Serializable] public class CommandLineConfigurationException : Exception { /// public CommandLineConfigurationException(string message) : base(message) { } + + /// + public CommandLineConfigurationException() + { + } + + /// + protected CommandLineConfigurationException(SerializationInfo info, StreamingContext context) : base(info, context) + { + } + + /// + public CommandLineConfigurationException(string message, Exception innerException) : base(message, innerException) + { + } } \ No newline at end of file