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

Sending form and multipart data with WebTestClient [SPR-16118] #20666

Closed
spring-issuemaster opened this issue Oct 26, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Oct 26, 2017

Andy Wilkinson opened SPR-16118 and commented

I'm enjoying the more fluent nature of WebTestClient when compared with MockMvc, but have found that sending a request with form or multipart data body jars a bit. The main problem is that, as far as I can tell, I have to create a MultiValueMap for the data separately and then use BodyInserters.fromMultipartData or BodyInserters.fromFormData, for example:

MultiValueMap<String, String> formData = new LinkedMultiValueMap<>();
formData.add("username", "Tester");
this.webTestClient.post().uri("/users").body(BodyInserters.fromFormData(formData))

I'd like to be able to do something like this instead:

this.webTestClient.post().uri("/users").body(BodyInserters.formData().param("username", "Tester"))

Affects: 5.0.1

Sub-tasks:

  • #20681 Chained API for form and multipart data in BodyInserters
  • #20682 Create builder for multipart bodies

Issue Links:

  • #20679 Multipart form data can no longer be sent with syncBody in WebTestClient

Referenced from: commits 14f02d7, 48c2cc1

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 26, 2017

Andy Wilkinson commented

Related to this, the code required to create multipart data appears to be quite a lot more verbose with WebTestClient. Either that, or, and there's a good chance this is it, I've missed something in the API.

With MockMvc I could do this:

MockMultipartFile image = new MockMultipartFile("image", "image.png", "image/png",
		"<<png data>>".getBytes());
MockMultipartFile metadata = new MockMultipartFile("metadata", "",
		"application/json", "{ \"version\": \"1.0\"}".getBytes());

The equivalent code for use with WebTestClient appears to be:

MultiValueMap<String, Object> multipartData = new LinkedMultiValueMap<>();
HttpHeaders imageHeaders = new HttpHeaders();
imageHeaders.add(HttpHeaders.CONTENT_TYPE, MediaType.IMAGE_PNG_VALUE);
Resource imageResource = new ByteArrayResource("<<png data>>".getBytes()) {

	@Override
	public String getFilename() {
		return "image.png";
	}

};
multipartData.add("image", new HttpEntity<>(imageResource, imageHeaders));
HttpHeaders metadataHeaders = new HttpHeaders();
metadataHeaders.add(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE);
multipartData.add("metadata",
		new HttpEntity<>("{\"version\":\"1.0\"}", metadataHeaders));
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 26, 2017

Rossen Stoyanchev commented

The following two should both work just the same:

this.webTestClient.post().uri("/users").body(BodyInserters.fromFormData(formData));
this.webTestClient.post().uri("/users").syncBody(formData);

In other words you don't have to use BodyInserters, it doesn' t add any value. As for .body(BodyInserters.formData().param("username", "Tester")) that would have to have some sort of .build() at the end wouldn't it? In the end I would rather consider some sort of multipart builder that produces a MultiValueMap. That would be generally useful across the RestTemplate, WebClient, and WebTestClient.

For the MultiValueMap itself, I don't think you need to wrap the image with HttpEntity. The writer will write the content type based on the file name. If you couple that with some local TestResource class that extends ByteArrayResource + returns a file name it becomes much better:

MultiValueMap<String, Object> multipartData = new LinkedMultiValueMap<>();
multipartData.add("image", new TestResource("<<png data>>", "image.png"));

The same would be true for the JSON part, if you were to use a typed Object instead of the serialized JSON.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 27, 2017

Andy Wilkinson commented

Thank you for the tips, Rossen.

In other words you don't have to use BodyInserters, it doesn't add any value

