Corrects the mapping of types to features in ITypeFeatureProvider

[gvkries]

Just an idea how to fix #15782. Removing FeatureEntry may be overkill, but it is not really required.

[gvkries]

/cc @MikeAlhayek @Piedone

[Piedone]

This may substitute #15787.

[MikeAlhayek]

@gvkries what actually fixes the issue here?

[gvkries]

@gvkries what actually fixes the issue here?

It's in the TypeFeatureProvider.TryAdd method. I outlined the solution in the issue as well.

[MikeAlhayek]

@gvkries I think there is two things that should be considered in the solutions.

1. A Type can belong to one or more feature.
2. I think the solution should be part of the ShellContainerFactory.

The only way to really know if a type was added by a startup file, is by calling that start up and then get all the types that were added in the process. I am not sure if this is the best process

I think something like this may actually solve the problem of needed [Feature] attribute all over the place. With this approach we can remove the calling typeFeatureProvider.TryAdd from the extension manager.

foreach (var startup in startups)
{
    var feature = blueprint.Dependencies.FirstOrDefault(x => x.Key == startup.GetType()).Value?.FeatureInfo;

    startup.ConfigureServices(featureServiceCollection);

    foreach (var type in featureServiceCollection.Select(x => x.GetImplementationType()))
    {
        if (type is null)
        {
            continue;
        }

        typeFeatureProvider.TryAdd(type, feature ?? _applicationFeature);
    }
}

With this approach, only a startup class will need a [Feature] attribute. Any class that is registered in that startup will automatically be tied to it. But then we also have to change the implementation of ITypeFeatureProvider so it allows mapping many features to a type.

I am not sure I have time to test out the above approach, but if you are able it, it may worth trying.

[gvkries]

@MikeAlhayek I take a look if I find some more time. But do you think we should work on this at all, or is it too much hassle?

[MikeAlhayek]

@gvkries I think we should. But we have to replace existing approach with a better approach. The current solution is to use Feature[] attribute. And we agree that it's not the best approach. So we need to come up with a better solution. If the solution I suggested works, then I think this will be win win and the developer will not need to use the Feature attribute outside the startup and the controller.

[gvkries]

@MikeAlhayek Your solution is almost the same as mine, I only kept the type registration in the ExtensionManager as-is and removed the FeatureEntry. FeatureEntry purely exists to keep a list of public types per feature (which is then also not correctly mapped to features without the FeatureAttribute). It has only two uses, one in the CompositionStrategy and one in the RoleUpdater. The role updater must be changed anyway to use the ITypeFeatureProvider instead, which leaves us with a single use case for the FeatureEntry during startup only. Additionally it keeps the type lists alive for the app lifetime, which is rarely or never used again (only during tenant restarts I think).
Maybe the best solution is to rework the CompositionStrategy as well. This way we may can get rid of the type mappings being added by the extension manager at all.

[MikeAlhayek]

which is then also not correctly mapped to features without the FeatureAttribute

This is the problem I think we should solve. And with the approach I provides, I am don't think we would need to add additional types to the ITypeFeatureProvider at all. So that is something you do once during a startup and it is accessible to all (Extension Manager, and maybe CompositionStrategy). I have not spent much time looking at CompositionStrategy or the extension manager. But ideally we manage this collection in once place "ShellContainerFactory" if possible.

[gvkries]

I tried this quickly, but ShellContainerFactory comes into the game too late, the ITypeFeatureProvider is already used before. So removing it being filled by the ExtensionManager brings some challenges.

I need to go now, I'll come back to this later.

[MikeAlhayek]

Thank for looking into it.

maybe we need to investigate why do we need to populate the _typeFeatureProvider using the extension manager? Quick look at the code tells me what should not have to do it that early. This is something that we need to do during the startup process. But I could be wrong since I have not really had time to peal this onion.

[gvkries]

I tried this quickly, but ShellContainerFactory comes into the game too late, the ITypeFeatureProvider is already used before. So removing it being filled by the ExtensionManager brings some challenges.

