Add support binding to types other than string or collection of string in HeaderModelBinder #7294

kichalla · 2018-01-24T23:18:04Z

Related to #5859

kichalla · 2018-01-24T23:21:56Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

+        /// <param name="innerModelBinder">The <see cref="IModelBinder"/> which does the actual
+        /// binding of values.</param>
+        /// <param name="loggerFactory">The <see cref="ILoggerFactory"/>.</param>
+        public HeaderModelBinder(ILoggerFactory loggerFactory, IModelBinder innerModelBinder)


Question: This HeaderModelBinder now delegates binding to the inner model binder. For users who upgrade to Preview2 and who use the HeaderModelBinderProvider as it is, then this constructor would be used and all is well, but if a user had been explicitly(the parameterless or the one taking in ILoggerFactory) using the HeaderModelBinder constructors, then the InnerModelBinder would be null breaking their application at runtime. We should have a backup (i.e use earlier design) in case the inner binder is null?

Yep. That's the non-breaking change way to do this.

I'd try to keep 1-2 tests that use the old code path and make the majority of them go down the new code path.

Sounds good 👍

rynowak · 2018-01-25T02:55:09Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

+            // SimpleTypeModelBinder adds a model error when binding to let's say and integer and the model is null.
+
+            // Explicitly pass in the header name as the key rather than taking in the model name to look for values
+            // as otherwise it would be breaking from earlier version where we didn't consider prefixes.


I would explain this a bit differently. Using the ModelName directly rather than the model path (full name) isn't just a back-compat concern. It's always the way this is supposed to work. There was never a desire for model binding a header value to compose keys like we do for forms and query strings.

Explicitly pass in the header name

Where is the code that does this? I would expect to see this show up at L113 or so

rynowak · 2018-01-25T02:57:10Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

-                // can't assign it to the property.
-                model = null;
-            }
+                bindingContext.ValueProvider = headerValueProvider;


Do you need to 'pop' the value provider or is it done for you?

It's done automatically for us. I have a test for this case: https://github.com/aspnet/Mvc/pull/7294/files#diff-7e7d3101aa01d6dab9e9b49c4c3ddf51R124

rynowak · 2018-01-25T02:57:52Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinderProvider.cs

-                    return new HeaderModelBinder(loggerFactory);
-                }
-                else
+                // Change the binding info to prevent recursion


This is probably worth a bit more explanation given how long we discussed it today :)

