Add streaming support for files in RESTEasy Reactive

[StFS]

Description

It would be nice to have a simple way to return file content as a stream in a reactive way in a REST resource method.

The original discussion is here.

Implementation ideas

This discussion took place on Zulip so I'm only conveying the ideas discussed there (by other people).

@geoand suggested to Stephane Epardaud making AsyncFile a known return type and properly handling it under the covers.

@Ladicek chimed in with:

Ladislav Thon: don't we have a way to do an equivalent of Vert.x's ReadStream.pipeTo(WriteStream)?
Ladislav Thon: I know we don't support StreamOutput in RESTEasy Reactive, but having a non-blocking variant of that would be really nice IMHO

Also, something to keep in mind is that it would be nice if a solution to this supported both whole files as well as some range from a file. I don't know enough about the Vert.X FS API to know if this would be handled transparently by calling AsyncFile.setReadPos(long) and AsyncFile.setReadLength(long) before returning it or whether some special handling would be required for this.

[quarkus-bot]

/cc @FroMage, @geoand, @stuartwdouglas

[FroMage]

Yup, supporting AsyncFile OOTB would make a lot of sense. Can you add your sample code here? Would supporting Path OOTB also make sense?

[geoand]

Would supporting Path OOTB also make sense?

It does to me

[indalaterre]

Does this feature request have a release plan? Currently this blocks the upgrade to resteasy-reactive

[geoand]

Hopefully we'll get something in for 1.14 but there is no guarantee.

[geoand]

@StFS can you add the code you ended using so we can have a reference of what we want to achive / simplify?

[indalaterre]

I was reading this post where the 1.14 could be the last 1.x version before the 2.x.
https://groups.google.com/g/quarkus-dev/c/oTBc0iHLxrw/m/_ykCmyi_BgAJ?pli=1

Is there any chance to have this enhancement in the 1.14 before the migration?

[geoand]

As you have read in that announcement, 1.14 will likely only happen if there are significant features to be added to it.

At this point, I can't promise anything regarding the timeline of this issue.

@indalaterre are you using StreamingOutput from JAX-RS and looking to move off it?

[indalaterre]

@geoand yes we use StreamingOutput to implement download functionalities for generated streams. We want to migrate to resteasy-reactive but we need to enhancement

[geoand]

@indalaterre can you paste an example snippet of how you are using it?

[indalaterre]

Snippet.txt

Here's the snippet

[geoand]

Thanks

[FroMage]

Snippet inline:

package com.cgm.nais.catalog;

import io.smallrye.mutiny.Uni;
import org.eclipse.microprofile.openapi.annotations.media.Content;
import org.eclipse.microprofile.openapi.annotations.media.Schema;
import org.eclipse.microprofile.openapi.annotations.parameters.Parameter;

import javax.ws.rs.PathParam;
import javax.ws.rs.core.Response;
import javax.ws.rs.core.StreamingOutput;

import static javax.ws.rs.core.MediaType.TEXT_PLAIN;

public class Snippet {

    public Uni<Response> getDocumentationByFilename(
            @Parameter(
                    required = true,
                    content = @Content(mediaType = TEXT_PLAIN, schema = @Schema(implementation = String.class)))
            @PathParam("filename") String fileName) {
        return getService()
                .getDocumentationByFilename(fileName)
                .map(item -> {
                    StreamingOutput stream = output -> output.write(item.array());
                    return Response.ok(stream)
                            .header("Content-Type", "application/pdf")
                            .build();
                });

}

[FroMage]

I forget who's our openapi expert, is it @phillip-kruger ?

@indalaterre can you tell us the type of item in your code?

[geoand]

I forget who's our openapi expert, is it @phillip-kruger ?

Exactly

[FroMage]

So, @phillip-kruger do you confirm that as far as (SR) open-api is concerned, the previous endpoint signature should be equivalent to:

    public Uni<Response> getDocumentationByFilename(String filename) {

[indalaterre]

@FroMage item it's a java.nio.ByteBuffer

[FroMage]

Hah, we should support those out of the box, if we don't already. https://quarkus.io/guides/resteasy-reactive#resource-types says we don't yet, but there's no good reason.

[phillip-kruger]

@FroMage Not sure what you mean ? Do you mean we should be able to omit the @Parameter ? The schema should be OK (set to String) but I am not sure about the required (Might need a BV NotNull or something) and the content media type I doubt will be set by default. Or am I missing the point ?

[FroMage]

I mean it looks like all that annotation specifies should be optional and can be inferred by the rest of the signature, no?

For the content-type, you're right we can't infer it. Though it's wrong anway since it claims to be text but then it's set to PDF ;)

[indalaterre]

Wait the text content-type is for the request while the pdf is for the response. This code works without resteasy-reactive so I think it's not a problem of the api

[phillip-kruger]

Yes, except for content type yes. Required I am not sure what the default is, so that might also needs to be set somehow, either with @Schema or something else like Bean validation. To set the response content type you can use @APIResponse

[FroMage]

OK lemme take that.

[geoand]

💪🏼

[indalaterre]

Hello, is there any chance to have this in 2.0?

[geoand]

@FroMage this was added in #16286, right?

[indalaterre]

@geoand I saw that there is a 13.3 milestone? Can also this one be released there?

[geoand]

I am not sure we want to make such a change in 1.13.3. But I'll let @FroMage decide.

[FroMage]

Yes, we now support sending files async. I guess we could backport this, I don't see why not, if there's a need.

[geoand]

@FroMage are you sure that #16286 will work with Vert.x 3?

Something tells me that even the PR applies cleanly to the 1.13 branch, there might be some subtle issues when it's run.

[FroMage]

Well, it's tested. If the tests pass, then it works and is tested exactly as much as it is currently on 2.0, so I'd say about the same level of confidence in both cases.

If it doesn't apply cleanly or the tests break, then we can decide if it's worth it or not.

[geoand]

OK, than can you please open a PR against the 1.13 branch? Best to know now if it applies cleanly and if the tests pass than wait until it's time to release

[FroMage]

OK done, thanks: #16653

[FroMage]

And it did not apply cleanly ;)

[geoand]

Thanks! Let's see how it goes

[drmalex07]

If we transform a Uni<AsyncFile> to a Uni<Response> (because, for example, we need to add custom content-disposition), are we sure that the underlying AsyncFile will be eventually closed (when the returned Uni terminates) ?

[geoand]

I wouldn't use that. Try RestResponse instead of Response

[drmalex07]

@geoand ok, i will have that in mind.
But, just to clarify this, i thought that RestResponse is just a strongly-typed variant of Response (because of this point in Quarkus Resteasy Reactive docs).
Does it make a difference in terms of how the underlying body is opened/closed ?

[geoand]

The difference is that because of the strong typing, Quarkus knows how to handle it under all circumstances.

With Response there are corner cases ...

[Manfred73]

When I use RestResponse<StreamingOutput> I get a compilation error in Intellij:

Required type: RestResponse <javax.ws.rs.core.StreamingOutput>
Provided: RestResponse <javax.ws.rs.core.StreamingOutput>

Both are the same classes (maybe different libs on the classpath?). Both seem to point to io:quarkus.resteasy:resteasy-reactive-common:2.12.3.Final though.

import java.io.IOException;
import java.io.OutputStream;
import javax.ws.rs.GET;
import javax.ws.rs.Path;
import javax.ws.rs.Produces;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.StreamingOutput;
import org.jboss.resteasy.reactive.RestResponse;

@Path("/v1/files")
public class QueryResource {

	@GET
	@Path("/test/filename/{filename}")
	@Produces(MediaType.APPLICATION_OCTET_STREAM)
	public RestResponse<StreamingOutput> streamFile() {
		final var streamingOutput = new StreamingOutput() {
			@Override
			public void write(OutputStream output) throws IOException, WebApplicationException {
				output.write("test string".getBytes());
			}
		};
		final var restResponse = RestResponse.ok(streamingOutput, MediaType.APPLICATION_OCTET_STREAM);
		return restResponse; // compilation error here
	}
}

UPDATE:
Immediately returning the response solves this:

return RestResponse.ok(streamingOutput, MediaType.APPLICATION_OCTET_STREAM);

[geoand]

That is very odd... I'll have a look next week

[geoand]

The problem you mentione above has nothing to do with Quarkus, it's a limitation of the var usage. If you use final StreamingOutput streamingOutput then everything works as expected

[Manfred73]

I'm using RestAssured to test my resources. Is it expected behaviour when returning a StreamingOutput, that the body is initially empty? How would you test this with RestAssured?

Real code:

@GET
@Path("/filename/{filename}")
@Produces(MediaType.APPLICATION_OCTET_STREAM)
public Uni<RestResponse<StreamingOutput>> readFileByFilename(@PathParam("filename") String filename) {
   validationService.validate(FileParams.builder().filename(filename).build());
   return resultCreator.create(queryService.getFileByFilename(filename));
}

The test:

@Test
void readByFilename() {
   // GIVEN
   final var testParams = mockSomeThings();

   // WHEN / THEN
   // How to test this StreamingOutput with restassured? Or should an IntegrationTest suffice here?
   given()
      .pathParam("filename", testParams.filename)
      .when()
      .get("/filename/{filename}")
      .then()
      .statusCode(200)
      .headers(
         "Content-Disposition", "attachment;filename=" + testParams.filename,
         "Content-Type", "application/octet-stream")
      .body(emptyString()); // is emptyString expected with StreamingOutput?
}