My reasoning was wrong, I only missed some types in the feature provider. Especially all non-DIed-types are of cause not added now, which causes the problem.

[MikeAlhayek]

What we build a fake Service Collection in the Extension Manager so we can get the correct mapping and therefore we won't need it in the ShellContainerFactory.

[gvkries]

As @MikeAlhayek said, I completely moved populating the ITypeFeatureProvider into the ShellContainerFactory. The shell container factory can then associate all types to their features according to their startups and also adds all types that are not held by DI.

Additionally this simplifies the extension manager and moves all usages that needs the feature types to rely on the type feature provider as well. No more duplicated responsibilities between extension manager and type feature provider.

Type sharing between multiple features is not considered yet.

[MikeAlhayek]

Make sure you do a round of testing to ensure that the types are tied to the correct feature. Before complicating things by associating one type to possibly multiple features

[gvkries]

Yes, I think we should skip that addition for now.

[MikeAlhayek]

Yes, I think we should skip that addition for now.

I am not sure you can. The extension manager uses every feature available (not the features that are associated to the current tenant). This means that if you have a type T that could be registered to feature A and feature B which both features are independent of each other, then the extension manager may assign type T to feature A and never to feature B. Now if you only enable feature B and not A, then the ITypeFeatureProvider will contain one less type which is what you are trying to solve here.

[gvkries]

Yes, I think we should skip that addition for now.

I am not sure you can. The extension manager uses every feature available (not the features that are associated to the current tenant). This means that if you have a type T that could be registered to feature A and feature B which both features are independent of each other, then the extension manager may assign type T to feature A and never to feature B. Now if you only enable feature B and not A, then the ITypeFeatureProvider will contain one less type which is what you are trying to solve here.

Yeah, that is correct. But the current solution is already an improvement.

[MikeAlhayek]

Not really. Using the Feature attribute currently gives you the correct mapping no matter what. If we make this change, then Feature attribute will not be honored as we would honer registration. In order for this to be an acceptable solution, I think it has to provide a better replacement to the existing solution.

[gvkries]

No, the current solution does not remove or change the feature attribute at all. Only types not marked with it are now correctly assigned to its feature if it is in the DI container. I was not really thinking of fully replacing the FeatureAttribute, only improving the assignment to avoid errors that are hard to find. But it's still work-in-progress, let me see what I can do with that problem...

[gvkries]

I've changed the ITypeFeatureProvider to allow types that are used by multiple features. I think this is mostly correct, but there are some challenges still to be done. E.g. all callees of ITypeFeatureProvider.GetFeaturesForDependency() will have to consider the fact that a returned feature may not be enabled at all.

[Piedone]

Sorry that I didnt join earlier. I'm still trying to understand this, but great discussion and work! It was quite a ride to go through all of this :).

Would you be able to join the Thursday triage meeting (https://docs.orchardcore.net/en/latest/resources/meeting/) and present this? We need a group consultation.

I think this approach is good, but I need to put more time into it.

[Piedone]

SecureMediaPermissions need to be adjusted too if this will be merged.

[gvkries]

Not necessarily, applying the feature attribute should still be possible and the behavior should not be changed by this PR.

[Piedone]

While it will work, it'll have an unnecessary attribute.

[Piedone]

Perhaps having a GetExtensionForDependency() would make the intent clearer for the consumers that only need that but now do First().

[gvkries]

Yes, that is a valid idea. But maybe we should review the responsibilities of ITypeFeatureProvider and IExtensionManager completely, there is some overlapping.

[github-actions]

This pull request has merge conflicts. Please resolve those before requesting a review.

[gvkries]

Would you be able to join the Thursday triage meeting (https://docs.orchardcore.net/en/latest/resources/meeting/) and present this? We need a group consultation.

I think I am able to join next week.

[Piedone]

That would be great, thank you!

[Piedone]

I couldn't take part today, but did you in the end?

[gvkries]

No, sorry. I was talking about this week, so next Thursday 👍

[Piedone]

OK, I'll try to take part too.