rynowak · 2018-01-25T02:58:18Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinderProvider.cs

                {
-                    logger.CannotCreateHeaderModelBinder(context.Metadata.ModelType);


Should we preserve this logging?

rynowak · 2018-01-25T02:58:54Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Internal/HeaderValueProvider.cs

+using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Mvc.Core;
+
+namespace Microsoft.AspNetCore.Mvc.ModelBinding.Internal


Can we make this proper internal? I don't think we should add more stuff like this. It leaks out and causes confusion.

rynowak · 2018-01-25T02:59:38Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Internal/HeaderValueProvider.cs

+        public bool ContainsPrefix(string prefix)
+        {
+            // In 2.0 version, HeaderModelBinder didn't consider prefix at all and doing now would break existing
+            // users, so ignore the key value totally and only rely on the field name


This is not just for back compat. It works this way on purpose.

rynowak · 2018-01-25T03:02:11Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Internal/HeaderValueProvider.cs

+            else
+            {
+                return new ValueProviderResult(values, _culture);
+            }


I think this code would be a lot simpler if we just passed in a headerName and a string[] values from the calling code. We already figured out whether or not we wanted to split csv headers in the calling code.

rynowak · 2018-01-25T03:02:19Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Internal/HeaderValueProvider.cs

+        }
+
+        /// <inheritdoc />
+        public ValueProviderResult GetValue(string key)


Where is key used?

We ignore this entirely on purpose (please check my reply comment in this file)

rynowak · 2018-01-25T03:03:17Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Internal/HeaderValueProvider.cs

+        {
+            // In 2.0 version, HeaderModelBinder didn't consider prefix at all and doing now would break existing
+            // users, so ignore the key value totally and only rely on the field name
+            return _headers.ContainsKey(_headerFieldName);


We should be setting up the model binding context such that key is going to be header name, and then checking that it's the appropriate value.

This implementation doesn't really conform to the expectations of the IValueProvider interface.

Yes, you are right and that's how I had it in the beginning, but I changed it because the current HeaderModelBinder sets the key to be the ModelName instead of the header name and changing it might break existing users. So the idea, as you can imagine, is to send the inner binders the model name (so that they can set the model state key as before), but make the header value provider just ignore the full model path.

jbagga · 2018-01-25T19:27:37Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

+
+            // Do not set ModelBindingResult to Failed on not finding the value as we want the inner modelbinder to
+            // do that. This would give a chance to the inner binder to add more useful information. For example,
+            // SimpleTypeModelBinder adds a model error when binding to let's say and integer and the model is null.


nit: "an integer"

rynowak · 2018-01-25T20:53:30Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs

@@ -490,7 +490,7 @@ static MvcCoreLoggerExtensions()
            _cannotCreateHeaderModelBinder = LoggerMessage.Define<Type>(
               LogLevel.Debug,
               20,
-               "Could not create a binder for type '{ModelType}' as this binder only supports 'System.String' type or a collection of 'System.String'.");
+               "Could not create a binder for type '{ModelType}' as this binder only supports simple types (like string, int, bool, enum) or a collection of them.");


or a collection of them

or a collection of simple types

dougbu

Test changes look pretty straightforward. Anything in particular we need to be looking at?

dougbu · 2018-01-25T23:07:09Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

        {
            if (bindingContext == null)
            {
                throw new ArgumentNullException(nameof(bindingContext));
            }

-            var request = bindingContext.HttpContext.Request;
+            _logger.AttemptingToBindModel(bindingContext);

            // Property name can be null if the model metadata represents a type (rather than a property or parameter).


Sounds like a corner case where the new implementation may have changed the behaviour. Do we have tests of this scenario?

What is this actually about? Is it about decorating on a complex type? if yes, we do not support it and I have tests in this PR to check that.

dougbu · 2018-01-25T23:12:06Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

        /// Initializes a new instance of <see cref="HeaderModelBinder"/>.
        /// </summary>
        /// <param name="loggerFactory">The <see cref="ILoggerFactory"/>.</param>
+        [Obsolete("This constructor is obsolete and will be removed in a future version. The recommended alternative"
+            + " is the overload that takes an " + nameof(ILoggerFactory) + " and an " + nameof(IModelBinder) + ".")]
        public HeaderModelBinder(ILoggerFactory loggerFactory)


Looks like this constructor was added in Preview1. If that's the case, just delete it.

dougbu · 2018-01-25T23:16:00Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

        /// <inheritdoc />
-        public Task BindModelAsync(ModelBindingContext bindingContext)
+        public async Task BindModelAsync(ModelBindingContext bindingContext)


No reason for an async state machine when doing the old thing. Suggest making this method non-async. Either call BindWithoutInnerBinder(...) and return Task.CompletedTask or a new method (that is async).

But, don't worry about it if others thing the "old thing" is an extreme corner case.

But, don't worry about it if others thing the "old thing" is an extreme corner case.

Exactly. It would be uncommon for users to have instantiated HMB by itself instead of going through HMBP. So, I will leave the code as it is.

dougbu · 2018-01-25T23:16:48Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

+            // Do not set ModelBindingResult to Failed on not finding the value in the header as we want the inner 
+            // modelbinder to do that. This would give a chance to the inner binder to add more useful information.
+            // For example, SimpleTypeModelBinder adds a model error when binding to let's say an integer and the
+            // model is null.


Doesn't this comment belong with the ContainsKey(...) check?

I did it here because the typical pattern that i have seen in other binders is that they short-circuit immediately when a value is not provided from the value provider. So, I just kept it here for that reason in case anyone tries to modify this binder in future.

What does the typical pattern have to do with the location of this comment? It's talking about the "not finding the value" case which is handled earlier. Move it to be right above the ContainsKey(...) check.

dougbu · 2018-01-25T23:19:25Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

+            var request = bindingContext.HttpContext.Request;
+
+            // Prevent breaking existing users in scenarios where they are binding to a 'string' property
+            // and expect the whole comma separated string, if any, as a single string and not as a string array.


Thought we had code that basically said "if mapping a value provider collection to a string, concatenate the collection's items"? If the difference is only between "X,Y,Z" and "X, Y, Z", we shouldn't bother with this extra bit.

Have we considered using a compat switch to drive this new model binding behavior? We wouldn't need this check at this point

Thought we had code that basically said "if mapping a value provider collection to a string, concatenate the collection's items"? If the difference is only between "X,Y,Z" and "X, Y, Z", we shouldn't bother with this extra bit.

The difference is between "X,Y,Z" and "X" (i.e if we always get comma separated values, the STMB would only take the first value in the list of values to bind to a string)

Have we considered using a compat switch to drive this new model binding behavior? We wouldn't need this check at this point

Good point. @dougbu what do you think? If you agree, please give a name for the switch too :-)

@dougbu I am interested in this switch because it would give a consistent experience (for example, if a query string had multiple values for a key, we only pick the first one).

The "entire header string" behaviour is what we want. Leave it alone because headers aren't query string values.

A switch to control whether or not the HMB uses the old code path despite a non-null inner binder might be interesting if anything behaved differently for the previously supported cases i.e. when binding string or collections of string. Is anything visible changing there? I'm hoping not; any visible behaviour change would be unintended.

The other possibility would be to disable binding non-strings / non-string collections when a CompatibitySwitch is set. That might be useful because someone may have implemented a model binding provider that handles, say, ints when the BindingSource is Header. @Eilon should we add a switch for this fairly-extreme customization?

Is anything visible changing there? I'm hoping not; any visible behaviour change would be unintended.

I have tests covering the old behavior(2.0) using a non-null InnerBinder. So we are fine.
https://github.com/aspnet/Mvc/pull/7294/files#diff-7e7d3101aa01d6dab9e9b49c4c3ddf51R132

If we think it's the right thing for new 2.1 apps to get a behavior that's different from 2.0, then, yeah, add a compat switch.

dougbu · 2018-01-25T23:23:49Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

+
+            // Prevent breaking existing users in scenarios where they are binding to a 'string' property
+            // and expect the whole comma separated string, if any, as a single string and not as a string array.
+            string[] values;


May be a small perf win to avoid allocating this array when !request.Headers.ContainsKey(headerName). Just use Array.Empty<string>() in that case.

dougbu · 2018-01-25T23:25:54Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Internal/HeaderValueProvider.cs

+        /// <inheritdoc />
+        public bool ContainsPrefix(string prefix)
+        {
+            return _isHeaderPresent;


This is odd and _isHeaderPresent shouldn't be necessary. Just use _values.Length == 0 everywhere.

As-is, ContainsPrefix(...) will return false and GetValue(...) will return a one-element array containing null when assigning to a non-collection. The two methods should do sensible things and be consistent.

dougbu · 2018-01-25T23:34:49Z

test/Microsoft.AspNetCore.Mvc.IntegrationTests/HeaderModelBinderIntegrationTest.cs

+
+            // ModelState
+            Assert.True(modelState.IsValid);
+            var entry = modelState["prefix.Manufacturer.NoCommaString"];


Use helper methods and Assert.Collection(...) to shorten this and make it easier to read.

pranavkm · 2018-01-25T22:20:38Z

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelBinderProviderContext.cs

+        /// <returns>An <see cref="IModelBinder"/>.</returns>
+        public virtual IModelBinder CreateBinder(ModelMetadata metadata, BindingInfo bindingInfo)
+        {
+            throw new NotImplementedException();


NotSupportedException?

Just curious, why?

According to the documentation

In other words, a NotImplementedException exception should be synonymous with "still in development."

pranavkm · 2018-01-25T23:45:16Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

+            var request = bindingContext.HttpContext.Request;
+
+            // Prevent breaking existing users in scenarios where they are binding to a 'string' property
+            // and expect the whole comma separated string, if any, as a single string and not as a string array.


Have we considered using a compat switch to drive this new model binding behavior? We wouldn't need this check at this point

pranavkm · 2018-01-25T23:47:06Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

+
+            // Create a new binding scope in order to supply the HeaderValueProvider so that the binders like
+            // SimpleTypeModelBinder can find values from header.
+            ModelBindingResult result;


Can we assign this inline?

Move this and its comment right above the using

pranavkm · 2018-01-25T23:48:56Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinderProvider.cs

-                context.BindingInfo.BindingSource.CanAcceptDataFrom(BindingSource.Header))
+            var bindingInfo = context.BindingInfo;
+            if (bindingInfo.BindingSource == null
+                || !bindingInfo.BindingSource.CanAcceptDataFrom(BindingSource.Header))


Formatting, || on the previous line

I thought we prefer this one? @dougbu , I recall this feedback was from you

Hmm, I remember commenting about keeping : this( and : base( together and on a separate line. On the other hand, binary operators either place seems fair game to me. I might however have commented on consistency within a file or a PR.

Put another way, I don't have a dog in this hunt.

pranavkm · 2018-01-25T23:50:02Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinderProvider.cs

+            // Since we are delegating the binding of the current model type to other binders, modify the
+            // binding source of the current model type to a non-FromHeader binding source in order to avoid an
+            // infinite recursion into this binder provider.
+            var nonFromHeaderBindingInfo = new BindingInfo(bindingInfo);


nestedBindingInfo?

pranavkm · 2018-01-25T23:54:13Z

test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/HeaderModelBinderProviderTest.cs

-        public void Create_WhenBindingSourceIsFromHeader_ReturnsBinder()
+        [Theory]
+        [InlineData(typeof(string))]
+        [InlineData(typeof(bool))]


This doesn't need to be exhaustive. A couple of data types should suffice. One numeric, one enum type, one DateTime(?) and a nullable struct

pranavkm · 2018-01-25T23:59:22Z

test/Microsoft.AspNetCore.Mvc.IntegrationTests/HeaderModelBinderIntegrationTest.cs

+            public Manufacturer Manufacturer { get; set; }
+        }
+
+        private class Manufacturer


Would something like this work:

[FromHeader] public class RequestMetadata { public string CorrelationId { get; set; } public string Date { get; set; } }

No, this is not supported and we have tests in HeaderModelBinderProviderTest.cs where it doesn't give a binder as its not a simple type.

kichalla

🆙 📅

kichalla · 2018-01-26T21:36:43Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

        {
            if (bindingContext == null)
            {
                throw new ArgumentNullException(nameof(bindingContext));
            }

-            var request = bindingContext.HttpContext.Request;
+            _logger.AttemptingToBindModel(bindingContext);

            // Property name can be null if the model metadata represents a type (rather than a property or parameter).


What is this actually about? Is it about decorating on a complex type? if yes, we do not support it and I have tests in this PR to check that.

kichalla · 2018-01-26T21:38:34Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

+            var request = bindingContext.HttpContext.Request;
+
+            // Prevent breaking existing users in scenarios where they are binding to a 'string' property
+            // and expect the whole comma separated string, if any, as a single string and not as a string array.


@dougbu I am interested in this switch because it would give a consistent experience (for example, if a query string had multiple values for a key, we only pick the first one).

dougbu · 2018-01-26T23:21:12Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

        }

        /// <summary>
        /// Initializes a new instance of <see cref="HeaderModelBinder"/>.
        /// </summary>
+        /// <param name="innerModelBinder">The <see cref="IModelBinder"/> which does the actual
+        /// binding of values.</param>
        /// <param name="loggerFactory">The <see cref="ILoggerFactory"/>.</param>


Order these to match the parameters.

dougbu · 2018-01-26T23:23:54Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

+
+            // Create a new binding scope in order to supply the HeaderValueProvider so that the binders like
+            // SimpleTypeModelBinder can find values from header.
+            ModelBindingResult result;


Move this and its comment right above the using

dougbu · 2018-01-26T23:25:01Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

        {
            if (bindingContext == null)
            {
                throw new ArgumentNullException(nameof(bindingContext));
            }

-            var request = bindingContext.HttpContext.Request;
+            _logger.AttemptingToBindModel(bindingContext);

            // Property name can be null if the model metadata represents a type (rather than a property or parameter).


dougbu · 2018-01-26T23:31:57Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinderProvider.cs

+            // binding source of the current model type to a non-FromHeader binding source in order to avoid an
+            // infinite recursion into this binder provider.
+            var nestedBindingInfo = new BindingInfo(bindingInfo);
+            nestedBindingInfo.BindingSource = BindingSource.ModelBinding;


Inline this line i.e. use

{ BindingSource = BindingSource.ModelBinding, }

dougbu · 2018-01-26T23:33:10Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Internal/HeaderValueProvider.cs

+
+namespace Microsoft.AspNetCore.Mvc.ModelBinding.Internal
+{
+    internal class HeaderValueProvider : IValueProvider


Why is this not private to the HeaderModelBinder? You're not testing this class directly.

dougbu · 2018-01-26T23:37:13Z

test/Microsoft.AspNetCore.Mvc.IntegrationTests/HeaderModelBinderIntegrationTest.cs

+
+        private class Manufacturer
+        {
+            [FromHeader]


Override some of the field names using [FromHeader("Different name")]

dougbu · 2018-01-26T23:40:36Z

test/Microsoft.AspNetCore.Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs

@@ -2701,7 +2701,7 @@ private class LocationInfo

            entry = Assert.Single(modelState, e => e.Key == "Info.Value.GpsCoordinates").Value;
            Assert.Equal("10,20", entry.AttemptedValue);
-            Assert.Equal(new[] { "10", "20" }, entry.RawValue);
+            Assert.Equal("10,20", entry.RawValue);


Wait. This implies something is changing that shouldn't be. I'm not that worried about RawValue per se. But, the "are we assigning to a non-collection" checks were intended to keep the behaviour as close as possible to the old ways. Why is this acting differently?

This is because HMB used to do this earlier https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs#L97 and in the new design individual binder set the result

kichalla · 2018-01-27T03:47:44Z

Updated

kichalla · 2018-01-30T00:37:41Z

🆙 📅

dougbu

Only a few smallish comments…
Also get @pranavkm's opinions.

dougbu · 2018-01-30T17:36:05Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

+                }
+            }
+
+            return new HeaderValueProvider(CultureInfo.InvariantCulture, values);


Don't pass in the culture or store it in a field. Use CultureInfo.InvariantCulture when creating the ValueProviderResult.

dougbu · 2018-01-30T17:44:32Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs

@@ -487,11 +488,16 @@ static MvcCoreLoggerExtensions()
               19,
               "Could not bind to model with name '{ModelName}' and type '{ModelType}' as the request did not have a content type of either 'application/x-www-form-urlencoded' or 'multipart/form-data'.");

-            _cannotCreateHeaderModelBinder = LoggerMessage.Define<Type>(
+            _cannotCreateHeaderModelBinderCompatVersion_2_0 = LoggerMessage.Define<Type>(
               LogLevel.Debug,
               20,


This id should be higher than any other value in this file.

I am using the same Id because this log message is a temporary one and would be removed in 3.0 version. Also it is mutually exclusive with the other one in this PR. So we should be fine?

Not sure what the issue is here - log event ids only need to be unique within a category name. It's always confusing no matter what, though.

dougbu · 2018-01-30T17:46:42Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

        }

        /// <summary>
        /// Initializes a new instance of <see cref="HeaderModelBinder"/>.
        /// </summary>
-        /// <param name="loggerFactory">The <see cref="ILoggerFactory"/>.</param>


Restore this line.

dougbu · 2018-01-30T17:46:58Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

        public HeaderModelBinder()
            : this(NullLoggerFactory.Instance)
        {
+            _logger = NullLogger.Instance;


Remove this line, it's redundant.

dougbu · 2018-01-30T17:57:21Z

test/Microsoft.AspNetCore.Mvc.IntegrationTests/ModelBindingTestHelper.cs

@@ -77,7 +89,7 @@ public static class ModelBindingTestHelper

            return new ParameterBinder(
                metadataProvider,
-                GetModelBinderFactory(metadataProvider, options),
+                new ModelBinderFactory(metadataProvider, options, services),


Check if GetModelBinderFactory(...) is still used. Remove it if not.

Yeah, its used in ParameterBinderExtensions.cs file

kichalla

🆙 📅

kichalla · 2018-01-30T18:21:18Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs

@@ -487,11 +488,16 @@ static MvcCoreLoggerExtensions()
               19,
               "Could not bind to model with name '{ModelName}' and type '{ModelType}' as the request did not have a content type of either 'application/x-www-form-urlencoded' or 'multipart/form-data'.");

-            _cannotCreateHeaderModelBinder = LoggerMessage.Define<Type>(
+            _cannotCreateHeaderModelBinderCompatVersion_2_0 = LoggerMessage.Define<Type>(
               LogLevel.Debug,
               20,


I am using the same Id because this log message is a temporary one and would be removed in 3.0 version. Also it is mutually exclusive with the other one in this PR. So we should be fine?

kichalla · 2018-01-30T18:22:18Z

test/Microsoft.AspNetCore.Mvc.IntegrationTests/ModelBindingTestHelper.cs

@@ -77,7 +89,7 @@ public static class ModelBindingTestHelper

            return new ParameterBinder(
                metadataProvider,
-                GetModelBinderFactory(metadataProvider, options),
+                new ModelBinderFactory(metadataProvider, options, services),


Yeah, its used in ParameterBinderExtensions.cs file

pranavkm

Looks good to me. Please get sign off from @rynowak \ @dougbu before checking this in.

pranavkm · 2018-01-30T18:42:06Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs

               LogLevel.Debug,
               20,
               "Could not create a binder for type '{ModelType}' as this binder only supports 'System.String' type or a collection of 'System.String'.");

+            _cannotCreateHeaderModelBinder = LoggerMessage.Define<Type>(
+               LogLevel.Debug,
+               20,


Should this have a different event id? 20's already used no?

I had commented on this here https://github.com/aspnet/Mvc/pull/7294/files#r164833556 . Let me know if you agree.

pranavkm · 2018-01-30T18:43:21Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

@@ -36,35 +37,117 @@ public HeaderModelBinder()
        /// </summary>
        /// <param name="loggerFactory">The <see cref="ILoggerFactory"/>.</param>
        public HeaderModelBinder(ILoggerFactory loggerFactory)


Since this was newly added for preview1, should we remove this one entirely? Presumably we want users to use the new behavior going forward.

This constructor would be used when users are on 2.0 compat mode. We could have not added this constructor but kept it as they could see logging from the binder.

pranavkm · 2018-01-30T18:45:26Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

        }

+        // to enable unit testing
+        internal IModelBinder InnerModelBinder { get; }


Should be fine to make this public. Not much of an implementation detail.

pranavkm · 2018-01-30T18:46:22Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

+
+            var headerValueProvider = GetHeaderValueProvider(headerName, bindingContext);
+
+            // Capture the top level object here as entering nested scope would make it 'false' always.


would make it 'false' ~~always~~

pranavkm · 2018-01-30T18:48:46Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinder.cs

+
+            public HeaderValueProvider(string[] values)
+            {
+                if (values == null)


You could just Debug.Assert this. We're the only ones calling this ctor.

pranavkm · 2018-01-30T18:53:21Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/HeaderModelBinderProvider.cs

-                // at runtime due to collections we can't modify.
-                if (context.Metadata.ModelType == typeof(string) ||
-                    context.Metadata.ElementType == typeof(string))
+            var options = context.Services.GetRequiredService<IOptions<MvcOptions>>().Value;


Could we cache the instance of MvcOptions once we've read it?

pranavkm · 2018-01-30T18:57:36Z

test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/HeaderModelBinderTests.cs

+        {
+            get
+            {
+                var guid = Guid.NewGuid();


Can you use fixed Guid here? Having varying data throws off our CI's test collection (since it gets reported as a different test name for each run

Ah yes! Good catch

dougbu

Make the log ids unique. Then, this looks done.

…than string and string collections

kichalla requested review from pranavkm, rynowak and dougbu January 24, 2018 23:18

kichalla commented Jan 24, 2018

View reviewed changes

rynowak reviewed Jan 25, 2018

View reviewed changes

jbagga reviewed Jan 25, 2018

View reviewed changes

rynowak reviewed Jan 25, 2018

View reviewed changes

dougbu suggested changes Jan 25, 2018

View reviewed changes

dougbu reviewed Jan 25, 2018

View reviewed changes

pranavkm reviewed Jan 26, 2018

View reviewed changes

kichalla changed the title ~~[Design] Add support binding to types other than string or collection of string in HeaderModelBinder~~ Add support binding to types other than string or collection of string in HeaderModelBinder Jan 26, 2018

kichalla commented Jan 26, 2018

View reviewed changes

dougbu suggested changes Jan 26, 2018

View reviewed changes

kichalla force-pushed the kichalla/headermodelbinder-change-modelbindingcontext-scope branch from 9a7d54c to a316ee9 Compare January 27, 2018 00:01

dougbu suggested changes Jan 30, 2018

View reviewed changes

kichalla commented Jan 30, 2018

View reviewed changes

pranavkm approved these changes Jan 30, 2018

View reviewed changes

dougbu approved these changes Jan 30, 2018

View reviewed changes

[Fixes #5859] Consider allowing binding header values to types other …

0215740

…than string and string collections

kichalla force-pushed the kichalla/headermodelbinder-change-modelbindingcontext-scope branch from 8f896e2 to 0215740 Compare January 30, 2018 19:47

kichalla merged commit 0215740 into dev Jan 30, 2018

dougbu deleted the kichalla/headermodelbinder-change-modelbindingcontext-scope branch July 15, 2018 03:44

mkArtakMSFT mentioned this pull request Nov 6, 2018

Feature Request: FromClaimAttribute(string claimName) #8634

Closed

		{
		logger.CannotCreateHeaderModelBinder(context.Metadata.ModelType);


		var headerValueProvider = GetHeaderValueProvider(headerName, bindingContext);

		// Capture the top level object here as entering nested scope would make it 'false' always.

Add support binding to types other than string or collection of string in HeaderModelBinder #7294

Add support binding to types other than string or collection of string in HeaderModelBinder #7294

Conversation

kichalla commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kichalla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kichalla Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

kichalla commented Jan 27, 2018

kichalla commented Jan 30, 2018

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kichalla Jan 27, 2018 •

edited

Loading