vertx-web icon indicating copy to clipboard operation
vertx-web copied to clipboard

Adding a CorsHandler together with BodyHandler since Vert.x 4.3.1

Open ppatierno opened this issue 2 years ago • 6 comments

In our Strimzi HTTP - Kafka bridge we are using the OpenAPI3RouterBuilder in order to create our routes from an OpenAPI definition. The bridge also supports to enable CORS and we are doing it at this line here:

https://github.com/strimzi/strimzi-kafka-bridge/blob/main/src/main/java/io/strimzi/kafka/bridge/http/HttpBridge.java#L169

What I observed is that going this way, the BodyHandler is not added so any request having a needed body ends with an error about Null body. AFAIU, this is happening since this change https://github.com/vert-x3/vertx-web/commit/2b5ea8e58e0159c983a4db8d38e2efaa72cd4375 in the Vert.x web component

if (globalHandlers.isEmpty()) {
      // TODO: this is very opinionated
      globalRoute.handler(BodyHandler.create());
    }

The BodyHandler is not added because the global handlers list is not empty due to the CorsHandler we are adding. I cannot find any good way to overcome this problem. We would need having CorsHandler as first, then BodyHandler, and then all the handlers related to the operations. Maybe using the rootHandler to set the CorsHandler is not the right strategy? But what would be the solution? When using the previous Vert.x versions (i.e. 4.1.4) where the OpenAPI3RouterFactory was in place instead of the OpenAPI3RouterBuilder, the above code from Strimzi seemed to work fine.

ppatierno avatar Dec 12 '22 14:12 ppatierno

@pmlopes tagging you as the author of that change ;-)

ppatierno avatar Dec 12 '22 14:12 ppatierno

The only way I found, also by documentation, is by not setting the CorsHandler as root handler but after creating the route doing this:

router.route().handler(getCorsHandler());

anyway it's adding this handler at the end and the CorsHandlerImpl.handle() method is never executed ... which should happen right after the BodyHandler.

The only workaround I found was using the deprecated bodyHandler method this way:

if (this.bridgeConfig.getHttpConfig().isCorsEnabled()) {
                    routerBuilder.rootHandler(getCorsHandler());
                    routerBuilder.bodyHandler(BodyHandler.create());
                }

or the rootHandler twice (even because bodyHandler is actually an "alias" for rootHandler):

if (this.bridgeConfig.getHttpConfig().isCorsEnabled()) {
                    routerBuilder.rootHandler(getCorsHandler());
                    routerBuilder.rootHandler(BodyHandler.create());
                }

What am I missing to reach the goal in a proper way if this is not the proper way? Shouldn't it be better to not call rootHandler but addGlobalHandler? Didn't exist something like this in the past if IIRC?

ppatierno avatar Dec 12 '22 15:12 ppatierno

Your findings are correct, I've commented on the linked PR. Yes the naming is "terrible", I wanted to preserve the API but the behavior is not really consistent with the expectation. I think the name you're proposing makes much more sense!

pmlopes avatar Dec 12 '22 18:12 pmlopes

Are we leaving this one to remember about the naming or you can even close if it makes sense to you ;-)

ppatierno avatar Dec 12 '22 18:12 ppatierno

In the new openapi router, we have fixed this by only having 1 way to do this.

pmlopes avatar Mar 23 '23 16:03 pmlopes

@pmlopes can you provide more details about what's the fix, what's the proper way to have CORS and Body handler working together and finally in which Vert.x version this fix will be available please?

ppatierno avatar Mar 24 '23 08:03 ppatierno

Hi, there is a complete new rebuild of the OpenAPI Router [1], which hopefully solves the problem. Please check out if this works for you.

I am closing this issue now, as the ticket refers to a version of OpenAPI Router that is no longer supported. If the bug still occurs with the new OpenAPI router version, I would be very happy if you would open a new issue.

The proper way to have CORS and Body handler working together is adding them in the order you prefer and then mount the generated Router. Or using the rootHandler [2] method. In case you add a BodyHandler by your own, you need a special RequestExtractor [3].

[1] https://vertx.io/docs/vertx-web-openapi-router/java/ [2] https://vertx.io/docs/apidocs/io/vertx/ext/web/openapi/router/RouterBuilder.html#rootHandler-io.vertx.core.Handler- [3] https://vertx.io/docs/vertx-web-openapi-router/java/#_routerbuilder

pk-work avatar May 28 '24 07:05 pk-work