Need ServiceFileProvider similar to ServiceFilterAttribute

mchender commented 7 years ago

I'd like to be able to retrieve an instance of my file provider from DI similar to how ServiceFilterAttribute works. https://docs.microsoft.com/en-us/aspnet/core/mvc/controllers/filters#servicefilterattribute

services.Configure<RazorViewEngineOptions>(options =>
{
     options.FileProviders.Clear();
      options.FileProviders.Add(new ServiceFileProvider(typeof(CacheFileProvider)));
});

This was discussed in another thread in Entity Framework but since they found a workaround I don't know that anyone followed up submitting the issue here. https://github.com/aspnet/EntityFramework/issues/4544#issuecomment-184794032

My current workaround was to follow the guidance provided in the following comment which stores a reference to IServiceProvider in a static property. However, they indicate it is a bad practice that should be avoided if possible. https://github.com/aspnet/DependencyInjection/issues/294#issuecomment-142206501

My true goal was to develop a library that can render a MVC Razor view to a string. There is an example of how to do this in the Entropy repository. However, it requires the view to exist on disk. https://github.com/aspnet/Entropy/tree/dev/samples/Mvc.RenderViewToString

My templates will come from a database so I need to be able to pass the view as a string and get the rendered output as a string. I updated the sample from Entropy by adding a custom implementation of IFileProvider that will load the the template from the cache. When I pass in the string from the database it adds the template to the cache using a randomly generated guid as the key. When I request the view from the IRazorViewEngine I use that guid as the file name so that later I can use it as the key to retrieve the template from the cache. https://github.com/mchender/Entropy/tree/dev/samples/Mvc.RenderViewToString

Snippet from https://github.com/mchender/Entropy/blob/dev/samples/Mvc.RenderViewToString/Program.cs

