Skip to content

Commit

Permalink
Update DAB CLI to return appropriate exit codes based upon exit reason (
Browse files Browse the repository at this point in the history
#2084)

## Why make this change?

- Closes #1777
- Certain failures that occurred within CLI execution resulted in the
CLI returning exit code 0 for success, even though there were failures.
- For example, `dab start {options}` may contain fully valid values, but
when used as arguments to start the DAB engine process within CLI,
engine startup fails. The failure didn't get reflected as error code -1.
- For example, `dab init {options}` may reference a file that already
exists in the path that the CLI uses to read/write config files, this
results in an error that is not properly reflected in the exit code.

## What is this change?

- Any failures that occur in CLI usage will result in the proper exit
codes:
    - `0` success
    - `-1` failure
- The change updates the usage of CommandLineParser. Instead of
swallowing exceptions and the status of `isSuccess` within the
`options.Handler()` methods, the handler methods all return the int
error code now.
- e.g. the following now captures the return codes after command line
parsing.
```csharp
parser.ParseArguments<InitOptions, AddOptions, UpdateOptions, StartOptions, ValidateOptions, ExportOptions, AddTelemetryOptions>(args)
                .MapResult(
```

## How was this tested?

- [x] Integration Tests
  • Loading branch information
seantleonard committed Mar 29, 2024
1 parent dbcb651 commit 0fbf7cf
Show file tree
Hide file tree
Showing 11 changed files with 159 additions and 56 deletions.
58 changes: 44 additions & 14 deletions src/Cli.Tests/EndToEndTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

using Azure.DataApiBuilder.Product;
using Cli.Constants;
using Microsoft.Data.SqlClient;

namespace Cli.Tests;
Expand All @@ -21,9 +22,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;

Expand Down Expand Up @@ -691,20 +693,48 @@ public void TestEngineStartUpWithVerboseAndLogLevelOptions(string logLevelOption
}

/// <summary>
/// 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 (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.
/// init --config "dab-config.MsSql.json" --database-type mssql --connection-string "InvalidConnectionString"
/// </summary>
[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: CliReturnCode.SUCCESS, actual: Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!));
}

/// <summary>
/// Validates that invalid verbs and options produce exit code -1 (CliReturnCode.GENERAL_ERROR).
/// </summary>
/// <param name="cliArguments">cli verbs, options, and option values</param>
[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")]
public void InvalidVerbsAndOptionsReturnNonZeroExitCode(string[] cliArguments)
{
Assert.AreEqual(expected: CliReturnCode.GENERAL_ERROR, actual: Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!));
}

/// <summary>
/// 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.
/// </summary>
/// <param name="cliArguments">cli verbs, options, and option values</param>
[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: CliReturnCode.GENERAL_ERROR, actual: Program.Execute(cliArguments, _cliLogger!, _fileSystem!, _runtimeConfigLoader!));
}

/// <summary>
Expand Down
7 changes: 5 additions & 2 deletions src/Cli/Commands/AddOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -56,12 +57,12 @@ public class AddOptions : EntityOptions
[Option("permissions", Required = true, Separator = ':', HelpText = "Permissions required to access the source table or container.")]
public IEnumerable<string> 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);
Expand All @@ -74,6 +75,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 ? CliReturnCode.SUCCESS : CliReturnCode.GENERAL_ERROR;
}
}
}
5 changes: 4 additions & 1 deletion src/Cli/Commands/AddTelemetryOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -32,7 +33,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());

Expand All @@ -46,6 +47,8 @@ public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileS
{
logger.LogError("Failed to add telemetry to the configuration file.");
}

