ShellDescriptorManager depends on YesSql.ISession and isn't ideal when only using OrchardCore without the CMS

[MikeAlhayek]

Is your feature request related to a problem? Please describe.

I am trying to use OrchardCore framework outside of the CMS in one of my projects. I want to use theming component so I configured the services like this

services.AddOrchardCore()
	    .AddMvc()
	    .WithTenants()
	    .AddDataStorage()
	    .AddTheming()
	    .AddCaching()
	    .ConfigureServices((tenantServices, serviceProvider) =>
	    {
	        tenantServices.AddResourceManagement();
	        tenantServices.AddTagHelpers<LinkTagHelper>();
	        tenantServices.AddTagHelpers<MetaTagHelper>();
	        tenantServices.AddTagHelpers<ResourcesTagHelper>();
	        tenantServices.AddTagHelpers<ScriptTagHelper>();
	        tenantServices.AddTagHelpers<StyleTagHelper>();

	    })
	    .Configure((app, routes, serviceProvider) =>
	    {
	        app.UseSession();
	        app.UseAuthorization();
	        app.UseAuthentication();
	        routes.MapAreaControllerRoute("default", ModuleName, "{controller=Home}/{action=Index}/{id?}");
	    });

The .AddDataStorage() extension registers ShellDescriptorManager class. However, ShellDescriptorManager seems to depend on YesSql.ISession In my case, I am not using YesSql.ISession like the CMS does.

Describe the solution you'd like

I think we should create IShellDescriptorStore which and inject it into ShellDescriptorManager instead of injecting ISession.

The IShellDescriptorStore would look something like this.

    public interface IShellDescriptorStore
    {
        public Task<ShellDescriptor> GetDefaultAsync();

        public Task<ShellDescriptor> GetAsync(int serialNumber);

        public Task UpdateAsync(ShellDescriptor shellDescriptor);
    }

Then we can create ShellDescriptorStore which depends on YesSql.ISession to make it easier to replace implementations and removed the YesSql.ISession dependency.

Describe alternatives you've considered

Currently, I implemented IShellDescriptorManager using CustomShellDescriptorManager class. Then replace the default implementation like so

tenantServices.Replace(ServiceDescriptor.Scoped<IShellDescriptorManager, CustomShellDescriptorManager>());

If IShellDescriptorStore is an acceptable approach, I can send in a pull request.

[Skrypt]

//cc @jtkech

Seems like it already works by using the DI so I'm not sure if it's necessary. Should we consider this for other reasons?

[MikeAlhayek]

@Skrypt I don't really want to maintain a separate copy of the ShellDescriptorManager. What if the default logic changed down the line. I would rather to keep rely on the default implementation. Introducing IShellDescriptorStore will enable one to easily swap out the store implementation without having to worry about the default implementation of IShellDescriptorStore.

Also, since ShellDescriptorManager is part of OrchardCore, it should not directly depend on YesSql.ISession.

[Skrypt]

I'm not sure, but I think it will also require to remove entirely YesSQL from OrchardCore.Infrastructure.
There is also DatabaseShellConfigurationSources and DatabaseShellsSettingsSources that requires YesSQL.
Also, need to see if there are not other Interfaces that will be required to be implemented.
I leave that one for @jtkech as he knows everything about the Shell.

[MikeAlhayek]

I am not asking that we remove the entire OrchardCore.Data.YesSql dependency from OC.Infrastructure "although this may not be a bad idea.

I do thinks adding IDatabaseShellConfigurationsStore, IDatabaseShellsSettingsStore and IShellDescriptorStore is a better way to isolate dependencies.

We could add a new project called OrchardCore.Infrastructure.YesSql and move the 3 implementation of IDatabaseShellConfigurationsStore, IDatabaseShellsSettingsStore and IShellDescriptorStore over to the new project. That'll give us a clean OrchardCore.Infrastructure without the YesSql dependency.

[ns8482e]

@CrestApps IMHO you don't need AddDataStorage as that is used to read/write features in YesSQL db.

For theming using OC Framework in ASP.NET core app (without CMS) you need to configure your startup as following. The only caveat is that you need to create your own MyFeaturesManager in your solution to exclude dependency that depends on YesSQL db. And Implement IThemeSelector

