Feature/auto setup v2 #8596

rserj · 2021-02-14T06:19:25Z

The Second proposal of AutoSetup based on
#4567
Some Key Notes

Implemented as Separate Module which has to be referenced to Web project and Enabled like this
services.AddOrchardCms().AddSetupFeatures("OrchardCore.AutoSetup");
Implemented Auto Setup as Middleware
Ability To start Setup by navigating to specified URL (AutoSetupOptions.TriggerSetupUrl ) (Might be useful for Kubernetes Init containers)
Does not conflict with a Setup module, if AutoSetup configuration wasn't provided Setup screen will be displayed

Skrypt · 2021-02-14T15:27:29Z

This feature always missing to be able to create multiple tenants. You can create one main tenant but can't add tenants inside that default tenant. Maybe it shouldn't, I don't know, but somehow if you want to automate the creation of OC tenants it would probably require to run recipes too on them afterward.

rserj · 2021-02-14T18:11:19Z

@Skrypt I noticed there is a CreateTenantTask, Can the Setup recipe create a Workflow Task and setup another tenant(s) for now?

Another option will be creating a SetupTenant Recipe Step, but it might be tricky since we have to wait until the current recipe has finished its execution, maybe somehow relying on ISetupEventHandler or Deferred tasks/IRecipeStepQueue (in O1).
We might consider a technic to include other installation recipes with the Context from the current recipe.
In the future, we might generate Admin credentials AccessToken-Key/Session-Key so we can use the CLI tool to log in as Admin and manage tenants

rserj · 2021-02-15T19:50:46Z

Added support for Multitenant setup

I do not know why the build is failed:

AutoSetupMiddleware.cs(165,17): error CS0117: 'SetupContext' does not contain a definition for 'AdminEmail'

I didn't touch SetupContext and it has a definition for AdminEmail and other properties
cc: @sebastienros

deanmarcussen

A quick look as I see the build failed.

The reason for the build failure is this pr was merged #7885

so the setup context has changed to use a properties dictionary.

It's interesting as a module, because then you might expect that you can continue to use it to setup tenants, once the default shell is running.

But you wouldn't be able to do that I suspect?

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupMiddleware.cs

deanmarcussen · 2021-02-16T10:22:05Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupStartupFilter.cs

+        /// <returns> The <see cref="Action"/>. </returns>
+        public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
+        {
+            var scopedServices = ShellScope.Services;


can you not inject these?

I'm not sure, I saw it is being used in the other parts, @jtkech can you suggest, please?

@rserj @deanmarcussen

Good question ;)

Yes it is used in some places, but because it is less DI friendly we could say it is better to use the DI when it is possible, we use it e.g. for ISiteService that is a tenant singleton for some reasons but that needs to be executed in a tenant / shell scope context, this to resolve scoped services.

Here it's the same kind of thing, yes IStartupFilter are executed under a shell scope context while building the tenant pipeline, but they are resolved through the tenant / shell container, not through a shell scope, we could have done it but we opted to keep for a tenant the pattern of IStartupFilter as for a regular "app" (here the tenant as an app with its own shell container).

// Create a nested pipeline to configure the tenant middleware pipeline var startupFilters = appBuilder.ApplicationServices.GetService<IEnumerable<IStartupFilter>>();

So here, if you inject IServiceProvider it would be related to the tenant / shell container, not the one of the current shell scope, so you would not be able to resolve scoped services. So yes, using ShellScope.Services is a solution, the other solution is to inject IHttpContextAccessor to get the RequestServices that we take care to be the services of the shell scope if there is a current one ;)

Both are using an AsyncLocal object to be retrieved in a given async execution flow, the current HttpContext or current ShellScope. Personnaly i would use ShellScope, but IHttpContextAccessor is more aspnetcore friendly.

Hmm, maybe another solution through a module startup Configure() that passes a scoped service provider, and with a lower ConfigureOrder to get called earlier. Hmm, i could get an auto setup working by only using the config sources stack + a few lines of code #4567 (comment), but i don't remember what was missing.

i would need to take a deeper look on your PR, i will do soon when i will have time.

src/OrchardCore.Modules/OrchardCore.AutoSetup/Options/AutoSetupOptions.cs

deanmarcussen · 2021-02-16T10:26:29Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/Options/AutoSetupOptions.cs

+        /// Gets or sets the Url which will trigger AutoSetup.
+        /// Leave it Empty if you want to Trigger Setup on any request
+        /// </summary>
+        public string TriggerSetupUrl { get; set; }


Suggested change

public string TriggerSetupUrl { get; set; }

public string AutoSetupSetupUrl { get; set; }

Sorry personally double setup "SetupSetup" confusing me, but I'm ok with it if others don't mind

Typo: suggest - AutoSetupPath

a. Trigger is weird
b. It's more a path than a url.

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupMiddleware.cs

deanmarcussen · 2021-02-16T11:00:31Z

src/OrchardCore.Cms.Web/appsettings.json

+    //    "DatabaseTablePrefix": "",
+    //    "RecipeName": "Agency"
+    //  },
+    //  "SubTenants": [


I think this could just be one array, where you find the Default shell by it's name.

I also thought about this, then decided to split one explicitly since:

It is not obvious, Not everyone knows the "Default" tenant name naming convention

The Default Tenant does not need RequestUrlHost and RequestUrlPrefix params

I think it should use the default sections, including for other tenants.
I also think it should only initialize the tenant the first time it's requested, and not on the first request of the default one.

@sebastienros sorry it is not clear to me, can you please rephrase your concern and/or give me an example?

Not sure which part you didn't understand. What I mean is that last time I saw the code, the tenants were all initialized when the application is starting. It would be better if a tenant was auto-initialized on the first request to it. So if you open the default tenant, it's initialized, then if you open tenant1 it get initialized.

I've implemented on-demand initialization please take a look

src/OrchardCore.Cms.Web/appsettings.json

rserj · 2021-02-16T21:14:58Z

The reason for the build failure is this pr was merged #7885

Thank you

It's interesting as a module, because then you might expect that you can continue to use it to setup tenants, once the default >shell is running.

But you wouldn't be able to do that I suspect?

Right, you won't able to, since you enabling it as a Setup feature AddSetupFeatures("OrchardCore.AutoSetup"), it will be turned off after the Default tenant gets installed.
I tried to create a Recipe step for creating a new Tenant, but It might be a recursion problem.
E.g if you added the NewTenant step into Default's Agency recipe and then try to use the same Agency recipe to install the new Sub-Tenant(s). In this case, we can fix recursion by checking "Site Names" against current Shells OR do not allow use same recipes for Default and Sub tenants. I decided to skip the Recipe step for now

Fixes after code review

sebastienros · 2021-02-18T19:04:02Z

src/OrchardCore.Cms.Web/appsettings.json

+    //    "DatabaseTablePrefix": "",
+    //    "RecipeName": "Agency"
+    //  },
+    //  "SubTenants": [


I think it should use the default sections, including for other tenants.
I also think it should only initialize the tenant the first time it's requested, and not on the first request of the default one.

sebastienros · 2021-02-18T19:04:57Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupMiddleware.cs

+                    }
+                }
+
+                httpContext.Response.Redirect("/");


~/ so it also works with the prefix of the default tenant

Reverting it back, when I use redirection to ~/ it redirects me to https://localhost:5001/~/ 404

@rserj

Yes, ~/ works when using Redirect (returning a RedirectResult) from a controller, because when executing the result the url helper.Content() is called and it uses the pathBase.

The pathBase contains an eventual virtual folder, and at this point it already contains the tenant prefix, the default tenant may have one prefix, so try the following

httpContext.Response.Redirect(httpContext.Request.PathBase)

sebastienros · 2021-02-18T19:05:18Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupMiddleware.cs

+            var stringBuilder = new StringBuilder();
+            foreach (var error in setupContext.Errors)
+            {
+                stringBuilder.Append($"{error.Key} : '{error.Value}'");


sebastienros · 2021-02-18T19:06:10Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupMiddleware.cs

+                stringBuilder.Append($"{error.Key} : '{error.Value}'");
+            }
+
+            _logger.LogError($"AutoSetup failed installing the site '{setupOptions.SiteName}' with errors: {stringBuilder}");


Suggested change

_logger.LogError($"AutoSetup failed installing the site '{setupOptions.SiteName}' with errors: {stringBuilder}");

_logger.LogError("AutoSetup failed installing the site '{SiteName}' with errors: {Errors}", setupOptions.SiteName, stingBuilder);

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupMiddleware.cs

sebastienros · 2021-02-18T19:09:08Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupStartupFilter.cs

+                            stringBuilder.Append(error.ErrorMessage);
+                        }
+
+                        _logger.LogError("AutoSetup did not start, configuration has following errors: {errors}", stringBuilder.ToString());


Suggested change

_logger.LogError("AutoSetup did not start, configuration has following errors: {errors}", stringBuilder.ToString());

_logger.LogError("AutoSetup did not start, configuration has following errors: {Errors}", stringBuilder.ToString());

sebastienros · 2021-02-18T19:13:05Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupStartupFilter.cs

+    /// <summary>
+    /// The filter which registers <see cref="AutoSetupMiddleware"/> to setup the site.
+    /// </summary>
+    public class AutoSetupStartupFilter : IStartupFilter


Is it necessary since you could register the middleware from Configure in the same Startup class that registers this filter?

Thanks for review
I thought about this, I think I could do this by using middleware in the Startup. I guess the main difference is that ASP.NET Core uses IStartupFilters in order to build the application pipeline, by using IStartupFilter I can guarantee that AutosetupMiddleware will be the first in the pipeline. Please correct me if I'm wrong.

Yes, your startup filter will run first (app level ones still run first) as we call the startup filters of a given tenant (using its own app builder) before building its pipeline (so calling the modules startup Configure()).

But for testing you can try to use a startup class, the one related to your auto setup feature, and override the ConfigureOrder property with a negative value, e.g. -100

But just to test it because somewhere i like the idea of using a startup filter, the only thing is that the startup Configure() provides the shell scope service provider, as commented in another comment, but for me not an issue by retrieving it with any other way

Thanks for the reviews, I replaced AutoSetupStartupFilter as a middleware in Statup.cs

sebastienros · 2021-02-18T19:13:47Z

I'd like to see if we can pass the arguments from the json file directly on the command line (since they are read the same way as from the ENV).

rserj · 2021-02-18T19:32:52Z

I'd like to see if we can pass the arguments from the json file directly on the command line (since they are read the same way as from the ENV).

Thanks for your review.
I tested it via env variables set in the launchSettings.json file in the Web project and the Docker container.
Env variables look like this

Default Tenant

 "OrchardCore__OrchardCore_AutoSetup__RootTenant__SiteName": "AutoSetup Example"
 "OrchardCore__OrchardCore_AutoSetup__RootTenant__RecipeName": "Agency"
 "OrchardCore__OrchardCore_AutoSetup__RootTenant__SiteTimeZone": "America/California"
 "OrchardCore__OrchardCore_AutoSetup__RootTenant__AdminUsername": "admin"
 "OrchardCore__OrchardCore_AutoSetup__RootTenant__AdminEmail": "my.admin@example.com"
 "OrchardCore__OrchardCore_AutoSetup__RootTenant__AdminPassword": "p@$Sw0rd"
 "OrchardCore__OrchardCore_AutoSetup__RootTenant__DatabaseProvider": "Sqlite"
 "OrchardCore__OrchardCore_AutoSetup__AutoSetupPath": ""

Single Sub Tenant

"OrchardCore__OrchardCore_AutoSetup__SubTenants__0__SiteName" : "Tenant Site"
"OrchardCore__OrchardCore_AutoSetup__SubTenants__0__RecipeName": "Agency"
"OrchardCore__OrchardCore_AutoSetup__SubTenants__0__SiteTimeZone": "America/California"
"OrchardCore__OrchardCore_AutoSetup__SubTenants__0__AdminUsername": "tenant_admin"
"OrchardCore__OrchardCore_AutoSetup__SubTenants__0__AdminEmail" : "tenant.admin@example.com"
"OrchardCore__OrchardCore_AutoSetup__SubTenants__0__AdminPassword": "p@$Sw0rd"
"OrchardCore__OrchardCore_AutoSetup__SubTenants__0__DatabaseTablePrefix": "tenant"
"OrchardCore__OrchardCore_AutoSetup__SubTenants__0__DatabaseProvider": "Sqlite"
"OrchardCore__OrchardCore_AutoSetup__SubTenants__0__DatabaseConnectionString": ""
"OrchardCore__OrchardCore_AutoSetup__SubTenants__0__RequestUrlHost": ""
"OrchardCore__OrchardCore_AutoSetup__SubTenants__0__RequestUrlPrefix": "tenant"

