Interrupt download on DownloadResponse.error

[ElZorrito]

Describe the bug

Hello,
I try vaadin 24.8.3
When I send a DownloadResponse.error in DownloadHandler like this :
new Anchor(event -> DownloadResponse.error(HttpStatusCode.BAD_REQUEST), "Test")
A file "téléchargement.txt" is downloaded on chrome.. a random filename .txt on firefox

Expected-behavior

Interrupt the download

Reproduction

new Anchor(event -> DownloadResponse.error(HttpStatusCode.BAD_REQUEST), "Test")

System Info

Vaadin 24.8.3
Chrome 138.0.7204.96
FIrefox 140

[mcollovati]

DownloadResponse.error(...) only creates an instance of the object but it does now write on the HTTP response.

If you implement a custom DownloadHandler you have to handle the response by your own. See https://vaadin.com/docs/latest/flow/advanced/downloads#using-downloadevent-and-lambda-expression for an example

[Legioth]

...and if you want to the DownloadResponse to be used, then you should wrap the callback in DownloadHandler.fromInputStream.

But I'm quite curious about how Chrome comes up with that kind of name for the downloaded file if that's indeed all the code that is relevant to the case.

[ElZorrito]

Thanks !
Ok I try it :

		new Anchor(event -> {
			final InputStream inputStream = myService.getInputStream();
			event.setFileName(fileName);
			event.getResponse().setHeader("Cache-Control", "public, max-age=3600");
			inputStream.transferTo(event.getOutputStream());
		}, "Download me!");

But when there is a RuntimeException in "myService.getInputStream()" who to interrupt everything ?
With this code, Vaddin forward to something like : VAADIN/dynamic/resource/2/9936d566-2ec5-4f38-b054-7db6c7bb2ad4/

[mcollovati]

Look at this example
https://vaadin.com/docs/latest/flow/advanced/downloads#download-content-from-inputstream

[ElZorrito]

I already tried, DownloadResponse.error(500) does not work

[mcollovati]

Did you use the DownloadHandler.fromInputStream(...) helper as in the example?

        new Anchor(DownloadHandler.fromInputStream(ev -> {
                    try {
                        ev.getResponse().setHeader("Cache-Control", "public, max-age=3600");
                        return new DownloadResponse(
                                myService.getInputStream(), fileName,
                                null, -1);
                    } catch (Exception e) {
                        return DownloadResponse.error(500);
                    }
                }
        ), "Download me!");

[mcollovati]

BTW @Legioth do you think it would be worth it adding cache control method(s) to DownloadResponse?

[Legioth]

I already tried, DownloadResponse.error(500) does not work

DownloadResponse doesn't work with the API that you're using. It only works with DownloadHandler.forInputStream.

If you don't want to use forInputStream, then you can follow this pattern instead:

new Anchor(event -> {
  try {
    final InputStream inputStream = myService.getInputStream();
    event.setFileName(fileName);
    event.getResponse().setHeader("Cache-Control", "public, max-age=3600");
    inputStream.transferTo(event.getOutputStream());
  } catch (Exception e) {
    event.getResponse().sendError(500, e.getMessage());
  }
}, "Download me!");

[Legioth]

adding cache control method(s) to DownloadResponse?

Not sure if it's worhtwhile. It's bad enough to have four constructor parameters - anything more complicated should really be a named method instead. But then we would have to introduce some kind of chaining so that you could do return new DownloadResponse(a, b, c, d).withCacheControl(e);. And at that point, it's a question on whether it's worthwhile compared to event.getResponse().setHeader.

We could maybe start by adding a shorthand method to DownloadEvent?

[mcollovati]

We could maybe start by adding a shorthand method to DownloadEvent?

Sounds good. We already have setters for content type and content length, so it makes sense to add something for cache control headers as well

[Legioth]

One thing to ponder there though is whether it would make sense to just pass through a string or if it would be more useful to take a more structured input and then also e.g. set a corresponding Pragma header when appropriate?

[mcollovati]

Cache control configurations seem quite extensive. I'll create a new ticket so we can move the discussion there

[ElZorrito]

Thanks to you, but nothing works.

new Anchor(DownloadHandler.fromInputStream(ev -> {
                    try {
                        ev.getResponse().setHeader("Cache-Control", "public, max-age=3600");
                        return new DownloadResponse(
                                myService.getInputStream(), fileName,
                                null, -1);
                    } catch (Exception e) {
                        return DownloadResponse.error(500);
                    }
                }
        ), "Download me!");

Chrome launch a download called "download.txt"

 new Anchor(event -> {
  try {
    final InputStream inputStream = myService.getInputStream();
    event.setFileName(fileName);
    event.getResponse().setHeader("Cache-Control", "public, max-age=3600");
    inputStream.transferTo(event.getOutputStream());
  } catch (Exception e) {
    event.getResponse().sendError(500, e.getMessage());
  }
}, "Download me!");

Chrome forward to something like : VAADIN/dynamic/resource/2/5a55ea08-09e0-4aa0-b89b-807778c6fa50/

I want to stop everything with no file downloaded and no redirection. Is there a way to do that ?