Skip to content
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

Introduce a way to set headers and status code for streaming response #33197

Merged
merged 2 commits into from
May 10, 2023

Conversation

geoand
Copy link
Contributor

@geoand geoand commented May 8, 2023

Relates to: #33130
Closes: #26523

@geoand
Copy link
Contributor Author

geoand commented May 8, 2023

@cescoffier WDYT?

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just made a small comment.

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 8, 2023
@github-actions
Copy link

github-actions bot commented May 8, 2023

🙈 The PR is closed and the preview is expired.

@quarkus-bot

This comment has been minimized.

@geoand geoand removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 8, 2023
@geoand
Copy link
Contributor Author

geoand commented May 9, 2023

@cescoffier I pushed a second commit which makes this feature even more usable, you might want to have a look as I needed to introduce a custom Multi (the implementation was copied pretty much verbatim from existing Mutiny code)

@quarkus-bot
Copy link

quarkus-bot bot commented May 9, 2023

Failing Jobs - Building 0e313b6

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 19

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 Windows #

- Failing: integration-tests/mongodb-panache 

📦 integration-tests/mongodb-panache

io.quarkus.it.mongodb.panache.reactive.ReactiveMongodbPanacheResourceTest.testMoreRepositoryFunctionalities line 351 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <500>.

@scrocquesel
Copy link
Contributor

Works for my use case as well. Thanks.

I'm just wondering if a builder pattern could improve API discoverability. It may not be a good idea, but generally, RestResponse is a well-known starting point to create a response builder for specific use cases. Having fromUni and fromMulti would help discover those supported use cases while keeping the builder pattern. The return type of the builder will also help the user determine what to use as the return type for its resource method.

For example:

return RestResponse.fromUni(Uni.createFrom().item(new MyObject()))
     .onItem()
        .header(Map.of("Content-Disposition", "attachment;filename=" + outerParam)) // header (String, String), for header known before the Uni returned
        .header("Content-Type", object -> object.contentType()) // header (String, Function<T, String>), when only the value vary
        .header(object -> Map.of("Content-Length", List.of(object.length()))) // header (Function<T, Map<String, List<String>>>), for total control
        .transformToMulti(object -> Multi.createFrom().publisher(object.getPublisher()))
        .build();

Also, it feels bad that because we're doing things reactively, we lose the strongly typed helper method of ResponseBuilder to set headers. That way, new methods may be added later to the builder to fill the gap.

@geoand
Copy link
Contributor Author

geoand commented May 10, 2023

Thanks for the suggestion.

The problem is that the RestResponse models a very specific thing - a single HTTP response.

When you are streaming data back however, that model doesn't work - you need something that represents the fact that you can only specify the headers once and then have a payload of multiple chunks.

@geoand
Copy link
Contributor Author

geoand commented May 10, 2023

I should also mention that RestResponse works perfectly with Uni (you don't loose it's usefulness in the Reactive world).

The problem as I mentioned above is the mismatch with the data streaming paradigm

@geoand geoand merged commit 02371fa into quarkusio:main May 10, 2023
@geoand geoand deleted the #33130 branch May 10, 2023 06:28
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label May 10, 2023
@quarkus-bot quarkus-bot bot added this to the 3.1 - main milestone May 10, 2023
@k-wilmeth
Copy link

Hello! I have not yet upgraded to Quarkus 3 in my application yet. Is there a RestMulti equivalent for Quarkus 2 that could be used to stream bytes? Thank you.

@geoand
Copy link
Contributor Author

geoand commented Jun 19, 2023

Hi,

No, there is not really

@k-wilmeth
Copy link

Huh... okay. Maybe it's time I look to upgrade then.
I was trying to get it to work with Multi<Buffer> but it doesn't seem to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RESTEasy Reactive - support binary content streaming with header control
4 participants