the 0 - Is an array index

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupMiddleware.cs

jtkech · 2021-02-22T10:14:17Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupMiddleware.cs

+        /// <param name="setupService"> The setup service. </param>
+        /// <param name="shellSettings"> The tenant shell settings. </param>
+        /// <returns> The <see cref="SetupContext"/>. to setup the site </returns>
+        private static async Task<SetupContext> GetSetupContext(BaseTenantSetupOptions options, ISetupService setupService, ShellSettings shellSettings)


GetSetupContextAsync

jtkech · 2021-02-22T10:29:36Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupStartupFilter.cs

+    /// <summary>
+    /// The filter which registers <see cref="AutoSetupMiddleware"/> to setup the site.
+    /// </summary>
+    public class AutoSetupStartupFilter : IStartupFilter


Yes, your startup filter will run first (app level ones still run first) as we call the startup filters of a given tenant (using its own app builder) before building its pipeline (so calling the modules startup Configure()).

But for testing you can try to use a startup class, the one related to your auto setup feature, and override the ConfigureOrder property with a negative value, e.g. -100

But just to test it because somewhere i like the idea of using a startup filter, the only thing is that the startup Configure() provides the shell scope service provider, as commented in another comment, but for me not an issue by retrieving it with any other way

jtkech · 2021-02-22T10:34:55Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupMiddleware.cs

+        /// <returns>
+        /// The <see cref="Task"/>.
+        /// </returns>
+        public async Task<bool> SetupTenant(ISetupService setupService, BaseTenantSetupOptions setupOptions, ShellSettings shellSettings)


SetupTenantAsync()

jtkech · 2021-02-22T10:46:55Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupMiddleware.cs

+        public async Task InvokeAsync(HttpContext httpContext)
+        {
+            var currentShellSettings = ShellScope.Context.Settings;
+            var setupService = httpContext.RequestServices.GetRequiredService<ISetupService>();


Just to say that here we use the AsyncLocal ShellScope to retrieve the settings, but not to resolve the service ;)

For me that's okay to use it or not for both, so just for infos the other way would be

var currentShellSettings = httpContext.RequestServices.GetRequiredService<ShellSettings>();

jtkech · 2021-02-22T10:48:40Z

@rserj

Just did a little review, will try to complete it asap, maybe this night.

src/OrchardCore.Modules/OrchardCore.AutoSetup/Manifest.cs

# Conflicts: # OrchardCore.sln

rserj · 2021-02-22T19:53:47Z

@jtkech @Skrypt Thanks for your reviews, I've applied your suggestions of renaming and description change.
I also got rid of the AutoSetupFilter, and change the dependency resolution of the ShellSettings

rserj · 2021-02-22T20:03:57Z

short demo

jtkech · 2021-03-04T22:59:17Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/Options/TenantSetupOptions.cs

+        /// <summary>
+        /// The default/root shell name.
+        /// </summary>
+        private const string DefaultShellName = "Default";


Not needed, see below

jtkech · 2021-03-04T23:02:09Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/Options/TenantSetupOptions.cs

+        /// <summary>
+        /// Gets the Flag which indicates a Default/Root shell/tenant.
+        /// </summary>
+        public bool IsDefault => string.Equals(ShellName, DefaultShellName, StringComparison.InvariantCultureIgnoreCase);


using OrchardCore.Environment.Shell; string.Equals(ShellName, ShellHelper.DefaultShellName, StringComparison.OrdinalIgnoreCase);

src/OrchardCore.Modules/OrchardCore.AutoSetup/Options/TenantSetupOptions.cs

jtkech · 2021-03-04T23:33:57Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupMiddleware.cs

+                        }
+                    }
+
+                    var redirectUrl = "/";


Did you try my suggestion? The pathBase contains an eventual virtual folder, and at this point it already contains the tenant prefix (the default tenant may have one prefix).

httpContext.Response.Redirect(httpContext.Request.PathBase)

Or maybe

httpContext.Response.Redirect($"{context.Request.PathBase}/");

