OSOE-548: Upgrade to Orchard Core 1.6 in Hosting-Tenants

[Psichorex]

OSOE-548

[jtkech]

@Psichorex Okay, see my #55 (comment)

[jtkech]

@Psichorex

Finally I think you are right to use an IFeatureEventHandler because ...ed event are executed in the deferred task initiated by the feature manager, so the one that we trigger will run after which is better.

For info in OC I suggested a PR to have a FeatureEventHandler class defining virtual methods so that you just have to override the ones you need, as we already have with the ModularTenantEvents class.

Then I think that on any feature enabled/disabled event, that can be related to a condition or conditional feature, you can trigger a deferred task but as said only once by using a local boolean e.g. _deferredTask. And in the deferred task you can resolve your options and the shell feature manager to apply all the logic.

So at least we have 2 deferred tasks, the one of the shell feature and ours, and as a maximum, if we enable/disable features, 2 deferred tasks again for a total of 4 deferred tasks.

So here what I quickly did for testing and to show the idea.

public class FeatureGuardEventHandler : FeatureEventHandler
{
    private bool _deferredTask;

    public override Task DisabledAsync(IFeatureInfo feature) => CheckFeatureConditionsAsync(feature);
    public override Task EnabledAsync(IFeatureInfo feature) => CheckFeatureConditionsAsync(feature);

    public Task CheckFeatureConditionsAsync(IFeatureInfo feature)
    {
        if (_deferredTask)
        {
            return Task.CompletedTask;
        }

        _deferredTask = true;

        ShellScope.AddDeferredTask(async scope =>
        {
            // For testing, in place of resolving here the options.
            Dictionary<string, List<string>> conditions = new()
            {
                { "OrchardCore.Feeds", new List<string> { "OrchardCore.Apis.GraphQL" } },
            };

            var shellFeaturesManager = ShellScope.Services.GetRequiredService<IShellFeaturesManager>();
            var enabledFeatures = (await shellFeaturesManager.GetEnabledFeaturesAsync()).ToArray();
            var enabledFeaturesIds = enabledFeatures.Select(f => f.Id).ToHashSet();

            var featuresToEnableIds = new HashSet<string>();
            var featuresToDisableIds = new HashSet<string>();

            foreach (var condition in conditions)
            {
                var hasConditional = enabledFeaturesIds.Contains(condition.Key);
                var hasCondition = enabledFeaturesIds.Intersect(condition.Value).Any();

                if (hasCondition && !hasConditional)
                {
                    featuresToEnableIds.Add(condition.Key);
                }

                if (!hasCondition && hasConditional)
                {
                    featuresToDisableIds.Add(condition.Key);
                }
            }

            if (featuresToEnableIds.Count == 0 && featuresToDisableIds.Count == 0)
            {
                return;
            }

            var avalaibleFeatures = await shellFeaturesManager.GetAvailableFeaturesAsync();

            var featuresToEnable = avalaibleFeatures
                .Where(feature => featuresToEnableIds.Contains(feature.Id))
                .ToArray();

            var featuresToDisable = enabledFeatures
                .Where(feature => featuresToDisableIds.Contains(feature.Id))
                .ToArray();

            if (featuresToEnable.Length == 0 && featuresToDisable.Length == 0)
            {
                return;
            }

            await shellFeaturesManager.UpdateFeaturesAsync(featuresToDisable, featuresToEnable, true);
        });

        return Task.CompletedTask;
    }
}

Notice that we no longer use _shellDescriptorManager, we could have and in that case, as said we can resolve the descriptor directly, for info .GetShellDescriptorAsync() is only useful to get the descriptor from the database to first build the tenant container, it is then registered in the tenant container DI.

[Psichorex]

@jtkech Thanks for all the information.
Based on my test after introducing the AddDefferedTask() part most of the issues are gone.
Also I made changes on how the service actually handles activation duplication:

In my opinion this is the safest way to never try to enable the same Feature twice using LINQ.Except()
This way allthough the conditionalFeatureIds might have already enabled features in it beause some of the condition features for them are the same, featureIdsToEnable will now never contain enabled features thanks to Except()
If the collection result of Except() has a Count of 0 we return as we do not have any Features that should be enabled.
I also introduced a part where we check for conditionals that do not exist:

This might help the usage of this whole service on condition there is a slight misspelling that would otherwise be hard to find.

Everything as far I have tested is working now.
Any combination of conditions and conditionals do not break the application.
Manually disabling conditions will disable conditionals we have a separate handler method that executes on DisabledAsync...
Manually turning back conditions will again turn on conditionals as intended.

