Add the possibility to register multiple interceptors

[nils-a]

related to #1405
related to #1093

• I have read the Contribution Guidelines
• I have commented on the issue above and discussed the intended changes
• A maintainer has signed off on the changes and the issue was assigned to me
• All newly added code is adequately covered by tests
• All existing tests are still running without errors
• The documentation was modified to reflect the changes OR no documentation changes are required.

Changes

The registration of the interceptor is now handled in ITypeRegistrar this makes it possible to register multiple interceptors and also to use DI in interceptors.
Additionally, an InterceptResult-Method was introduced that runs after the command and can be used to alter the result value or to do some cleanup after the command.

[nils-a]

@spectreconsole/maintainers what do you think?

[rcdailey]

@nils-a I took a look at the code changes. I even pulled down your branch on my machine, did a dotnet pack and dotnet nuget push to a local feed just to be able to use your changes in action.

My overall first impression is positive! The additions to ICommandInterceptor has allowed me to clean up a significant chunk of what I had in my Program.cs, especially regarding the "Hacks" I had to implement.

There's one small gap, however. When an exception occurs in a command class, I need to be able to handle that with access to DI. Unfortunately, the InterceptResult() doesn't seem like it would be called when there's an exception that leaks out of a command class. Instead, that gets handled in CommandApp.RunAsync() where either 1) it gets leaked out to my Program.cs (which is how I currently do it) or 2) gets passed to the provided exception handler, if provided.

Here's the problem I'm dealing with. I use Serilog in my application. I register a factory in DI that I use to create an ILogger instance which gets registered as a singleton. When I handle exceptions in my Program.cs, I do not have access to my underlying ILifetimeScope, so I'm forced to use something like AnsiConsole.WriteException(). But this would not deliver that exception information to other important sinks I've registered with Serilog (e.g. file logs).

When an exception occurs, I'd like to be able to use an instance of ILogger to write the exception information through Serilog, so that it gets delivered to multiple destinations (console included). With the provided exception handler mechanism, this isn't possible since I am not allowed an opportunity to resolve ILogger from my DI container.

One way I can think of to allow for this is to have something like ICommandExceptionHandler with semantics similar to the interceptors that gets resolved (by Spectre.Console) through DI, giving me an opportunity to resolve dependencies in a constructor that I can use to log exception information. An exception object would be passed to a method on this interface that I can implement, which is where my logging would be performed.

If we can reuse ICommandInterceptor for this, that'd be fine too, but given the different levels in the stack trace, I'm not sure if that type is usable at the CommandApp level. Design & solution I leave to you, but I can't completely get rid of my exception handling logic from Program.cs (nor my hack) until I'm able to do this.

If you need additional details or review, please don't hesitate to ask!

[rcdailey]

One small detail I might add:

If you decide to go with a registered type to handle exception processing, we still need the existing exception handler functionality as a fallback. There's a possibility that, due to DI dependencies and order of execution, that you would fail to resolve instances of ICommandExceptionHandler (or whatever you use).

In those cases, the behavior I'd expect is that you silently ignore those failures and treat them as if they are not registered to begin with. You'd proceed to call the configured exception handler (or propagate it, depending on how the user configured things).

