helidon-io / helidon

Java libraries for writing microservices
https://helidon.io
Apache License 2.0
3.44k stars 562 forks source link

4.x: register routing in weighted order of Server and HTTP Features #8826

Closed tomas-langer closed 1 month ago

tomas-langer commented 1 month ago

Resolves: #8816

Description

Correctly handle order of Server Features and HTTP Features when registering non-HTTP Feature elements to the routing.

With the updated version, all elements (filters, routes, services) are ordered depending on the feature's weight. This is achieved by creating a new HttpFeature for each Server Feature with the same weight, that collects all registrations and applies them once the HTTP Features are ordered by weight. HttpFeature registered from a Server Feature is left intact and applied on the real builder (as this already works as it should)

Documentation

This aligns with the internal documentation of feature weights.

I have added update of WebServer documentation in the latest commit.

tjquinno commented 1 month ago

The coding changes look OK to me, although I do not know this area of the system.

The user whose questions raised our awareness of this issue has specifically asked for public documentation explaining, first, the ordering of filters vs. handlers (maybe we think it should be intuitive but we can at least state what that ordering is) and, second, how weighting affects that ordering.

Ideally such a doc update should be part of this PR...maybe a brief section on the webserver page in the request handling section?

RickyFrost commented 1 month ago

Tim, I was also asking about the ordering of filters vs filters from features. It seems like an undue burden to create a ServerFeature, and an HttpFeature, just so I can correctly weight my filter, don't you think? Seems like that would be buried too deep in uptake, and be subject to unnecessary brittleness for each new release... Sounds like a good argument for a "standard" feature where you can plug in your filter (with a weight, and a name maybe?).

tomas-langer commented 1 month ago

Tim, I was also asking about the ordering of filters vs filters from features. It seems like an undue burden to create a ServerFeature, and an HttpFeature, just so I can correctly weight my filter, don't you think? Seems like that would be buried too deep in uptake, and be subject to unnecessary brittleness for each new release... Sounds like a good argument for a "standard" feature where you can plug in your filter (with a weight, and a name maybe?).

After this PR, you can either create an HttpFeature or a ServerFeature - depending on your use case. The filters registered directly in routing are always ordered by insertion order.

So the ordering is:

- filters, routes, `HttpService` from features (server or HTTP) that have higher than default weight
- filters, routes, `HttpService` from routing ordered by insertion
- filters, routes, `HttpService` from features that have lower than default weight

Routing is "just" another HTTP feature that has default weight (except for the capability to register HttpFeatures), that is why you cannot register a filter outside of its scope. We could add a method addFilter(Filter filter, double weight) and wrap it in an HttpFeature ourself, if you think this would improve your use case a lot. Or would you prefer to analyze the filter instance, and if it is weighted, use its weight?

tomas-langer commented 1 month ago

After a discussion with @RickyFrost, I still think we should require a feature to have filter/routing weighted. Otherwise we may end up with confusing behavior

tomas-langer commented 1 month ago

Updated documentation of WebServer