// Configure Features and Theming
 services.AddOrchardCore().WithFeatures()
                .AddTheming().AddMvc();

            services.AddOrchardCore().ConfigureServices( svc=>
            {
                svc.AddScoped<IShellFeaturesManager, MyFeaturesManager>();
                svc.AddScoped<IThemeSelector, MyThemeSelector>();
            });

MyFeaturesManager ( same code as ShellFeatureManager except removed dependency)

public class MyFeaturesManager : IShellFeaturesManager
    {
        private readonly IExtensionManager _extensionManager;
        private readonly ShellDescriptor _shellDescriptor;

        public MyFeaturesManager(
            IExtensionManager extensionManager,
            ShellDescriptor shellDescriptor
        )
        {
            _extensionManager = extensionManager;
            _shellDescriptor = shellDescriptor;
        }

        public Task<IEnumerable<IFeatureInfo>> GetEnabledFeaturesAsync()
        {
            return Task.FromResult(_extensionManager.GetFeatures().Where(f => _shellDescriptor.Features.Any(sf => sf.Id == f.Id)));
        }

        public Task<IEnumerable<IFeatureInfo>> GetAlwaysEnabledFeaturesAsync()
        {
            return Task.FromResult(_extensionManager.GetFeatures().Where(f => f.IsAlwaysEnabled || _shellDescriptor.Features.Any(sf => sf.Id == f.Id && sf.AlwaysEnabled)));
        }

        public Task<IEnumerable<IFeatureInfo>> GetDisabledFeaturesAsync()
        {
            return Task.FromResult(_extensionManager.GetFeatures().Where(f => _shellDescriptor.Features.All(sf => sf.Id != f.Id)));
        }

        public Task<(IEnumerable<IFeatureInfo>, IEnumerable<IFeatureInfo>)> UpdateFeaturesAsync(IEnumerable<IFeatureInfo> featuresToDisable, IEnumerable<IFeatureInfo> featuresToEnable, bool force)
        {
            throw new NotSupportedException();
            
        }

        public Task<IEnumerable<IExtensionInfo>> GetEnabledExtensionsAsync()
        {
            // enabled extensions are those which have at least one enabled feature.
            var enabledIds = _extensionManager.GetFeatures().Where(f => _shellDescriptor
                .Features.Any(sf => sf.Id == f.Id)).Select(f => f.Extension.Id).Distinct().ToArray();

            // Extensions are still ordered according to the weight of their first features.
            return Task.FromResult(_extensionManager.GetExtensions().Where(e => enabledIds.Contains(e.Id)));
        }
    }

[jtkech]

@Skrypt @CrestApps @ns8482e

I will look at it tomorrow more in depth if I have time.

As stated by @ns8482e you may not need to call AddDataStorage().

For info when we use WithTenants(), normally the features states are intended to be defined from the configuration, WithTenants() already register a simple ConfiguredFeaturesShellDescriptorManager that doesn't depend on yesSql.

OrchardCore/src/OrchardCore/OrchardCore/Modules/Extensions/OrchardCoreBuilderExtensions.cs

Lines 57 to 74 in 1586fa0

	/// <summary>
	/// Registers tenants defined in configuration.
	/// </summary>
	public static OrchardCoreBuilder WithTenants(this OrchardCoreBuilder builder)
	{
	var services = builder.ApplicationServices;
	services.AddSingleton<IShellsSettingsSources, ShellsSettingsSources>();
	services.AddSingleton<IShellsConfigurationSources, ShellsConfigurationSources>();
	services.AddSingleton<IShellConfigurationSources, ShellConfigurationSources>();
	services.AddTransient<IConfigureOptions<ShellOptions>, ShellOptionsSetup>();
	services.AddSingleton<IShellSettingsManager, ShellSettingsManager>();
	return builder.ConfigureServices(s =>
	{
	s.AddScoped<IShellDescriptorManager, ConfiguredFeaturesShellDescriptorManager>();
	});
	}

Then it depends from which source you want to define the feature states, simple to manage fixed states, more complex with dynamic states as you may have to call handlers and so on. Yes I understand the need to only override the store part, hmm but it may be a sensitive part as we may do things that are related to the YesSql behavior, e.g.

OrchardCore/src/OrchardCore/OrchardCore.Infrastructure/Shell/ShellDescriptorManager.cs

Lines 118 to 123 in 1586fa0

	_session.Save(shellDescriptorRecord);
	// In the 'ChangedAsync()' event the shell will be released so that, on request, a new one will be built.
	// So, we commit the session earlier to prevent a new shell from being built from an outdated descriptor.
	await _session.SaveChangesAsync();