This allows me to do AnsiConsole.WriteException() (actually, I'd probably just let spectre handle it) in the configured exception handler as a fallback, and use ILogger in my exception handler subclass.

So basically I think this means:

1. If registered exception handler objects are available and process an exception, we don't go to the command handler fallback
2. If no exception handlers are registered, OR they fail to be constructed, then we use the exception handler as a fallback.

You might even make #1 a user choice similar to how ASP.NET middleware works (user can say they "handled" the exception or not, which would control whether or not the fallback is used).

All this can probably get pretty complicated so again, I am happy to let you work out the details. I just wanted to brain dump some logistical concerns.

Thanks again for everything.

[nils-a]

@rcdailey thank you for the feedback. Regarding exception handling with DI, you might want to have a look at #1411 which should address that.

[rcdailey]

Excellent!! With both of these changes, I am able to get rid of all of my hacks. I cherry picked your other PR into this one, did another local build, and tested out the changes on my end.

This is the Program.cs I started with (before your two changes):

internal static class Program
{
    private static ILifetimeScope? _scope;
    private static IBaseCommandSetupTask[] _tasks = Array.Empty<IBaseCommandSetupTask>();
    private static ILogger? _log;

    [SuppressMessage("Design", "CA1031:Do not catch general exception types")]
    public static int Main(string[] args)
    {
        var builder = new ContainerBuilder();
        CompositionRoot.Setup(builder);

        var logLevelSwitch = new LoggingLevelSwitch();
        var appDataPathProvider = new AppDataPathProvider();
        CompositionRoot.RegisterExternal(builder, logLevelSwitch, appDataPathProvider);

        var app = new CommandApp(new AutofacTypeRegistrar(builder, s => _scope = s));
        app.Configure(config =>
        {
        #if DEBUG
            config.PropagateExceptions();
            config.ValidateExamples();
        #endif

            config.Settings.PropagateExceptions = true;
            config.Settings.StrictParsing = true;

            config.SetApplicationName("recyclarr");
            config.SetApplicationVersion(
                $"v{GitVersionInformation.SemVer} ({GitVersionInformation.FullBuildMetaData})");

            var interceptor = new CliInterceptor(logLevelSwitch, appDataPathProvider);
            interceptor.OnIntercepted.Subscribe(_ => OnAppInitialized());
            config.SetInterceptor(interceptor);

            CliSetup.Commands(config);
        });

        var result = 1;
        try
        {
            result = app.Run(args);
        }
        catch (Exception ex)
        {
            if (_log is null)
            {
                AnsiConsole.WriteException(ex, ExceptionFormats.ShortenEverything);
            }
            else
            {
                _log.Error(ex, "Non-recoverable Exception");
            }
        }
        finally
        {
            OnAppCleanup();
        }

        return result;
    }

    private static void OnAppInitialized()
    {
        if (_scope is null)
        {
            throw new InvalidProgramException("Composition root is not initialized");
        }

        _log = _scope.Resolve<ILogger>();
        _log.Debug("Recyclarr Version: {Version}", GitVersionInformation.InformationalVersion);

        _tasks = _scope.Resolve<IOrderedEnumerable<IBaseCommandSetupTask>>().ToArray();
        _tasks.ForEach(x => x.OnStart());
    }

    private static void OnAppCleanup()
    {
        _tasks.Reverse().ForEach(x => x.OnFinish());
    }
}

And this is where we are after all the cleanup:

internal static class Program
{
    [SuppressMessage("Design", "CA1031:Do not catch general exception types")]
    public static int Main(string[] args)
    {
        var builder = new ContainerBuilder();
        CompositionRoot.Setup(builder);

        var app = new CommandApp(new AutofacTypeRegistrar(builder));
        app.Configure(config =>
        {
        #if DEBUG
            config.PropagateExceptions();
            config.ValidateExamples();
        #endif

            // config.Settings.PropagateExceptions = true;
            config.Settings.StrictParsing = true;

            config.SetApplicationName("recyclarr");
            config.SetApplicationVersion(
                $"v{GitVersionInformation.SemVer} ({GitVersionInformation.FullBuildMetaData})");

            config.SetExceptionHandler((ex, resolver) =>
            {
                var log = (ILogger?) resolver?.Resolve(typeof(ILogger));
                if (log is null)
                {
                    AnsiConsole.WriteException(ex, ExceptionFormats.ShortenEverything);
                }
                else
                {
                    log.Error(ex, "Non-recoverable Exception");
                }
            });

            CliSetup.Commands(config);
        });

        return app.Run(args);
    }
}

I was able to clean up quite a bit. Also notice I no longer needed the hack in my custom type registrar to access the ILifetimeScope. Another nice touch is the SetExceptionHandler(), which I can use now, to grab the ILogger. It would be nice if there was a version of Resolve() that was more statically typed with generics, but not a huge deal.

The other stuff cleaned up nicely as separate interceptor classes:

public class VersionLogInterceptor(ILogger log) : ICommandInterceptor
{
    public void Intercept(CommandContext context, CommandSettings settings)
    {
        log.Debug("Recyclarr Version: {Version}", GitVersionInformation.InformationalVersion);
    }
}

public class GlobalTaskInterceptor(IOrderedEnumerable<IGlobalSetupTask> tasks) : ICommandInterceptor
{
    public void Intercept(CommandContext context, CommandSettings settings)
    {
        tasks.ForEach(x => x.OnStart());
    }

    public void InterceptResult(CommandContext context, CommandSettings settings, ref int result)
    {
        tasks.Reverse().ForEach(x => x.OnFinish());
    }
}

Overall I am pleased with these improvements. Thank you very much for listening to my feedback!