CommandHandler.Create on abstract class may not work correctly

When linking a command to an handler with the method CommandHandler.Create(MethodInfo, objet?) if the specified MethodInfo is not implemented in the concrete class but in the inherited class, the invocation will fail.

I have this simplified class structure:

public abstract class MyCommandHandlerBase : ICommandHandler
{
   public abstract Task<int> InvokeAsync(InvocationContext context);
}

public sealed class FirstCommandHandler : MyCommandHandlerBase
{
      public override Task<int> InvokeAsync(InvocationContext context) { }
}

public abstract class MyCommandHandlerBase2 : MyCommandHandlerBase
{
      public override Task<int> InvokeAsync(InvocationContext context) { }
      protected abstract void DoWhatever();
}

public sealed class SecondCommandHandler : MyCommandHandlerBase2 
{
      public override void DoWhatever() { }
}

public sealed class ThirdCommandHandler : MyCommandHandlerBase2 
{
      public override void DoWhatever() { }
}

Command are defined:

 var command = new Command("X")
{
    new Command("A")
    {
        Handler = CommandHandler.Create(typeof(FirstCommandHandler).GetMethod(nameof(ICommandHandler.InvokeAsync))!)
    },
    new Command("B")
    {
        Handler = CommandHandler.Create(typeof(SecondCommandHandler).GetMethod(nameof(ICommandHandler.InvokeAsync))!)
    },
    new Command("c")
    {
        Handler = CommandHandler.Create(typeof(ThirdCommandHandler).GetMethod(nameof(ICommandHandler.InvokeAsync))!)
    }
}

When calling the command "X A", it is working correctly.
When calling the commands "X B" or "X C":

Unhandled exception: System.Reflection.TargetException: Non-static method requires a target.
   at System.Reflection.RuntimeMethodInfo.CheckConsistency(Object target)
   at System.Reflection.RuntimeMethodInfo.InvokeArgumentsCheck(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at System.CommandLine.Invocation.ModelBindingCommandHandler.InvokeAsync(InvocationContext context)
   at System.CommandLine.Invocation.InvocationPipeline.<>c__DisplayClass4_0.<<BuildInvocationChain>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<UseParseErrorReporting>b__21_0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass19_0.<<UseMiddleware>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass16_0.<<UseHelp>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass25_0.<<UseVersionOption>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass23_0.<<UseTypoCorrections>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<UseSuggestDirective>b__22_0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<UseParseDirective>b__20_0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<UseDebugDirective>b__11_0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<RegisterWithDotnetSuggest>b__10_0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass14_0.<<UseExceptionHandler>b__0>d.MoveNext()

As a workaround, we can override the InvokeAsync method in the concrete classes:

public override Task<int> InvokeAsync(InvocationContext context)
{
    return base.InvokeAsync(context);
}

Because there's a performance and complexity cost to doing this sort of runtime search for handler methods, this isn't a change we would make. In the reflection APIs it's reasonable to expect people to choose a non-abstract method as a handler.

In the reflection APIs it's reasonable to expect people to choose a non-abstract method as a handler.

The issue is not an abstract method as a handler, but the handler method is implemeted in a class base, since the handler is a derived class:

    internal class BaseCommand : ICommandHandler
    {
        public virtual string DisplayText => typeof(BaseCommand ).FullName;

        internal static Command Create()
        {
            var command = new Command("test")
            {
                new Command("base")
                {
                    Handler = CommandHandler.Create(typeof(BaseCommand ).GetMethod(nameof(ICommandHandler.InvokeAsync))!)
                },
                new Command("derived")
                {
                    Handler = CommandHandler.Create(typeof(DerivedCommand ).GetMethod(nameof(ICommandHandler.InvokeAsync))!)
                }
            };
            return command;
        }

        public Task<int> InvokeAsync(InvocationContext context)
        {
            Console.WriteLine(DisplayText);
            return Task.FromResult(0);
        }
    }

    internal class DerivedCommand : BaseCommand 
    {
        public override string DisplayText => typeof(DerivedCommand).FullName;
    }

In this case, no absract class or method, but the result would always the same output.

What if:

command-line-api/src/System.CommandLine/Binding/MethodInfoHandlerDescriptor.cs

Line 42 in c41a10c

    
           public override ModelDescriptor Parent => ModelDescriptor.FromType(_handlerMethodInfo.DeclaringType);

Replaced by

  public override ModelDescriptor Parent => ModelDescriptor.FromType(_handlerMethodInfo.ReflectedType);

?

If that change works, we'd accept a PR.

I will take a look at the end of the week.

@ycrumeyrolle

Hi,

I just ran into the same issue. I have a bunch of commands and a lot of shared code between them, naturally, I wanted to extract the functionality to a base class. However, InvokeAsync in the base class is not invoked.

Overriding InvokeAsync in the derived class works - thanks for the workaround!

@ycrumeyrolle did you have a chance to look at this?

Thanks,
Balazs

@ba32107

@ba32107 it should be fixed with the PR #1197

Navigation Menu

Repository navigation

CommandHandler.Create on abstract class may not work correctly #1153

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

CommandHandler.Create on abstract class may not work correctly #1153

Description

Activity

jonsequitur commented on Jan 5, 2021

ycrumeyrolle commented on Jan 5, 2021

jonsequitur commented on Jan 5, 2021

ycrumeyrolle commented on Jan 5, 2021

ba32107 commented on Jun 12, 2021

ycrumeyrolle commented on Jun 12, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions