Use of new StaticFileOptions for Modules #10118

ns8482e · 2021-08-15T22:11:13Z

Use of new StaticFileOptions() for module files, instead resolving from DI.

~~Replace app.UseStaticFiles(options); to use app.UseStaticFiles(); and configuring Module's FileProviders in PostConfigure.~~

~~This allows external module ( e.g. Blazor Server) to inject any additional FileProviders in tenant's pipeline - that ultimately used by StaticFileMiddleware.~~

Static Files from HostWebRoot can be served using app.UseStaticFiles() in tenant pipeline.

services.AddOrchardCore()
                .AddMvc()
                .WithTenants()
                .Configure((app, routes, serviceProvider) =>
                {
                    app.UseStaticFiles();
                });

Fixes #6505
Fixes #2966
Fixes #6474

…leOptions Fixes OrchardCMS#6505 OrchardCMS#2966

willnationsdev · 2021-08-17T07:44:11Z

lol, wow, I just started investigating the possibilities, ran into the issue, saw the workaround, and found it was being fixed soon. Gotta love open-source. :-D

sebastienros · 2021-08-19T17:47:18Z

src/OrchardCore/OrchardCore/Modules/Extensions/ConfigureModuleStaticFileOptions.cs

+                options.FileProvider = _fileProvider;
+            }
+
+            var cacheControl = _shellConfiguration.GetValue("StaticFileOptions:CacheControl", $"public, max-age={TimeSpan.FromDays(30).TotalSeconds}, s-max-age={TimeSpan.FromDays(365.25).TotalSeconds}");


s-maxage typo ?

Since there are settings, maybe put these values in there too.

looks like typo, will fix it. It's reading from settings if available- the second param is default fallback

Updated code and docs to s-maxage

sebastienros · 2021-08-19T17:48:26Z

src/docs/reference/core/Modules/README.md

@@ -60,9 +60,12 @@ You can find a sample application here: [`OrchardCore.Mvc.Web`](../../../../Orch

 The following configuration values are used by default and can be customized:

+By default each tenant doesn't serve static files from Host `wwwroot`. To serve Host `wwwroot` for each tenant, set `ServeHostWebRoot` to `true` in tenant config.


Has it always been the case?

Currently static files from Host wwwroot is only served to Default tenant (and not to other tenants) by configuring app.UseStaticFiles() in Host startup before app.UseOrchardCore()

Keeping the same behavior by setting default value to false

jtkech · 2021-08-20T03:36:55Z

src/OrchardCore/OrchardCore/Modules/Extensions/ConfigureModuleStaticFileOptions.cs

+            _shellConfiguration = shellConfiguration;
+            _environment = environment;
+        }
+        public void PostConfigure(string name, StaticFileOptions options)


Add new line before

jtkech · 2021-08-20T03:37:50Z

src/OrchardCore/OrchardCore/Modules/Extensions/ConfigureModuleStaticFileOptions.cs

+            name = name ?? throw new ArgumentNullException(nameof(name));
+            options = options ?? throw new ArgumentNullException(nameof(options));
+
+            if (name != Microsoft.Extensions.Options.Options.DefaultName)


Maybe add an using above so that here we could just use Options.DefaultName

using Microsoft.Extensions.Options; is already included at line 7, but compiler complains when use Options.DefaultName

jtkech · 2021-08-20T03:39:02Z

src/OrchardCore/OrchardCore/Modules/Extensions/ConfigureModuleStaticFileOptions.cs

+            {
+                // Serve Tenant Static files only from module
+                options.FileProvider = _fileProvider;
+            }


Maybe we still need to set options.RequestPath = ""

As this could be vary based on source of static file and developer's configuration, may be we don't use PostConfigure and we just use new StaticFileOptions() for module files?

jtkech · 2021-08-20T04:04:34Z

src/OrchardCore/OrchardCore/Modules/Extensions/ConfigureModuleStaticFileOptions.cs

+            }
+
+            // Cache static files for a year as they are coming from embedded resources and should not vary
+            ctx.Context.Response.Headers[HeaderNames.CacheControl] = cacheControl;


Hmm, but not fully true if we compose with the WebRootFileProvider that may be related to not embedded files.

Maybe we need 2 UseStaticFiles(), then maybe the same options but at least with 2 separate default values.

As this could be vary based on source of static file, may be we don't use PostConfigure and we just use new StaticFileOptions() for module files?

jtkech · 2021-08-20T04:31:34Z

@ns8482e @sebastienros Just did a little review

Hmm, for content root files each tenant already compose with ContentRootFileProvider without any option

OrchardCore/src/OrchardCore/OrchardCore/Modules/Extensions/ApplicationBuilderExtensions.cs

Lines 20 to 22 in ac7598c

    
           env.ContentRootFileProvider = new CompositeFileProvider( 
        
               new ModuleEmbeddedFileProvider(appContext), 
        
               env.ContentRootFileProvider);

Maybe at the tenant level we could also compose with the WebRootFileProvider without needing an option for this.

Now not sure we need a post configuration, maybe jut a regular one, then as said in the review maybe a separate static file options as the WebRootFileProvider may be related to non embedded files.

Need a little more time to focus on it again ;) I will complete my review asap.

ns8482e · 2021-08-20T13:15:12Z

@jtkech , @sebastienros

Maybe at the tenant level we could also compose with the WebRootFileProvider without needing an option for this.

The real issue is due to following lines,

OrchardCore/src/OrchardCore/OrchardCore/Modules/Extensions/ServiceCollectionExtensions.cs

Lines 210 to 213 in ac7598c

    
                           var options = serviceProvider.GetRequiredService<IOptions<StaticFileOptions>>().Value; 
        
                           options.RequestPath = ""; 
        
                           options.FileProvider = fileProvider;

Line 210 evaluates IOptions<StaticFileOptions> outiside StaticMiddleware
Line 213 replaces all framework provided FileProviders resolved via IOptions<StaticFileOptions> including WebRootFileProvider, StaticWebAssetsFileProvider and ManifestEmbeddedFileProvider

So even if a developer uses UseStaticFiles() inside tenant pipeline, the resolved FileProviers are always ModuleStaticFileProviders

IMHO that value of IOptions<StaticFileOptions> must only be resolved inside StaticMiddleware so that all Framework provided FileProviders and Custom FileProviders that uses IOptions pattern to configure are available to StaticMiddleware when use UseStaticFiles() without any params.

May be better option is not to use or configure IOptions<StaticFileOptions> at all and change the line 210 as following and not interfere any values of IOptions<StaticFileOptions>

var options = new StaticFileOptions(); 
  
 options.RequestPath = ""; 
 options.FileProvider = fileProvider;

And Let developer control whether to serve files from wwwroot in their modules in tenant pipeline using UseStaticFiles()

ns8482e · 2021-08-23T12:20:46Z

@sebastienros @jtkech removed, IPostConfigureOptions

jtkech · 2021-08-23T23:51:53Z

@ns8482e Yes, I prefer this ;)

And let people do what they want from the app with our helpers, e.g.

services.AddOrchardCms()
    .Configure((app, routes, serviceProvider) =>
    {
        app.UseStaticFiles();
    });

Maybe add it to the doc, and maybe saying that for mutating / collaborate to the related host options better to still do it at the host level. Note: As I remember, not so good at the tenant level to mutate host level options because we may build tenants concurrently, we had this kind of problem with routing services / options where we needed at the tenant level to remove from the DI some host level singleton services / configuration options before re-registering them.

So okay for now that at the tenant level we always use a new StaticFileOptions() as we already do for tenant static files.

OrchardCore/src/OrchardCore.Modules/OrchardCore.Tenants/Startup.cs

