Add a service that allows constructing parts of an ApplicationModel instance

[mgubis]

public abstract class PageApplicationModelPartsFactory
{
    public abstract ParameterModel CreateDefault(....);
} 

BTW the compat expectations of an API like this are:

1. This API will respect things like compat switches, so if we make behaviour change it's backed by a compat switch
2. We're allowed to make new overloads and call those instead if requirements change. The existing APIs will work the same and might be obsoleted over time.

---

We've been using .NET Core 2.1 for our project so far and tried to upgrade to 2.2. We are using customized IPageApplicationModelProvider. Since we wanted to preserve the functionality of the default razor provider, we are inheriting from DefaultPageApplicationModelProvider (as suggested in official Microsoft paper: https://docs.microsoft.com/en-us/aspnet/core/razor-pages/razor-pages-conventions?view=aspnetcore-2.2 (Replace the default page app model provider). The problem is, that in 2.2, DefaultPageApplicationModelProvider became internal and thus we can't inherit from it.

Is this way of replacing the default page app model provider not supported any more? Is there any other preferred way available?

Thank you.

[pranavkm]

The problem is, that in 2.2, DefaultPageApplicationModelProvider became internal and thus we can't inherit from it.
Unfortunately the type was in the .Internal namespace and inheriting from it was never a supported scenario. We have a longer discussion about this here - #4932

Is this way of replacing the default page app model provider not supported any more? Is there any other preferred way available?

In MVC, provider types, including IPageApplicationModel provider instances are invoked in their declared order. The expected approach is to register a custom application model provider after the default one (an order of 0 should suffice since the default ones are initialized to -1000.). More commonly, we expect users to author conventions to modify the results of the application model provider.

I'll work on getting the docs updated, it shouldn't have recommended inheriting from the DefaultApplicationModelProvider. In the meanwhile, if you could detail your scenario, I'd be happy to provide the code that you would need and possibly use that as the sample in the docs.

[mgubis]

Thank you for quick answer - I really appreciate it.

I'll try to explain our use case:

We have a in-house developed framework, which generates JavaScript code for available postbacks of the Page. That code does lot of additional stuff (i.e. blocks UI, prevents duplicate postbacks ...). This framework takes all methods of the Page decorated with attribute PostbackHandler, assumes name of the postback to be the name of the method and generates code. It is bit difficult to change this behavior, since it is now used in many projects and would in many cases introduce quite high QA costs.

The problem we were facing was, that DefaultPageApplicationModelProvider (which is responsible for extracting page handlers) didn't identify these methods (because their names didn't start with OnPost). That's why we have inherited from DefaultPageApplicationModelProvider.

Compressed snippet of our provider (working in 2.1) is following:

        public class PageApplicationModelProvider : DefaultPageApplicationModelProvider
        {
            public PageApplicationModelProvider(IModelMetadataProvider modelMetadataProvider, IOptions<MvcOptions> options) : base(modelMetadataProvider, options) { }

            private PageHandlerModel processPostbackHandlerMethod(PostbackHandlerAttribute attr, MethodInfo method)
            {
                // create page handler model
                PageHandlerModel pageHandlerModel = new PageHandlerModel(method, method.GetCustomAttributes(inherit: true))
                {
                    Name = method.Name,
                    HandlerName = SwiftPostbackManager.Current.MethodToHandlerName(method),
                    HttpMethod = "POST"
                };

                int paramIndex = 0;
                // now populate parameter info properly
                foreach (ParameterInfo pi in method.GetParameters())
                {
                    PageParameterModel phm = this.CreateParameterModel(pi);
                    phm.BindingInfo = new BindingInfo()
                    {
                        BindingSource = BindingSource.Form,
                        BinderType = typeof(JsonBinder),
                        BinderModelName = SwiftPostbackManager.Current.ParameterIndexToInputName(paramIndex)
                    };

                    pageHandlerModel.Parameters.Add(phm);
                    paramIndex++;
                }
                return pageHandlerModel;
            }

            protected override PageHandlerModel CreateHandlerModel(MethodInfo method)
            {
                PostbackHandlerAttribute postbackAttribute = method.GetCustomAttribute<PostbackHandlerAttribute>(true);
                if (postbackAttribute != null)
                    return processPostbackHandlerMethod(postbackAttribute, method);
                else
                    return base.CreateHandlerModel(method);
            }
        }

