Introduce EnabledByDependencyOnly flag for features to allow the feature's state to be auto (enabled or disabled)

[MikeAlhayek]

Fix #12279

A feature with EnabledByDependencyOnly, will automatically get enabled/disabled as needed by other features.

Let's say we have the following features

[assembly: Feature(
    Id = "OC.Notification",
    EnabledByDependencyOnly = true
)]

[assembly: Feature(
    Id = "OC.Notification.Email",
    Dependencies = new[]
    {
        "OC.Notification"
    }
)]

[assembly: Feature(
    Id = "OC.Notification.Push",
    Dependencies = new[]
    {
        "OC.Notification"
    }
)]

[assembly: Feature(
    Id = "OC.Notification.Web",
    Dependencies = new[]
    {
        "OC.Notification"
    }
)]

The feature OC.Notification will automatically get enabled when any of OC.Notification.Email, OC.Notification.Push, or OC.Notification.Web are enabled by the user.

At the same time, OC.Notification will automatically get disabled when all of OC.Notification.Email, OC.Notification.Push, and OC.Notification.Web are disabled.

The key difference between EnabledByDependencyOnly and IsAlwaysEnabled is that the latter will always be enabled regardless whether it's needed or not.

Checkout this screen-cast to the end to see the full behavior in action.

[jtkech]

Hmm but Listable is an UI concern, not sure that the underlying feature should be aware of this. This is the OC.Features that would need to decide how to list a feature based on a real feature state as IsAlwaysEnabled, would need a real feature state e.g. EnabledOnlyByDependency.

So a non listable dependency would be enabled by a dependent, but what happens if you disable all dependents, the dependency would be still enabled and we could not disable it as it is non listable.

Would need to check if there is any remaining dependent, if not we could auto disable the dependency. Hmm and if this dependency has other dependencies ;)

[jtkech]

Hmm maybe OC.Features settings where we could add/remove which features are non listable

[MikeAlhayek]

@jtkech that settings is done in the Manifest as far as I know. if there other way to define settings to the feature? But may be you have better ideas.

[jtkech]

Did you see this part of my previous comment?

So a non listable dependency would be enabled by a dependent, but what happens if you disable all dependents, the dependency would be still enabled and we could not disable it as it is non listable.

Would need to check if there is any remaining dependent, if not we could auto disable the dependency. Hmm and if this dependency has other dependencies ;)

[MikeAlhayek]

@jtkech sorry I missed that first comment. I did not scroll up when I saw the commend.

Good point. I agree with you, we'll need a way to check for dependent. maybe hook into features events after a module is disable, then disable features. not sure how the right approach there.

When a module is disable, check if any of it's dependencies is listable. If any, then check it any active feature still need is or disable it automatically

[MikeAlhayek]

@jtkech I added logic into the ShellFeaturesManager to automatically enable/disable non-listable features.

Note: I think we should name this flag something other than listable. Maybe something like IsOnDemand since this feature. I'll await your feedback before I go though and rename the flag.

[MikeAlhayek]

@jtkech this PR is ready for review. I also updated the title/description with the used terminology to eliminate confusion. The core change here is in ShellFeaturesManager where we enforce EnabledByDependencyOnly

[jtkech]

@MikeAlhayek

First just for info I already didn't fully like how DefaultTenantOnly and IsAlwaysEnabled was added. They are not fully managed at a low level but only used at an higher level e.g. by OC.Features which is only an UI to manage features. So for me they are not full IFeatureInfo states but more like metadata.

DefaultTenantOnly is only used to filter what returns GetAvailableFeaturesAsync() which is used at an higher level e.g. by OC.Features, but nothing prevent to enable such a feature in any tenant by code.

IsAlwaysEnabled is confusing as it may be disabled if it has not been enabled a first time, then it can't be disabled but here also nothing prevents to do it by code. Here I'm not talking about the AlwaysEnabled property of the ShellFeature(s) that we can register through the DI e.g. with AddGlobalFeatures, this AlwaysEnabled property is fully managed e.g. in ShellDescriptorFeaturesManager.

So, looks like we already use manifest properties that are not full feature info states but more like metadata used by higher level services, but without saying yet that is good to add another one ;)

---