Did you try my suggestion?

Yes, as you mentioned below context.Request.PathBase can be empty in this case I have to redirect to /.
Plus since we have AutoSetupOptions.AutoSetupPath option the Request Url can be different from Tenant Url in this case I have to redirect to Tenant's Url

jtkech · 2021-03-04T23:34:34Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupMiddleware.cs

+                    }
+
+                    httpContext.Response.Redirect(redirectUrl);
+                    await httpContext.Response.CompleteAsync();


CompleteAsync(): Is it necessary?

jtkech · 2021-03-04T23:36:33Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/Startup.cs

+            var logger = serviceProvider.GetRequiredService<ILogger<Startup>>();
+
+
+            if (currentShellSettings.State == Environment.Shell.Models.TenantState.Uninitialized)


Add an using so that you can just use TenantState.Uninitialized

jtkech · 2021-03-04T23:37:20Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/Startup.cs

+                }
+            }
+
+            base.Configure(app, routes, serviceProvider);


You can remove this line

jtkech · 2021-03-04T23:38:54Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupMiddleware.cs

+            IShellHost shellHost,
+            ILogger<AutoSetupMiddleware> logger)
+        {
+            _logger = logger;


Maybe put the logger at the end to keep the same order

jtkech · 2021-03-05T00:23:39Z

@rserj

One of my concerns was to use or not pre-configured tenants, we can already do that by defining tenant sections, which are recognized as such if they contains a "State": "Uninitialized" field. In that case, when you start the app all pre-configured tenants are created automatically by the shell settings manager. And you can have global values (just under the OrchardCore section) that will be shared by all tenants.

But also makes sense to have a separate section providing data that will only be used if the auto setup feaure is used, and that will override any pre-configured value, as done through the Tenants api controller. Note: But not for all properties in the Tenants mvc controller, e.g. if the database provider is already pre-configured, it is not shown on the setup screen.

So, i'm not against and I will approve, but first i just did another little review.

# Conflicts: # OrchardCore.sln

rserj · 2021-03-05T04:54:48Z

One of my concerns was to use or not pre-configured tenants, we can already do that by defining tenant sections, which are recognized as such if they contains a "State": "Uninitialized" field

Do You mean defining the tenants section in App_Data tenants.json file? Cause this Autosetup feature will make sure and create other Tenants only if the Default tenant has successfully installed.

deanmarcussen · 2021-03-05T08:16:49Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/Startup.cs

+                            stringBuilder.Append(error.ErrorMessage);
+                        }
+
+                        logger.LogError("AutoSetup did not start, configuration has following errors: {errors}", stringBuilder.ToString());


Here, you are using localized strings for the validation, but then recording the validation errors in the log file, so it doesn't make sense that they are localized. Because log files don't contain localization.

If it was going to the front end, it would make sense for them to be localized, but when they are being appended to the log file, then it makes less sense. Because none of the ASP.Net errors, are localized, nor is the actual error message, of AutoSetup did not start.

So having AutoSetup did not start : {errors in another language} doesn't quite add up to me?

thanks for your review, I agree with you, I've removed localization

deanmarcussen · 2021-03-05T08:17:00Z

src/OrchardCore.Modules/OrchardCore.AutoSetup/Startup.cs

+
+            if (currentShellSettings.State == TenantState.Uninitialized)
+            {
+                var optionsAccessor = serviceProvider.GetRequiredService<IOptions<AutoSetupOptions>>();


Suggested change

var optionsAccessor = serviceProvider.GetRequiredService<IOptions<AutoSetupOptions>>();

var options = serviceProvider.GetRequiredService<IOptions<AutoSetupOptions>>().Value;

jtkech · 2021-03-06T02:49:02Z

@rserj

Do You mean defining the tenants section in App_Data tenants.json file? Cause this Autosetup feature will make sure and create other Tenants only if the Default tenant has successfully installed.

No, in any regular config source as the appsettings.json at the root of the app. tenants.json is a kind of config source but that is more intended to be mutated when the setup is completed, it holds some required settings (not all) and has an higher precedence so that a pre-configured value can be overridden (or not), same logic for any other value with the specific appsettings.json under a tenant folder, and that has an higher precedence than e.g. the one at the app root.

A Default tenant always exists (even it is not pre-configured) at least in an Uninitialized state, and we can create and setup another tenant even if the default one is not setup, we currently do this in our unit tests.

But that's okay, as said makes sense to have a separate and dedicated config section for all auto setup data, and then, because other tenants are not intended to be pre-configured (so not auto created by the shell settings manager), we need to use the default tenant to create them. I'm okay with this, but i think you got the idea ;)

