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

DI dependencies of features in the same assembly are mapped to the wrong feature #15782

Closed
gvkries opened this issue Apr 17, 2024 · 8 comments · Fixed by #15793
Closed

DI dependencies of features in the same assembly are mapped to the wrong feature #15782

gvkries opened this issue Apr 17, 2024 · 8 comments · Fixed by #15793
Labels
Milestone

Comments

@gvkries
Copy link
Contributor

gvkries commented Apr 17, 2024

Describe the bug

In case a single assembly contains several features, the dependencies added to the ITypeFeatureProvider may not be correct. All dependencies are added to one arbitrary feature, no matter which feature actually registers it in DI. I guess anything is added to the first feature being enabled.

In turn this causes things like the RoleUpdater to not correctly apply default permissions, because enabling a second feature in a single module is not correctly detected.

This can only be prevented, when all necessary dependencies are marked with the FeatureAttribute.

@Piedone
Copy link
Member

Piedone commented Apr 17, 2024

This came up here.

@gvkries
Copy link
Contributor Author

gvkries commented Apr 18, 2024

I've reviewed the ExtensionManager and it appears to function correctly and by design. It's essential to apply the FeatureAttribute to map specific types to features accurately.

Additionally, I've taken a brief look at ITypeFeatureProvider and found that the main issue arises with RoleUpdater when detecting whether to apply default permissions. This process relies on the correct feature-to-type mapping, which requires all IPermissionProvider implementations to be marked with the FeatureAttribute if more than one provider is present in a module.

Given this, I'm hesitant to recommend changes to the ExtensionManager, as it could lead us into complex territory. Instead, it may suffice to correct the IPermissionProviders by ensuring the FeatureAttribute is properly applied.

@MikeAlhayek
Copy link
Member

Yes the solution is to use FeatureAttribute. The issue is that the ShellContainerFactory has no way of knowing which feature should the type belong to without explicitly using FeatureAttribute attribute.

var attribute = type.GetCustomAttributes<FeatureAttribute>(false).FirstOrDefault();
if (attribute is not null)
{
feature = featureServiceCollection.Key.Extension.Features
.FirstOrDefault(f => f.Id == attribute.FeatureName)
?? feature;
}
}

@MikeAlhayek
Copy link
Member

Oh and by the way, any implementation of IShapeTableProvider and data migration IDataMigration that belongs to a specific feature in a module SHOULD also be decorated with the feature attribute.

@Piedone
Copy link
Member

Piedone commented Apr 19, 2024

That looks like an inconvenient API, since otherwise we only need [Feature]s for controllers, due to them being auto-registered, otherwise it's all up to the Startups. Can't we do better and rather figure out the registration source of the types, so we don't have to add a lot of attributes to the code for no obviously apparent reason?

@MikeAlhayek
Copy link
Member

Yeah it would be nice if we don't have to use Feature attribute all over the place. I agree. Can you think of a better way to associate each time with a feature?

Maybe one way to eliminate the use of ITypeFeatureProvider. Or scan all the registered type when calling all the startups and assign each type to the feature if the startup.

@gvkries
Copy link
Contributor Author

gvkries commented Apr 19, 2024

The ShellContainerFactory walks over all types and we can use that fact to update the mapping in the ITypeFeatureProvider, e.g. in the TypeFeatureProvider (https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore/OrchardCore/Extensions/Features/TypeFeatureProvider.cs) directly:

public void TryAdd(Type type, IFeatureInfo feature)
{
---     _features.TryAdd(type, feature);

+++  _ = _features.AddOrUpdate(type, (t, f) => f, (curType, curFeature, newFeature) =>
+++  {
+++    // If the type is currently only mapped to the module, update it with the more specific feature.
+++    if (curFeature != newFeature &&
+++        curFeature.Extension.Manifest.ModuleInfo.Id == curFeature.Id &&
+++        curFeature.Extension.Features.Contains(newFeature))
+++    {
+++      Debug.WriteLine($"TypeFeatureProvider changed mapping of type '{curType}' from '{curFeature.Id}' to '{newFeature.Id}'.");
+++      return newFeature;
+++    }
+++
+++    return curFeature;
+++  }, feature);
}

That would bring us almost there, all types are then assigned to the correct feature in the ITypeFeatureProvider.

But there is also the FeatureEntry.ExportedTypes property, which is populated by the ExtensionManager before the shell container factory is building the service collections. At this point we have no way of actually telling the correct feature without the attribute and the types assigned to the FeatureEntry would still not be correct and maybe worse, they would then be differently mapped as by the type feature provider.

As the FeatureEntry.ExportedTypes are only used to get the required features in the ComposisitionStrategy and again by the RoleUpdater, maybe the best solution is to get rid of FeatureEntry and reuse the ITypeFeatureProvider for this functionality as well.

@MikeAlhayek
Copy link
Member

possible solution in this comment #15793 (comment)

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

Successfully merging a pull request may close this issue.

3 participants