I had assumed that BodyInserters.fromFormData was required for the multi-value map to be interpreted as multipart form data. I'm now a bit confused about the purpose of that method. When would you use it rather than syncBody given that the map's being passed straight in in both cases (there's no publisher involved).

that would have to have some sort of .build() at the end wouldn't it?

I'd envisaged each call to param returning an inserter that's ready for use, either by creating a new one with the additional parameter, or be modifying the existing one.

I would rather consider some sort of multipart builder that produces a MultiValueMap. That would be generally useful across the RestTemplate, WebClient, and WebTestClient.

That sounds to me like it would be a nice enhancement.

For the MultiValueMap itself, I don't think you need to wrap the image with HttpEntity. The writer will write the content type based on the file name. If you couple that with some local TestResource class that extends ByteArrayResource + returns a file name it becomes much better

That is indeed much better. When I started working on this I had to dig into the code to figure out how to set a header. That's when I noticed the special treatment of HttpEntity so I just went from there. Perhaps this is largely a documentation issue, or an issue of me not reading the right documentation? What should I have looked at to learn about the support for HttpEntity and inferring the content type from a file name?

Would you consider adding something like TestResource to the Framework? From what I've learned here, it would go a long way to replicating the functionality that's offered by MockMultipartFile.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 27, 2017

Rossen Stoyanchev commented

I had assumed that BodyInserters.fromFormData was required ... I'm now a bit confused about the purpose ...

There is no practical difference I can think of between that and syncBody(multiValueMap), besides advertising the idea that such a thing exists.

I'd envisaged each call to param returning an inserter that's ready for use, either by creating a new one with the additional parameter, or be modifying the existing one.

Arjen Poutsma what do you think of the idea of enhancing BodyInserters.fromFormData to support chained addition of parameters? It wouldn't be hard to do and would actually create a practical advantage of using it over syncBody.

-The second observation here is that fromFormData takes a MultiValueMap<String, String> whereas the writer works with MultiValueMap<String, ?>, which means you can't pass part headers. That looks like a bug to me, one that we can probably still correct. I don't think it would break existing code. And if we made such a change then you could do even more with chained methods on fromFormData.- (retracted: fromMultpartData is for this)

Arjen Poutsma independent of that, what do you think of adding a MultipartHttpEntity sub-class of HttpEntity? It could provide a constructor with a content-type, and/or a builder for adding part headers + body similar to the RequestEntity sub-class.

What should I have looked at to learn about the support for HttpEntity and inferring the content type from a file name?

I think we could add some coverage of multipart requests to the reference for WebTestClient, WebClient, and RestTemplate. Probably also improve the fromFormData method to point out syncBody and the fact you can pass HttpEntity with part headers. I will set a fix version for that alone.

Would you consider adding something like TestResource to the Framework? From what I've learned here, it would go a long way to replicating the functionality that's offered by MockMultipartFile.

Sam Brannen what do you think of adding something like MockResource to spring-test which would be a ByteArrayResource + a file name. What package could that go in?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 30, 2017

Andy Wilkinson commented

I just upgraded to Framework 5.0.1 and noticed a change in behaviour that's related to this discussion. Previously, this worked:

MultiValueMap<String, Object> multipartData = new LinkedMultiValueMap<>();
multipartData.add("file", new ByteArrayResource(new byte[] { 1, 2, 3, 4 }) {

	@Override
	public String getFilename() {
		return "image.png";
	}

});
ExchangeResult result = WebTestClient
		.bindToRouterFunction(RouterFunctions.route(POST("/foo"), (req) -> {
			req.body(BodyExtractors.toMultipartData()).block();
			return ServerResponse.ok().build();
		})).configureClient().baseUrl("http://localhost").build().post()
		.uri("/foo").syncBody(multipartData).exchange().expectBody()
		.returnResult();

Following the upgrade it appears to be trying to send form data rather than multipart data and, as a result, it fails:

java.lang.ClassCastException: org.springframework.restdocs.webtestclient.WebTestClientRequestConverterTests$1 cannot be cast to java.lang.String
	at org.springframework.http.codec.FormHttpMessageWriter.generateForm(FormHttpMessageWriter.java:127)
	at org.springframework.http.codec.FormHttpMessageWriter.lambda$write$0(FormHttpMessageWriter.java:102)
	at org.springframework.http.codec.FormHttpMessageWriter$$Lambda$185/198499365.apply(Unknown Source)
	at reactor.core.publisher.FluxMapFuseable$MapFuseableSubscriber.onNext(FluxMapFuseable.java:107)
	at reactor.core.publisher.Operators$ScalarSubscription.request(Operators.java:1649)
	at reactor.core.publisher.FluxMapFuseable$MapFuseableSubscriber.request(FluxMapFuseable.java:156)
	at reactor.core.publisher.MonoFlatMap$FlatMapMain.onSubscribe(MonoFlatMap.java:103)
	at reactor.core.publisher.FluxMapFuseable$MapFuseableSubscriber.onSubscribe(FluxMapFuseable.java:90)
	at reactor.core.publisher.MonoJust.subscribe(MonoJust.java:54)
	at reactor.core.publisher.MonoMapFuseable.subscribe(MonoMapFuseable.java:59)
	at reactor.core.publisher.MonoFlatMap.subscribe(MonoFlatMap.java:60)
	at reactor.core.publisher.Mono.subscribe(Mono.java:2913)
	at reactor.core.publisher.Mono.subscribeWith(Mono.java:3021)
	at reactor.core.publisher.Mono.subscribe(Mono.java:2907)
	at reactor.core.publisher.Mono.subscribe(Mono.java:2874)
	at reactor.core.publisher.Mono.subscribe(Mono.java:2846)
	at org.springframework.test.web.reactive.server.HttpHandlerConnector.connect(HttpHandlerConnector.java:101)
	at org.springframework.test.web.reactive.server.WiretapConnector.connect(WiretapConnector.java:61)
	at org.springframework.web.reactive.function.client.ExchangeFunctions$DefaultExchangeFunction.exchange(ExchangeFunctions.java:74)
	at org.springframework.web.reactive.function.client.DefaultWebClient$DefaultRequestBodyUriSpec.exchange(DefaultWebClient.java:327)
	at org.springframework.test.web.reactive.server.DefaultWebTestClient$DefaultRequestBodyUriSpec.exchange(DefaultWebTestClient.java:282)
	at org.springframework.restdocs.webtestclient.WebTestClientRequestConverterTests.multipartUploadFromResource(WebTestClientRequestConverterTests.java:191)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:483)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:86)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:459)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:678)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:382)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:192)
	Suppressed: java.lang.Exception: #block terminated with an error
		at reactor.core.publisher.BlockingSingleSubscriber.blockingGet(BlockingSingleSubscriber.java:126)
		at reactor.core.publisher.Mono.block(Mono.java:1185)
		... 25 more