return isSuccess ? CliReturnCode.SUCCESS : CliReturnCode.GENERAL_ERROR;
}
}
}
5 changes: 4 additions & 1 deletion src/Cli/Commands/InitOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -125,18 +126,20 @@ public class InitOptions : Options
[Option("graphql.multiple-create.enabled", Required = false, HelpText = "(Default: false) Enables multiple create operation for GraphQL. Supported values: true, false.")]
public CliBool MultipleCreateOperationEnabled { 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.TryGenerateConfig(this, loader, fileSystem);
if (isSuccess)
{
logger.LogInformation("Config file generated.");
logger.LogInformation("SUGGESTION: Use 'dab add [entity-name] [options]' to add new entities in your config.");
return CliReturnCode.SUCCESS;
}
else
{
logger.LogError("Could not generate config file.");
return CliReturnCode.GENERAL_ERROR;
}
}
}
Expand Down
5 changes: 4 additions & 1 deletion src/Cli/Commands/StartOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -37,7 +38,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);
Expand All @@ -46,6 +47,8 @@ public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileS
{
logger.LogError("Failed to start the engine.");
}

return isSuccess ? CliReturnCode.SUCCESS : CliReturnCode.GENERAL_ERROR;
}
}
}
7 changes: 5 additions & 2 deletions src/Cli/Commands/UpdateOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -96,12 +97,12 @@ public class UpdateOptions : EntityOptions
[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<string>? 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 CliReturnCode.GENERAL_ERROR;
}

bool isSuccess = ConfigGenerator.TryUpdateEntityWithOptions(this, loader, fileSystem);
Expand All @@ -114,6 +115,8 @@ public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileS
{
logger.LogError("Could not update the entity: {Entity}.", Entity);
}

return isSuccess ? CliReturnCode.SUCCESS : CliReturnCode.GENERAL_ERROR;
}
}
}
5 changes: 4 additions & 1 deletion src/Cli/Commands/ValidateOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -24,7 +25,7 @@ public ValidateOptions(string config)
/// This Handler method is responsible for validating the config file and is called when `validate`
/// command is invoked.
/// </summary>
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);
Expand All @@ -37,6 +38,8 @@ public void Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileS
{
logger.LogError("Config is invalid. Check above logs for details.");
}

return isValidConfig ? CliReturnCode.SUCCESS : CliReturnCode.GENERAL_ERROR;
}
}
}
14 changes: 14 additions & 0 deletions src/Cli/Constants/CliReturnCode.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System.Runtime.CompilerServices;

[assembly: InternalsVisibleTo("Cli.Tests")]
namespace Cli.Constants
{
internal class CliReturnCode
{
public const int SUCCESS = 0;
public const int GENERAL_ERROR = -1;
}
}
47 changes: 47 additions & 0 deletions src/Cli/DabCliParserErrorHandler.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using CommandLine;

namespace Cli
{
/// <summary>
/// Processes errors that occur during parsing of CLI verbs (start, init, export, add, update, etc) and their arguments.
/// </summary>
public class DabCliParserErrorHandler
{
/// <summary>
/// Processes errors accumulated by each parser in parser.ParseArguments<parsers>().
/// 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
/// -> --help.
/// -> --version
/// </summary>
/// <param name="err">Collection of Error objects collected by the CLI parser.</param>
/// <returns>Return code: 0 when --help is used, otherwise -1.</returns>
public static int ProcessErrorsAndReturnExitCode(IEnumerable<Error> 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,
/// when user does --help or --versions.
/// dab --help -> ErrorType.HelpVerbRequestedError
/// dab [command-name] --help -> ErrorType.HelpRequestedError
/// dab --version -> ErrorType.VersionRequestedError
List<Error> 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;
}
}
}
9 changes: 6 additions & 3 deletions src/Cli/Exporter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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!);

Expand All @@ -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(
Expand All @@ -32,14 +32,15 @@ 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(() =>
{
_ = ConfigGenerator.TryStartEngineWithOptions(startOptions, loader, fileSystem);
}, cancellationToken);

bool isSuccess = false;
if (options.GraphQL)
{
int retryCount = 5;
Expand All @@ -50,6 +51,7 @@ public static void Export(ExportOptions options, ILogger logger, FileSystemRunti
try
{
ExportGraphQL(options, runtimeConfig, fileSystem);
isSuccess = true;
break;
}
catch
Expand All @@ -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)
Expand Down

0 comments on commit 0fbf7cf

Please sign in to comment.