That said I took a look at the code, before going further I think that you don't need all the recursive stuff, _extensionManager.GetFeatureDependencies() already returns all cached transitive dependencies retrieved from the dependency graph. And the code could be even simpler by using _extensionManager.GetDependentFeatures() which returns all cached transitive dependents.

Updated: As an additional note, just thought about the recipe FeatureStep that may be executed in the context of a setup, knowing that some features are only enabled during the setup (e.g. those only added through AddSetupFeatures()). So using the dependencies graph during a setup may be wrong.

[MikeAlhayek]

@jtkech
I agree with you on the IsAlwaysEnabled, I always found it confusing since it’s not really alway available until it’s first enabled somehow.

I’ll update the ShellFeaturesManager to remove the recursive stuff. Thanks for the tip there.

Are you saying that IShellFeatureManager isn’t the lowest level to control features? If not, what would be the lowest level to control features? I assumed that it’s the lowest level and is the right place for adding my logic. My logic should be added to the lowest level so it can’t be bypassed. Also, there should be a service that gets called after the shell becomes running that enabled all the AlwaysEnabled features.

Also, if the IFeatureValidator isn’t executed at the lowest level, that means the FeaturesProfile is not also secure as it should be bullet proof

[jtkech]

@MikeAlhayek Sorry didn't have so much time

Did you see this part of my comment? Meaning that half of the logic is already implemented.

Hmm maybe you don't need to manage the features to be auto enabled, the needed dependencies are already managed in ShellDescriptorFeaturesManager.GetFeaturesToEnable()

We already manage missing dependencies, moreover we already manage remaining dependents with ShellDescriptorFeaturesManager.GetFeaturesToDisable().

So not yet sure at which level the EnabledByDependencyOnly (or another name) features settings should be set, and at which level the logic should be applied.

But about the logic I think we only need to always add all EnabledByDependencyOnly features to the featuresToDisable, this way they will be disabled but only if there are no remaining dependents.

Note: If at least at the ShellFeaturesManager, the logic would be also applied for recipes, but here we can consider that it is only an admin UI facility.

[MikeAlhayek]

@jtkech i have not had much time to look at the code. I should have say after tomorrow.

However, it’s my understanding that your concerned about adding the extra property since it introduce lots of changes. I was thinking to wait on more feedback on that front before worrying about the property implementation. Are you okay with adding EnabledByDependencyOnly as I already did?

[jtkech]

@MikeAlhayek So read my previous comment, then I did a quick test that seems to work.

So as said features are already auto enabled, the new thing would be to auto disabled them (and not list them in the admin UI), not sure about the name but for now let's call it ByDependencyOnly.

So for testing I populated a _byDependencyOnly string array with feature id(s) whose values could come from a config section e.g. OrchardCore_Features:ByDependencyOnly[].

Then in OC.Features.AdminController.EnableOrDisableFeaturesAsync(), if the action is .Disable or .Toggle we just need to retireve the ByDependencyOnly that are currently enabled (optional).

var byDependencyOnly = (await _shellFeaturesManager.GetEnabledFeaturesAsync())
        .Where(f => _byDependencyOnly.Contains(f.Id))

Then just after in 2 places just add them to the featuresToDisable.

    await _shellFeaturesManager.UpdateFeaturesAsync(
        featuresToDisable.Concat(byDependencyOnly),
        featuresToEnable,
        force == true);

So the logic can be done in 2 or 3 lines of code ;)

[MikeAlhayek]

@jtkech I updated the code by only adding all features that are EnabledByDependencyOnly to the allFeaturesToDisable list. I tested out the code and all seems to be working as expected.

Updated
One thing that is bothering me, we do not validate the features before installing them in ShellDescriptorFeaturesManager. I think we should replace the line.

var allFeaturesToInstall = allFeaturesToEnable
                .Where(f => !installedFeatureIds.Contains(f.Id))
                .ToList();

with

var allFeaturesToInstall = new List<IFeatureInfo>();

