OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.36k stars 2.37k forks source link

Allow more control for middleware pipeline ordering #15716

Open gvkries opened 5 months ago

gvkries commented 5 months ago

Is your feature request related to a problem? Please describe.

When a feature has a dependency on two other features and must explicitly add a middleware in between these two dependencies, ordering cannot be achieved with ConfigureOrder alone without possibly changing the order related to other modules.

The ordering of startups currently only allows to have one or more dependencies ensuring startup occurs after those dependencies, or a global Order and ConfigureOrder property, which changes the order in relation to all other modules.

Describe the solution you'd like

It would be beneficial to explicitly set the order only in relation to other dependencies.

Piedone commented 5 months ago

Can you please explain why Order doesn't work? I think it should only be a problem if both dependencies have the same Order (like having the default 0). But If Dependency A has Order 10, and Dependency B 0, then if you set it to 5 in your module's Startup, it should register in between the two.

Related: https://github.com/OrchardCMS/OrchardCore/pull/15173#discussion_r1536273253

gvkries commented 5 months ago

Most of Orchards own modules have a default ordering of 0 and you cannot change that order from a module. Even when working with the Orchard source directly, you have to change the order of all concerning modules. Which then effects all other modules as well. Using feature dependencies also does not solve this, if your requirement is to have the module be loaded before your dependency. Additionally when changing the Order, any feature dependency will be ignored as well.

To summarize: For internal modules I guess Order can be enough, if everything is calculated very carefully. But at least for external modules (e.g. when using NuGet packages), you cannot achieve every ordering that might be required.

Piedone commented 5 months ago

Hmm. We'd then have three ways to configure this (module dependency, Order, middleware ordering) with different drawbacks.

I looked into why ASP.NET Core doesn't have anything for middleware dependencies/such ordering, but I couldn't find anything. I guess there's a reason, though.

gvkries commented 5 months ago

I looked into why ASP.NET Core doesn't have anything for middleware dependencies/such ordering, but I couldn't find anything. I guess there's a reason, though.

Yeah, in normal ASP.NET it's always explicitly ordered in the startup of the app. Only the modularity of Orchard brings this complexity with it 🤷‍♂️ And I don't have a simple solution yet.

hishamco commented 5 months ago

@gvkries As you said before most of the built-in OC modules have order 0, could you please give me an example when you set the order of your module is not enough?

gvkries commented 5 months ago

@gvkries As you said before most of the built-in OC modules have order 0, could you please give me an example when you set the order of your module is not enough?

See #15173 (comment) for a real world example, but the problem is not limited to this.

Any module with a specific order requirement in relation to two ore more other modules will have this problem. You cannot describe ordering without also changing the order of the core modules.

hishamco commented 5 months ago

You can set the order of newly added modules to run before or after any built-in module, let me check the PR that you refer to

gvkries commented 5 months ago

You can set the order of newly added modules to run before or after any built-in module, let me check the PR that you refer to

Yes, but you cannot insert in between two or more modules.

ns8482e commented 2 months ago

@gvkries IMO there is no ordering issue with orchard and orchard provides you full control maintain order of middleware in your module.

The issue in referenced PR comment is due to incorrectly defining the dependency.
Here you have defined Media.Security as dependent of Media resulted into Middleware defined in "Media.Security" run after Media.

IMO you really don't need to define feature Media.Security instead you should just define MediaSecurityStartup with RequeresFeature("User.Security", "Media") and define ConfigureOrder that will place the Middleware defined in Security after Autorization, and after Users but before Media

Piedone commented 2 months ago

IMO you really don't need to define feature Media.Security instead you should just define MediaSecurityStartup with RequeresFeature("User.Security", "Media")

That's not really suitable here. Unlike something like modules extending Audit Trail or Deployment with their module-specific features, Secure Media is not something that should be enabled without it being explicitly wanted by the user, since it adds overhead to every /media request.

gvkries commented 2 months ago

IMO you really don't need to define feature Media.Security instead you should just define MediaSecurityStartup with RequeresFeature("User.Security", "Media") and define ConfigureOrder that will place the Middleware defined in Security after Autorization, and after Users but before Media

  • Define Dependency for feature only if you want to run Startup after dependency.
  • However if you ever need to run Startup before specific module but run only when such module is enabled - don't define dependency rather define RequeresFeature startup

This would not solve the problem. If you change ConfigureOrder e.g. to a negative value, the middleware will be moved before both dependencies and not between them, as long as the modules you depend on have the default order 0. It is not possible to explicitly move a middleware between two modules.

ns8482e commented 2 months ago

IMO you really don't need to define feature Media.Security instead you should just define MediaSecurityStartup with RequeresFeature("User.Security", "Media")

That's not really suitable here. Unlike something like modules extending Audit Trail or Deployment with their module-specific features, Secure Media is not something that should be enabled without it being explicitly wanted by the user, since it adds overhead to every /media request.

Secure media could be an optional admin setting on media, available when user security and media feature are enabled, can be achieved using requiresfeature without defining media secure feature

gvkries commented 2 months ago

Secure media could be an optional admin setting on media, available when user security and media feature are enabled, can be achieved using requiresfeature without defining media secure feature

It could, but that's an arbitrary design decision and I opted for an additional feature. This problem here has nothing to do with it, it was just uncovered by working on the secure media.