As you can see, we were in fact reusing most of the provider functionality and did only small extension on the CreateHandlerModel method. I think this is not possible with adding new IPageApplicationModelProvider - we would have to rewrite complete functionality of the DefaultPageApplicationModelProvider. It seems, that the DefaultPageApplicationModelProvider was nicely prepared for this (documentation + nice virtual CreateHandlerModel method) - is there any other way to do this in 2.2?

Thanks a lot for your help.

[pranavkm]

A convention like this should work. Could you give it a try?

public class PostBackHandlerModelConvention : IPageApplicationModelConvention
{
    public void Apply(PageApplicationModel model)
    {
        var methods = model.HandlerType.GetMethods();

        foreach (var method in methods.Where(m => m.IsDefined(typeof(PostbackHandlerAttribute), inherit: true)))
        {
            var existingPageHandler = model.HandlerMethods.FirstOrDefault(m => m.MethodInfo == method);
            if (existingPageHandler != null)
            {
                // If an existing non-post back handler method exists for the action, remove it since we'll recreate it soon.
                model.HandlerMethods.Remove(existingPageHandler);
            }

            var attribute = method.GetCustomAttribute<PostbackHandlerAttribute>(inherit: true);
            model.HandlerMethods.Add(CreatePostbackHandlerMethod(attribute, method));
        }
    }

    private PageHandlerModel CreatePostbackHandlerMethod(PostbackHandlerAttribute attr, MethodInfo method)
    {
        // create page handler model
        ...
    }
}

[mgubis]

Thanks - I tried it out and it works. I came across two problems, which I had to resolve and it would be great if you could confirm, that it is the only way (no other solution (more elegant) exists):

1. CreatePostbackHandlerMethod has to also create CreateParameterModel for parameters of the postback handler. The old code was using CreateParameterModel of the DefaultPageApplicationModelProvider. I just grabbed the code from the repo of 2.2 and put it into Convention. The problem is, that in case the implementation of CreateParameterModel changes in DefaultPageApplicationModelProvider in a future, my convention will still generate handler models with old parameter model creation. Is there any way around this or do we have to live with it?

2. Implementation of the CreateParameterModel requires IModelMetadataProvider. The registration of the convention requires instance of the convention - so it is not possible to use DI to populate IModelMetadataProvider. I worked this around with following:

private class PageApplicationModelConventionOptions : IConfigureOptions<RazorPagesOptions>
        {
            private readonly SwiftInvoker.PageApplicationModelConvention convention;
            public PageApplicationModelConventionOptions(SwiftInvoker.PageApplicationModelConvention convention)
            {
                this.convention = convention;
            }

            public void Configure(RazorPagesOptions options) => options.Conventions.Add(this.convention);
        }

registering it with:

services.AddSingleton<IConfigureOptions<RazorPagesOptions>, PageApplicationModelConventionOptions>();
services.AddSingleton<SwiftInvoker.PageApplicationModelConvention>();

Is there any other (more proper) way to get around this?

Also, during the migration from 2.1 to 2.2 I came across problem with auto-discovery of the controllers (I worked around it by settings assemblies manually using AddApplicationPart - and this solution is fine with me). Should I open the issue for this (so it is documented in case others encounter the same)?

Thanks for your help.

[pranavkm]

The problem is, that in case the implementation of CreateParameterModel changes in DefaultPageApplicationModelProvider in a future, my convention will still generate handler models with old parameter model creation.

That's true. @rynowak what do you think about having a factory method for application model pieces? e.g. var parameterModel = PageParameterModel.CreateDefault(...);?

Implementation of the CreateParameterModel requires IModelMetadataProvider. The registration of the convention requires instance of the convention - so it is not possible to use DI to populate IModelMetadataProvider. I worked this around with following:

Your code looks fine. There's some syntactical sugar for Options which might work too:

services.AddOptions<RazorPagesOptions>()
  .Configure<IModelMetadataProvider>((options, modelMetadataProvider) => {  ...  })

[rynowak]

I would generally really prefer to not expose our guts as an API. I need to know more about the scenario.

[mgubis]

@pranavkm - Thanks for the assistance

@rynowak - I will try to summarize:

