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

Add body(Object) method to ServerResponse.BodyBuilder [SPR-15461] #20021

Closed
spring-issuemaster opened this Issue Apr 19, 2017 · 5 comments

Comments

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

spring-issuemaster commented Apr 19, 2017

Arjen Poutsma opened SPR-15461 and commented

We should add a ServerResponse.BodyBuilder.body(Object) method, to make it easier for users to set the response body to a non-Publisher type. Currently, this task can be accomplished via body(BodyInserters.fromObject(Object)), but because this method requires a static import of BodyInserters, the discoverability of it is not as high as body(Publisher, Class)). As such, users will go this route:

return ServerResponse.ok().body(Mono.just("Hello World"), String.class);

instead of the more elegant:

return ServerResponse.ok().body(fromObject("Hello World"));

However, adding the method has a serious consequence: there is the potential for accidental method overloading between a the new Object method and the existing Publisher body method. As a result, users will accidentally write a response with the publisher itself, rather than its contents:

Flux<String> flux = ...
return ServerResponse.ok().body(flux); // Whoops, should have been body(flux, String.class);

Of course, we cannot automatically call the correct method, because of the lacking Class argument. There are, however, a couple of other ways we can resolve this.

  1. Give the method a different name, such as bodyFromObject. If we go this route, we should be consistent and also rename the existing Publisher-based body method to bodyFromPublisher. As such, it has serious consequences for the brevity and UX of the API.
  2. Perform a runtime-check to see whether the Object passed to body is a Publisher, and throw an exception if so, warning the user of the error of their ways.
  3. Do not add the method. Since this a Reactive web framework, the case can be made that returning non-reactive types as a response is not a main design goal. Returning a String makes for a great demo, but might not be as necessary for real-life reactive scenarios.

Issue Links:

  • #20027 ServerResponse.BodyBuilder.body(Object) shadows body(Publisher) in Kotlin ServerResponseExtensions

Referenced from: commits 30f61e0

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Apr 19, 2017

Arjen Poutsma commented

During the 2017-04-19 meeting, it was decided to go for option 2, and also applies this to WebClient.RequestBodySpec.body(Object).

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Apr 19, 2017

Arjen Poutsma commented

Fixed in 30f61e0

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Apr 20, 2017

Dariusz Bacinski commented

there is an issue related to this change #20027

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Apr 20, 2017

Dariusz Bacinski commented

It is also very weird that you favour plain objects instead of Flux/Mono. Before this change we didn't have to pass String.class when passing Mono/Flux body.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Apr 21, 2017

Arjen Poutsma commented

It is also very weird that you favour plain objects instead of Flux/Mono. Before this change we didn't have to pass String.class when passing Mono/Flux body.

"Never attribute to malice that which is adequately explained by stupidity" - Hanlon's razor

I think option 3 given in the description above makes it very clear that we do not favour plain objects over reactive types. The only reason we went for option 2 and added it, was because we thought it had no negative consequences, which it does not have in Java. But we did not consider the Kotlin extensions, because there body(Object) shadows/overrides the existing body(Publisher) extension method, and apparently member methods take precedence over extension methods. As I made clear in #20027, this was unintentional.

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.