-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add JSON streaming for RESTEasy Reactive Jsonb and Jackson #20908
Conversation
Glad to see this, thanks! @FroMage is the expert on streaming, so let's get a review from him :) |
@@ -197,24 +242,40 @@ public void handle(ResteasyReactiveRequestContext requestContext) throws Excepti | |||
// media type negotiation and fixed entity writer set up, perhaps it's better than | |||
// cancelling the normal route? | |||
// or make this SSE produce build-time | |||
MediaType[] mediaTypes = requestContext.getTarget().getProduces().getSortedMediaTypes(); | |||
if (mediaTypes.length != 1) | |||
MediaType[] mediaTypes = requestContext.getTarget().getProduces().getSortedOriginalMediaTypes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to use getSortedOriginalMediaTypes
because when using application/stream+json
, we were only getting application/json
using the method getSortedMediaTypes
.
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 1e32c23
Failures⚙️ Initial JDK 11 Build #- Failing: independent-projects/resteasy-reactive/server/runtime
! Skipped: devtools/bom-descriptor-json docs extensions/agroal/deployment and 210 more 📦 independent-projects/resteasy-reactive/server/runtime✖ |
PR needs formatting |
1e32c23
to
482e38e
Compare
It should be fixed now |
482e38e
to
eb1994f
Compare
PR updated to resolve some new conflicts with main. |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building eb1994f
Failures⚙️ Native Tests - Windows - hibernate-validator #- Failing: integration-tests/hibernate-validator
📦 integration-tests/hibernate-validator✖ |
This now needs a rebase |
eb1994f
to
2ba9c70
Compare
Done |
@FroMage can you take a look at this one? |
@geoand what should we do with this one? |
Ping @FroMage again 😊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, except a few comments, and requires a mention in the documentation, linking to the stream specs.
...ctive/common/runtime/src/main/java/org/jboss/resteasy/reactive/common/util/SseMediaType.java
Outdated
Show resolved
Hide resolved
...ment/src/test/java/io/quarkus/resteasy/reactive/jackson/deployment/test/sse/SseResource.java
Outdated
Show resolved
Hide resolved
Really sorry about the late review, this completely slipped past me. |
2ba9c70
to
43a5a92
Compare
@FroMage PR updated with one of your suggestions |
43a5a92
to
0c102bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just the question of the supertype of RestMediaType
left.
...tive/common/runtime/src/main/java/org/jboss/resteasy/reactive/common/util/RestMediaType.java
Outdated
Show resolved
Hide resolved
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 0c102bf
Full information is available in the Build summary check run. Failures⚙️ Gradle Tests - JDK 11 Windows #- Failing: integration-tests/gradle
📦 integration-tests/gradle✖
|
0c102bf
to
f154712
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building f154712
Failures⚙️ Initial JDK 11 Build #- Failing: extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment
! Skipped: docs extensions/apicurio-registry-avro/deployment extensions/avro/deployment and 77 more 📦 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment✖ |
f154712
to
9f1823d
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 9f1823d
Full information is available in the Build summary check run. Failures⚙️ Gradle Tests - JDK 11 Windows #- Failing: integration-tests/gradle
📦 integration-tests/gradle✖
✖
|
We now support json streaming. Example: ``` @path("stream-json/multi") @get @produces(SseMediaType.APPLICATION_STREAM_JSON) @RestSseElementType(MediaType.APPLICATION_JSON) public Multi<Message> multiStreamJson() { return Multi.createFrom().items(new Message("hello"), new Message("stef")); } ``` We support "application/x-ndjson" and "application/stream+json". Moreover, I've added a new utility class called `SseMediaType` to be used by users. The implementation is similar to `MediaType`.
9f1823d
to
d684ebe
Compare
Failing Jobs - Building d684ebe
Full information is available in the Build summary check run. Failures⚙️ Gradle Tests - JDK 11 Windows #- Failing: integration-tests/gradle
📦 integration-tests/gradle✖
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
CI failure unrelated: merging |
We're wrongly using `/n` instead of `\n`. This was done in quarkusio#20908
We now support json streaming. Example:
We support "application/x-ndjson" and "application/stream+json".
Moreover, I've added a new utility class called
SseMediaType
to be used by users. The implementation is similar toMediaType
.Questions:
Fix #13663