Lines 106 to 117 in c1f261f

    
                       app.UseStaticFiles(new StaticFileOptions 
        
                       { 
        
                           FileProvider = tenantFileProvider, 
        
                           DefaultContentType = "application/octet-stream", 
        
                           ServeUnknownFileTypes = true, 
        
                           // Cache the tenant static files for 30 days 
        
                           OnPrepareResponse = ctx => 
        
                           { 
        
                               ctx.Context.Response.Headers[HeaderNames.CacheControl] = $"public, max-age={TimeSpan.FromDays(30).TotalSeconds}, s-max-age={TimeSpan.FromDays(365.25).TotalSeconds}"; 
        
                           } 
        
                       });

Notice that here the CacheControl doesn't come from the shell configuration, hmm but we would need to have separate tenant level options, so that's okay for now, will see if needed.

For module embedded static files maybe we could use the same code pattern as above, then not sure about setting other properties e.g. DefaultContentType and ServeUnknownFileTypes as done above. Before they were coming from the current host level options.

Hmm, so one idea would be to still resolve the host level StaticFileOptions but in place of mutating it, clone it to a new one with the same properties unless for RequestPath, FileProvider and OnPrepareResponse. So exactly as before but without mutating the current host level StaticFileOptions.

ns8482e · 2021-08-25T00:52:44Z

@jtkech @agriffard what would be accurate path to document this? IMHO This is related to framework and and not related to Tenant module.

jtkech · 2021-08-25T03:20:05Z

@ns8482e

what would be accurate path to document this? IMHO This is related to framework and and not related to Tenant module.

Or maybe in a section dedicated to static files but didn't find it, anyway should not block this PR.

Last thing, what you think about still resolving the host level StaticFileOptions to still read the properties that we don't override, but this to populate the new StaticFileOptions that you created.

The idea is to work as before for modules static files but just without mutating the host StaticFileOptions. Hmm, but maybe better now that host StaticFileOptions don't act on module static files.

So as you want ;) let me know before approving.

ns8482e · 2021-08-25T14:53:22Z

@ns8482e IMHO, StaticFileOptions (being singleton) should only resolve from DI in StaticMiddleware, resolving early may result in PostConfigure that's added in later in pipeline not being called e.g. Blazor Server's PostConfigure

jtkech · 2021-08-25T19:46:05Z

@ns8482e

First for info I did some tests, when we resolve the options while building a tenant pipeline, we get a different instance per tenant, so no problem when building multiple tenants concurrently as I was talking about.

StaticFileOptions (being singleton) should only resolve from DI in StaticMiddleware, resolving early may result in PostConfigure that's added in later in pipeline not being called e.g. Blazor Server's PostConfigure

Okay but I was talking about still resolving it in our OCBuilder.Configure(), as we actually do, so after any services.Configure() and services.PostConfigure(), but this without mutating the options because it is the source of the related issue (if I understood correctly).

Anyway, I'm fine with just creating a new StaticFileOptions in place of resolving it, this for modules static files, so I will approve, but just before, I will commit a change to show what I mean => Still resolving the options but without mutating it (as it seems to be the issue), this to still use the values that we don't override so that it behaves as before.

Suggested change: Still resolve options but without mutating it

Fix CI build

ns8482e · 2021-08-25T23:06:19Z

@jtkech verified the changes - it works!

src/OrchardCore/OrchardCore/Modules/Extensions/ServiceCollectionExtensions.cs

jtkech · 2021-08-25T23:40:26Z

@ns8482e

Thanks for having tested it!

Use of IPostConfigureOptions<StaticFileOptions> to configure StaticFi…

e0dbe3f

…leOptions Fixes OrchardCMS#6505 OrchardCMS#2966

ns8482e requested a review from agriffard as a code owner August 15, 2021 22:11

removed unused variable

64dbdae

sebastienros reviewed Aug 19, 2021

View reviewed changes

sebastienros requested a review from jtkech August 19, 2021 17:48

fixed typo

26c7473

jtkech reviewed Aug 20, 2021

View reviewed changes

Removed IPostConfigureOptions

