From 892e48f86474e1e1885bddef62620cfd2402dc09 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Tue, 5 Mar 2024 13:36:58 -0800 Subject: [PATCH 1/7] update cli to return -1 when failure occurs. 0 when successful. Next revision can explore returning specific codes for each unique error. --- src/Cli/Commands/AddOptions.cs | 6 ++-- src/Cli/Commands/AddTelemetryOptions.cs | 4 ++- src/Cli/Commands/InitOptions.cs | 6 +++- src/Cli/Commands/StartOptions.cs | 4 ++- src/Cli/Commands/UpdateOptions.cs | 6 ++-- src/Cli/Commands/ValidateOptions.cs | 4 ++- src/Cli/Exporter.cs | 9 ++++-- src/Cli/Program.cs | 37 +++++++--------------- src/Cli/ResultHandler.cs | 42 +++++++++++++++++++++++++ 9 files changed, 81 insertions(+), 37 deletions(-) create mode 100644 src/Cli/ResultHandler.cs diff --git a/src/Cli/Commands/AddOptions.cs b/src/Cli/Commands/AddOptions.cs index aa5d4c3667..6d2e799990 100644 --- a/src/Cli/Commands/AddOptions.cs +++ b/src/Cli/Commands/AddOptions.cs @@ -56,12 +56,12 @@ public AddOptions( [Option("permissions", Required = true, Separator = ':', HelpText = "Permissions required to access the source table or container.")] public IEnumerable Permissions { get; } - public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) + public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) { logger.LogInformation("{productName} {version}", PRODUCT_NAME, ProductInfo.GetProductVersion()); if (!IsEntityProvided(Entity, logger, command: "add")) { - return; + return -1; } bool isSuccess = ConfigGenerator.TryAddEntityToConfigWithOptions(this, loader, fileSystem); @@ -74,6 +74,8 @@ public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileS { logger.LogError("Could not add entity: {Entity} with source: {Source} and permissions: {permissions}.", Entity, Source, string.Join(SEPARATOR, Permissions)); } + + return isSuccess ? 0 : -1; } } } diff --git a/src/Cli/Commands/AddTelemetryOptions.cs b/src/Cli/Commands/AddTelemetryOptions.cs index f51bf47a98..2465eb5b5e 100644 --- a/src/Cli/Commands/AddTelemetryOptions.cs +++ b/src/Cli/Commands/AddTelemetryOptions.cs @@ -32,7 +32,7 @@ public AddTelemetryOptions(string appInsightsConnString, CliBool appInsightsEnab [Option("app-insights-enabled", Default = CliBool.True, Required = false, HelpText = "(Default: true) Enable/Disable Application Insights")] public CliBool AppInsightsEnabled { get; } - public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) + public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) { logger.LogInformation("{productName} {version}", PRODUCT_NAME, ProductInfo.GetProductVersion()); @@ -46,6 +46,8 @@ public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileS { logger.LogError("Failed to add telemetry to the configuration file."); } + + return isSuccess ? 0 : -1; } } } diff --git a/src/Cli/Commands/InitOptions.cs b/src/Cli/Commands/InitOptions.cs index 1d7ec4bf59..45da3bcd1f 100644 --- a/src/Cli/Commands/InitOptions.cs +++ b/src/Cli/Commands/InitOptions.cs @@ -120,7 +120,9 @@ public InitOptions( [Option("rest.request-body-strict", Required = false, HelpText = "(Default: true) Allow extraneous fields in the request body for REST.")] public CliBool RestRequestBodyStrict { get; } - public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) + // public int returnCode; + + public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) { logger.LogInformation("{productName} {version}", PRODUCT_NAME, ProductInfo.GetProductVersion()); bool isSuccess = ConfigGenerator.TryGenerateConfig(this, loader, fileSystem); @@ -128,10 +130,12 @@ public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileS { logger.LogInformation("Config file generated."); logger.LogInformation("SUGGESTION: Use 'dab add [entity-name] [options]' to add new entities in your config."); + return 0; } else { logger.LogError("Could not generate config file."); + return -1; } } } diff --git a/src/Cli/Commands/StartOptions.cs b/src/Cli/Commands/StartOptions.cs index a3ae765d35..0d9187f219 100644 --- a/src/Cli/Commands/StartOptions.cs +++ b/src/Cli/Commands/StartOptions.cs @@ -35,7 +35,7 @@ public StartOptions(bool verbose, LogLevel? logLevel, bool isHttpsRedirectionDis [Option("no-https-redirect", Required = false, HelpText = "Disables automatic https redirects.")] public bool IsHttpsRedirectionDisabled { get; } - public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) + public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) { logger.LogInformation("{productName} {version}", PRODUCT_NAME, ProductInfo.GetProductVersion()); bool isSuccess = ConfigGenerator.TryStartEngineWithOptions(this, loader, fileSystem); @@ -44,6 +44,8 @@ public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileS { logger.LogError("Failed to start the engine."); } + + return isSuccess ? 0 : -1; } } } diff --git a/src/Cli/Commands/UpdateOptions.cs b/src/Cli/Commands/UpdateOptions.cs index 700f267a5f..7589bb530b 100644 --- a/src/Cli/Commands/UpdateOptions.cs +++ b/src/Cli/Commands/UpdateOptions.cs @@ -96,12 +96,12 @@ public UpdateOptions( [Option('m', "map", Separator = ',', Required = false, HelpText = "Specify mappings between database fields and GraphQL and REST fields. format: --map \"backendName1:exposedName1,backendName2:exposedName2,...\".")] public IEnumerable? Map { get; } - public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) + public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) { logger.LogInformation("{productName} {version}", PRODUCT_NAME, ProductInfo.GetProductVersion()); if (!IsEntityProvided(Entity, logger, command: "update")) { - return; + return -1; } bool isSuccess = ConfigGenerator.TryUpdateEntityWithOptions(this, loader, fileSystem); @@ -114,6 +114,8 @@ public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileS { logger.LogError("Could not update the entity: {Entity}.", Entity); } + + return isSuccess ? 0 : -1; } } } diff --git a/src/Cli/Commands/ValidateOptions.cs b/src/Cli/Commands/ValidateOptions.cs index 1bd13ac658..98a811c948 100644 --- a/src/Cli/Commands/ValidateOptions.cs +++ b/src/Cli/Commands/ValidateOptions.cs @@ -24,7 +24,7 @@ public ValidateOptions(string config) /// This Handler method is responsible for validating the config file and is called when `validate` /// command is invoked. /// - public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) + public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) { logger.LogInformation("{productName} {version}", PRODUCT_NAME, ProductInfo.GetProductVersion()); bool isValidConfig = ConfigGenerator.IsConfigValid(this, loader, fileSystem); @@ -37,6 +37,8 @@ public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileS { logger.LogError("Config is invalid. Check above logs for details."); } + + return isValidConfig ? 0 : -1; } } } diff --git a/src/Cli/Exporter.cs b/src/Cli/Exporter.cs index f5bb90fc09..4b24c4b23f 100644 --- a/src/Cli/Exporter.cs +++ b/src/Cli/Exporter.cs @@ -13,7 +13,7 @@ namespace Cli { internal static class Exporter { - public static void Export(ExportOptions options, ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) + public static int Export(ExportOptions options, ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) { StartOptions startOptions = new(false, LogLevel.None, false, options.Config!); @@ -23,7 +23,7 @@ public static void Export(ExportOptions options, ILogger logger, FileSystemRunti if (!TryGetConfigFileBasedOnCliPrecedence(loader, options.Config, out string runtimeConfigFile)) { logger.LogError("Failed to find the config file provided, check your options and try again."); - return; + return -1; } if (!loader.TryLoadConfig( @@ -32,7 +32,7 @@ public static void Export(ExportOptions options, ILogger logger, FileSystemRunti replaceEnvVar: true) || runtimeConfig is null) { logger.LogError("Failed to read the config file: {runtimeConfigFile}.", runtimeConfigFile); - return; + return -1; } Task server = Task.Run(() => @@ -40,6 +40,7 @@ public static void Export(ExportOptions options, ILogger logger, FileSystemRunti _ = ConfigGenerator.TryStartEngineWithOptions(startOptions, loader, fileSystem); }, cancellationToken); + bool isSuccess = false; if (options.GraphQL) { int retryCount = 5; @@ -50,6 +51,7 @@ public static void Export(ExportOptions options, ILogger logger, FileSystemRunti try { ExportGraphQL(options, runtimeConfig, fileSystem); + isSuccess = true; break; } catch @@ -65,6 +67,7 @@ public static void Export(ExportOptions options, ILogger logger, FileSystemRunti } cancellationTokenSource.Cancel(); + return isSuccess ? 0 : -1; } private static void ExportGraphQL(ExportOptions options, RuntimeConfig runtimeConfig, System.IO.Abstractions.IFileSystem fileSystem) diff --git a/src/Cli/Program.cs b/src/Cli/Program.cs index 4731d13ef2..2b34103911 100644 --- a/src/Cli/Program.cs +++ b/src/Cli/Program.cs @@ -52,33 +52,18 @@ public static int Execute(string[] args, ILogger cliLogger, IFileSystem fileSyst }); // Parsing user arguments and executing required methods. - ParserResult? result = parser.ParseArguments(args) - .WithParsed((Action)(options => options.Handler(cliLogger, loader, fileSystem))) - .WithParsed((Action)(options => options.Handler(cliLogger, loader, fileSystem))) - .WithParsed((Action)(options => options.Handler(cliLogger, loader, fileSystem))) - .WithParsed((Action)(options => options.Handler(cliLogger, loader, fileSystem))) - .WithParsed((Action)(options => options.Handler(cliLogger, loader, fileSystem))) - .WithParsed((Action)(options => options.Handler(cliLogger, loader, fileSystem))) - .WithParsed((Action)(options => Exporter.Export(options, cliLogger, loader, fileSystem))) - .WithNotParsed(err => - { - /// System.CommandLine considers --help and --version as NonParsed Errors - /// ref: https://github.com/commandlineparser/commandline/issues/630 - /// This is a workaround to make sure our app exits with exit code 0, - /// when user does --help or --versions. - /// dab --help -> ErrorType.HelpVerbRequestedError - /// dab [command-name] --help -> ErrorType.HelpRequestedError - /// dab --version -> ErrorType.VersionRequestedError - List errors = err.ToList(); - if (errors.Any(e => e.Tag == ErrorType.VersionRequestedError - || e.Tag == ErrorType.HelpRequestedError - || e.Tag == ErrorType.HelpVerbRequestedError)) - { - isHelpOrVersionRequested = true; - } - }); + int result = parser.ParseArguments(args) + .MapResult( + (InitOptions options) => options.Handler(cliLogger, loader, fileSystem), + (AddOptions options) => options.Handler(cliLogger, loader, fileSystem), + (UpdateOptions options) => options.Handler(cliLogger, loader, fileSystem), + (StartOptions options) => options.Handler(cliLogger, loader, fileSystem), + (ValidateOptions options) => options.Handler(cliLogger, loader, fileSystem), + (AddTelemetryOptions options) => options.Handler(cliLogger, loader, fileSystem), + (ExportOptions options) => Exporter.Export(options, cliLogger, loader, fileSystem), + errors => ResultHandler.ProcessErrorsAndReturnExitCode(errors)); - return ((result is Parsed) || (isHelpOrVersionRequested)) ? 0 : -1; + return result; } } } diff --git a/src/Cli/ResultHandler.cs b/src/Cli/ResultHandler.cs new file mode 100644 index 0000000000..179e4ed907 --- /dev/null +++ b/src/Cli/ResultHandler.cs @@ -0,0 +1,42 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Cli.Commands; +using CommandLine; +using NodaTime.Calendars; + +namespace Cli +{ + public class ResultHandler + { + public static int RunInitAndReturnExitCode(InitOptions o) + { + Console.WriteLine("Success"); + int exitCode = 0; + + return exitCode; + } + + public static int ProcessErrorsAndReturnExitCode(IEnumerable err) + { + /// System.CommandLine considers --help and --version as NonParsed Errors + /// ref: https://github.com/commandlineparser/commandline/issues/630 + /// This is a workaround to make sure our app exits with exit code 0, + /// when user does --help or --versions. + /// dab --help -> ErrorType.HelpVerbRequestedError + /// dab [command-name] --help -> ErrorType.HelpRequestedError + /// dab --version -> ErrorType.VersionRequestedError + + bool isHelpOrVersionRequested = false; + List errors = err.ToList(); + if (errors.Any(e => e.Tag == ErrorType.VersionRequestedError + || e.Tag == ErrorType.HelpRequestedError + || e.Tag == ErrorType.HelpVerbRequestedError)) + { + isHelpOrVersionRequested = true; + } + + return isHelpOrVersionRequested ? 0 : -1; + } + } +} From 795b213debd4195eb1c1d4867bde2a037b39c010 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Tue, 5 Mar 2024 13:41:41 -0800 Subject: [PATCH 2/7] Cleanup unused code and function. --- src/Cli/Program.cs | 3 --- src/Cli/ResultHandler.cs | 15 +++------------ 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/src/Cli/Program.cs b/src/Cli/Program.cs index 2b34103911..a0435fb190 100644 --- a/src/Cli/Program.cs +++ b/src/Cli/Program.cs @@ -42,9 +42,6 @@ public static int Main(string[] args) public static int Execute(string[] args, ILogger cliLogger, IFileSystem fileSystem, FileSystemRuntimeConfigLoader loader) { - // To know if `--help` or `--version` was requested. - bool isHelpOrVersionRequested = false; - Parser parser = new(settings => { settings.CaseInsensitiveEnumValues = true; diff --git a/src/Cli/ResultHandler.cs b/src/Cli/ResultHandler.cs index 179e4ed907..7f99d09e5e 100644 --- a/src/Cli/ResultHandler.cs +++ b/src/Cli/ResultHandler.cs @@ -1,24 +1,17 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -using Cli.Commands; using CommandLine; -using NodaTime.Calendars; namespace Cli { public class ResultHandler { - public static int RunInitAndReturnExitCode(InitOptions o) - { - Console.WriteLine("Success"); - int exitCode = 0; - - return exitCode; - } - public static int ProcessErrorsAndReturnExitCode(IEnumerable err) { + // To know if `--help` or `--version` was requested. + bool isHelpOrVersionRequested = false; + /// System.CommandLine considers --help and --version as NonParsed Errors /// ref: https://github.com/commandlineparser/commandline/issues/630 /// This is a workaround to make sure our app exits with exit code 0, @@ -26,8 +19,6 @@ public static int ProcessErrorsAndReturnExitCode(IEnumerable err) /// dab --help -> ErrorType.HelpVerbRequestedError /// dab [command-name] --help -> ErrorType.HelpRequestedError /// dab --version -> ErrorType.VersionRequestedError - - bool isHelpOrVersionRequested = false; List errors = err.ToList(); if (errors.Any(e => e.Tag == ErrorType.VersionRequestedError || e.Tag == ErrorType.HelpRequestedError From e6d0b63beca13de8b600eb15a41583000ab43d21 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Thu, 7 Mar 2024 18:20:42 -0800 Subject: [PATCH 3/7] add tests and comments. --- src/Cli.Tests/EndToEndTests.cs | 36 +++++++++++++------ src/Cli/Commands/InitOptions.cs | 2 +- ...Handler.cs => DabCliParserErrorHandler.cs} | 13 ++++++- src/Cli/Program.cs | 15 ++++++-- 4 files changed, 50 insertions(+), 16 deletions(-) rename src/Cli/{ResultHandler.cs => DabCliParserErrorHandler.cs} (61%) diff --git a/src/Cli.Tests/EndToEndTests.cs b/src/Cli.Tests/EndToEndTests.cs index 143013e3dc..94cf94d880 100644 --- a/src/Cli.Tests/EndToEndTests.cs +++ b/src/Cli.Tests/EndToEndTests.cs @@ -607,20 +607,34 @@ public void TestEngineStartUpWithVerboseAndLogLevelOptions(string logLevelOption } /// - /// Test to verify that `--help` and `--version` along with know command/option produce the exit code 0, - /// while unknown commands/options have exit code -1. + /// Validates that valid usage of verbs and associated options produce exit code 0 indicated success. + /// Verifies that explicitly implemented verbs (add, update, init, start) and appropriately + /// supplied options produce exit code 0. + /// Verifies that non-explicitly implemented DAB CLI options `--help` and `--version` produce exit code 0. + /// init --config "dab-config.MsSql.json" --database-type mssql --connection-string "InvalidConnectionString" /// [DataTestMethod] - [DataRow(new string[] { "--version" }, 0, DisplayName = "Checking version should have exit code 0.")] - [DataRow(new string[] { "--help" }, 0, DisplayName = "Checking commands with help should have exit code 0.")] - [DataRow(new string[] { "add", "--help" }, 0, DisplayName = "Checking options with help should have exit code 0.")] - [DataRow(new string[] { "initialize" }, -1, DisplayName = "Invalid Command should have exit code -1.")] - [DataRow(new string[] { "init", "--database-name", "mssql" }, -1, DisplayName = "Invalid Options should have exit code -1.")] - [DataRow(new string[] { "init", "--database-type", "mssql", "-c", TEST_RUNTIME_CONFIG_FILE }, 0, - DisplayName = "Correct command with correct options should have exit code 0.")] - public void VerifyExitCodeForCli(string[] cliArguments, int expectedErrorCode) + [DataRow(new string[] { "--version" }, DisplayName = "Checking version.")] + [DataRow(new string[] { "--help" }, DisplayName = "Valid verbs with help.")] + [DataRow(new string[] { "add", "--help" }, DisplayName = "Valid options with help.")] + [DataRow(new string[] { "init", "--database-type", "mssql", "-c", TEST_RUNTIME_CONFIG_FILE }, DisplayName = "Valid verb with supported option.")] + public void ValidVerbsAndOptionsReturnZero(string[] cliArguments) { - Assert.AreEqual(expectedErrorCode, Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); + Assert.AreEqual(expected: 0, Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); + } + + /// + /// Validates that invalid verbs and options produce a non-zero exit code. (-1) + /// + /// cli verbs, options, and option values + [DataTestMethod] + [DataRow(new string[] { "--remove-telemetry" }, DisplayName = "Usage of non-existent verb remove-telemetry")] + [DataRow(new string[] { "--initialize" }, DisplayName = "Usage of invalid verb (longform of init not supported) initialize")] + [DataRow(new string[] { "init", "--database-name", "mssql" }, DisplayName = "Invalid init options database-name")] + [DataRow(new string[] { "start", "--config", "nonexistentfile.json" }, DisplayName = "Valid verb with supported option, but value triggers exception.")] + public void InvalidVerbsAndOptionsReturnNonZeroExitCode(string[] cliArguments) + { + Assert.AreEqual(expected: -1, Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); } /// diff --git a/src/Cli/Commands/InitOptions.cs b/src/Cli/Commands/InitOptions.cs index 45da3bcd1f..b949638c1d 100644 --- a/src/Cli/Commands/InitOptions.cs +++ b/src/Cli/Commands/InitOptions.cs @@ -120,7 +120,7 @@ public InitOptions( [Option("rest.request-body-strict", Required = false, HelpText = "(Default: true) Allow extraneous fields in the request body for REST.")] public CliBool RestRequestBodyStrict { get; } - // public int returnCode; + // public int returnCode; public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) { diff --git a/src/Cli/ResultHandler.cs b/src/Cli/DabCliParserErrorHandler.cs similarity index 61% rename from src/Cli/ResultHandler.cs rename to src/Cli/DabCliParserErrorHandler.cs index 7f99d09e5e..0e0f6c8913 100644 --- a/src/Cli/ResultHandler.cs +++ b/src/Cli/DabCliParserErrorHandler.cs @@ -5,8 +5,19 @@ namespace Cli { - public class ResultHandler + /// + /// Processes errors that occur during parsing of CLI verbs (start, init, export, add, update, etc) and their arguments. + /// + public class DabCliParserErrorHandler { + /// + /// Processes errors accumulated by each parser in parser.ParseArguments(). + /// For DAB CLI, this only includes scenarios where the user provides invalid DAB CLI input. + /// e.g. incorrectly formed or missing options and parameters. + /// Additionally, an error is tracked if the user uses an unsupported CLI verb or uses --help. + /// + /// Collection of Error objects collected by the CLI parser. + /// Return code: 0 when --help is used, otherwise -1. public static int ProcessErrorsAndReturnExitCode(IEnumerable err) { // To know if `--help` or `--version` was requested. diff --git a/src/Cli/Program.cs b/src/Cli/Program.cs index a0435fb190..1b9ae47a88 100644 --- a/src/Cli/Program.cs +++ b/src/Cli/Program.cs @@ -26,20 +26,29 @@ public static int Main(string[] args) // Load environment variables from .env file if present. DotNetEnv.Env.Load(); - // Setting up Logger for CLI. + // Logger setup and configuration ILoggerFactory loggerFactory = Utils.LoggerFactoryForCli; - ILogger cliLogger = loggerFactory.CreateLogger(); ILogger configGeneratorLogger = loggerFactory.CreateLogger(); ILogger cliUtilsLogger = loggerFactory.CreateLogger(); ConfigGenerator.SetLoggerForCliConfigGenerator(configGeneratorLogger); Utils.SetCliUtilsLogger(cliUtilsLogger); + + // Sets up the filesystem used for reading and writing runtime configuration files. IFileSystem fileSystem = new FileSystem(); FileSystemRuntimeConfigLoader loader = new(fileSystem); return Execute(args, cliLogger, fileSystem, loader); } + /// + /// Execute the CLI command + /// + /// Command line arguments + /// Logger used as sink for informational and error messages. + /// Filesystem used for reading and writing configuration files, and exporting GraphQL schemas. + /// Loads the runtime config. + /// Exit Code: 0 success, -1 failure public static int Execute(string[] args, ILogger cliLogger, IFileSystem fileSystem, FileSystemRuntimeConfigLoader loader) { Parser parser = new(settings => @@ -58,7 +67,7 @@ public static int Execute(string[] args, ILogger cliLogger, IFileSystem fileSyst (ValidateOptions options) => options.Handler(cliLogger, loader, fileSystem), (AddTelemetryOptions options) => options.Handler(cliLogger, loader, fileSystem), (ExportOptions options) => Exporter.Export(options, cliLogger, loader, fileSystem), - errors => ResultHandler.ProcessErrorsAndReturnExitCode(errors)); + errors => DabCliParserErrorHandler.ProcessErrorsAndReturnExitCode(errors)); return result; } From 56eb9ff6ff9b43e2ccad50de83e5d594fb38fda3 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Thu, 7 Mar 2024 20:13:57 -0800 Subject: [PATCH 4/7] update tests and added cases to cover the int return code enhancements. --- src/Cli.Tests/EndToEndTests.cs | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/Cli.Tests/EndToEndTests.cs b/src/Cli.Tests/EndToEndTests.cs index 94cf94d880..90b0c40f82 100644 --- a/src/Cli.Tests/EndToEndTests.cs +++ b/src/Cli.Tests/EndToEndTests.cs @@ -21,9 +21,10 @@ public class EndToEndTests public void TestInitialize() { MockFileSystem fileSystem = FileSystemUtils.ProvisionMockFileSystem(); - fileSystem.AddFile( - TEST_SCHEMA_FILE, - new MockFileData("")); + // Mock GraphQL Schema File + fileSystem.AddFile(TEST_SCHEMA_FILE, new MockFileData("")); + // Empty runtime config file + fileSystem.AddFile("dab-config-empty.json", new MockFileData("")); _fileSystem = fileSystem; @@ -631,12 +632,26 @@ public void ValidVerbsAndOptionsReturnZero(string[] cliArguments) [DataRow(new string[] { "--remove-telemetry" }, DisplayName = "Usage of non-existent verb remove-telemetry")] [DataRow(new string[] { "--initialize" }, DisplayName = "Usage of invalid verb (longform of init not supported) initialize")] [DataRow(new string[] { "init", "--database-name", "mssql" }, DisplayName = "Invalid init options database-name")] - [DataRow(new string[] { "start", "--config", "nonexistentfile.json" }, DisplayName = "Valid verb with supported option, but value triggers exception.")] public void InvalidVerbsAndOptionsReturnNonZeroExitCode(string[] cliArguments) { Assert.AreEqual(expected: -1, Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); } + /// + /// Usage of valid verbs and options with values triggering exceptions should produce a non-zero exit code. + /// - File read/write issues when reading/writing to the config file. + /// - DAB engine failure. + /// + /// cli verbs, options, and option values + [DataTestMethod] + [DataRow(new string[] { "init", "--config", "dab-config-empty.json", "--database-type", "mssql", "--connection-string", "SampleValue" }, + DisplayName = "Config file value used already exists on the file system and results in init failure.")] + [DataRow(new string[] { "start", "--config", "dab-config-empty.json" }, DisplayName = "Config file value used is empty and engine startup fails")] + public void CliAndEngineFailuresReturnNonZeroExitCode(string[] cliArguments) + { + Assert.AreEqual(expected: -1, Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); + } + /// /// Test to verify that if entity is not specified in the add/update /// command, a custom (more user friendly) message is displayed. From b9dc379fe6e247faa36afb0a6933ac2ae636ac59 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Mon, 11 Mar 2024 17:12:36 -0700 Subject: [PATCH 5/7] add const return codes in new constants folder and remove commented out code. --- src/Cli/Commands/AddOptions.cs | 3 ++- src/Cli/Commands/AddTelemetryOptions.cs | 3 ++- src/Cli/Commands/InitOptions.cs | 7 +++---- src/Cli/Commands/StartOptions.cs | 3 ++- src/Cli/Commands/UpdateOptions.cs | 5 +++-- src/Cli/Commands/ValidateOptions.cs | 3 ++- src/Cli/Constants/CliReturnCode.cs | 17 +++++++++++++++++ 7 files changed, 31 insertions(+), 10 deletions(-) create mode 100644 src/Cli/Constants/CliReturnCode.cs diff --git a/src/Cli/Commands/AddOptions.cs b/src/Cli/Commands/AddOptions.cs index 6d2e799990..edba7197ac 100644 --- a/src/Cli/Commands/AddOptions.cs +++ b/src/Cli/Commands/AddOptions.cs @@ -4,6 +4,7 @@ using System.IO.Abstractions; using Azure.DataApiBuilder.Config; using Azure.DataApiBuilder.Product; +using Cli.Constants; using CommandLine; using Microsoft.Extensions.Logging; using static Cli.Utils; @@ -75,7 +76,7 @@ public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSy logger.LogError("Could not add entity: {Entity} with source: {Source} and permissions: {permissions}.", Entity, Source, string.Join(SEPARATOR, Permissions)); } - return isSuccess ? 0 : -1; + return isSuccess ? CliReturnCode.SUCCESS : CliReturnCode.GENERAL_ERROR; } } } diff --git a/src/Cli/Commands/AddTelemetryOptions.cs b/src/Cli/Commands/AddTelemetryOptions.cs index 2465eb5b5e..7cd3d38be9 100644 --- a/src/Cli/Commands/AddTelemetryOptions.cs +++ b/src/Cli/Commands/AddTelemetryOptions.cs @@ -5,6 +5,7 @@ using Azure.DataApiBuilder.Config; using Azure.DataApiBuilder.Config.ObjectModel; using Azure.DataApiBuilder.Product; +using Cli.Constants; using CommandLine; using Microsoft.Extensions.Logging; using static Cli.Utils; @@ -47,7 +48,7 @@ public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSy logger.LogError("Failed to add telemetry to the configuration file."); } - return isSuccess ? 0 : -1; + return isSuccess ? CliReturnCode.SUCCESS : CliReturnCode.GENERAL_ERROR; } } } diff --git a/src/Cli/Commands/InitOptions.cs b/src/Cli/Commands/InitOptions.cs index b949638c1d..f67cb69b2d 100644 --- a/src/Cli/Commands/InitOptions.cs +++ b/src/Cli/Commands/InitOptions.cs @@ -5,6 +5,7 @@ using Azure.DataApiBuilder.Config; using Azure.DataApiBuilder.Config.ObjectModel; using Azure.DataApiBuilder.Product; +using Cli.Constants; using CommandLine; using Microsoft.Extensions.Logging; using static Cli.Utils; @@ -120,8 +121,6 @@ public InitOptions( [Option("rest.request-body-strict", Required = false, HelpText = "(Default: true) Allow extraneous fields in the request body for REST.")] public CliBool RestRequestBodyStrict { get; } - // public int returnCode; - public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) { logger.LogInformation("{productName} {version}", PRODUCT_NAME, ProductInfo.GetProductVersion()); @@ -130,12 +129,12 @@ public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSy { logger.LogInformation("Config file generated."); logger.LogInformation("SUGGESTION: Use 'dab add [entity-name] [options]' to add new entities in your config."); - return 0; + return CliReturnCode.SUCCESS; } else { logger.LogError("Could not generate config file."); - return -1; + return CliReturnCode.GENERAL_ERROR; } } } diff --git a/src/Cli/Commands/StartOptions.cs b/src/Cli/Commands/StartOptions.cs index 0d9187f219..933b6c77fd 100644 --- a/src/Cli/Commands/StartOptions.cs +++ b/src/Cli/Commands/StartOptions.cs @@ -4,6 +4,7 @@ using System.IO.Abstractions; using Azure.DataApiBuilder.Config; using Azure.DataApiBuilder.Product; +using Cli.Constants; using CommandLine; using Microsoft.Extensions.Logging; using static Cli.Utils; @@ -45,7 +46,7 @@ public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSy logger.LogError("Failed to start the engine."); } - return isSuccess ? 0 : -1; + return isSuccess ? CliReturnCode.SUCCESS : CliReturnCode.GENERAL_ERROR; } } } diff --git a/src/Cli/Commands/UpdateOptions.cs b/src/Cli/Commands/UpdateOptions.cs index 7589bb530b..edaf285484 100644 --- a/src/Cli/Commands/UpdateOptions.cs +++ b/src/Cli/Commands/UpdateOptions.cs @@ -4,6 +4,7 @@ using System.IO.Abstractions; using Azure.DataApiBuilder.Config; using Azure.DataApiBuilder.Product; +using Cli.Constants; using CommandLine; using Microsoft.Extensions.Logging; using static Cli.Utils; @@ -101,7 +102,7 @@ public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSy logger.LogInformation("{productName} {version}", PRODUCT_NAME, ProductInfo.GetProductVersion()); if (!IsEntityProvided(Entity, logger, command: "update")) { - return -1; + return CliReturnCode.GENERAL_ERROR; } bool isSuccess = ConfigGenerator.TryUpdateEntityWithOptions(this, loader, fileSystem); @@ -115,7 +116,7 @@ public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSy logger.LogError("Could not update the entity: {Entity}.", Entity); } - return isSuccess ? 0 : -1; + return isSuccess ? CliReturnCode.SUCCESS : CliReturnCode.GENERAL_ERROR; } } } diff --git a/src/Cli/Commands/ValidateOptions.cs b/src/Cli/Commands/ValidateOptions.cs index 98a811c948..8f2ca124d2 100644 --- a/src/Cli/Commands/ValidateOptions.cs +++ b/src/Cli/Commands/ValidateOptions.cs @@ -4,6 +4,7 @@ using System.IO.Abstractions; using Azure.DataApiBuilder.Config; using Azure.DataApiBuilder.Product; +using Cli.Constants; using CommandLine; using Microsoft.Extensions.Logging; using static Cli.Utils; @@ -38,7 +39,7 @@ public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSy logger.LogError("Config is invalid. Check above logs for details."); } - return isValidConfig ? 0 : -1; + return isValidConfig ? CliReturnCode.SUCCESS : CliReturnCode.GENERAL_ERROR; } } } diff --git a/src/Cli/Constants/CliReturnCode.cs b/src/Cli/Constants/CliReturnCode.cs new file mode 100644 index 0000000000..dad80e8ecb --- /dev/null +++ b/src/Cli/Constants/CliReturnCode.cs @@ -0,0 +1,17 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; + +namespace Cli.Constants +{ + internal class CliReturnCode + { + public const int SUCCESS = 0; + public const int GENERAL_ERROR = -1; + } +} From 73df65f167f1326f4ec52e14c864cefd7ef21a45 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Mon, 11 Mar 2024 17:19:35 -0700 Subject: [PATCH 6/7] Update CLITESTs to utilize new return codes and run dotnet format --- src/Cli.Tests/EndToEndTests.cs | 11 ++++++----- src/Cli/Constants/CliReturnCode.cs | 7 ++----- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/Cli.Tests/EndToEndTests.cs b/src/Cli.Tests/EndToEndTests.cs index 90b0c40f82..f456d9d8e5 100644 --- a/src/Cli.Tests/EndToEndTests.cs +++ b/src/Cli.Tests/EndToEndTests.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using Azure.DataApiBuilder.Product; +using Cli.Constants; using Microsoft.Data.SqlClient; namespace Cli.Tests; @@ -608,7 +609,7 @@ public void TestEngineStartUpWithVerboseAndLogLevelOptions(string logLevelOption } /// - /// Validates that valid usage of verbs and associated options produce exit code 0 indicated success. + /// Validates that valid usage of verbs and associated options produce exit code 0 (CliReturnCode.SUCCESS). /// Verifies that explicitly implemented verbs (add, update, init, start) and appropriately /// supplied options produce exit code 0. /// Verifies that non-explicitly implemented DAB CLI options `--help` and `--version` produce exit code 0. @@ -621,11 +622,11 @@ public void TestEngineStartUpWithVerboseAndLogLevelOptions(string logLevelOption [DataRow(new string[] { "init", "--database-type", "mssql", "-c", TEST_RUNTIME_CONFIG_FILE }, DisplayName = "Valid verb with supported option.")] public void ValidVerbsAndOptionsReturnZero(string[] cliArguments) { - Assert.AreEqual(expected: 0, Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); + Assert.AreEqual(expected: CliReturnCode.SUCCESS, Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); } /// - /// Validates that invalid verbs and options produce a non-zero exit code. (-1) + /// Validates that invalid verbs and options produce exit code -1 (CliReturnCode.GENERAL_ERROR). /// /// cli verbs, options, and option values [DataTestMethod] @@ -634,7 +635,7 @@ public void ValidVerbsAndOptionsReturnZero(string[] cliArguments) [DataRow(new string[] { "init", "--database-name", "mssql" }, DisplayName = "Invalid init options database-name")] public void InvalidVerbsAndOptionsReturnNonZeroExitCode(string[] cliArguments) { - Assert.AreEqual(expected: -1, Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); + Assert.AreEqual(expected: CliReturnCode.GENERAL_ERROR, Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); } /// @@ -649,7 +650,7 @@ public void InvalidVerbsAndOptionsReturnNonZeroExitCode(string[] cliArguments) [DataRow(new string[] { "start", "--config", "dab-config-empty.json" }, DisplayName = "Config file value used is empty and engine startup fails")] public void CliAndEngineFailuresReturnNonZeroExitCode(string[] cliArguments) { - Assert.AreEqual(expected: -1, Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); + Assert.AreEqual(expected: CliReturnCode.GENERAL_ERROR, Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); } /// diff --git a/src/Cli/Constants/CliReturnCode.cs b/src/Cli/Constants/CliReturnCode.cs index dad80e8ecb..0a83ef50a6 100644 --- a/src/Cli/Constants/CliReturnCode.cs +++ b/src/Cli/Constants/CliReturnCode.cs @@ -1,12 +1,9 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -using System; -using System.Collections.Generic; -using System.Linq; -using System.Text; -using System.Threading.Tasks; +using System.Runtime.CompilerServices; +[assembly: InternalsVisibleTo("Cli.Tests")] namespace Cli.Constants { internal class CliReturnCode From c98b88e5829214773a1da0a9903b775f9ae1cfab Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Fri, 29 Mar 2024 08:54:57 -0700 Subject: [PATCH 7/7] update comment for DabCliParserErrorHandler to denote "--version" results in error tracking. also added "actual" named param to assert test cases for consistency since "expected" was included. --- src/Cli.Tests/EndToEndTests.cs | 6 +++--- src/Cli/DabCliParserErrorHandler.cs | 5 ++++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Cli.Tests/EndToEndTests.cs b/src/Cli.Tests/EndToEndTests.cs index f456d9d8e5..807b89cb03 100644 --- a/src/Cli.Tests/EndToEndTests.cs +++ b/src/Cli.Tests/EndToEndTests.cs @@ -622,7 +622,7 @@ public void TestEngineStartUpWithVerboseAndLogLevelOptions(string logLevelOption [DataRow(new string[] { "init", "--database-type", "mssql", "-c", TEST_RUNTIME_CONFIG_FILE }, DisplayName = "Valid verb with supported option.")] public void ValidVerbsAndOptionsReturnZero(string[] cliArguments) { - Assert.AreEqual(expected: CliReturnCode.SUCCESS, Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); + Assert.AreEqual(expected: CliReturnCode.SUCCESS, actual: Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); } /// @@ -635,7 +635,7 @@ public void ValidVerbsAndOptionsReturnZero(string[] cliArguments) [DataRow(new string[] { "init", "--database-name", "mssql" }, DisplayName = "Invalid init options database-name")] public void InvalidVerbsAndOptionsReturnNonZeroExitCode(string[] cliArguments) { - Assert.AreEqual(expected: CliReturnCode.GENERAL_ERROR, Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); + Assert.AreEqual(expected: CliReturnCode.GENERAL_ERROR, actual: Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); } /// @@ -650,7 +650,7 @@ public void InvalidVerbsAndOptionsReturnNonZeroExitCode(string[] cliArguments) [DataRow(new string[] { "start", "--config", "dab-config-empty.json" }, DisplayName = "Config file value used is empty and engine startup fails")] public void CliAndEngineFailuresReturnNonZeroExitCode(string[] cliArguments) { - Assert.AreEqual(expected: CliReturnCode.GENERAL_ERROR, Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); + Assert.AreEqual(expected: CliReturnCode.GENERAL_ERROR, actual: Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!)); } /// diff --git a/src/Cli/DabCliParserErrorHandler.cs b/src/Cli/DabCliParserErrorHandler.cs index 0e0f6c8913..d6b71338b2 100644 --- a/src/Cli/DabCliParserErrorHandler.cs +++ b/src/Cli/DabCliParserErrorHandler.cs @@ -14,7 +14,10 @@ public class DabCliParserErrorHandler /// Processes errors accumulated by each parser in parser.ParseArguments(). /// For DAB CLI, this only includes scenarios where the user provides invalid DAB CLI input. /// e.g. incorrectly formed or missing options and parameters. - /// Additionally, an error is tracked if the user uses an unsupported CLI verb or uses --help. + /// Additionally, an error is tracked if the user uses: + /// -> an unsupported CLI verb + /// -> --help. + /// -> --version /// /// Collection of Error objects collected by the CLI parser. /// Return code: 0 when --help is used, otherwise -1.