Improves CompositionStrategy and ExtensionManager and adds unit tests for CompositionStrategy

[MikeAlhayek]

This pull request improves the CompositionStrategy and ExtensionManager components in OrchardCore. It also introduces comprehensive unit tests for CompositionStrategy to ensure reliability and maintainability.

[Piedone]

Could you please check @gvkries?

[hishamco]

I was thinking to ask about unit tests, but it's good to have them on this PR

[gvkries]

I believe there’s no bug here: ShellBlueprint.Dependencies should not contain startup types with missing requirements, and this is already enforced by CompositionStrategy. The extra check for RequireFeaturesAttribute in ShellContainerFactory is redundant and shouldn't be added, and the related unit test doesn’t make sense in this context.

That said, the new unit tests for CompositionStrategy are valuable and the minor improvements to that class are also good changes—those should definitely be kept.

Suggested actions:

Remove the duplicate check in ShellContainerFactory and its associated test.
Keep the new tests and refactoring in CompositionStrategy.

Thanks!

[MikeAlhayek]

@gvkries if you're suggesting that the startup unit test is invalid, I suggest we proof that there indeed no issue in our code base. Are you able to repo @Piedone raised issue outside the unit tests? If this indeed is not a problem, then I don't see how Zoltan could have encountered the issue. Something is missing here are we should ensure that the RequiredFeatures is doing what it is intended to do. The ConfigureServices, Configure and ConfigureAsync should not be called if at least one of the listed features is not enabled.

[gvkries]

@gvkries if you're suggesting that the startup unit test is invalid, I suggest we proof that there indeed no issue in our code base.

Yes, your new unit tests for CompositionStrategy are verifying this condition. Those are very good.
But as ShellContainerFactory is only resolving the startup types in the given ShellBlueprint via DI, I don't see the point of testing the container factory for this condition.

Are you able to repo @Piedone raised issue outside the unit tests? If this indeed is not a problem, then I don't see how Zoltan could have encountered the issue. Something is missing here are we should ensure that the RequiredFeatures is doing what it is intended to do. The ConfigureServices, Configure and ConfigureAsync should not be called if at least one of the listed features is not enabled.

I've not been able to reproduce the issue in OC. Every startup with unmet require attribute is correctly skipped and never instantiated while I was testing it.
I would suspect that the tenants feature is somehow enabled on their site.

[Piedone]

Replied under CrestApps/CrestApps.OrchardCore#136 since there's more conversation. See CrestApps/CrestApps.OrchardCore#136 (comment).

[gvkries]

@MikeAlhayek I've finalized your PR, hope you're okay with that.

I've taken the new unit tests and the improvements for the CompositionStratey. I've also modified the IExtensionManager to take IEnumerable<string>s instead of string arrays.

Outdated

[MikeAlhayek]

@gvkries thank you! Sorry I have not had much time to complete it. Thanks for doing it.

[gvkries]

@MikeAlhayek As we are discussing about breaking changes in #18241, what do you think about this one here. I added a new overload in the IExtensionManager to take an IEnumerable instead of an array, but I've done it in a none-breaking way. Since we are already not binary compatible in v3, maybe we should also binary-break here, as it would simplify this PR a lot (no more casting in every call to the new methods).

[MikeAlhayek]

@gvkries I think the change you added here are valid. In this case, you won't encounter and unexpected runtime exception because the same signature "as before" still exists which.