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

Support filename hint for client side use of ResourceDecoder #25516

Closed
filiphr opened this issue Aug 3, 2020 · 2 comments
Closed

Support filename hint for client side use of ResourceDecoder #25516

filiphr opened this issue Aug 3, 2020 · 2 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@filiphr
Copy link
Contributor

filiphr commented Aug 3, 2020

With #22267 the ResourceDecoder was extended to support a filename hint that allows setting the filename of the Resource. However, this does not work correctly with using the Spring WebClient.

e.g.

ResponseEntity<ByteArrayResource> response = webClient.get("/some-resource)
    .flatMap(response -> response.toEntity(ByteArrayResource.class))
    .block();

The resource returned by this response will have null when invoking getFileName().

The reason for that is the following:

When invoking response.ToEntity(ByteArrayResource.class). We will reach:

private static <T> BodyExtractor<Mono<T>, ReactiveHttpInputMessage> toMono(ResolvableType elementType) {
return (inputMessage, context) ->
readWithMessageReaders(inputMessage, context, elementType,
(HttpMessageReader<T> reader) -> readToMono(inputMessage, context, elementType, reader),
ex -> Mono.from(unsupportedErrorHandler(inputMessage, ex)),
skipBodyAsMono(inputMessage));
}

Which calls readToMono:

private static <T> Mono<T> readToMono(ReactiveHttpInputMessage message, BodyExtractor.Context context,
ResolvableType type, HttpMessageReader<T> reader) {
return context.serverResponse()
.map(response -> reader.readMono(type, type, (ServerHttpRequest) message, response, context.hints()))
.orElseGet(() -> reader.readMono(type, message, context.hints()));

There are now 2 possibilities for invoking the reader:

  1. reader.readMono(type, type, (ServerHttpRequest) message, response, context.hints()) - When the BodyExtractor.Context#serverResponse is not empty, which is the case when using ServerRequest. This case is correct.
  2. reader.readMono(type, message, context.hints()) - When the BodyExtractor.Context#serverResponse is empty which is the case for a ClientResponse

Since we are using WebClient we will skip the analysis for 1. And go to 2.

The serverResponse on the extractor is set to empty here:

this.bodyExtractorContext = new BodyExtractor.Context() {
@Override
public List<HttpMessageReader<?>> messageReaders() {
return strategies.messageReaders();
}
@Override
public Optional<ServerHttpResponse> serverResponse() {
return Optional.empty();
}
@Override
public Map<String, Object> hints() {
return Hints.from(Hints.LOG_PREFIX_HINT, logPrefix);
}
};

When we look into the implementation of reader.readMono(type, message, context.hints()). More precisely the implementation of

Mono<T> readMono(ResolvableType elementType, ReactiveHttpInputMessage message, Map<String, Object> hints);

In DecoderHttpMessageReader:

@Override
public Mono<T> readMono(ResolvableType elementType, ReactiveHttpInputMessage message, Map<String, Object> hints) {
MediaType contentType = getContentType(message);
return this.decoder.decodeToMono(message.getBody(), elementType, contentType, hints);
}

We can see that the hints are passed as is without creating new hints based on the message.

The fix would be to also get new hints from ReactiveHttpInputMessage when reading a flux or mono without a ServerHttpResponse.

More precisely in this location:

@Override
public Flux<T> read(ResolvableType elementType, ReactiveHttpInputMessage message, Map<String, Object> hints) {
MediaType contentType = getContentType(message);
return this.decoder.decode(message.getBody(), elementType, contentType, hints);
}
@Override
public Mono<T> readMono(ResolvableType elementType, ReactiveHttpInputMessage message, Map<String, Object> hints) {
MediaType contentType = getContentType(message);
return this.decoder.decodeToMono(message.getBody(), elementType, contentType, hints);
}

This can be done only for the ResourceHttpMessageReader and not for DecoderHttpMessageReader

Reproducable Test case
/**
 * Unit tests for {@link ResourceHttpMessageReader}.
 *
 * @author Filip Hrisafov
 */
public class ResourceHttpMessageReaderTests extends AbstractLeakCheckingTests {

	private final ResourceHttpMessageReader reader = new ResourceHttpMessageReader();

	@Test
	void readResourceAsMono() throws IOException {
		String body = "Test resource content";
		MockServerHttpRequest request = request(body, "test.txt");
		Resource result = this.reader.readMono(ResolvableType.forClass(ByteArrayResource.class), request, null)
				.single()
				.block();

		assertThat(result).isNotNull();
		assertThat(result.getFilename()).isEqualTo("test.txt");
		try (InputStream stream = result.getInputStream()) {
			assertThat(stream).hasContent(body);
		}
	}

	private MockServerHttpRequest request(String body, String filename) {
		return request(Mono.just(stringBuffer(body)), filename);
	}

	private MockServerHttpRequest request(Publisher<? extends DataBuffer> body, String filename) {
		return MockServerHttpRequest
				.method(HttpMethod.GET, "/")
				.header(HttpHeaders.CONTENT_TYPE, MediaType.TEXT_PLAIN_VALUE)
				.header(HttpHeaders.CONTENT_DISPOSITION, ContentDisposition.builder("attachment")
						.name("file")
						.filename(filename)
						.build()
						.toString())
				.body(body);
	}

	private DataBuffer stringBuffer(String value) {
		byte[] bytes = value.getBytes(StandardCharsets.UTF_8);
		DataBuffer buffer = this.bufferFactory.allocateBuffer(bytes.length);
		buffer.write(bytes);
		return buffer;
	}

}

I am not providing a PR, because I am not sure how you would best like this to be fixed.

Edit: I submitted the issue before having the chance to clean it up.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 3, 2020
@filiphr filiphr changed the title Resource filename is null Spring WebClient Resource filename is null when using Spring WebClient Aug 4, 2020
@sbrannen sbrannen added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Aug 10, 2020
@rstoyanchev rstoyanchev self-assigned this Aug 13, 2020
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Aug 13, 2020
@rstoyanchev rstoyanchev added this to the 5.3 RC1 milestone Aug 13, 2020
@rstoyanchev rstoyanchev changed the title Resource filename is null when using Spring WebClient Support filename hint for client side use of ResourceDecoder Aug 13, 2020
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Aug 13, 2020

Indeed we have slightly richer context on the server side where we tend to extract hints from controller method signatures. However hints could also be extracted from headers as in this case.

I experimented with this additional protected method in DecoderHttpMessageReader which can then be invoked from the decode methods with just a ReactiveHttpInputMessage:

protected Map<String, Object> getReadHints(ResolvableType elementType, ReactiveHttpInputMessage message) {
	return Hints.none();
}

This can then be overridden in ResourceHttpMessageReader and that seems to work, so I'll put this together for 5.3.

@filiphr
Copy link
Contributor Author

filiphr commented Aug 14, 2020

Thanks a lot for doing the enhancement @rstoyanchev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants