From 578af59f0ce2a5bf89a7e8fa7d38ce4a7def8bd4 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Tue, 20 Sep 2016 23:46:21 +0200 Subject: [PATCH] Polish byte-range resource handling --- .../reactive/resource/ResourceWebHandler.java | 5 +-- .../resource/ResourceWebHandlerTests.java | 8 ++-- .../http/codec/ResourceHttpMessageWriter.java | 38 +++++++++++-------- .../codec/ResourceHttpMessageWriterTests.java | 14 +++---- 4 files changed, 35 insertions(+), 30 deletions(-) diff --git a/spring-web-reactive/src/main/java/org/springframework/web/reactive/resource/ResourceWebHandler.java b/spring-web-reactive/src/main/java/org/springframework/web/reactive/resource/ResourceWebHandler.java index bda5181981..99b557b809 100644 --- a/spring-web-reactive/src/main/java/org/springframework/web/reactive/resource/ResourceWebHandler.java +++ b/spring-web-reactive/src/main/java/org/springframework/web/reactive/resource/ResourceWebHandler.java @@ -54,7 +54,6 @@ import org.springframework.web.reactive.HandlerMapping; import org.springframework.web.reactive.accept.CompositeContentTypeResolver; import org.springframework.web.reactive.accept.PathExtensionContentTypeResolver; import org.springframework.web.server.MethodNotAllowedException; -import org.springframework.web.server.ResponseStatusException; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebHandler; @@ -331,6 +330,7 @@ public class ResourceWebHandler // Content phase if (HttpMethod.HEAD.equals(exchange.getRequest().getMethod())) { setHeaders(exchange, resource, mediaType); + exchange.getResponse().getHeaders().set(HttpHeaders.ACCEPT_RANGES, "bytes"); logger.trace("HEAD request - skipping content"); return Mono.empty(); } @@ -340,7 +340,7 @@ public class ResourceWebHandler null, ResolvableType.forClass(Resource.class), mediaType, exchange.getRequest(), exchange.getResponse(), Collections.emptyMap()); } - catch (IOException|ResponseStatusException ex) { + catch (IOException ex) { return Mono.error(ex); } }); @@ -503,7 +503,6 @@ public class ResourceWebHandler HttpHeaders resourceHeaders = ((HttpResource) resource).getResponseHeaders(); exchange.getResponse().getHeaders().putAll(resourceHeaders); } - headers.set(HttpHeaders.ACCEPT_RANGES, "bytes"); } diff --git a/spring-web-reactive/src/test/java/org/springframework/web/reactive/resource/ResourceWebHandlerTests.java b/spring-web-reactive/src/test/java/org/springframework/web/reactive/resource/ResourceWebHandlerTests.java index 2bd9d0260b..a2d068bd8e 100644 --- a/spring-web-reactive/src/test/java/org/springframework/web/reactive/resource/ResourceWebHandlerTests.java +++ b/spring-web-reactive/src/test/java/org/springframework/web/reactive/resource/ResourceWebHandlerTests.java @@ -525,10 +525,10 @@ public class ResourceWebHandlerTests { this.request.addHeader("Range", "bytes= foo bar"); this.exchange.getAttributes().put(PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "foo.txt"); - TestSubscriber.subscribe(this.handler.handle(this.exchange)) - .assertErrorWith(throwable -> { - assertThat(throwable, instanceOf(IllegalArgumentException.class)); - }); + TestSubscriber.subscribe(this.handler.handle(this.exchange)).assertComplete(); + + assertEquals(HttpStatus.REQUESTED_RANGE_NOT_SATISFIABLE, this.response.getStatusCode()); + assertEquals("bytes", this.response.getHeaders().getFirst("Accept-Ranges")); } @Test diff --git a/spring-web/src/main/java/org/springframework/http/codec/ResourceHttpMessageWriter.java b/spring-web/src/main/java/org/springframework/http/codec/ResourceHttpMessageWriter.java index 6baa294ebd..0aab70ecac 100644 --- a/spring-web/src/main/java/org/springframework/http/codec/ResourceHttpMessageWriter.java +++ b/spring-web/src/main/java/org/springframework/http/codec/ResourceHttpMessageWriter.java @@ -104,24 +104,30 @@ public class ResourceHttpMessageWriter extends AbstractServerHttpMessageWriter write(Publisher inputStream, ResolvableType streamType, ResolvableType elementType, MediaType mediaType, ServerHttpRequest request, ServerHttpResponse response, Map hints) { - - Map mergedHints = new HashMap<>(hints); - mergedHints.putAll(resolveWriteHints(streamType, elementType, mediaType, request)); - if (mergedHints.containsKey(HTTP_RANGE_REQUEST_HINT)) { - response.setStatusCode(HttpStatus.PARTIAL_CONTENT); - List httpRanges = (List) mergedHints.get(HTTP_RANGE_REQUEST_HINT); - if (httpRanges.size() > 1) { - final String boundary = MimeTypeUtils.generateMultipartBoundaryString(); - mergedHints.put(ResourceRegionHttpMessageWriter.BOUNDARY_STRING_HINT, boundary); + try { + response.getHeaders().set(HttpHeaders.ACCEPT_RANGES, "bytes"); + Map mergedHints = new HashMap<>(hints); + mergedHints.putAll(resolveWriteHints(streamType, elementType, mediaType, request)); + if (mergedHints.containsKey(HTTP_RANGE_REQUEST_HINT)) { + response.setStatusCode(HttpStatus.PARTIAL_CONTENT); + List httpRanges = (List) mergedHints.get(HTTP_RANGE_REQUEST_HINT); + if (httpRanges.size() > 1) { + final String boundary = MimeTypeUtils.generateMultipartBoundaryString(); + mergedHints.put(ResourceRegionHttpMessageWriter.BOUNDARY_STRING_HINT, boundary); + } + Flux regions = Flux.from(inputStream) + .flatMap(resource -> Flux.fromIterable(HttpRange.toResourceRegions(httpRanges, resource))); + + return this.resourceRegionHttpMessageWriter + .write(regions, ResolvableType.forClass(ResourceRegion.class), mediaType, response, mergedHints); + } + else { + return write(inputStream, elementType, mediaType, response, mergedHints); } - Flux regions = Flux.from(inputStream) - .flatMap(resource -> Flux.fromIterable(HttpRange.toResourceRegions(httpRanges, resource))); - - return this.resourceRegionHttpMessageWriter - .write(regions, ResolvableType.forClass(ResourceRegion.class), mediaType, response, mergedHints); } - else { - return write(inputStream, elementType, mediaType, response, mergedHints); + catch (IllegalArgumentException exc) { + response.setStatusCode(HttpStatus.REQUESTED_RANGE_NOT_SATISFIABLE); + return response.setComplete(); } } diff --git a/spring-web/src/test/java/org/springframework/http/codec/ResourceHttpMessageWriterTests.java b/spring-web/src/test/java/org/springframework/http/codec/ResourceHttpMessageWriterTests.java index ee03eb790d..4a0cbb6d16 100644 --- a/spring-web/src/test/java/org/springframework/http/codec/ResourceHttpMessageWriterTests.java +++ b/spring-web/src/test/java/org/springframework/http/codec/ResourceHttpMessageWriterTests.java @@ -20,9 +20,7 @@ import java.nio.charset.StandardCharsets; import java.util.Collections; import org.junit.Before; -import org.junit.Rule; import org.junit.Test; -import org.junit.rules.ExpectedException; import org.reactivestreams.Publisher; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; @@ -36,6 +34,7 @@ import org.springframework.core.io.buffer.DataBufferUtils; import org.springframework.core.io.buffer.support.DataBufferTestUtils; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpRange; +import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest; import org.springframework.mock.http.server.reactive.test.MockServerHttpResponse; @@ -60,9 +59,6 @@ public class ResourceHttpMessageWriterTests { private Resource resource; - @Rule - public ExpectedException expectedException = ExpectedException.none(); - @Before public void setUp() throws Exception { @@ -84,6 +80,7 @@ public class ResourceHttpMessageWriterTests { assertThat(this.response.getHeaders().getContentType(), is(MediaType.TEXT_PLAIN)); assertThat(this.response.getHeaders().getContentLength(), is(39L)); + assertThat(this.response.getHeaders().getFirst(HttpHeaders.ACCEPT_RANGES), is("bytes")); Mono result = reduceToString(this.response.getBody(), this.response.bufferFactory()); TestSubscriber.subscribe(result).assertComplete().assertValues("Spring Framework test resource content."); @@ -98,6 +95,7 @@ public class ResourceHttpMessageWriterTests { assertThat(this.response.getHeaders().getContentType(), is(MediaType.TEXT_PLAIN)); assertThat(this.response.getHeaders().getFirst(HttpHeaders.CONTENT_RANGE), is("bytes 0-5/39")); + assertThat(this.response.getHeaders().getFirst(HttpHeaders.ACCEPT_RANGES), is("bytes")); assertThat(this.response.getHeaders().getContentLength(), is(6L)); Mono result = reduceToString(this.response.getBody(), this.response.bufferFactory()); @@ -105,12 +103,14 @@ public class ResourceHttpMessageWriterTests { } @Test - public void shouldThrowErrorInvalidRange() throws Exception { + public void shouldSetRangeNotSatisfiableStatus() throws Exception { this.request.getHeaders().set(HttpHeaders.RANGE, "invalid"); - this.expectedException.expect(IllegalArgumentException.class); TestSubscriber.subscribe(this.writer.write(Mono.just(resource), null, ResolvableType.forClass(Resource.class), MediaType.TEXT_PLAIN, this.request, this.response, Collections.emptyMap())); + + assertThat(this.response.getHeaders().getFirst(HttpHeaders.ACCEPT_RANGES), is("bytes")); + assertThat(this.response.getStatusCode(), is(HttpStatus.REQUESTED_RANGE_NOT_SATISFIABLE)); } private Mono reduceToString(Publisher buffers, DataBufferFactory bufferFactory) {