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

WebFlux ServerResponse does not overwrite already present response headers #27741

Closed
rlndd opened this issue Nov 27, 2021 · 1 comment
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Milestone

Comments

@rlndd
Copy link

rlndd commented Nov 27, 2021

It seems like ResponseEntityResultHandler behaves differently compared to ServerResponseResultHandler regarding their handling of already present HTTP headers.

For example: Github

@SpringBootApplication
public class ResponseHandlerIssueApplication {

    public static void main(String[] args) {
        SpringApplication.run(ResponseHandlerIssueApplication.class, args);
    }

}

@Component
class DefaultNoCacheHeaderFilter implements WebFilter {
    @Override
    public Mono<Void> filter(ServerWebExchange exchange, WebFilterChain chain) {
        var headers = exchange.getResponse().getHeaders();
        if (headers.getCacheControl() == null) {
            headers.setCacheControl(CacheControl.noCache());
        }
        return chain.filter(exchange);
    }
}

@RestController
class TestController {
    @GetMapping("/responseEntity")
    public Mono<ResponseEntity<String>> cached() {
        var rsp = ResponseEntity
                .status(HttpStatus.OK)
                .cacheControl(CacheControl.maxAge(Duration.ofHours(1)))
                .body("supposed to be cached");
        return Mono.just(rsp);
    }
}

@Configuration
class RouterConfig {
    @Bean
    public RouterFunction<ServerResponse> routerFunction() {
        return route(GET("serverResponse"),
                req -> ServerResponse.ok()
                        .cacheControl(CacheControl.maxAge(Duration.ofHours(1)))
                        .body(BodyInserters.fromValue("supposed to be cached")
        ));
    }
}

Testing using curl:

curl -v 'http://localhost:8080/responseEntity'
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> GET /responseEntity HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.64.1
> Accept: */*
>
< HTTP/1.1 200 OK
< Cache-Control: max-age=3600
< Content-Type: text/plain;charset=UTF-8
< Content-Length: 21
<
* Connection #0 to host localhost left intact
supposed to be cached* Closing connection 0
curl -v 'http://localhost:8080/serverResponse'
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> GET /serverResponse HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.64.1
> Accept: */*
>
< HTTP/1.1 200 OK
< Cache-Control: no-cache
< Content-Type: text/plain;charset=UTF-8
< Content-Length: 21
<
* Connection #0 to host localhost left intact
supposed to be cached* Closing connection 0

As we can see the /responseEntity endpoint returns max-age=3600 for our Cache-Control header. However, /serverResponse returns no-cache.
ResponseEntityResultHandler seems to be overriding existing headers and DefaultServerResponseBuilder not to.

HttpHeaders entityHeaders = httpEntity.getHeaders();
HttpHeaders responseHeaders = exchange.getResponse().getHeaders();
if (!entityHeaders.isEmpty()) {
entityHeaders.entrySet().stream()
.forEach(entry -> responseHeaders.put(entry.getKey(), entry.getValue()));
}

private static <K,V> void copy(MultiValueMap<K,V> src, MultiValueMap<K,V> dst) {
if (!src.isEmpty()) {
src.entrySet().stream()
.filter(entry -> !dst.containsKey(entry.getKey()))
.forEach(entry -> dst.put(entry.getKey(), entry.getValue()));
}
}

The fix might be trivial, but maybe the intended behavior should be discussed.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 27, 2021
@jhoeller jhoeller added this to the Triage Queue milestone Nov 29, 2021
@jhoeller jhoeller added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Nov 29, 2021
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Nov 29, 2021

I think the headers from the handler should take precedence since it is the main and most specific request handling logic vs a filter which is applies to many requests. A handler can also check for the presence of existing header values if it really needs to, while in the opposite case it would have no way of overriding existing header values.

On the Spring MVC side, HttpEntityMethodProcessor and AbstractServerResponse also seem to write the headers from the handler.

@poutsma maybe this is something that needs to be adjusted in WebFlux.fn?

@rstoyanchev rstoyanchev changed the title ResponseEntityResultHandler and ServerResponseResultHandler have different behavior regarding already present headers ResponseEntityResultHandler and ServerResponseResultHandler behave differently with regards to already present response headers Nov 29, 2021
@poutsma poutsma modified the milestones: Triage Queue, 5.3.14 Nov 29, 2021
@poutsma poutsma removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 29, 2021
@poutsma poutsma changed the title ResponseEntityResultHandler and ServerResponseResultHandler behave differently with regards to already present response headers WebFlux ServerResponse does not overwrite already present response headers Nov 30, 2021
@poutsma poutsma added the type: bug A general bug label Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

5 participants