From fa703a987dcb736a737b63343022169ec103a794 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Mon, 18 May 2020 15:36:13 -0700 Subject: [PATCH 01/12] Sync to upstream --- src/System.CommandLine/Option.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.CommandLine/Option.cs b/src/System.CommandLine/Option.cs index 75a6910208..74efa6be3c 100644 --- a/src/System.CommandLine/Option.cs +++ b/src/System.CommandLine/Option.cs @@ -45,7 +45,7 @@ public virtual Argument Argument IArgument IOption.Argument => Argument; public bool Required { get; set; } - + string IValueDescriptor.ValueName => Name; Type IValueDescriptor.ValueType => Argument.ArgumentType; From 0b64118facb6580f8e7b832efa3458b4d07e00d5 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Wed, 5 Aug 2020 07:23:04 -0700 Subject: [PATCH 02/12] Prior to GetValue refactoring --- .../ModelBindingCommandHandlerTests.cs | 50 ++++++++++++ .../Invocation/InvocationExtensions.cs | 26 ++++++ .../Invocation/ModelBindingCommandHandler.cs | 80 +++++++++++++++++-- 3 files changed, 149 insertions(+), 7 deletions(-) create mode 100644 src/System.CommandLine/Invocation/InvocationExtensions.cs diff --git a/src/System.CommandLine.Tests/Binding/ModelBindingCommandHandlerTests.cs b/src/System.CommandLine.Tests/Binding/ModelBindingCommandHandlerTests.cs index 1d7d239114..b0b09359f4 100644 --- a/src/System.CommandLine.Tests/Binding/ModelBindingCommandHandlerTests.cs +++ b/src/System.CommandLine.Tests/Binding/ModelBindingCommandHandlerTests.cs @@ -458,6 +458,56 @@ public async Task Handler_method_receives_command_arguments_bound_to_the_specifi c.AssertBoundValue(boundValue); } + [Theory] + [InlineData(typeof(ClassWithCtorParameter))] + [InlineData(typeof(ClassWithSetter))] + [InlineData(typeof(ClassWithCtorParameter))] + [InlineData(typeof(ClassWithSetter))] + [InlineData(typeof(FileInfo))] + [InlineData(typeof(FileInfo[]))] + [InlineData(typeof(string[]))] + [InlineData(typeof(List))] + [InlineData(typeof(int[]))] + [InlineData(typeof(List))] + public async Task Handler_method_receives_command_arguments_explicitly_bound_to_the_specified_type( + Type type) + { + var c = _bindingCases[type]; + + var captureMethod = GetType() + .GetMethod(nameof(CaptureMethod), BindingFlags.NonPublic | BindingFlags.Static) + .MakeGenericMethod(c.ParameterType); + var parameter = captureMethod.GetParameters().First(); + + var handler = CommandHandler.Create(captureMethod); + + var argument = new Argument + { + Name = "value", + ArgumentType = c.ParameterType + }; + + var command = new Command( + "command") + { + argument + }; + handler.BindParameter(parameter, argument); + command.Handler = handler; + + var parseResult = command.Parse(c.CommandLine); + + var invocationContext = new InvocationContext(parseResult); + + await handler.InvokeAsync(invocationContext); + + var boundValue = ((BoundValueCapturer)invocationContext.InvocationResult).BoundValue; + + boundValue.Should().BeOfType(c.ParameterType); + + c.AssertBoundValue(boundValue); + } + private static void CaptureMethod(T value, InvocationContext invocationContext) { invocationContext.InvocationResult = new BoundValueCapturer(value); diff --git a/src/System.CommandLine/Invocation/InvocationExtensions.cs b/src/System.CommandLine/Invocation/InvocationExtensions.cs new file mode 100644 index 0000000000..7d267b2c93 --- /dev/null +++ b/src/System.CommandLine/Invocation/InvocationExtensions.cs @@ -0,0 +1,26 @@ +using System.Reflection; + +namespace System.CommandLine.Invocation +{ + public static class InvocationExtensions + { + public static void BindParameter(this ICommandHandler handler, ParameterInfo param, Option option) + { + // check for nulls + if (!(handler is ModelBindingCommandHandler bindingHandler)) + { + throw new InvalidOperationException("Cannot bind to this type of handler"); + } + bindingHandler.BindParameter(param, option); + } + + public static void BindParameter(this ICommandHandler handler, ParameterInfo param, Argument argument) + { + if (!(handler is ModelBindingCommandHandler bindingHandler)) + { + throw new InvalidOperationException("Cannot bind to this type of handler"); + } + bindingHandler.BindParameter(param, argument); + } + } +} diff --git a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs index b037b26243..11da478040 100644 --- a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs +++ b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs @@ -17,6 +17,8 @@ internal class ModelBindingCommandHandler : ICommandHandler private readonly ModelBinder? _invocationTargetBinder; private readonly MethodInfo? _handlerMethodInfo; private readonly IReadOnlyList _parameterDescriptors; + private Dictionary _invokeArgumentBindingSources { get; } = + new Dictionary(); public ModelBindingCommandHandler( MethodInfo handlerMethodInfo, @@ -51,14 +53,22 @@ public async Task InvokeAsync(InvocationContext context) { var bindingContext = context.BindingContext; - var parameterBinders = _parameterDescriptors - .Select(p => bindingContext.GetModelBinder(p)) - .ToList(); + var invocationArguments = new object?[_parameterDescriptors.Count()]; + var length = _parameterDescriptors.Count(); - var invocationArguments = - parameterBinders - .Select(binder => binder.CreateInstance(bindingContext)) - .ToArray(); + for (int i = 0; i < length; i++) + { + var paramDesc = _parameterDescriptors[i]; + if (_invokeArgumentBindingSources.TryGetValue(paramDesc, out var valueSource)) + { + invocationArguments[i] = ValueFromValueSource(paramDesc, valueSource, bindingContext); + } + else + { + var binder = bindingContext.GetModelBinder(paramDesc); + invocationArguments[i] = binder.CreateInstance(bindingContext); + } + } var invocationTarget = _invocationTarget ?? _invocationTargetBinder?.CreateInstance(bindingContext); @@ -77,5 +87,61 @@ public async Task InvokeAsync(InvocationContext context) return await CommandHandler.GetResultCodeAsync(result, context); } + + private object? ValueFromValueSource(ParameterDescriptor paramDesc, IValueSource valueSource, BindingContext bindingContext) + { + BoundValue? boundValue; + if (valueSource is null) + { + // If there is no source to bind from, no value can be bound. + return null; + } + if (bindingContext.TryBindToScalarValue( + paramDesc, + valueSource, + out boundValue)) + { + // boundValue has been set + } + else if ( paramDesc.HasDefaultValue) + { + boundValue = BoundValue.DefaultForValueDescriptor(paramDesc); + } + if (!(boundValue is null)) + { + return boundValue.Value; + } + var parameterBinder = bindingContext.GetModelBinder(paramDesc); + return parameterBinder.CreateInstance(bindingContext); + } + + public void BindParameter(ParameterInfo param, Argument argument) + { + var _ = argument ?? throw new InvalidOperationException("You must specify an argument to bind"); + BindValueSource(param, new SpecificSymbolValueSource(argument)); + } + + public void BindParameter(ParameterInfo param, Option option) + { + var _ = option ?? throw new InvalidOperationException("You must specify an argument to bind"); + BindValueSource(param, new SpecificSymbolValueSource(option)); + } + + private void BindValueSource(ParameterInfo param, IValueSource valueSource) + { + var paramDesc = FindParameterDescriptor(param); + if (paramDesc is null) + { + throw new InvalidOperationException("You must bind to a parameter on this handler"); + } + _invokeArgumentBindingSources.Add(paramDesc, valueSource); + } + + private ParameterDescriptor? FindParameterDescriptor(ParameterInfo? param) + => param == null + ? null + : _parameterDescriptors + .FirstOrDefault(x => x.ValueName == param.Name && + x.ValueType == param.ParameterType); } } \ No newline at end of file From 4294e94a1d7f03bf137c4be2a5b318de5941d1d5 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Wed, 5 Aug 2020 09:43:21 -0700 Subject: [PATCH 03/12] Refactor GetBoundValue --- src/System.CommandLine/Binding/ModelBinder.cs | 41 +++++++++++-------- .../Invocation/ModelBindingCommandHandler.cs | 41 ++++--------------- 2 files changed, 32 insertions(+), 50 deletions(-) diff --git a/src/System.CommandLine/Binding/ModelBinder.cs b/src/System.CommandLine/Binding/ModelBinder.cs index 60bd9f29e6..96319bb21c 100644 --- a/src/System.CommandLine/Binding/ModelBinder.cs +++ b/src/System.CommandLine/Binding/ModelBinder.cs @@ -84,13 +84,13 @@ public void BindConstructorArgumentFromValue(ParameterInfo parameter, if (ctorDesc is null) throw new ArgumentException(paramName: nameof(parameter), message: "Parameter is not described by any of the model constructor descriptors."); - + var paramDesc = ctorDesc.ParameterDescriptors[parameter.Position]; ConstructorArgumentBindingSources[paramDesc] = new SpecificSymbolValueSource(valueDescriptor); } - public void BindMemberFromValue(PropertyInfo property, + public void BindMemberFromValue(PropertyInfo property, IValueDescriptor valueDescriptor) { var propertyDescriptor = FindModelPropertyDescriptor( @@ -108,9 +108,9 @@ public void BindMemberFromValue(PropertyInfo property, var values = GetValues( // No binding sources, as were are attempting to bind a value // for the model itself, not for its ctor args or its members. - bindingSources: null, - bindingContext: context, - new[] { ValueDescriptor }, + bindingSources: null, + bindingContext: context, + new[] { ValueDescriptor }, includeMissingValues: false); if (values.Count == 1 && @@ -140,7 +140,7 @@ private bool TryDefaultConstructorAndPropertiesStrategy( { var boundConstructorArguments = GetValues( ConstructorArgumentBindingSources, - context, + context, constructor.ParameterDescriptors, true); @@ -201,14 +201,7 @@ private IReadOnlyList GetValues( var valueSource = GetValueSource(bindingSources, bindingContext, valueDescriptor); - BoundValue? boundValue; - if (!bindingContext.TryBindToScalarValue( - valueDescriptor, - valueSource, - out boundValue) && valueDescriptor.HasDefaultValue) - { - boundValue = BoundValue.DefaultForValueDescriptor(valueDescriptor); - } + BoundValue? boundValue = GetBoundValue(valueSource, bindingContext, valueDescriptor); if (boundValue is null) { @@ -219,13 +212,12 @@ private IReadOnlyList GetValues( { if (parameterDescriptor.HasDefaultValue) boundValue = BoundValue.DefaultForValueDescriptor(parameterDescriptor); - else if (parameterDescriptor.AllowsNull && + else if (parameterDescriptor.AllowsNull && ShouldPassNullToConstructor(constructorDescriptor.Parent, constructorDescriptor)) boundValue = BoundValue.DefaultForType(valueDescriptor); } } } - if (boundValue != null) { values.Add(boundValue); @@ -235,6 +227,21 @@ private IReadOnlyList GetValues( return values; } + internal static BoundValue? GetBoundValue(IValueSource valueSource, BindingContext bindingContext, + IValueDescriptor valueDescriptor) + { + BoundValue? boundValue; + if (!bindingContext.TryBindToScalarValue( + valueDescriptor, + valueSource, + out boundValue) && valueDescriptor.HasDefaultValue) + { + boundValue = BoundValue.DefaultForValueDescriptor(valueDescriptor); + } + + return boundValue; + } + private IValueSource GetValueSource( IDictionary? bindingSources, BindingContext bindingContext, @@ -264,7 +271,7 @@ private IValueSource GetValueSource( public override string ToString() => $"{ModelDescriptor.ModelType.Name}"; - private bool ShouldPassNullToConstructor(ModelDescriptor modelDescriptor, + private static bool ShouldPassNullToConstructor(ModelDescriptor modelDescriptor, ConstructorDescriptor? ctor = null) { if (!(ctor is null)) diff --git a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs index 11da478040..a81f251049 100644 --- a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs +++ b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs @@ -61,13 +61,15 @@ public async Task InvokeAsync(InvocationContext context) var paramDesc = _parameterDescriptors[i]; if (_invokeArgumentBindingSources.TryGetValue(paramDesc, out var valueSource)) { - invocationArguments[i] = ValueFromValueSource(paramDesc, valueSource, bindingContext); - } - else - { - var binder = bindingContext.GetModelBinder(paramDesc); - invocationArguments[i] = binder.CreateInstance(bindingContext); + var boundValue = ModelBinder.GetBoundValue(valueSource, bindingContext, paramDesc); + if (!(boundValue is null)) + { + invocationArguments[i] = boundValue.Value; + continue; + } } + var binder = bindingContext.GetModelBinder(paramDesc); + invocationArguments[i] = binder.CreateInstance(bindingContext); } var invocationTarget = _invocationTarget ?? @@ -88,33 +90,6 @@ public async Task InvokeAsync(InvocationContext context) return await CommandHandler.GetResultCodeAsync(result, context); } - private object? ValueFromValueSource(ParameterDescriptor paramDesc, IValueSource valueSource, BindingContext bindingContext) - { - BoundValue? boundValue; - if (valueSource is null) - { - // If there is no source to bind from, no value can be bound. - return null; - } - if (bindingContext.TryBindToScalarValue( - paramDesc, - valueSource, - out boundValue)) - { - // boundValue has been set - } - else if ( paramDesc.HasDefaultValue) - { - boundValue = BoundValue.DefaultForValueDescriptor(paramDesc); - } - if (!(boundValue is null)) - { - return boundValue.Value; - } - var parameterBinder = bindingContext.GetModelBinder(paramDesc); - return parameterBinder.CreateInstance(bindingContext); - } - public void BindParameter(ParameterInfo param, Argument argument) { var _ = argument ?? throw new InvalidOperationException("You must specify an argument to bind"); From 87b5bda63d626178cebba0f54e5da1ad18ce9665 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Fri, 7 Aug 2020 07:57:35 -0700 Subject: [PATCH 04/12] WIP --- .../Binding/ModelBinderConstructorTests.cs | 143 ++++++++ .../Binding/ModelBinderTests.cs | 30 +- .../ModelBindingCommandHandlerTests.cs | 18 +- src/System.CommandLine/Binding/ModelBinder.cs | 47 ++- .../Binding/ModelBinder2.cs | 338 ++++++++++++++++++ .../Binding/ParameterDescriptor.cs | 16 +- .../Binding/ServiceProviderValueSource.cs | 8 +- .../Binding/SpecificSymbolValueSource.cs | 4 +- .../Invocation/ModelBindingCommandHandler.cs | 11 +- 9 files changed, 562 insertions(+), 53 deletions(-) create mode 100644 src/System.CommandLine.Tests/Binding/ModelBinderConstructorTests.cs create mode 100644 src/System.CommandLine/Binding/ModelBinder2.cs diff --git a/src/System.CommandLine.Tests/Binding/ModelBinderConstructorTests.cs b/src/System.CommandLine.Tests/Binding/ModelBinderConstructorTests.cs new file mode 100644 index 0000000000..0a088f97a9 --- /dev/null +++ b/src/System.CommandLine.Tests/Binding/ModelBinderConstructorTests.cs @@ -0,0 +1,143 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System.Collections.Generic; +using System.CommandLine.Binding; +using System.CommandLine.Invocation; +using System.CommandLine.IO; +using System.IO; +using System.Linq; +using System.Reflection; +using System.Threading.Tasks; +using FluentAssertions; +using FluentAssertions.Execution; +using Xunit; +using common = System.CommandLine.Tests.Binding.ModelBindingCommandHandlerTests; + +namespace System.CommandLine.Tests.Binding +{ + public class ModelBinderConstructorTests + { + + [Theory] + //[InlineData(typeof(ClassWithCtorParameter))] + [InlineData(typeof(ClassWithSetter))] + //[InlineData(typeof(ClassWithCtorParameter))] + [InlineData(typeof(ClassWithSetter))] + [InlineData(typeof(FileInfo))] + [InlineData(typeof(FileInfo[]))] + [InlineData(typeof(string[]))] + [InlineData(typeof(List))] + [InlineData(typeof(int[]))] + [InlineData(typeof(List))] + public async Task Handler_constructor_receives_option_arguments_bound_to_the_specified_type( + Type type) + { + var testCase = common.BindingCases[type]; + ICommandHandler handler = CommandHandler.Create( + MakeGenericType(typeof(ClassForCaptureMethod<>), testCase.ParameterType) + .GetMethod(nameof(ClassForCaptureMethod.Invoke))); + Command command = GetSingleArgumentCommand(testCase); + command.Handler = handler; + + var parseResult = command.Parse($"--value {testCase.CommandLine}"); + var invocationContext = new InvocationContext(parseResult); + await handler.InvokeAsync(invocationContext); + + var boundValue = ((BoundValueCapturer)invocationContext.InvocationResult).BoundValue; + boundValue.Should().BeAssignableTo(testCase.ParameterType); + testCase.AssertBoundValue(boundValue); + } + + [Theory] + //[InlineData(typeof(ClassWithCtorParameter))] + //[InlineData(typeof(ClassWithSetter))] + //[InlineData(typeof(ClassWithCtorParameter))] + //[InlineData(typeof(ClassWithSetter))] + [InlineData(typeof(FileInfo))] + [InlineData(typeof(FileInfo[]))] + [InlineData(typeof(string[]))] + [InlineData(typeof(List))] + [InlineData(typeof(int[]))] + [InlineData(typeof(List))] + public async Task Constructor_receives_option_arguments_bound_to_the_specified_type( + Type type) + { + var testCase = common.BindingCases[type]; + var typeToCreate = MakeGenericType(typeof(ClassForCreate<>), testCase.ParameterType); + Command command = GetSingleArgumentCommand(testCase); + + var binder = new ModelBinder(typeToCreate); + var commandLine = $"--value {testCase.CommandLine}"; + var bindingContext = new BindingContext(command.Parse(commandLine)); + var instance = binder.CreateInstance(bindingContext) as ClassForCreateBase; + + instance.Value.Should().BeAssignableTo(testCase.ParameterType); + testCase.AssertBoundValue(instance.Value); + } + + private static Command GetSingleArgumentCommand(BindingTestCase testCase) + { + return new Command("command") + { + new Option("--value") + { + Argument = new Argument + { + ArgumentType = testCase.ParameterType + } + } + }; + } + + private MethodInfo MakeGenericMethod(Type type, string methodName, params Type[] typeParameters) + => type.GetMethod(methodName, BindingFlags.Public | BindingFlags.Instance) + .MakeGenericMethod(typeParameters); + + private Type MakeGenericType(Type openType, params Type[] typeParameters) + => openType.MakeGenericType(typeParameters); + + private static void CaptureMethod(T value, InvocationContext invocationContext) + { + invocationContext.InvocationResult = new BoundValueCapturer(value); + } + + private class BoundValueCapturer : IInvocationResult + { + public BoundValueCapturer(object boundValue) + { + BoundValue = boundValue; + } + + public object BoundValue { get; } + + public void Apply(InvocationContext context) + { + } + } + + private class ClassForCaptureMethod + { + public ClassForCaptureMethod(T value, InvocationContext invocationContext) + { + invocationContext.InvocationResult = new BoundValueCapturer(value); + } + + public void Invoke() { } + } + + private class ClassForCreateBase + { + public object? Value { get; protected set; } + + } + private class ClassForCreate : ClassForCreateBase + { + public ClassForCreate(T value) + { + Value = value; + } + + } + } +} diff --git a/src/System.CommandLine.Tests/Binding/ModelBinderTests.cs b/src/System.CommandLine.Tests/Binding/ModelBinderTests.cs index 3e9c84a6d9..f09a941c77 100644 --- a/src/System.CommandLine.Tests/Binding/ModelBinderTests.cs +++ b/src/System.CommandLine.Tests/Binding/ModelBinderTests.cs @@ -79,6 +79,34 @@ public void Command_arguments_are_bound_by_name_to_constructor_parameters( valueReceivedValue.Should().Be(expectedValue); } + [Theory] + [InlineData(typeof(FileInfo), "MyFile.cs")] + public void Command_arguments_are_bound_by_name_to_complex_constructor_parameters( + Type type, + string commandLine) + { + var targetType = typeof(ClassWithCtorParameter<>).MakeGenericType(type); + var binder = new ModelBinder(targetType); + + var command = new Command("the-command") + { + new Argument + { + Name = "value", + ArgumentType = type + } + }; + + var bindingContext = new BindingContext(command.Parse(commandLine)); + + var instance = binder.CreateInstance(bindingContext); + + object valueReceivedValue = ((dynamic)instance).Value; + var expectedValue = new FileInfo(commandLine); + + valueReceivedValue.Should().BeEquivalentTo(expectedValue); + } + [Fact] public void Explicitly_configured_default_values_can_be_bound_by_name_to_constructor_parameters() { @@ -334,7 +362,7 @@ public void Values_from_parent_command_arguments_are_bound_by_name_by_default() instance.IntOption.Should().Be(123); } - + [Fact] public void Default_values_from_parent_command_arguments_are_bound_by_name_by_default() { diff --git a/src/System.CommandLine.Tests/Binding/ModelBindingCommandHandlerTests.cs b/src/System.CommandLine.Tests/Binding/ModelBindingCommandHandlerTests.cs index b0b09359f4..1368ad302b 100644 --- a/src/System.CommandLine.Tests/Binding/ModelBindingCommandHandlerTests.cs +++ b/src/System.CommandLine.Tests/Binding/ModelBindingCommandHandlerTests.cs @@ -337,7 +337,7 @@ public async Task Handler_method_receives_option_arguments_bound_to_the_specifie bool useDelegate, string variation = null) { - var testCase = _bindingCases[(type, variation)]; + var testCase = BindingCases[(type, variation)]; ICommandHandler handler; if (!useDelegate) @@ -426,7 +426,7 @@ public async Task When_binding_fails_due_to_parameter_naming_mismatch_then_handl public async Task Handler_method_receives_command_arguments_bound_to_the_specified_type( Type type) { - var c = _bindingCases[type]; + var c = BindingCases[type]; var captureMethod = GetType() .GetMethod(nameof(CaptureMethod), BindingFlags.NonPublic | BindingFlags.Static) @@ -472,7 +472,7 @@ public async Task Handler_method_receives_command_arguments_bound_to_the_specifi public async Task Handler_method_receives_command_arguments_explicitly_bound_to_the_specified_type( Type type) { - var c = _bindingCases[type]; + var c = BindingCases[type]; var captureMethod = GetType() .GetMethod(nameof(CaptureMethod), BindingFlags.NonPublic | BindingFlags.Static) @@ -535,7 +535,7 @@ public void Apply(InvocationContext context) } } - private static readonly BindingTestSet _bindingCases = new BindingTestSet + internal static readonly BindingTestSet BindingCases = new BindingTestSet { BindingTestCase.Create>( "123", @@ -659,22 +659,22 @@ public void Apply(InvocationContext context) o => o.Should().BeEquivalentTo(new List { 1, 2 })) }; - private static string NonexistentPathWithoutTrailingSlash() + internal static string NonexistentPathWithoutTrailingSlash() { return Path.Combine( ExistingDirectory(), "does-not-exist"); } - private static string NonexistentPathWithTrailingSlash() => + internal static string NonexistentPathWithTrailingSlash() => NonexistentPathWithoutTrailingSlash() + Path.DirectorySeparatorChar; - private static string NonexistentPathWithTrailingAltSlash() => + internal static string NonexistentPathWithTrailingAltSlash() => NonexistentPathWithoutTrailingSlash() + Path.AltDirectorySeparatorChar; - private static string ExistingFile() => + internal static string ExistingFile() => Directory.GetFiles(ExistingDirectory()).FirstOrDefault() ?? throw new AssertionFailedException("No files found in current directory"); - private static string ExistingDirectory() => Directory.GetCurrentDirectory(); + internal static string ExistingDirectory() => Directory.GetCurrentDirectory(); } } diff --git a/src/System.CommandLine/Binding/ModelBinder.cs b/src/System.CommandLine/Binding/ModelBinder.cs index 96319bb21c..51f8bff772 100644 --- a/src/System.CommandLine/Binding/ModelBinder.cs +++ b/src/System.CommandLine/Binding/ModelBinder.cs @@ -8,9 +8,9 @@ namespace System.CommandLine.Binding { - public class ModelBinder + public class ModelBinder3 { - public ModelBinder(Type modelType) : this(new AnonymousValueDescriptor(modelType)) + public ModelBinder3(Type modelType) : this(new AnonymousValueDescriptor(modelType)) { if (modelType is null) { @@ -18,7 +18,7 @@ public ModelBinder(Type modelType) : this(new AnonymousValueDescriptor(modelType } } - internal ModelBinder(IValueDescriptor valueDescriptor) + internal ModelBinder3(IValueDescriptor valueDescriptor) { ValueDescriptor = valueDescriptor ?? throw new ArgumentNullException(nameof(valueDescriptor)); @@ -136,20 +136,10 @@ private bool TryDefaultConstructorAndPropertiesStrategy( .ConstructorDescriptors .OrderByDescending(d => d.ParameterDescriptors.Count); - foreach (var constructor in constructorDescriptors) - { - var boundConstructorArguments = GetValues( - ConstructorArgumentBindingSources, - context, - constructor.ParameterDescriptors, - true); + var (constructor, boundConstructorArguments) = FindConstructor(constructorDescriptors, context); - if (boundConstructorArguments.Count != constructor.ParameterDescriptors.Count) - { - continue; - } - - // Found invokable constructor, invoke and return + if (!(constructor is null)) + { var values = boundConstructorArguments.Select(v => v.Value).ToArray(); try @@ -164,13 +154,30 @@ private bool TryDefaultConstructorAndPropertiesStrategy( } catch { - instance = null; - return false; + // continue to failure exit } } - instance = null; return false; + + } + + private (ConstructorDescriptor?, IReadOnlyList?) FindConstructor(IOrderedEnumerable constructorDescriptors, BindingContext context) + { + foreach (var constructor in constructorDescriptors) + { + var boundConstructorArguments = GetValues( + ConstructorArgumentBindingSources, + context, + constructor.ParameterDescriptors, + true); + + if (boundConstructorArguments.Count == constructor.ParameterDescriptors.Count) + { + return (constructor, boundConstructorArguments); + } + } + return (null, null); } public void UpdateInstance(T instance, BindingContext bindingContext) @@ -205,6 +212,8 @@ private IReadOnlyList GetValues( if (boundValue is null) { + var binder = bindingContext.GetModelBinder(valueDescriptor); + var value = binder.CreateInstance(bindingContext); if (includeMissingValues) { if (valueDescriptor is ParameterDescriptor parameterDescriptor && diff --git a/src/System.CommandLine/Binding/ModelBinder2.cs b/src/System.CommandLine/Binding/ModelBinder2.cs new file mode 100644 index 0000000000..ccbb2723c9 --- /dev/null +++ b/src/System.CommandLine/Binding/ModelBinder2.cs @@ -0,0 +1,338 @@ +using System.Collections.Generic; +using System.ComponentModel; +using System.Diagnostics.CodeAnalysis; +using System.Linq; +using System.Reflection; +using System.Runtime.CompilerServices; + +namespace System.CommandLine.Binding +{ + public class ModelBinder + { + public ModelBinder(Type modelType) + : this(new AnonymousValueDescriptor(modelType)) + => _ = modelType ?? throw new ArgumentNullException(nameof(modelType)); + + internal ModelBinder(IValueDescriptor valueDescriptor) + { + ValueDescriptor = valueDescriptor ?? throw new ArgumentNullException(nameof(valueDescriptor)); + ModelDescriptor = ModelDescriptor.FromType(valueDescriptor.ValueType); + } + + public IValueDescriptor ValueDescriptor { get; } + public ModelDescriptor ModelDescriptor { get; } + public bool EnforceExplicitBinding { get; set; } + + internal Dictionary ConstructorArgumentBindingSources { get; } = + new Dictionary(); + + internal Dictionary MemberBindingSources { get; } = + new Dictionary(); + + // Consider deprecating in favor or BindingConfiguration/BindingContext attach validatation. Then make internal. + // Or at least rename to "ConfigureBinding" or similar + public void BindConstructorArgumentFromValue(ParameterInfo parameter, IValueDescriptor valueDescriptor) + { + var constructor = FindConstructorOrThrow(parameter, "Parameter must be declared on a constructor."); + var ctorDesc = FindModelConstructorDescriptor(constructor); + + if (ctorDesc is null) + throw new ArgumentException(paramName: nameof(parameter), + message: "Parameter is not described by any of the model constructor descriptors."); + + var paramDesc = ctorDesc.ParameterDescriptors[parameter.Position]; + ConstructorArgumentBindingSources[paramDesc] = new SpecificSymbolValueSource(valueDescriptor); + } + + public void BindMemberFromValue(PropertyInfo property, IValueDescriptor valueDescriptor) + { + var propertyDescriptor = FindModelPropertyDescriptor(property.PropertyType, property.Name); + + if (propertyDescriptor is null) + throw new ArgumentException(paramName: nameof(property), + message: "Property is not described by any of the model property descriptors."); + + MemberBindingSources[propertyDescriptor] = new SpecificSymbolValueSource(valueDescriptor); + } + + public object? CreateInstance(BindingContext bindingContext) + { + var (_, newInstance, _) = CreateInstanceInternal(bindingContext, true); + return newInstance; + } + + private (bool success, object? newInstance, bool usedParameterlessConstructor) CreateInstanceInternal(BindingContext bindingContext, + bool throwIfNoConstructor) + { + var (constructor, boundValues) = GetConstructorAndAgs(bindingContext); + if (constructor is null) + { + if (throwIfNoConstructor) + { + throw new InvalidOperationException("No appropriate constructor found"); + } + return (false, null, false); + } + + var values = boundValues.Select(x => x.Value).ToArray(); + object? newInstance = null; + try + { + newInstance = constructor.Invoke(values); + } + catch + { + return (false, null, false); + } + if (!(newInstance is null)) + { + UpdateInstance(newInstance, bindingContext); + } + return (true, newInstance, constructor.ParameterDescriptors.Any()); + } + + public void UpdateInstance(T instance, BindingContext bindingContext) + { + var boundValues = GetValues( + MemberBindingSources, + bindingContext, + ModelDescriptor.PropertyDescriptors, + includeMissingValues: false); + + foreach (var boundValue in boundValues) + { + ((PropertyDescriptor)boundValue.ValueDescriptor).SetValue(instance, boundValue.Value); + } + } + + private (ConstructorDescriptor?, IReadOnlyList?) GetConstructorAndAgs(BindingContext bindingContext) + { + var constructorDescriptors = + ModelDescriptor + .ConstructorDescriptors + .OrderByDescending(d => d.ParameterDescriptors.Count); + foreach (var constructor in constructorDescriptors) + { + var boundConstructorArguments = GetValues( + ConstructorArgumentBindingSources, + bindingContext, + constructor.ParameterDescriptors, + true); + + if (boundConstructorArguments.Count == constructor.ParameterDescriptors.Count) + { + return (constructor, boundConstructorArguments); + } + } + return (null, null); + } + + + private IReadOnlyList GetValues( + IDictionary? bindingSources, + BindingContext bindingContext, + IReadOnlyList valueDescriptors, + bool includeMissingValues) + { + var values = new List(); + + for (var index = 0; index < valueDescriptors.Count; index++) + { + var valueDescriptor = valueDescriptors[index]; + var valueSource = GetValueSource(bindingSources, bindingContext, valueDescriptor); + BoundValue? boundValue = GetBoundValue(valueSource, bindingContext, valueDescriptor, includeMissingValues, ModelDescriptor); + + //if (boundValue is null) + //{ + + // var binder = bindingContext.GetModelBinder(valueDescriptor); + // // if there are constructors with parameters, we will try to bind + + // if (boundValue is null && includeMissingValues) + // { + // if (valueDescriptor.HasDefaultValue) + // { + // boundValue = BoundValue.DefaultForValueDescriptor(valueDescriptor); + // } + // if (valueDescriptor.ValueType == ModelDescriptor.ModelType) + // { + // throw new NotImplementedException("Recursive models are not allowed."); + // } + // var (success, newInstance) = binder.CreateInstanceInternal(bindingContext, false); + // if (success) + // { + // // might change to early loop, but this might make flow more clear + // boundValue = new BoundValue(newInstance, valueDescriptor, valueSource); + // } + + // if (valueDescriptor is ParameterDescriptor parameterDescriptor) + // { + // else if (parameterDescriptor.AllowsNull) + // // Logic dropped here - misnamed and purpose unclear: ShouldPassNullToConstructor(constructorDescriptor.Parent, constructorDescriptor)) + // boundValue = BoundValue.DefaultForType(valueDescriptor); + // } + // } + //} + + if (boundValue != null) + { + values.Add(boundValue); + } + } + + return values; + } + + private IValueSource GetValueSource( + IDictionary? bindingSources, + BindingContext bindingContext, + IValueDescriptor valueDescriptor) + { + if (!(bindingSources is null) && + bindingSources.TryGetValue(valueDescriptor, out IValueSource? valueSource)) + { + return valueSource; + } + + if (bindingContext.TryGetValueSource(valueDescriptor, out valueSource)) + { + return valueSource; + } + + if (!EnforceExplicitBinding) + { + // Return a value source that will match from the parseResult + // by name and type (or a possible conversion) + return new ParseResultMatchingValueSource(); + } + + return new MissingValueSource(); + } + + internal static BoundValue? GetBoundValue(IValueSource valueSource, + BindingContext bindingContext, + IValueDescriptor valueDescriptor, + bool includeMissingValues, + ModelDescriptor? modelDescriptor = null) + { + BoundValue? boundValue; + if (bindingContext.TryBindToScalarValue( + valueDescriptor, + valueSource, + out boundValue)) + { + return boundValue; + } + + if (valueDescriptor.HasDefaultValue) + { + return BoundValue.DefaultForValueDescriptor(valueDescriptor); + } + + if (!(modelDescriptor is null) && valueDescriptor.ValueType == modelDescriptor.ModelType) + { + throw new NotImplementedException("Recursive models are not allowed."); + } + + var binder = bindingContext.GetModelBinder(valueDescriptor); + // if there are constructors with parameters, we will try to bind + var (success, newInstance, usedParameterlessConstructor) = binder.CreateInstanceInternal(bindingContext, false); + if (success && usedParameterlessConstructor) + { + return new BoundValue(newInstance, valueDescriptor, valueSource); + } + + if (includeMissingValues) + { + if (valueDescriptor is ParameterDescriptor parameterDescriptor && parameterDescriptor.AllowsNull) + { + return new BoundValue(parameterDescriptor.GetDefaultValue(), valueDescriptor, valueSource); + } + // Logic dropped here - misnamed and purpose unclear: ShouldPassNullToConstructor(constructorDescriptor.Parent, constructorDescriptor)) + return BoundValue.DefaultForType(valueDescriptor); + } + return null; + } + } + + + protected ConstructorDescriptor FindModelConstructorDescriptor(ConstructorInfo constructorInfo) + { + var constructorParameters = constructorInfo.GetParameters(); + + return ModelDescriptor.ConstructorDescriptors + .FirstOrDefault(ctorDesc + => ModelDescriptor.ModelType == constructorInfo.DeclaringType && + ctorDesc.ParameterDescriptors + .Any(x => constructorParameters.Any(y => MatchParameter(x, y)))); + + static bool MatchParameter(ParameterDescriptor desc, ParameterInfo info) + { + return desc.ValueType == info.ParameterType && + desc.ValueName == info.Name && + desc.HasDefaultValue == info.HasDefaultValue && + desc.AllowsNull == ParameterDescriptor.CalculateAllowsNull(info); + } + } + + protected IValueDescriptor FindModelPropertyDescriptor(Type propertyType, string propertyName) + { + return ModelDescriptor.PropertyDescriptors + .FirstOrDefault(desc => + desc.ValueType == propertyType && + string.Equals(desc.ValueName, propertyName, StringComparison.Ordinal) + ); + } + + + + + + + + + + private ConstructorInfo FindConstructorOrThrow(ParameterInfo parameter, string message) + { + if (!(parameter.Member is ConstructorInfo constructor)) + { + throw new ArgumentException(paramName: nameof(parameter), + message: message); + } + return constructor; + } + + private class ConstructorDescriptorEquality : IEqualityComparer + { + public bool Equals(ParameterDescriptor x, ParameterDescriptor y) + { + return x.ValueType == x.ValueType && + x.AllowsNull == x.AllowsNull && + x.HasDefaultValue == x.HasDefaultValue; + } + + public int GetHashCode(ParameterDescriptor obj) + { + throw new NotImplementedException(); + } + } + + private class AnonymousValueDescriptor : IValueDescriptor + { + public Type ValueType { get; } + + public AnonymousValueDescriptor(Type modelType) + { + ValueType = modelType; + } + + public string ValueName => ""; + + public bool HasDefaultValue => false; + + public object? GetDefaultValue() => null; + + public override string ToString() => $"{ValueType}"; + } +} +} diff --git a/src/System.CommandLine/Binding/ParameterDescriptor.cs b/src/System.CommandLine/Binding/ParameterDescriptor.cs index 29b3729380..e81138b309 100644 --- a/src/System.CommandLine/Binding/ParameterDescriptor.cs +++ b/src/System.CommandLine/Binding/ParameterDescriptor.cs @@ -32,22 +32,16 @@ public bool AllowsNull { if (_allowsNull is null) { - if (_parameterInfo.ParameterType.IsNullable()) - { - _allowsNull = true; - } - - if (_parameterInfo.HasDefaultValue && - _parameterInfo.DefaultValue is null) - { - _allowsNull = true; - } + _allowsNull = CalculateAllowsNull(_parameterInfo); } - return _allowsNull ?? false; } } + public static bool CalculateAllowsNull(ParameterInfo parameterInfo) + => parameterInfo.ParameterType.IsNullable() || + (parameterInfo.HasDefaultValue && parameterInfo.DefaultValue is null); + public object? GetDefaultValue() => _parameterInfo.DefaultValue is DBNull ? ValueType.GetDefaultValueForType() diff --git a/src/System.CommandLine/Binding/ServiceProviderValueSource.cs b/src/System.CommandLine/Binding/ServiceProviderValueSource.cs index 6c4e80010e..04a927da70 100644 --- a/src/System.CommandLine/Binding/ServiceProviderValueSource.cs +++ b/src/System.CommandLine/Binding/ServiceProviderValueSource.cs @@ -5,12 +5,12 @@ namespace System.CommandLine.Binding { internal class ServiceProviderValueSource : IValueSource { - public bool TryGetValue( - IValueDescriptor valueDescriptor, - BindingContext? bindingContext, - out object? boundValue) + public bool TryGetValue(IValueDescriptor valueDescriptor, + BindingContext? bindingContext, + out object? boundValue) { boundValue = bindingContext?.ServiceProvider.GetService(valueDescriptor.ValueType); + // ?? Why return true if the service isn't found? return true; } } diff --git a/src/System.CommandLine/Binding/SpecificSymbolValueSource.cs b/src/System.CommandLine/Binding/SpecificSymbolValueSource.cs index 746aae3d2b..e12a260a28 100644 --- a/src/System.CommandLine/Binding/SpecificSymbolValueSource.cs +++ b/src/System.CommandLine/Binding/SpecificSymbolValueSource.cs @@ -15,8 +15,8 @@ public SpecificSymbolValueSource(IValueDescriptor valueDescriptor) public IValueDescriptor ValueDescriptor { get; } public bool TryGetValue(IValueDescriptor valueDescriptor, - BindingContext? bindingContext, - out object? boundValue) + BindingContext? bindingContext, + out object? boundValue) { var specificDescriptor = ValueDescriptor; switch (specificDescriptor) diff --git a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs index a81f251049..6e83b0dfbc 100644 --- a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs +++ b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs @@ -61,7 +61,7 @@ public async Task InvokeAsync(InvocationContext context) var paramDesc = _parameterDescriptors[i]; if (_invokeArgumentBindingSources.TryGetValue(paramDesc, out var valueSource)) { - var boundValue = ModelBinder.GetBoundValue(valueSource, bindingContext, paramDesc); + var boundValue = ModelBinder.GetBoundValue(valueSource, bindingContext, modelDescriptor: paramDesc); if (!(boundValue is null)) { invocationArguments[i] = boundValue.Value; @@ -72,15 +72,12 @@ public async Task InvokeAsync(InvocationContext context) invocationArguments[i] = binder.CreateInstance(bindingContext); } - var invocationTarget = _invocationTarget ?? - _invocationTargetBinder?.CreateInstance(bindingContext); - object result; if (_handlerDelegate is null) { - result = _handlerMethodInfo!.Invoke( - invocationTarget, - invocationArguments); + var invocationTarget = _invocationTarget ?? + _invocationTargetBinder?.CreateInstance(bindingContext); + result = _handlerMethodInfo!.Invoke(invocationTarget, invocationArguments); } else { From 63faa9bf5fe3af263f4c91efee914673c492dc00 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Sat, 8 Aug 2020 07:10:27 -0700 Subject: [PATCH 05/12] Sort of works. Prior to Lazy redeisgn --- .../Binding/ModelBinder2.cs | 221 +++++++++--------- .../Binding/ParameterDescriptor.cs | 1 + .../Invocation/ModelBindingCommandHandler.cs | 2 +- 3 files changed, 106 insertions(+), 118 deletions(-) diff --git a/src/System.CommandLine/Binding/ModelBinder2.cs b/src/System.CommandLine/Binding/ModelBinder2.cs index ccbb2723c9..364aa236b8 100644 --- a/src/System.CommandLine/Binding/ModelBinder2.cs +++ b/src/System.CommandLine/Binding/ModelBinder2.cs @@ -4,6 +4,7 @@ using System.Linq; using System.Reflection; using System.Runtime.CompilerServices; +using System.Threading; namespace System.CommandLine.Binding { @@ -61,17 +62,18 @@ public void BindMemberFromValue(PropertyInfo property, IValueDescriptor valueDes return newInstance; } - private (bool success, object? newInstance, bool usedParameterlessConstructor) CreateInstanceInternal(BindingContext bindingContext, - bool throwIfNoConstructor) + private (bool success, object? newInstance, bool anyNonDefaults) CreateInstanceInternal( + BindingContext bindingContext, + bool throwIfNoConstructor) { - var (constructor, boundValues) = GetConstructorAndAgs(bindingContext); + var constructorAndArgs = GetConstructorAndAgs(bindingContext); + var constructor = constructorAndArgs.Constructor; + var boundValues = constructorAndArgs.BoundValues; if (constructor is null) { - if (throwIfNoConstructor) - { - throw new InvalidOperationException("No appropriate constructor found"); - } - return (false, null, false); + return throwIfNoConstructor + ? throw new InvalidOperationException("No appropriate constructor found") + : ((bool success, object? newInstance, bool anyNonDefaults))(false, null, false); } var values = boundValues.Select(x => x.Value).ToArray(); @@ -88,12 +90,12 @@ public void BindMemberFromValue(PropertyInfo property, IValueDescriptor valueDes { UpdateInstance(newInstance, bindingContext); } - return (true, newInstance, constructor.ParameterDescriptors.Any()); + return (true, newInstance, constructorAndArgs.NonDefaultsUsed); } public void UpdateInstance(T instance, BindingContext bindingContext) { - var boundValues = GetValues( + var (boundValues, anyNonDefaults) = GetValues( MemberBindingSources, bindingContext, ModelDescriptor.PropertyDescriptors, @@ -105,7 +107,7 @@ public void UpdateInstance(T instance, BindingContext bindingContext) } } - private (ConstructorDescriptor?, IReadOnlyList?) GetConstructorAndAgs(BindingContext bindingContext) + private ConstructorAndArgs GetConstructorAndAgs(BindingContext bindingContext) { var constructorDescriptors = ModelDescriptor @@ -113,74 +115,46 @@ public void UpdateInstance(T instance, BindingContext bindingContext) .OrderByDescending(d => d.ParameterDescriptors.Count); foreach (var constructor in constructorDescriptors) { - var boundConstructorArguments = GetValues( + var (boundValues, anyNonDefaults) = GetValues( ConstructorArgumentBindingSources, bindingContext, constructor.ParameterDescriptors, true); - if (boundConstructorArguments.Count == constructor.ParameterDescriptors.Count) + if (boundValues.Count == constructor.ParameterDescriptors.Count) { - return (constructor, boundConstructorArguments); + return new ConstructorAndArgs (constructor, boundValues, anyNonDefaults); } } - return (null, null); + return new ConstructorAndArgs(null, null, false); } - private IReadOnlyList GetValues( + private (IReadOnlyList boundValues, bool anyNonDefaults) GetValues( IDictionary? bindingSources, BindingContext bindingContext, IReadOnlyList valueDescriptors, bool includeMissingValues) { var values = new List(); + var anyNonDefaults = false; for (var index = 0; index < valueDescriptors.Count; index++) { var valueDescriptor = valueDescriptors[index]; var valueSource = GetValueSource(bindingSources, bindingContext, valueDescriptor); - BoundValue? boundValue = GetBoundValue(valueSource, bindingContext, valueDescriptor, includeMissingValues, ModelDescriptor); - - //if (boundValue is null) - //{ - - // var binder = bindingContext.GetModelBinder(valueDescriptor); - // // if there are constructors with parameters, we will try to bind - - // if (boundValue is null && includeMissingValues) - // { - // if (valueDescriptor.HasDefaultValue) - // { - // boundValue = BoundValue.DefaultForValueDescriptor(valueDescriptor); - // } - // if (valueDescriptor.ValueType == ModelDescriptor.ModelType) - // { - // throw new NotImplementedException("Recursive models are not allowed."); - // } - // var (success, newInstance) = binder.CreateInstanceInternal(bindingContext, false); - // if (success) - // { - // // might change to early loop, but this might make flow more clear - // boundValue = new BoundValue(newInstance, valueDescriptor, valueSource); - // } - - // if (valueDescriptor is ParameterDescriptor parameterDescriptor) - // { - // else if (parameterDescriptor.AllowsNull) - // // Logic dropped here - misnamed and purpose unclear: ShouldPassNullToConstructor(constructorDescriptor.Parent, constructorDescriptor)) - // boundValue = BoundValue.DefaultForType(valueDescriptor); - // } - // } - //} - + var (boundValue, usedNonDefault) = GetBoundValue(valueSource, bindingContext, valueDescriptor, includeMissingValues, ModelDescriptor); + if (usedNonDefault && !anyNonDefaults) + { + anyNonDefaults = true; + } if (boundValue != null) { values.Add(boundValue); } } - return values; + return (values, anyNonDefaults); } private IValueSource GetValueSource( @@ -209,11 +183,12 @@ private IValueSource GetValueSource( return new MissingValueSource(); } - internal static BoundValue? GetBoundValue(IValueSource valueSource, - BindingContext bindingContext, - IValueDescriptor valueDescriptor, - bool includeMissingValues, - ModelDescriptor? modelDescriptor = null) + internal static (BoundValue? boundValue, bool usedNonDefault) GetBoundValue( + IValueSource valueSource, + BindingContext bindingContext, + IValueDescriptor valueDescriptor, + bool includeMissingValues, + ModelDescriptor? modelDescriptor = null) { BoundValue? boundValue; if (bindingContext.TryBindToScalarValue( @@ -221,68 +196,66 @@ private IValueSource GetValueSource( valueSource, out boundValue)) { - return boundValue; + return (boundValue, true); } if (valueDescriptor.HasDefaultValue) { - return BoundValue.DefaultForValueDescriptor(valueDescriptor); + return (BoundValue.DefaultForValueDescriptor(valueDescriptor), false); } if (!(modelDescriptor is null) && valueDescriptor.ValueType == modelDescriptor.ModelType) { throw new NotImplementedException("Recursive models are not allowed."); } - var binder = bindingContext.GetModelBinder(valueDescriptor); // if there are constructors with parameters, we will try to bind - var (success, newInstance, usedParameterlessConstructor) = binder.CreateInstanceInternal(bindingContext, false); - if (success && usedParameterlessConstructor) + var (success, newInstance, usedNonDefaults) = binder.CreateInstanceInternal(bindingContext, false); + if (success && usedNonDefaults) { - return new BoundValue(newInstance, valueDescriptor, valueSource); + return (new BoundValue(newInstance, valueDescriptor, valueSource), true); } if (includeMissingValues) { if (valueDescriptor is ParameterDescriptor parameterDescriptor && parameterDescriptor.AllowsNull) { - return new BoundValue(parameterDescriptor.GetDefaultValue(), valueDescriptor, valueSource); + return (new BoundValue(parameterDescriptor.GetDefaultValue(), valueDescriptor, valueSource), false); } // Logic dropped here - misnamed and purpose unclear: ShouldPassNullToConstructor(constructorDescriptor.Parent, constructorDescriptor)) - return BoundValue.DefaultForType(valueDescriptor); + return (BoundValue.DefaultForType(valueDescriptor), false); } - return null; + return (null, false); } - } - protected ConstructorDescriptor FindModelConstructorDescriptor(ConstructorInfo constructorInfo) - { - var constructorParameters = constructorInfo.GetParameters(); + protected ConstructorDescriptor FindModelConstructorDescriptor(ConstructorInfo constructorInfo) + { + var constructorParameters = constructorInfo.GetParameters(); - return ModelDescriptor.ConstructorDescriptors - .FirstOrDefault(ctorDesc - => ModelDescriptor.ModelType == constructorInfo.DeclaringType && - ctorDesc.ParameterDescriptors - .Any(x => constructorParameters.Any(y => MatchParameter(x, y)))); + return ModelDescriptor.ConstructorDescriptors + .FirstOrDefault(ctorDesc + => ModelDescriptor.ModelType == constructorInfo.DeclaringType && + ctorDesc.ParameterDescriptors + .Any(x => constructorParameters.Any(y => MatchParameter(x, y)))); - static bool MatchParameter(ParameterDescriptor desc, ParameterInfo info) - { - return desc.ValueType == info.ParameterType && - desc.ValueName == info.Name && - desc.HasDefaultValue == info.HasDefaultValue && - desc.AllowsNull == ParameterDescriptor.CalculateAllowsNull(info); + static bool MatchParameter(ParameterDescriptor desc, ParameterInfo info) + { + return desc.ValueType == info.ParameterType && + desc.ValueName == info.Name && + desc.HasDefaultValue == info.HasDefaultValue && + desc.AllowsNull == ParameterDescriptor.CalculateAllowsNull(info); + } } - } - protected IValueDescriptor FindModelPropertyDescriptor(Type propertyType, string propertyName) - { - return ModelDescriptor.PropertyDescriptors - .FirstOrDefault(desc => - desc.ValueType == propertyType && - string.Equals(desc.ValueName, propertyName, StringComparison.Ordinal) - ); - } + protected IValueDescriptor FindModelPropertyDescriptor(Type propertyType, string propertyName) + { + return ModelDescriptor.PropertyDescriptors + .FirstOrDefault(desc => + desc.ValueType == propertyType && + string.Equals(desc.ValueName, propertyName, StringComparison.Ordinal) + ); + } @@ -292,47 +265,61 @@ protected IValueDescriptor FindModelPropertyDescriptor(Type propertyType, string - private ConstructorInfo FindConstructorOrThrow(ParameterInfo parameter, string message) - { - if (!(parameter.Member is ConstructorInfo constructor)) + private ConstructorInfo FindConstructorOrThrow(ParameterInfo parameter, string message) { - throw new ArgumentException(paramName: nameof(parameter), - message: message); + if (!(parameter.Member is ConstructorInfo constructor)) + { + throw new ArgumentException(paramName: nameof(parameter), + message: message); + } + return constructor; } - return constructor; - } - private class ConstructorDescriptorEquality : IEqualityComparer - { - public bool Equals(ParameterDescriptor x, ParameterDescriptor y) + private class ConstructorDescriptorEquality : IEqualityComparer { - return x.ValueType == x.ValueType && - x.AllowsNull == x.AllowsNull && - x.HasDefaultValue == x.HasDefaultValue; + public bool Equals(ParameterDescriptor x, ParameterDescriptor y) + { + return x.ValueType == x.ValueType && + x.AllowsNull == x.AllowsNull && + x.HasDefaultValue == x.HasDefaultValue; + } + + public int GetHashCode(ParameterDescriptor obj) + { + throw new NotImplementedException(); + } } - public int GetHashCode(ParameterDescriptor obj) + private class AnonymousValueDescriptor : IValueDescriptor { - throw new NotImplementedException(); + public Type ValueType { get; } + + public AnonymousValueDescriptor(Type modelType) + { + ValueType = modelType; + } + + public string ValueName => ""; + + public bool HasDefaultValue => false; + + public object? GetDefaultValue() => null; + + public override string ToString() => $"{ValueType}"; } } - private class AnonymousValueDescriptor : IValueDescriptor + internal struct ConstructorAndArgs { - public Type ValueType { get; } + public ConstructorDescriptor? Constructor { get; } + public IReadOnlyList? BoundValues { get; } + public bool NonDefaultsUsed { get; } - public AnonymousValueDescriptor(Type modelType) + public ConstructorAndArgs(ConstructorDescriptor? constructor, IReadOnlyList? boundValues, bool nonDefaultsUsed) { - ValueType = modelType; + Constructor = constructor; + BoundValues = boundValues; + NonDefaultsUsed = nonDefaultsUsed; } - - public string ValueName => ""; - - public bool HasDefaultValue => false; - - public object? GetDefaultValue() => null; - - public override string ToString() => $"{ValueType}"; } } -} diff --git a/src/System.CommandLine/Binding/ParameterDescriptor.cs b/src/System.CommandLine/Binding/ParameterDescriptor.cs index e81138b309..7c3282cc3f 100644 --- a/src/System.CommandLine/Binding/ParameterDescriptor.cs +++ b/src/System.CommandLine/Binding/ParameterDescriptor.cs @@ -26,6 +26,7 @@ internal ParameterDescriptor( public bool HasDefaultValue => _parameterInfo.HasDefaultValue; + // ?? This is used in model binder to determine whether to call GetDefaultValue. This is either misnamed or there is another issue public bool AllowsNull { get diff --git a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs index 6e83b0dfbc..7811cea159 100644 --- a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs +++ b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs @@ -61,7 +61,7 @@ public async Task InvokeAsync(InvocationContext context) var paramDesc = _parameterDescriptors[i]; if (_invokeArgumentBindingSources.TryGetValue(paramDesc, out var valueSource)) { - var boundValue = ModelBinder.GetBoundValue(valueSource, bindingContext, modelDescriptor: paramDesc); + var (boundValue, _) = ModelBinder.GetBoundValue(valueSource, bindingContext, paramDesc, true); if (!(boundValue is null)) { invocationArguments[i] = boundValue.Value; From 80487a946ce70447180666e2e9ea5c235eab5169 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Sun, 9 Aug 2020 10:39:33 -0700 Subject: [PATCH 06/12] All tests passing!!! Needs refactor --- .../Binding/ModelBinderTests.cs | 7 +- .../Binding/BindingContext.cs | 9 ++ src/System.CommandLine/Binding/BoundValue.cs | 25 ++- .../Binding/DelegateHandlerDescriptor.cs | 2 +- .../Binding/MethodInfoHandlerDescriptor.cs | 4 +- .../Binding/ModelBinder2.cs | 146 ++++++++++++++---- .../Invocation/ModelBindingCommandHandler.cs | 66 +++++--- 7 files changed, 196 insertions(+), 63 deletions(-) diff --git a/src/System.CommandLine.Tests/Binding/ModelBinderTests.cs b/src/System.CommandLine.Tests/Binding/ModelBinderTests.cs index f09a941c77..ea3c61ffaf 100644 --- a/src/System.CommandLine.Tests/Binding/ModelBinderTests.cs +++ b/src/System.CommandLine.Tests/Binding/ModelBinderTests.cs @@ -104,7 +104,12 @@ public void Command_arguments_are_bound_by_name_to_complex_constructor_parameter object valueReceivedValue = ((dynamic)instance).Value; var expectedValue = new FileInfo(commandLine); - valueReceivedValue.Should().BeEquivalentTo(expectedValue); + valueReceivedValue.Should().BeOfType(); + var fileInfoValue = valueReceivedValue as FileInfo; + fileInfoValue.FullName.Should().Be(expectedValue.FullName); + // The following fails when it attempts to compare the Length of the file. I have + // no idea why this previously worked. + //valueReceivedValue.Should().BeEquivalentTo(expectedValue); } [Fact] diff --git a/src/System.CommandLine/Binding/BindingContext.cs b/src/System.CommandLine/Binding/BindingContext.cs index 7104958df4..d9b8a29c97 100644 --- a/src/System.CommandLine/Binding/BindingContext.cs +++ b/src/System.CommandLine/Binding/BindingContext.cs @@ -63,6 +63,15 @@ public ModelBinder GetModelBinder(IValueDescriptor valueDescriptor) return new ModelBinder(valueDescriptor); } + internal ModelBinder GetModelBinder(Type type) + { + if (_modelBindersByValueDescriptor.TryGetValue(type, out ModelBinder binder)) + { + return binder; + } + return new ModelBinder(type); + } + public void AddService(Type serviceType, Func factory) { ServiceProvider.AddService(serviceType, factory); diff --git a/src/System.CommandLine/Binding/BoundValue.cs b/src/System.CommandLine/Binding/BoundValue.cs index 608771a72e..c28f0a081f 100644 --- a/src/System.CommandLine/Binding/BoundValue.cs +++ b/src/System.CommandLine/Binding/BoundValue.cs @@ -5,6 +5,7 @@ namespace System.CommandLine.Binding { public class BoundValue { + // ?? Why have an internal constructor on a public readonly class? internal BoundValue( object? value, IValueDescriptor valueDescriptor, @@ -25,7 +26,7 @@ internal BoundValue( public IValueSource ValueSource { get; } - public object? Value { get; } + public virtual object? Value { get; } public override string ToString() => $"{ValueDescriptor}: {Value}"; @@ -53,4 +54,26 @@ public static BoundValue DefaultForValueDescriptor(IValueDescriptor valueDescrip valueSource); } } + + //public class LazyBoundValue : BoundValue + //{ + // private bool _valueHasBeenSet; + // internal LazyBoundValue( IValueDescriptor valueDescriptor, IValueSource valueSource) + // : base(null, valueDescriptor, valueSource) + // { + // } + + // public override object? Value + // { + // get + // { + // if (!_valueHasBeenSet ) + // { + // object? value; + // if (ValueSource.TryGetValue(ValueDescriptor, )) + // } + // return base.Value; + // } + // } + //} } diff --git a/src/System.CommandLine/Binding/DelegateHandlerDescriptor.cs b/src/System.CommandLine/Binding/DelegateHandlerDescriptor.cs index 9e5f418910..a58d642718 100644 --- a/src/System.CommandLine/Binding/DelegateHandlerDescriptor.cs +++ b/src/System.CommandLine/Binding/DelegateHandlerDescriptor.cs @@ -20,7 +20,7 @@ public override ICommandHandler GetCommandHandler() { return new ModelBindingCommandHandler( _handlerDelegate, - ParameterDescriptors); + this); } public override ModelDescriptor? Parent => null; diff --git a/src/System.CommandLine/Binding/MethodInfoHandlerDescriptor.cs b/src/System.CommandLine/Binding/MethodInfoHandlerDescriptor.cs index e048abb5ad..72783b737f 100644 --- a/src/System.CommandLine/Binding/MethodInfoHandlerDescriptor.cs +++ b/src/System.CommandLine/Binding/MethodInfoHandlerDescriptor.cs @@ -28,13 +28,13 @@ public override ICommandHandler GetCommandHandler() { return new ModelBindingCommandHandler( _handlerMethodInfo, - ParameterDescriptors); + this); } else { return new ModelBindingCommandHandler( _handlerMethodInfo, - ParameterDescriptors, + this, _invocationTarget); } } diff --git a/src/System.CommandLine/Binding/ModelBinder2.cs b/src/System.CommandLine/Binding/ModelBinder2.cs index 364aa236b8..b2cc2f7e6a 100644 --- a/src/System.CommandLine/Binding/ModelBinder2.cs +++ b/src/System.CommandLine/Binding/ModelBinder2.cs @@ -4,6 +4,7 @@ using System.Linq; using System.Reflection; using System.Runtime.CompilerServices; +using System.Text.RegularExpressions; using System.Threading; namespace System.CommandLine.Binding @@ -66,16 +67,78 @@ public void BindMemberFromValue(PropertyInfo property, IValueDescriptor valueDes BindingContext bindingContext, bool throwIfNoConstructor) { - var constructorAndArgs = GetConstructorAndAgs(bindingContext); + if (DisallowedBindingType()) + { + throw new InvalidOperationException($"The type {ModelDescriptor.ModelType} cannot be bound"); + } + if (CanShortCut(bindingContext)) + { + return GetSimpleModelValue(MemberBindingSources, bindingContext); + } + var constructorAndArgs = GetConstructorAndArgs(bindingContext); var constructor = constructorAndArgs.Constructor; var boundValues = constructorAndArgs.BoundValues; + bool nonDefaultsUsed = constructorAndArgs.NonDefaultsUsed; if (constructor is null) { - return throwIfNoConstructor - ? throw new InvalidOperationException("No appropriate constructor found") - : ((bool success, object? newInstance, bool anyNonDefaults))(false, null, false); + return GetSimpleModelValue(ConstructorArgumentBindingSources, bindingContext); + //var valueSource = GetValueSource(ConstructorArgumentBindingSources, bindingContext, ValueDescriptor); + //var (boundValue, usedNonDefault) = GetBoundValue(valueSource, bindingContext, ValueDescriptor, true, ModelDescriptor); + //return boundValue is null + // ? (false, (object?)null, false) + // : (true, boundValue.Value, usedNonDefault); + } + + return InstanceFromSpecificConstructor(bindingContext, constructor, boundValues, ref nonDefaultsUsed); + } + + private bool DisallowedBindingType() + { + var disallowedTypes = new List + { + typeof(Span<>), + typeof(ReadOnlySpan<>) + }; + var type = ModelDescriptor.ModelType; + return disallowedTypes + .Any(x => type.IsGenericType && (type.GetGenericTypeDefinition() == x)); + } + + private bool CanShortCut(BindingContext bindingContext) + { + var explicitTypesToShortcut = new List + { + typeof(string) + }; + Type modelType = ModelDescriptor.ModelType; + return modelType.IsPrimitive || + IsNullable(modelType) || + explicitTypesToShortcut.Contains(modelType); + + static bool IsNullable(Type type) + { + return type.IsGenericType && + type.GetGenericTypeDefinition() == typeof(Nullable<>); + } + } + + private (bool success, object? newInstance, bool anyNonDefaults) GetSimpleModelValue( + IDictionary? bindingSources, BindingContext bindingContext) + { + var valueSource = GetValueSource(bindingSources, bindingContext, ValueDescriptor, EnforceExplicitBinding); + if (bindingContext.TryBindToScalarValue( + ValueDescriptor, + valueSource, + out var boundValue)) + { + return (true, boundValue?.Value, true); } + return (false, null, false); + + } + private (bool success, object newInstance, bool anyNonDefaults) InstanceFromSpecificConstructor(BindingContext bindingContext, ConstructorDescriptor? constructor, IReadOnlyList? boundValues, ref bool nonDefaultsUsed) + { var values = boundValues.Select(x => x.Value).ToArray(); object? newInstance = null; try @@ -88,52 +151,71 @@ public void BindMemberFromValue(PropertyInfo property, IValueDescriptor valueDes } if (!(newInstance is null)) { - UpdateInstance(newInstance, bindingContext); + nonDefaultsUsed = UpdateInstanceInternalNotifyIfNonDefaultsUsed(newInstance, bindingContext); } - return (true, newInstance, constructorAndArgs.NonDefaultsUsed); + return (true, newInstance, nonDefaultsUsed); } public void UpdateInstance(T instance, BindingContext bindingContext) + => UpdateInstanceInternalNotifyIfNonDefaultsUsed(instance, bindingContext); + + private bool UpdateInstanceInternalNotifyIfNonDefaultsUsed(T instance, BindingContext bindingContext) { - var (boundValues, anyNonDefaults) = GetValues( + var (boundValues, anyNonDefaults) = GetBoundValues( MemberBindingSources, bindingContext, ModelDescriptor.PropertyDescriptors, + ModelDescriptor.ModelType, + EnforceExplicitBinding, includeMissingValues: false); foreach (var boundValue in boundValues) { ((PropertyDescriptor)boundValue.ValueDescriptor).SetValue(instance, boundValue.Value); } + + return anyNonDefaults; } - private ConstructorAndArgs GetConstructorAndAgs(BindingContext bindingContext) + private ConstructorAndArgs GetConstructorAndArgs(BindingContext bindingContext) { var constructorDescriptors = ModelDescriptor .ConstructorDescriptors .OrderByDescending(d => d.ParameterDescriptors.Count); + ConstructorAndArgs? bestNonMatching = null; foreach (var constructor in constructorDescriptors) { - var (boundValues, anyNonDefaults) = GetValues( + var (boundValues, anyNonDefaults) = GetBoundValues( ConstructorArgumentBindingSources, bindingContext, constructor.ParameterDescriptors, + ModelDescriptor.ModelType, + EnforceExplicitBinding, true); if (boundValues.Count == constructor.ParameterDescriptors.Count) { - return new ConstructorAndArgs (constructor, boundValues, anyNonDefaults); + var match = new ConstructorAndArgs(constructor, boundValues, anyNonDefaults); + if (anyNonDefaults) + { // based on parameter length, first usable constructor that utilizes CLI definition + return match; + } + bestNonMatching ??= match; } } - return new ConstructorAndArgs(null, null, false); + return bestNonMatching is null + ? new ConstructorAndArgs(null, null, false) + : bestNonMatching; } - private (IReadOnlyList boundValues, bool anyNonDefaults) GetValues( + internal static (IReadOnlyList boundValues, bool anyNonDefaults) GetBoundValues( IDictionary? bindingSources, BindingContext bindingContext, IReadOnlyList valueDescriptors, + Type parentType, + bool enforceExplicitBinding, bool includeMissingValues) { var values = new List(); @@ -142,8 +224,9 @@ private ConstructorAndArgs GetConstructorAndAgs(BindingContext bindingContext) for (var index = 0; index < valueDescriptors.Count; index++) { var valueDescriptor = valueDescriptors[index]; - var valueSource = GetValueSource(bindingSources, bindingContext, valueDescriptor); - var (boundValue, usedNonDefault) = GetBoundValue(valueSource, bindingContext, valueDescriptor, includeMissingValues, ModelDescriptor); + var valueSource = GetValueSource(bindingSources, bindingContext, valueDescriptor, enforceExplicitBinding); + var (boundValue, usedNonDefault) = + GetBoundValue(valueSource, bindingContext, valueDescriptor, includeMissingValues, parentType); if (usedNonDefault && !anyNonDefaults) { anyNonDefaults = true; @@ -157,10 +240,10 @@ private ConstructorAndArgs GetConstructorAndAgs(BindingContext bindingContext) return (values, anyNonDefaults); } - private IValueSource GetValueSource( - IDictionary? bindingSources, - BindingContext bindingContext, - IValueDescriptor valueDescriptor) + internal static IValueSource GetValueSource(IDictionary? bindingSources, + BindingContext bindingContext, + IValueDescriptor valueDescriptor, + bool enforceExplicitBinding) { if (!(bindingSources is null) && bindingSources.TryGetValue(valueDescriptor, out IValueSource? valueSource)) @@ -173,7 +256,7 @@ private IValueSource GetValueSource( return valueSource; } - if (!EnforceExplicitBinding) + if (!enforceExplicitBinding) { // Return a value source that will match from the parseResult // by name and type (or a possible conversion) @@ -188,13 +271,12 @@ internal static (BoundValue? boundValue, bool usedNonDefault) GetBoundValue( BindingContext bindingContext, IValueDescriptor valueDescriptor, bool includeMissingValues, - ModelDescriptor? modelDescriptor = null) + Type parentType) { - BoundValue? boundValue; if (bindingContext.TryBindToScalarValue( valueDescriptor, valueSource, - out boundValue)) + out var boundValue)) { return (boundValue, true); } @@ -204,16 +286,14 @@ internal static (BoundValue? boundValue, bool usedNonDefault) GetBoundValue( return (BoundValue.DefaultForValueDescriptor(valueDescriptor), false); } - if (!(modelDescriptor is null) && valueDescriptor.ValueType == modelDescriptor.ModelType) + if (!(valueDescriptor.ValueType == parentType)) // Recursive models aren't allowed { - throw new NotImplementedException("Recursive models are not allowed."); - } - var binder = bindingContext.GetModelBinder(valueDescriptor); - // if there are constructors with parameters, we will try to bind - var (success, newInstance, usedNonDefaults) = binder.CreateInstanceInternal(bindingContext, false); - if (success && usedNonDefaults) - { - return (new BoundValue(newInstance, valueDescriptor, valueSource), true); + var binder = bindingContext.GetModelBinder(valueDescriptor); + var (success, newInstance, usedNonDefaults) = binder.CreateInstanceInternal(bindingContext, false); + if (success) + { + return (new BoundValue(newInstance, valueDescriptor, valueSource), usedNonDefaults); + } } if (includeMissingValues) @@ -290,7 +370,7 @@ public int GetHashCode(ParameterDescriptor obj) } } - private class AnonymousValueDescriptor : IValueDescriptor + internal class AnonymousValueDescriptor : IValueDescriptor { public Type ValueType { get; } @@ -309,7 +389,7 @@ public AnonymousValueDescriptor(Type modelType) } } - internal struct ConstructorAndArgs + internal class ConstructorAndArgs { public ConstructorDescriptor? Constructor { get; } public IReadOnlyList? BoundValues { get; } diff --git a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs index 7811cea159..c830e0c468 100644 --- a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs +++ b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs @@ -17,60 +17,76 @@ internal class ModelBindingCommandHandler : ICommandHandler private readonly ModelBinder? _invocationTargetBinder; private readonly MethodInfo? _handlerMethodInfo; private readonly IReadOnlyList _parameterDescriptors; + private readonly IMethodDescriptor _methodDescriptor; private Dictionary _invokeArgumentBindingSources { get; } = new Dictionary(); + private bool EnforceExplicitBinding = false; // Wrong formatting as hint to figure out how to set this public ModelBindingCommandHandler( MethodInfo handlerMethodInfo, - IReadOnlyList parameterDescriptors) + IMethodDescriptor methodDescriptor) { _handlerMethodInfo = handlerMethodInfo ?? throw new ArgumentNullException(nameof(handlerMethodInfo)); _invocationTargetBinder = _handlerMethodInfo.IsStatic ? null : new ModelBinder(_handlerMethodInfo.DeclaringType); - _parameterDescriptors = parameterDescriptors ?? throw new ArgumentNullException(nameof(parameterDescriptors)); + _methodDescriptor = methodDescriptor ?? throw new ArgumentNullException(nameof(methodDescriptor)); + _parameterDescriptors = methodDescriptor.ParameterDescriptors ; } public ModelBindingCommandHandler( MethodInfo handlerMethodInfo, - IReadOnlyList parameterDescriptors, + IMethodDescriptor methodDescriptor, object? invocationTarget) + :this(handlerMethodInfo, methodDescriptor ) { _invocationTarget = invocationTarget; - _handlerMethodInfo = handlerMethodInfo ?? throw new ArgumentNullException(nameof(handlerMethodInfo)); - _parameterDescriptors = parameterDescriptors ?? throw new ArgumentNullException(nameof(parameterDescriptors)); } public ModelBindingCommandHandler( - Delegate handlerDelegate, - IReadOnlyList parameterDescriptors) + Delegate handlerDelegate, + IMethodDescriptor methodDescriptor) { _handlerDelegate = handlerDelegate ?? throw new ArgumentNullException(nameof(handlerDelegate)); - _parameterDescriptors = parameterDescriptors ?? throw new ArgumentNullException(nameof(parameterDescriptors)); + _methodDescriptor = methodDescriptor ?? throw new ArgumentNullException(nameof(methodDescriptor)); } public async Task InvokeAsync(InvocationContext context) { var bindingContext = context.BindingContext; - var invocationArguments = new object?[_parameterDescriptors.Count()]; - var length = _parameterDescriptors.Count(); + var (boundValues, _) = ModelBinder.GetBoundValues( + _invokeArgumentBindingSources, + bindingContext, + _methodDescriptor.ParameterDescriptors, + _methodDescriptor.Parent?.ModelType ?? typeof(object), + EnforceExplicitBinding, + true); + var invocationArguments = boundValues + .Select(x => x.Value) + .ToArray(); - for (int i = 0; i < length; i++) - { - var paramDesc = _parameterDescriptors[i]; - if (_invokeArgumentBindingSources.TryGetValue(paramDesc, out var valueSource)) - { - var (boundValue, _) = ModelBinder.GetBoundValue(valueSource, bindingContext, paramDesc, true); - if (!(boundValue is null)) - { - invocationArguments[i] = boundValue.Value; - continue; - } - } - var binder = bindingContext.GetModelBinder(paramDesc); - invocationArguments[i] = binder.CreateInstance(bindingContext); - } + //var invocationArguments = new object?[_parameterDescriptors.Count()]; + //var length = _parameterDescriptors.Count(); + + //for (int i = 0; i < length; i++) + //{ + // var paramDesc = _parameterDescriptors[i]; + // var binder = bindingContext.GetModelBinder(paramDesc); + // IValueSource? valueSource; + // if (!_invokeArgumentBindingSources.TryGetValue(paramDesc, out valueSource)) + // { + // valueSource = binder.GetValueSource(_invokeArgumentBindingSources, bindingContext, paramDesc); + // } + // var (boundValue, _) = ModelBinder.GetBoundValue(valueSource, bindingContext, paramDesc, true, binder.ModelDescriptor); + // if (!(boundValue is null)) + // { + // invocationArguments[i] = boundValue.Value; + // continue; + // } + + // invocationArguments[i] = binder.CreateInstance(bindingContext); + //} object result; if (_handlerDelegate is null) From cb6791ca88b415384d02fa5e1d5020c33a852157 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Sun, 9 Aug 2020 11:27:42 -0700 Subject: [PATCH 07/12] Small refactoring and removing old code --- src/System.CommandLine/Binding/ModelBinder.cs | 438 +++++++++++------- .../Binding/ModelBinder2.cs | 405 ---------------- .../Invocation/ModelBindingCommandHandler.cs | 35 +- 3 files changed, 264 insertions(+), 614 deletions(-) delete mode 100644 src/System.CommandLine/Binding/ModelBinder2.cs diff --git a/src/System.CommandLine/Binding/ModelBinder.cs b/src/System.CommandLine/Binding/ModelBinder.cs index 51f8bff772..bbc954b777 100644 --- a/src/System.CommandLine/Binding/ModelBinder.cs +++ b/src/System.CommandLine/Binding/ModelBinder.cs @@ -1,34 +1,28 @@ -// Copyright (c) .NET Foundation and contributors. All rights reserved. -// Licensed under the MIT license. See LICENSE file in the project root for full license information. - -using System.Collections.Generic; +using System.Collections.Generic; +using System.ComponentModel; using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Reflection; +using System.Runtime.CompilerServices; +using System.Text.RegularExpressions; +using System.Threading; namespace System.CommandLine.Binding { - public class ModelBinder3 + public class ModelBinder { - public ModelBinder3(Type modelType) : this(new AnonymousValueDescriptor(modelType)) - { - if (modelType is null) - { - throw new ArgumentNullException(nameof(modelType)); - } - } + public ModelBinder(Type modelType) + : this(new AnonymousValueDescriptor(modelType)) + => _ = modelType ?? throw new ArgumentNullException(nameof(modelType)); - internal ModelBinder3(IValueDescriptor valueDescriptor) + internal ModelBinder(IValueDescriptor valueDescriptor) { ValueDescriptor = valueDescriptor ?? throw new ArgumentNullException(nameof(valueDescriptor)); - ModelDescriptor = ModelDescriptor.FromType(valueDescriptor.ValueType); } - public ModelDescriptor ModelDescriptor { get; } - public IValueDescriptor ValueDescriptor { get; } - + public ModelDescriptor ModelDescriptor { get; } public bool EnforceExplicitBinding { get; set; } internal Dictionary ConstructorArgumentBindingSources { get; } = @@ -37,195 +31,192 @@ internal ModelBinder3(IValueDescriptor valueDescriptor) internal Dictionary MemberBindingSources { get; } = new Dictionary(); - protected ConstructorDescriptor FindModelConstructorDescriptor( - ConstructorInfo constructorInfo) - { - var cmpCtorDesc = new ConstructorDescriptor(constructorInfo, - // Parent does not matter for comparison and can be invalid. - parent: ModelDescriptor); - var cmpParamDescs = cmpCtorDesc.ParameterDescriptors - .Select(GetParameterDescriptorComparands) - .ToList(); - - return ModelDescriptor.ConstructorDescriptors - .FirstOrDefault(matchCtorDesc => - { - if (matchCtorDesc.Parent.ModelType != constructorInfo.DeclaringType) - return false; - return matchCtorDesc.ParameterDescriptors - .Select(GetParameterDescriptorComparands) - .SequenceEqual(cmpParamDescs); - }); - - // Name matching is not necessary for overload descisions. - static (Type paramType, bool allowNull, bool hasDefaultValue) - GetParameterDescriptorComparands(ParameterDescriptor desc) => - (desc.ValueType, desc.AllowsNull, desc.HasDefaultValue); - } - - protected IValueDescriptor FindModelPropertyDescriptor( - Type propertyType, string propertyName) - { - return ModelDescriptor.PropertyDescriptors - .FirstOrDefault(desc => - desc.ValueType == propertyType && - string.Equals(desc.ValueName, propertyName, StringComparison.Ordinal) - ); - } - - public void BindConstructorArgumentFromValue(ParameterInfo parameter, - IValueDescriptor valueDescriptor) + // Consider deprecating in favor or BindingConfiguration/BindingContext attach validatation. Then make internal. + // Or at least rename to "ConfigureBinding" or similar + public void BindConstructorArgumentFromValue(ParameterInfo parameter, IValueDescriptor valueDescriptor) { - if (!(parameter.Member is ConstructorInfo constructor)) - throw new ArgumentException(paramName: nameof(parameter), - message: "Parameter must be declared on a constructor."); - + var constructor = FindConstructorOrThrow(parameter, "Parameter must be declared on a constructor."); var ctorDesc = FindModelConstructorDescriptor(constructor); + if (ctorDesc is null) throw new ArgumentException(paramName: nameof(parameter), message: "Parameter is not described by any of the model constructor descriptors."); var paramDesc = ctorDesc.ParameterDescriptors[parameter.Position]; - ConstructorArgumentBindingSources[paramDesc] = - new SpecificSymbolValueSource(valueDescriptor); + ConstructorArgumentBindingSources[paramDesc] = new SpecificSymbolValueSource(valueDescriptor); } - public void BindMemberFromValue(PropertyInfo property, - IValueDescriptor valueDescriptor) + public void BindMemberFromValue(PropertyInfo property, IValueDescriptor valueDescriptor) { - var propertyDescriptor = FindModelPropertyDescriptor( - property.PropertyType, property.Name); + var propertyDescriptor = FindModelPropertyDescriptor(property.PropertyType, property.Name); + if (propertyDescriptor is null) throw new ArgumentException(paramName: nameof(property), message: "Property is not described by any of the model property descriptors."); - MemberBindingSources[propertyDescriptor] = - new SpecificSymbolValueSource(valueDescriptor); + MemberBindingSources[propertyDescriptor] = new SpecificSymbolValueSource(valueDescriptor); } - public object? CreateInstance(BindingContext context) + public object? CreateInstance(BindingContext bindingContext) { - var values = GetValues( - // No binding sources, as were are attempting to bind a value - // for the model itself, not for its ctor args or its members. - bindingSources: null, - bindingContext: context, - new[] { ValueDescriptor }, - includeMissingValues: false); + var (_, newInstance, _) = CreateInstanceInternal(bindingContext); + return newInstance; + } - if (values.Count == 1 && - ModelDescriptor.ModelType.IsAssignableFrom(values[0].ValueDescriptor.ValueType)) + private (bool success, object? newInstance, bool anyNonDefaults) CreateInstanceInternal( + BindingContext bindingContext) + { + if (DisallowedBindingType()) { - return values[0].Value; + throw new InvalidOperationException($"The type {ModelDescriptor.ModelType} cannot be bound"); } - - if (TryDefaultConstructorAndPropertiesStrategy(context, out var fromCtor)) + if (ShortCutTheBinding(bindingContext)) { - return fromCtor; + return GetSimpleModelValue(MemberBindingSources, bindingContext); } - - return values.SingleOrDefault()?.Value; + var constructorAndArgs = GetBestConstructorAndArgs(bindingContext); + var constructor = constructorAndArgs.Constructor; + var boundValues = constructorAndArgs.BoundValues; + bool nonDefaultsUsed = constructorAndArgs.NonDefaultsUsed; + return constructor is null + ? GetSimpleModelValue(ConstructorArgumentBindingSources, bindingContext) + : InstanceFromSpecificConstructor(bindingContext, constructor, boundValues, ref nonDefaultsUsed); } - private bool TryDefaultConstructorAndPropertiesStrategy( - BindingContext context, - [NotNullWhen(true)] out object? instance) + private bool DisallowedBindingType() { - var constructorDescriptors = - ModelDescriptor - .ConstructorDescriptors - .OrderByDescending(d => d.ParameterDescriptors.Count); - - var (constructor, boundConstructorArguments) = FindConstructor(constructorDescriptors, context); - - if (!(constructor is null)) + var disallowedTypes = new List { - var values = boundConstructorArguments.Select(v => v.Value).ToArray(); - - try - { - var fromModelBinder = constructor.Invoke(values); - - UpdateInstance(fromModelBinder, context); - - instance = fromModelBinder; + typeof(Span<>), + typeof(ReadOnlySpan<>) + }; + var type = ModelDescriptor.ModelType; + return disallowedTypes + .Any(x => type.IsGenericType && (type.GetGenericTypeDefinition() == x)); + } - return true; - } - catch - { - // continue to failure exit - } + private bool ShortCutTheBinding(BindingContext bindingContext) + { + var explicitTypesToShortcut = new List + { + typeof(string) + }; + Type modelType = ModelDescriptor.ModelType; + return modelType.IsPrimitive || + IsNullable(modelType) || + explicitTypesToShortcut.Contains(modelType); + + static bool IsNullable(Type type) + { + return type.IsGenericType && + type.GetGenericTypeDefinition() == typeof(Nullable<>); } - instance = null; - return false; + } + private (bool success, object? newInstance, bool anyNonDefaults) GetSimpleModelValue( + IDictionary? bindingSources, BindingContext bindingContext) + { + var valueSource = GetValueSource(bindingSources, bindingContext, ValueDescriptor, EnforceExplicitBinding); + return bindingContext.TryBindToScalarValue(ValueDescriptor, + valueSource, + out var boundValue) + ? (true, boundValue?.Value, true) + : (false,(object?) null, false); } - private (ConstructorDescriptor?, IReadOnlyList?) FindConstructor(IOrderedEnumerable constructorDescriptors, BindingContext context) + private (bool success, object? newInstance, bool anyNonDefaults) InstanceFromSpecificConstructor( + BindingContext bindingContext, ConstructorDescriptor constructor, IReadOnlyList? boundValues, ref bool nonDefaultsUsed) { - foreach (var constructor in constructorDescriptors) + var values = boundValues.Select(x => x.Value).ToArray(); + object? newInstance = null; + try { - var boundConstructorArguments = GetValues( - ConstructorArgumentBindingSources, - context, - constructor.ParameterDescriptors, - true); - - if (boundConstructorArguments.Count == constructor.ParameterDescriptors.Count) - { - return (constructor, boundConstructorArguments); - } + newInstance = constructor.Invoke(values); } - return (null, null); + catch + { + return (false, null, false); + } + if (!(newInstance is null)) + { + nonDefaultsUsed = UpdateInstanceInternalNotifyIfNonDefaultsUsed(newInstance, bindingContext); + } + return (true, newInstance, nonDefaultsUsed); } public void UpdateInstance(T instance, BindingContext bindingContext) + => UpdateInstanceInternalNotifyIfNonDefaultsUsed(instance, bindingContext); + + private bool UpdateInstanceInternalNotifyIfNonDefaultsUsed(T instance, BindingContext bindingContext) { - var boundValues = GetValues( + var (boundValues, anyNonDefaults) = GetBoundValues( MemberBindingSources, bindingContext, ModelDescriptor.PropertyDescriptors, + EnforceExplicitBinding, + ModelDescriptor.ModelType, includeMissingValues: false); foreach (var boundValue in boundValues) { ((PropertyDescriptor)boundValue.ValueDescriptor).SetValue(instance, boundValue.Value); } + + return anyNonDefaults; } - private IReadOnlyList GetValues( - IDictionary? bindingSources, - BindingContext bindingContext, - IReadOnlyList valueDescriptors, - bool includeMissingValues) + private ConstructorAndArgs GetBestConstructorAndArgs(BindingContext bindingContext) + { + var constructorDescriptors = + ModelDescriptor + .ConstructorDescriptors + .OrderByDescending(d => d.ParameterDescriptors.Count); + ConstructorAndArgs? bestNonMatching = null; + foreach (var constructor in constructorDescriptors) + { + var (boundValues, anyNonDefaults) = GetBoundValues( + ConstructorArgumentBindingSources, + bindingContext, + constructor.ParameterDescriptors, + EnforceExplicitBinding, + ModelDescriptor.ModelType, + true); + + if (boundValues.Count == constructor.ParameterDescriptors.Count) + { + var match = new ConstructorAndArgs(constructor, boundValues, anyNonDefaults); + if (anyNonDefaults) + { // based on parameter length, first usable constructor that utilizes CLI definition + return match; + } + bestNonMatching ??= match; + } + } + return bestNonMatching is null + ? new ConstructorAndArgs(null, null, false) + : bestNonMatching; + } + + internal static (IReadOnlyList boundValues, bool anyNonDefaults) GetBoundValues( + IDictionary? bindingSources, + BindingContext bindingContext, + IReadOnlyList valueDescriptors, + bool enforceExplicitBinding, + Type? parentType = null, + bool includeMissingValues = true) { var values = new List(); + var anyNonDefaults = false; for (var index = 0; index < valueDescriptors.Count; index++) { var valueDescriptor = valueDescriptors[index]; - - var valueSource = GetValueSource(bindingSources, bindingContext, valueDescriptor); - - BoundValue? boundValue = GetBoundValue(valueSource, bindingContext, valueDescriptor); - - if (boundValue is null) + var valueSource = GetValueSource(bindingSources, bindingContext, valueDescriptor, enforceExplicitBinding); + var (boundValue, usedNonDefault) = + GetBoundValue(valueSource, bindingContext, valueDescriptor, includeMissingValues, parentType); + if (usedNonDefault && !anyNonDefaults) { - var binder = bindingContext.GetModelBinder(valueDescriptor); - var value = binder.CreateInstance(bindingContext); - if (includeMissingValues) - { - if (valueDescriptor is ParameterDescriptor parameterDescriptor && - parameterDescriptor.Parent is ConstructorDescriptor constructorDescriptor) - { - if (parameterDescriptor.HasDefaultValue) - boundValue = BoundValue.DefaultForValueDescriptor(parameterDescriptor); - else if (parameterDescriptor.AllowsNull && - ShouldPassNullToConstructor(constructorDescriptor.Parent, constructorDescriptor)) - boundValue = BoundValue.DefaultForType(valueDescriptor); - } - } + anyNonDefaults = true; } if (boundValue != null) { @@ -233,28 +224,13 @@ private IReadOnlyList GetValues( } } - return values; - } - - internal static BoundValue? GetBoundValue(IValueSource valueSource, BindingContext bindingContext, - IValueDescriptor valueDescriptor) - { - BoundValue? boundValue; - if (!bindingContext.TryBindToScalarValue( - valueDescriptor, - valueSource, - out boundValue) && valueDescriptor.HasDefaultValue) - { - boundValue = BoundValue.DefaultForValueDescriptor(valueDescriptor); - } - - return boundValue; + return (values, anyNonDefaults); } - private IValueSource GetValueSource( - IDictionary? bindingSources, - BindingContext bindingContext, - IValueDescriptor valueDescriptor) + internal static IValueSource GetValueSource(IDictionary? bindingSources, + BindingContext bindingContext, + IValueDescriptor valueDescriptor, + bool enforceExplicitBinding) { if (!(bindingSources is null) && bindingSources.TryGetValue(valueDescriptor, out IValueSource? valueSource)) @@ -267,7 +243,7 @@ private IValueSource GetValueSource( return valueSource; } - if (!EnforceExplicitBinding) + if (!enforceExplicitBinding) { // Return a value source that will match from the parseResult // by name and type (or a possible conversion) @@ -277,21 +253,111 @@ private IValueSource GetValueSource( return new MissingValueSource(); } - public override string ToString() => - $"{ModelDescriptor.ModelType.Name}"; + internal static (BoundValue? boundValue, bool usedNonDefault) GetBoundValue( + IValueSource valueSource, + BindingContext bindingContext, + IValueDescriptor valueDescriptor, + bool includeMissingValues, + Type? parentType) + { + if (bindingContext.TryBindToScalarValue( + valueDescriptor, + valueSource, + out var boundValue)) + { + return (boundValue, true); + } + + if (valueDescriptor.HasDefaultValue) + { + return (BoundValue.DefaultForValueDescriptor(valueDescriptor), false); + } + + if (valueDescriptor.ValueType != parentType) // Recursive models aren't allowed + { + var binder = bindingContext.GetModelBinder(valueDescriptor); + var (success, newInstance, usedNonDefaults) = binder.CreateInstanceInternal(bindingContext); + if (success) + { + return (new BoundValue(newInstance, valueDescriptor, valueSource), usedNonDefaults); + } + } + + if (includeMissingValues) + { + if (valueDescriptor is ParameterDescriptor parameterDescriptor && parameterDescriptor.AllowsNull) + { + return (new BoundValue(parameterDescriptor.GetDefaultValue(), valueDescriptor, valueSource), false); + } + // Logic dropped here - misnamed and purpose unclear: ShouldPassNullToConstructor(constructorDescriptor.Parent, constructorDescriptor)) + return (BoundValue.DefaultForType(valueDescriptor), false); + } + return (null, false); + } + + + protected ConstructorDescriptor FindModelConstructorDescriptor(ConstructorInfo constructorInfo) + { + var constructorParameters = constructorInfo.GetParameters(); + + return ModelDescriptor.ConstructorDescriptors + .FirstOrDefault(ctorDesc + => ModelDescriptor.ModelType == constructorInfo.DeclaringType && + ctorDesc.ParameterDescriptors + .Any(x => constructorParameters.Any(y => MatchParameter(x, y)))); + + static bool MatchParameter(ParameterDescriptor desc, ParameterInfo info) + { + return desc.ValueType == info.ParameterType && + desc.ValueName == info.Name && + desc.HasDefaultValue == info.HasDefaultValue && + desc.AllowsNull == ParameterDescriptor.CalculateAllowsNull(info); + } + } + + protected IValueDescriptor FindModelPropertyDescriptor(Type propertyType, string propertyName) + { + return ModelDescriptor.PropertyDescriptors + .FirstOrDefault(desc => + desc.ValueType == propertyType && + string.Equals(desc.ValueName, propertyName, StringComparison.Ordinal) + ); + } + + + + + + - private static bool ShouldPassNullToConstructor(ModelDescriptor modelDescriptor, - ConstructorDescriptor? ctor = null) + + + private ConstructorInfo FindConstructorOrThrow(ParameterInfo parameter, string message) + { + if (!(parameter.Member is ConstructorInfo constructor)) + { + throw new ArgumentException(paramName: nameof(parameter), + message: message); + } + return constructor; + } + + private class ConstructorDescriptorEquality : IEqualityComparer { - if (!(ctor is null)) + public bool Equals(ParameterDescriptor x, ParameterDescriptor y) { - return ctor.ParameterDescriptors.All(d => d.AllowsNull); + return x.ValueType == x.ValueType && + x.AllowsNull == x.AllowsNull && + x.HasDefaultValue == x.HasDefaultValue; } - return !modelDescriptor.ModelType.IsNullable(); + public int GetHashCode(ParameterDescriptor obj) + { + throw new NotImplementedException(); + } } - private class AnonymousValueDescriptor : IValueDescriptor + internal class AnonymousValueDescriptor : IValueDescriptor { public Type ValueType { get; } @@ -309,4 +375,18 @@ public AnonymousValueDescriptor(Type modelType) public override string ToString() => $"{ValueType}"; } } + + internal class ConstructorAndArgs + { + public ConstructorDescriptor? Constructor { get; } + public IReadOnlyList? BoundValues { get; } + public bool NonDefaultsUsed { get; } + + public ConstructorAndArgs(ConstructorDescriptor? constructor, IReadOnlyList? boundValues, bool nonDefaultsUsed) + { + Constructor = constructor; + BoundValues = boundValues; + NonDefaultsUsed = nonDefaultsUsed; + } + } } diff --git a/src/System.CommandLine/Binding/ModelBinder2.cs b/src/System.CommandLine/Binding/ModelBinder2.cs deleted file mode 100644 index b2cc2f7e6a..0000000000 --- a/src/System.CommandLine/Binding/ModelBinder2.cs +++ /dev/null @@ -1,405 +0,0 @@ -using System.Collections.Generic; -using System.ComponentModel; -using System.Diagnostics.CodeAnalysis; -using System.Linq; -using System.Reflection; -using System.Runtime.CompilerServices; -using System.Text.RegularExpressions; -using System.Threading; - -namespace System.CommandLine.Binding -{ - public class ModelBinder - { - public ModelBinder(Type modelType) - : this(new AnonymousValueDescriptor(modelType)) - => _ = modelType ?? throw new ArgumentNullException(nameof(modelType)); - - internal ModelBinder(IValueDescriptor valueDescriptor) - { - ValueDescriptor = valueDescriptor ?? throw new ArgumentNullException(nameof(valueDescriptor)); - ModelDescriptor = ModelDescriptor.FromType(valueDescriptor.ValueType); - } - - public IValueDescriptor ValueDescriptor { get; } - public ModelDescriptor ModelDescriptor { get; } - public bool EnforceExplicitBinding { get; set; } - - internal Dictionary ConstructorArgumentBindingSources { get; } = - new Dictionary(); - - internal Dictionary MemberBindingSources { get; } = - new Dictionary(); - - // Consider deprecating in favor or BindingConfiguration/BindingContext attach validatation. Then make internal. - // Or at least rename to "ConfigureBinding" or similar - public void BindConstructorArgumentFromValue(ParameterInfo parameter, IValueDescriptor valueDescriptor) - { - var constructor = FindConstructorOrThrow(parameter, "Parameter must be declared on a constructor."); - var ctorDesc = FindModelConstructorDescriptor(constructor); - - if (ctorDesc is null) - throw new ArgumentException(paramName: nameof(parameter), - message: "Parameter is not described by any of the model constructor descriptors."); - - var paramDesc = ctorDesc.ParameterDescriptors[parameter.Position]; - ConstructorArgumentBindingSources[paramDesc] = new SpecificSymbolValueSource(valueDescriptor); - } - - public void BindMemberFromValue(PropertyInfo property, IValueDescriptor valueDescriptor) - { - var propertyDescriptor = FindModelPropertyDescriptor(property.PropertyType, property.Name); - - if (propertyDescriptor is null) - throw new ArgumentException(paramName: nameof(property), - message: "Property is not described by any of the model property descriptors."); - - MemberBindingSources[propertyDescriptor] = new SpecificSymbolValueSource(valueDescriptor); - } - - public object? CreateInstance(BindingContext bindingContext) - { - var (_, newInstance, _) = CreateInstanceInternal(bindingContext, true); - return newInstance; - } - - private (bool success, object? newInstance, bool anyNonDefaults) CreateInstanceInternal( - BindingContext bindingContext, - bool throwIfNoConstructor) - { - if (DisallowedBindingType()) - { - throw new InvalidOperationException($"The type {ModelDescriptor.ModelType} cannot be bound"); - } - if (CanShortCut(bindingContext)) - { - return GetSimpleModelValue(MemberBindingSources, bindingContext); - } - var constructorAndArgs = GetConstructorAndArgs(bindingContext); - var constructor = constructorAndArgs.Constructor; - var boundValues = constructorAndArgs.BoundValues; - bool nonDefaultsUsed = constructorAndArgs.NonDefaultsUsed; - if (constructor is null) - { - return GetSimpleModelValue(ConstructorArgumentBindingSources, bindingContext); - //var valueSource = GetValueSource(ConstructorArgumentBindingSources, bindingContext, ValueDescriptor); - //var (boundValue, usedNonDefault) = GetBoundValue(valueSource, bindingContext, ValueDescriptor, true, ModelDescriptor); - //return boundValue is null - // ? (false, (object?)null, false) - // : (true, boundValue.Value, usedNonDefault); - } - - return InstanceFromSpecificConstructor(bindingContext, constructor, boundValues, ref nonDefaultsUsed); - } - - private bool DisallowedBindingType() - { - var disallowedTypes = new List - { - typeof(Span<>), - typeof(ReadOnlySpan<>) - }; - var type = ModelDescriptor.ModelType; - return disallowedTypes - .Any(x => type.IsGenericType && (type.GetGenericTypeDefinition() == x)); - } - - private bool CanShortCut(BindingContext bindingContext) - { - var explicitTypesToShortcut = new List - { - typeof(string) - }; - Type modelType = ModelDescriptor.ModelType; - return modelType.IsPrimitive || - IsNullable(modelType) || - explicitTypesToShortcut.Contains(modelType); - - static bool IsNullable(Type type) - { - return type.IsGenericType && - type.GetGenericTypeDefinition() == typeof(Nullable<>); - } - } - - private (bool success, object? newInstance, bool anyNonDefaults) GetSimpleModelValue( - IDictionary? bindingSources, BindingContext bindingContext) - { - var valueSource = GetValueSource(bindingSources, bindingContext, ValueDescriptor, EnforceExplicitBinding); - if (bindingContext.TryBindToScalarValue( - ValueDescriptor, - valueSource, - out var boundValue)) - { - return (true, boundValue?.Value, true); - } - return (false, null, false); - - } - - private (bool success, object newInstance, bool anyNonDefaults) InstanceFromSpecificConstructor(BindingContext bindingContext, ConstructorDescriptor? constructor, IReadOnlyList? boundValues, ref bool nonDefaultsUsed) - { - var values = boundValues.Select(x => x.Value).ToArray(); - object? newInstance = null; - try - { - newInstance = constructor.Invoke(values); - } - catch - { - return (false, null, false); - } - if (!(newInstance is null)) - { - nonDefaultsUsed = UpdateInstanceInternalNotifyIfNonDefaultsUsed(newInstance, bindingContext); - } - return (true, newInstance, nonDefaultsUsed); - } - - public void UpdateInstance(T instance, BindingContext bindingContext) - => UpdateInstanceInternalNotifyIfNonDefaultsUsed(instance, bindingContext); - - private bool UpdateInstanceInternalNotifyIfNonDefaultsUsed(T instance, BindingContext bindingContext) - { - var (boundValues, anyNonDefaults) = GetBoundValues( - MemberBindingSources, - bindingContext, - ModelDescriptor.PropertyDescriptors, - ModelDescriptor.ModelType, - EnforceExplicitBinding, - includeMissingValues: false); - - foreach (var boundValue in boundValues) - { - ((PropertyDescriptor)boundValue.ValueDescriptor).SetValue(instance, boundValue.Value); - } - - return anyNonDefaults; - } - - private ConstructorAndArgs GetConstructorAndArgs(BindingContext bindingContext) - { - var constructorDescriptors = - ModelDescriptor - .ConstructorDescriptors - .OrderByDescending(d => d.ParameterDescriptors.Count); - ConstructorAndArgs? bestNonMatching = null; - foreach (var constructor in constructorDescriptors) - { - var (boundValues, anyNonDefaults) = GetBoundValues( - ConstructorArgumentBindingSources, - bindingContext, - constructor.ParameterDescriptors, - ModelDescriptor.ModelType, - EnforceExplicitBinding, - true); - - if (boundValues.Count == constructor.ParameterDescriptors.Count) - { - var match = new ConstructorAndArgs(constructor, boundValues, anyNonDefaults); - if (anyNonDefaults) - { // based on parameter length, first usable constructor that utilizes CLI definition - return match; - } - bestNonMatching ??= match; - } - } - return bestNonMatching is null - ? new ConstructorAndArgs(null, null, false) - : bestNonMatching; - } - - - internal static (IReadOnlyList boundValues, bool anyNonDefaults) GetBoundValues( - IDictionary? bindingSources, - BindingContext bindingContext, - IReadOnlyList valueDescriptors, - Type parentType, - bool enforceExplicitBinding, - bool includeMissingValues) - { - var values = new List(); - var anyNonDefaults = false; - - for (var index = 0; index < valueDescriptors.Count; index++) - { - var valueDescriptor = valueDescriptors[index]; - var valueSource = GetValueSource(bindingSources, bindingContext, valueDescriptor, enforceExplicitBinding); - var (boundValue, usedNonDefault) = - GetBoundValue(valueSource, bindingContext, valueDescriptor, includeMissingValues, parentType); - if (usedNonDefault && !anyNonDefaults) - { - anyNonDefaults = true; - } - if (boundValue != null) - { - values.Add(boundValue); - } - } - - return (values, anyNonDefaults); - } - - internal static IValueSource GetValueSource(IDictionary? bindingSources, - BindingContext bindingContext, - IValueDescriptor valueDescriptor, - bool enforceExplicitBinding) - { - if (!(bindingSources is null) && - bindingSources.TryGetValue(valueDescriptor, out IValueSource? valueSource)) - { - return valueSource; - } - - if (bindingContext.TryGetValueSource(valueDescriptor, out valueSource)) - { - return valueSource; - } - - if (!enforceExplicitBinding) - { - // Return a value source that will match from the parseResult - // by name and type (or a possible conversion) - return new ParseResultMatchingValueSource(); - } - - return new MissingValueSource(); - } - - internal static (BoundValue? boundValue, bool usedNonDefault) GetBoundValue( - IValueSource valueSource, - BindingContext bindingContext, - IValueDescriptor valueDescriptor, - bool includeMissingValues, - Type parentType) - { - if (bindingContext.TryBindToScalarValue( - valueDescriptor, - valueSource, - out var boundValue)) - { - return (boundValue, true); - } - - if (valueDescriptor.HasDefaultValue) - { - return (BoundValue.DefaultForValueDescriptor(valueDescriptor), false); - } - - if (!(valueDescriptor.ValueType == parentType)) // Recursive models aren't allowed - { - var binder = bindingContext.GetModelBinder(valueDescriptor); - var (success, newInstance, usedNonDefaults) = binder.CreateInstanceInternal(bindingContext, false); - if (success) - { - return (new BoundValue(newInstance, valueDescriptor, valueSource), usedNonDefaults); - } - } - - if (includeMissingValues) - { - if (valueDescriptor is ParameterDescriptor parameterDescriptor && parameterDescriptor.AllowsNull) - { - return (new BoundValue(parameterDescriptor.GetDefaultValue(), valueDescriptor, valueSource), false); - } - // Logic dropped here - misnamed and purpose unclear: ShouldPassNullToConstructor(constructorDescriptor.Parent, constructorDescriptor)) - return (BoundValue.DefaultForType(valueDescriptor), false); - } - return (null, false); - } - - - protected ConstructorDescriptor FindModelConstructorDescriptor(ConstructorInfo constructorInfo) - { - var constructorParameters = constructorInfo.GetParameters(); - - return ModelDescriptor.ConstructorDescriptors - .FirstOrDefault(ctorDesc - => ModelDescriptor.ModelType == constructorInfo.DeclaringType && - ctorDesc.ParameterDescriptors - .Any(x => constructorParameters.Any(y => MatchParameter(x, y)))); - - static bool MatchParameter(ParameterDescriptor desc, ParameterInfo info) - { - return desc.ValueType == info.ParameterType && - desc.ValueName == info.Name && - desc.HasDefaultValue == info.HasDefaultValue && - desc.AllowsNull == ParameterDescriptor.CalculateAllowsNull(info); - } - } - - protected IValueDescriptor FindModelPropertyDescriptor(Type propertyType, string propertyName) - { - return ModelDescriptor.PropertyDescriptors - .FirstOrDefault(desc => - desc.ValueType == propertyType && - string.Equals(desc.ValueName, propertyName, StringComparison.Ordinal) - ); - } - - - - - - - - - - private ConstructorInfo FindConstructorOrThrow(ParameterInfo parameter, string message) - { - if (!(parameter.Member is ConstructorInfo constructor)) - { - throw new ArgumentException(paramName: nameof(parameter), - message: message); - } - return constructor; - } - - private class ConstructorDescriptorEquality : IEqualityComparer - { - public bool Equals(ParameterDescriptor x, ParameterDescriptor y) - { - return x.ValueType == x.ValueType && - x.AllowsNull == x.AllowsNull && - x.HasDefaultValue == x.HasDefaultValue; - } - - public int GetHashCode(ParameterDescriptor obj) - { - throw new NotImplementedException(); - } - } - - internal class AnonymousValueDescriptor : IValueDescriptor - { - public Type ValueType { get; } - - public AnonymousValueDescriptor(Type modelType) - { - ValueType = modelType; - } - - public string ValueName => ""; - - public bool HasDefaultValue => false; - - public object? GetDefaultValue() => null; - - public override string ToString() => $"{ValueType}"; - } - } - - internal class ConstructorAndArgs - { - public ConstructorDescriptor? Constructor { get; } - public IReadOnlyList? BoundValues { get; } - public bool NonDefaultsUsed { get; } - - public ConstructorAndArgs(ConstructorDescriptor? constructor, IReadOnlyList? boundValues, bool nonDefaultsUsed) - { - Constructor = constructor; - BoundValues = boundValues; - NonDefaultsUsed = nonDefaultsUsed; - } - } -} diff --git a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs index c830e0c468..666d4116cd 100644 --- a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs +++ b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs @@ -16,7 +16,6 @@ internal class ModelBindingCommandHandler : ICommandHandler private readonly object? _invocationTarget; private readonly ModelBinder? _invocationTargetBinder; private readonly MethodInfo? _handlerMethodInfo; - private readonly IReadOnlyList _parameterDescriptors; private readonly IMethodDescriptor _methodDescriptor; private Dictionary _invokeArgumentBindingSources { get; } = new Dictionary(); @@ -31,7 +30,6 @@ public ModelBindingCommandHandler( ? null : new ModelBinder(_handlerMethodInfo.DeclaringType); _methodDescriptor = methodDescriptor ?? throw new ArgumentNullException(nameof(methodDescriptor)); - _parameterDescriptors = methodDescriptor.ParameterDescriptors ; } public ModelBindingCommandHandler( @@ -59,35 +57,12 @@ public async Task InvokeAsync(InvocationContext context) _invokeArgumentBindingSources, bindingContext, _methodDescriptor.ParameterDescriptors, - _methodDescriptor.Parent?.ModelType ?? typeof(object), - EnforceExplicitBinding, - true); + EnforceExplicitBinding); + var invocationArguments = boundValues .Select(x => x.Value) .ToArray(); - //var invocationArguments = new object?[_parameterDescriptors.Count()]; - //var length = _parameterDescriptors.Count(); - - //for (int i = 0; i < length; i++) - //{ - // var paramDesc = _parameterDescriptors[i]; - // var binder = bindingContext.GetModelBinder(paramDesc); - // IValueSource? valueSource; - // if (!_invokeArgumentBindingSources.TryGetValue(paramDesc, out valueSource)) - // { - // valueSource = binder.GetValueSource(_invokeArgumentBindingSources, bindingContext, paramDesc); - // } - // var (boundValue, _) = ModelBinder.GetBoundValue(valueSource, bindingContext, paramDesc, true, binder.ModelDescriptor); - // if (!(boundValue is null)) - // { - // invocationArguments[i] = boundValue.Value; - // continue; - // } - - // invocationArguments[i] = binder.CreateInstance(bindingContext); - //} - object result; if (_handlerDelegate is null) { @@ -103,13 +78,13 @@ public async Task InvokeAsync(InvocationContext context) return await CommandHandler.GetResultCodeAsync(result, context); } - public void BindParameter(ParameterInfo param, Argument argument) + internal void BindParameter(ParameterInfo param, Argument argument) { var _ = argument ?? throw new InvalidOperationException("You must specify an argument to bind"); BindValueSource(param, new SpecificSymbolValueSource(argument)); } - public void BindParameter(ParameterInfo param, Option option) + internal void BindParameter(ParameterInfo param, Option option) { var _ = option ?? throw new InvalidOperationException("You must specify an argument to bind"); BindValueSource(param, new SpecificSymbolValueSource(option)); @@ -128,7 +103,7 @@ private void BindValueSource(ParameterInfo param, IValueSource valueSource) private ParameterDescriptor? FindParameterDescriptor(ParameterInfo? param) => param == null ? null - : _parameterDescriptors + : _methodDescriptor.ParameterDescriptors .FirstOrDefault(x => x.ValueName == param.Name && x.ValueType == param.ParameterType); } From 05a395b2832addfa99d8e219c63f9eabac414fad Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Thu, 13 Aug 2020 05:43:09 -0700 Subject: [PATCH 08/12] Cleanup adn remove dead code --- src/System.CommandLine/Binding/ModelBinder.cs | 24 ++++--------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/src/System.CommandLine/Binding/ModelBinder.cs b/src/System.CommandLine/Binding/ModelBinder.cs index bbc954b777..19f049f6e2 100644 --- a/src/System.CommandLine/Binding/ModelBinder.cs +++ b/src/System.CommandLine/Binding/ModelBinder.cs @@ -1,11 +1,6 @@ using System.Collections.Generic; -using System.ComponentModel; -using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Reflection; -using System.Runtime.CompilerServices; -using System.Text.RegularExpressions; -using System.Threading; namespace System.CommandLine.Binding { @@ -39,8 +34,10 @@ public void BindConstructorArgumentFromValue(ParameterInfo parameter, IValueDesc var ctorDesc = FindModelConstructorDescriptor(constructor); if (ctorDesc is null) + { throw new ArgumentException(paramName: nameof(parameter), message: "Parameter is not described by any of the model constructor descriptors."); + } var paramDesc = ctorDesc.ParameterDescriptors[parameter.Position]; ConstructorArgumentBindingSources[paramDesc] = new SpecificSymbolValueSource(valueDescriptor); @@ -51,8 +48,10 @@ public void BindMemberFromValue(PropertyInfo property, IValueDescriptor valueDes var propertyDescriptor = FindModelPropertyDescriptor(property.PropertyType, property.Name); if (propertyDescriptor is null) + { throw new ArgumentException(paramName: nameof(property), message: "Property is not described by any of the model property descriptors."); + } MemberBindingSources[propertyDescriptor] = new SpecificSymbolValueSource(valueDescriptor); } @@ -342,21 +341,6 @@ private ConstructorInfo FindConstructorOrThrow(ParameterInfo parameter, string m return constructor; } - private class ConstructorDescriptorEquality : IEqualityComparer - { - public bool Equals(ParameterDescriptor x, ParameterDescriptor y) - { - return x.ValueType == x.ValueType && - x.AllowsNull == x.AllowsNull && - x.HasDefaultValue == x.HasDefaultValue; - } - - public int GetHashCode(ParameterDescriptor obj) - { - throw new NotImplementedException(); - } - } - internal class AnonymousValueDescriptor : IValueDescriptor { public Type ValueType { get; } From 1fd7908219efcae8864c247fa11ad0069391d66e Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Fri, 14 Aug 2020 07:11:18 -0700 Subject: [PATCH 09/12] Updated tests. 3 questions: search // ?? --- .../ModelBindingCommandHandlerTests.cs | 102 +++++++++++++++--- src/System.CommandLine/Binding/ModelBinder.cs | 8 -- .../Invocation/ModelBindingCommandHandler.cs | 2 +- 3 files changed, 86 insertions(+), 26 deletions(-) diff --git a/src/System.CommandLine.Tests/Binding/ModelBindingCommandHandlerTests.cs b/src/System.CommandLine.Tests/Binding/ModelBindingCommandHandlerTests.cs index 1368ad302b..e913e5067a 100644 --- a/src/System.CommandLine.Tests/Binding/ModelBindingCommandHandlerTests.cs +++ b/src/System.CommandLine.Tests/Binding/ModelBindingCommandHandlerTests.cs @@ -155,7 +155,7 @@ public void When_name_is_not_among_aliases_then_binder_will_bind_option_by_name( receivedHeaders.Should().BeEquivalentTo("one", "two"); } - + [Theory] [InlineData(typeof(string), "hello", "hello")] [InlineData(typeof(int), "123", 123)] @@ -312,12 +312,12 @@ public void When_argument_type_is_more_specific_than_parameter_type_then_paramet [InlineData(typeof(FileInfo), true)] [InlineData(typeof(FileInfo[]), false)] [InlineData(typeof(FileInfo[]), true)] - + [InlineData(typeof(DirectoryInfo), false)] [InlineData(typeof(DirectoryInfo), true)] [InlineData(typeof(DirectoryInfo[]), false)] [InlineData(typeof(DirectoryInfo[]), true)] - + [InlineData(typeof(FileSystemInfo), true, nameof(ExistingFile))] [InlineData(typeof(FileSystemInfo), true, nameof(ExistingDirectory))] [InlineData(typeof(FileSystemInfo), true, nameof(NonexistentPathWithTrailingSlash))] @@ -350,13 +350,13 @@ public async Task Handler_method_receives_option_arguments_bound_to_the_specifie } else { - var createCaptureDelegate = GetType() - .GetMethod(nameof(CaptureDelegate), BindingFlags.NonPublic | BindingFlags.Static) - .MakeGenericMethod(testCase.ParameterType); + var createCaptureDelegate = GetType() + .GetMethod(nameof(CaptureDelegate), BindingFlags.NonPublic | BindingFlags.Static) + .MakeGenericMethod(testCase.ParameterType); - var @delegate = createCaptureDelegate.Invoke(null, null); + var @delegate = createCaptureDelegate.Invoke(null, null); - handler = CommandHandler.Create((dynamic) @delegate); + handler = CommandHandler.Create((dynamic)@delegate); } var command = new Command("command") @@ -383,14 +383,14 @@ public async Task Handler_method_receives_option_arguments_bound_to_the_specifie testCase.AssertBoundValue(boundValue); } - + [Fact] public async Task When_binding_fails_due_to_parameter_naming_mismatch_then_handler_is_called_and_no_error_is_produced() { string[] received = { "this should get overwritten" }; var o = new Option( - new[] { "-i" }, + new[] { "-i" }, "Path to an image or directory of supported images") { Argument = new Argument() @@ -459,7 +459,9 @@ public async Task Handler_method_receives_command_arguments_bound_to_the_specifi } [Theory] - [InlineData(typeof(ClassWithCtorParameter))] + [InlineData(typeof(int))] + [InlineData(typeof(string))] + [InlineData(typeof(bool))] [InlineData(typeof(ClassWithSetter))] [InlineData(typeof(ClassWithCtorParameter))] [InlineData(typeof(ClassWithSetter))] @@ -470,7 +472,7 @@ public async Task Handler_method_receives_command_arguments_bound_to_the_specifi [InlineData(typeof(int[]))] [InlineData(typeof(List))] public async Task Handler_method_receives_command_arguments_explicitly_bound_to_the_specified_type( - Type type) + Type type) { var c = BindingCases[type]; @@ -508,6 +510,62 @@ public async Task Handler_method_receives_command_arguments_explicitly_bound_to_ c.AssertBoundValue(boundValue); } + [Theory] + [InlineData(typeof(int))] + [InlineData(typeof(string))] + [InlineData(typeof(bool))] + [InlineData(typeof(ClassWithCtorParameter))] + [InlineData(typeof(ClassWithSetter))] + [InlineData(typeof(ClassWithCtorParameter))] + [InlineData(typeof(ClassWithSetter))] + [InlineData(typeof(FileInfo))] + [InlineData(typeof(FileInfo[]))] + [InlineData(typeof(string[]))] + [InlineData(typeof(List))] + [InlineData(typeof(int[]))] + [InlineData(typeof(List))] + public async Task Handler_method_receives_command_options_explicitly_bound_to_the_specified_type( + Type type) + { + var c = BindingCases[type]; + + var captureMethod = GetType() + .GetMethod(nameof(CaptureMethod), BindingFlags.NonPublic | BindingFlags.Static) + .MakeGenericMethod(c.ParameterType); + var parameter = captureMethod.GetParameters().First(); + + var handler = CommandHandler.Create(captureMethod); + + var option = new Option("--value") + { + Argument = new Argument + { + ArgumentType = c.ParameterType + } + }; + + var command = new Command( + "command") + { + option + }; + handler.BindParameter(parameter, option); + command.Handler = handler; + + var commandLine = $"--value {c.CommandLine}"; + var parseResult = command.Parse(commandLine); + + var invocationContext = new InvocationContext(parseResult); + + await handler.InvokeAsync(invocationContext); + + var boundValue = ((BoundValueCapturer)invocationContext.InvocationResult).BoundValue; + + boundValue.Should().BeOfType(c.ParameterType); + + c.AssertBoundValue(boundValue); + } + private static void CaptureMethod(T value, InvocationContext invocationContext) { invocationContext.InvocationResult = new BoundValueCapturer(value); @@ -537,10 +595,20 @@ public void Apply(InvocationContext context) internal static readonly BindingTestSet BindingCases = new BindingTestSet { + BindingTestCase.Create( + "123", + o => o.Should().Be(123)), + + BindingTestCase.Create( + "123", + o => o.Should().Be("123")), + BindingTestCase.Create( + "true", + o => o.Should().BeTrue()), BindingTestCase.Create>( "123", o => o.Value.Should().Be(123)), - + BindingTestCase.Create>( "123", o => o.Value.Should().Be(123)), @@ -628,7 +696,7 @@ public void Apply(InvocationContext context) .Which .FullName .Should() - .Be(NonexistentPathWithTrailingSlash(), + .Be(NonexistentPathWithTrailingSlash(), "DirectoryInfo replaces Path.AltDirectorySeparatorChar with Path.DirectorySeparatorChar on Windows"), variationName: nameof(NonexistentPathWithTrailingAltSlash)), @@ -666,13 +734,13 @@ internal static string NonexistentPathWithoutTrailingSlash() "does-not-exist"); } - internal static string NonexistentPathWithTrailingSlash() => + internal static string NonexistentPathWithTrailingSlash() => NonexistentPathWithoutTrailingSlash() + Path.DirectorySeparatorChar; - internal static string NonexistentPathWithTrailingAltSlash() => + internal static string NonexistentPathWithTrailingAltSlash() => NonexistentPathWithoutTrailingSlash() + Path.AltDirectorySeparatorChar; internal static string ExistingFile() => - Directory.GetFiles(ExistingDirectory()).FirstOrDefault() ?? + Directory.GetFiles(ExistingDirectory()).FirstOrDefault() ?? throw new AssertionFailedException("No files found in current directory"); internal static string ExistingDirectory() => Directory.GetCurrentDirectory(); diff --git a/src/System.CommandLine/Binding/ModelBinder.cs b/src/System.CommandLine/Binding/ModelBinder.cs index 19f049f6e2..93ca1cb4c6 100644 --- a/src/System.CommandLine/Binding/ModelBinder.cs +++ b/src/System.CommandLine/Binding/ModelBinder.cs @@ -323,14 +323,6 @@ protected IValueDescriptor FindModelPropertyDescriptor(Type propertyType, string ); } - - - - - - - - private ConstructorInfo FindConstructorOrThrow(ParameterInfo parameter, string message) { if (!(parameter.Member is ConstructorInfo constructor)) diff --git a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs index 2f36ea0a0d..95557b4b29 100644 --- a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs +++ b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs @@ -85,7 +85,7 @@ internal void BindParameter(ParameterInfo param, Argument argument) internal void BindParameter(ParameterInfo param, Option option) { - var _ = option ?? throw new InvalidOperationException("You must specify an argument to bind"); + var _ = option ?? throw new InvalidOperationException("You must specify an option to bind"); BindValueSource(param, new SpecificSymbolValueSource(option)); } From 1ddbc18221f6c1bf544fc4b003214bb8b90b13b2 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Sat, 15 Aug 2020 05:12:33 -0700 Subject: [PATCH 10/12] Cleanup and respond to comments --- .../Binding/ModelBinderConstructorTests.cs | 20 +++++++++---------- .../Binding/BindingContext.cs | 9 --------- .../Binding/ServiceProviderValueSource.cs | 1 - .../Invocation/ModelBindingCommandHandler.cs | 2 +- 4 files changed, 11 insertions(+), 21 deletions(-) diff --git a/src/System.CommandLine.Tests/Binding/ModelBinderConstructorTests.cs b/src/System.CommandLine.Tests/Binding/ModelBinderConstructorTests.cs index 0a088f97a9..b0f4a7b438 100644 --- a/src/System.CommandLine.Tests/Binding/ModelBinderConstructorTests.cs +++ b/src/System.CommandLine.Tests/Binding/ModelBinderConstructorTests.cs @@ -35,8 +35,8 @@ public async Task Handler_constructor_receives_option_arguments_bound_to_the_spe { var testCase = common.BindingCases[type]; ICommandHandler handler = CommandHandler.Create( - MakeGenericType(typeof(ClassForCaptureMethod<>), testCase.ParameterType) - .GetMethod(nameof(ClassForCaptureMethod.Invoke))); + MakeGenericType(typeof(TesttCommandHandler<>), testCase.ParameterType) + .GetMethod(nameof(TesttCommandHandler.Invoke))); Command command = GetSingleArgumentCommand(testCase); command.Handler = handler; @@ -60,17 +60,17 @@ public async Task Handler_constructor_receives_option_arguments_bound_to_the_spe [InlineData(typeof(List))] [InlineData(typeof(int[]))] [InlineData(typeof(List))] - public async Task Constructor_receives_option_arguments_bound_to_the_specified_type( + public async Task Model_constructor_receives_option_arguments_bound_to_the_specified_type( Type type) { var testCase = common.BindingCases[type]; - var typeToCreate = MakeGenericType(typeof(ClassForCreate<>), testCase.ParameterType); + var typeToCreate = MakeGenericType(typeof(TestModel<>), testCase.ParameterType); Command command = GetSingleArgumentCommand(testCase); var binder = new ModelBinder(typeToCreate); var commandLine = $"--value {testCase.CommandLine}"; var bindingContext = new BindingContext(command.Parse(commandLine)); - var instance = binder.CreateInstance(bindingContext) as ClassForCreateBase; + var instance = binder.CreateInstance(bindingContext) as TestModelBase; instance.Value.Should().BeAssignableTo(testCase.ParameterType); testCase.AssertBoundValue(instance.Value); @@ -116,9 +116,9 @@ public void Apply(InvocationContext context) } } - private class ClassForCaptureMethod + private class TesttCommandHandler { - public ClassForCaptureMethod(T value, InvocationContext invocationContext) + public TesttCommandHandler(T value, InvocationContext invocationContext) { invocationContext.InvocationResult = new BoundValueCapturer(value); } @@ -126,14 +126,14 @@ public ClassForCaptureMethod(T value, InvocationContext invocationContext) public void Invoke() { } } - private class ClassForCreateBase + private class TestModelBase { public object? Value { get; protected set; } } - private class ClassForCreate : ClassForCreateBase + private class TestModel : TestModelBase { - public ClassForCreate(T value) + public TestModel(T value) { Value = value; } diff --git a/src/System.CommandLine/Binding/BindingContext.cs b/src/System.CommandLine/Binding/BindingContext.cs index d9b8a29c97..7104958df4 100644 --- a/src/System.CommandLine/Binding/BindingContext.cs +++ b/src/System.CommandLine/Binding/BindingContext.cs @@ -63,15 +63,6 @@ public ModelBinder GetModelBinder(IValueDescriptor valueDescriptor) return new ModelBinder(valueDescriptor); } - internal ModelBinder GetModelBinder(Type type) - { - if (_modelBindersByValueDescriptor.TryGetValue(type, out ModelBinder binder)) - { - return binder; - } - return new ModelBinder(type); - } - public void AddService(Type serviceType, Func factory) { ServiceProvider.AddService(serviceType, factory); diff --git a/src/System.CommandLine/Binding/ServiceProviderValueSource.cs b/src/System.CommandLine/Binding/ServiceProviderValueSource.cs index 04a927da70..187fa4e48b 100644 --- a/src/System.CommandLine/Binding/ServiceProviderValueSource.cs +++ b/src/System.CommandLine/Binding/ServiceProviderValueSource.cs @@ -10,7 +10,6 @@ public bool TryGetValue(IValueDescriptor valueDescriptor, out object? boundValue) { boundValue = bindingContext?.ServiceProvider.GetService(valueDescriptor.ValueType); - // ?? Why return true if the service isn't found? return true; } } diff --git a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs index 95557b4b29..9e6bed4693 100644 --- a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs +++ b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs @@ -18,7 +18,7 @@ internal class ModelBindingCommandHandler : ICommandHandler private readonly IMethodDescriptor _methodDescriptor; private Dictionary _invokeArgumentBindingSources { get; } = new Dictionary(); - private bool EnforceExplicitBinding = false; // Wrong formatting as hint to figure out how to set this + private bool EnforceExplicitBinding = false; // ?? Wrong formatting as hint to figure out how to set this public ModelBindingCommandHandler( MethodInfo handlerMethodInfo, From fd687426064bf406b556c59449f5371de30a2954 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Mon, 17 Aug 2020 14:16:23 -0700 Subject: [PATCH 11/12] Cleanup --- src/System.CommandLine/Binding/BoundValue.cs | 1 - src/System.CommandLine/Binding/ParameterDescriptor.cs | 1 - 2 files changed, 2 deletions(-) diff --git a/src/System.CommandLine/Binding/BoundValue.cs b/src/System.CommandLine/Binding/BoundValue.cs index c28f0a081f..6eeb32ef37 100644 --- a/src/System.CommandLine/Binding/BoundValue.cs +++ b/src/System.CommandLine/Binding/BoundValue.cs @@ -5,7 +5,6 @@ namespace System.CommandLine.Binding { public class BoundValue { - // ?? Why have an internal constructor on a public readonly class? internal BoundValue( object? value, IValueDescriptor valueDescriptor, diff --git a/src/System.CommandLine/Binding/ParameterDescriptor.cs b/src/System.CommandLine/Binding/ParameterDescriptor.cs index 7c3282cc3f..e81138b309 100644 --- a/src/System.CommandLine/Binding/ParameterDescriptor.cs +++ b/src/System.CommandLine/Binding/ParameterDescriptor.cs @@ -26,7 +26,6 @@ internal ParameterDescriptor( public bool HasDefaultValue => _parameterInfo.HasDefaultValue; - // ?? This is used in model binder to determine whether to call GetDefaultValue. This is either misnamed or there is another issue public bool AllowsNull { get From fae829e4867d47499c2d6ae2fa5d84d76d1a3c11 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Fri, 21 Aug 2020 14:51:29 -0700 Subject: [PATCH 12/12] Fixed warnings that were breaking Arcade, uncommented tests and fixed --- .../Binding/ModelBinderConstructorTests.cs | 12 ++++++------ src/System.CommandLine/Binding/ModelDescriptor.cs | 2 +- .../Invocation/ModelBindingCommandHandler.cs | 8 ++++---- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/System.CommandLine.Tests/Binding/ModelBinderConstructorTests.cs b/src/System.CommandLine.Tests/Binding/ModelBinderConstructorTests.cs index b0f4a7b438..6474933f24 100644 --- a/src/System.CommandLine.Tests/Binding/ModelBinderConstructorTests.cs +++ b/src/System.CommandLine.Tests/Binding/ModelBinderConstructorTests.cs @@ -50,17 +50,17 @@ public async Task Handler_constructor_receives_option_arguments_bound_to_the_spe } [Theory] - //[InlineData(typeof(ClassWithCtorParameter))] - //[InlineData(typeof(ClassWithSetter))] - //[InlineData(typeof(ClassWithCtorParameter))] - //[InlineData(typeof(ClassWithSetter))] + [InlineData(typeof(ClassWithCtorParameter))] + [InlineData(typeof(ClassWithSetter))] + [InlineData(typeof(ClassWithCtorParameter))] + [InlineData(typeof(ClassWithSetter))] [InlineData(typeof(FileInfo))] [InlineData(typeof(FileInfo[]))] [InlineData(typeof(string[]))] [InlineData(typeof(List))] [InlineData(typeof(int[]))] [InlineData(typeof(List))] - public async Task Model_constructor_receives_option_arguments_bound_to_the_specified_type( + public void Model_constructor_receives_option_arguments_bound_to_the_specified_type( Type type) { var testCase = common.BindingCases[type]; @@ -128,7 +128,7 @@ public void Invoke() { } private class TestModelBase { - public object? Value { get; protected set; } + public object Value { get; protected set; } } private class TestModel : TestModelBase diff --git a/src/System.CommandLine/Binding/ModelDescriptor.cs b/src/System.CommandLine/Binding/ModelDescriptor.cs index 9759498026..b96535f3ce 100644 --- a/src/System.CommandLine/Binding/ModelDescriptor.cs +++ b/src/System.CommandLine/Binding/ModelDescriptor.cs @@ -35,7 +35,7 @@ protected ModelDescriptor(Type modelType) public IReadOnlyList PropertyDescriptors => _propertyDescriptors ??= ModelType.GetProperties(CommonBindingFlags) - .Where(p => p.CanWrite) + .Where(p => p.CanWrite && p.SetMethod.IsPublic) .Select(i => new PropertyDescriptor(i, this)) .ToList(); diff --git a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs index 9e6bed4693..467867fbd6 100644 --- a/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs +++ b/src/System.CommandLine/Invocation/ModelBindingCommandHandler.cs @@ -16,9 +16,9 @@ internal class ModelBindingCommandHandler : ICommandHandler private readonly ModelBinder? _invocationTargetBinder; private readonly MethodInfo? _handlerMethodInfo; private readonly IMethodDescriptor _methodDescriptor; - private Dictionary _invokeArgumentBindingSources { get; } = + private Dictionary invokeArgumentBindingSources { get; } = new Dictionary(); - private bool EnforceExplicitBinding = false; // ?? Wrong formatting as hint to figure out how to set this + private readonly bool EnforceExplicitBinding = false; // ?? Wrong formatting as hint to figure out how to set this public ModelBindingCommandHandler( MethodInfo handlerMethodInfo, @@ -53,7 +53,7 @@ public async Task InvokeAsync(InvocationContext context) var bindingContext = context.BindingContext; var (boundValues, _) = ModelBinder.GetBoundValues( - _invokeArgumentBindingSources, + invokeArgumentBindingSources, bindingContext, _methodDescriptor.ParameterDescriptors, EnforceExplicitBinding); @@ -96,7 +96,7 @@ private void BindValueSource(ParameterInfo param, IValueSource valueSource) { throw new InvalidOperationException("You must bind to a parameter on this handler"); } - _invokeArgumentBindingSources.Add(paramDesc, valueSource); + invokeArgumentBindingSources.Add(paramDesc, valueSource); } private ParameterDescriptor? FindParameterDescriptor(ParameterInfo? param)