Provide better intercepting model for controllers returning @ResponseBody [SPR-10859]

[spring-projects-issues]

Marcel Overdijk opened SPR-10859 and commented

I'm using Spring as rest server where all controller actions return @ResponseBody annotated results. Just like Twitter I'm implementing 'suppress_response_codes' functionality.

Normally I will return json response like:

HTTP Status	Body
200	{ "first_name": "Marcel", "last_name": "Overdijk" }
400	{ "error": "the error message" }

But with a suppress_response_codes=true query param in the request it is:

HTTP Status	Body
200	{ "status": 200, "data": { "first_name": "Marcel", "last_name": "Overdijk" } }
200	{ "status": 400, "data": { "error": "the error message" } }

Implementing a HandlerInterceptor does not seem the best fit for intercepting controller actions returning @ResponseBody responsed as the converter seems to have already send the data in the postHandle.

There should be decent intercepting model for @ResponseBody where it's possible to retrieve the original object being returned by the controller actions.

---

Affects: 3.2.4

Issue Links:

• Add JSONP support to MappingJackson2MessageConverter [SPR-9899] #14532 Add JSONP support to MappingJackson2MessageConverter ("is depended on by")
• Consider adding a post-processing hook for the return value of @RequestMapping methods [SPR-10367] #15000 Consider adding a post-processing hook for the return value of @RequestMapping methods ("is duplicated by")
• MockMvc ignores HTTP status code overridden by filter [SPR-11760] #16382 MockMvc ignores HTTP status code overridden by filter
• Integrate Jackson @JsonView [SPR-7156] #11815 Integrate Jackson @JsonView
• Response is committed before Interceptor postHandle invoked [SPR-9226] #13864 Response is committed before Interceptor postHandle invoked ("supersedes")
• Make HandlerInterceptor play nice with @ResponseBody [SPR-7897] #12553 Make HandlerInterceptor play nice with @ResponseBody ("supersedes")

Referenced from: commits c9d0ebd, 96b18c8, 2655c50

6 votes, 12 watchers

[spring-projects-issues]

Michael Osipov commented

Isn't it absolutely pointless to repeat the same information again? I don't think that twitter is a good example here.

[spring-projects-issues]

Marcel Overdijk commented

~michael-o what point are you trying to make? your comment is completely worthless...

[spring-projects-issues]

Michael Osipov commented

Your usecase is a bad example because the information in the response is redundant. There is no need to duplicate the response code.

[spring-projects-issues]

Marcel Overdijk commented

The response code is not duplicated.
The use case is sending a http 200 always and having the actual status code in response body.
Please read the ticket.

[spring-projects-issues]

Michael Osipov commented

My bad, did not read careful enough. I have encountered such an API before. It remains bad design. This isn't REST because you do abuse the HTTP status codes.

[spring-projects-issues]

Jon Brisbin commented

This is simply JSONP without the function call. It's not abusing HTTP status codes but ensuring that handlers are always invoked no matter the outcome according to the status, which might be correct in a RESTful situation but can't be dealt with in a purely RESTful way because a browser isn't a RESTful client (it makes certain assumptions in the Ajax APIs that don't always fit well with what's being accomplished).

If using JSONP, a similar situation exists where you need to take a string of JSON, change the Content-Type to application/javascript, change the HTTP status code to ensure that the Ajax library will actually invoke the function, and wrap the text with a call to a Javascript function. All this should happen after all the other normal JSON processing. Currently, however, it's hard to do this with anything other than a ServletFilter.

[spring-projects-issues]

Marcel Overdijk commented

Jon Brisbin Please vote for issue if you think this is important, so it might get attention from from Spring devs.

[spring-projects-issues]

Sergey Gluschenko commented

The other usecase is that I need to add completely custom headers to response, while controller's methods are returning @ResponseBody

[spring-projects-issues]

Michael Osipov commented

Sergey Gluschenko, why don't you simply use ResponseEntity?

[spring-projects-issues]

Sergey Gluschenko commented

