From c735ffb39b0404e97ae72d9e2c6f5e52fe6eb18c Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Fri, 17 Mar 2017 17:29:05 -0400 Subject: [PATCH] Fix content type issue in ResourceRegionHttpMessageWriter ResourceRegionHttpMessageWriter no longer extends from EncoderHttpMessageWriter freeing it to pass the correct content type into the encoder. Considering that the main benefit of EncoderHttpMessageWriter is to deal with content type fallback cases, there is nothing to be missed. Furthermore ResourceRegionHttpMessageWriter is a package private class that is used internally within ResourceHttpMessageWriter and never exposed externally as a an actual HttpMessageWriter. Issue: SPR-15358 --- .../http/codec/ResourceHttpMessageWriter.java | 4 +- .../ResourceRegionHttpMessageWriter.java | 53 ++++++++++++------- .../ResourceRegionHttpMessageWriterTests.java | 22 +++----- .../resource/ResourceWebHandlerTests.java | 2 - 4 files changed, 44 insertions(+), 37 deletions(-) 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 0aab70ecac..eb8fa0c45b 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 @@ -119,9 +119,9 @@ public class ResourceHttpMessageWriter extends AbstractServerHttpMessageWriter Flux.fromIterable(HttpRange.toResourceRegions(httpRanges, resource))); return this.resourceRegionHttpMessageWriter - .write(regions, ResolvableType.forClass(ResourceRegion.class), mediaType, response, mergedHints); + .writeRegions(regions, mediaType, response, mergedHints); } - else { + else { return write(inputStream, elementType, mediaType, response, mergedHints); } } diff --git a/spring-web/src/main/java/org/springframework/http/codec/ResourceRegionHttpMessageWriter.java b/spring-web/src/main/java/org/springframework/http/codec/ResourceRegionHttpMessageWriter.java index 4ccef76a95..8fb17b4f6b 100644 --- a/spring-web/src/main/java/org/springframework/http/codec/ResourceRegionHttpMessageWriter.java +++ b/spring-web/src/main/java/org/springframework/http/codec/ResourceRegionHttpMessageWriter.java @@ -24,54 +24,66 @@ import java.util.Optional; import java.util.OptionalLong; import org.reactivestreams.Publisher; +import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import org.springframework.core.ResolvableType; import org.springframework.core.codec.ResourceRegionEncoder; import org.springframework.core.io.InputStreamResource; import org.springframework.core.io.Resource; +import org.springframework.core.io.buffer.DataBuffer; +import org.springframework.core.io.buffer.DataBufferFactory; import org.springframework.core.io.support.ResourceRegion; import org.springframework.http.MediaType; import org.springframework.http.ReactiveHttpOutputMessage; import org.springframework.http.ZeroCopyHttpOutputMessage; /** - * Implementation of {@link HttpMessageWriter} that can write - * {@link ResourceRegion ResourceRegion}s. - * - *

Note that there is no {@link HttpMessageReader} counterpart. + * Package private helper for {@link ResourceHttpMessageWriter} to assist with + * writing {@link ResourceRegion ResourceRegion}s. * * @author Brian Clozel + * @author Rossen Stoyanchev * @since 5.0 */ -class ResourceRegionHttpMessageWriter extends EncoderHttpMessageWriter { +class ResourceRegionHttpMessageWriter { public static final String BOUNDARY_STRING_HINT = ResourceRegionHttpMessageWriter.class.getName() + ".boundaryString"; + private static final ResolvableType TYPE = ResolvableType.forClass(ResourceRegion.class); + + + private final ResourceRegionEncoder encoder; + + public ResourceRegionHttpMessageWriter() { - super(new ResourceRegionEncoder()); + this.encoder = new ResourceRegionEncoder(); } public ResourceRegionHttpMessageWriter(int bufferSize) { - super(new ResourceRegionEncoder(bufferSize)); + this.encoder = new ResourceRegionEncoder(bufferSize); } - @Override - public Mono write(Publisher inputStream, ResolvableType type, - MediaType contentType, ReactiveHttpOutputMessage outputMessage, Map hints) { + + public Mono writeRegions(Publisher inputStream, MediaType contentType, + ReactiveHttpOutputMessage outputMessage, Map hints) { if (hints != null && hints.containsKey(BOUNDARY_STRING_HINT)) { String boundary = (String) hints.get(BOUNDARY_STRING_HINT); hints.put(ResourceRegionEncoder.BOUNDARY_STRING_HINT, boundary); - outputMessage.getHeaders() - .setContentType(MediaType.parseMediaType("multipart/byteranges;boundary=" + boundary)); - return super.write(inputStream, type, contentType, outputMessage, hints); + + MediaType multipartType = MediaType.parseMediaType("multipart/byteranges;boundary=" + boundary); + outputMessage.getHeaders().setContentType(multipartType); + + DataBufferFactory bufferFactory = outputMessage.bufferFactory(); + Flux body = this.encoder.encode(inputStream, bufferFactory, TYPE, contentType, hints); + return outputMessage.writeWith(body); } else { return Mono.from(inputStream) .then(region -> { writeSingleResourceRegionHeaders(region, contentType, outputMessage); - return writeResourceRegion(region, type, outputMessage); + return writeResourceRegion(region, outputMessage); }); } } @@ -109,8 +121,8 @@ class ResourceRegionHttpMessageWriter extends EncoderHttpMessageWriter writeResourceRegion(ResourceRegion region, - ResolvableType type, ReactiveHttpOutputMessage outputMessage) { + private Mono writeResourceRegion(ResourceRegion region, ReactiveHttpOutputMessage outputMessage) { + if (outputMessage instanceof ZeroCopyHttpOutputMessage) { Optional file = getFile(region.getResource()); if (file.isPresent()) { @@ -122,8 +134,13 @@ class ResourceRegionHttpMessageWriter extends EncoderHttpMessageWriter hints = Collections.emptyMap(); + + Flux body = this.encoder.encode(Mono.just(region), bufferFactory, TYPE, contentType, hints); + return outputMessage.writeWith(body); } private static Optional getFile(Resource resource) { diff --git a/spring-web/src/test/java/org/springframework/http/codec/ResourceRegionHttpMessageWriterTests.java b/spring-web/src/test/java/org/springframework/http/codec/ResourceRegionHttpMessageWriterTests.java index 56669b6f99..adef5264e8 100644 --- a/spring-web/src/test/java/org/springframework/http/codec/ResourceRegionHttpMessageWriterTests.java +++ b/spring-web/src/test/java/org/springframework/http/codec/ResourceRegionHttpMessageWriterTests.java @@ -23,7 +23,6 @@ import java.util.HashMap; import java.util.Map; import org.junit.Before; -import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -31,7 +30,6 @@ import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; -import org.springframework.core.ResolvableType; import org.springframework.core.io.ByteArrayResource; import org.springframework.core.io.Resource; import org.springframework.core.io.support.ResourceRegion; @@ -41,8 +39,10 @@ import org.springframework.mock.http.server.reactive.test.MockServerHttpResponse import org.springframework.util.MimeTypeUtils; import org.springframework.util.StringUtils; -import static org.hamcrest.Matchers.*; -import static org.junit.Assert.*; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.startsWith; +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertThat; /** * Unit tests for {@link ResourceRegionHttpMessageWriter}. @@ -67,19 +67,13 @@ public class ResourceRegionHttpMessageWriterTests { } - @Test - public void writableMediaTypes() throws Exception { - assertThat(this.writer.getWritableMediaTypes(), - containsInAnyOrder(MimeTypeUtils.APPLICATION_OCTET_STREAM, MimeTypeUtils.ALL)); - } - @Test public void shouldWriteResourceRegion() throws Exception { ResourceRegion region = new ResourceRegion(this.resource, 0, 6); + Map hints = Collections.emptyMap(); - Mono mono = this.writer.write(Mono.just(region), ResolvableType.forClass(ResourceRegion.class), - MediaType.TEXT_PLAIN, this.response, Collections.emptyMap()); + Mono mono = this.writer.writeRegions(Mono.just(region), MediaType.TEXT_PLAIN, this.response, hints); StepVerifier.create(mono).expectComplete().verify(); assertThat(this.response.getHeaders().getContentType(), is(MediaType.TEXT_PLAIN)); @@ -91,7 +85,6 @@ public class ResourceRegionHttpMessageWriterTests { } @Test - @Ignore("Until issue resolved: ResourceRegion should not use response content-type") public void shouldWriteMultipleResourceRegions() throws Exception { Flux regions = Flux.just( new ResourceRegion(this.resource, 0, 6), @@ -103,8 +96,7 @@ public class ResourceRegionHttpMessageWriterTests { Map hints = new HashMap<>(1); hints.put(ResourceRegionHttpMessageWriter.BOUNDARY_STRING_HINT, boundary); - Mono mono = this.writer.write(regions, ResolvableType.forClass(ResourceRegion.class), - MediaType.TEXT_PLAIN, this.response, hints); + Mono mono = this.writer.writeRegions(regions, MediaType.TEXT_PLAIN, this.response, hints); StepVerifier.create(mono).expectComplete().verify(); HttpHeaders headers = this.response.getHeaders(); diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/resource/ResourceWebHandlerTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/resource/ResourceWebHandlerTests.java index ba5b32bf19..cf3a5af6e4 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/resource/ResourceWebHandlerTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/resource/ResourceWebHandlerTests.java @@ -26,7 +26,6 @@ import java.util.List; import java.util.concurrent.TimeUnit; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; @@ -538,7 +537,6 @@ public class ResourceWebHandlerTests { } @Test - @Ignore("Until issue resolved: ResourceRegion should not use response content-type") public void partialContentMultipleByteRanges() throws Exception { MockServerWebExchange exchange = MockServerHttpRequest.get("").header("Range", "bytes=0-1, 4-5, 8-9").toExchange(); setPathWithinHandlerMapping(exchange, "foo.txt");