Hibernate reactive routes quickstart improvements #626

mkouba · 2020-07-21T13:35:18Z

quarkusio/quarkus#10871 must be merged first...

- based on the latest reactive routes API improvements

mkouba · 2020-07-21T13:36:52Z

CC @DavideD

cescoffier · 2020-07-21T13:41:35Z

I just merged quarkusio/quarkus#10871.

DavideD · 2020-07-21T14:10:14Z

...rnate-reactive-routes-quickstart/src/main/java/org/acme/hibernate/reactive/FruitsRoutes.java

-    public Multi<Fruit> getAll(RoutingContext rc) throws Exception {
-        return session.createNamedQuery(Fruit.FIND_ALL, Fruit.class).getResults();
+    public Multi<Fruit> getAll() throws Exception {
+        return ReactiveRoutes.asJsonArray(session.createNamedQuery(Fruit.FIND_ALL, Fruit.class).getResults());


Is ReactiveRoutes.asJsonArray( required now?

I think that it was required even before but the test does not validate the proper syntax of json array...

Yes, it required (For now) to creates an asynchronous json array. Each item is written in the response when ready (and sent to the client).

We are improving this by inferring this from the produces attribute. But it's WIP...

It surprised me because when I write

@Route(methods = GET, path = "/") public Uni<List<Fruit>> getAll(RoutingContext rc) throws Exception { return session.createNamedQuery( Fruit.FIND_ALL, Fruit.class).getResultList(); }

It works as expected. I'm wondering if it wouldn't look better to return a Uni<List>?

yes it looks better, but the quickstart is supposed to teach to people "the right way".

@cescoffier are you optimistic about your WIP to be doable? If so, maybe it's ok for the example to already show the simpler syntax? We're not working to win a benchmark war yet ;)

P.S. I see that the alternative API which I referred to - one which produces a "real Multi" - isn't around yet, but I expect we'll be able to make one based on the scrollable API of ORM - wich is backed by server side cursors.

@gavinking ^

let's use what looks best then.

Uni<List<Fruit>> then? ;-)

"Hibernate Reactive does not support this yet" does not sound good

I don't think this is a Hibernate Reactive issue, it's a general issue when designing the API. Not saying that we need to add a comment but something similar to this would tell the user the differences:

In this case it makes sense to return a Uni<List<Fruit>> because we return a reasonable amount of results. Consider returning a Multi<Fruit> for streaming results (link to example).

The example doesn't even have to be a Hibernate Reactive one.

For everybody interested, this is the issue explaining why Hibernate Reactive hasn't a proper Multi return method: hibernate/hibernate-reactive#153

Having some feedback is always nice.

DavideD · 2020-07-21T15:49:48Z

By the way, does the quickstart work when you go to http://localhost:8080/index.html?
I removed this dependency last minute thinking that it was unnecessary

        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-resteasy-jsonb</artifactId>
        </dependency>

I forgot to test the web page and it doens't seem to work without it. Any idea why?

gsmet · 2020-07-21T16:24:19Z

It would be weird that you would need any RESTEasy dependency when going full reactive routes. But I'll let Martin answer.

CI is failing with:

2020-07-21T13:57:20.3143319Z 2020-07-21 13:57:20,311 ERROR [org.acm.hib.rea.FruitsRoutes] (vert.x-eventloop-thread-2) Failed to handle request: java.lang.NullPointerException
2020-07-21T13:57:20.3144029Z 	at org.acme.hibernate.reactive.FruitsRoutes_RouteHandler_update_2420491368c662f8824cb079244c86a88df9203b.invoke(FruitsRoutes_RouteHandler_update_2420491368c662f8824cb079244c86a88df9203b.zig:104)
2020-07-21T13:57:20.3144426Z 	at io.quarkus.vertx.web.runtime.RouteHandler.handle(RouteHandler.java:53)
2020-07-21T13:57:20.3144773Z 	at io.quarkus.vertx.web.runtime.RouteHandler.handle(RouteHandler.java:17)
2020-07-21T13:57:20.3145132Z 	at io.vertx.ext.web.impl.RouteState.handleContext(RouteState.java:1034)
2020-07-21T13:57:20.3145481Z 	at io.vertx.ext.web.impl.RoutingContextImplBase.iterateNext(RoutingContextImplBase.java:95)
2020-07-21T13:57:20.3145837Z 	at io.vertx.ext.web.impl.RoutingContextImpl.next(RoutingContextImpl.java:133)
2020-07-21T13:57:20.3146183Z 	at io.vertx.ext.web.handler.impl.BodyHandlerImpl.handle(BodyHandlerImpl.java:95)
2020-07-21T13:57:20.3146525Z 	at io.vertx.ext.web.handler.impl.BodyHandlerImpl.handle(BodyHandlerImpl.java:42)
2020-07-21T13:57:20.3146913Z 	at io.quarkus.vertx.http.runtime.VertxHttpRecorder$16.handle(VertxHttpRecorder.java:1053)
2020-07-21T13:57:20.3147279Z 	at io.quarkus.vertx.http.runtime.VertxHttpRecorder$16.handle(VertxHttpRecorder.java:1026)
2020-07-21T13:57:20.3147626Z 	at io.vertx.ext.web.impl.RouteState.handleContext(RouteState.java:1034)
2020-07-21T13:57:20.3148033Z 	at io.vertx.ext.web.impl.RoutingContextImplBase.iterateNext(RoutingContextImplBase.java:131)
2020-07-21T13:57:20.3148369Z 	at io.vertx.ext.web.impl.RoutingContextImpl.next(RoutingContextImpl.java:133)
2020-07-21T13:57:20.3148716Z 	at io.vertx.ext.web.handler.impl.BodyHandlerImpl$BHandler.doEnd(BodyHandlerImpl.java:296)
2020-07-21T13:57:20.3149076Z 	at io.vertx.ext.web.handler.impl.BodyHandlerImpl$BHandler.end(BodyHandlerImpl.java:276)
2020-07-21T13:57:20.3149426Z 	at io.vertx.ext.web.handler.impl.BodyHandlerImpl.lambda$handle$0(BodyHandlerImpl.java:87)
2020-07-21T13:57:20.3149776Z 	at io.vertx.core.http.impl.HttpServerRequestImpl.onEnd(HttpServerRequestImpl.java:521)
2020-07-21T13:57:20.3150166Z 	at io.vertx.core.http.impl.HttpServerRequestImpl.lambda$pendingQueue$1(HttpServerRequestImpl.java:11

so there's something wrong going on.

cescoffier · 2020-07-21T16:26:07Z

By removing the json dependency it removes the reasteasy extension and the index page is not served anymore.

gsmet · 2020-07-21T16:33:14Z

You need RESTEasy to server static resources even when using Reactive routes?

cescoffier · 2020-07-21T16:37:47Z

I believe so, maybe not. I can't remember if we moved to the Vert.x Static file handler or if it's still the Resteasy feature that is used.

mkouba · 2020-07-22T06:16:29Z

@gsmet That's very likely a failure related to quarkusio/quarkus#10871. It should be fixed now.

@DavideD @gsmet @cescoffier For resteasy standalone we register a wrapped io.vertx.ext.web.handler.StaticHandler here: https://github.com/quarkusio/quarkus/blob/master/extensions/resteasy/runtime/src/main/java/io/quarkus/resteasy/runtime/standalone/ResteasyStandaloneRecorder.java#L134-L151 (by the way the original vertx issue was fixed in 3.8.4 so it should not be necessary to wrap it anymore). We should probably move the static handler logic to vertx-http?

cescoffier · 2020-07-22T06:27:57Z

@mkouba Yes, we should move this to vertx-http. @stuartwdouglas What's your opinion on this?

DavideD · 2020-07-22T06:47:51Z

Out of curiosity, what's the difference between vertx-web and vertx-http?

mkouba · 2020-07-22T06:49:45Z

Out of curiosity, what's the difference between vertx-web and vertx-http?

vertx-web = reactive routes, user-facing API. vertx-http - core components around the web server.

DavideD · 2020-07-22T06:56:58Z

I see, thanks

stuartwdouglas · 2020-07-22T07:00:55Z

Sounds reasonable.

mkouba · 2020-07-24T11:44:00Z

The CI failure is probably unrelated:

2020-07-24T11:15:15.8660015Z [ERROR]   AggregatorTest.test ? Timeout test() timed out after 10 seconds
...
2020-07-24T11:15:15.8788004Z [INFO] kafka-streams-quickstart-aggregator ................ FAILURE [ 44.550 s]

mkouba · 2020-07-27T13:34:26Z

@DavideD The CI is green now! Feel free to merge this PR ;-).

gsmet · 2020-07-27T16:09:33Z

I don't think he can :).

Let me know if it's ready to go in, I will merge it.

mkouba · 2020-07-28T07:53:39Z

I don't think he can :).

Ah, you're right. So I think it's ready but maybe one more +1 would be good ;-).

DavideD · 2020-07-28T08:22:16Z

...rnate-reactive-routes-quickstart/src/main/java/org/acme/hibernate/reactive/FruitsRoutes.java

-                        .continueWith(httpResponse(rc, 404));
+    public Uni<Fruit> update(@Body Fruit fruit, @Param String id) {
+        if (fruit == null || fruit.getName() == null) {
+            return Uni.createFrom().failure(new IllegalArgumentException("Fruit name was not set on request."));


A curiosity, does it have to be a Uni in this case? Can't we just throw the exception?

We can. But I think that it's better to avoid throwing an exception unless we have to...

DavideD · 2020-07-28T08:26:32Z

It's OK for me to merge it (somebody else needs to do ti though) but without the additional dependency going to http://localhost:8080/index.html is not going to work (unless something has changed already). We should also add a test that checks that the page is up and running so that we won't forget about it in the future.

mkouba · 2020-07-28T08:27:55Z

but without the additional dependency going to http://localhost:8080/index.html is not going to work (unless something has changed already).

This is already fixed in master...

DavideD · 2020-07-28T08:31:33Z

This is already fixed in master...

Awesome!

gsmet · 2020-07-28T11:45:18Z

Merging then, thanks everyone!

Hibernate reactive routes quickstart improvements

3421fbe

- based on the latest reactive routes API improvements

mkouba requested review from cescoffier and Sanne July 21, 2020 13:35

cescoffier approved these changes Jul 21, 2020

View reviewed changes

DavideD reviewed Jul 21, 2020

View reviewed changes

Replace Multi<Fruit> with Uni<List<Fruit>>

7365c7b

mkouba force-pushed the hibernate-reactive-routes-improvements branch from 14bb2c8 to 7365c7b Compare July 24, 2020 10:47

DavideD reviewed Jul 28, 2020

View reviewed changes

gsmet merged commit b2281df into quarkusio:development Jul 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hibernate reactive routes quickstart improvements #626

Hibernate reactive routes quickstart improvements #626

mkouba commented Jul 21, 2020

mkouba commented Jul 21, 2020

cescoffier commented Jul 21, 2020

DavideD Jul 21, 2020

mkouba Jul 21, 2020

cescoffier Jul 21, 2020

DavideD Jul 21, 2020

Sanne Jul 21, 2020

Sanne Jul 22, 2020

Sanne Jul 22, 2020

mkouba Jul 22, 2020

DavideD Jul 22, 2020 •

edited

Loading

DavideD Jul 22, 2020

DavideD commented Jul 21, 2020

gsmet commented Jul 21, 2020

cescoffier commented Jul 21, 2020

gsmet commented Jul 21, 2020

cescoffier commented Jul 21, 2020

mkouba commented Jul 22, 2020

cescoffier commented Jul 22, 2020

DavideD commented Jul 22, 2020

mkouba commented Jul 22, 2020

DavideD commented Jul 22, 2020

stuartwdouglas commented Jul 22, 2020

mkouba commented Jul 24, 2020

mkouba commented Jul 27, 2020

gsmet commented Jul 27, 2020

mkouba commented Jul 28, 2020

DavideD Jul 28, 2020

mkouba Jul 28, 2020

DavideD commented Jul 28, 2020

mkouba commented Jul 28, 2020

DavideD commented Jul 28, 2020

gsmet commented Jul 28, 2020

Hibernate reactive routes quickstart improvements #626

Hibernate reactive routes quickstart improvements #626

Conversation

mkouba commented Jul 21, 2020

mkouba commented Jul 21, 2020

cescoffier commented Jul 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavideD Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavideD commented Jul 21, 2020

gsmet commented Jul 21, 2020

cescoffier commented Jul 21, 2020

gsmet commented Jul 21, 2020

cescoffier commented Jul 21, 2020

mkouba commented Jul 22, 2020

cescoffier commented Jul 22, 2020

DavideD commented Jul 22, 2020

mkouba commented Jul 22, 2020

DavideD commented Jul 22, 2020

stuartwdouglas commented Jul 22, 2020

mkouba commented Jul 24, 2020

mkouba commented Jul 27, 2020

gsmet commented Jul 27, 2020

mkouba commented Jul 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavideD commented Jul 28, 2020

mkouba commented Jul 28, 2020

DavideD commented Jul 28, 2020

gsmet commented Jul 28, 2020

DavideD Jul 22, 2020 •

edited

Loading