Note: But what happens if you create a tenant already pre-configured, hmm i think it will just override its settings

Yes, as you mentioned below context.Request.PathBase can be empty in this case I have to redirect to /

So use $"{context.Request.PathBase}/", maybe check if the PathBase.Value is empty before adding a trailing slash. Note: Using the PathBase ensures that we use the right url encoding, this is what it does through its implicit ToString().

I'm trying it, before talking about url host and prefix, will see later, if you have a virtual folder, can be simulated by doing the following Map() in the main startup, curently the redirect doesn't work, if i use the PathBase it works

    public void Configure(IApplicationBuilder app, IHostEnvironment env)
    {
        app.Map("/virtual", builder =>
        {
            if (env.IsDevelopment())
            {
                builder.UseDeveloperExceptionPage();
            }

            builder.UseStaticFiles();

            builder.UseOrchardCore();
        });
    }

Plus since we have AutoSetupOptions.AutoSetupPath option the Request Url can be different from Tenant Url in this case I have to redirect to Tenant's Url

Okay, you do a Map(options.AutoSetupPath), in that case the PathBase will also contain the setup path, but if you do

app.MapWhen(ctx => ctx.Request.Path.StartsWithSegments(options.AutoSetupPath), appBuilder =>
    appBuilder.UseMiddleware<AutoSetupMiddleware>());

The request PathBase will still contain a virtual folder (if one), the url prefix (if one), but the setup path will be in the request Path, so here also you can redirect by using the PathBase, i tried it it works

Note: We have a StartsWithNormalizedSegments() extension that deals better with trailing slashes.

app.MapWhen(ctx => ctx.Request.Path.StartsWithNormalizedSegments(options.AutoSetupPath), appBuilder =>
    appBuilder.UseMiddleware<AutoSetupMiddleware>());

See in the RunningShellTable how we deal with the host and port (or not) and prefix (or not), see also the Index.cshtml of OC.Tenants, see GetDisplayUrl() for display only, and GetEncodedUrl() that does the url encoding for the link, this through the implicit ToString() of each uri component. Hmm, in this Index.cshtml we add the port of the current request indifferently, i think that if a host is defined (here as a string) it should contain the right port if needed, i will fix it.

Note: If the host is defined (not empty), we still need to take into account the original PathBase that may contain a virtual folder, this before we append to it the tenant prefix, we have an httpContext feature for this.

var pathString = httpContext.Features.Get<ShellContextFeature>().OriginalPathBase;

Then about the prefix, regarding the code in the running shell table, if a host is defined for a tenant, this tenant can have a prefix or not, i tried it with a fake domain by using the localtest.me site, it works with or without defining a prefix for this tenant, so in the Index.cshtml that's okay, we can always add the url prefix segment if not empty.

So, aplying the same, here the code that i tried and that works if we use the above MapWhen(), e.g. it works with a virtual folder and the url is well encoded before updating the location header.

                var pathBase = httpContext.Request.PathBase;
                if (!pathBase.HasValue)
                {
                    pathBase = "/";
                }

                var hostString = httpContext.Request.Host;
                if (!String.IsNullOrEmpty(setupTenant.RequestUrlHost))
                {
                    hostString = new HostString(setupTenant.RequestUrlHost);
                }

                var pathString = httpContext.Features.Get<ShellContextFeature>().OriginalPathBase;
                if (!String.IsNullOrEmpty(shellSettings.RequestUrlPrefix))
                {
                    pathString = pathString.Add('/' + shellSettings.RequestUrlPrefix);
                }

                httpContext.Response.Redirect($"{httpContext.Request.Scheme}://{hostString + pathString}");

                return;

Also, i had some ThrowResponseAlreadyStartedException, in fact when you redirect you just need to bypass the pipeline by doing a return, if we still have this exception we may have to check if the response HasStarted, will see.