55daece

ns8482e changed the title ~~Use of IPostConfigureOptions<StaticFileOptions> to configure StaticFileOptions for Modules~~ Use of new StaticFileOptions for Modules Aug 23, 2021

fix typo

9c82fec

jtkech added 3 commits August 25, 2021 21:57

Update ServiceCollectionExtensions.cs

d1fcbf2

Suggested change: Still resolve options but without mutating it

Update ServiceCollectionExtensions.cs

dae5058

Fix CI build

Suggested change: Still resolve options but without mutating it.

58a88e6

ns8482e commented Aug 25, 2021

View reviewed changes

src/OrchardCore/OrchardCore/Modules/Extensions/ServiceCollectionExtensions.cs Show resolved Hide resolved

jtkech approved these changes Aug 25, 2021

View reviewed changes

sebastienros merged commit ac1402b into OrchardCMS:main Sep 2, 2021

ns8482e deleted the ns8482e/#6505 branch September 2, 2021 18:22

deanmarcussen mentioned this pull request Oct 7, 2021

s-max-age should be s-maxage? #8911

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use of new StaticFileOptions for Modules #10118

Use of new StaticFileOptions for Modules #10118

ns8482e commented Aug 15, 2021 •

edited

Loading

willnationsdev commented Aug 17, 2021

sebastienros Aug 19, 2021 •

edited

Loading

sebastienros Aug 19, 2021

ns8482e Aug 19, 2021

ns8482e Aug 19, 2021

sebastienros Aug 19, 2021

ns8482e Aug 19, 2021 •

edited

Loading

jtkech Aug 20, 2021

jtkech Aug 20, 2021

ns8482e Aug 20, 2021 •

edited

Loading

jtkech Aug 20, 2021

ns8482e Aug 20, 2021

jtkech Aug 20, 2021

ns8482e Aug 20, 2021

jtkech commented Aug 20, 2021 •

edited

Loading

ns8482e commented Aug 20, 2021 •

edited

Loading

ns8482e commented Aug 23, 2021

jtkech commented Aug 23, 2021

ns8482e commented Aug 25, 2021

jtkech commented Aug 25, 2021

ns8482e commented Aug 25, 2021

jtkech commented Aug 25, 2021

ns8482e commented Aug 25, 2021 •

edited

Loading

jtkech commented Aug 25, 2021

		@@ -60,9 +60,12 @@ You can find a sample application here: [`OrchardCore.Mvc.Web`](../../../../Orch

		The following configuration values are used by default and can be customized:

		By default each tenant doesn't serve static files from Host `wwwroot`. To serve Host `wwwroot` for each tenant, set `ServeHostWebRoot` to `true` in tenant config.

Use of new StaticFileOptions for Modules #10118

Use of new StaticFileOptions for Modules #10118

Conversation

ns8482e commented Aug 15, 2021 • edited Loading

willnationsdev commented Aug 17, 2021

sebastienros Aug 19, 2021 • edited Loading

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

ns8482e Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ns8482e Aug 20, 2021 • edited Loading

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

jtkech commented Aug 20, 2021 • edited Loading

ns8482e commented Aug 20, 2021 • edited Loading

ns8482e commented Aug 23, 2021

jtkech commented Aug 23, 2021

ns8482e commented Aug 25, 2021

jtkech commented Aug 25, 2021

ns8482e commented Aug 25, 2021

jtkech commented Aug 25, 2021

ns8482e commented Aug 25, 2021 • edited Loading

jtkech commented Aug 25, 2021

ns8482e commented Aug 15, 2021 •

edited

Loading

sebastienros Aug 19, 2021 •

edited

Loading

ns8482e Aug 19, 2021 •

edited

Loading

ns8482e Aug 20, 2021 •

edited

Loading

jtkech commented Aug 20, 2021 •

edited

Loading

ns8482e commented Aug 20, 2021 •

edited

Loading

ns8482e commented Aug 25, 2021 •

edited

Loading