foreach (var feature in allFeaturesToEnable)
{
	if(installedFeatureIds.Contains(f.Id))
	{
		continue;
	}
	
    var isFeatureValid = true;
    foreach (var validator in _featureValidators)
    {
        isFeatureValid = await validator.IsFeatureValidAsync(feature.Id);
        // When a feature is marked as invalid it cannot be reintroduced.
        if (!isFeatureValid)
        {
            break;
        }
    }

    if (isFeatureValid)
    {
        allFeaturesToInstall.Add(feature);
    }
}

If you agree, maybe I can make this part of a separate PR unless you want it part of this PR too. Thoughts?

[jtkech]

.ToHashSet() can be removed, IFeatureInfo is not IEquatable, would be only compared by ref, okay if there are no cloned instances but anyway _extensionManager already returns a cached and distinct collection. In fact we only use .ToHashSet() to ensure distincts feature ids strings when we process the collection later on.

Here I would only extract the enabledFeatures to be used later on, this to only try to disable the EnabledByDependencyOnly features retrieved from the enabled features only (see below).

        var enabledFeatures = _extensionManager.GetFeatures()
            .Where(f => shellDescriptor.Features.Any(sf => sf.Id == f.Id));

        var enabledFeatureIds = enabledFeatures.Select(f => f.Id).ToHashSet();

[jtkech]

And then here only try to disable the EnabledByDependencyOnly features that are currently enabled. Also the logic here is to do the Union() before GetFeaturesToDisable() (that checks dependents) is applied, even if missing dependencies are also managed in ShellDescriptorManager.

        var allFeaturesToDisable = featuresToDisable
            // Try to disable 'EnabledByDependencyOnly' features if they have no more dependents.
            .Union(enabledFeatures.Where(f => f.EnabledByDependencyOnly))
            .Where(f => !alwaysEnabledIds.Contains(f.Id))
            .SelectMany(feature => GetFeaturesToDisable(feature, enabledFeatureIds, force))
            .Distinct()
            .Reverse()
            .ToList();

[jtkech]

One suggestion is to do it below by using button tag with the bootstrap disabled attribute, so that it keeps the same rendering flow.

@if (feature.ByDependencyOnly || feature.IsAlwaysEnabled)
{
    @if (!feature.IsEnabled)
    {
        <button class="btn btn-primary btn-sm" disabled>@T["Enable"]</button>
    }
    else
    {
        <button class="btn btn-danger btn-sm" disabled>@T["Disable"]</button>
    }
}
else if (showEnable && !feature.IsEnabled)
{
    <a id="[email protected](feature.Descriptor.Id)" ........ >@T["Enable"]</a>
}
else if (showDisable && feature.IsEnabled)
{
    ...
}

[MikeAlhayek]

@jtkech i'll get these changes made and retest. Probably won't happen before tomorrow.

But what about my comment about adding featureValidators check before installing features.

[jtkech]

About _featureValidators, yes it is only used at the ShellFeaturesManager level in GetAvailableFeaturesAsync() that is called by OC.Features in the AdminController and FeatureStep.

Looks like the code you are suggesting doesn't prevent to enable a feature if not valid, installed features is just an history of all features that has been enabled even if currently disabled.

But not sure we want to use _featureValidators at the ShellDescriptorFeaturesManager level, the only one oob validator we have is related to DefaultTenantOnly which is then managed at the ShellFeaturesManager level, and people are intended to use GetAvailableFeaturesAsync().

So yes better to use a separate PR if you still want to suggest something around this.

Idem, not sure if ByDepencyOnly should be managed at the ShellDescriptorFeaturesManager level, I will do a sample PR where I will use e.g. an OrchardCore_Features config section allowing to specify features that we want to be auto disabled by dependency, just to show what I mean.

[MikeAlhayek]

@jtkech there are couple of services that around feature management. I feel we should only be using one service to manage features for a given ShellDescriptor. So if we have a service that update the shell by enabling/disabling features, that service will validate the given feature ids and then process the update. I will try to submit another PR once we are done with this one just as a concept and we'll see how you feel about it.

The reason I think we should be validating using the validator is for DefaultTenant and also the TeantFeatures. I think that should be validated in the services. These validator should be part of the shellFeaturesManagers as it should be responsible of validating features before enabling them just like we do with the AlwaysEnabled features.

And for the record, I still think "IsOnDemand" or "EnabledOnDemand" is a better property name than "EnabledByDependenyOnly". Because what we are trying to do here is provide a feature that is available on demand which is which it's needed.