But but but, if you define an url host for a given tenant (not the default one), because you pre-created it, the host is already in the ambient context (and also the url prefix), anyway you will not be able to trigger its setup if you don't use the url host that you defined for it. For the default tenant, hmm we fallback to it for any host, even if not yet defined, so if your running instance is already listening for this host, seems to be okay. If so, we would not need the above code, even for the default tenant, and the final code would be

                var pathBase = httpContext.Request.PathBase;
                if (!pathBase.HasValue)
                {
                    pathBase = "/";
                }

                httpContext.Response.Redirect(pathBase);

                return;

Sorry for the long story, more line to describe what i was meaning, than the final code ;)

What do you think about checking that the url prefix doesn't contain more than one segment, as done in the OC.Tenants api and mvc controllers.

Finally, do a Remove and Sort usings in your files, remove unused ones, order alphabetically, but System.* first.

Update: Sometimes it show up that it is initializing, sometimes some exceptions, will see tomorow.

jtkech · 2021-03-08T06:28:21Z

@rserj

First, i don't want to bother you with my suggestions and i don't want to block your PR ;)

For now i only did minor changes, trying it revealed some little issues in other places that i'm fixing in separate PRs. As said above, sometimes the browser pre-sends some requests and then it just shows up that the tenant is initializing, normal and it doesn't prevent the setup to complete, so not a big issue but better to see the redirection operate.

For now, seems to be okay when not using an AutoSetupPath, unless if i intentionaly hit f5 multiple times, so one suggestion would be to not have the AutoSetupPath option, need more time to check it.

Note: Here the singularity is that we are mutating things but through an http GET method.

Also related to multiple requests, sometimes i had yesSql / sqlite exceptions, here also it doesn't prevent the setup already started to complete. For now we only mark that the tenant is initializing, but not immediately and without any locking system. I already thought about this, now maybe worth to do something.

So, the other suggestion would be to prevent multiple setups on the same tenant, maybe a global solution as now there are multiple ways to setup a tenant (Api, Worflow), but still not sure if it is worth doing it, maybe a compromise.

So, need a little more time to complete my tests and commit some suggestions, then you will be free to revert / update what you don't agree with.

jtkech · 2021-03-09T05:32:47Z

@rserj

Sorry, did not have so much time, but just fixed the race conditions, i will commit my suggestions tomorrow

rserj · 2021-03-09T11:15:35Z

@jtkech thanks for reviewing PR, I reworked the redirection logic by using MapWhen and PathBase I tested it with AutoSetupPath, and without it, it works for me.

rserajedinov added 2 commits February 13, 2021 21:53

Add AutoSetup Module

0288b12

Adding Commented Configuration for AutoSetup

ce97f37

rserj mentioned this pull request Feb 14, 2021

AutoSetup feature for Setup module #4567

Closed

Commenting TriggerSetupUrl in the appsettings

2ab324d

rserajedinov added 2 commits February 15, 2021 03:04

Multi-Tenant auto-setup

16abd9c

Add new line to csproj file

6163cd3

deanmarcussen reviewed Feb 16, 2021

View reviewed changes

rserajedinov added 3 commits February 16, 2021 09:09

Merge branch 'dev' into feature/auto-setup-v2

dc2c188

Update SetupContext properties assigment

502becd

Fix async recipes retrieval after code review

7df96bd

Rename TriggerSetupUrl to AutoSetupPath. Add Warning message to config

0e2c0d2

Fixes after code review

sebastienros reviewed Feb 18, 2021

View reviewed changes

rserajedinov added 3 commits February 18, 2021 11:52

Fixing Error Output after code review

ea0c294

Changing Append to AppendLine

d966bd6

Redirecting with ~/ fix after code review

55c5334

jtkech reviewed Feb 22, 2021

View reviewed changes

Skrypt reviewed Feb 22, 2021

View reviewed changes

src/OrchardCore.Modules/OrchardCore.AutoSetup/Manifest.cs Outdated Show resolved Hide resolved

rserajedinov added 3 commits February 22, 2021 08:59

renamings after code review

7a9840a

Move AutoSetupFilter logic to Startup

51b51ca

Merge branch 'dev' into feature/auto-setup-v2

2b82bbd

# Conflicts: # OrchardCore.sln

