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

Null response status in WebFilter after controller method [SPR-17368] #21901

Closed
spring-issuemaster opened this Issue Oct 11, 2018 · 7 comments

Comments

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

spring-issuemaster commented Oct 11, 2018

Florian Beaufumé opened SPR-17368 and commented

I developed a simple org.springframework.web.server.WebFilter, in a reactive application based on Spring Boot 2.0.5, to log the HTTP response status (and other data not relevant here).

The filter method is basically:

return chain.filter(exchange).doAfterTerminate(
 () -> System.out.println("Status=" + exchange.getResponse().getStatusCode()));

For many REST controller methods the logged status is null.

From my StackOverflow post https://stackoverflow.com/questions/52668050/spring-webflux-statuscode-is-null-in-a-webfilter I discovered that +the status is null when the return type of the REST controller does not use ResponseEntity+, e.g.: void, Mono<Void>, String, Mono<String>, MyBean, Mono<MyBean>, etc.

+The status is correctly 200 when the return type uses ResponseEntity+: ReponseEntity<Void>, Mono<ResponseEntity<Void>>, ReponseEntity<String>, Mono<ResponseEntity<String>>, ReponseEntity<MyBean>, Mono<ResponseEntity<MyBean>>, etc.

I wrote a MWE, see https://github.com/fbeaufume/webflux-filter-sample

Looks like a bug to me. Is it? Any hope for a fix?

Regards, Florian


Affects: 5.0.9

Reference URL: https://stackoverflow.com/questions/52668050/spring-webflux-statuscode-is-null-in-a-webfilter

Referenced from: commits 75b1396

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 12, 2018

Rossen Stoyanchev commented

This is expected. The getStatus method is clearly marked as @Nullable and documented. A null return value means the status has not been explicitly set. Servers default to 200 (OK). 

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 12, 2018

Florian Beaufumé commented

Thank you Rosen for the feedback.

The status being @Nullable makes sense to me.

But it being null in a WebFilter after the delegation to the business method doesn't. To me it is counter-intuitive and error prone since the business method returned its result. Also note that the blocking equivalent correctly provides the response status in the filter after the business method delegation.

Any hope of the current behavior being changed?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 12, 2018

Rossen Stoyanchev commented

It's can be tricky to choose a place to set the status given that controller methods can return asynchronous values. In any case it would have to be before the return value is handled, or otherwise doing it after is too late. So we're effectively setting a default response status at a a somewhat arbitrary point in time, that is difficult to explain, and in the process creating an inconsistent way of checking for the status before vs after.

The current behavior is at least consistent in that at all times it indicates whether anything called response.setStatus or not. Essentially it's Optional.ofNullable(status).orElse(HttpStatus.OK). Question is how much value is there in knowing whether the status was explicitly set or not vs having to always check like this.

At best I can see setting the response status to HttpStatus.OK from the start and removing @Nullable from the method.

 

 

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 15, 2018

Florian Beaufumé commented

I also noticed that, in a WebFilter when I do not get the HTTP status, I nevertheless correctly get the response headers (such as "Content-Type" and "Content-Length").

Another argument in favor of 200 instead of null I guess.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 26, 2018

Rossen Stoyanchev commented

This has turned out to be a little more involved than expected, and I've also run into an unexpected issue. Now that 200 is set on the response by default, during WebSocket upgrades Tomcat and Jetty will keep the 200 instead of ignoring it and returning 101 as expected by the client. This also breaks a lot of tests that currently check for a null status. And on top of that we can't actually remove the @Nullable on getStatusCode() because of the potential for a custom integer status that is outside the range of HttpStatus enum values.

To address the issues I think we need to get rid of the statusCode field in AbstractServerHttpResponse, and instead propagate the status through to the underlying response. That way we are not forced to set a default value and on access we'll rely on whatever the default for the underlying server response is. This will also match to similar changes related to response headers, see #21783, where we now wrap the underlying native headers vs caching and applying at response commit time.

At this point however we're a little too close to 5.1.2 so I'm moving the fix version to 5.1.3.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 30, 2018

Florian Beaufumé commented

Thank you for the feedback.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 13, 2018

Rossen Stoyanchev commented

This should be fixed in master now. The logic is more or less the same, except each server specific implementation of ServerHttpResponse#getStatusCode() falls back on whatever the default status code value is for that server, e.g. 200 for Servlet containers. 

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.