HTML and tag helpers misname elements for expressions like `m => ModelType`

[pjeannet]

Hi, as stated in the title, my custom model binder stopped working after updating to core 1.1. I couldn't find any anouncement for changes on model binding, am I missing a new package or something? Here is my startup file :

services.AddMvc(options =>
{
   options.Filters.Add(new AuthorizeFilter(defaultPolicy));
   options.ModelBinderProviders.Insert(0, new TemplateSectionModelBinderProvider());
});

Thanks!

[frankabbruzzese]

I have two custom binders and they continued working with 1.1. So probably it is a problem in the implementation of either your specific Provider or your specific ModelBinder that creates the problem. Add a breakpoint in the provider to verify it is called, and if ita actually returns your binder, or for if for some reason it aborts its normal path.

[pjeannet]

I was probably not specific enough : as soon as I update the packages to their 1.1 version, the model binder does not get called at all. Breakpoints do not trigger in the provider neither.

[frankabbruzzese]

My model binders are part of a library so I add them after services.AddMvc(.. with a services.AddMy Library() So I add them with :

services.AddTransient<IConfigureOptions<MvcViewOptions>, MyCustomSetup>();

public class MyCustomSetup: IConfigureOptions<MvcOptions>
    {
        public void Configure(MvcOptions options)
        {
          ....

Try the same, maybe putting them in AddMvc is too early and the option object has not been already populated with the "standard providers". So when it is populated the collection is preliminarly cleared, and your provider is removed.

This is the only explanation coming to my mind...since my software works properly also with 1.1

[Eilon]

@pierre-weceipt can you share more of your app? Or perhaps upload it to GitHub so we can take a look? We're not aware of any changes in this area that could cause this.

[pjeannet]

As migrating to 1.1 was not a priority for us I delayed dealing with the issue. After runing more tests the model binder is now correctly called (I don't know what changed since before holidays). What is surprising is that I detected an issue that should have prevented the binder to work before but was not..

One last thing is strange though, the project can run but VS and CLI is telling me packages can't be restored. Here is the message :

log  : Restoring packages for tool 'Microsoft.VisualStudio.Web.CodeGeneration.Tools' in C:\Users\Pierre\Documents\Visual Studio 2015\Projects\nao\src\Thelos.NAO\project.json...
error: Package Microsoft.Composition 1.0.27 is not compatible with netcoreapp1.0 (.NETCoreApp,Version=v1.0). Package Microsoft.Composition 1.0.27 supports: portable-net45+win8+wp8+wpa81 (.NETPortable,Version=v0.0,Profile=Profile259)
error: One or more packages are incompatible with .NETCoreApp,Version=v1.0.

my global.json :

{
  "projects": [ "src", "test" ],
  "sdk": {
    "version": "1.0.0-preview2-1-003177"
  }
}

and my project.json :

{
  "buildOptions": {
    "emitEntryPoint": true,
    "preserveCompilationContext": true
  },

  "dependencies": {
    "Microsoft.AspNetCore.Authentication.Cookies": "1.1.0",
    "Microsoft.AspNetCore.Diagnostics": "1.1.0",
    "Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore": "1.1.0",
    "Microsoft.AspNetCore.Identity.EntityFrameworkCore": "1.1.0",
    "Microsoft.AspNetCore.Mvc": "1.1.0",
    "Microsoft.AspNetCore.Server.IISIntegration": "1.1.0",
    "Microsoft.AspNetCore.StaticFiles": "1.1.0",
    "Microsoft.EntityFrameworkCore": "1.1.0",
    "Microsoft.EntityFrameworkCore.Design": "1.1.0",
    "Microsoft.EntityFrameworkCore.SqlServer": "1.1.0",
    "Microsoft.EntityFrameworkCore.SqlServer.Design": "1.1.0",
    "Microsoft.Extensions.Configuration.EnvironmentVariables": "1.1.0",
    "Microsoft.Extensions.Configuration.Json": "1.1.0",
    "Microsoft.Extensions.Configuration.UserSecrets": "1.1.0",
    "Microsoft.Extensions.Logging": "1.1.0",
    "Microsoft.Extensions.Logging.Console": "1.1.0",
    "Microsoft.Extensions.Logging.Debug": "1.1.0",
    "Microsoft.Extensions.Options.ConfigurationExtensions": "1.1.0",
    "Microsoft.VisualStudio.Web.BrowserLink.Loader": "14.1.0",
    "Microsoft.VisualStudio.Web.CodeGenerators.Mvc": "1.1.0-preview4-final",
    "Microsoft.WindowsAzure.ConfigurationManager": "3.2.3",
    "ncalc": "1.3.8",
    "Newtonsoft.Json": "9.0.1",
    "Sendgrid": "8.0.5",
    "WindowsAzure.Storage": "8.0.0"
  },

  "tools": {
    "Microsoft.AspNetCore.Razor.Tools": "1.1.0-preview4-final",
    "Microsoft.AspNetCore.Server.IISIntegration.Tools": "1.1.0-preview4-final",
    "Microsoft.EntityFrameworkCore.Tools": "1.1.0-preview4-final",
    "Microsoft.Extensions.SecretManager.Tools": "1.1.0-preview4-final",
    "Microsoft.VisualStudio.Web.CodeGeneration.Tools": "1.1.0-preview4-final"
  },

  "frameworks": {
    "net461": { }
  },

  "publishOptions": {
    "include": [
      "wwwroot",
      "Views",
      "appsettings.json",
      "web.config"
    ]
  },

  "scripts": {
    "prepublish": [ "npm install", "bower install", "gulp clean", "gulp min" ],
    "postpublish": [ "dotnet publish-iis --publish-folder %publish:OutputPath% --framework %publish:FullTargetFramework%" ]
  }
}

[pjeannet]

What is surprising is that I detected an issue that should have prevented the binder to work before but was not..

As a follow up, in my view

@{
    var ModelType = @Model.GetType().FullName;
}
<input asp-for="@ModelType" type="hidden" />

is generating with 1.0.1

<input name="Type" id="Type" type="hidden" value="RatioContentBlock">

and for 1.1

<input name="ModelType" id="ModelType" type="hidden" value="RatioContentBlock">

So I had to change a line in the binder to catch ModelType instead of Type

[dougbu]

@pierre-weceipt generating the name / id "Type" with 1.0.1 looks like a straight-up bug. Alternatively, something else changed in your app when updating to the 1.1.0 packages. (It's not clear where MVC could get that name.)

Are you suggesting there's a problem with the new HTML?

[pjeannet]

@dougbu no, the only change is that now the name is correctly ModelType instead of previously Type. I think it was a bug in 1.0.1, as I understood razor should be using the var name for input name but was changing ModelType in Type (the previous code was all in a razor view).

But this part is fine for me now, I just had to change a string in my model binder to match the new var name. I'm more interested in understanding why the cli is launching the error message but project is still managing to launch.

[frankabbruzzese]

Since expression like @Model.MyProperty must generate names like MyModel, prefix "Model." is removed, probably in 1.0.1 also "Model" prefix were erroneously removed in some circumstances (I suspect when there are no dots in the total name)

[Eilon]

@pierre-weceipt we would like more info to try and reproduce the problem. Can you show the implementation of your model binder and provider?

@dougbu can you start taking a look at this?

[pjeannet]

@Eilon here is my code : The model binder :

using Microsoft.AspNetCore.Mvc.ModelBinding;
using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using Thelos.NAO.Models.Templates.AccountingTables;
using Thelos.NAO.Models.Templates.Sections.Conditions;
using Thelos.NAO.Models.Templates.Sections.ContentBlocks;

namespace Thelos.NAO.Binders
{
    public class TemplateSectionModelBinder : IModelBinder
    {
        private readonly IModelMetadataProvider _metadataProvider;
        private readonly Dictionary<string, IModelBinder> _binders;

        public TemplateSectionModelBinder(
            IModelMetadataProvider metadataProvider,
            Dictionary<string, IModelBinder> binders
            )
        {
            _metadataProvider = metadataProvider;
            _binders = binders;
        }

        public async Task BindModelAsync(ModelBindingContext bindingContext)
        {
            var typeResult = bindingContext.ValueProvider.GetValue(bindingContext.ModelName + ".Type");
            if (typeResult == ValueProviderResult.None)
            {
                bindingContext.Result = ModelBindingResult.Failed();
                return;
            }

            IModelBinder binder;
            if (!_binders.TryGetValue(typeResult.FirstValue, out binder))
            {
                bindingContext.Result = ModelBindingResult.Failed();
                return;
            }

            // Now know the type exists in the assembly.
            var type = Type.GetType(typeResult.FirstValue);

            if (!typeof(ContentBlock).IsAssignableFrom(type) && !typeof(Condition).IsAssignableFrom(type) && !typeof(Row).IsAssignableFrom(type)
                || (!typeResult.FirstValue.StartsWith("Thelos.NAO.Models.Templates") && !typeResult.FirstValue.StartsWith("Thelos.NAO.Models.AccountingTables")))
            {
                throw new InvalidOperationException("Bad Type");
            }

            var metadata = _metadataProvider.GetMetadataForType(type);
            var model = Activator.CreateInstance(type);
            var result = bindingContext.Result;
            bindingContext.Model = model;

            using (bindingContext.EnterNestedScope(
                metadata,
                bindingContext.FieldName,
                bindingContext.ModelName,
                model: model))
            {
                await binder.BindModelAsync(bindingContext);
            }

            bindingContext.Result = ModelBindingResult.Success(model);
            return;
        }
    }
}

and the provider

using Microsoft.AspNetCore.Mvc.ModelBinding;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Threading.Tasks;
using Thelos.NAO.Models.Templates.AccountingTables;
using Thelos.NAO.Models.Templates.Sections.Conditions;
using Thelos.NAO.Models.Templates.Sections.ContentBlocks;

namespace Thelos.NAO.Binders
{
    public class TemplateSectionModelBinderProvider : IModelBinderProvider
    {
        public IModelBinder GetBinder(ModelBinderProviderContext context)
        {
            if (context == null)
            {
                throw new ArgumentNullException(nameof(context));
            }

            if (context.Metadata.ModelType == typeof(ContentBlock) || context.Metadata.ModelType == typeof(Condition) || context.Metadata.ModelType == typeof(Row))
            {
                var binders = new Dictionary<string, IModelBinder>();
                foreach (var type in typeof(TemplateSectionModelBinderProvider).GetTypeInfo().Assembly.GetTypes())
                {
                    var typeInfo = type.GetTypeInfo();
                    if (typeInfo.IsAbstract || typeInfo.IsNested)
                    {
                        continue;
                    }

                    if (!(typeInfo.IsClass || typeInfo.IsPublic))
                    {
                        continue;
                    }

                    var metadata = context.MetadataProvider.GetMetadataForType(type);
                    var binder = context.CreateBinder(metadata);
                    binders.Add(type.FullName, binder);
                }

                return new TemplateSectionModelBinder(context.MetadataProvider, binders);

            }
            return null;
        }
    }
}

declared in the startup file

services.AddMvc(options =>
             {
                options.ModelBinderProviders.Insert(0, new TemplateSectionModelBinderProvider());
            });

the views contain

@{
    var ModelType = @Model.GetType().FullName;
}
<input asp-for="@ModelType" type="hidden" />

[dougbu]

@pierre-weceipt it's still not clear how your model binder could work. The view should submit the type in a field named ModelType and the binder is looking for Type. Have you updated your binder to expect the correct name?

If the field name is not the problem, have you debugged and found the provider or binder is not invoked at all?

[pjeannet]

@dougbu I agree with you, this shouldn't work, yet it does. Razor is generating the field as 'Type' instead of 'ModelType' when in 1.0.x (this code is for 1.0.x). However it seems that it is solved in 1.1 and I had to change the model binder to listen to 'ModelType' as it should have since the beginning.

[dougbu]

@pierre-weceipt I'd appreciate a small repro for the problem in 1.0.x. Do you still have a sample showing the wrong name generation? Ideal is a small project in a public GitHub repo.

[pjeannet]

@dougbu Here is the project, page "Bug" in the navbar.

[dougbu]

I've confirmed this issue and found it repros when using HTML helpers, including HTML helpers in MVC 5.2.3. Surprised nobody found the issue before; problem has been in the code forever.

Bug is an overly-accepting StartsWith() in ExpressionHelper, line 105. ASP.NET Core MVC 1.0.2 code shown below as well.

Problem does not repro in MVC 1.1.0-preview1 and later because we rewrote ExpressionHelper. The new lastIsModel approach uses string.Equals() instead.

if (text.StartsWith(".model", StringComparison.OrdinalIgnoreCase))
{
    // 6 is the length of the string ".model".
    builder.Remove(0, 6);
}

[dougbu]

Updated issue title and cleared labels other than bug.

@Eilon I'm torn on this one. It can be worked around with new variable names and it appears our customers haven't been impacted over the years. On the other hand, the generated names break model binding, there's no indication where the problem lies, and the fix is simple.

Either way, let's do this all-or-nothing: If we patch Core 1.0.x, I suggest we also patch 5.2.4.

[dougbu]

Proposing this for next 1.0.x patch. Bug is not an issue in 1.1.x.

[Eilon]

This patch bug is approved. Please use the normal code review process w/ a PR and make sure the fix is in the correct branch, then close the bug and mark it as done.

[dougbu]

• fa710e6 in rel/1.0.3
• cherry-picked test changes as a5c7a7e into dev
• did not merge rel/1.0.3 branch into dev because this product code change does not apply and branches have diverged significantly