[jtkech]

• Yes maybe some services could be merged, don't remember all the history but globally splitting allows to separate concerns and e.g. to keep some services while using different implementations of others.

• The validators are used in .GetAvailableFeaturesAsync() that is called by the OC.Features controller and also called by the recipe FeatureStep, so more OC.Features management centric. I agree with you that DefaultTenantOnly could be also checked at a lower level, but not TenantFeatures.

• just like we do with the AlwaysEnabled features.

  But not with IsAlwaysEnabled which is only checked at the OC.Features level, AlwaysEnabled is a ShellFeature property that really ensures that the feature is always enabled, IsAlwaysEnabled is an IFeatureInfo property as DefaultTenantOnly.

• Yes some properties may not be checked at the right level and/or not defined at the right place, we already discussed about this, was useful to better understand the context, but if you want to suggest some refactoring around them maybe better to use at least one separate PR.

---

• Focusing back on our new property whose first name was Listable, I have no closed opinion but ATM I think it is more focused on Admin UI management, so I'm thinking about an OC.Features configuration section. For example could be useful to use it for OC.Indexing that would be auto enabled/disabled if we have at least one dependent search engine (as Lucene) or not, but without forcing this behavior as it would be if we define it in the manifest.

  For example with another property name for the fun ;)

  "OrchardCore_Features": {
    "EnabledAsNeeded": [
      "OrchardCore.Indexing"
    ]
  }
  

• At least we now agree on the logic which is now implemented in much less lines of code ;) But in case we use the above config section, the logic would be only in the OC.Features AdminController, unless we define a FeaturesOptions in a core project, and so on ;)

[MikeAlhayek]

@jtkech thank you for the detailed response. When I suggested the name Listable, after few comments I determined it was a bad name since it cause confusing. The concept is really make a feature truly available on demand. Meaning the user should not be able to manually enable/disable it using OC.Features. But we still need to maintain its state by enabling it and disabling it when needed.

I personally don't think the config settings will be very useful. But could be missing the point. I think having the feature auto managed by the shell manager is much more powerful since the user does not have to worry about its state what so ever. It'll be enable when needed and disabled when it's not used. This will help also freeing up resources when they are no longer needed "unlike always available." These two have different use case and each provide different behavior.

Updated
As for your example, we could define a feature called "OrchardCore.Indexing". This feature would be on demand. It'll provide some service, controllers and routes. This feature would depend on interface only like ISearchProvider for example. Now, we would create a "OrchardCore.Lucene" module that would depend on "OrchardCore.Indexing" and this feature would implement and register the ISearchProvider interface. Now the Lucene feature would only provide implementation.

Anyway, once we are done with this PR, I'll attempt to refactor the code in a separate PR and see if the refactoring make sense or not.

Can you do me a favor here please? Can you please think about using the name "EnabledOnDemand" instead of "EnabledByDependencyOnly"? I think the term "EnabledOnDemand" is a much better fit and explain the behavior better.

[jtkech]

Okay, hmm one "problem" is that the controller/routes are defined in the OnDemand feature, and then that somehow "depends" on at least one of its dependents being enabled (having a real dependency on this OnDemand feature), this looks like a kind of circular dependency.

Anyway I let you see at the triage meeting (not at a good time for me to meet) if this need makes sense, if so at which level the property should be defined and at which level the implemention should be done.

If we use a new manifest property, maybe a more generic as Meta that could hold multiple values, so that it would be easier to add another one in the future.

Let me know if this can be clarified, so that I can review again the PR for the implementation details.

---

Hmm, maybe a kind of [RequireFeatures()] but in place of requiring all passed features, would require any of them. For info we have this code in CompositionStrategy, so would be easy to do by using the same kind of implementation but with a .Any() in place of a .All().

var requiredFeatures = RequireFeaturesAttribute.GetRequiredFeatureNamesForType(exportedType);
if (requiredFeatures.All(x => featureNames.Contains(x)))
{
    entries.Add(exportedType, feature);
}

Updated: Or the same RequireFeatures attribute but with a new optional parameter

[MikeAlhayek]