Michael Osipov, that's because I'm writing an utility and I don't want to force end users to change their preferred approach of working with Spring MVC. I accept your proposal as a workaround, but I think you'll agree that there is already a number of live usecases that require improvement requested.

[spring-projects-issues]

Michael Osipov commented

Sergey Gluschenko, yes, I do agree. I have created a similar ticket ages ago, see #14614.

[spring-projects-issues]

Sandeep Chouksey commented

I do have a use case where we need to add customer header to response and our current approach is based on @ResponseBody and we dont want to change our implementation. As Sergey Gluschenko mentioned, this will be a good option to improvise.

[spring-projects-issues]

Jason Eacott commented

I have a use case where I need to generate response headers where the header content depends on data generated during the request. I'm currently using @ResponseBody widely and this issue is unexpected and a real pain. (I wish the need for @ResponseBody could be autodetected instead of manually setting this annotation on every method but thats another issue).

[spring-projects-issues]

Jason Eacott commented

I have a use case where I need to generate response headers where the header content depends on data generated during the request. I'm currently using @ResponseBody widely and this issue is unexpected and a real pain. (I wish the need for @ResponseBody could be autodetected instead of manually setting this annotation on every method but thats another issue).

I also tried this without @ResponseBody, using ResponseEntity instead and it made no difference. The response is always committed from ServletInvocableHandlerMethod before postHandle is called :(

[spring-projects-issues]

Rossen Stoyanchev commented

I wish the need for @ResponseBody could be autodetected

I'm not sure I understand exactly what you mean but if you haven't yet, check @RestController.

The response is always committed from ServletInvocableHandlerMethod before postHandle is called

Actually we don't do that, at least not explicitly. There were some converters that would cause the OutputStream to be closed but those were fixed (see this commit). Keep in mind however that when the content written to the response exceeds the Servlet container output buffer, at that point the response is committed. So waiting until after the content has been written isn't necessarily going to work and any additional "post"-writing hooks we provide through this ticket isn't going to change that .

[spring-projects-issues]

Jon Brisbin commented

I had to do something like this for JSONP support. It basically had to always return a 200 no matter what happened in the @Controller so it could return JavaScript that could be eval'd in the browser to invoke an error handler. I did this by extending RequestMappingHandlerMapping and implementing my own invocation logic.

[spring-projects-issues]

Jason commented

I ended up using an Aspect on my controller methods to modify all my responses.

[spring-projects-issues]

Rossen Stoyanchev commented

Support for this has been added in the master branch.

See commit 96b18c that adds ResponseBodyInterceptor and also the next commit 51fc3b with a concrete JsonViewResponseBodyInterceptor implementation. And here is a test that is based on the example in the original description.

Update May 30, 2014: ResponseBodyInterceptor renamed to ResponseBodyAdvice.

[spring-projects-issues]

Rossen Stoyanchev commented

Marking as resolved for now but this is very much still open for further refinements based on feedback!

[spring-projects-issues]

Rossen Stoyanchev commented

I've also added a JSONP ResponseBodyInterceptor base class (see commit 1338d4). Also see updated test with JSONP advice.

[spring-projects-issues]

Ken Wang commented

Hey Rossen, first off awesome work on this piece! It's perfect for my needs of wrapping the return values from my Controllers.

However, one small thing preventing me from making proper use of it is that the Advice is not triggered when the return value is null. I opened an issue about this #16766. It would be great if you can take a look when you have time. Thanks.

[spring-projects-issues]

Marcel Overdijk commented

Nice to see this feature.
As the changed class is named "ResponseBodyAdvice" (Body) I wonder if this class should also be used to only add a Header to controllers returning ResponseEntity's?

[spring-projects-issues]

Rossen Stoyanchev commented

Thanks and thanks for suggesting the idea!

Yes you have a point about the name. It is meant as a reference to @ResponseBody-style methods where we use HttpMessageConverter's handle the controller's return value and for that ResponseEntity is another alternative (and a super set). So take the name for what it is, a compromise that's meant to be intuitive. The contract itself and the Javadoc should make it clear what you can do.