[jtkech]

@Psichorex All good then!

• Personally I would not keep KeepConditionallyEnabledFeaturesEnabledAsync and DisableConditionallyEnabledFeaturesAsync, just trigger the deferred task on any feature change (enabled / disabled) in your handler, but only once by using a local boolean e.g. _deferredTaskRegistered, so nothing to do if already set to true.

  Here we are multiplexing multiple feature events in only one, that's why I first suggested to use an IShellDescriptorManagerEventHandler but the event happens a little too early.

• And then apply all the logic in the deferred task (acting as a checker) based on the current state of the shell descriptor (that may have been changed in the meantime by another thread/process), this to change the state of any conditional feature if needed.

  Here we are handling the event in the deferred task.

[Psichorex]

@jtkech I would rather avoid removing KeepConditionallyEnabledFeaturesEnabledAsync and DisableConditionallyEnabledFeaturesAsync as they both work and somewhat contribute to a more readable code as the functionalities are separated. Also they don't need to utilize a Deferred Task as disabling can only happen by user interaction which will only trigger 1 Handler execution and problem was only during a Tenant setup where all the basic OrchardFeatures and recipe enabled features were getting enabled continuously.

Also I tried the local boolean prior you mentioning it because I thought about that already but it is causing errors.
The first handler execution will set the bool to true and the DeferredTask will never run again.
Seems that every single Handler run has a different scope to it. So the DeferredTask will run as many times as the Handler but at least it prevents recursive calls which were causing the ShellDescriptor going out of sync.

If you think that the current solution is sufficient please leave an approved label on the PR as you have highly contributed on this review for which I am really grateful.

[jtkech]

@Psichorex No problem and sorry if I bother you a little ;) I only describe how i think it should be done.

... they both work and somewhat contribute to a more readable code as the functionalities are separated.

For me there is only one feature, change the state of conditionals based on the state of their conditions. In the sample code (see below) I introduced some checkings, e.g you can't enable a DefaultTenantOnly feature, you can't disable an IsAlwaysEnabled feature (it would result in an infinite loop), I may have missed one but it's more maintainable to have all this logic in only one place.

Also they don't need to utilize a Deferred Task as disabling can only happen by user interaction which will only trigger 1 Handler execution.

This is not a good reason, it should also work with a recipe (a setup one or not) disabling multiple features, anyway disabling a single feature may disable multiple ones through the dependency graph.

Seems that every single Handler run has a different scope to it. So the DeferredTask will run as many times as the Handler.

As I have tried it doesn't behave like this, in a given scope by using a boolean our deferred task is triggered only once. But yes, if it enables/disables feature(s), our handler is called again in another deferred task (and scope) with the boolean equal to false, so we trigger our deferred task a second (and last) time but normally for nothing (unless another thread/process changed the features states).

But yes, as it is currently done without the bool, a deferred task is triggered for each feature to enable.

---

Also for info, yes in the deferred task you should resolve again scoped services as we are in a different scope. Yes, you can reference a tenant singleton, but in that case better to init a variable in the handler var someService = _someService so that the deferred task closure references that variable in place of the scoped handler holding _someService, this from another scope. For the options I recommend to resolve them in the deferred task too.

---

So I think it is worth to try the following where I introduced some additional checkings as mentioned above, quickly done so I could have made a mistake, otherwise if it doesn't work it would mean that there is something else to understand and fix.

public class FeatureGuardEventHandler : FeatureEventHandler
{
    private bool _deferredTask;

    public override Task DisabledAsync(IFeatureInfo feature) => TriggerDeferredTaskAsync();
    public override Task EnabledAsync(IFeatureInfo feature) => TriggerDeferredTaskAsync();