It's trying to cast the anonymous ByteArrayResource subclass to a String. I can work around the problem by using .body(BodyInserters.fromMultipartData(multipartData)) in place of .syncBody(multipartData).

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 30, 2017

Rossen Stoyanchev commented

Could you create a separate ticket that we can treat as a 5.0.1 regression separate from the improvements discussed here? Most likely the issue is unrelated to the WebTestClient in any case and has more to do with the underlying codecs, configurer, and inserters.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 30, 2017

Andy Wilkinson commented

Done: #20679

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 6, 2017

Rossen Stoyanchev commented

To summarize:

  • #20681 adds the chained-method, in-line syntax to BodyInserters for form and multipart data.
  • #20682 adds a MultipartBodyBuilder (see difference before and after).
  • the reference doc for WebClient now explicitly discussed form data and multipart requests (with explanations on content-type being optional) and the reference doc for WebTestClient refers to that also
  • the Javadoc between syncBody and BodyInserters has cross-references to each other

The only thing left from the list above is a MockResource. I will leave this one out because it's not a fully baked concept. First of all ByteResource already provides an implementation. Extending that with a filename only has a specific purpose for multipart requests, where for example getFile would throw an exception and that may not make sense for another use case. For simulating a file upload in testing, I would suggest using an actual temp file:

File myFile = File.createTempFile("test-file", ".txt");
FileCopyUtils.copy(bytes, myFile);
Resource resource = new FileSystemResource(myFile);
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 6, 2017

Rossen Stoyanchev commented

Resolving for now but if you think anything remains, let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.