Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Corrects the mapping of types to features in ITypeFeatureProvider #15793

Merged
merged 31 commits into from
May 24, 2024

Conversation

gvkries
Copy link
Contributor

@gvkries gvkries commented Apr 19, 2024

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

@gvkries
Copy link
Contributor Author

gvkries commented Apr 19, 2024

/cc @MikeAlhayek @Piedone

@Piedone
Copy link
Member

Piedone commented Apr 19, 2024

This may substitute #15787.

@MikeAlhayek
Copy link
Member

@gvkries what actually fixes the issue here?

@gvkries
Copy link
Contributor Author

gvkries commented Apr 19, 2024

@gvkries what actually fixes the issue here?

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

@MikeAlhayek
Copy link
Member

@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
Copy link
Contributor Author

gvkries commented Apr 19, 2024

@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
Copy link
Member

@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
Copy link
Contributor Author

gvkries commented Apr 19, 2024

@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
Copy link
Member

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
Copy link
Contributor Author

gvkries commented Apr 19, 2024

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
Copy link
Member

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
Copy link
Contributor Author

gvkries commented Apr 20, 2024

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
Copy link
Member

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
Copy link
Contributor Author

gvkries commented Apr 22, 2024

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
Copy link
Member

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
Copy link
Contributor Author

gvkries commented Apr 22, 2024

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

@MikeAlhayek
Copy link
Member

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
Copy link
Contributor Author

gvkries commented Apr 22, 2024

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
Copy link
Member

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
Copy link
Contributor Author

gvkries commented Apr 22, 2024

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
Copy link
Contributor Author

gvkries commented May 17, 2024

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
Copy link
Member

Piedone commented May 17, 2024

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
Copy link
Contributor Author

gvkries commented May 17, 2024

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

@Piedone
Copy link
Member

Piedone commented May 21, 2024

Are you ready for another round of reviews?

@gvkries
Copy link
Contributor Author

gvkries commented May 22, 2024

Are you ready for another round of reviews?

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

sebastienros and others added 2 commits May 23, 2024 10:22
…TypeFeatureProvider.cs

Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>
@MikeAlhayek
Copy link
Member

@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
Copy link
Contributor Author

gvkries commented May 24, 2024

@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 🙈

Copy link
Member

@MikeAlhayek MikeAlhayek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a documentation in the release notes for awareness

@MikeAlhayek MikeAlhayek enabled auto-merge (squash) May 24, 2024 16:23
@MikeAlhayek MikeAlhayek merged commit 95bbd6d into OrchardCMS:main May 24, 2024
11 checks passed
@gvkries
Copy link
Contributor Author

gvkries commented May 25, 2024

I added a documentation in the release notes for awareness

@MikeAlhayek Many thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DI dependencies of features in the same assembly are mapped to the wrong feature
4 participants