1. We need to customize the way the handlers of the page are discovered (we need to discover them by the custom attribute, which is also used in other parts of our in-house framework). All other properties of the handlers should be created in default way (including PageParameterModel).
2. It was no problem to implement 1) in .NET Core 2.1 - we just derived from DefaultPageApplicationModelProvider and has overriden CreateHandlerModel. Method CreateParameterModel of DefaultPageApplicationModelProvider was accessible to us.
3. DefaultPageApplicationModelProvider was made internal in .NET Core 2.2 - thus we can't inherit from it. Thanks to @pranavkm, we have implemented convention, which modifies PageApplicationModel. Unfortunately the code must create PageParameterModel (in order to create PageHandlerModel). We had to copy the code of the method CreateParameterModel from DefaultPageApplicationModelProvider (because we want to preserve default behavior). This is problematic in long term perspective, because we need to maintain this part of the code and check with any future .NET Core release for any change.

Please let me know if you need any more details.

[rynowak]

OK thanks that makes sense.

It was no problem to implement 1) in .NET Core 2.1 - we just derived from DefaultPageApplicationModelProvider and has overriden CreateHandlerModel

From my point of view this is somewhat problematic for us because in means that all of the default implementations are effectively frozen. We have to think about these as official contracts. This is why a lot of these things have gone internal. Rationalizing what kinds of things people are doing is easier when they talk to us about it, so I appreciate your patience.

That's true. @rynowak what do you think about having a factory method for application model pieces? e.g. var parameterModel = PageParameterModel.CreateDefault(...);?

@pranavkm - What do we need the IMMP for? I think this will be much more resilient to change if we make a service instead.

public abstract class PageApplicationModelPartsFactory
{
    public abstract ParameterModel CreateDefault(....);
} 

BTW the compat expectations of an API like this are:

1. This API will respect things like compat switches, so if we make behaviour change it's backed by a compat switch
2. We're allowed to make new overloads and call those instead if requirements change. The existing APIs will work the same and might be obsoleted over time.

[pranavkm]

We use IMMP to produce the ModelMetadata enhanced BindingInfo for parameters. Making it a service sounds reasonable.

[pranavkm]

Updated the title and description for the issue with the suggested API. @mgubis we'd be happy to accept a PR for this change 👍

[mgubis]

@pranavkm - I'd be happy to supply a PR for this change. Just as a word of warning - I am not extremely familiar with internals of the .NET Core development (I would even say I am beginner when it comes to .NET Core internals) - so I might need a help here and there. Can I contact you in case of any questions?

@rynowak, @pranavkm - Just to refine/summarize the proposed solution (I am not sure whether I've got it right):

1. We will create public IPageApplicationModelPartsProvider interface in Microsoft.AspNetCore.Mvc.ApplicationModels namespace. This interface will contain methods from the DefaultPageApplicationModelProvider (i.e. CreateParameterModel, CreatePropertyModel - this will be refined during development).
2. We will create internal DefaultPageApplicationModelPartsProvider class implementing IPageApplicationModelPartsProvider, which will actually contain implementation moved from the DefaultPageApplicationModelProvider.
3. We will inject IPageApplicationModelPartsProvider into the DefaultPageApplicationModelPartsProvider (can we change the signature of the constructor or should we inject into member instead?) and use methods from injected IPageApplicationModelPartsProvider in the internals of DefaultPageApplicationModelProvider.
4. We will register DefaultPageApplicationModelPartsProvider in AddServices method.

Is this solution okay with you? This effectively moves logic away from DefaultPageApplicationModelProvider to DefaultPageApplicationModelPartsProvider - I am not sure whether this is desired.

[pranavkm]

1 & 2 look fine. Since DefaultApplicationModelProvider is internal, adding parameters to it's constructor is not a breaking change.
4. Yup services.TryAddSingleton<IPageApplicationModelPartsProviders, DefaultPageApplicationModelPartsProvider>()

Your plan sounds great!

[mgubis]

I have finally managed to implement the changes as agreed (I apologize for the long time it took). Nevertheless, I came across following issue:

DefaultPageApplicationModelProvider is currently unit tested. DefaultPageApplicationModelPartsProvider (created by me) is not directly unit tested (I didn't create neither specific tests, nor migrated tests from DefaultPageApplicationModelProvider). Its functionality is indirectly tested via existing unit tests of DefaultPageApplicationModelProvider. Should I also rework related tests or is the indirect testing via DefaultPageApplicationModelProvider sufficient (I am new to the project and would like to have advisement from people working on the project for long time)?

Thanks a lot for the patience :).