Spring new interface exposes inner response for testing

In my production setup, I use ServerResponse.async, and in tests, I need to access the wrapped CompletableFuture<ServerResponse>. right now the only way to access is through reflection, and that depends on the field name remaining the same in the future.

This pull requests introduces interface AsyncServerResponse with access to inner futureResponse. and the class previously called AsyncServerResponse, is now DefaultAsyncServerResponse.

This is the same as with EntityResponse. that exposes the entity, of a DefaultEntityResponse<T>.

I realize that a user of this interface can act upon the inner future response, but to get at it, the user will have to manually cast to AsyncServerResponse and by that, take responsibility for their actions.

Comment From: alexfeigin

sorry for the formatting changes, out of habit.. if one of the maintainers can reformat back the the spring standard that would be helpful @poutsma

Comment From: poutsma

I am reluctant to expose the completable future, because it requires us to keep using that type forever, while we want to be able to use a different asynchronous approach when necessary. In other words: the fact that the AsyncServerResponse uses a CompletableFuture is a current implementation detail of that class.

As you've noted, the EntityResponse does expose its entity, which can also be a CompletableFuture to have a asynchronous body, i.e. a EntityResponse<CompletableFuture<?>>. So perhaps you can get access to the future in your tests this way?

Comment From: alexfeigin

My production code returns ServerResponse objects.. and the implementation for some returns immediate responses: some entity, some default responses with redirects, and some times the responses are async using the ServerResponse.async

There is no way to cast a ServerResponse created with ServerResponse.async to EntityResponse.. unless we will provide a spring web util method to make the translation, as of course, AsyncServerResponse does not implement EntityResponse<CompletableFuture<ServerResponse>>...

I understand your concerns about the current and future implementations of AsyncServerResponse. Maybe we should instead allow for a generic AsyncServerResponse<T> where a user that is aware of the implementation (i.e the user that used ServerResponse.async with a CompletableFuture) can get the Object capture of ? and cast it back to the CompletableFuture<ServerResponse>

Comment From: alexfeigin

I had a new idea, with your concerns in mind, please take a look at the change @poutsma.

Instead of making the interface AsyncServerResponse expose the inside implementation details, I have exposed a concrete "toSync" blocking get method. it will return a finished ServerResponse, this is good for tests, as they will likely need to block to check the result either way.

Let's try to agree on a requirement for testing for results of ServerResponse.async.. I think that a user should be able to test the status code, headers, cookies, and if exists the body, of any response they created in production code paths. with the change I'm proposing, a user will be Able to do the following test code:

Object body = null;
if(response instanceof AsyncServerResponse){
    response = ((AsyncServerResponse)response).get(); // blocking or throwing the appropriate execution exception
}
if(response instanceof EntityResponse){
    body = ((EntityResponse)response).entity();
}
int statusCode = response.rawStatusCode();
HttpHeaders headers = response.headers();
MultiValueMap<String, Cookie> cookies = response.cookies();

AssertThat(anything).isEqualTo(expected);

without the change I'm proposing, a user will have access on a AsyncServerResponse instance only if the futureResponse has finished (which the user will only be able to poll with a loop catching the exceptions getting thrown..) and even then, if the response had a body, there is no access to that without either using reflection, or mocking and reading everything the method writeTo(HttpServletRequest request, HttpServletResponse response, Context context) wants.. Which I started to do, and it was such a pain, that I started this pull request instead.

Comment From: poutsma

I like the idea of the blocking get method, and will merge this PR for 5.3.2.

Comment From: poutsma

Manually merged, see https://github.com/spring-projects/spring-framework/commit/8f0ad73bfdb855467bf62f7c00e53868fe7b6e5a.

Thank you for providing a PR!