    public Task TriggerDeferredTaskAsync()
    {
        if (_deferredTask)
        {
            return Task.CompletedTask;
        }

        _deferredTask = true;

        ShellScope.AddDeferredTask(async scope =>
        {
            // Can be checked from the handler.
            if (scope.ShellContext.Settings.IsDefaultShell())
            {
                //return;
            }

            var options = scope.ServiceProvider.GetRequiredService<IOptions<ConditionallyEnabledFeaturesOptions>>().Value;
            if (options.EnableFeatureIfOtherFeatureIsEnabled is null ||
                options.EnableFeatureIfOtherFeatureIsEnabled.Count == 0)
            {
                return;
            }

            var conditions = options.EnableFeatureIfOtherFeatureIsEnabled;
            var shellFeaturesManager = scope.ServiceProvider.GetRequiredService<IShellFeaturesManager>();

            var enabledFeatures = (await shellFeaturesManager.GetEnabledFeaturesAsync()).ToArray();
            var enabledFeaturesIds = enabledFeatures.Select(f => f.Id).ToHashSet();

            var featuresToEnableIds = new HashSet<string>();
            var featuresToDisableIds = new HashSet<string>();
            foreach (var condition in conditions)
            {
                var hasConditional = enabledFeaturesIds.Contains(condition.Key);
                var hasCondition = enabledFeaturesIds.Intersect(condition.Value).Any();

                if (hasCondition && !hasConditional)
                {
                    featuresToEnableIds.Add(condition.Key);
                }

                if (!hasCondition && hasConditional)
                {
                    featuresToDisableIds.Add(condition.Key);
                }
            }

            if (featuresToEnableIds.Count == 0 && featuresToDisableIds.Count == 0)
            {
                return;
            }

            var avalaibleFeatures = await shellFeaturesManager.GetAvailableFeaturesAsync();
            var featuresToEnable = avalaibleFeatures
                .Where(feature => featuresToEnableIds.Contains(feature.Id))
                .ToArray();

            var featuresToDisable = enabledFeatures
                .Where(feature => featuresToDisableIds.Contains(feature.Id))
                .ToArray();

            if (featuresToEnable.Any(feature => feature.DefaultTenantOnly || feature.EnabledByDependencyOnly))
            {
                throw new InvalidOperationException("Can't enable a 'DefaultTenantOnly' feature ...");
            }

            if (featuresToDisable.Any(feature => feature.IsAlwaysEnabled || feature.EnabledByDependencyOnly))
            {
                throw new InvalidOperationException("Can't disable an 'IsAlwaysEnabled' feature ...");
            }

            if (featuresToEnable.Length == 0 && featuresToDisable.Length == 0)
            {
                return;
            }

            if (featuresToEnable.Intersect(featuresToDisable).Any())
            {
                throw new InvalidOperationException("A feature can't be both enabled and disabled ...");
            }

            await shellFeaturesManager.UpdateFeaturesAsync(featuresToDisable, featuresToEnable, true);
        });

        return Task.CompletedTask;
    }
}

[Psichorex]

@jtkech I reworked the whole Handler based on your solution.
You are right and now I see it's much more clear to have 1 Handling method for all of this. I refactored your solution so that it fits into our analysers. But now there's only one method in a DeferredTask and everything is working.
@Piedone Please review the new code here.

[Piedone]

I got it, yes, but it's up to Roland to reply.

[Psichorex]

For me doing anything in Feature Manager never executes the DefferedTask again. So locally the functionality for me with a local boolean is nit working as desired per Piedone. I will double check it but I was not able to catch any breakpoints after the tenant was set up.

[jtkech]

I will double check it

Yes please.

but I was not able to catch any breakpoints after the tenant was set up.

That's the weird thing for me ;)

How a boolean held by a scoped service can be then always equal to true?

If it still doesn't work for you, can you share a simple project and repro steps?

So that I could check it with the exact same code, thanks.

[Piedone]

Address the remaining remarks of JT.

[Psichorex]

@jtkech Hi JT I found the problem was caused by Rider suggesting that the boolean should be private static bool but making the field static was causing it to keep it's value eventhough we are shifting scoped.
Now everything is working.

[Psichorex]

@Piedone That .Exists() in Lombiq.Hosting.Tenants.Management/Services/SetupWithRecipesFilterService call is failing the NuGet Publish for some reason. It is very interesting because it is failing with a build error but I can't see any. Replacing the Exists with Any() could be a quick solution.

[Piedone]

It's just that Helpful Libraries got an update since the most recent NuGet alpha, so I'm publishing a new package of it too.

[jtkech]

@Psichorex

Okay cool then.

Only one thing, you set the boolean to true in the wrong place, should be set before triggering the deferred task, not inside the deferred task itself, this is not an Executed but a Triggered checking.

    if (_deferredTaskTriggered)
    {
        return Task.CompletedTask;
    }

    _deferredTaskTriggered = true;
    ShellScope.AddDeferredTask(async scope =>
    {
        ...

Otherwise deferred tasks would be still triggered, anyway not so good to reference a field of a service resolved from a scope that is no longer valid, better to use an intermediate var but normally for read only.

[Psichorex]

@jtkech Thank you so much for all the help and knowledge you gave here. I really appriciate it.
I also fixed your last remark.

[jtkech]

@Psichorex no problem, thanks for the last fix