diff --git a/core/Azure.Mcp.Core/src/Commands/Subscription/SubscriptionCommand`2.cs b/core/Azure.Mcp.Core/src/Commands/Subscription/SubscriptionCommand`2.cs index ecffff5981..ae6e31f1ae 100644 --- a/core/Azure.Mcp.Core/src/Commands/Subscription/SubscriptionCommand`2.cs +++ b/core/Azure.Mcp.Core/src/Commands/Subscription/SubscriptionCommand`2.cs @@ -9,15 +9,10 @@ namespace Azure.Mcp.Core.Commands.Subscription; public abstract class SubscriptionCommand< - [DynamicallyAccessedMembers(TrimAnnotations.CommandAnnotations)] TOptions, TResult> + [DynamicallyAccessedMembers(TrimAnnotations.CommandAnnotations)] TOptions, TResult>(ISubscriptionResolver subscriptionResolver) : AuthenticatedCommand where TOptions : class, ISubscriptionOption { - private readonly ISubscriptionResolver _subscriptionResolver; - - protected SubscriptionCommand(ISubscriptionResolver subscriptionResolver) - { - _subscriptionResolver = subscriptionResolver; - } + private readonly ISubscriptionResolver _subscriptionResolver = subscriptionResolver; public override void ValidateOptions(TOptions options, ValidationResult validationResult) { diff --git a/core/Microsoft.Mcp.Core/src/Commands/BaseCommand`2.cs b/core/Microsoft.Mcp.Core/src/Commands/BaseCommand`2.cs index 0306ead348..cafdbecdaa 100644 --- a/core/Microsoft.Mcp.Core/src/Commands/BaseCommand`2.cs +++ b/core/Microsoft.Mcp.Core/src/Commands/BaseCommand`2.cs @@ -71,6 +71,11 @@ public virtual void PostBindOptions(TOptions options) { } + /// + /// Validates the options after they have been bound. + /// + /// The options to validate. + /// The validation result to populate. public virtual void ValidateOptions(TOptions options, ValidationResult validationResult) { } diff --git a/core/Microsoft.Mcp.Core/src/Extensions/CommandResultExtensions.cs b/core/Microsoft.Mcp.Core/src/Extensions/CommandResultExtensions.cs index 7bf0198802..3b7e1f1c25 100644 --- a/core/Microsoft.Mcp.Core/src/Extensions/CommandResultExtensions.cs +++ b/core/Microsoft.Mcp.Core/src/Extensions/CommandResultExtensions.cs @@ -123,6 +123,23 @@ public static bool TryGetValue(this CommandResult commandResult, string optio return GetValueOrDefaultImpl(commandResult, option); } + /// + /// Special version for GetValueOrDefault for new option binding as the Option is immutable once created. + /// + /// The name-based lookup was needed because the static Option would be re-created in RegisterOptions when changing + /// requiredness. CommandResult result retrieval uses the Option instance references as a Dictionary key. So, + /// changing requiredness in RegisterOptions and using the static Option in BindOptions would break silently and + /// necessitated the name based retrieval. That is no longer necessary with the new option binding approach and + /// this more performant approach should be used. + /// + /// + /// The type of the option value + /// The command result + /// The option + /// The value of the option, or the default value if not found or not set + internal static T? GetValueOrDefaultWithoutName(this CommandResult commandResult, Option option) + => GetValueOrDefaultImpl(commandResult, option); + private static T? GetValueOrDefaultImpl(CommandResult commandResult, Option option) { ArgumentNullException.ThrowIfNull(commandResult); diff --git a/core/Microsoft.Mcp.Core/src/Extensions/ParseResultExtensions.cs b/core/Microsoft.Mcp.Core/src/Extensions/ParseResultExtensions.cs index 7a32c3a461..15d7584b0e 100644 --- a/core/Microsoft.Mcp.Core/src/Extensions/ParseResultExtensions.cs +++ b/core/Microsoft.Mcp.Core/src/Extensions/ParseResultExtensions.cs @@ -16,6 +16,23 @@ public static bool TryGetValue(this ParseResult parseResult, string optionNam public static T? GetValueOrDefault(this ParseResult parseResult, Option option) => GetValueOrDefault(parseResult, option.Name); + /// + /// Special version for GetValueOrDefault for new option binding as the Option is immutable once created. + /// + /// The name-based lookup was needed because the static Option would be re-created in RegisterOptions when changing + /// requiredness. ParseResult result retrieval uses the Option instance references as a Dictionary key. So, + /// changing requiredness in RegisterOptions and using the static Option in BindOptions would break silently and + /// necessitated the name based retrieval. That is no longer necessary with the new option binding approach and + /// this more performant approach should be used. + /// + /// + /// The type of the option value + /// The parse result + /// The option + /// The value of the option, or the default value if not found or not set + internal static T? GetValueOrDefaultWithoutName(this ParseResult parseResult, Option option) + => parseResult.CommandResult.GetValueOrDefaultWithoutName(option); + /// /// Gets the value of an option by name, returning default if not found or not set /// diff --git a/core/Microsoft.Mcp.Core/src/Options/EnumOptionValidator.cs b/core/Microsoft.Mcp.Core/src/Options/EnumOptionValidator.cs deleted file mode 100644 index 269963c916..0000000000 --- a/core/Microsoft.Mcp.Core/src/Options/EnumOptionValidator.cs +++ /dev/null @@ -1,36 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -using System.CommandLine.Parsing; - -namespace Microsoft.Mcp.Core.Options; - -/// -/// Provides case-insensitive validation and completion for enum options represented as Option<string>. -/// -internal static class EnumOptionValidator -{ - /// - /// Configures an option with case-insensitive enum validation and completion sources. - /// Mirrors the behavior of AcceptOnlyFromAmong but with case-insensitive matching. - /// - public static void Configure(Option option, Type enumType) - { - var names = Enum.GetNames(enumType); - var allowed = string.Join(", ", names); - var namesSet = new HashSet(names, StringComparer.OrdinalIgnoreCase); - - option.Validators.Add(result => - { - foreach (Token token in result.Tokens) - { - if (!namesSet.Contains(token.Value)) - { - result.AddError($"Argument '{token.Value}' not recognized. Must be one of: {allowed}"); - } - } - }); - - option.CompletionSources.Add(names); - } -} diff --git a/core/Microsoft.Mcp.Core/src/Options/OptionAttribute.cs b/core/Microsoft.Mcp.Core/src/Options/OptionAttribute.cs index e78538d08d..5d96f8d662 100644 --- a/core/Microsoft.Mcp.Core/src/Options/OptionAttribute.cs +++ b/core/Microsoft.Mcp.Core/src/Options/OptionAttribute.cs @@ -40,11 +40,21 @@ public OptionAttribute(string description) /// public string? Name { get; init; } + /// + /// Additional CLI option names (without the "--" prefix). + /// + public string[]? Aliases { get; init; } + /// /// A description of what the option controls. Used in help text and by AI agents. /// public string? Description { get; init; } + /// + /// A default value for the option when a value is not provided. Must match the property type being attributed. + /// + public object? DefaultValue { get; init; } + /// /// Whether the option is hidden from help output. /// diff --git a/core/Microsoft.Mcp.Core/src/Options/OptionBinder.cs b/core/Microsoft.Mcp.Core/src/Options/OptionBinder.cs index 8276ad82c6..6591abeb60 100644 --- a/core/Microsoft.Mcp.Core/src/Options/OptionBinder.cs +++ b/core/Microsoft.Mcp.Core/src/Options/OptionBinder.cs @@ -6,7 +6,6 @@ using System.Net; using System.Reflection; using Microsoft.Mcp.Core.Commands; -using Microsoft.Mcp.Core.Extensions; namespace Microsoft.Mcp.Core.Options; @@ -20,168 +19,77 @@ public static class OptionBinder DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.PublicParameterlessConstructor; - /// - /// To prevent native AOT builds from trimming away the filled generic Options methods, we have to maintain a - /// centralized factory pattern. Each entry provides both the option factory (for registration) and value binder - /// (for parsing). If a type is not in this dictionary and is not an enum, it is unsupported and will be rejected - /// at option registration time. - /// - private static readonly ConcurrentDictionary s_typeHandlers = new() - { - // String - [typeof(string)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(string[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // Boolean - [typeof(bool)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(bool?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(bool[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // Int - [typeof(int)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(int?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(int[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // Long - [typeof(long)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(long?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(long[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // Short - [typeof(short)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(short?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(short[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // Byte - [typeof(byte)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(byte?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(byte[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // SByte - [typeof(sbyte)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(sbyte?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(sbyte[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // UShort - [typeof(ushort)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(ushort?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(ushort[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // UInt - [typeof(uint)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(uint?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(uint[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // ULong - [typeof(ulong)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(ulong?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(ulong[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // Float - [typeof(float)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(float?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(float[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // Double - [typeof(double)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(double?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(double[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // Decimal - [typeof(decimal)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(decimal?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(decimal[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // Char - [typeof(char)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(char?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(char[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // DateTime - [typeof(DateTime)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(DateTime?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(DateTime[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // DateTimeOffset - [typeof(DateTimeOffset)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(DateTimeOffset?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(DateTimeOffset[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // TimeSpan - [typeof(TimeSpan)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(TimeSpan?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(TimeSpan[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - - // Guid - [typeof(Guid)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(Guid?)] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - [typeof(Guid[])] = new(name => new Option(name), (pr, n) => pr.GetValueOrDefault(n)), - }; + private static readonly ConcurrentDictionary s_optionTypeHandlers = new(); /// /// Registers System.CommandLine options on a command based on the public properties of . /// + /// The command to register options on. public static void RegisterOptions<[DynamicallyAccessedMembers(OptionBindingMembers)] TOptions>(Command command) where TOptions : class { - var descriptors = OptionDescriptor.FromType(); - foreach (var descriptor in descriptors) + var handlers = s_optionTypeHandlers.GetOrAdd(typeof(TOptions), _ => GetOptionTypeHandlers()); + foreach (var handler in handlers) { - var option = CreateOption(descriptor); - command.Options.Add(option); + command.Options.Add(handler.Option); } } + private static OptionTypeHandler[] GetOptionTypeHandlers<[DynamicallyAccessedMembers(OptionBindingMembers)] TOptions>() where TOptions : class + { + var descriptors = OptionDescriptor.FromType(); + return [.. descriptors.Select(d => new OptionTypeHandler(d))]; + } + /// /// Creates a new instance and populates its properties /// from the parsed command-line values. /// + /// The parsed command-line values. public static TOptions BindOptions<[DynamicallyAccessedMembers(OptionBindingMembers)] TOptions>(ParseResult parseResult) where TOptions : class { var instance = (TOptions)CreateInstance(typeof(TOptions)); - var descriptors = OptionDescriptor.FromType(); List missingOptions = []; List errors = []; Dictionary? parentInstances = null; + var handlers = s_optionTypeHandlers.GetOrAdd(typeof(TOptions), _ => GetOptionTypeHandlers()); - foreach (var descriptor in descriptors) + foreach (var handler in handlers) { - var optionName = $"--{descriptor.Name}"; - object? value; try { - value = GetOptionValue(parseResult, descriptor.Type, optionName); + value = handler.Binder.Invoke(parseResult); } catch (Exception ex) when (ex is InvalidOperationException or FormatException or OverflowException or ArgumentException) { - errors.Add($"Invalid value for '{optionName}': {ex.Message}"); + errors.Add($"Invalid value for '{handler.Option.Name}': {ex.Message}"); continue; } if (value is null) { - if (descriptor.Required) + if (handler.Option.Required) { - missingOptions.Add(optionName); + missingOptions.Add(handler.Option.Name); } continue; } - if (descriptor.ParentProperty is not null) + if (handler.Descriptor.ParentProperty is not null) { parentInstances ??= []; - if (!parentInstances.TryGetValue(descriptor.ParentProperty, out var parent)) + if (!parentInstances.TryGetValue(handler.Descriptor.ParentProperty, out var parent)) { - parent = CreateInstance(descriptor.ParentProperty.PropertyType); - parentInstances[descriptor.ParentProperty] = parent; + parent = CreateInstance(handler.Descriptor.ParentProperty.PropertyType); + parentInstances[handler.Descriptor.ParentProperty] = parent; } - descriptor.TargetProperty.SetValue(parent, value); + handler.Descriptor.TargetProperty.SetValue(parent, value); } else { - descriptor.TargetProperty.SetValue(instance, value); + handler.Descriptor.TargetProperty.SetValue(instance, value); } } @@ -215,84 +123,6 @@ public static class OptionBinder return instance; } - private static Option CreateOption(OptionDescriptor descriptor) - { - var name = $"--{descriptor.Name}"; - var handler = GetHandler(descriptor.Type); - var option = handler.CreateOption(name); - option.Description = descriptor.Description; - option.Required = descriptor.Required; - option.Hidden = descriptor.Hidden; - - // For array/collection types, allow multiple values after a single option token - // e.g., --modules RedisBloom RedisJSON instead of --modules RedisBloom --modules RedisJSON - if (descriptor.Type.IsArray || (descriptor.Type != typeof(string) && descriptor.Type.IsAssignableTo(typeof(System.Collections.IEnumerable)))) - { - option.Arity = ArgumentArity.OneOrMore; - option.AllowMultipleArgumentsPerToken = true; - } - - return option; - } - - private static object? GetOptionValue(ParseResult parseResult, Type type, string optionName) - { - var handler = GetHandler(type); - return handler.GetValue(parseResult, optionName); - } - - private static OptionTypeHandler GetHandler(Type type) - { - if (s_typeHandlers.TryGetValue(type, out var handler)) - { - return handler; - } - - // Enums (and Nullable): represented as Option with constrained values - Type? underlyingEnum = GetUnderlyingEnumType(type); - if (underlyingEnum is not null) - { - return s_typeHandlers.GetOrAdd(type, _ => new OptionTypeHandler( - name => - { - var option = new Option(name); - EnumOptionValidator.Configure(option, underlyingEnum); - return option; - }, - (pr, n) => - { - var stringValue = pr.GetValueOrDefault(n); - - if (stringValue is null) - { - return null; - } - - return Enum.Parse(underlyingEnum, stringValue, ignoreCase: true); - })); - } - - throw new InvalidOperationException( - $"Unsupported option type '{type}'. Add a handler to s_typeHandlers in OptionBinder, or override RegisterOptions/BindOptions in the command."); - } - - private static Type? GetUnderlyingEnumType(Type type) - { - if (type.IsEnum) - { - return type; - } - - Type? nullable = Nullable.GetUnderlyingType(type); - - if (nullable is not null && nullable.IsEnum) - { - return nullable; - } - - return null; - } - [UnconditionalSuppressMessage("Trimming", "IL2067:UnrecognizedReflectionPattern", Justification = "Nested option types are rooted by the application via property references.")] [UnconditionalSuppressMessage("AOT", "IL3050:RequiresDynamicCode", @@ -302,12 +132,4 @@ private static object CreateInstance(Type type) return Activator.CreateInstance(type) ?? throw new InvalidOperationException($"Failed to create instance of nested options type '{type.Name}'. Ensure it has a public parameterless constructor."); } - - private sealed class OptionTypeHandler( - Func createOption, - Func getValue) - { - public Option CreateOption(string name) => createOption(name); - public object? GetValue(ParseResult parseResult, string optionName) => getValue(parseResult, optionName); - } } diff --git a/core/Microsoft.Mcp.Core/src/Options/OptionDescriptor.cs b/core/Microsoft.Mcp.Core/src/Options/OptionDescriptor.cs index c6708ae4b6..d63ba6b3b6 100644 --- a/core/Microsoft.Mcp.Core/src/Options/OptionDescriptor.cs +++ b/core/Microsoft.Mcp.Core/src/Options/OptionDescriptor.cs @@ -4,16 +4,17 @@ using System.Diagnostics.CodeAnalysis; using System.Reflection; using System.Runtime.CompilerServices; -using Microsoft.Mcp.Core.Commands; namespace Microsoft.Mcp.Core.Options; public class OptionDescriptor { public required string Name { get; init; } - public string? Description { get; init; } + public required string Description { get; init; } + public string[] Aliases { get; init; } = []; public bool Required { get; init; } public bool Hidden { get; init; } + public object? DefaultValue { get; init; } public required PropertyInfo TargetProperty { get; init; } public required Type Type { get; init; } public PropertyInfo? ParentProperty { get; init; } @@ -58,6 +59,7 @@ private static void CollectDescriptors(Type type, string? prefix, List