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

Allow more control for middleware pipeline ordering #15716

Open
gvkries opened this issue Apr 10, 2024 · 13 comments
Open

Allow more control for middleware pipeline ordering #15716

gvkries opened this issue Apr 10, 2024 · 13 comments
Milestone

Comments

@gvkries
Copy link
Contributor

gvkries commented Apr 10, 2024

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

Piedone commented Apr 10, 2024

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: #15173 (comment)

@gvkries
Copy link
Contributor Author

gvkries commented Apr 10, 2024

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

Piedone commented Apr 10, 2024

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

gvkries commented Apr 11, 2024

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

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

gvkries commented Apr 13, 2024

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

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

gvkries commented Apr 14, 2024

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

ns8482e commented Jul 14, 2024

@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

  • 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

@Piedone
Copy link
Member

Piedone commented Jul 14, 2024

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

gvkries commented Jul 15, 2024

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

ns8482e commented Jul 16, 2024

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

gvkries commented Jul 17, 2024

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.

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

No branches or pull requests

4 participants