search

vert-x3 / vertx-web

HTTP web applications for Vert.x
Apache License 2.0
1.11k stars 535 forks source link

BodyHandler should not be added before the ProxyHandler #2616

Closed tsegismont closed 2 months ago

tsegismont commented 6 months ago

See #2614

Added documentation and make ProxyHandler fail fast if the BodyHandler has been seen.

tsegismont commented 5 months ago

Thanks @pmlopes for the feedback! I have a couple of comments:

1/ I think ProxyHandler should extend ProtocolUpgradeHandler rather than PlatformHandler because the user might want to add a security handler before. 2/ Perhaps we should keep both mechanisms because a handler having a priority lower than BODY is not protected in the case of the following setup:

    router.post().handler(BodyHandler.create());
    router.route("/product").handler(ProxyHandler.create(productServiceProxy));

Or am I missing something?

pmlopes commented 5 months ago

@tsegismont yeap, that's correct, platform wouldn't allow anything before (other than another platform) protocol upgrade is a better suited interface. The unspoken contract is that if the handler performs any async call and doesn't terminate the request, it should pause/resume the request to ensure that once a body handler is on the route the body isn't lost like here:

https://github.com/vert-x3/vertx-web/blob/bcf9dea0540121405fff08f6f0c8b881ede634d1/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/AuthenticationHandlerImpl.java#L58-L61

and

https://github.com/vert-x3/vertx-web/blob/bcf9dea0540121405fff08f6f0c8b881ede634d1/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/AuthenticationHandlerImpl.java#L96-L99

I guess we should have these “bespoken” rules in the interface javadoc too 😅

pmlopes commented 5 months ago

You can also add another interface to cleary mark proxy related stuff if protocol upgrade seems odd. There is no cost as the check only happens once in the lifecycle of the router, in the weight() function.

tsegismont commented 5 months ago

You can also add another interface to cleary mark proxy related stuff if protocol upgrade seems odd. There is no cost as the check only happens once in the lifecycle of the router, in the weight() function.

Protocol upgrade is fine with me, the proxy can do this anyway (websocket proxy).

tsegismont commented 5 months ago

@pmlopes PTAL