rserj requested a review from sebastienros February 22, 2021 20:06

rserajedinov added 3 commits February 25, 2021 21:02

fix AutoSetup to multi-targetting

f145941

Add Autosetup to the Core Targets project

93835bd

Add Auto Setup Module Specs

14056c1

rserj requested a review from agriffard as a code owner March 1, 2021 05:20

Some changes in specs

afe5d42

agriffard requested a review from jtkech March 1, 2021 10:56

jtkech reviewed Mar 4, 2021

View reviewed changes

rserajedinov added 2 commits March 4, 2021 17:29

Merge branch 'dev' into feature/auto-setup-v2

1ec420a

# Conflicts: # OrchardCore.sln

Small fixes after code review

118277d

deanmarcussen reviewed Mar 5, 2021

View reviewed changes

rserajedinov added 3 commits March 5, 2021 03:45

Change Options Validation

72e4e88

cli command change in documentation

bc06061

rename optionsAccessor variable after code review

ad25846

Reworking redirection logic after code review

4dbfdd3

jtkech added 8 commits March 9, 2021 22:46

Update AutoSetupMiddleware.cs

b124d61

Update AutoSetupOptions.cs

80c64c2

Update TenantSetupOptions.cs

3162d38

Update Startup.cs

f31b99f

Update OrchardCore.AutoSetup.csproj

a896fd3

Update OrchardCore.Application.Cms.Core.Targets.csproj

fb16c9a

Update AutoSetupMiddleware.cs

26c488b

Update Startup.cs

6fa1dc9

jtkech approved these changes Mar 9, 2021

View reviewed changes

agriffard merged commit 3123a9a into OrchardCMS:dev Mar 9, 2021

	public string TriggerSetupUrl { get; set; }
	public string AutoSetupSetupUrl { get; set; }

	_logger.LogError($"AutoSetup failed installing the site '{setupOptions.SiteName}' with errors: {stringBuilder}");
	_logger.LogError("AutoSetup failed installing the site '{SiteName}' with errors: {Errors}", setupOptions.SiteName, stingBuilder);

	_logger.LogError("AutoSetup did not start, configuration has following errors: {errors}", stringBuilder.ToString());
	_logger.LogError("AutoSetup did not start, configuration has following errors: {Errors}", stringBuilder.ToString());

		var logger = serviceProvider.GetRequiredService<ILogger<Startup>>();


		if (currentShellSettings.State == Environment.Shell.Models.TenantState.Uninitialized)

	var optionsAccessor = serviceProvider.GetRequiredService<IOptions<AutoSetupOptions>>();
	var options = serviceProvider.GetRequiredService<IOptions<AutoSetupOptions>>().Value;

Feature/auto setup v2 #8596

Feature/auto setup v2 #8596

Conversation

rserj commented Feb 14, 2021 • edited

Skrypt commented Feb 14, 2021

rserj commented Feb 14, 2021 • edited

rserj commented Feb 15, 2021 • edited

deanmarcussen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtkech Feb 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rserj Feb 16, 2021 • edited

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

rserj Feb 16, 2021 • edited

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

rserj commented Feb 16, 2021 • edited

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

sebastienros commented Feb 18, 2021

rserj commented Feb 18, 2021 • edited

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 Feb 22, 2021

rserj commented Feb 22, 2021 • edited

rserj commented Feb 22, 2021

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 Mar 4, 2021 • edited

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

jtkech commented Mar 5, 2021

rserj commented Mar 5, 2021 • edited

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 Mar 6, 2021 • edited

jtkech commented Mar 8, 2021

jtkech commented Mar 9, 2021

rserj commented Mar 9, 2021

rserj commented Feb 14, 2021 •

edited

rserj commented Feb 14, 2021 •

edited

rserj commented Feb 15, 2021 •

edited

jtkech Feb 17, 2021 •

edited

rserj Feb 16, 2021 •

edited

rserj Feb 16, 2021 •

edited

rserj commented Feb 16, 2021 •

edited

rserj commented Feb 18, 2021 •

edited

rserj commented Feb 22, 2021 •

edited

jtkech Mar 4, 2021 •

edited

rserj commented Mar 5, 2021 •

edited

jtkech commented Mar 6, 2021 •

edited