@jtkech about the change request. I personally think I like that change in the UI. It becomes confusing. The idea of having a feedback at a meta data in a badge is that it would provide the info away from the action.

Also, the only way the EnabledByDependencyOnly would be disabled is by not applying GetFeaturesToDisable(...) to the feature that are EnabledByDependencyOnly otherwise the code won't work. So the Union(...) would have to come after calling GetFeaturesToDisable(...) on any feature to disable from the request.

As for my example, I am going to create an example in a separate branch to show you what I mean. I'll ping you when I am done with it.

[MikeAlhayek]

@jtkech
I have create a sample project for you to evaluate please clone (https://github.com/MikeAlhayek/OrchardCore/tree/ExampleOfUsingOnDemandFeature) to test it out.
Once you cloned the project, run the site, then enable "OC.Notification.Email" and/or "OC.Notification.Push" features.

For now the push notification will just log to the log file just for the sake to f. If you enable the "OC.Notification.Email" be sure to also configure your email settings to see the email. You could configure the email to go to a local folder for now. Finally, if you actually you want notification, edit your user profile and check the notification

When you run the project and enabled at least one type of notification, you should see a new menu item "Example >> Notify User" you can use to test out the notifications. Please let me know your thought about this sample project

Here is a screenshot of a real example

One more thing to consider

It would be a huge to add an attribute to target tags. For example RequireAnyFeatures(Tag = "push-notification")

With that, we can implement a feature that will look for at least one other feature with the tag "push-notification" before it become available.

For example, we can add the interface in OC.PushNotification but it will not have any implementation of any services. So we automatically load it when at least one other feature is enabled with a specific tag.

// with feature will auto enable when at least one feature is enabled with the tag "push-notification"
[Feature("OC.PushNotification")]
[RequireAnyFeatures(Tag = "push-notification")]
public class PushNotification : StartupBase
{
	
}


// this feature will have "push-notification" tag
[Feature("OC.PushNotification.OneSignal")]
public class OneSignalPushNotification : StartupBase
{
	
}


// this feature will have "push-notification" tag
[Feature("OC.PushNotification.AmazonSns")]
public class AmazonSnsPushNotification : StartupBase
{
	
}

[sebastienros]

During triage suggestions were to add the disable/enable button but in a disabled state, for this flag and for the "always enabled" one, to keep consistency between features, but add a tooltip to explain why the disable/enable button can't be clicked.

Agree with the issue and solution.

[MikeAlhayek]

@sebastienros I changed the button as per your and @jtkech suggestion.

[jtkech]

@MikeAlhayek

So the Union(...) would have to come after calling GetFeaturesToDisable(...) ...

Just retried, it works assuming the force param is still true, you should have tried it ;)

The main point is that you didn't take into account my comment #12281 (comment) saying to only try to disable the EnabledByDependencyOnly features that are currently enabled.

This would have prevented useless feature events handlers calls just after.

    var enabledFeatures = _extensionManager.GetFeatures()
        .Where(f => shellDescriptor.Features.Any(sf => sf.Id == f.Id));

    var enabledFeatureIds = enabledFeatures.Select(f => f.Id).ToHashSet();
    ...

    var allFeaturesToDisable = featuresToDisable
        // Try to disable 'EnabledByDependencyOnly' features if they have no more dependents.
        .Union(enabledFeatures.Where(f => f.EnabledByDependencyOnly))
        .Where(f => !alwaysEnabledIds.Contains(f.Id))
        .SelectMany(feature => GetFeaturesToDisable(feature, enabledFeatureIds, force))
        ...

Finally the prop is still named EnabledByDependencyOnly but the below variable is named onDemandFeaturesToDisable , but not crucial.

        var onDemandFeaturesToDisable = _extensionManager.GetFeatures()
            .Where(f => f.EnabledByDependencyOnly);

Can you do a PR for the above tweaks?

[MikeAlhayek]

@jtkech yes. I tested with the force parameter and it did not work.Humm I just noticed you already merged the PR. Maybe you did the final changes

[jtkech]

No, @sebastienros merged it which is good ;)

The main point is to retrieve the OnDemand but only from the enabled features, it will prevent to always call the disabling handlers for all OnDemand features.

[MikeAlhayek]

@jtkech I'll add these changes with a new PR.