public static Task<string> RenderViewAsync(RazorViewToStringRenderer helper)
        {

            var model = new EmailViewModel
            {
                UserName = "User",
                SenderName = "Sender",
                UserData1 = 1,
                UserData2 = 2
            };

            string template = $@"
                                    <!DOCTYPE html>

                                    <html>
                                    <body>
                                        <div>
                                            @model Mvc.RenderViewToString.EmailViewModel

                                    `       Hello @Model.UserName,

                                            <p>

                                                This is a generic email about something.<br />

                                                It also contains some content that is generated by a partial view.<br />
                                                @inject Mvc.RenderViewToString.EmailReportGenerator ReportGenerator

                                                This is the partial view content.<br />
                                                @ReportGenerator.GenerateReport(Model.UserData1, Model.UserData2).<br />
                                                <br />
                                            </p>
                                        </div>
                                        <footer>
                                            Thanks,<br />
                                            @Model.SenderName
                                        </footer>
                                    </body>
                                    </html>
                                    ";

            return helper.RenderViewToStringAsync(template, model);
}

Snippet from https://github.com/mchender/Entropy/blob/dev/samples/Mvc.RenderViewToString/RazorViewToStringRenderer.cs

public async Task<string> RenderViewToStringAsync<TModel>(string template, TModel model)
{
            var actionContext = GetActionContext();

            //add the template to the cache
            var name = Guid.NewGuid().ToString();
            _cache.Set(name, template);

            var viewEngineResult = _viewEngine.FindView(actionContext, name, false);

Snippet from https://github.com/mchender/Entropy/blob/dev/samples/Mvc.RenderViewToString/CacheFileInfo.cs

private void GetView(string viewName)
{
            string fileNameWithoutExtension = Path.GetFileNameWithoutExtension(_viewPath);
            if (fileNameWithoutExtension == "_ViewImports")
                _viewContent = null;
            else
            {
                _lastModified = DateTime.UtcNow;
                _exists = true;
                IMemoryCache cache = ServiceProviderProvider.ServiceProvider.GetRequiredService<IMemoryCache>();
                var template = cache.Get(fileNameWithoutExtension);
                _viewContent = Encoding.UTF8.GetBytes(template.ToString());
            }
}

It would be much cleaner if IMemoryCache could be loaded using DI instead of having to use a static property to access IServiceProvider to request the service myself.

muratg commented 6 years ago

@danroth27 @DamianEdwards thoughts?

danroth27 commented 6 years ago

@Eilon @pranavkm @rynowak

pranavkm commented 6 years ago

For the issue that @mchender is running in to, IConfigureOptions would be a better way to get to it rather than using the static field:

public class CustomFileProviderOptions : IConfigureOptions<RazorViewEngineOptions>
{
    private IFileProvider _fileProvider;

    public CustomFileProviderOptions(IMyApplicationFileProvider fileProvider)
    {
        _fileProvider = fileProvider;
    }

    public void Configure(RazorViewEngineOptions options)
    {
        options.FileProviders.Clear();
        options.FileProviders.Add(_fileProvider);
    }
}

services.AddMvc();
services.AddTransient<IConfigureOptions<RazorViewEngineOptions>, CustomFileProviderOptions>();
services.AddSingleton<IMyApplicationFileProvider, CacheFileProvider>(); // Use a custom marker interface so you aren't conflating it with random IFileProvider instances in the DI

If we do make this a first class feature (and I'm not entirely convinced we should), it would be consistent to ensure Hosting IHostingEnvironment.ContentRootFileProvider \ WebRootFileProvider and StaticFiles also support this.

rynowak commented 6 years ago

It would be amazing to have Configure<TOptions, TArg1>(Action<TOptions, TArg1>) where TArg1 is resolved from services. Basically the code that @pranavkm provided except the Configure method calls your delegate and is provided by the options system.

We have to write these classes like this all of the time in MVC and it's tedious.

/cc @davidfowl is this something that was discuussed?

davidfowl commented 6 years ago

It's not a bad idea but its noisy because you have to specify all generic arguments. @HaoK recently made registering IConfigureOptions a bit nicer in 2.1. You can do this now:

Old:

services.AddTransient<IConfigureOptions<RazorViewEngineOptions>, CustomFileProviderOptions>();

New:

services. ConfigureOptions<CustomFileProviderOptions>();

mchender commented 6 years ago

@pranavkm that provides a workaround for the question in this post. Is there some equivalent workaround which would allow me to get around having to pass an instance of my file provider when configuring the hosting environment?

IFileProvider fileProvider = new CacheFileProvider();
services.AddSingleton<IHostingEnvironment>(new HostingEnvironment
{
       ApplicationName =  applicationEnvironment.ApplicationName,
       WebRootFileProvider = fileProvider,
});

rynowak commented 6 years ago

It's not a bad idea but its noisy because you have to specify all generic arguments.

But you still have to write the class:

public class CustomFileProviderOptions : IConfigureOptions<RazorViewEngineOptions>
{
    private IFileProvider _fileProvider;

    public CustomFileProviderOptions(IMyApplicationFileProvider fileProvider)
    {
        _fileProvider = fileProvider;
    }

    public void Configure(RazorViewEngineOptions options)
    {
        options.FileProviders.Clear();
        options.FileProviders.Add(_fileProvider);
    }
}

5/7 loc of that are boilerplate. Feels bad man

services.Configure<RazorViewEngineOptions, IMyApplicationFileProvider>((options, provider) =>
{
        options.FileProviders.Clear();
        options.FileProviders.Add(_fileProvider);
});

VS

services.ConfigureOptions<CustomFileProviderOptions>();
...
public class CustomFileProviderOptions : IConfigureOptions<RazorViewEngineOptions>
{
    private IFileProvider _fileProvider;

    public CustomFileProviderOptions(IMyApplicationFileProvider fileProvider)
    {
        _fileProvider = fileProvider;
    }

    public void Configure(RazorViewEngineOptions options)
    {
        options.FileProviders.Clear();
        options.FileProviders.Add(_fileProvider);
    }
}

Tratcher commented 6 years ago

Still not sure why this discussion is in this repo...

mchender commented 6 years ago

@Tratcher based on this comment https://github.com/aspnet/EntityFrameworkCore/issues/4544#issuecomment-184794032

mchender commented 6 years ago

@pranavkm you can ignore my follow up question. It looks like I don't have to set WebRootFileProvider on the HostingEnvironment for the application to work properly. I made the changes you suggested and I was able to get DI to resolve the services I need in CacheFileInfo. Here's the commit. https://github.com/mchender/Entropy/commit/7405b5965d86c793f81b05c44908a611a4813b6a

My issues are resolved. Feel free to close this issue if needed. Otherwise I am interested in the result of the conversation between @rynowak and @davidfowl and how to either make this a first class feature or at least make how it's configured as intuitive as possible.

HaoK commented 6 years ago

Ok, we could add a new generic base configure, maybe something like:

services.ConfigureFromDI<TOptions, TDependency>(Action<TOptions> configure)

which would register as transient something like:

class ConfigureFromDI<TOptions, TDependency> : IConfigureOptions<TOptions> {
    ConfigureWithDI(TDependency dep) => Dep = dep;
    TDependency Dep { get; }
}

If the configure options needs more than one dependency, to use this sugar you'd have to wrap them and register it and get them out as properties?

services.AddScoped<MyOptionsDep>();
class MyOptionsDep {
   MyOptionsDep(MyService1 s, MyService2, s2);

  MyService1 Service1 { get; }
  MyService2 Service1 { get; }
}

davidfowl commented 6 years ago

Leave out the FromDI part it would just be configure with a bunch of overloads for dependencies. We can start with 5 dependencies 😄

HaoK commented 6 years ago

Ok so just have 5 overloads, with 1 to 5 extra args, and they will just roll their own otherwise. I'll file an issue

HaoK commented 6 years ago

https://github.com/aspnet/Options/issues/236

aspnet-hello commented 6 years ago

This issue was moved to aspnet/Home#2450

aspnet / StaticFiles

Need ServiceFileProvider similar to ServiceFilterAttribute #184