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]

@gvkries can you please confirm that we solved the issue where one type can belong to multiple features?

Also, at this point you should also remove the [Feature] attribute from classes like Migration, shape table provider, permissions (anything except for Startup files and controllers).

Yes, all types can be assigned to one or more features. The ITypeFeatureProvider returns an enumerable of features now. But we have several occasions where only one feature is expected, that part is a little bit tricky. The provider will return the first feature which has the same ID as the extension in that case. This mimics the previous behavior, because the extension manager has always assigned the type to the feature with the extension ID there too.

I will remove the feature attribute and check any problem arises from that.

[MikeAlhayek]

shouldn't we just return null instead of throwing exception here?

[gvkries]

It was throwing before and actually all public types of a module must be assigned to at least one feature anyway. It would be an error if someone uses this with an internal type and a type that is not registered in DI.
Therefore I think we should keep it throwing.

[MikeAlhayek]

same here. probably null is better

[MikeAlhayek]

materialize this using .ToArray() to prevent evaluating this every time

[gvkries]

Callers are enumerating only once, one caller even with an Any() condition. ToArray() would be worse performance wise.

[gvkries]

I have now removed all FeatureAttributes from several classes that should not require it anymore. Even if I couldn't find a problem, I'm a little bit nervous with about change and I hope this doesn't break anything.

Especially, the removal of the attribute from some classes that were assigned to the application default feature, i.e. they had a [Feature(Application.DefaultFeatureId)] assigned, causes those to be logically moved to a different feature. This is true e.g. for some core provides (e.g. CoreShapes etc.).

I think this should not break anything, but maybe it is more safe to keep the attribute on those classes that have no specific feature assigned via a startup.

[Piedone]

Yeah, those that had a Feature attribute with a feature different than what they'll get now should rather remain. I have no idea what may depend on them but it's probably safe to assume that something can.

[gvkries]

You are right, I added the attribute back to the classes in question. Shouldn't have removed them in the first place. 😇

[Piedone]

Are you ready for another round of reviews?

[gvkries]

Are you ready for another round of reviews?

Yes, I try to attend to the meeting tomorrow, if there is one.

[MikeAlhayek]

@gvkries during the call, you mentioned there are minor things that you want to do to this PR. Once you are done, I'll do one last final review.

[gvkries]

@gvkries during the call, you mentioned there are minor things that you want to do to this PR. Once you are done, I'll do one last final review.

No sorry, I'm done with this for now. I was just trying to explain that I'm not happy that some types are now assigned to two features by default. But I want to keep it this way, because I'm not sure if I break something if I change that. Basically all types are assigned to a feature that has the same ID as the parent extension. This will be the first feature entry and an additional more specific feature is added afterwards, if there is one. This way calling GetFeatureForDependency will return the same value as before.

So please go ahead and check it out 🙈

[MikeAlhayek]

I added a documentation in the release notes for awareness