But if you only need fixed states per tenant, you could just register a simple implementation of IShellDescriptorManager after the one registered by WithTenants(), finally maybe your own extension replacing WithTenants().

[ns8482e]

@jtkech

The issue is ShapePlacementParsingStrategy depends on IShellFeaturesManager. The only implementation added in DI is defined in OrchardCore.Infrastructure see below.

OrchardCore/src/OrchardCore/OrchardCore.Infrastructure/Shell/ShellOrchardCoreBuilderExtensions.cs

Lines 21 to 23 in 1586fa0

	services.AddScoped<IShellFeaturesManager, ShellFeaturesManager>();
	services.AddScoped<IShellDescriptorFeaturesManager, ShellDescriptorFeaturesManager>();
	});

So one solution would be move ID registration of ShellFeaturesManager from infrastructure project to OrchardCore project

[jtkech]

@ns8482e Ah okay thanks

Yes, I only looked at WithTenants() and didn't check if we use AddTheming() but still without a yesSql dependency.

Yes I agree, here only ShellDescriptorManager would be registered, then ShellDescriptorFeaturesManager and ShellFeaturesManager could be registered e.g. in AddHostingShellServices().

In the meantime can be splitted by using custom OC builder extensions

[ns8482e]

@jtkech

Verified that following works - I guess if ShellFeaturesManager and ShellDescriptorFeaturesManager gets registered OOTB with AddOrchardCore() would be nice

  services.AddOrchardCore()
                .AddMvc()
                .WithTenants()
                .AddTheming()
                .ConfigureServices(svc =>
                {
                    svc.AddScoped<IShellFeaturesManager, ShellFeaturesManager>();
                    svc.AddScoped<IShellDescriptorFeaturesManager,ShellDescriptorFeaturesManager>();
                    svc.AddScoped<IThemeSelector, MyThemeSelector>();
                });

and appsettings.json

{
"OrchardCore": {
    "Default": {
      "State": "Running",
      "Features": ["Theme1", "Theme2"]
    }
  }
}

[MikeAlhayek]

@jtkech since I no longer need AddDataStorage and the approach provided Niraj worked, I no longer need this. However, I still strongly suggest we add IDatabaseShellConfigurationsStore, IDatabaseShellsSettingsStore and IShellDescriptorStoreas explained above. I can put in a pull request if you like. If so, Would you like for me to put the implementation into a new project called OrchardCore.Infrastructure.YesSql?

[jtkech]

@CrestApps Yes, we already use this for DocumentManager that injects an IDocumentStore whose implementation may be a DatabaseDocumentStore or a FileDocumentStore. Yes, feel free to start a new PR.

Hmm, maybe IShellDescriptorStore could be implemented in OC.Data.YesSql.

Then not sure about IDatabaseShellConfigurationsStore and IDatabaseShellsSettingsStore , we already have related custom app level helpers, e.g. AddDatabaseShellsConfiguration() where the extensibility is more related to config sources.

OrchardCore/src/OrchardCore/OrchardCore.Infrastructure/Shells.Database/Extensions/DatabaseShellsOrchardCoreBuilderExtensions.cs

Lines 12 to 20 in 1586fa0

	public static OrchardCoreBuilder AddDatabaseShellsConfiguration(this OrchardCoreBuilder builder)
	{
	var services = builder.ApplicationServices;
	services.Replace(ServiceDescriptor.Singleton<IShellsSettingsSources, DatabaseShellsSettingsSources>());
	services.Replace(ServiceDescriptor.Singleton<IShellConfigurationSources, DatabaseShellConfigurationSources>());
	return builder;
	}

Hmm, but yes the above extension is defined in OC.Infrastructure and could be moved to OC.Abstractions.

@ns8482e

Verified that following works - I guess if ShellFeaturesManager and ShellDescriptorFeaturesManager gets registered OOTB with AddOrchardCore() would be nice

Thanks for confirming it, yes would be good I think, maybe worth that you suggest a PR for this.

[sebastienros]

We could abstract this storage services and be able to provide other ways that don't use yessql, or even SQL in general.
Meaning being able to switch implementations, even though we provide the yessql one by default, and maybe only.

[TFleury]

Issue was closed but ShellDescriptorManager still depends on YesSql.ISession

I think ISession have to be replaced by IDocumentStore.